Repository navigation
Add --enable-nested-virtualization to setup-gcp - #1669
Benjamin Elder (BenTheElder) merged 4 commits into
Conversation
`setup-gcp` creates one node pool, `substrate-node-pool`, and sets no `SandboxConfig` on it, so nothing about that pool is gVisor-specific. The `gvisor-pool` named by `NODE_POOL_NAME` is a different pool that only `hack/teardown.sh` touches, so the old variable named a pool this tool never creates. Micro-VM workers need the same knob for a non-gVisor runtime, which makes the name actively misleading. The `--machine-type` flag and the `Config.MachineType` field already carried neutral names, so only the environment variable and its help text change. Developers copy `hack/ate-dev-env.sh.example` to an untracked `.ate-dev-env.sh` once and edit it from there, so most existing setups still export the old name. `setup-gcp` reads it when `NODE_MACHINE_TYPE` is unset and warns, which keeps those setups on their chosen machine type instead of dropping them onto the `c3-standard-4` default without a word. Remove the fallback once the team has renamed the variable. Verified with `go build ./tools/...` and `go test ./tools/setup-gcp/...`, covering the new name, the old name, both set, and neither.
16da292 to
67ef0e2
Compare
Micro-VM workers (`ateom-microvm`) need `/dev/kvm`, which a GKE node only has when its pool was created with nested virtualization. `setup-gcp` had no way to ask for it, so provisioning a cluster for micro-VM work meant building the node pool by hand with `gcloud`. `create cluster` and `bootstrap` now accept `--enable-nested-virtualization` (`ENABLE_NESTED_VIRTUALIZATION`), which sets `AdvancedMachineFeatures.EnableNestedVirtualization` on the pool the tool creates. It defaults to on, since running micro-VM workers is a reason to create a cluster with this tool and the default `c3-standard-4` supports the feature. Pass `--enable-nested-virtualization=false` for a cluster that does not need it; the field then stays unset, leaving GKE its own default. A machine type GCE excludes from the feature, such as E2 or an AMD type other than N4D, now fails at cluster creation rather than quietly creating nodes without `/dev/kvm`. GKE fixes the setting at node creation and cannot update an existing pool into it. The reconcile path therefore logs a warning naming the remedy instead of reporting a change it did not make. With the default on, that warning reaches every existing cluster whose pools predate the flag, which is the intended signal: those nodes cannot place micro-VM workers. The check passes when any pool has the feature, because keeping KVM nodes in a second pool added by hand is a normal arrangement. Verified with `go test ./tools/setup-gcp/...`, covering the request builder with the flag on and off, and the pool scan across nil, empty, mixed, and disabled clusters.
GCE excludes AMD machine types other than N4D from nested virtualization, so an N2D instance cannot run the microVM runtime. The Linux prerequisites named N2/N2D together as examples of KVM-capable machines, pointing readers at a series that never works. Both N4 and N4D support nested virtualization, so name those instead.
67ef0e2 to
43c27d7
Compare
Benjamin Elder (BenTheElder)
left a comment
There was a problem hiding this comment.
AI review I agree with (dismissed irrelevant comments):
…un time getEnv parses values with strconv.ParseBool, which rejects spellings such as "off" and "no". It discarded those values without a word and fell back to the default, so ENABLE_NESTED_VIRTUALIZATION=off left nested virtualization on. Log the variable, its value, and the fallback when parsing fails. Commands register flags for the same variable in more than one init, so the log records each variable once rather than once per registration. resolveMachineTypeDefault treated NODE_MACHINE_TYPE as set whenever it was present, so exporting it empty suppressed the GVISOR_NODE_MACHINE_TYPE fallback and left the machine type blank. GKE then picks e2-medium, which cannot run nested virtualization. Treat an empty value as unset. The deprecation warning for GVISOR_NODE_MACHINE_TYPE ran from a package-level variable initializer, so it fired on every invocation, including --help and subcommands that never read a machine type. Warn from the commands that read the flag instead, and stay quiet when --machine-type was passed explicitly.
| return ret.(T) | ||
| if err != nil { | ||
| if _, warned := warnedEnvKeys.LoadOrStore(key, struct{}{}); !warned { | ||
| slog.Warn("Ignoring unparsable environment variable", "key", key, "value", val, "using", fallback, "error", err) |
There was a problem hiding this comment.
🤖 nit 🟢 – using=fallback can be wrong. This runs at flag-registration time, so an explicit CLI flag still wins afterwards. ENABLE_NESTED_VIRTUALIZATION=off ... --enable-nested-virtualization=false logs using=true while the cluster is created with nested virtualization off.
Dropping the using field, or wording it as the default rather than the effective value, would avoid the contradiction.
| | `--subnetwork` | VPC subnetwork name. | `SUBNETWORK` | `default` | | ||
| | `--machine-type` | Machine type for the gVisor node pool. | `GVISOR_NODE_MACHINE_TYPE` | `c3-standard-4` | | ||
| | `--machine-type` | Machine type for the node pool. | `NODE_MACHINE_TYPE` | `c3-standard-4` | | ||
| | `--enable-nested-virtualization` | Create the node pool with nested virtualization, exposing `/dev/kvm` for micro-VM workers. | `ENABLE_NESTED_VIRTUALIZATION` | `true` | |
There was a problem hiding this comment.
🤖 should-fix 🟡 – Worth a note here that this applies only to the pool setup-gcp creates. GKE does not propagate nested virtualization to pools created later, and it cannot be enabled on an existing pool.
The "create additional pools" recipe further down (lines 150-154) only mentions --node-labels, so a pool made by following it has no /dev/kvm and micro-VM workers scheduled there stay Pending with no obvious cause. Adding the nested-virtualization flag to that recipe would close the gap.
There was a problem hiding this comment.
[Or we can consider a follow-up to support reconciling, but again, this code should really be moved out to the GKE installer repo anyhow, this tool does not belong in the main repo]
Benjamin Elder (BenTheElder)
left a comment
There was a problem hiding this comment.
I'm ok to handle further nits in follow-up and land and iterate
Nested virtualization exposes
/dev/kvm, which microVMs need. The new--enable-nested-virtualizationflag turns it on for the node poolsetup-gcpcreates. It defaults to on, so pass--enable-nested-virtualization=falsefor a cluster that does not need it.Also, rename the env var
GVISOR_NODE_MACHINE_TYPEtoNODE_MACHINE_TYPEto be consistent with other env vars and to avoid confusion for microVMs. The old env var still works with a warning.