Arvados

Notes from Ward: Optimize for the following, in order:

• Development time
• Egress bandwidth (this is what we pay the most for)
• Reliability
• Scalability (e.g., you can run more than one at a time to decrease total runtime)

What's the rational for having this "pull" from the destination instead of "push" from the source? The issues of fetching the index and potential security question of having a copy of the data manager key and blob signing key offsite both go away if the data is "pushed" from the source cluster to the target cluster.

Peter Amstutz wrote:

What's the rational for having this "pull" from the destination instead of "push" from the source? The issues of fetching the index and potential security question of having a copy of the data manager key and blob signing key offsite both go away if the data is "pushed" from the source cluster to the target cluster.

As specified this can be used as either a "push" or a "pull": functionally, it doesn't matter whether you run it on the source cluster, the destination cluster, or somewhere else entirely, as long as you can access Keep (either keepproxy or the individual keepstore nodes) at both ends, and your bandwidth to both ends is compatible with your patience/deadlines.

If the destination index can't be fetched, keep-rsync would have to maintain its own independent "checkpoint state" information. A checkpoint state other than the actual destination index would not be nearly as convincing as an assurance that all blocks have been copied. For example, it would be left as a manual exercise to determine whether Data Manager on the destination side had garbage-collected some of the data that (according to the checkpoint) had already been copied.

• split the "data manager key" in two: one key for getting index, one for deleting blocks (this might be a good idea anyway). It would border on silly for the source cluster to refuse to give out its "get index" key even though it's agreeing to send all of its data, given how easy it is to change data manager keys after the copy finishes.
• add a "maintain own destination index" feature -- essentially a checkpoint file that can be populated from a keepstore index and updated on the fly. With the right set of features, this would be very flexible: the sysadmin could decide when to copy the destination index out-of-band, when to have keep-rsync update it by adding blocks as they're copied, and when to have keep-rsync update it by using the keepstore "index" API on the destination side.

commit f3b5ffc2a4409d4c1b676e40ba4e582ca2beeb61

Enhanced our testing environment to allow creating a second set of keepservers to facilitate keep-rsync testing.

Review comments for 7167-blob-sign-sdk branch:

• keepclient/perms.go
  • glint suggests adding comments for ErrSignatureExpired etc (all 4 of the errors defined)
  • Comment on VerifySignature says “ExpiredError” and “PermissionError”. It would be helpful to either use the same names as the defined errors or something like “signature expired error” (plain text than a non-existing error definition name) …
  • Would you hate too much the names expiryHex, expiryTime (instead of expHex, expTime) and signatureHex (in place of sigHex)?

• keepclient/perms_test.go is failing due to error definition renames; "undefined: PermissionError" and "undefined: ExpiredError"

• keepstore/permission_tests.go: since there is no other tests “without” a permission signature, I don’t see why “TestSignLocatorUsesPermissionSecret” can’t simply be “TestSignLocator” (same with the other test). If you prefer to use such a name, can we call it “TestSignLocator’With/Using’PermissionSecret”?

• While at it, may be you can address the golint complaints you newly introduced in keepstore/azure_blob_volume.go?

Radhika Chippada wrote:

Review comments for 7167-blob-sign-sdk branch:

• keepclient/perms.go
  • glint suggests adding comments for ErrSignatureExpired etc (all 4 of the errors defined)

Fixed (and merged "Malformed" into "Invalid")

• Comment on VerifySignature says “ExpiredError” and “PermissionError”. It would be helpful to either use the same names as the defined errors or something like “signature expired error” (plain text than a non-existing error definition name) …

Fixed

• Would you hate too much the names expiryHex, expiryTime (instead of expHex, expTime) and signatureHex (in place of sigHex)?

Fixed

• keepclient/perms_test.go is failing due to error definition renames; "undefined: PermissionError" and "undefined: ExpiredError"

Fixed (whoops, only ran keepstore tests, not sdk tests!)

• keepstore/permission_tests.go: since there is no other tests “without” a permission signature, I don’t see why “TestSignLocatorUsesPermissionSecret” can’t simply be “TestSignLocator” (same with the other test). If you prefer to use such a name, can we call it “TestSignLocator’With/Using’PermissionSecret”?

Fixed. (I was trying to say the only thing tested here that isn't already covered in the SDK tests is that PermissionSecret is actually being used.)

• While at it, may be you can address the golint complaints you newly introduced in keepstore/azure_blob_volume.go?

Added this to #7159, since it's still open.

Branch 7167-blob-sign-sdk lgtm. Thanks.

SDK¶

• 
  client := arvadosclient.New(arvadosclient.APIConfig{
    APIToken: "foo",
    APIHost: "bar",
  })
  

• ...over this:

• 
  client := arvadosclient.ArvadosClient{
    APIToken: "foo",
    APIHost: "bar",
  }
  

• IOW, it seems like the only real purpose of New() is to initialize the Client field. The Go stdlibs seems to use "leave Client nil to use the default", so that would be ideal. For this case I don't think we actually need to deal with that, though: we can move "readConfigFromFile" to arvadosclient.NewFromFile(filename) and initialize Client there. I suppose this means adding a BlobSigningKey field to the ArvadosClient struct, which seems like a reasonable change even though most clients will just leave it empty / not use it. (Conveniently, this should fix the bug that keep-rsync uses the same blobSigningKey global for both src and dst.)

Config file reader should use strings.SplitN(line, "=", 2), and use TrimSpace() on kv[0] and kv[1]

Config file reader should accept "foo" (a config file containing no "/") to mean "read config from $HOME/.config/arvados/foo.conf", like arv-copy does.

• setupKeepClients returns two *keepclient.KeepClient and an error
• instead of tracking an arvSrc variable separately from kcSrc, just use kcSrc.Arvados when you need it

run_test_server¶

--keep-enforce-permissions should be action='store_true' instead of type=str

enforce_permissions should not become a global. Just pass run_keep(enforce_permissions=args.keep_enforce_permissions).

The "--keep-existing" flag looks like trouble. It seems like a circuitous way to satisfy a rather specific and unusual use case: "start two keepstore processes that the API server doesn't know about, and one that it does know about." For the sake of getting keep-rsync working I can live with it. But I think it would be better to start keepstore procs directly from arvadostest without using the Python stuff at all, and do most of the rsync tests by specifying srcKeepServicesJSON and dstKeepServicesJSON (i.e., without contacting an API server at all).

keep-rsync¶

copyBlocksToDst should take kcSrc and kcDst as arguments, rather than using globals. (Same goes for the other new globals -- seems like they can all be passed in to the relevant functions as arguments.)

Combine these into one line ("Getting block %d of %d: %v"?):

+               log.Printf("Getting block %d of %d", done+1, total)
+
+               log.Printf("Getting block: %v", locator)

In getUniqueLocators, how about:

-               if _, ok := uniqueLocators[locator]; !ok {
-                       uniqueLocators[locator] = true
-               }
+               uniqueLocators[locator] = true

Also in getUniqueLocators, instead of building one long index and then splitting it, how about something like:

for uuid := range kc.LocalRoots() {
  reader, err := kc.GetIndex(uuid, indexPrefix)
  scanner := bufio.NewScanner(reader)
  for scanner.Scan() {
    uniqueLocators[strings.Split(scanner.Text(), " ")[0]] = true
  }
}

main() already prints the error returned by performKeepRsync(), so it shouldn't be necessary to log errors from copyBlocksToDst() as well. For example, how about:

-                       log.Printf("Error putting block data: %q %v", locator, err)
-                       return err
+                       return fmt.Errorf("Write error: %q %v", locator, err)

The log message "copying block" should say "Writing block %d of %d: %v" (pick one of %q and %v and use for both "reading" and "writing" messages)

keep-rsync tests¶

• 
  hash := kcDst.PutB([]byte(fmt.Sprintf("test-data-%d", i)))
  _, _, err := kcDst.Ask(getLocator)
  c.Assert(err, IsNil)
  dstLocators = append(dstLocators, hash) // hash will be already be signed if necessary
  

• I don't think the extra assertions belong here. We're not testing keepclient, we're just setting up our keep-rsync test.

c.Check(localRoots != nil, Equals, true) should be c.Check(localRoots, Not(IsNil))

SDK

This "config struct" idea seemed good when we talked about it before, ...

Done

we can move "readConfigFromFile" to arvadosclient.NewFromFile(filename) and initialize Client there

With all the changes in (the one above and the one below), I did not see the need to move this and change / rearrange / add additional tests. Hence, I left this here. May be we can move this when we have another use case for this?

(Conveniently, this should fix the bug that keep-rsync uses the same blobSigningKey global for both src and dst.)

Fixed this

Config file reader should use strings.SplitN(line, "=", 2), and use TrimSpace() on kv⁰ and kv¹

Done

Config file reader should accept "foo" (a config file containing no "/") to mean "read config from $HOME/.config/arvados/foo.conf", like arv-copy does.

Done

"initializeKeepRsync" looks like it should be called "setupKeepClients",

Done

and it looks like we could fix the use of globals something like this:

Done and got rid off all the globals

run_test_server

--keep-enforce-permissions should be action='store_true' instead of type=str

Done

enforce_permissions should not become a global. Just pass run_keep(enforce_permissions=args.keep_enforce_permissions).

Done

The "--keep-existing" flag looks like trouble ...

Removed this and replaced with "num-keep-servers". All the 3 keep servers are created at once now and the first two are used as src and the third is used as dst

keep-rsync

copyBlocksToDst should take kcSrc and kcDst as arguments,

Done. Removed all globals

Combine these into one line ("Getting block %d of %d: %v"?):

Done

In getUniqueLocators, how about:

Done

Also in getUniqueLocators, instead of building one long index and then splitting it, how about something like:

Done

main() already prints the error returned by performKeepRsync(), so it shouldn't be necessary to log errors from copyBlocksToDst() as well. For example, how about:

Done

The log message "copying block" should say "Writing block %d of %d: %v" (pick one of %q and %v and use for both "reading" and "writing" messages)

Done

keep-rsync tests

setupTestData has three copies of what looks like a verbose way to do something like this:

Done. Also, since PutB also return err, I added the check on PutB and did not see the need to do an Ask

c.Check(localRoots != nil, Equals, true) should be c.Check(localRoots, Not(IsNil))

Done. NotNil is available.

I can live with using APIConfig this way in keep-rsync for now, but let's move it from SDK to keep-rsync, unexport it, and get rid of the redundant "consists of" comment.

Rename DiscoverKeepServersFromJSON to LoadKeepServicesFromJSON

• // always:
  kc := keepclient.New()
  // optional:
  kc.LoadKeepServicesFromJSON(...)
  // discover keep services automatically on first use if you didn't Load or Discover them yourself:
  kc.Get(foo)
  

• For now, "load or discover" will be a mandatory step when using the New() method. When we make the "automatic" way work, new code will be a bit easier to write, and existing code will continue to work.

MakeKeepClient (and any new variants) should initialize its "insecure" flag from arv.ApiInsecure instead of an env var.

In run_test_server.py, stop_keep() should also accept a num_servers argument.

In run_test_server.py, instead of the "if args.num_keep_servers is not None:" block, get argparse to do it -- say parser.add_argument('--num-keep-servers', default=2, ...), and pass args.num_keep_servers directly to run_keep(). (This should also let you remove "default is 2 keep servers" from the help text because argparse will already know/show what the default is.)

• -       srcConfig, dstConfig, srcBlobSigningKey, _, err := loadConfig(srcConfigFile, dstConfigFile)
  +       srcConfig, srcBlobSigningKey, _, err := loadConfig(srcConfigFile)
  +       if err != nil { ... }
  +       dstConfig, _, _, err := loadConfig(dstConfigFile)
  

Help text for -replications should say that 0 means "use default replication level configured on destination server".

If -replications isn't provided and there's an error loading the destination discovery doc, return an error instead of using 2.

let's move it from SDK to keep-rsync, unexport it, and get rid of the redundant "consists of" comment.

Done

Rename DiscoverKeepServersFromJSON to LoadKeepServicesFromJSON

Done

MakeKeepClientFromJSON doesn't really seem necessary ... For now, "load or discover" will be a mandatory step when using the New() method

Done

MakeKeepClient (and any new variants) should initialize its "insecure" flag from arv.ApiInsecure instead of an env var.

Done

In run_test_server.py, stop_keep() should also accept a num_servers argument.

Done

In run_test_server.py, ... default=2 ...

Done

loadConfig, readConfigFromFile, and setupKeepClients have two copies of code ...

Done. readConfigFromFile was already reused

Help text for -replications should say that 0 means "use default replication level configured on destination server".

Done

If -replications isn't provided and there's an error loading the destination discovery doc, return an error instead of using 2.

Done

• should each be about two lines long -- just setupRsync(c,whatever,true,1) and then call a testNoCrosstalk helper twice, once for (src,dst) and once for (dst,src).
• don't need to check whether blobSigningKey is empty (SignLocator is a no-op if the signing key is empty)
• shouldn't do all those extra checks that are really just testing keepclient and keepstore -- testNoCrosstalk() probably just needs something like

• dstLocator, _, err := kcDst.PutB([]byte("srcdata"))
  c.Assert(err, Equals, nil)
  dstLocator = strings.Split(dstLocator, "+A")[0]
  _, _, _, err := kcSrc.Get(keepclient.SignLocator(dstLocator, ...))
  c.Assert(err, NotNil)
  c.Check(err.Error(), Equals, "Block not found")
  

Make dstIndex accessible to the individual test cases (like dstLocators etc) so TestKeepRsync_WithNoSuchPrefixInSrc can check len(dstIndex) len(dstLocators), and TestKeepRsync_WithPrefix can check len(dstIndex) > len(dstLocators).

arvadostest.StartAPI() should move from setupRsync() to SetUpSuite() (we can leave the same one running throughout the tests, right?)

arvadostest.StopKeep() should move from TearDownSuite() to TearDownTest() (since we StartKeep in each test)

The setupRsync arguments updateDstReplications bool, replications int seem to be redundant. How about just pass zero as replications when testing "discovery doc default" cases, and drop updateDstReplications?

setupRsync calls setupKeepClients with a replications arg, but then it overwrites kcDst.Want_replicas by itself. Instead of overwriting, it should check kcDst.Want_replicas replications (unless replications==0), right? (IOW, the checks done in TestInitializeRsyncReplicationsCount and TestInitializeRsyncDefaultReplicationsCount should be done right in setupRsync itself instead.)

The first if prefix "" { case in testKeepRsync looks unnecessary: the "else" case is correct whether or not prefix "".

The "else" side of the second if prefix == "" { case should comment that the magic number 2 depends on the coincidence that len(dstLocators) is 2 and len(dstLocatorsMatchingPrefix) is 0 (or would be if we computed it) for all of the prefixes that we have tests for.

Probably defer: It looks like we aren't testing whether we waste time copying blocks that already exist in dst. Perhaps we could backdate the "dst" blocks before running rsync, then filter dstIndex by mtime, and make sure none of dstLocators were touched during the rsync.

After 1 block is done, the progress display should show ETA based on time elapsed and number of blocks done/todo. (We could go further and inspect sizes, but it won't make much difference to the outcome: either there are lots of blocks, in which case the time taken for an average "done" block is a good prediction of an average "todo" block, or there aren't lots of blocks, in which case ETA is small and probably doesn't matter that much.)

Instead of ioutil.ReadAll() followed by kcDst.PutB(), how about:

reader, size, _, err := kcSrc.Get(getLocator)
// [check err]
_, _, err = kcDst.PutHR(getLocator[:32], reader, size)
// [check err]

Some tests like TestLoadConfig don't actually use servers, so they shouldn't be part of ServerRequiredSuite (this should save a bunch of time starting and stopping Keep servers via Python wrapper).

We should log total #blocks on src and dst before starting to copy data (otherwise it's hard to see the difference between "not much to do because not much src data found" and "not much to do because dst is already nearly complete").

• filename = os.Getenv("HOME") + "/.config/arvados/" + filename

• "The name of the source Arvados instance (required) - points at an Arvados config file. May be either a pathname to a config file, or (for example) "foo" as shorthand for $HOME/.config/arvados/foo.conf"

The TestRsyncPutInOne_GetFromOtherShouldFail and TestRsyncWithBlobSigning_PutInOne_GetFromOtherShouldFail tests

Updated

Make dstIndex accessible to the individual test cases

Done

arvadostest.StartAPI() should move from setupRsync() to SetUpSuite()

Done

arvadostest.StopKeep() should move from TearDownSuite() to TearDownTest()

Done (this was unintentional and now corrected)

The setupRsync arguments updateDstReplications bool, replications int seem to be redundant.

Updated

setupRsync calls setupKeepClients with a replications arg, but then it overwrites ...

Improved this

The first if prefix "" { case in testKeepRsync looks unnecessary: ...

Done

The "else" side of the second if prefix == "" { case should comment that ...

Updated comment

After 1 block is done, the progress display should show ETA ...

Done. The acronym ETA does not seem correct in this context. May be we should say, "estimated time remaining ..."?

Instead of ioutil.ReadAll() followed by kcDst.PutB(), how about ...

Done

Some tests like TestLoadConfig don't actually use servers ...

Added ServerNotRequiredSuite

We should log total #blocks on src and dst before starting to copy data

Done

This should have + ".conf" at the end: filename = os.Getenv("HOME") + "/.config/arvados/" + filename

Done

Update help message for -src and -dst. Maybe just copy the arv-copy help messages:

Updated comment

• change conf args to -src an -dst to match spec and arv-copy
• update ETA logging (remove redundant "copying block X of X", compute ETA based on average time for all blocks done so far, not just the very first block)
• in arvadostest.go, always pass --num-keep-servers to python wrapper, and update comments

The rest LGTM, thanks. I'm waiting for the full test suite to finish. If the above changes look good & pass tests for you, please merge. Thanks!

(Even though I have not reopened this ticket "yet"), I was unhappy with the fact that a big chunk of the code in the keep-rsync -> main method was not tested. The main func had all the arg parsing code that had not been tested. Unfortunately, this is the problem with all our go code where we have a main func. So, I researched a little, after discussing with Tom, and:

• extended the keep-rsync main func. All the code from main is now moved into a separate doMain func.

• doMain func returns error on any errors and main does the usual Fatalf on any errors.

• Args are parsed using flag.FlagSet rather than the default flag. This is needed if we need to set flags several time from different tests.

• Another set of tests are added to pass various combination of good and bad flags. Also added a test that uses good config files and does end-to-end keep-rsync test (though this does not actually do any copying to dst, it tests a lot of the other code).

• Happy to report that we can now test other go code with main func using this code as an example

• Also, tested the script manually at command prompt using the command:

 go run keep-rsync.go -src test -replications x -dst test 

• commit c9018ad3b7f67f2f15a6a6671bf271858926586e has this update. Please review and let me know any comments.

• commit a9e08c498ac8126026d1226f34913f87c8e590a0 removes StartKeepWithParams and StopKeepWithParams funcs and exposes only StartKeep and StopKeep with the parameters. All the earlier references are updated.

Thanks.