Arvados

21702-keep-web-replace_files @ 6b978471b3be79960b230b0278fcb778cbba8101 -- developer-run-tests: #4439

• All agreed upon points are implemented / addressed.
  • ✅ keep-web uses the replace_files API, and does not serialize concurrent writes to a single collection
• Anything not implemented (discovered or discussed during work) has a follow-up story.
  • N/A
• Code is tested and passing, both automated and manual, what manual testing was done is described
  • ✅ upsized IntegrationSuite.TestConcurrentWrites a bit by doing more writes, and writing larger files
• Documentation has been updated.
  • N/A
• Behaves appropriately at the intended scale (describe intended scale).
  • ✅ it's now safe to run multiple keep-web servers behind a load balancer
• Considered backwards and forwards compatibility issues between client and server.
  • no known compatibility issues
• Follows our coding standards and GUI style guidelines.
  • ✅

There's a bit of code that mimics functionality in the WebDAV library: we need to understand the "destination" header in a move/copy request in order to build an appropriate replace_files request.

I contemplated skipping the webdav handler entirely for the MOVE/COPY/DELETE/MKCOL cases, because we don't even look at the modifications they make to the temporary filesystem. But the webdav handler does implement a number of checks (like refusing to MOVE a collection with Depth other than infinity) and I'd rather accept redundant filesystem operations than duplicate all that validation code.

That would be worth revisiting that if/when someone starts using the WebDAV COPY method on non-trivial-sized files/directories. As it stands the webdav handler actually reads and writes all of the file content, only for us to throw away the result. To be clear, this branch doesn't make the situation worse at all.

In trials on my dev machine, the (new upsized) IntegrationSuite.TestConcurrentWrites test runs in about 25s, compared to 35s with the old "serialize writes to same collection ID" code.

I think the performance improvement will be bigger for some more lifelike scenarios -- in particular, concurrent writes where the file data transfer from client to Keep takes much longer than the collection-update transaction. I'm not sure that kind of performance testing should be a blocker, given the other (more important?) goal of correctness in a load-balancing setup.

• test copy collection with depth 0
• test copy/move collection with invalid depth

developer-run-tests: #4445

• fix COPY collection with depth 0
• test copy/move/delete with various allowed/disallowed depth headers
• revert double-size TestConcurrentWrites (decided it was using too many gigabytes of RAM for no good reason)

The comments are great, the code is straightforward, I would be fine for this to be merged as-is, I do have a couple of suggestions for improvement.

I'm concerned about the way that the loop of TestDepthHeader relies on past trials to have succeeded, but merely checks success with c.Check. This means if one early trial fails, potentially many others will fail after it because their expected preconditions are not met. This will generate a lot of failures and make it more difficult to find the "real" problem.

Minimum nicest change would be to change the Check calls to Assert, since we can't know if it's safe to continue testing after a failure.

Next nice change on top of that would be to test each "set of trials" (starting with the setup triplet) separately. e.g., you could change TestDepthHeader to a function that takes a slice of trials, and then have a separate functions for TestDeleteDepth, TestMoveDepth, and TestDeleteEmptyDirectoryDepth (and maybe this could even be broken down further into different sets).

In the code, I think it would be nice to s/rfc4918/RFC 4918/g to match the IETF's own styling.

Thanks.

developer-run-tests: #4453

• check → assert
• rfcNNNN → RFC NNNN (here and in a few other places)
• merged main

failed keepstoreSuite.TestBlockWrite_MultipleStorageClasses -- retry developer-run-tests-remainder: #4675