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

Bug #397480 reported by Andreas Hasenack
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
Landscape Client
Fix Released
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
Revision history for this message
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
Revision history for this message
Free Ekanayaka (free.ekanayaka) wrote :

Test packages are available in my PPA as usual.

Revision history for this message
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

Revision history for this message
Andreas Hasenack (ahasenack) wrote :

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

Revision history for this message
Free Ekanayaka (free.ekanayaka) wrote :

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

Revision history for this message
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.

Revision history for this message
Christopher Armstrong (radix) wrote :

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

Revision history for this message
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

Revision history for this message
Free Ekanayaka (free.ekanayaka) wrote : Re: [Bug 397480] Re: Client captures stderr from run_command when constructing hash-id-databases url

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.

Revision history for this message
Free Ekanayaka (free.ekanayaka) wrote :

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  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.