Client captures stderr from run_command when constructing hash-id-databases url

Bug #397480 reported by Andreas Hasenack on 2009-07-09
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
Landscape Client
High
Free Ekanayaka

Bug Description

In packate/taskhandler.py:104
            try:
                # XXX replace these with L{SmartFacade} methods
                codename = run_command("lsb_release -cs")
                arch = run_command("dpkg --print-architecture")
            except CommandError, error:
                logging.warning(warning % str(error))
                return None

On karmic:
root@ubuntu:/usr/lib/python2.6/dist-packages/landscape# sudo -u landscape dpkg --print-architecture
dpkg: warning: failed to open configuration file '/root/.dpkg.cfg' for reading: Permission denied
i386
root@ubuntu:/usr/lib/python2.6/dist-packages/landscape# sudo -u landscape dpkg --print-architecture 2>/dev/null
i386
root@ubuntu:/usr/lib/python2.6/dist-packages/landscape#

So what happens is that "arch" gets both the dpkg warning and the i386 result, and this messes up the hash-id-databases url.

Changed in landscape-client:
importance: Undecided → High
milestone: none → 1.3.2.2
Andreas Hasenack (ahasenack) wrote :

This is the OS environment when that dpkg command is run:
{'LANG': 'en_US.UTF-8', 'TERM': 'xterm', 'SHELL': '/bin/bash', 'LESSCLOSE': '/usr/bin/lesspipe %s %s', 'XDG_SESSION_COOKIE': 'efe2d5b46d94f29e14a546ac4a55faa5-1247149430.519881-35331106', 'SHLVL': '1', 'SSH_TTY': '/dev/pts/0', 'OLDPWD': '/var/lib/landscape/client', 'PWD': '/usr/lib/python2.6/dist-packages/landscape', 'LESSOPEN': '| /usr/bin/lesspipe %s', 'SSH_CLIENT': '192.168.122.1 51690 22', 'LOGNAME': 'root', 'USER': 'root', 'PATH': '/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games', 'MAIL': '/var/mail/root', 'LS_COLORS': 'rs=0:di=01;34:ln=01;36:hl=44;37:pi=40;33:so=01;35:do=01;35:bd=40;33;01:cd=40;33;01:or=40;31;01:su=37;41:sg=30;43:ca=30;41:tw=30;42:ow=34;42:st=37;44:ex=01;32:*.tar=01;31:*.tgz=01;31:*.arj=01;31:*.taz=01;31:*.lzh=01;31:*.lzma=01;31:*.zip=01;31:*.z=01;31:*.Z=01;31:*.dz=01;31:*.gz=01;31:*.bz2=01;31:*.bz=01;31:*.tbz2=01;31:*.tz=01;31:*.deb=01;31:*.rpm=01;31:*.jar=01;31:*.rar=01;31:*.ace=01;31:*.zoo=01;31:*.cpio=01;31:*.7z=01;31:*.rz=01;31:*.jpg=01;35:*.jpeg=01;35:*.gif=01;35:*.bmp=01;35:*.pbm=01;35:*.pgm=01;35:*.ppm=01;35:*.tga=01;35:*.xbm=01;35:*.xpm=01;35:*.tif=01;35:*.tiff=01;35:*.png=01;35:*.svg=01;35:*.svgz=01;35:*.mng=01;35:*.pcx=01;35:*.mov=01;35:*.mpg=01;35:*.mpeg=01;35:*.m2v=01;35:*.mkv=01;35:*.ogm=01;35:*.mp4=01;35:*.m4v=01;35:*.mp4v=01;35:*.vob=01;35:*.qt=01;35:*.nuv=01;35:*.wmv=01;35:*.asf=01;35:*.rm=01;35:*.rmvb=01;35:*.flc=01;35:*.avi=01;35:*.fli=01;35:*.flv=01;35:*.gl=01;35:*.dl=01;35:*.xcf=01;35:*.xwd=01;35:*.yuv=01;35:*.axv=01;35:*.anx=01;35:*.ogv=01;35:*.ogx=01;35:*.aac=00;36:*.au=00;36:*.flac=00;36:*.mid=00;36:*.midi=00;36:*.mka=00;36:*.mp3=00;36:*.mpc=00;36:*.ogg=00;36:*.ra=00;36:*.wav=00;36:*.axa=00;36:*.oga=00;36:*.spx=00;36:*.xspf=00;36:', 'HOME': '/root', '_': '/usr/bin/landscape-client', 'SSH_CONNECTION': '192.168.122.1 51690 192.168.122.166 22'}

Notice how HOME and USER, among others, still point to the root user and his home.

Changed in landscape-client:
assignee: nobody → Free Ekanayaka (free.ekanayaka)
status: New → In Progress
tags: added: review
Free Ekanayaka (free.ekanayaka) wrote :

Test packages are available in my PPA as usual.

Andreas Hasenack (ahasenack) wrote :

The fix reports "amd64" correctly when that's the case (tested with an EC2 large instance) and the hash-id-database is correctly downloaded.

qa + 1

Andreas Hasenack (ahasenack) wrote :

BTW, tests fail with this branch if the landscape user does not exist in the system.

Free Ekanayaka (free.ekanayaka) wrote :

Thanks Andreas, I've fixed the issue of tests failing if the landscape user does not exit.

Christopher Armstrong (radix) wrote :

[1]
+ arch = self._facade.get_arch()
+ if not isinstance(arch, str):
+ logging.warning(warning % "unknown dpkg architecture")
+ return None

This strikes me as a strange check and warning. Do you know in which cases the arch is not a str? does get_arch return None sometimes? Is it ever something else? Maybe the check could be something more explicit.

[2]
in watchdog.py

- We do this to avoid any problems when landscape-client is invoked from its
+ In particular unsert all variables beginning with DEBIAN_ or DEBCONF_,

"unsert" should be "unset"

[3] I don't see tests for the new code that sets HOME, USER, and LOGNAME in the environment in the watchdog.

Christopher Armstrong (radix) wrote :

btw, I was wrong about [3], I was just confused by mocker :)

Kevin McDermott (bigkevmcd) wrote :

With Chris' responses above addressed (I checked r135)

I do wonder how get_arch() can return invalid responses?

#1

Can you perhaps add a comment explaining under what circumstances you expect an invalid response, in case someone's actually having to debug something similar later?

Should we log this case?

Looks good to me, with these issues addressed... +1

Thanks Chris!

[1]

After some IRC discussion and double check of smart.backends.deb.base.getArchitecture,
I changed the check to "if arch is None", which anyway should never be executed, as
getArchitecture always returns a string.

[2]

Fixed.

[3]

The test was actually there, but buried in mocker expectation, we
cleared this out on IRC.

Thanks a lot Kevin!

#1

The rational is that the check isn't really needed at the moment,
because Smart will always return a string. However I thought to
include it as well, as a sanity/paranoia check to protect us from
possible future bugs in smart.

So at the moment there aren't any circumstances I would expect an
invalid response.

tags: removed: review
Changed in landscape-client:
status: In Progress → Fix Committed
Changed in landscape-client:
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers