Run smart update in the package-reporter instead of having a cronjob

This branch needs the smart-update wrapper from smart. I've updated the smart packages to included it, so you can apt-get install it from my PPA:

deb http://ppa.launchpad.net/free.ekanayaka/ppa/ubuntu jaunty main

Just replace jaunty with hardy or intrepid if you use those platforms.

Nice branch!

[1]
+ run_command("smart-update --after %d" % self.smart_update_interval)

In our package, the command will probably be /usr/share/smart/smart-update. So maybe it would be nice to add /usr/share/smart to the PATH. That will be easier with my other suggestion: use twisted.internet.utils.getProcessOutput, which will give you a real Deferred for running a process.

[2] The smart update call in debian/landscape-client.init can be removed now.

[3] The cron file debian/landscape-client.cron.hourly can be removed as well.

Thanks!

Thanks Thomas!

[1]

Thanks for the hint. I've use "/usr/lib/smart/smart-update" as default
(being it a C executable, the package can't install it in /usr/share),
and modified the packages in my PPA repo accordingly.

[2], [3]

Yeah, I didn't address these because the debian/ directory seems to be
out of date (at least it is not in sync with the jaunty package), and
in general I think we should keep package-related data and code
outside of the trunk (that's the recommended approach). I'll talk with
Chris about this.

For now I removed both the cron job and the call to smart-update in
the landscape-client.init script.

For what it's worth, I don't think the 'smart-update' program should be installed by the smart packages. Since it must be set SUID, I think we should make it so that only the landscape user can execute it (by setting it as the owner and only allowing owner to execute). I guess that means either adding it to the landscape-client package or creating a new landscape-smart package.

When dealing with cron jobs in packages, be careful since it's not just about removing the file from debian/. Some postinst actions have to be taken too as far as I understand it. See bug #288205 and bug #281285.

Thanks Chris, that's a good point indeed.

So let's include the smart-update in the landscape-client package and
set its permissions to:

-rwsr-xr-- 1 root landscape 6152 2009-05-06 13:21 /usr/bin/smart-update

This way only user belonging to the landscape group (e.g. landscape)
will be able to run it. I'll get back to you on the channel for the
details.

[4] Some pyflakes, with a worrying one not new in this branch:

landscape/package/reporter.py:12: 'CommandError' imported but unused
landscape/package/reporter.py:12: 'run_command' imported but unused
landscape/package/reporter.py:98: undefined name 'hash_id_db'

I guess I need to review it once smart-update is in this branch.

Thanks Thomas.

|--==> On Tue, 12 May 2009 07:07:57 -0000, Thomas Herve <email address hidden> said:

[4]

If fixed the warnings. The worrying one was this:

- hash_id_db.fd.close()
+ hash_id_db_fd.close()

Now the test properly fails if you revert the change above. This was
bad bug, but luckily it won't affect the functionality, because the
exception it raised it's ignored.

I've also fixed a few failing tests due to python 2.6 and my weird
terminal (multi-term for emacs).

Okay, I've added the needed packaging modification to the branch and
tested the package on hardy (built with autoppa).

It worked fine, the only annoying thing is the reporter warning every
time about smart-update failing, because when smart is passed the
"--after 60" option it exits with status 1 if there's not yet need to
update.

This should probably be fixed in smart itself, by returning 0 in those
cases (it's not really a failure), or introducing some special exit
status code for that specific case.

I've also addressed Andreas' comment about removing the old cron
script, that is handle in the postinst.

 tag review

It looks good to me. There are some important packaging tricks, so I guess some testing is needed. Just 2 comments:

[1]
+ python_version = sys.version.split()[0].split(".")[0:2]
+ if map(int, python_version) < [2, 6]:

You can do that instead:

+ if sys.version_info[:2] < (2, 6):

[2]
+ char *smart_argv[] = {"/usr/bin/smart", "update", NULL, NULL};

The right path needs to be "/usr/share/smart/smart", as landscape-client depends only on python-smartpm which doesn't contain /usr/bin/smart.

Thanks!

Thanks!

[1]

Much nicer indeed :)

[2]

Alright, I'm going to update the smart package accordingly, as
discussed on the IRC channel.

> -rwsr-xr-- 1 root landscape 6152 2009-05-06 13:21 /usr/bin/smart-update

Is there something creating the "landscape" group? From what I remember and checked after installing landscape-client, there is no "landscape" group, only an user, and its primary group is *nogroup*.

Sorry, I see there is.

[1]

+def get_username():
+ try:
+ username = os.getlogin()
+ except OSError:
+ # Some xterminals do not register themselves with utmp, which
+ # this function uses. If so, fall back to the USER env variable.
+ username = os.getenv("USER")
+ return username
+
+
 def get_default_environment():
- username = os.getlogin()
+ username = get_username()

Instead of working around the bug in os.getlogin(), I think you should instead use pwd.getpwuid(os.getuid())[0] to get the current user's name.

[2] Indentation is off here:

+ # Add the setuid flag to smart-update and it be executable by users in
+ # the landscape group (that normally means landscape itself)
+ smart_update=/usr/lib/smart/smart-update
+ if ! dpkg-statoverride --list $smart_update >/dev/null 2>&1; then
+ dpkg-statoverride --update --add root landscape 4754 $smart_update
+ fi

[3] Why are we setting the real and effective uids and gids?

    setreuid(pwd->pw_uid, pwd->pw_uid);
    setregid(pwd->pw_gid, pwd->pw_gid);

Is it really necessary? I guess it's a protection in case the program being run does something with UIDs, and we want to make sure to give it something consistent? in that case, you may want to use setresuid and setresgid to also set the SAVED uid. :-)

I haven't yet done a very thorough test of this code, but these are the things I noticed with a quick view of the diff.

I'm getting a bunch of test errors with this branch that don't happen in trunk.

It all starts with:
[landscape/tests/test_configuration.py]
...............................................................................Error occurred contacting Landscape Client. Is it running?
Unhandled error in Deferred:
Traceback (most recent call last):
  File "/usr/lib/python2.5/site-packages/twisted/internet/defer.py", line 328, in _runCallbacks
    self.result = callback(self.result, *args, **kw)
  File "/usr/lib/python2.5/site-packages/twisted/internet/defer.py", line 518, in _cbDeferred
    self.errback(failure.Failure(FirstError(result, index)))
  File "/usr/lib/python2.5/site-packages/twisted/internet/defer.py", line 269, in errback
    self._startRunCallbacks(fail)
  File "/usr/lib/python2.5/site-packages/twisted/internet/defer.py", line 312, in _startRunCallbacks
    self._runCallbacks()
--- <exception caught here> ---
  File "/usr/lib/python2.5/site-packages/twisted/internet/defer.py", line 328, in _runCallbacks
    self.result = callback(self.result, *args, **kw)
  File "/home/andreas/canonical/landscape-client/free/run-smart-update-in-package-reporter/landscape/configuration.py", line 455, in catch_all
    reactor.callLater(0, reactor.stop)
  File "/home/andreas/canonical/landscape-client/free/run-smart-update-in-package-reporter/landscape/tests/mocker.py", line 1055, in __getattribute__
    return self.__mocker_act__("getattr", (name,))
  File "/home/andreas/canonical/landscape-client/free/run-smart-update-in-package-reporter/landscape/tests/mocker.py", line 1046, in __mocker_act__
    raise AssertionError(os.linesep.join(message))
exceptions.AssertionError: [Mocker] Unmet expectation:

=> twisted.internet.reactor.stop
 - Should be after: print_text_mock('Invalid account name or registration password.', error=True)

Eerror: test_register_failure [
Traceback (most recent call last):
  File "/usr/lib/python2.5/site-packages/twisted/internet/defer.py", line 328, in _runCallbacks
    self.result = callback(self.result, *args, **kw)
  File "/home/andreas/canonical/landscape-client/free/run-smart-update-in-package-reporter/landscape/configuration.py", line 455, in catch_all
    reactor.callLater(0, reactor.stop)
  File "/home/andreas/canonical/landscape-client/free/run-smart-update-in-package-reporter/landscape/tests/mocker.py", line 1055, in __getattribute__
    return self.__mocker_act__("getattr", (name,))
  File "/home/andreas/canonical/landscape-client/free/run-smart-update-in-package-reporter/landscape/tests/mocker.py", line 1046, in __mocker_act__
    raise AssertionError(os.linesep.join(message))
AssertionError: [Mocker] Unmet expectation:

=> twisted.internet.reactor.stop
 - Should be after: print_text_mock('Invalid account name or registration password.', error=True)

]
Error occurred contacting Landscape Client. Is it running?
(...)

There are several pages after this.

This is the end:
(...)
[landscape/user/tests/test_management.py]
...........................................
----------------------------------------------------------------------
Ran 43 tests in 0.084s

OK
(tests=43, failures=0, errors=0)

Total test cases: 1599
Total d...

Thanks for the review, Chris!

[1]

Nice, thanks for hint.

[2]

Fixed.

[3]

  > setreuid(pwd->pw_uid, pwd->pw_uid);
  > setregid(pwd->pw_gid, pwd->pw_gid);
  > Is it really necessary? I guess it's a protection in case the program

AFAIK, it is necessary to actually set the effective uid to 0. OTOH I
don't think setresuid is necessary, because the purpose of the setuid
permission flag is indeed to set the saved uid of the process to the
owner of the file once the OS executes it.

I might be off here but I see one thing that seem just odd:

[1] In smart-update/Makefile, the binary is installed as /usr/bin/smart-update
    but in debian/landscape-client.postinst, and
    landscape/package/reporter.py, the path used is
    /usr/lib/smart/smart-update. I then noticed that debian/rules
    installs the binary to that path, so this should be fine given my
    little understanding here.

    I suspect a little comment could be in order here, but that may be
    just me.

All in all, +1

Tested on dapper against the 1.3.2 packages from the Landscape PPA, qa +1.