gcp: support `additionalDisks` for GCP instance #3477

HuijingHei · 2023-05-17T07:53:42Z

Force this test to not run by default unless named specifically or `--tag confidential` is passed to `kola run`, also requires `--gcp-machinetype n2d-standard-2 --gcp-confidential-vm --gcp-localssd=NVME` It will create confidential instance on GCP with 1 nvme persistent disk and 1 local ssd disk, then check the new udev rules make effect. Based on coreos/coreos-assembler#3474 and coreos/coreos-assembler#3477. Fix coreos/fedora-coreos-tracker#1457

jlebon

This looks sane, but OTOH I was hoping in the future we'd generalize the --add-disk stuff to not apply only to QEMU. It's also already exposed in kola tests as additionalDisks, so for testing we just need to wire that through to the GCP backend. (The big advantage here is the same as the confidential case; it lets the test declare its requirements rather than require running multiple separate invocations of kola run with different switches.) WDYT?

travier

Mostly LGTM.

mantle/cmd/kola/options.go

travier · 2023-05-17T13:31:33Z

mantle/cmd/kola/options.go

@@ -124,6 +124,7 @@ func init() {
 	bv(&kola.GCPOptions.ServiceAuth, "gcp-service-auth", false, "for non-interactive auth when running within GCP")
 	sv(&kola.GCPOptions.JSONKeyFile, "gcp-json-key", "", "use a service account's JSON key for authentication (default \"~/"+auth.GCPConfigPath+"\")")
 	bv(&kola.GCPOptions.Confidential, "gcp-confidential-vm", false, "create confidential instances")
+	sv(&kola.GCPOptions.LocalSSD, "gcp-localssd", "", "attach local ssd disk, should be NVME or SCSI")


Should we "validate" the values here before launching the instance or is this done by the GCP library?

Force this test to not run by default unless named specifically or `--tag confidential` is passed to `kola run`, also requires `--gcp-machinetype n2d-standard-2 --gcp-confidential-vm --gcp-localssd=NVME` It will create confidential instance on GCP with 1 nvme persistent disk and 1 local ssd disk, then check the new udev rules make effect. Based on coreos/coreos-assembler#3474 and coreos/coreos-assembler#3477. Fix coreos/fedora-coreos-tracker#1457

jlebon

Some comments, but the general idea looks great to me!

mantle/platform/api/gcloud/compute.go

jlebon · 2023-05-18T13:41:14Z

mantle/platform/api/gcloud/compute.go

@@ -33,8 +34,56 @@ func (a *API) vmname() string {
 	return fmt.Sprintf("%s-%x", a.options.BaseName, b)
 }

+// ["local-ssd:interface=NVME,size=375"]
+func ParseDiskSpec(spec string, zone string) (*compute.AttachedDisk, error) {


I think it's fine for now to have this here, but we should ideally dedupe this with the one in qemu.go. We can have a generic parser that returns the size in gigabytes as a uint and any options in a map[string]string.

But again, I'm OK to duplicate for now! Maybe let's add a comment in that case?

Can I understand that it should return like map[string]string{"size": "5G", "interface": "NVME"} after parsing ? Might do this in the later patch.

I was thinking more something like:

package util func ParseDiskSpec(spec string) (uint, map[string]string, error) { ... }

?

Where the uint is the disk size in gigabytes. Since it's a required argument, we can enforce proper typing for it. Also, some cloud API bindings may take it directly as an integer (like GCP here) too which is nice. Others might require a string but suffixed differently (e.g. "GB" vs "G"), which would have to live in the cloud-specific code.

mantle/platform/api/gcloud/compute.go

cgwalters · 2023-05-18T13:43:59Z

mantle/platform/api/gcloud/compute.go

@@ -33,8 +34,56 @@ func (a *API) vmname() string {
 	return fmt.Sprintf("%s-%x", a.options.BaseName, b)
 }

+// ["local-ssd:interface=NVME,size=375"]
+func ParseDiskSpec(spec string, zone string) (*compute.AttachedDisk, error) {


Optional, but I think it'd be pretty easy to have a unit test or two for this function.

mantle/platform/api/gcloud/compute.go

cgwalters · 2023-05-18T13:48:30Z

One tension I see here is that dealing with "cloud configuration" in the general case, particularly abstracting across clouds gets really complicated. This is where it may make sense longer term for us to leverage more mature tools in this space...e.g. terraform.

jlebon · 2023-05-18T13:58:56Z

For consistency, maybe let's have it be more similar to the QEMU one, where the main non-optional item is the size, and then the optional attributes after. So e.g. ["5G:type=TYPE"]. We can default type to local-ssd so that just 5G works.

Related to the "cloud configuration" problem above, to expand on this bit, I think probably our approach should be something where we have sane defaults, so that generic specs like 5G should work fine cross-platform, e.g. we can have a "/var on separate disk" test that runs fine cross-platform with this.

But if you're testing the intricacies of a cloud (e.g. whether the GCP udev rules for persistent NVMe disks show up correctly), then it's fine to have more cloud-specific options since your test will be targeting GCP only anyway.

So we don't have to build a very abstract spec that works cross-platform. I think just the basics would cover a lot.

Force this test to not run by default unless named specifically or `--tag confidential` is passed to `kola run`, also requires `--gcp-machinetype n2d-standard-2 --gcp-confidential-vm` It will create confidential instance on GCP with 1 nvme persistent disk and 1 local ssd disk, then check the new udev rules make effect. Based on coreos/coreos-assembler#3474 and coreos/coreos-assembler#3477. Fix coreos/fedora-coreos-tracker#1457

HuijingHei · 2023-05-19T11:46:08Z

Thanks @jlebon, @cgwalters and @travier for the suggestions.

Build cosa with this patch locally, and run test based on coreos/fedora-coreos-config#2425, get 2 nvme disks for the new instance.

$ ./bin/kola run -E config/src/config/ -d -p gcp --gcp-image=projects/fedora-coreos-cloud/global/images/fedora-coreos-38-20230507-20-3-gcp-x86-64 --gcp-json-key=/srv/tool/gcp.json --gcp-project=xxx --gcp-machinetype n2d-standard-2 --gcp-confidential-vm ext.config.platforms.gcp.nvme-symlink

kola-runext-nvme-symlink[2153]: ++ nvme list-subsys -o json
+ nvme_info='[
  {
    "HostNQN":"nqn.2014-08.org.nvmexpress:uuid:176c7036-3c20-560a-a9b4-4624ed48e5da",
    "Subsystems":[
      {
        "Name":"nvme-subsys1",
        "NQN":"nqn.2014.08.org.nvmexpress:1ae00000nvme_card-pd",
        "Paths":[
          {
            "Name":"nvme1",
            "Transport":"pcie",
            "Address":"0000:00:05.0",
            "State":"live"
          }
        ]
      },
      {
        "Name":"nvme-subsys0",
        "NQN":"nqn.2014.08.org.nvmexpress:1ae00000nvme_card",
        "Paths":[
          {
            "Name":"nvme0",
            "Transport":"pcie",
            "Address":"0000:00:03.0",
            "State":"live"
          }
        ]
      }
    ]
  }
]'

See coreos#3477 (comment)

cgwalters · 2023-05-23T12:50:58Z

mantle/platform/api/gcloud/compute.go

+	if len(opts.AdditionalDisks) > 0 {
+		for _, spec := range opts.AdditionalDisks {


Completely minor nit, but is the len check here needed? Won't the loop be a no-op if the length is zero anyways?

(With stuff like this, feel free to merge as is. Minor fixups like this can be done as a followup alongside other cleanups)

Force this test to not run by default unless named specifically or `--tag confidential` is passed to `kola run`, also requires `--gcp-machinetype n2d-standard-2 --gcp-confidential-vm` It will create confidential instance on GCP with 1 nvme persistent disk and 1 local ssd disk, then check the new udev rules make effect. Based on coreos/coreos-assembler#3474 and coreos/coreos-assembler#3477. Fix coreos/fedora-coreos-tracker#1457

See coreos#3477 (comment)

Force this test to not run by default unless named specifically or `--tag confidential` is passed to `kola run`, also requires `--gcp-machinetype n2d-standard-2 --gcp-confidential-vm` It will create confidential instance on GCP with 1 nvme persistent disk and 1 local ssd disk, then check the new udev rules make effect. Based on coreos/coreos-assembler#3474 and coreos/coreos-assembler#3477. Fix coreos/fedora-coreos-tracker#1457

See coreos#3477 (comment)

Force this test to not run by default unless named specifically or `--tag confidential` is passed to `kola run`, also requires `--gcp-machinetype n2d-standard-2 --gcp-confidential-vm` It will create confidential instance on GCP with 1 nvme persistent disk and 1 local ssd disk, then check the new udev rules make effect. Based on coreos/coreos-assembler#3474 and coreos/coreos-assembler#3477. Fix coreos/fedora-coreos-tracker#1457

See coreos#3477 (comment)

@jlebon

Refer to @jlebon's suggestion, see coreos#3477 (comment)

@jlebon

Refer to @jlebon's suggestion, see coreos#3477 (comment)

@jlebon

Refer to @jlebon's suggestion, see #3477 (comment)

Force this test to not run by default unless named specifically or `--tag confidential` is passed to `kola run`, also requires `--gcp-machinetype n2d-standard-2 --gcp-confidential-vm` It will create confidential instance on GCP with 1 nvme persistent disk and 1 local ssd disk, then check the new udev rules make effect. Based on coreos/coreos-assembler#3474 and coreos/coreos-assembler#3477. Fix coreos/fedora-coreos-tracker#1457

HuijingHei mentioned this pull request May 17, 2023

tests: add nvme-symlink test to verify GCP udev rules coreos/fedora-coreos-config#2425

Merged

jlebon reviewed May 17, 2023

View reviewed changes

travier reviewed May 17, 2023

View reviewed changes

HuijingHei added the do-not-merge/work-in-progress label May 18, 2023

HuijingHei force-pushed the gcp-local-ssd branch 9 times, most recently from 50e7ff7 to 1c3e344 Compare May 18, 2023 08:11

jlebon reviewed May 18, 2023

View reviewed changes

cgwalters reviewed May 18, 2023

View reviewed changes

HuijingHei force-pushed the gcp-local-ssd branch from 1c3e344 to 1ecad0c Compare May 19, 2023 10:35

HuijingHei changed the title ~~gcp: add --gcp-localssd to support local ssd for instances~~ gcp: support additionalDisks for GCP instance May 19, 2023

openshift-ci bot removed the do-not-merge/work-in-progress label May 19, 2023

HuijingHei force-pushed the gcp-local-ssd branch 3 times, most recently from e26365a to c22463d Compare May 19, 2023 11:08

HuijingHei added a commit to HuijingHei/coreos-assembler that referenced this pull request May 23, 2023

util: put ParseDiskSpec func to common

6bfca69

See coreos#3477 (comment)

HuijingHei added a commit to HuijingHei/coreos-assembler that referenced this pull request May 23, 2023

util: put ParseDiskSpec func to common

b41cee3

See coreos#3477 (comment)

HuijingHei added a commit to HuijingHei/coreos-assembler that referenced this pull request May 23, 2023

util: put ParseDiskSpec func to common

f841e42

See coreos#3477 (comment)

HuijingHei added a commit to HuijingHei/coreos-assembler that referenced this pull request May 23, 2023

util: put ParseDiskSpec func to common

2098727

See coreos#3477 (comment)

cgwalters approved these changes May 23, 2023

View reviewed changes

HuijingHei merged commit f929b8f into coreos:main May 24, 2023

HuijingHei deleted the gcp-local-ssd branch May 24, 2023 06:18

HuijingHei added a commit to HuijingHei/coreos-assembler that referenced this pull request May 24, 2023

util: put ParseDiskSpec func to common

be2b841

See coreos#3477 (comment)

HuijingHei added a commit to HuijingHei/coreos-assembler that referenced this pull request May 24, 2023

util: put ParseDiskSpec func to common

162ad20

See coreos#3477 (comment)

HuijingHei added a commit to HuijingHei/coreos-assembler that referenced this pull request May 24, 2023

util: put ParseDiskSpec func to common

c2bc195

See coreos#3477 (comment)

HuijingHei added a commit to HuijingHei/coreos-assembler that referenced this pull request May 24, 2023

util: put ParseDiskSpec func to common

f865c17

See coreos#3477 (comment)

HuijingHei added a commit to HuijingHei/coreos-assembler that referenced this pull request May 25, 2023

util: put ParseDiskSpec func to common

74f7b54

See coreos#3477 (comment)

HuijingHei added a commit to HuijingHei/coreos-assembler that referenced this pull request May 26, 2023

util: put ParseDiskSpec func to common

e413155

See coreos#3477 (comment)

HuijingHei added a commit to HuijingHei/coreos-assembler that referenced this pull request May 26, 2023

util: put ParseDiskSpec func to common

7040b6b

See coreos#3477 (comment)

HuijingHei added a commit to HuijingHei/coreos-assembler that referenced this pull request May 26, 2023

util: put ParseDiskSpec func to common

81c678e

See coreos#3477 (comment)

HuijingHei added a commit to HuijingHei/coreos-assembler that referenced this pull request May 27, 2023

util: put ParseDiskSpec func to common

0ce6434

See coreos#3477 (comment)

HuijingHei added a commit to HuijingHei/coreos-assembler that referenced this pull request May 27, 2023

util: put ParseDiskSpec func to common

b2a4002

See coreos#3477 (comment)

HuijingHei added a commit to HuijingHei/coreos-assembler that referenced this pull request May 29, 2023

util: put ParseDiskSpec func to common

e853e0e

See coreos#3477 (comment)

HuijingHei added a commit to HuijingHei/coreos-assembler that referenced this pull request May 30, 2023

util: put ParseDiskSpec func to common

938a837

See coreos#3477 (comment)

HuijingHei added a commit to HuijingHei/coreos-assembler that referenced this pull request May 30, 2023

util: put ParseDiskSpec func to common

031c812

See coreos#3477 (comment)

HuijingHei added a commit to HuijingHei/coreos-assembler that referenced this pull request May 31, 2023

util: put ParseDiskSpec func to common

ed479ee

Refer to @jlebon's suggestion, see coreos#3477 (comment)

HuijingHei added a commit to HuijingHei/coreos-assembler that referenced this pull request May 31, 2023

util: put ParseDiskSpec func to common

17ad185

Refer to @jlebon's suggestion, see coreos#3477 (comment)

cgwalters pushed a commit that referenced this pull request May 31, 2023

util: put ParseDiskSpec func to common

33cc9fd

Refer to @jlebon's suggestion, see #3477 (comment)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gcp: support `additionalDisks` for GCP instance #3477

gcp: support `additionalDisks` for GCP instance #3477

HuijingHei commented May 17, 2023 •

edited

Loading

jlebon left a comment

travier left a comment

travier May 17, 2023

jlebon left a comment

jlebon May 18, 2023

HuijingHei May 19, 2023 •

edited

Loading

jlebon May 19, 2023

cgwalters May 18, 2023

cgwalters commented May 18, 2023

jlebon commented May 18, 2023

HuijingHei commented May 19, 2023 •

edited

Loading

cgwalters May 23, 2023

cgwalters May 23, 2023

		if len(opts.AdditionalDisks) > 0 {
		for _, spec := range opts.AdditionalDisks {

gcp: support additionalDisks for GCP instance #3477

gcp: support additionalDisks for GCP instance #3477

Conversation

HuijingHei commented May 17, 2023 • edited Loading

jlebon left a comment

Choose a reason for hiding this comment

travier left a comment

Choose a reason for hiding this comment

travier May 17, 2023

Choose a reason for hiding this comment

jlebon left a comment

Choose a reason for hiding this comment

jlebon May 18, 2023

Choose a reason for hiding this comment

HuijingHei May 19, 2023 • edited Loading

Choose a reason for hiding this comment

jlebon May 19, 2023

Choose a reason for hiding this comment

cgwalters May 18, 2023

Choose a reason for hiding this comment

cgwalters commented May 18, 2023

jlebon commented May 18, 2023

HuijingHei commented May 19, 2023 • edited Loading

cgwalters May 23, 2023

Choose a reason for hiding this comment

cgwalters May 23, 2023

Choose a reason for hiding this comment

gcp: support `additionalDisks` for GCP instance #3477

gcp: support `additionalDisks` for GCP instance #3477

HuijingHei commented May 17, 2023 •

edited

Loading

HuijingHei May 19, 2023 •

edited

Loading

HuijingHei commented May 19, 2023 •

edited

Loading