Project

General

Profile

Actions

Bug #19582

closed

Enable UseAWSS3v2Driver by default on keepstore

Added by Lucas Di Pentima 4 months ago. Updated 3 months ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
Keep
Target version:
Start date:
10/13/2022
Due date:
% Done:

100%

Estimated time:
(Total: 0.00 h)
Story points:
-
Release relationship:
Auto

Description

Since #17339 was resolved, we're good to enable this new driver by default. When doing some tests copying data from pirca to jutro, it was observed a 30% thoughput increase when reconfiguring the keepstores with it.


Subtasks 1 (0 open1 closed)

Task #19609: Review 19582-aws-s3v2-driverResolvedPeter Amstutz10/13/2022

Actions

Related issues

Related to Arvados - Bug #19234: New keepstore s3 driver needs non-empty "region" to work with GCPResolvedTom Clegg11/02/2022

Actions
Actions #1

Updated by Lucas Di Pentima 4 months ago

  • Assigned To set to Lucas Di Pentima
Actions #2

Updated by Peter Amstutz 4 months ago

Should include a note in the upgrade notes.

Actions #3

Updated by Lucas Di Pentima 4 months ago

  • Status changed from New to In Progress
Actions #4

Updated by Lucas Di Pentima 4 months ago

Updates at 451fd7e - branch 19582-aws-s3v2-driver
Test run: developer-run-tests: #3317

  • Enables Volumes.x.DriverParameters.UseAWSS3v2Driver by default.
  • Updates upgrade notes explaining how to go back to the old driver if needed.
Actions #5

Updated by Lucas Di Pentima 4 months ago

  • Related to Bug #19234: New keepstore s3 driver needs non-empty "region" to work with GCP added
Actions #6

Updated by Peter Amstutz 4 months ago

Lucas Di Pentima wrote in #note-4:

Updates at 451fd7e - branch 19582-aws-s3v2-driver
Test run: developer-run-tests: #3317

  • Enables Volumes.x.DriverParameters.UseAWSS3v2Driver by default.
  • Updates upgrade notes explaining how to go back to the old driver if needed.

DriverParameters is under SAMPLE in the Volumes section, it's not totally clear to me that those are actually taken as defaults? I don't know where in the code it would merge those. Did you confirm that changing it in config.default.yml actually changes the value in "arvados-server config-dump"?

I created a dummy volume in the config and used config-dump, the only key that's appearing is the one I defined.

      x94bf-nyw5e-222222222222222:
        AccessViaHosts:
          http://localhost:25108/:
            ReadOnly: false
        Driver: S3
        DriverParameters:
          IAMRole: abcd
        ReadOnly: false
        Replication: 0
        StorageClasses:
          default: true

It looks to me like the real actual defaults are applied here in S3AWSVolume.check()

    if v.ConnectTimeout == 0 {
        v.ConnectTimeout = s3DefaultConnectTimeout
    }
    if v.ReadTimeout == 0 {
        v.ReadTimeout = s3DefaultReadTimeout
    }

The parameters get loaded in chooseS3VolumeDriver using json.Unmarshal(volume.DriverParameters, v)

So for toggling the default value of this feature specifically, we either want to do something like: if the parameter isn't defined in DriverParameters (which is a json.RawMessage), then enable it.

On the other hand, a more general solution would be have the config system load default values in this case.

Actions #7

Updated by Lucas Di Pentima 3 months ago

Updates at 5c0d99ac1
Test run: developer-run-tests: #3322

  • Adds relevant code to chooseS3VolumeDriver() so that it applies the new default.
  • I'm not sure how to test this, as it doesn't seem to be previously existing tests for the driver choosing mechanism.
  • arvados-server config-dump won't show the expected output because the DriverParameters complete struct is realized deep in keepstore's code instead of the config loader.
Actions #8

Updated by Peter Amstutz 3 months ago

Lucas Di Pentima wrote in #note-7:

Updates at 5c0d99ac1
Test run: developer-run-tests: #3322

  • Adds relevant code to chooseS3VolumeDriver() so that it applies the new default.
  • I'm not sure how to test this, as it doesn't seem to be previously existing tests for the driver choosing mechanism.
  • arvados-server config-dump won't show the expected output because the DriverParameters complete struct is realized deep in keepstore's code instead of the config loader.

From chat: how about adding a log statement at startup that says which driver it is using, then we can quickly confirm that it is using the intended one.

Actions #9

Updated by Peter Amstutz 3 months ago

Just to be clear, the branch looks fine, but I think it would be valuable to add a logging statement about which driver it is actually using and then manually confirming it is using the right one.

Actions #10

Updated by Lucas Di Pentima 3 months ago

  • Target version changed from 2022-10-26 sprint to 2022-11-09 sprint
Actions #11

Updated by Lucas Di Pentima 3 months ago

Sorry for the delay on this.

I've merged the latest main at 07baa0ed0 and built a dev version of the arvados-server binary. Then, copied it to keep0.tordo and added the SystemLogs.LogLevel=debug configuration.

Running the dev version shows that it's using the new S3 driver versus the goamz driver that the currently installed binary uses:

root@keep0:/home/lucas# ./arvados-server keepstore
{"ClusterID":"tordo","PID":9052,"level":"info","msg":"keepstore dev (go1.17.7) starting, pid 9052","time":"2022-10-31T19:50:08.541313775Z"}
{"ClusterID":"tordo","PID":9052,"level":"debug","msg":"Using AWS S3 v2 driver","time":"2022-10-31T19:50:08.541422667Z"}
{"ClusterID":"tordo","PID":9052,"level":"info","msg":"started volume tordo-nyw5e-000000000000000 (s3-bucket:\"tordo-nyw5e-000000000000000-volume\"), ReadOnly=false","time":"2022-10-31T19:50:08.541522053Z"}
{"ClusterID":"tordo","Listen":"10.253.255.42:25107","PID":9052,"Service":"keepstore","URL":"http://keep0.tordo.arvadosapi.com:25107/","Version":"dev (go1.17.7)","level":"info","msg":"listening","time":"2022-10-31T19:50:08.546093010Z"}
^C
root@keep0:/home/lucas# /usr/bin/keepstore
{"ClusterID":"tordo","PID":9057,"level":"info","msg":"keepstore 2.5.0~dev20221031171231 (go1.17.7) starting, pid 9057","time":"2022-10-31T19:50:31.027144607Z"}
{"ClusterID":"tordo","PID":9057,"level":"debug","msg":"Using goamz S3 driver","time":"2022-10-31T19:50:31.027250897Z"}
{"ClusterID":"tordo","PID":9057,"level":"info","msg":"started volume tordo-nyw5e-000000000000000 (s3-bucket:\"tordo-nyw5e-000000000000000-volume\"), ReadOnly=false","time":"2022-10-31T19:50:31.027288835Z"}
{"ClusterID":"tordo","Listen":"10.253.255.42:25107","PID":9057,"Service":"keepstore","URL":"http://keep0.tordo.arvadosapi.com:25107/","Version":"2.5.0~dev20221031171231 (go1.17.7)","level":"info","msg":"listening","time":"2022-10-31T19:50:31.033830766Z"}
Actions #12

Updated by Lucas Di Pentima 3 months ago

Running tests just in case: developer-run-tests: #3348

Actions #13

Updated by Peter Amstutz 3 months ago

Lucas Di Pentima wrote in #note-11:

Sorry for the delay on this.

I've merged the latest main at 07baa0ed0 and built a dev version of the arvados-server binary. Then, copied it to keep0.tordo and added the SystemLogs.LogLevel=debug configuration.

Running the dev version shows that it's using the new S3 driver versus the goamz driver that the currently installed binary uses:

[...]

Perfect, so it was already logging the driver, just needed to verify it. This LGTM.

Actions #14

Updated by Lucas Di Pentima 3 months ago

  • Status changed from In Progress to Resolved
Actions

Also available in: Atom PDF