Detect and report changes about Smart package locks

[Free Ekanayaka]

The Landscape client should query Smart about the package locks
currently sent on the system, and report all possible changes to the
server.

Full information about locking should be reported to the server, this includes:

1) A new "package-locks" message, with "set" and "unset" fields, to report about newly set/unset package locks. For Smart a package lock is basically an expression matching zero or more packages, any matched package will be locked and Smart will never change its status, for example if it's installed it won't be removed or upgraded, if it's not installed it won't be installed. The package lock expression is defined by a name and optionally a relation/version condition, for example the package lock "foo < 1.0" matches all packages named "foo" with version lower than 1.0.

2) Two new "locked" and "not-locked" fields in the "packages" message, to report changes about the locking status of packages, as consequence of newly created/removed package locks. For example if the package foo-0.9 has id 1, then adding the smart lock "foo < 1.0" will eventually result in a "packages" message with a "locked" field listing package id 1.

[Thomas Herve (therve)]

Nice branch! Just one comment:

[1] The sequence_to_ranges calls are not tested (although I'm not sure how useful they are).

+1!

[Free Ekanayaka (free.ekanayaka)]

Thanks, Thomas!

[1]

I've added two more tests for them. Having sequences of consecutive ids is probably rare in this case, but I guess it doesn't hurt to have it.

[Jamu Kakar (jkakar)]

[1]

+ @param relation, version: The condition the package version must
+ satisfy in order to be locked.

The Epytext syntax used here is incorrect, I believe.

[2]

+ def set_package_lock(self, name, relation="", version=""):

and then:

+ # Smart wants None here
+ if relation == "":
+ relation = None
+ if version == "":
+ version = None

I think you should use None as the default values. It will allow
you to remove the conversion code you have and shouldn't have any
impact on the behaviour of _validate_lock_condition.

[3]

+ def remove_package_lock(self, name, relation="", version=""):

and then:

+ # Smart wants () here
+ if not relation:
+ relation = ()
+ if not version:
+ version = ()

I recommend you use () as the default values for relation and
version here. It will allow you to remove the conversion code
without impacting the other surrounding code.

[4]

+ if set_package_locks:
+ message["set"] = sorted(set_package_locks)
+ if unset_package_locks:
+ message["unset"] = sorted(unset_package_locks)

Would you mind calling these keys 'lock' and 'unlock'. I realize
that's pretty nit-picky, but it feels a bit more clear to me.

[5]

+ If Smart indicates us locked packages we didn't know about, report

s/us// please.

[6]

+ Locked packages whose hashes are unkwnown don't get reported.

s/unkwnown/unknown/ please.

[7]

+ cursor.execute("CREATE TABLE package_locks"
+ " (name TEXT NOT NULL, relation TEXT, version TEXT,"
+ " UNIQUE(name, relation, version))")
+ cursor.execute("CREATE TABLE locked"
+ " (id INTEGER PRIMARY KEY)")

You need to add a patch to correctly upgrade existing package store
databases.

[Free Ekanayaka (free.ekanayaka)]

Thanks for reviewing, Jamu!

[1], [5], [6]

Fixed

[2], [3]

Yeah, I had noticed that as well, but I basically wanted to keep the default values of these "set" methods consistent with the values returned by the "get" ones. I agree it introduces unnecessary complexity though, so I changed the code as you suggest.

[4]

What about "added" and "removed" instead? The fact is that "lock" and "unlock" are actually further consequences the package locks themselves have on the packages, at this point we're really just reporting about newly "added" or "removed" package locks.

Another even better option for me would be "set" and "removed", which matches exactly the smart command line terminology, where you type things like:

smart flag --set lock "foo > 1.0"

and

smart flag --remove lock "foo > 1.0"

[7]

Absolutely a patch system would be better.

However we don't have any in place yet, and the code as it is now would actually succeed in upgrading an existent database, because the two CREATE TABLE statements are performed first, and they will succeed as the two tables don't exist yet. I guess a proper patch system would probably need a separate branch, I would personally postpone it till when it's actually necessary, I don't feel strong on this though.

[Jamu Kakar (jkakar)]

Free:

[4]

I'd be happy with "create" and "delete", and in fact, that would
match the hardware inventory plugin. Anyway, please do what you
feel is best, it was really just a minor suggestion.

[7]

I guess running queries that fail doesn't raise OperationalError and
so the transaction ends up getting committed...? Have you verified
that this code works with existing installations?

[Free Ekanayaka (free.ekanayaka)]

Jamu:

[4]

I went for "created" and "deleted", using the past participle because it refers to something that is indeed already happened (like the "installed" field of the packages message typ
[7]

Yes, I tested this in real-world and it works. The OperationalError exception is raised, and the transaction is rolled back, however the rollback preserves tables created during the transaction, weirdly enough it must be a Sqlite "feature" :) I've added an explicit unit test for this, so we can catch possible changes of this behavior across different Ubuntu releases (thanks buildbot!)

[Free Ekanayaka (free.ekanayaka)]

Got a verbal +1 from Jamu during a Skype call.

[Andreas Hasenack (ahasenack)]

Confirmed in staging.

[Andreas Hasenack (ahasenack)]

This was released already for the mentioned distros bar Intrepid, which is EOL and I didn't check.