Skip to content

Add scratch-utils CI image, ci-base VM builder, and go-build preload disk - #908

Open
Brian-McM wants to merge 34 commits into
projectcalico:masterfrom
Brian-McM:bm-add-cctools-and-ci-base
Open

Add scratch-utils CI image, ci-base VM builder, and go-build preload disk#908
Brian-McM wants to merge 34 commits into
projectcalico:masterfrom
Brian-McM:bm-add-cctools-and-ci-base

Conversation

@Brian-McM

@Brian-McM Brian-McM commented Sep 2, 2026

Copy link
Copy Markdown
Collaborator

Summary

Adds the CI VM tooling the ArgoCI kind-rig needs, as three pieces that share one
source of truth for versions:

  • scratch-utils — one static binary (createvm / deletevm / secret /
    runonvm) that drives the GCE VM lifecycle over the GCP APIs directly, with no
    gcloud and no bash, so it runs from a distroless image. Published as
    calico/scratch-utils.
  • vm-images/ci-base/ — builds the ci-base GCE image: docker, go, kind, kubectl and
    gh baked in, plus the heavy CI container images pre-pulled, so a CI VM boots
    ready and does zero per-run installs.
  • preload-disks/ci-cache/ — builds a GKE secondary-boot-disk image with
    calico/go-build preloaded, so build pods start with it already on the node
    instead of pulling several hundred MB per run.

It's CI VM tooling that belongs alongside go-build, so it's folded into this
module rather than a separate one.

Layout

Both builder trees are plural with the artifact as a subfolder, so a second one
lands as a sibling rather than a rewrite:

vm-images/ci-base/        # the GCE image family it produces
preload-disks/ci-cache/   # what it caches

Each subfolder is named for the axis a sibling would vary on. No shared driver is
factored out yet — with one type in each tree that would be guessing at what the
second needs, and the two trees share nothing anyway (one drives gcloud, the other
drives daisy through a fetched Go tool).

Everything derives from images/calico-go-build/versions.yaml

This is the part worth reviewing closely. A job that runs on one of these VMs
would otherwise have run inside calico/go-build, so it has to see the same Go —
which means the VM's toolchain must track that file, not this module's go.mod.

vm-images/ci-base/build-image.sh resolves the Go version, its checksum and the
calico/go-build tag from it (via the existing hack/generate-version-tag-name.sh,
the same helper the release tagging uses) and injects them into provision.sh,
which runs as the builder VM's startup-script where it cannot read the repo.
preload-disks/ci-cache resolves the same tag for the image it preloads. Both require
those values rather than defaulting them: silently baking a stale Go is the exact
drift this indirection exists to prevent.

Tools with no home in that file — kind, gh, the kind node image — are pinned in
vm-images/ci-base/versions.yaml. Nothing floats on latest or stable.txt, so two
builds of one commit produce the same image.

Image naming

Images are named off the go-build release tag, so a VM image and the toolchain it
was built against are matchable by eye:

go-build image calico/go-build:1.27.1-llvm21.1.8-k8s1.37.0
ci-base VM image ci-base-1-27-1-llvm21-1-8-k8s1-37-0
preload disk cic-1-27-1-llvm21-1-8-k8s1-37-0

hack/generate-image-name.sh builds these. Two constraints shape it: GCE names
are RFC1035 so the dots become hyphens (the exact tag is kept as a go-build-tag
label), and GKE caps a secondary boot disk name at 39 characters — enforced
when a node pool attaches the image, long after it built cleanly — which is why
the preload prefix is terse and why the generator takes -m to fail at build time
instead.

On a tag build the version comes from $SEMAPHORE_GIT_TAG_NAME, matching what
calico-go-build-cd already does; that is the only place the re-release suffix
lives (-1, -2), which generate-version-tag-name.sh cannot know about.
Otherwise it uses the branch.

CI

  • Build check for the calico/scratch-utils image on every PR.
  • New go test block, run in the golang image at the version versions.yaml
    pins — so tests see the same Go the go-build image ships and the agent needs
    none of its own. This repo ran no Go tests before.
  • promotions/calico-scratch-utils.yml — change-gated publish, mirroring the
    other image promotions.
  • promotions/ci-base-vm-image.yml and promotions/preload-disk-image.yml
    chained off the go-build publish, auto-promoted on a release tag, so both
    images rebuild once the go-build image they bake or cache actually exists.
    Gated on the tag rather than merely on that pipeline passing, since it also runs
    for master, where a rebuild would spend a builder VM per merge for a throwaway
    branch image. Both remain promotable by hand for a one-off rebuild.

New dependencies

google.golang.org/api (compute) and golang.org/x/crypto (ssh). Both are for
scratch-utils; nothing else in the module imports them.

Deliberately not added: gke-disk-image-builder. It has a clean library API,
but its go.mod still declares the old GoogleCloudPlatform/ai-on-gke path while
the code lives at ai-on-gke/tools, so go get at the real location fails on a
path mismatch and the declared path resolves only to an abandoned 2023 snapshot.
preload-disks/ci-cache fetches a pinned commit instead, which also keeps compute-daisy
and the cloud.google.com/go stack — about 17 modules — out of this module's
dependency graph.

Testing

  • 21 unit tests across 6 files: gofmt, go vet and go test clean on Go 1.27.1.
    Coverage is the pure logic plus the security-relevant path — untar's guard
    against tar entries escaping the destination, which GetDir applies to whatever
    a remote sends. That test was mutation-checked: it fails when the guard is
    removed. shellQuote is verified by round-tripping hostile values through a real
    bash, including inside the export NAME=... line the env file is built from.
  • Both images were actually built and used: ci-base-1-27-1-llvm21-1-8-k8s1-37-0
    booted real CI VMs, and the preload disk builder was run end-to-end with only its
    defaults. The 1.27.1 rebuilds follow this merge.
  • scratch-utils:v0.3 built, pushed and pulled back to confirm the binary
    dispatches and errors cleanly without credentials.

Notes for review

  • Subcommand, not command. The image ENTRYPOINT is the binary, so a pod passes
    args: [createvm] (or command: [scratch-utils, createvm]). A bare
    command: [createvm] overrides the entrypoint and fails to exec.
  • max-run-duration is eventual, not guaranteed. It fires on time, but it is
    itself an operation: observed in us-central1-a, an insert stayed RUNNING for over
    90 minutes and the deadline's own DELETE then sat PENDING behind it. Hence the
    per-zone timeout in the zone loop rather than relying on the backstop.
  • GOOGLE_VM_IMAGE pins an exact VM image; unset, the ci-base family gives
    the newest. The pin is what lets the image be rolled without rebuilding the
    binary.
  • The kindest/node tag is still hand-maintained in vm-images/ci-base/versions.yaml — it
    comes from calico's lib/kind, which this repo has no view of.

Authored with AI assistance (Claude Code).

cctools is a single static binary (createvm/deletevm/secret/runonvm subcommands)
that drives the ArgoCI kind-rig's GCE VM lifecycle over the GCP APIs -- no gcloud,
no bash -- so it runs from a distroless image. It is folded into this module
(rather than a separate one) since it is CI VM tooling that lives alongside
go-build; that adds google.golang.org/api (compute) + golang.org/x/crypto (ssh).

- cmd/cctools dispatches the four subcommands; cctools/ holds the shared gce (VM
  create/delete + native x/crypto/ssh) and ccutil (secret materialization +
  compute ADC) packages plus the subcommands.
- images/calico-cctools: distroless image (carries the CA certs GCP API TLS needs),
  built + published as calico/cctools via images/Makefile calico-cctools-image/-cd
  and a Semaphore build block + change-gated promotion.
- vm-image: the ci-base GCE image builder (provision.sh bakes docker/go/kind/
  kubectl/gh and pre-pulls the heavy images; build-image.sh snapshots it into an
  image family). Manual for now; a master/release promotion can build it on merge.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

There are a few concrete correctness/documentation issues (SSH PutData stderr capture, tar file descriptor handling, misleading GCE package/docs, and some mismatched/incorrect docs/config defaults) that should be addressed before merging.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR introduces cctools (a single, distroless-friendly Go binary for CI GCE VM lifecycle + SSH-based remote execution) and adds vm-image scripts for building a reusable ci-base GCE image with common CI tooling preinstalled, along with CI wiring to build/publish the calico/cctools image.

Changes:

  • Add cmd/cctools dispatcher plus cctools/ packages implementing createvm, deletevm, secret, and runonvm over GCP APIs and native SSH.
  • Add vm-image/ scripts/docs to provision and snapshot a ci-base GCE image.
  • Add images/calico-cctools image + Makefile/Semaphore plumbing for build and master-branch publish.
File summaries
File Description
vm-image/README.md Documents the ci-base image and how to build/refresh it.
vm-image/provision.sh Boot-time provisioning script to bake CI tooling and pre-pulled images into the base VM image.
vm-image/build-image.sh Script to create a builder VM, wait for provisioning, snapshot to an image family, and clean up.
images/Makefile Adds build/publish targets for the new calico/cctools image.
images/calico-cctools/versions.yaml Introduces version pinning for the calico/cctools image tag.
images/calico-cctools/Dockerfile Defines a distroless image that only contains the cctools binary.
go.sum Records checksums for newly introduced Go dependencies (GCP + SSH).
go.mod Adds module dependencies required for cctools (and additional new requirements).
cmd/Makefile Builds cctools alongside existing cmd binaries.
cmd/cctools/main.go Subcommand dispatcher for cctools binary.
cctools/subcmd/secret/secret.go Implements cctools secret for env-var-to-file materialization.
cctools/subcmd/runonvm/runonvm.go Implements SSH-based upload/run/download workflow for executing scripts on a VM.
cctools/subcmd/deletevm/deletevm.go Implements best-effort VM deletion by name/zone discovery over compute API.
cctools/subcmd/createvm/createvm.go Implements VM creation from an image family and writes chosen zone to a file.
cctools/gce/vm.go Compute API client and VM create/delete/zone-discovery logic.
cctools/gce/ssh.go SSH key injection + native SSH transport and tar-based put/get helpers.
cctools/ccutil/secret.go Shared helpers for secret materialization and setting up compute ADC.
.semaphore/semaphore.yml Adds build block + auto-promotion trigger for the cctools image.
.semaphore/promotions/calico-cctools.yml Adds a promotion pipeline to publish calico/cctools images on master merges.
.gitignore Ignores the new images/calico-cctools/bin staging directory.
Review details
  • Files reviewed: 18/20 changed files
  • Comments generated: 10
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread cctools/gce/ssh.go Outdated
Comment thread scratch-utils/gce/ssh.go
Comment thread vm-image/README.md Outdated
Comment thread vm-image/provision.sh Outdated
Comment thread cctools/gce/vm.go Outdated
Comment thread scratch-utils/gce/vm.go Outdated
Comment thread scratch-utils/gce/vm.go Outdated
Comment thread scratch-utils/subcmd/createvm/createvm.go
Comment thread scratch-utils/subcmd/runonvm/runonvm.go Outdated
Comment thread go.mod Outdated
Brian-McM and others added 3 commits September 2, 2026 13:46
Rename throughout: cctools/ -> scratch-utils/, cmd/cctools -> cmd/scratch-utils,
images/calico-cctools -> images/calico-scratch-utils, calico/cctools ->
calico/scratch-utils, and the CALICO_CCTOOLS make vars. The ccutil package moves
with it as scratch-utils/util.

Review fixes:

- go.mod: drop golang.org/x/tools and honnef.co/go/tools. Nothing here imports
  them; they leaked in from an unrelated local experiment and dragged a large
  tree into go.sum. Re-tidied.

- gce.PutData: errBuf was declared but never wired to sess.Stderr, so every
  upload failure reported an empty remote stderr. Wire it up, and do the same
  for PutDir, which was equally silent.

- createvm/runonvm: bound the compute-API calls with a context deadline.
  waitZoneOp loops on ZoneOperations.Wait, so an operation that never reaches
  DONE hung the step until the workflow's own timeout. deletevm already had one.
  runonvm scopes its deadline to setup only -- running the script is the CI job
  and stays unbounded.

- cmd/Makefile: bin/scratch-utils-$(ARCH) depended only on main.go, so editing
  anything under scratch-utils/ left an incremental build shipping a stale
  binary. Depend on the whole source tree.

- Drop stale comments: there is no WaitReady, guest attributes are never
  enabled, and the cloud/kind-rig/... paths are from the origin repo.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
….yaml

Copilot flagged provision.sh's GO_VERSION=1.26.5 as out of step with go.mod's
go 1.27.0, but go.mod is the wrong thing to track. A job that runs on the ci-base
VM is a job that would otherwise have run inside calico/go-build, so the VM has to
see the Go that image ships. images/calico-go-build/versions.yaml is the single
source of truth both are built from, and hack/generate-version-tag-name.sh already
composes the image tag from it for release tagging.

provision.sh runs on the builder as its startup-script and cannot read the repo,
so build-image.sh now resolves the versions and injects them as a preamble:

  GO_VERSION     <- generate-version-tag-name.sh -g            (1.27.0)
  GO_SHA256      <- .golang.checksum.sha256.amd64              (verified on download)
  GO_BUILD_IMAGE <- calico/go-build:$(generate-version-tag-name.sh)

The prepull list uses GO_BUILD_IMAGE instead of the hardcoded (and by now stale)
calico/go-build:1.26.5-llvm21.1.8-k8s1.37.0-beta.0-1, so it cannot drift either.
provision.sh requires these rather than defaulting them -- silently baking a stale
Go into the image is the drift this indirection exists to prevent. kindest/node
stays hardcoded; it comes from calico's lib/kind, not from this repo.

Also from the same review:

- tarDir: close each file after copying instead of deferring inside the Walk
  callback, which held every descriptor open until the walk finished.
- gce.Config: drop the Project field. Client is already scoped to a project and
  every other method used c.project; Create was the odd one out, and an empty
  cfg.Project would have failed confusingly.
- parseDiskGB: report the value as the caller set it, not the trimmed/uppercased
  remains.
- shellQuote: reword the doc comment. The misrendered sequence was gofmt itself
  rewriting a doubled apostrophe in a comment into a curly quote, so the escape
  cannot be spelled there -- say so and point at the code.
- README: fix the cloud/kind-rig/vm-image path and the run-kindrig.sh /
  vm-bootstrap.sh references, none of which exist in this repo, and document
  where the versions come from.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Promotions
----------
The previous commit picked up an in-progress semaphore.yml edit whose two
promotion entries pointed at pipeline files that were never committed, so both
would have failed to resolve. Add them, along with the preload-disk scripts the
second one drives:

- promotions/ci-base-vm-image.yml  -> vm-image/build-image.sh
- promotions/preload-disk-image.yml -> preload-disk/build-preload-disk.sh

Both stay manual-only (no auto_promote): each spins up a throwaway GCE builder
VM, so it is a deliberate act, not something every master push should do. Both
prologues install yq if the agent lacks it, since the build scripts now resolve
versions from the versions.yaml files.

preload-disk had the same stale go-build tag vm-image did
(1.26.5-llvm21.1.8-k8s1.37.0-beta.0-1); it now resolves the image from
images/calico-go-build/versions.yaml through the same helper. A floating tag
preloads nothing anyway -- the node cache only hits the exact ref a pod requests.

Reproducible VM images
----------------------
kind, kubectl and gh were installed from dl/latest, stable.txt and
releases/latest, so two builds of the same commit produced different images.
kubectl now tracks .kubernetes.version from the go-build versions file (same k8s
release the image is cut against); kind and gh are pinned in a new
vm-image/versions.yaml, along with the kind node image that was hardcoded in
provision.sh. All four download URLs verified to resolve.

Tests
-----
This repo ran no Go tests at all, so add a block that runs them in the official
golang image at the version versions.yaml pins -- the agent needs no Go of its
own, and the tests see the same Go the go-build image ships.

Coverage is the pure functions plus the one security-relevant path: untar's
guard against tar entries that escape the destination, which GetDir applies to
whatever a remote sends. That test was mutation-checked -- it fails when the
guard is removed. shellQuote is verified by round-tripping hostile values
through a real bash, including inside the `export NAME=...` line the env file is
made of.

Remaining review fixes
----------------------
- FindZone: quote the filter value (an unquoted name containing - or . is a
  syntax error to the API, not a non-match) and follow pagination, since
  AggregatedList spans every zone.
- PutDir: close the pipe's read end so the tar goroutine cannot park forever on a
  write nobody will read if the session dies early, and surface the tar error
  instead of discarding it -- it meant we shipped a truncated tree that the
  remote untarred without complaint.
- createvm/runonvm: drop mustEnv's os.Exit from inside run(); return an error or
  an exit code like every other failure path there.
- Dockerfile: use an absolute ENTRYPOINT rather than relying on the base image's
  PATH.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@hjiawei hjiawei self-assigned this Sep 3, 2026
Brian-McM and others added 15 commits September 2, 2026 20:27
The builder was fetched from ai-on-gke/tools at `main`, so two runs of the same
toolchain commit could build their disks with different builder code. That was
tolerable while this was a manual job; it is not once it runs on every go-build
release.

The old comment recommended pinning a SHA, but `git clone --branch` accepts only
branch and tag names -- a SHA fails with "Remote branch <sha> not found", and
upstream publishes no tags at all, so `main` was in practice the only usable
value. Fetch with init + fetch + checkout FETCH_HEAD instead, which takes a SHA,
branch or tag alike, and record the commit in preload-disk/versions.yaml.

Not imported as a Go module, though it has a clean library API: the module's
go.mod still declares the old GoogleCloudPlatform/ai-on-gke path while the code
lives at ai-on-gke/tools, so `go get` at the real location fails on a path
mismatch and the declared path resolves only to a 2023 snapshot whose directory
was deleted from that repo. Fetching a pinned commit gets current code and keeps
compute-daisy and the cloud.google.com/go stack -- ~17 modules -- out of this
repo's dependency graph, which scratch-utils shares.

Verified: the pinned commit fetches, checks out at that exact SHA, and compiles.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Both images were timestamped -- ci-base-20260902-133600 -- which says when a
build happened but not which toolchain it corresponds to. Name them the same way
the go-build images are, so the two line up by eye:

  go-build image    calico/go-build:1.27.0-llvm21.1.8-k8s1.37.0
  ci-base VM        ci-base-1-27-0-llvm21-1-8-k8s1-37-0
  preload disk      go-build-preload-1-27-0-llvm21-1-8-k8s1-37-0

hack/generate-image-name.sh composes the name, alongside the existing
generate-version-tag-name.sh it calls. Two things it has to handle:

- GCE resource names are RFC1035 -- lowercase, digits and hyphens, first
  character a letter, 63 max -- so the dots in a version string are not legal in
  a name. They become hyphens, and the exact tag goes on as a `go-build-tag`
  label so the un-mangled version is still there to look up.

- The version comes from $SEMAPHORE_GIT_TAG_NAME on a tag build, because that is
  the only place the re-release suffix lives: a CVE fix that leaves every
  compiler version untouched reuses the version tag with -1, -2 appended, and
  generate-version-tag-name.sh does not emit that. Without this, two releases
  would fight over one image name. Outside a tag build it falls back to the
  versions file, which is right for a manual run.

A deterministic name collides where a timestamp never did, so both scripts now
check for the image up front and refuse with instructions -- rather than building
a builder VM for four minutes and failing at the image-create step.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
generate-image-name.sh fell back to the versions file when no tag triggered the
build, so a manual promotion off master produced ci-base-1-27-0-llvm..., the same
name a release build would later claim -- and the release build would then be
refused because "that release is already built".

Follow what calico-go-build-cd already does instead
(.semaphore/promotions/calico-go-build.yml): tag build uses the git tag, anything
else uses SEMAPHORE_GIT_WORKING_BRANCH. A local run uses the checked-out branch,
and only a detached HEAD with no CI hints falls back to the versions file.

  release tag   1.27.0-llvm21.1.8-k8s1.37.0-1  ->  ci-base-1-27-0-llvm21-1-8-k8s1-37-0-1
  branch        master                         ->  ci-base-master
  branch        go1.27                         ->  ci-base-go1-27

Branch names reach places version strings do not, so the fold to a legal RFC1035
name now handles slashes, underscores, uppercase, repeated separators and leading
or trailing hyphens, not just dots. feature/BM_Fix -> ci-base-feature-bm-fix.

Rebuild behaviour splits the same way calico/go-build's tags do. A release image
is immutable: a collision means that release is already built, so the build is
refused. A branch image is the moving "latest build of this branch", like the
calico/go-build:<branch> docker tag, so it is deleted and rebuilt. Both checks
still run before the builder VM is created.

preload-disk carries an extra warning on the branch path: a node pool pins the
image name in --secondary-boot-disk, so replacing one deletes something a pool
may still reference. Release images are never replaced, which is the case that
matters for a pool pinned to a release.

No `latest` equivalent is needed for the VM image -- the image family already
serves that role, and createvm asks for the family.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The warning implied a pool pinning the image loses its disk. It does not: the
secondary boot disk is attached at node creation, so existing nodes already hold
their copy and are unaffected. Only nodes created during the rebuild window miss
the cache. The pool stores the image resource path rather than an id, so a
same-name recreate leaves its config valid.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Comment-only. The prose had drifted long -- several blocks ran five or six lines
where one or two carry the same point, and the shell scripts were up to 39%
comment. Keep the why, drop the essay: -286/+206 lines.

No behaviour change; build, vet, tests, shell syntax and YAML parse all verified.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…gurable

createvm could only select by image FAMILY, which always resolves to the newest
member, so there was no way to hold a VM image steady -- and no way to roll to a
new one without rebuilding scratch-utils. GOOGLE_VM_IMAGE now takes an exact name
(ci-base-1-27-0-llvm21-1-8-k8s1-37-0) and wins over the family; unset, behaviour
is unchanged. Now that images are named per go-build release, that pin is the
point of the naming.

preload-disk gains NETWORK/SUBNET, defaulting to semaphore-autotest. The builder
hardcoded "default", which fails in unique-caldron-775: that network is LEGACY
(10.240.0.0/16, no subnets in any region) and the builder always requests a
subnetwork, so daisy rejected the workflow with subnetworkResourceDoesNotExist.
`gcloud compute instances create` is happy with a legacy network, which is why
vm-image builds worked and this did not. Every real CI VM in the project uses
semaphore-autotest.

Both verified by building the real images against
calico/go-build:1.27.0-llvm21.1.8-k8s1.37.0:

  ci-base-1-27-0-llvm21-1-8-k8s1-37-0            READY, family ci-base
  go-build-preload-1-27-0-llvm21-1-8-k8s1-37-0   READY

Image tag bumped to v0.2 per the versions.yaml convention, since the binary's
behaviour changed.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The runonvm package doc showed `command: [runonvm, ...]`, which is wrong for a
pod: the image ENTRYPOINT is the scratch-utils binary, and Kubernetes `command`
REPLACES the entrypoint, so a bare subcommand name fails with

  exec: "createvm": executable file not found in $PATH

Show `command: [scratch-utils, runonvm, ...]`, note that a plain container
template can use `args` and leave the entrypoint alone, and repeat the point in
the binary's own doc where the subcommands are listed.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A run failed with:

  could not create <vm> in any zone: insert in us-central1-f: context deadline exceeded

which blames the last zone for a deadline an earlier one had already spent. All
four zones shared a single 10-minute context, so a slow zone consumed the whole
budget and every later zone failed its insert instantly -- and because lastErr was
overwritten each time, the only error reported came from a zone that was never
really attempted. The actual failure was discarded.

- Each zone now attempts under its own PerZoneTimeout (3m). The zone list exists
  so a zone short on capacity can be skipped, which only works if each gets a
  budget of its own.
- Every zone's error is collected with errors.Join, and a zone skipped because the
  context was already dead says "not attempted" rather than posing as a failure.
  Which zones were out of capacity and which were never reached is the thing you
  need from a CI log.
- A zone whose insert was accepted but whose wait failed is now cleaned up
  best-effort. Abandoning it left a VM running to max-run-duration while we booted
  another elsewhere -- two VMs for one job.
- createvm derives its overall deadline from the zone count rather than a fixed
  10m, which had silently starved the later zones.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Picks up the zone-loop fix: per-zone deadlines, every zone's error reported, and
best-effort cleanup of an instance abandoned mid-create.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Create's doc called max-run-duration "a leaked-VM backstop independent of any
cleanup step". Evidence says otherwise. In us-central1-a today:

  15:43:51Z insert  RUNNING   end=          <- still running 1h41m later
  15:43:59Z insert  DONE      end=17:08:52Z <- the sibling took 1h25m
  17:18:05Z delete  PENDING   end=          <- the deadline's own DELETE, stuck

The reclaim fires on time but is itself an operation, and it queued behind the
wedged insert. A VM whose insert is stuck can outlive its max-run-duration, so
the backstop is eventual rather than guaranteed.

Also record why PerZoneTimeout is what it is: the failure mode is slowness, not
rejection. The zone accepted the insert and then took 85 minutes, rather than
returning an out-of-capacity error the zone loop could react to.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three places said the disk image had to live in the GKE cluster's project. It
does not. --secondary-boot-disk takes a fully-qualified resource path, and GKE
documents cross-project use directly; what a cluster elsewhere needs is
roles/compute.imageUser on the image's project, granted to BOTH the cluster
project's default compute service account and its container-engine-robot service
agent.

Record the grants, since missing either fails node creation rather than image
creation and so surfaces long after the mistake.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Attaching the disk failed at node-pool creation:

  Secondary boot disk image name must be at most 39 characters
  (current name "go-build-preload-1-27-0-llvm21-1-8-k8s1-37-0" is 44)

GKE caps a secondary boot disk image name at 39, well under GCE's own 63, and
enforces it when a node pool ATTACHES the image. The image itself built and
reported READY, so the mistake surfaced a long way from its cause.

generate-image-name.sh gains -m to cap below 63; preload-disk passes -m 39 so an
over-long name fails at build time instead. ci-base keeps the 63 default.

The prefix shortens to "gbp" rather than abbreviating the version, which keeps
the go-build tag verbatim so the disk and the image it caches still match by eye.
It fits the longest real tag shape too: gbp-1-26-5-llvm21-1-8-k8s1-37-0-rc-1-1 is
38 of 39. A readable prefix like "preload" would have fit today's tag at 35 and
broken on the next release candidate.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The disk was being built in unique-caldron-775 while the GKE clusters that
consume it live in tigera-cc-dev. Cross-project attachment works, but it needs
compute.imageUser for both the cluster project's compute SA and its GKE service
agent, and the daisy scratch bucket would need cross-project write too since the
builder VM streams its serial log there. Building in the consuming project drops
all of that.

- PROJECT defaults to tigera-cc-dev.
- NETWORK/SUBNET go back to "default". That project's default network is
  auto-mode with a real us-central1 subnet, so the builder's own defaults work.
  semaphore-autotest is now documented as the unique-caldron-775 exception --
  its default network is legacy with no subnets at all -- rather than baked in.
- GCS_PATH points at gs://gke-argo-disk-images-cc-dev, created in tigera-cc-dev.
  The name differs from the unique-caldron-775 bucket only because GCS names are
  globally unique.

Verified by running the builder with these defaults and nothing overridden but a
throwaway image name: validation, builder VM, unpack, snapshot and cleanup all
passed, and the test image was deleted.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ssage

GCE control-plane calls occasionally drop the HTTP/2 connection mid-request
("http2: client connection lost") or return a 429/5xx. Wrap the instance get,
the ssh-keys set-metadata and the operation wait in a 4-attempt exponential
backoff so that flake stops failing a CI job.

Every error is wrapped with the caller's `what`, including the permanent one.
Returning fn's error bare -- as the first cut did -- regressed the messages this
replaced: `fmt.Errorf("get instance %s: %w", ...)` became a naked
"googleapi: Error 404: not found" with nothing saying which call or which
instance produced it, and 404/permission-denied is the common case, not the
retried one. The context-cancelled path names the last failure alongside the
deadline for the same reason.

Also splits the doc comment, which was attached to retryBackoff rather than to
retry, so godoc rendered it on the wrong symbol.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@Brian-McM Brian-McM changed the title Add cctools CI-tools image + ci-base VM builder Add scratch-utils CI image, ci-base VM builder, and go-build preload disk Sep 9, 2026
The script ended by printing a gcloud node-pools create invocation. The people
running it know what comes next, and attaching the disk is due to be automated
anyway. The command is still in README.md for anyone who wants it.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@Brian-McM
Brian-McM requested a lite review from Copilot September 9, 2026 16:48
vm-image/ and preload-disk/ each held exactly one builder, so a second would have
meant renaming the directory. Pluralise them and name each subfolder for the axis
a sibling would vary on:

  vm-image/     -> vm-images/ci-base/         the GCE image family it produces
  preload-disk/ -> preload-disks/go-build/    what it caches

Neither name is invented: ci-base is already the family createvm defaults
GOOGLE_VM_IMAGE_FAMILY to, and go-build is what the gbp image prefix stands for.

No shared driver is factored out. With one type in each tree that would be
guessing at what a second needs, and the two trees share nothing with each other
anyway -- one drives gcloud directly, the other drives daisy through a fetched Go
tool. Worth doing when a real sibling shows what actually varies.

The functional part of the move is REPO, which both scripts derive from their own
location to find hack/ and images/calico-go-build/versions.yaml; it is now two
levels up rather than one. Verified both resolve the helpers and versions files
from the new depth and still produce ci-base-1-27-1-llvm21-1-8-k8s1-37-0 and
gbp-1-27-1-llvm21-1-8-k8s1-37-0.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Second pass, comment-only: -175/+91 lines. The remaining multi-line blocks are
package docs and script usage headers, which earn the space; everything else is
one or two lines.

Mostly cut the historical narrative. A comment needs the fact that shapes the
code, not the story of how it was found -- "us-central1-a once took 85 minutes to
finish an insert" carries the reason for a per-zone timeout without three
sentences reconstructing the incident. Also dropped a duplicated prepull comment
in provision.sh that said the same thing twice in a row.

Density is now 10-25% across the tree, from 12-40%.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

It introduces confirmed command-injection risks in the SSH helper and an invalid GCE label value that would break gcloud compute images create.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

vm-images/ci-base/build-image.sh:123

  • The go-build-tag label value produced here is not a valid GCE label value: it starts with a digit and uses underscores. gcloud compute images create --labels requires label values to match [a-z]([-a-z0-9]*[a-z0-9])? (lowercase, digits, hyphens; must start with a letter), so this will fail at image-create time. Consider prefixing with a letter and normalizing to hyphens.
  • Files reviewed: 32/34 changed files
  • Comments generated: 8
  • Review effort level: Lite

Comment thread scratch-utils/gce/ssh.go Outdated
Comment thread scratch-utils/gce/ssh.go Outdated
Comment thread scratch-utils/gce/ssh.go
Comment thread scratch-utils/subcmd/runonvm/runonvm.go
Comment thread scratch-utils/subcmd/createvm/createvm.go
Comment thread preload-disks/go-build/README.md Outdated
Comment thread scratch-utils/subcmd/deletevm/deletevm.go Outdated
Comment thread vm-images/ci-base/README.md Outdated
Brian-McM and others added 4 commits September 9, 2026 09:55
kind.node_image becomes kind.node_images, a list, so the image can carry every
k8s version the rig tests against instead of one. build-image.sh joins the list
into KIND_NODE_IMAGES (space-separated -- env vars cannot hold arrays, and image
refs contain no spaces) and provision.sh appends them to the prepull list, which
is now built rather than a literal.

An empty list is rejected at build time: pre-pulling nothing would still produce
a working image, just one that silently loses the caching it exists for.

Unchanged: kind's own binary is still a single version, since only one can live
at /usr/local/bin/kind.

Verified the plural path with three entries and the guard with an empty list.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
go-build implied the disk only ever holds that image. It does not: CONTAINER_IMAGES
is a list, and the point of the disk is caching whatever a CI pool pulls often, so
the name should describe the disk rather than one of its contents.

Not "argo-cache" either -- nothing about it is Argo-specific.

The earlier objection to this name was mine and it was wrong. It assumed siblings
under preload-disks/ would each cache a different image, which would make
"ci-cache" a category rather than a discriminator. But there is one cache disk
holding N images, so the category IS the name.

Reframed the header comment and README to match: the default contents are
calico/go-build because that is what CI pulls today, not because the disk is for
go-build.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
290 lines of prose for five scripts that already explain themselves in their
headers. Removed.

Three things lived only in the preload README, so they moved into that script's
header rather than disappearing:

- the node-pool attach invocation, and that image streaming must be on;
- that a secondary boot disk can only be set at pool CREATE time, there being no
  update flag for it;
- the cross-project grant: roles/compute.imageUser for BOTH the cluster project's
  default compute SA and its container-engine-robot service agent. That one is
  worth eight lines because missing either fails NODE creation rather than pool
  creation, so it surfaces a long way from its cause.

Also fixed the five now-dangling "see README.md" pointers.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… sizes

From Copilot's second review. The quoting one is real: fmt %q emits a Go
double-quoted string, and bash still expands $(...) and backticks inside double
quotes, so PutData/PutDir/GetDir would execute a remote path like

  /tmp/x$(id -u)   ->   mkdir -p "/tmp/x$(id -u)"   ->   /tmp/x1000

Those paths come from --put/--put-env/--get, so the old comment claiming they
were "paths we control" was doing the work of an assumption rather than an
escape. Replaced with single-quote escaping.

runonvm's shellQuote moves to util.ShellQuote so gce can use it too -- one
implementation, tested once -- and its tests move with it. Added a test that
proves the point directly; mutation-checked by reverting to %q, which expands all
three payloads.

Also:

- runonvm validates --env NAMES. Values were quoted, but a name is interpolated
  bare into `export NAME=...`, so "FOO; rm -rf /" would run when the script
  sources the file.
- parseDiskGB rejects non-positive sizes. "0" and "-10GB" parsed fine and reached
  the GCE API, failing there instead of at the env var that caused it.
- deletevm's doc said any failure returns 0, but a missing VM_NAME exits 1. The
  behaviour is right -- a misconfigured cleanup step deleted nothing and is worth
  failing on -- so the comment now says that.

Not changed: Copilot also claimed the go-build-tag label value is invalid because
it starts with a digit and contains underscores. Those are label KEY rules; that
exact value is live on ci-base-1-27-1-llvm21-1-8-k8s1-37-0 today.

The preload image prefix becomes "cic" to match the ci-cache rename.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The VM image build/provisioning path currently risks producing “successful” images with broken tool installs (non-failing curl downloads and non-enforcing post-provision checks), and SetupComputeADC writes secrets to a predictable temp path.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details
  • Files reviewed: 32/34 changed files
  • Comments generated: 5
  • Review effort level: Lite

Comment thread scratch-utils/util/secret.go
Comment thread scratch-utils/gce/vm.go
Comment thread vm-images/ci-base/build-image.sh Outdated
Comment thread vm-images/ci-base/provision.sh Outdated
Comment thread scratch-utils/util/shell.go Outdated
Brian-McM and others added 5 commits September 9, 2026 12:07
…ction

From Copilot's third review. The download one is the serious one: provision.sh
fetched kind, kubectl and gh with `curl -sSL`, no -f. curl exits 0 on an HTTP
error and writes the error page to the output file, so a 404 installed a 9-byte
"Not Found" as /usr/local/bin/kind, chmod 0755, and the image built clean and
shipped broken. Only Go was protected, by its checksum. Verified both halves: the
old form writes the error page and exits 0; the new fetch() (-f, via retry) fails.

The post-provision check could not catch it either -- it ended in `|| true` and
piped through `head`. It now runs under `set -e` with no pipes, additionally
asserts the go-build image is in the docker cache, and aborts before snapshotting.
That also closes the silent pre-pull failure noted earlier: provision.sh warns
rather than failing on a pull, so this is what notices.

- isNotFound matched the substring "notFound" in an error message. Now it matches
  a googleapi 404. The old form read a permission error whose prose mentioned
  notFound as "already deleted", which would make cleanup skip a live VM.
- SetupComputeADC wrote the SA key to /tmp/compute-sa.json via os.WriteFile,
  which follows symlinks and collides between runs. Now os.CreateTemp: O_EXCL,
  random name, 0600.

Both new tests mutation-checked -- each fails when its fix is reverted.

Not changed: Copilot said gofmt does not alter comment typography. It does, for
DOC comments, since Go 1.19 -- verified on go1.27.0, where a doc comment's ''
became a curly quote and an identical comment inside a function body did not. The
comment now says doc comments specifically, which is what my wording had blurred.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The base was distroless/static:nonroot for two things it provides that scratch
does not, both verified as hard failures on a bare scratch image:

  CreateTemp: FAIL open /tmp/...: no such file or directory
  HTTPS:      FAIL x509: certificate signed by unknown authority

No trust store means every GCP API call fails, which is the whole job, and no
/tmp means SetupComputeADC cannot write the compute SA key.

Both are now prepared from UBI, the same way calico-base builds its own rootfs
for its scratch image: the cert bundle lands at the first path Go checks on
linux, and /tmp is created 1777. USER is numeric so no /etc/passwd is needed,
using the uid distroless nonroot used. Verified on the built image running as
65532: CreateTemp ok, HTTPS ok.

18.7MB -> 16.8MB. The binary is 16.5MB of that either way, so this is a small
win; the reason to prefer it is that the image now contains only what was put
there deliberately, and a future need for tzdata or nsswitch.conf shows up at
build time instead of being silently satisfied.

Image tag v0.4 -- the contents changed even though the binary did not.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The image tag came from a hand-bumped versions.yaml while the promotion fired on
change_in, which left two holes:

- Editing scratch-utils/ without bumping the version republished the SAME image
  tag with different contents. Docker tags are mutable and GCR has no
  immutability, so anything pinning v0.4 silently changed under it.
- change_in listed no go.mod/go.sum, so a dependency bump -- an x/crypto CVE,
  say -- rebuilt the binary but published nothing. The fix would not ship.

Adopt what calico-go-build already does, which closes both:

- master publishes the moving :master and :latest, so ANY change ships,
  including a bare go.mod bump that no path filter would have caught. Nothing
  pins these expecting immutability.
- a scratch-utils-v* tag publishes the immutable :vX.Y that workflows pin. The
  tag is created by a new GitHub Action when the version lands on master, so an
  image tag cannot be republished with new contents -- that needs a new version,
  which makes a new tag.

calico-binfmt avoids this by deriving its tag from the qemu version it packages.
scratch-utils packages our own code, so there is no upstream version to borrow,
which is why the hand-maintained number did not hold.

The tag is prefixed because this repo's release tags share a namespace with
1.27.1-llvm21.1.8-k8s1.37.0, and a second binary here would need its own; the
promotion strips the prefix so the image tag is v0.4, not scratch-utils-v0.4.

Deliberately not tied to the go-build release cadence: a Go CVE bump would
republish an identical binary under a new tag, and a scratch-utils fix has
nothing to do with a Go release.

Dry-run verified: a tag build pushes only :v0.4 and :v0.4-amd64; master pushes
only :master, :latest and their -amd64 forms; a missing BRANCH_NAME fails the
target rather than pushing something wrong.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Both images bake or cache calico/go-build, so rebuilding them belongs after that
image is published, not on a manual trigger someone has to remember. Moved from
the root pipeline into promotions/calico-go-build.yml, which is also the only
place they can be ordered: sibling promotions off the root pipeline run
independently, so a rebuild could start before the image it consumes exists.

Gated on the release tag rather than merely on that pipeline passing. It also
runs for master, where a rebuild would spend a builder VM per merge and, with no
tag set, generate-image-name.sh falls back to the branch -- producing the
throwaway ci-base-master / cic-master images rather than a release. The regex is
the one the parent promotion already uses; checked it matches 1.27.1-llvm...,
its -1 re-release form and an -rc.1-1 tag, and does not match master or
scratch-utils-v0.4.

Both stay promotable by hand from the Semaphore UI for a one-off rebuild.

Nested pipeline_file paths are written relative to .semaphore/, matching how the
root pipeline references these same files.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
auto_promote only governs AUTOMATIC promotion -- it does not stop anyone
promoting a pipeline by hand. Both blocks had no block-level `when`, so a manual
promote from any PR would have created a real GCE builder VM and published an
image named after the head branch.

Give them the gate calico-go-build's blocks already carry. On a PR the branch is
pull-request-N, so neither side matches and the block is skipped; master and a
release tag still run, which keeps the auto-promotion working and leaves a
deliberate manual rebuild from master available.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

@hjiawei hjiawei left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review of the scratch-utils / ci-base / preload-disk changes. Findings are inline. The one at .semaphore/promotions/calico-go-build.yml:41 is blocking: as written, neither new pipeline can be reached.

Comment thread .semaphore/promotions/calico-go-build.yml Outdated
- name: Build ci-base VM image
pipeline_file: promotions/ci-base-vm-image.yml
auto_promote:
when: "result = 'passed' AND tag =~ '^1\\.\\d+\\.\\d+-llvm\\d+\\.\\d\\.\\d-k8s1\\.\\d+\\.\\d+'"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

llvm\d+\.\d\.\d only matches a single-digit LLVM minor and patch. On a bump to llvm 21.1.10 the regex consumes llvm21.1.1 and then needs -k8s1. where the tag has 0-k8s1., so it stops matching.

Nothing errors when that happens: auto_promote just never fires, and the identical run.when gate in ci-base-vm-image.yml:48 / preload-disk-image.yml:54 also goes false, so even a manual promotion gives a green pipeline with the block skipped. The release ships with no ci-base image and no preload disk.

llvm\d+(\.\d+){2} fixes it. Four copies of this pattern now exist — worth a single anchor they all reference.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not that this isn't true, but I copied this verbatim from our other pipelines so we should probably fix those there too.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just fixed this for all instances of the regex in this PR

Comment thread scratch-utils/gce/vm.go
if err != nil {
return "", err
}
for scope, list := range agg.Items {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Go randomizes map iteration order, so returning on the first non-empty scope picks an arbitrary zone when the same VM name exists in two.

That isn't hypothetical here: PerZoneTimeout (3m) can leave an insert still RUNNING in an earlier zone whose best-effort DELETE is also still PENDING — the 85-minute us-central1-a case the comment at :55 describes — while Create succeeds in the next zone. Instance names are zone-scoped, so GCE happily holds both. deletevm with ZONE unset then deletes one at a coin flip and leaks the other, and runonvm with ZONE unset can SSH into the abandoned VM and run the job on a machine createvm never reported.

Collect every matching zone, then either fail loudly on >1 or act on all of them.

Comment thread scratch-utils/gce/ssh.go
if err := sess.Start(cmd); err != nil {
return err
}
if err := untar(stdout, localDir); err != nil {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This deadlocks. StdoutPipe's contract is that the reader must keep draining or the remote command blocks; on the untar error path the pipe is abandoned with the remote tar czf - . still writing. The SSH window fills, the remote tar blocks on write and never exits, and Wait never returns.

Reachable from the escapes dest guard, ENOSPC locally, or a permission error creating a target file. Because GetDir runs from runonvm's deferred --get loop, the hang happens after the user's script finished and reported its exit code — it looks like the job wedged at 100%.

io.Copy(io.Discard, stdout) or sess.Close() before Wait on the error path.

Comment thread scratch-utils/gce/ssh.go Outdated
deadline := time.Now().Add(3 * time.Minute)
var lastErr error
for time.Now().Before(deadline) {
client, err := ssh.Dial("tcp", addr, cfg)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ssh.ClientConfig.Timeout bounds only the TCP connect — ssh.Dial passes it to net.DialTimeout and then runs NewClientConn with no deadline on the conn.

The state this loop exists to ride out is exactly the one that hangs: a freshly-booted VM whose sshd has bound the socket but isn't ready to send its version string accepts the TCP connection and then stalls. ssh.Dial never returns, so neither the deadline check nor the ctx.Done() select below is ever reached, and runonvm's 10-minute setupCtx doesn't fire either. The step hangs until CI kills it.

Dial with net.DialTimeout, conn.SetDeadline, then ssh.NewClientConn — or race each attempt against ctx in a goroutine.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

Comment thread scratch-utils/subcmd/runonvm/runonvm.go Outdated
fmt.Fprintf(os.Stderr, "runonvm: upload script: %v\n", err)
return 1
}
runCmd := "bash " + remoteScript

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These two are the only places the remote paths go into a shell command unquoted — PutData runs the same value through util.ShellQuote one call earlier (ssh.go:187).

remoteScript is path.Join("/tmp", path.Base(script)) from caller-supplied --script or Argo's trailing arg. --script '/work/my step.sh' uploads to /tmp/my step.sh correctly (quoted) and then runs bash /tmp/my step.sh, which the remote shell splits in two: "No such file or directory" after every upload already succeeded, pointing at the wrong thing.

util.ShellQuote on both.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

Comment thread cmd/Makefile Outdated

# Unlike the single-file binaries above, this one spans ../scratch-utils/, so list
# every .go file it compiles -- otherwise an incremental build ships a stale binary.
SCRATCH_UTILS_SRCS = $(shell find scratch-utils ../scratch-utils -name '*.go')

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two things here:

= is recursive, so this find re-runs every time make expands the prerequisite list. := runs it once.

Bigger: build: now depends on bin/scratch-utils-$(ARCH), and build is a prerequisite of calico-go-build-image (images/Makefile:67) and calico-binfmt-image (:100). Every arch of the go-build image matrix now downloads and compiles google.golang.org/api, gRPC and the OTel stack — roughly 20 modules those images never contain — before it can start its own Dockerfile.

Give bin/scratch-utils-$(ARCH) its own target that only calico-scratch-utils-image depends on.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

Comment thread vm-images/ci-base/build-image.sh Outdated
log "creating image $IMAGE in family $FAMILY"
gcloud compute images create "$IMAGE" --project="$PROJECT" \
--source-disk="$BUILDER" --source-disk-zone="$ZONE" --family="$FAMILY" \
--labels="go-build-tag=$(echo "$GO_BUILD_IMAGE" | sed 's|.*:||; s|\.|_|g')"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment above says the label keeps the exact tag, and the PR description repeats it, but the sed rewrites every dot: the stored value is 1_27_1-llvm21_1_8-k8s1_37_0.

So gcloud compute images list --filter="labels.go-build-tag=1.27.1-llvm21.1.8-k8s1.37.0" returns nothing, which is the label's only stated purpose — the image name already carries the same munged form.

GCE label values allow dashes, so s|\.|-|g would at least match the name. Either way the comment should say what's actually stored.

return n, nil
}

func envOr(key, def string) string {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This same eight-line envOr is also in deletevm.go:67 and runonvm.go:225, and all three files already import scratch-utils/util — whose doc comment says it's the home for exactly this. The empty-vs-unset semantics here are the distinction LocalSecret and --put-env care about, so a change to it now has to land in three places or the subcommands quietly diverge.

Separately, createvm_test.go:51 hand-rolls containsStr; every other test file in the PR just uses strings.Contains.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

simplified.

Comment thread scratch-utils/gce/vm.go Outdated
}

// PerZoneTimeout bounds ONE zone's attempt, so a degraded zone can be skipped. A
// single deadline across the loop let one slow zone consume all of it and blamed

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment (and a few others) explains what the code used to do and why the old bug is gone, rather than the present state — that belongs in the commit message.

Same shape at deletevm.go:32 ("pointing ADC at a nonexistent /secrets path made every delete fail auth..."), createvm.go:63 ("a fixed cap silently starved the later zones..."), build-image.sh:106 ("This used to end in || true..."), and util/secret.go:61 + util/secret_test.go:71 ("The key used to go to a fixed /tmp path").

The forward-looking half of each is the part worth keeping — here, that the timeout is per-zone so a degraded zone can be skipped, and that 3 minutes is chosen because the failure mode is slowness.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I trimmed all the comments.

Brian-McM and others added 3 commits September 11, 2026 09:11
Blocking
--------
Nested pipeline_file resolves from the current pipeline's directory, not
.semaphore/, so promotions/ci-base-vm-image.yml from inside promotions/ pointed
at promotions/promotions/... Neither chained pipeline was reachable, by
auto-promote or by hand. Now bare filenames.

Silent failures
---------------
The release-tag regex used llvm\d+\.\d\.\d, which stops matching at llvm 21.1.10:
\d\.\d consumes "1.1" and then wants -k8s1. where the tag has "0-". Nothing
errors -- auto_promote never fires and the run.when gates go false, so the
pipeline is green, the block is skipped and the release ships with no ci-base
image and no cache disk. Widened to llvm\d+(\.\d+){2} in all 11 copies; 7 were
pre-existing on master for base/binfmt/go-build, which had the same latent bug.

provision.sh pulled the pre-pull list with `|| echo`, which neutralises set -e,
so a registry blip let the readiness marker be written and the image publish with
an empty cache -- visible only as jobs getting slower. Now fatal, every entry is
asserted present afterwards, and build-image.sh's verify covers the node images
and registry:2 rather than only go-build.

GOOGLE_VM_MAX_RUN_DURATION accepted "0s" and "-5m": instanceSpec then omits the
whole Scheduling block, so the VM got no reclaim deadline at all -- the backstop
deletevm relies on when it returns 0 on a failed cleanup. Extracted parseMaxRun
with the same positive check parseDiskGB already had, and tested it.

Hangs and wrong targets
-----------------------
GetDir deadlocked on the untar error path: StdoutPipe requires the reader to keep
reading, and the remote tar was left writing into a full window, so Wait never
returned. It runs from runonvm's deferred --get, after the job reported its exit
code, so it presented as a job wedged at 100%. Drains before waiting now.

ssh.Dial bounds only the TCP connect -- it passes Timeout to net.DialTimeout and
runs NewClientConn with no deadline (confirmed in x/crypto v0.55.0). A VM whose
sshd has bound the socket but is not answering accepts the connection and stalls,
which is precisely the state the retry loop exists for, and neither the deadline
check nor ctx.Done() was ever reached. Replaced with an explicit dial that sets a
handshake deadline and clears it before the session.

No keepalive was configured and x/crypto/ssh sends none, while GCP drops an idle
established flow after 10 minutes. Any quiet stretch in the job killed the
session and the artifact pull with it. Added a 60s keepalive for the connection's
life.

FindZone returned the first non-empty scope of a map, so with the same name in
two zones it picked arbitrarily. That state is reachable: a zone whose insert
outlived PerZoneTimeout keeps its VM while the next zone succeeds. Rather than
refuse outright -- which would break the job when only one VM is usable -- it now
prefers the live instance, falls back to a lone going-away one so deletevm can
still reap a TERMINATED VM holding its disk, and refuses only on real ambiguity.
Selection is factored into pickZone and covered by tests.

The external IP was read from the instance snapshot taken before SetMetadata and
its operation, so on a fresh VM natIP could still be empty -- a terminal error
where a re-read seconds later succeeds. Now read after the op, through retry,
with an errNotReady sentinel so the retry loop treats "not populated yet" as
worth another attempt.

runonvm ran `bash <remoteScript>` unquoted, the only remote path left unquoted
after the earlier pass -- a --script with a space split the command and reported
"No such file or directory" right after the upload succeeded.

Build path
----------
The builder VMs had no reclaim deadline of their own, unlike the ones createvm
makes, while the provision poll can run to ~40 minutes against a 30-minute
execution_time_limit. Added --max-run-duration=60m with DELETE, which holds even
when no trap runs, and widened the traps to EXIT INT TERM. Note the premise that
bash skips an EXIT trap on SIGTERM did not reproduce -- it ran -- so the
max-run-duration is the fix that matters here, not the trap.

Hygiene
-------
cmd/Makefile used = for the find, re-running it on every expansion, and put
bin/scratch-utils-$(ARCH) on `build`, which calico-go-build-image and
calico-binfmt-image depend on -- so every arch of those images compiled the GCP
SDK for a binary they do not contain. Split into scratch-utils-build, which only
calico-scratch-utils-image asks for; verified the go-build image build no longer
mentions it.

The go-build-tag label claimed to keep the exact tag but stored underscores, so a
filter on the real tag matched nothing -- the label's only purpose. Dots now
become dashes, matching the image name, and the comment says what is stored.

envOr was triplicated across the three subcommands that all already import util;
moved to util.EnvOr with the empty-means-absent semantics documented. Dropped a
hand-rolled containsStr for strings.Contains.

Trimmed six comments that narrated a fixed bug rather than the present state,
keeping the forward-looking half.

30 tests, green on Go 1.27.1 locally and in the pinned container. The new guards
were mutation-checked: each fails when its fix is reverted.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
FindZone preferred a live instance but fell back to a going-away one, which
conflated two different questions. A run step is about to SSH in and start a job:
a STOPPING or TERMINATED VM is not a slower answer, it is the wrong one, and
using it either fails confusingly or half-works.

Split by intent instead. FindLiveZone errors naming the state --
"vm-1 exists but is not usable (us-central1-a=STOPPING)" -- and runonvm uses it.
FindZone still takes either, preferring the live one, because deletevm wants to
reap a TERMINATED VM that is stopped but still holding its disk. Both share the
listing; only the pick differs.

Two live instances stay a refusal rather than a guess. Note STAGING counts as
live, so a wedged insert that has not reached STOPPING makes the pair ambiguous
rather than resolving toward the RUNNING one -- picking the other would be a
guess about a VM that may yet become the real one.

Mutation-checked: restoring the fallback fails both single-dying-instance cases
under requireLive.

Also comment-only: trimmed the blocks added for this PR down to the reason the
code is the way it is, dropping the restated mechanics and the incident
narratives. The Semaphore pipeline comments are removed outright.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The compute API's instance status enum lists both STOPPED ("has stopped
successfully") and TERMINATED ("has stopped, either by explicit action or
underlying failure"). goingAway covered only TERMINATED, so a STOPPED VM counted
as live and FindLiveZone would have handed runonvm a halted machine to SSH into.

GCE reports TERMINATED for a stopped instance today, which is why this had not
surfaced, but nothing about the API guarantees that stays true.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants