MESSAGE quota resource implemention

[Greg Banks]

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.