Arvados

16427-undelete @ 167e2537365ec68fe6be6a330a2eb698f177aa05 -- developer-run-tests: #1879

TODO: accept UUID of collection-delete/update log entry (currently must provide file containing manifest text)
TODO: doc page

Review comments:

lib/undelete/cmd.go

+ func (und undeleter) newestMtime(logger logrus.FieldLogger, blk string, svc arvados.KeepService) (time.Time, bool) {

It would be nice to follow the Go error pattern instead, and make the second arg an 'error'. Presumably, block not on svc is an error too!

+func (und undeleter) ensureSafe(ctx context.Context, logger logrus.FieldLogger, blk string, svc arvados.KeepService, blobsigttl time.Duration, blobsigexp time.Time) bool {

Same - why not return an error instead of a bool?

+func (und undeleter) RecoverManifest(mtxt string) (string, error) {

Is there a particular reason why half a BlobSigningTTL is a good choice for the save deadline? Can the reason go into a comment?

+ for i := 0; i < 2*len(services); i++ {

It would be great to make it a bit more clear that these are worker threads, and that the number of threads is being sized by the number of keepstore nodes (rather than the cores on the client, or something). Maybe something like

"workerThreads := 2 * len(services)"

+ if havenot > 0 {
+ if have > 0 {
+ und.logger.Warn("partial recovery is not implemented")
+ }
+ return "", fmt.Errorf("unable to recover %d of %d blocks", havenot, have+havenot)
+ }

Partial unrecovery will be nice. Just listing the affected blocks, could also be nice. Blocks + affected filenames even better. That should probably be part of the partial recovery feature.

lib/undelete/cmd_test.go

+func (*Suite) TestUntrashAndTouchBlock(c *check.C) {

+ if fi, err := os.Stat(datadir); err != nil || !fi.IsDir() {
+ c.Logf("skipping datadir %q, evidently neither keepstore is using it", datadir)
+ continue
+ }

This seems like it could generate unnecessary noise in the tests, no? Can we make this debug-only, or even just skip the logging altogether? It doesn't seem super useful.

Functionally: running arvados-server with the `undelete` command prints this, which seems unhelpful:

$ ./arvados-server undelete
Usage: ./arvados-server undelete [options] uuid_or_file ...
-config file
Site configuration file (default may be overridden by setting an ARVADOS_CONFIG environment variable) (default "/etc/arvados/config.yml")
-legacy-crunch-dispatch-slurm-config file
Legacy crunch-dispatch-slurm configuration file (default "/etc/arvados/crunch-dispatch-slurm/crunch-dispatch-slurm.yml")
-legacy-git-httpd-config file
Legacy arv-git-httpd configuration file (default "/etc/arvados/git-httpd/git-httpd.yml")
-legacy-keepbalance-config file
Legacy keep-balance configuration file (default "/etc/arvados/keep-balance/keep-balance.yml")
-legacy-keepproxy-config file
Legacy keepproxy configuration file (default "/etc/arvados/keepproxy/keepproxy.yml")
-legacy-keepstore-config file
Legacy keepstore configuration file (default "/etc/arvados/keepstore/keepstore.yml")
-legacy-keepweb-config file
Legacy keep-web configuration file (default "/etc/arvados/keep-web/keep-web.yml")
-legacy-ws-config file
Legacy arvados-ws configuration file (default "/etc/arvados/ws/ws.yml")
-log-level string
logging level (debug, info, ...) (default "info")
-skip-legacy
Don't load legacy config files
INFO[2020-06-03T09:59:49.409731940-04:00] exiting

Can we have some help text?

Otherwise, LGTM so far!

Ward Vandewege wrote:

It would be nice to follow the Go error pattern instead, and make the second arg an 'error'. Presumably, block not on svc is an error too!

Not exactly (we might go on to find it on a different server), but yes, I've updated both funcs to return error instead of an ok bool.

Is there a particular reason why half a BlobSigningTTL is a good choice for the save deadline? Can the reason go into a comment?

Yes, added comment.

+ for i := 0; i < 2*len(services); i++ {

Added explanatory comment for this too.

Partial unrecovery will be nice. Just listing the affected blocks, could also be nice. Blocks + affected filenames even better. That should probably be part of the partial recovery feature.

Yeah, I think the first "partial recovery" feature would remove any files that are affected by the missing blocks, and save the files that are still intact, and exit non-zero.

Manifests don't support sparse files, so our only options for saving partial files seem to be zero-padding and truncating, so if you start with "abcdefghi" and "def" goes missing, you can recover either "abcghi" or "abc\0\0\0ghi". (I don't think we're going to implement either right away though.)

This seems like it could generate unnecessary noise in the tests, no? Can we make this debug-only, or even just skip the logging altogether? It doesn't seem super useful.

Yes, it only shows up when the test fails or you run -check.vv, but I changed it to print the datadirs that are used instead of the ones that aren't, which is pretty much the same information but less mysterious.

Functionally: running arvados-server with the `undelete` command prints this, which seems unhelpful:

Removed the legacy config flags, added more explanation, and made the "Usage: ..." part appear in the "bad args" case as well as the "no positional args" case.

Usage:  
        ./tmp/GOPATH/bin/arvados-server undelete [options ...] /path/to/manifest.txt [...]

        This program recovers deleted collections. Recovery is
        possible when the collection's manifest is still available and
        all of its data blocks are still available or recoverable
        (e.g., garbage collection is not enabled, the blocks are too
        new for garbage collection, the blocks are referenced by other
        collections, or the blocks have been trashed but not yet
        deleted).

        For each provided collection manifest, once all data blocks
        are recovered/protected from garbage collection, a new
        collection is saved and its UUID is printed on stdout.

        Exit status will be zero if recovery is successful, i.e., a
        collection is saved for each provided manifest.
Options:
  -config file
        Site configuration file (default may be overridden by setting an ARVADOS_CONFIG environment variable) (default "/etc/arvados/config.yml")
  -log-level string
        logging level (debug, info, ...) (default "info")

16427-undelete @ f525d01de971b8b06dabc827bc0bbf46ee7ce9cc -- developer-run-tests: #1892

and with master merged:

16427-undelete @ 2be95ce7f069d9eb131b0d2d922a5e556f75810c -- developer-run-tests: #1893

Tom Clegg wrote:

Ward Vandewege wrote:

It would be nice to follow the Go error pattern instead, and make the second arg an 'error'. Presumably, block not on svc is an error too!

Not exactly (we might go on to find it on a different server), but yes, I've updated both funcs to return error instead of an ok bool.

Cool. Looks like the comments for both functions still needs updating.

Partial unrecovery will be nice. Just listing the affected blocks, could also be nice. Blocks + affected filenames even better. That should probably be part of the partial recovery feature.

Yeah, I think the first "partial recovery" feature would remove any files that are affected by the missing blocks, and save the files that are still intact, and exit non-zero.

Yes. That would be very useful already.

Manifests don't support sparse files, so our only options for saving partial files seem to be zero-padding and truncating, so if you start with "abcdefghi" and "def" goes missing, you can recover either "abcghi" or "abc\0\0\0ghi". (I don't think we're going to implement either right away though.)

I would probably prefer the zero padding approach (because it shows exactly what sections are missing). I agree that this could be a follow on story.

This seems like it could generate unnecessary noise in the tests, no? Can we make this debug-only, or even just skip the logging altogether? It doesn't seem super useful.

Yes, it only shows up when the test fails or you run -check.vv, but I changed it to print the datadirs that are used instead of the ones that aren't, which is pretty much the same information but less mysterious.

Perfect, thanks.

Functionally: running arvados-server with the `undelete` command prints this, which seems unhelpful:

Removed the legacy config flags, added more explanation, and made the "Usage: ..." part appear in the "bad args" case as well as the "no positional args" case.

Excellent thank you.

LGTM with those function comments updated. Thanks!

Tom Clegg wrote:

Nice! I like the documentation.

This branch also adds the "get old manifest from log entry UUID" feature.

16427-undelete @ 570c793f1aa43fd9763a0368554dd395dacdd238 -- developer-run-tests: #1897

Yeah that LGTM. The test failure looks unrelated, do you agree?

Should we also add "provide collection UUID -> get old version from most recent log entry concerning that collection" ...? That would probably be the most convenient way to do the most obvious case, i.e., undo the last change, whether or not it was a "delete".

Yes, that would be nice. Should we call that 'undo' instead of 'undelete' ?

Should we also add "provide collection UUID -> get old version from most recent log entry concerning that collection" ...? That would probably be the most convenient way to do the most obvious case, i.e., undo the last change, whether or not it was a "delete".

Yes, that would be nice. Should we call that 'undo' instead of 'undelete' ?

I think "undo" isn't quite right because we're (intentionally) saving the recovered data elsewhere, rather than restoring the same collection UUID back to an earlier state. Same reasoning applies to calling the current functionality "undelete" for that matter -- it doesn't actually make the collection UUID come back, there's no attempt to restore name/metadata, etc. Would "recover-collection-data" be better, and make sense whether passing a log uuid, collection uuid, or manifest saved in a file? It's a bit unwieldy/verbose but I'm thinking that's ok for something only used in rare/unusual cases.

Tom Clegg wrote:

Should we also add "provide collection UUID -> get old version from most recent log entry concerning that collection" ...? That would probably be the most convenient way to do the most obvious case, i.e., undo the last change, whether or not it was a "delete".

Yes, that would be nice. Should we call that 'undo' instead of 'undelete' ?

I think "undo" isn't quite right because we're (intentionally) saving the recovered data elsewhere, rather than restoring the same collection UUID back to an earlier state. Same reasoning applies to calling the current functionality "undelete" for that matter -- it doesn't actually make the collection UUID come back, there's no attempt to restore name/metadata, etc. Would "recover-collection-data" be better, and make sense whether passing a log uuid, collection uuid, or manifest saved in a file? It's a bit unwieldy/verbose but I'm thinking that's ok for something only used in rare/unusual cases.

Yes, good call. You could shorten it to recover-collection if you want.

developer-run-tests: #1913

• rename subcommand to "recover-collection"
• accept collection UUID (equivalent to passing UUID of most recent log entry with matching object_uuid)

Tom Clegg wrote:

16427-undelete @ e48478841828b1dbab8b69eb9453db23b42ed63f -- developer-run-tests: #1913

• rename subcommand to "recover-collection"
• accept collection UUID (equivalent to passing UUID of most recent log entry with matching object_uuid)

LGTM thanks!