[API] Accept container mounts that specify both uuid and portable_data_hash
"uuid", or both may be provided in a container request.
- If both are provided, the uuid is considered advisory, and the container uses the provided portable_data_hash.
- (unchanged) If only the uuid is provided, the container uses the portable data hash corresponding to the given uuid at the time the container is assigned to the container request.
#7 Updated by Lucas Di Pentima over 1 year ago
I have some questions about this story:
1. The fact that the uuid is considered advisory when passing both pdh & uuid means that it should be preserved on the resolved mount instead of being deleted? I ask because there're tests checking that it should be deleted.
2. When both pdh & uuid are passed, should we still have to check that they match, right? There's also a test confirming this.
3. Always resolving to PDH (this time giving priority to PDH when both are given) means that this story is only about the API server behavior change, or is something else needed?
#8 Updated by Lucas Di Pentima over 1 year ago
In the meantime:
Updates at d8a9500ef - branch
Test run: https://ci.curoverse.com/job/developer-run-tests/970/
- Prioritize pdh over uuid when both provided
- Don't delete uuid key on resolved mounts (as I understand, if they're advisory data, they shouldn't be deleted)
- Update tests
#9 Updated by Tom Clegg over 1 year ago
The container request mounts can have UUID, PDH, or both. If both are given, the UUID is ignored (but preserved).
The container mounts ("resolved mounts") never have UUID, always PDH (so the
mount.delete 'uuid' line should still be there). Including the UUID would interfere with container reuse.
If a container request's PDH and UUID don't match, we use the PDH anyway. This might be the only actual change here: previously this caused an error, which meant an update race would cause failures for a client that looks up the UUIDs by itself (e.g., to ensure all containers in a workflow use the same PDH for a given UUID) but still wants to preserve the UUIDs as provenance information.
#10 Updated by Lucas Di Pentima over 1 year ago
Updates at 467250f94
Test run: https://ci.curoverse.com/job/developer-run-tests/971/
Following above comments, removed the collection lookup by pdh, and only try getting the collection mount by uuid when the pdh is not provided. Updated tests.
#12 Updated by Lucas Di Pentima over 1 year ago
Some workbench integration tests failed -- fix at 50288d61c8654a87053a2b3492672cf7d8d71930
Test run: https://ci.curoverse.com/job/developer-run-tests/974/
#13 Updated by Tom Clegg over 1 year ago
Looks like this restores the possibility of a container mount with no UUID and no PDH. This means "start with empty collection" according to the Containers API spec. Seems like we need a test for this in the API suite, rather than relying on WB integration tests.
With that, LGTM.
#14 Updated by Lucas Di Pentima over 1 year ago
Added test at 2480ed0b8, testing one more time before merging: https://ci.curoverse.com/job/developer-run-tests/975/