Arvados

With #17967, the exported config has defaults, but keepstore and apiserver both use the default storage classes if the caller doesn't specify anything -- so the SDKs probably don't even need to look up the defaults.

Tom Clegg wrote:

With #17967, the exported config has defaults, but keepstore and apiserver both use the default storage classes if the caller doesn't specify anything -- so the SDKs probably don't even need to look up the defaults.

Let's think this through.

1. The client picks a random server (rendezvous hashed) to write to
2. The server sees there are no storage classes specified, so it assumes the client wants the default ones
3. The server doesn't offer any of the default storage classes
4. The servers sends back an error (what error?)
5. The client tries the next server in the list
6. The next server offers one default storage class but not the other
7. It sends back success with replication=2 (because that's what the storage class's replication is)
8. The client declares success (but didn't write to both storage classes)

So I think it would be better than nothing for backwards compatibility, but in some situations it would not do the right thing.

Peter Amstutz wrote:

1. The client declares success (but didn't write to both storage classes)

Yeah, good call. The client does need to track storage classes after all.

Updates at 5ac636b5a - branch 17696-pysdk-default-storage-class
Test run: developer-run-tests: #2644

• Updates Python SDK to request the configured storage classes when none are explicitly requested.
• Updates the documentation on the collection's API section.
• Adds & updates tests.

Pending¶

• Migration cookbook recipe: Depends on #17994 -- Maybe this task should be on its own story blocked by it.
• Go SDK (in progress)

Update on PySDK branch¶

Updates at 742be53
Test run: developer-run-tests: #2645

• Dropped the commit about changing the StorageClasses.SAMPLE.Default value to false because it made some tests fail.

Updates at 3b492e126 - branch 17696-gosdk-default-storage-class
Test run: developer-run-tests: #2647

• Adds ClusterConfig() function to arvadosclient.
• Loads configured default storage classes when creating a new KeepClient.
• Adds tests.

Lucas Di Pentima wrote:

Updates at 5ac636b5a - branch 17696-pysdk-default-storage-class
Test run: developer-run-tests: #2644

• Updates Python SDK to request the configured storage classes when none are explicitly requested.
• Updates the documentation on the collection's API section.
• Adds & updates tests.

I think we also need the Collection class to take its storage classes from the default. In the current code, a new Collection record will write blocks to the default storage class from the config, but the record will be written to the API with empty storage classes, and we assume the API server will update it to the same default storage classes that the blocks were written to. If, for some reason, the API server behaved differently in setting the default, all the blocks would all have been written to the wrong place.

If the Collection object gets its default storage classes from the config (to be used when not explicitly specified) then both the keep client and Collection object will use the same storage classes.

Lucas Di Pentima wrote:

Updates at 3b492e126 - branch 17696-gosdk-default-storage-class
Test run: developer-run-tests: #2647

• Adds ClusterConfig() function to arvadosclient.
• Loads configured default storage classes when creating a new KeepClient.
• Adds tests.

It hurts a bit that we're adding new features to arvadosclient instead of consolidating it with arvados.Client, maybe someday we'll find time to fix that.

Same comment goes for the Go SDK as for the Python SDK, the arvados.Collection.FileSystem() method should set c.StorageClassesDesired to the config default instead of assuming that keepclient and API server will agree.

Peter Amstutz wrote:

I think we also need the Collection class to take its storage classes from the default. In the current code, a new Collection record will write blocks to the default storage class from the config, but the record will be written to the API with empty storage classes, and we assume the API server will update it to the same default storage classes that the blocks were written to. If, for some reason, the API server behaved differently in setting the default, all the blocks would all have been written to the wrong place.
If the Collection object gets its default storage classes from the config (to be used when not explicitly specified) then both the keep client and Collection object will use the same storage classes.

Updates at 99e0417c6 (branch 17696-pysdk-default-storage-class)
Test run: developer-run-tests: #2654

• Adds default storage classes usage on Collection class.
• Adds test.

• developer-run-tests-sdk-python-ruby: #1963
• developer-run-tests-remainder: #2759

Fixes tests.

After discussion with the team, we decided that the default storage classes handling will be left to just the keep clients so I've dropped the latest commits and rebase to main.

PySDK side at 5209c88 - branch 17696-pysdk-default-storage-class
Test run: developer-run-tests: #2655

Updates at 913c7638c (GoSDK branch)
Test run: developer-run-tests: #2659

• Adds integration test that proves that a Collection is saved with the same storage_classes_desired list as the KeepClient uses.
• In doing the above, I realized that the kc.loadDefaultSorageClasses() call should be done on keepclient.New().
• Updates previously added test.

Lucas Di Pentima wrote:

Updates at 913c7638c (GoSDK branch)
Test run: developer-run-tests: #2659

• Adds integration test that proves that a Collection is saved with the same storage_classes_desired list as the KeepClient uses.
• In doing the above, I realized that the kc.loadDefaultSorageClasses() call should be done on keepclient.New().
• Updates previously added test.

Is KeepClient.loadDefaultClasses() backwards compatible? I see it prints a warning in that case, and DefaultStorageClasses will be nil. Is that handled properly? Glancing at BlockWrite, I'm not confident that it is. Does it need to make sure to set [default] ?

Lucas Di Pentima wrote:

After discussion with the team, we decided that the default storage classes handling will be left to just the keep clients so I've dropped the latest commits and rebase to main.

PySDK side at 5209c88 - branch 17696-pysdk-default-storage-class
Test run: developer-run-tests: #2655

LGTM

Peter Amstutz wrote:

Is KeepClient.loadDefaultClasses() backwards compatible? I see it prints a warning in that case, and DefaultStorageClasses will be nil. Is that handled properly? Glancing at BlockWrite, I'm not confident that it is. Does it need to make sure to set [default] ?

Updates at 6610ed6 (rebased)
Test run: developer-run-tests: #2666
Test re-run: developer-run-tests-remainder: #2772

• Updated (& fixed) tests that simulate the case when talking to an old cluster that doesn't have storage classes keep support & exported config defaults.

Lucas Di Pentima wrote:

Peter Amstutz wrote:

Is KeepClient.loadDefaultClasses() backwards compatible? I see it prints a warning in that case, and DefaultStorageClasses will be nil. Is that handled properly? Glancing at BlockWrite, I'm not confident that it is. Does it need to make sure to set [default] ?

Updates at 6610ed6 (rebased)
Test run: developer-run-tests: #2666
Test re-run: developer-run-tests-remainder: #2772

• Updated (& fixed) tests that simulate the case when talking to an old cluster that doesn't have storage classes keep support & exported config defaults.

LGTM