MESSAGE quota resource implemention

[Greg Banks]

Julien Coloos wrote:
> Hi,
>
> As discussed on IRC last week, I worked on implementing MESSAGE quota 
> resource in cyrus (see RFC 2087; STORAGE resource already being handled).
> I created a branch based on Greg's 'annotate' one on github, since his 
> work on annotation storage management made mine a lot easier.
>
> Details on the changes I made:
>
> cyrus-imapd:
>   - added MESSAGE quota resource management
>     -> updated cunit test
>     -> added 'quotawarnmsg' option which behaviour is similar to 
> 'quotawarnkb'; warning message shown when selecting folder in IMAP 
> tells which resource limit was triggered
>     -> added 'autocreatequotamsg' option, to set MESSAGE limit 
> (unlimited by default) when user auto-creates its mailbox
>   - xfer transfers all non-unlimited resources
>   - added helper function to compute annotation storage usage for a 
> given mailbox
>   - changed the way quota entries are read/written
>     -> resources presence in fetched entry is remembered
>     -> when writing entry, only present resources are written
>     -> when setting resources limits, entries which were not present 
> upon fetching are now marked as present and their current usage is 
> computed
>   - quota utility lists and computes (-f) all resources associated to 
> mailbox
> The branch 'quotamessage/gnb/annotate' is available here: 
> git://github.com/worldline-messaging/cyrus-imapd.git. It is based on 
> Greg's 'annotate' branch on github.
>
> cassandane:
>   - added tests for MESSAGE resource
>   - modified current tests to play a bit with subfolders and setting 
> quota after adding messages/annotations
>   - special test to check new resource usage is computed when 
> necessary for pre-existing mailboxes (which only have usage for 
> STORAGE resource)
> The branch 'quotamessage/gnb/master' is available here: 
> git://github.com/worldline-messaging/cassandane.git. It is based on 
> Greg's 'master' branch on github.
>

That all sounds promising and I look forward to reading it.
>
> Things that may be worth noting:
>   - DUMP/UNDUMP currently does nothing special about MESSAGE or 
> X-ANNOTATION-STORAGE quota resources
>     -> should it be transferred ?

Yes.  I've been lazy updating the dump/undump code.
>     -> without breaking backward compatibility, limits could only be 
> transferred through a 'fake' file entry, as for annotations
>   - quota usage is currently stored in a uquota_t variable, and delta 
> is computed as quota_t; so theorically there could be overflow issues 
> if quota usage to add/substract cannot be held in a quota_t; in 
> practice it should be unlikely since that would mean a usage of over 
> 2^63-1
That seems...unlikely.

>
>
> Why the change concerning quota entries read/write ?
> The thing is that I wanted to make version upgrading as painless as 
> possible, both for users and in the code.
> With previous code, when quota entry exists and is read, missing 
> resources are initialised with default values (0 usage, unlimited). 
> Thus only usage delta is tracked, and actual usage computing would 
> only happen if quota entry was missing: this is not nice when 
> upgrading, since lots of mailboxes are likely to have an entry with 
> only the STORAGE resource present.
> So actual usage has to be computed at some point for newly handled 
> resources. The idea here is to compute it when setting the resource 
> for the first time. To do that it was necessary to know when the 
> resource was not previously present and keep it that way until its 
> limit is finally set for the first time.
> This scheme has another advantage: for platforms where only STORAGE 
> quota is used, quota entries size remains as it is now. Only people 
> using those new quota resources will have their quota entries grow to 
> store the new data.
>
> Comments are welcomed :)

I'll look at the code before commenting.
>
>
> I think that there are two things that may also be done concerning 
> quota entries:
>   - always recompute resource usage when changing its limit from 
> unlimited to bounded
>     -> currently it is only done once when setting the usage limit for 
> the first time
>     -> that way, it may not be necessary to track resource presence 
> when reading/writing quota entries
>     -> but maybe it could be too time consuming in some cases, since 
> it seems to be possible to associate a quota resource to a whole 
> domain (recomputing usage for all mailboxes would then take some time)
>   - do not write resource in quota entry when its usage is unlimited
>     -> except for the STORAGE resource, for backward compatibility
>     -> would also help keeping quota entries size to the bare minimum
>
> What do you think ?

This one's tough, I wasn't sure what to do.  However, I'm happy to leave 
it to the administrator to have to manually run quota -f (maybe twice!) 
if they set a quota on a resource that is already being used.  I'm 
unconvinced that automatically doing the equivalent of -f as a side 
effect of setting the first limit is necessary or wise.  Perhaps we 
should a) document it clearly and b) detect the situation and put an 
obvious message saying something like "you may need to run quota -f ..." 
where the human making the change will see it.  Also, such a message 
might be useful when usage underflow is detected.
>
>
> PS: should I open a new bugzilla ticket for that ?
>
> PPS:
> cunit: on my computer cunit tests succeed except the 'foreach' one in 
> the 'quota' suite which timeouts (it seems messing with 4096 
> quotalegacy is too much for my computer).
Yeah, I hit a similar problem myself the other day, and solved it by 
reducing the scale of the test so it completed in < 10 sec.  
Alternatively you could bump up TEST_TIMEOUT_MS in cunit/unit.c.

> cassandane: a few quota tests I added fail due to some of the issues I 
> reported (#5327 and #5329). And poor lemmings (well thought out name 
> in this context :)) are dying hopelessly and endlessly in the 
> Cassandane::Cyrus::Master test, preventing it to complete. Don't know 
> if it is normal.
Sorry, that's expected.  The Master tests are exploring the master 
process' resilience to certain misbehaviours in the services it starts, 
and many of them will fail with the current master code.  The fixes for 
master and some cleanups are in 
https://github.com/gnb/cyrus-imapd/commits/iris405 and will get merged 
into master at some time in the future when I get around to it. In the 
meantime I use the '!Master' argument to testrunner.pl to skip those tests.

-- 
Greg.