On 15/09/11 03:14, Julien Coloos wrote: > Le 12/09/2011 23:09, Bron Gondwana a écrit : >> >>> - for 'old' mailboxes (those created before the annotation storage >>> usage field in the index header), current annotations usage shall be >>> computed (and added to the quota entry) upon upgrading; this way >>> users won't have to run 'quota -f' for all quotaroots after >>> switching to this new version ;) >> Definitely. Upgrading usually handles things like that. It's >> the right way[tm]. >> > Pushed a few lines of code in the 'upgrade_index' function. Though it > still lacks the annotation storage usage computing. > It's in the branch 'gnb/annotate/fixes'; commit > ed61f721487804b205e399c538fc35ddc153bd93.
Thanks Julien, I've cherry-picked this commit and the "Fixes" commit from the other day into my annotate branch. BTW I'm working on making reconstruct calculate the X-ANNOTATION-STORAGE quota. Some of that code may also be useful during upgrade and for handling message expunging. > >>> Actually I just gave up the 'old' test: there is no easy way to >>> simulate upgrading mailbox index, or at least I don't feel confident >>> enough to make it in cassandane :( No, there isn't a really good way at the moment. >> Easiest way is the same way I did for the broken quotas test. >> Have a tar file with the contents of the "old mailbox" which >> you unpack onto the filesystem, and then open the mailbox and >> check that the "upgraded" fields are what you would expect. >> >> I also did something similar for the "crash on thread" test, >> where there were 5 messages which were known to be able to >> crash the THREAD command. I unpacked the folder contents >> with those messages in a test. > Nice idea :) Bron's method of saving a tarball of the state of a mailbox generated by an older release is reasonably straightforward. The difficulty however is going to be working out whether the result of the upgrade is correct. In the general case, you can't just test that the first open of the mailbox doesn't report an error from upgrading, you need to test at least: * the mailbox is re-written with the latest version number * all the "new" fields have correct or at least feasible values * the messages previously present are still present and in the same order with the same contents and the same metadata (uid, flags, internaldate, etc) To do this properly you really need to save, independently of the Cyrus mailbox store, a whole lot of information about the expected results of the mailbox open so that Cassandane can check it. The two other alternatives are both worse: a) don't check and b) hardcode checks into the Cassandane code which depend intimately on details of the data in the mailbox state tarball. I'm thinking that what we need is a standalone utility built from the Cassandane source tree. Someone would point this utility at a live working Cyrus instance (not one controlled by Cassandane), and it would create a new folder and fill the folder with known data, then harvest the on-disk state left by Cyrus and packages that state up into a tarball along with a separate description of the messages. That tarball could then be checked into the Cassandane tree and used to run upgrade tests with reasonable post-upgrade checks. The advantages I see for this approach are: * when (not 'if') we need to improve either the data generated or the checks run, we can improve this utility and then re-capture new state as time permits. * the versions of Cyrus tested can be on separate machines from Cassandane itself - which allows testing upgrades from really old versions which don't install or run on modern OSes due to library issues, and - allows testing upgrades from Cyrus on other platforms (e.g. platforms with different endianness or wordsize). BTW at some point in the future I want Cassandane to be able to handle multiple installed versions of Cyrus, so that we can test configurations like cross-version replication. But that's not really the same problem as upgrade testing. For upgrade testing, we don't need functioning code for the old Cyrus versions, just a snapshot of their spoor. For cross-version replication or murder testing we need live installs to poke. The difference is critical, because I figure we'll need to support upgrade from much older versions than we support in a cross-version murder, and those older versions will be quite challenging to install or get running on the same platform that we develop on. > Pushed a few things. Thanks. I pulled in your commit "Added quota MESSAGE resource (RFC 2087) test cases." yesterday and then refactored the new tests a bit, but didn't get a chance to push it out again until today. > Added an option hash so that when running commands: > - test can run system commands (based on the current code which would > run cyrus commands inside the current cassandane instance directory) > - finer I/O redirections are possible > - working directory can be specified > While adding those new 'features', I still kept the 'run_utility' method > (cyrus commands) and simply added the 'run_command' method for other > commands. commit "Added possibility to run system commands..." I like the idea of an options hash, it allows us to clean up some of the horrible hacks I did. However: - if run_utility() is going to take an options hash then start_utility_bg() should too. - you should make the "gdb" feature be a special option, not a special value for $options->{mode}. Actually I wouldn't mind if you just broke it, there's no code that depends on it and it's a pain to use. - you don't need both $options->{mode} and $options->{redirects}. The single callsite that uses a non-default mode could be easily adjusted to use whatever more general redirect mechanism you come up with. Let's lose the mode entirely and reduce the complexity of _fork_whatever(). @@ -76,6 +76,7 @@ sub new my $self = { name => undef, basedir => undef, + workingdir => getcwd(), installation => 'default', cyrus_prefix => undef, config => Cassandane::Config->default()->clone(), This hunk appears to serve no purpose. The entirety of the Cassandane code expects to run in the same working directory, which is the root of the cassandane tree, and nothing should be fiddling with that. Also, in terms of terminology, I note that run_utility() calls _fork_command() and run_command() also calls _fork_command(). So in some places "command" means "a program that is NOT Cyrus code" and in other places the same word means "a program which might or might not be Cyrus code" which is confusing. > > Then, as you suggested on IRC, I added an 'unpack' method to extract > tar/gz/bz2 (and combinations) files using system commands. commit "Added test instance method to unpack tar/gz/bz2 (and combinations) files." Having an "unpack" method is certainly useful. However: * given that you're using GNU-tar specific flags like -C -j and -z, you may well take advantage of modern GNU tar's ability to work out whether it needs -j or -z itself when used with -x * the method should be in Cassandane::Instance rather than Cassandane::Cyrus::TestCase, so you can use it to unpack data into any of the instances in a multiple-instance test (e.g. a replication test) * it might be a good idea to have a default value for the $dst argument which points to $instance->{basedir}, or even allow $dst to be a relative pathname from $instance->{basedir}. If $dst were the last argument you could make it entirely optional. > > And I added back my quota upgrade test from a cyrus v2.4(.11) mailbox, > using two tar.gz files containing a mailbox content and its quota file. commit "Added cyrus quota test when upgrading from 2.4 mailbox." Ok, this is good for testing the narrow case of upgrade and annotation quota, but * I don't see why you'd want to use "user.cyrusv24" as the test data, when "user.cassandane" is already set up for you by the Cassandane infrastructure? * you don't need "$self->{instance}{workingdir}/data/cyrus/quota_upgrade_v2_4.user.tar.gz", you can achieve the same effect with abs_path("data/cyrus/quota_upgrade_v2_4.user.tar.gz") > > It's in the 'quotamessage/gnb/annotate' branch; commits > d2bf4e4f42f7f8c9b53713441116ddbea5b0a265, > 86a52daeed5a03f078b88de67d3d10b51a7f8cc4 and > fb827e8fc77529a5e23465d2c19d6e88adf7cae8. > > > Maybe you won't keep everything I pushed, but I hope some parts will be > helpful :) It definitely is helpful. Can you please address the issues I mention above and rebase (my commit "fork_utility() defines some environment vars" clashes with your "Added possibility to run system commands...") and then I'll be happy to merge this into my annotate branch. -- Greg.