roachprod: Support AWS GP3 drives.#58275
Conversation
|
This change is |
f750f8e to
05d0000
Compare
0f89a2c to
22007da
Compare
|
cc @kenliu for triage - this should be reviewed by the dev inf team |
065cb1a to
fd91d24
Compare
petermattis
left a comment
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @miretskiy)
pkg/cmd/roachprod/vm/aws/aws.go, line 90 at r1 (raw file):
type disk ebsDisk return json.Marshal(&struct { DeleteOnTermination bool `json:"DeleteOnTermination"`
Is this structuring used simply so that DeleteOnTermination is not present in ebsDisk? It might be slightly less awkward to used a named struct:
type extendedDisk struct {
ebsDisk
DeleteOnTermination bool ...
}
pkg/cmd/roachprod/vm/aws/aws.go, line 107 at r1 (raw file):
// This is not strictly needed since AWS sdk would return error anyway, // but we can return a nicer error message sooner. if d.VolumeSize == 0 {
Could we instead use the flag default for --aws-ebs-volume-size of 500 GB?
pkg/cmd/roachprod/vm/aws/aws.go, line 115 at r1 (raw file):
// Nothing -- size checked above. case "gp3": if d.IOPs == 0 || d.IOPs > 16000 {
Should this be d.IOPS < 3000?
pkg/cmd/roachprod/vm/aws/aws.go, line 118 at r1 (raw file):
return errors.AssertionFailedf("Iops required for gp3 disk: [3000, 16000]") } if d.Throughput == 0 {
Could we provide a reasonable default if throughput isn't specified?
pkg/cmd/roachprod/vm/aws/aws.go, line 141 at r1 (raw file):
} type eBSVolumeList []*ebsVolume
Nit: elsewhere you're using the capitalization ebs, but here you're using eBS. I don't have a preference except for consistency.
miretskiy
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/cmd/roachprod/vm/aws/aws.go, line 90 at r1 (raw file):
Previously, petermattis (Peter Mattis) wrote…
Is this structuring used simply so that
DeleteOnTerminationis not present inebsDisk? It might be slightly less awkward to used a named struct:type extendedDisk struct { ebsDisk DeleteOnTermination bool ... }
Agreed... Code started off a bit more complex -- but now marshal is just as you say, to add one more attribute. Made it explicit, as you suggested.
pkg/cmd/roachprod/vm/aws/aws.go, line 107 at r1 (raw file):
Previously, petermattis (Peter Mattis) wrote…
Could we instead use the flag default for
--aws-ebs-volume-sizeof 500 GB?
Done.
pkg/cmd/roachprod/vm/aws/aws.go, line 115 at r1 (raw file):
Previously, petermattis (Peter Mattis) wrote…
Should this be
d.IOPS < 3000?
No, gp3 drives have minimum 3000 iops. I changed this error condition to check for >16000 and added default 3000 IOPs if not set.
pkg/cmd/roachprod/vm/aws/aws.go, line 118 at r1 (raw file):
Previously, petermattis (Peter Mattis) wrote…
Could we provide a reasonable default if throughput isn't specified?
Yup -- set to 125MB, which is base throughput.
pkg/cmd/roachprod/vm/aws/aws.go, line 141 at r1 (raw file):
Previously, petermattis (Peter Mattis) wrote…
Nit: elsewhere you're using the capitalization
ebs, but here you're usingeBS. I don't have a preference except for consistency.
Ooops -- a typo. Changed to "ebsVolumeList" -- no need to export.
Add support for spinning up VMs with GP3 drives. Make it possible to specify multiple, possibly different EBS volumes for each aws instance. Release Notes: None
petermattis
left a comment
There was a problem hiding this comment.
Reviewable status:
|
tftr |
|
bors r=petermattis |
|
This PR was included in a batch that was canceled, it will be automatically retried |
|
Build failed (retrying...): |
|
Build succeeded: |
Add support for spinning up VMs with GP3 drives.
Make it possible to specify multiple, possibly different EBS
volumes for each aws instance.
Release Notes: None