roachprod: Support AWS GP3 drives.

[miretskiy]

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

[cockroach-teamcity]

This change is 

[knz]

cc @kenliu for triage - this should be reviewed by the dev inf team

[petermattis]

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]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @petermattis)

---

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 DeleteOnTermination is not present in ebsDisk? 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-size of 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 using eBS. I don't have a preference except for consistency.

Ooops -- a typo. Changed to "ebsVolumeList" -- no need to export.

[petermattis]

Reviewable status: complete! 1 of 0 LGTMs obtained

[miretskiy]

tftr

[miretskiy]

bors r=petermattis

[craig]

This PR was included in a batch that was canceled, it will be automatically retried

[craig]

Build failed (retrying...):

• GitHub CI (Cockroach)

[craig]

Build succeeded:

• GitHub CI (Cockroach)