Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

servers not running backupd should not complain about missing backups.db #6

Closed
elliefm opened this issue Jul 18, 2016 · 8 comments
Closed
Assignees
Labels
3.0 affects 3.0

Comments

@elliefm
Copy link
Contributor

elliefm commented Jul 18, 2016

When compiled with --enable-backups, backups.db becomes one of the databases that ctl_cyrusdb examines on recover, checkpoint, etc.

If it doesn't exist -- which you would expect if the server isn't being used as a backup server -- then it whinges thus:

Jul 19 08:40:43 debian-testing frodo/ctl_cyrusdb[2074]: archiving database file: /cyrus/slot/frodo/var/imap/backups.db
Jul 19 08:40:43 debian-testing frodo/ctl_cyrusdb[2074]: IOERROR: opening /cyrus/slot/frodo/var/imap/backups.db: No such file or directory
Jul 19 08:40:43 debian-testing frodo/ctl_cyrusdb[2074]: DBERROR: error archiving database file: /cyrus/slot/frodo/var/imap/backups.db
Jul 19 08:40:43 debian-testing frodo/ctl_cyrusdb[2074]: DBERROR: archive /cyrus/slot/frodo/var/imap/db: cyrusdb error

It's complaining because it's trying to perform an archive operation on the missing database file, which it's doing because I set its doarchive to 1 in dblist. I'm not sure if this is needed for this database or not. The database is a mapping from userid => backup location. It can be recreated easily enough, though there's not currently a tool for it.

The checkpoint and recover operations don't care that it's not there, it's just the archiving stage that's bothered.

Workaround: just create an empty database where it expects it to be

Possible solutions:

  • disable doarchive for backups.db
  • make archive ignore missing files, like checkpoint and recover do
  • set doarchive, but conditionally based on whether backup configuration exists

Input appreciated!

@elliefm elliefm self-assigned this Jul 18, 2016
@elliefm
Copy link
Contributor Author

elliefm commented Jul 18, 2016

I did think that --enable-backup was required for imap (i.e. not backup) servers that needed to be restored to, but now I'm not sure. With a quick look, I don't see compilation guards around the "am able to accept restore attempts" code. (But maybe there should be.)

So another workaround might be to only compile with --enable-backup for servers that expect to run backupd (and thus must have a backups.db, in which case an error when it is missing is correct). But having two separate builds is a hassle so I don't consider that to be a solution.

@elliefm
Copy link
Contributor Author

elliefm commented Jul 18, 2016

For reference:

static struct cyrusdb {
    const char *name;
    const char **configptr;
    cyrusdb_archiver *archiver;
    int doarchive;
} dblist[] = {
    { FNAME_MBOXLIST,           &config_mboxlist_db,    NULL,   1 },
    { FNAME_QUOTADB,            &config_quota_db,       NULL,   1 },
    { FNAME_GLOBALANNOTATIONS,  &config_annotation_db,  NULL,   1 },
#ifdef ENABLE_BACKUP
    { FNAME_BACKUPDB,           &config_backup_db,      NULL,   1 },
#endif
    { FNAME_DELIVERDB,          &config_duplicate_db,   NULL,   0 },
    { FNAME_TLSSESSIONS,        &config_tls_sessions_db,NULL,   0 },
    { FNAME_PTSDB,              &config_ptscache_db,    NULL,   0 },
    { FNAME_STATUSCACHEDB,      &config_statuscache_db, NULL,   0 },
    { NULL,                     NULL,                   NULL,   0 }
};

The tl;dr here is I'm not sure what the criteria are for some of those databases to be doarchive, and some not

@brong
Copy link
Member

brong commented Jul 18, 2016

I'd make doarchive handle a file not existing I think - without making a big fuss. It probably should be archived, because it's stuff that needs to live for some time.

Those 4 without doarchive are databases that we store on tmpfs at FastMail and discard every reboot - they're really not important at all!

@elliefm
Copy link
Contributor Author

elliefm commented Jul 19, 2016

https://github.com/elliefm/cyrus-imapd/compare/v30/6-ctl_cyrusdb-skip-archive-of-missing seems like the minimum change to shush these errors -- though note that it will also no longer error if any of the other databases are missing, either

I'm a bit worried about silently ignoring cases where a missing database is actually a major problem, though. There's a big difference between "we haven't needed it yet" and "wtf it was there a minute ago"...

What's the usual/desired behaviour model for these databases? Do the parts that use them autovivify them if they're missing? If so, do they report when they do that? Or do we consider the initial creation of these database files an administrator installation step, and thereafter a missing database is cause for concern?

@brong
Copy link
Member

brong commented Jul 19, 2016

I'd prefer if we could suppress it down a layer in ->archive rather than doing a stat on the filename. There's no requirement that the filename match a real file on disk for the database to "exist" anywhere else.

@brong
Copy link
Member

brong commented Jul 19, 2016

I'm pretty sure most of them are opened with CYRUSDB_CREATE, so they autovivify

@elliefm
Copy link
Contributor Author

elliefm commented Jul 19, 2016

In that case, I'm not going to worry too much about detecting disappearing databases -- that's either not a problem in practice, or is beyond the scope of what we're looking at right now.

I checked and all of database types use either cyrusdb_generic_archive (which essentially calls cyrus_copyfile) or cyrusdb_generic_noarchive (which does nothing). So I've moved the stat into cyrusdb_generic_archive, and added a gentle debug log so if something more interesting than "file not found" occurs, there's at least some record of it.

Same link as before:
https://github.com/elliefm/cyrus-imapd/compare/v30/6-ctl_cyrusdb-skip-archive-of-missing

elliefm added a commit to elliefm/cyrus-imapd that referenced this issue Aug 1, 2016
@elliefm
Copy link
Contributor Author

elliefm commented Aug 1, 2016

I've converted elliefm/cyrus-imapd to be a fork of this one rather than a standalone repository, which I think means I can now do this:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.0 affects 3.0
Projects
None yet
Development

No branches or pull requests

2 participants