Stop prefetching rootfs images during node bootstrap - #22
Merged
Conversation
This was referenced Aug 9, 2026
artemnikitin
force-pushed
the
docs/single-bucket-arch-prefix
branch
from
August 9, 2026 10:41
9a9492c to
091abf3
Compare
The images bucket is no longer architecture-specific. Objects are stored under an <arch>/ key prefix and the agent reads the prefix matching the node it runs on, so a bucket serves every architecture. No functional Terraform change: s3_images_bucket_id and images_bucket_name were already single-bucket variables, and node IAM is already bucket-wide, so key prefixes need no policy change. What changes is the documented contract. Two statements became wrong rather than merely incomplete: switching to Graviton no longer requires repointing s3_images_bucket_id at a separate arm64 bucket, and node_ami_architecture no longer has to be kept in sync with which bucket a node was given. Following the old runbook would now point a node at a bucket holding both architectures under prefixes it would not find. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The IAM policy docs and the GCP tfvars example still described a bucket per architecture. The JSON policies themselves needed no change: both clouds grant bucket-wide object access with no prefix conditions, so key prefixes are already covered. AGENTS.md asks for iam-policies/ to be kept current with changes, and these were the last places still telling a reader to provision or select an architecture-specific bucket. The AWS AMI row still says ARM64: that one is genuinely architecture-specific, since it describes building the Graviton node image rather than uploading rootfs images. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Node bootstrap prefetches rootfs images before the agent starts, and both providers assumed objects sat at the bucket root. With images stored under an <arch>/ prefix that assumption breaks a fresh deploy, not just an upgrade. On AWS, `aws s3 sync` preserves key structure and its `*` matches across `/`, so a node downloaded every architecture into nested directories the agent never reads, then the agent fetched its own copies on top. For the current tenant image set that is roughly 36 GiB of prefetch plus 18 GiB of agent cache against a 50 GB root volume. On GCP the opposite: `gcloud storage`'s `*` does not cross `/`, so gs://bucket/*.ext4 matched nothing. Under `set -euo pipefail` the failure aborted the startup script before the agent config was written, leaving a node that never enrolled. Both now derive the prefix from `uname -m` and flatten it into /var/lib/images, matching where the agent resolves images. The GCP copy is additionally best-effort: prefetch is an optimization, and the agent syncs images itself, so an empty prefix must not prevent a node from starting. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Neither cloud CLI writes Firework's write-token sidecars, so every prefetched image was downloaded a second time by the agent moments after it started. Bootstrap also pulled every image for the node's architecture, while the agent downloads only the images actually assigned to that node. The agent is simply the better place for this: it fetches the assigned subset and maintains the sidecars that let a restart skip unchanged objects. Both bucket variables are required by the data-plane stacks and are always written into the agent config, so image sync is never disabled and removing the prefetch cannot strand a node. This also retires the GCP startup hazard entirely rather than guarding it: there is no longer a copy that can match nothing and abort the script under `set -e` before the agent config is written. The architecture guidance is corrected alongside it. Three statements in the AWS data-plane guide disagreed with each other about whether switching to Graviton means repointing the images bucket, and the Packer guide still warned about aiming a node at an arm64 rootfs bucket. Under one bucket only the AMI, instance type, and architecture setting change. The claim that bootstrap and the agent both select via `uname -m` was also wrong in a way this commit makes moot: bootstrap no longer selects anything, and the agent uses its own binary's architecture. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
artemnikitin
force-pushed
the
docs/single-bucket-arch-prefix
branch
from
August 9, 2026 10:44
091abf3 to
739fcaa
Compare
artemnikitin
added a commit
to artemnikitin/firework-gitops-example
that referenced
this pull request
Aug 14, 2026
Publishes both architectures to **one bucket per cloud**, under an `<arch>/` key prefix, instead of a separate bucket per architecture. ```text <images-bucket>/ amd64/tenant-1-kibana-rootfs.ext4 arm64/tenant-1-kibana-rootfs.ext4 ``` The prefix uses the Go vocabulary (`amd64`, `arm64`) that `TARGET_PLATFORM` already carries — deliberately not the AWS `x86_64` spelling, which the agent would never look under. An unrecognised `TARGET_PLATFORM` is rejected rather than published without a prefix. ## What changed - `scripts/push-images.sh` — derives `TARGET_ARCH` from `TARGET_PLATFORM` and prefixes the destination key - `Makefile` — threads `TARGET_PLATFORM` through the `push-*` targets, and drops its own default so an unset push fails instead of assuming amd64 - `.github/workflows/build-images.yaml` — the per-arch bucket resolution step is gone; both legs upload unconditionally to `S3_IMAGES_BUCKET` / `GCS_IMAGES_BUCKET` - `scripts/test-push-images.sh` — asserts prefixed destinations, arm64-prefix, bad-platform, unset-platform, and that the workflow passes the **matrix expression** (not merely the key) to both upload steps (13 checks, all passing) - `docs/ci-pipeline.md`, `AGENTS.md` — rewritten bucket semantics plus a migration runbook ## This reverts much of #28 `S3_IMAGES_BUCKET_ARM64` / `_AMD64` and the "no bucket configured" `::warning` are removed. Two comments went with them because they became false — the arm64 leg now *does* get AWS credentials and *does* upload, and "publishing an architecture nobody consumes" is no longer a reachable state. That mechanism existed because both architectures produced identical object names, making one bucket unsafe. Putting the architecture in the key removes the reason for it. ## Why this is a correctness fix Under per-arch buckets, pointing a node at the wrong bucket gave a silent wrong-architecture boot — a guest kernel panic at init, which is how [the Aug 7 mismatch](https://github.com/artemnikitin/firework-deployment-example) presented. With the architecture in the key, a node can only address its own images. **Scoped after review:** a missing image fails loudly only on a node with no local copy — the agent falls back to a cached image at debug level, so an existing node stays quiet. So verify bucket contents directly rather than waiting for a node to report a gap. A freshly provisioned node has no cache and does fail loudly. It also makes a mixed-architecture fleet free: node configs carry no architecture, so one desired state serves both. ## Deploy order No live environments, so there is no migration and merge order is unconstrained. The only ordering that matters operationally: **publish images before deploying nodes**, because a fresh node has no image cache to fall back on. Companions: artemnikitin/firework#45 (agent resolves the prefix) and artemnikitin/firework-deployment-example#22 (node bootstrap picks its own prefix — a functional fix, found in review). **Storage impact:** the arm64 leg starts uploading for the first time, so images-bucket usage roughly doubles. CI IAM is bucket-wide on both clouds, so no policy change is needed. ## Restored safeguard (review finding) Collapsing the per-arch bucket resolution also removed the warning that fired when a publishing run had no bucket configured — leaving builds green while uploading nothing. A publishing run now **fails** when neither bucket variable is set, and warns per provider when only one is. ## Migration runbook removed (review finding) It had four defects that only mattered if followed: no step to repoint nodes off a separate ARM64 bucket, verification covering only amd64 and only S3, a requirement that every flat object have a prefixed counterpart while acknowledging some are intentionally obsolete, and a listing that parsed `PRE <prefix>/` rows into empty entries — making the pre-deletion "must be empty" check unreliable. With no deployments on the old layout, it's replaced by a short note on deleting inert root-level objects, using `list-objects-v2 --delimiter /` so those empty entries cannot occur. ## The failure mode this guards against Both architectures now publish to one bucket under **identical object names**. If an upload step loses its `TARGET_PLATFORM` env, the arm64 leg publishes under `amd64/` and overwrites the amd64 images — silently, until a node boots a guest built for the wrong architecture. The backend cases run the Makefile directly and never see the workflow, so that wiring had no coverage. Review caught that the first version of the guard only checked the *key* was present — hardcoding `TARGET_PLATFORM: linux/amd64` in all three steps passed the entire suite. It now matches `${{ matrix.target_platform }}`, verified against exactly that bypass. **A second footgun, same shape:** `make build-arm64` sets `TARGET_PLATFORM` inside its own recipe while `push-s3` read it from the make environment, so `make build-arm64 && make push-s3` silently published arm64 images under `amd64/`. `push-images.sh` now requires an explicit `TARGET_PLATFORM`; `make build` is unchanged because `build-images.sh` defaults on its own. ## Testing `bash scripts/test-push-images.sh` — 13/13 pass. `shellcheck` clean on both scripts. Workflow YAML parses and the step list is as intended. --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
artemnikitin
marked this pull request as ready for review
August 14, 2026 16:24
artemnikitin
added a commit
to artemnikitin/firework
that referenced
this pull request
Aug 14, 2026
Lets images for every architecture share **one bucket per cloud**, separated by an `<arch>/` key prefix, instead of requiring one bucket per architecture. The agent resolves the prefix from the node it runs on. ## The change Remote object key and local filename were the same string, both from `filepath.Base(svc.Image)` (`internal/imagesync/sync.go:63-64`). That coupling is what forced the *bucket* to be the architecture selector. Decoupling them is the whole change: ```go name := filepath.Base(path) s.syncOne(ctx, s.remoteKey(name), filepath.Join(s.imagesDir, name)) ``` `remoteKey` prepends `runtime.GOARCH`. Local layout is untouched. **The architecture is not configurable.** It comes from the running agent binary, which is built for its node. A YAML knob would reintroduce exactly the misconfiguration this removes. ## Why this is a correctness fix, not just ergonomics Today, pointing a node at the wrong bucket yields a silent wrong-architecture boot — a guest kernel panic at init with no indication of the cause. That has happened in this deployment before. With the architecture in the key, a node can only address its own images. **Scoped correctly after review:** a missing image fails loudly *only on a node with no local copy*. `syncOne` falls back to a cached image whenever an object is absent, logging at debug level, so an existing node stays quiet. That fallback predates this change. What is guaranteed unconditionally is that a node never reads another architecture's object — the cached file came from that node's own bucket, so serving it is not a cross-arch boot. ## Everything upstream stays architecture-neutral The enricher still derives `/var/lib/images/<tenant>-<service>-rootfs.ext4`, `writeVMConfig` still passes `svc.Image` through, and `/var/lib/images` keeps its flat layout. **One desired state serves a mixed-architecture fleet** — each node resolves the same logical image to its own build. Per-arch buckets could not do this without duplicated data-plane module instantiations. ## Kernel carve-out `ensureLocalKernelAlias` now matches on the key's basename. Without it a prefixed key never looks like `vmlinux-<maj>.<min>`, disabling the alias fallback. Verified as real coverage by reverting the fix: `TestSync_NotFoundInS3_ResolvesUnversionedKernelAlias` fails with `image testarch/vmlinux-5.10 not found in object storage`. **Scope, corrected after review:** this does not affect the current fleet. Packer saves the canonical two-part name (`packer/scripts/02a-download-kernel.sh:28-31`), so `os.Stat` succeeds first and the alias never fires. The fix is right for the library, but no node here depends on it — and for the same reason, the "republish `vmlinux-*` under `<arch>/`" note below is precautionary only: Packer bakes the kernel into the machine image and uploads nothing to the images bucket. ## Deploy order No live environments, so there is no migration and merge order is unconstrained. The only ordering that matters operationally: **publish images before deploying nodes** (artemnikitin/firework-gitops-example#29, then a `force_rebuild` dispatch). A fresh node has no image cache to fall back on. Companion: artemnikitin/firework-deployment-example#22 makes node bootstrap pick its own architecture prefix — a functional fix found in review, without which a fresh node either fills its root volume (AWS) or aborts startup before enrolling (GCP). There is deliberately **no fallback to the unprefixed key** — that would let an arm64 node fetch a flat (amd64) object. This is distinct from `syncOne`'s pre-existing fallback to an already-cached *local* file, which is per-node and cannot cross architectures. ## Testing `make fmt`, `make test`, `make test-race`, `make lint` all clean. Existing sync tests now run against prefixed keys; added coverage for architecture isolation, unprefixed local paths, and `runtime.GOARCH` derivation. Part of the multi-arch single-bucket work. Related: #44 (layered rootfs). --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Removes the rootfs image prefetch from node bootstrap and corrects architecture documentation that described the images bucket as architecture-specific.
Why bootstrap stopped prefetching
Node bootstrap prefetched images before the agent started. Two problems, both found in review:
It broke a fresh deploy under the new key layout (the original P1). With images under an
<arch>/prefix, the two providers failed in opposite directions:aws s3 syncpreserves key structure and its*matches across/, so a node downloaded every architecture into nested directories the agent never reads: ~36 GiB of prefetch plus ~18 GiB of agent cache against a 50 GB root volume.gcloud storage's*does not cross/, sogs://bucket/*.ext4matched nothing. Underset -euo pipefailthat aborted the startup script before/etc/firework/agent.yamlwas written, leaving a node that never enrolled.And the prefetch was never useful. Neither cloud CLI writes Firework's write-token sidecars, so the agent re-downloaded every prefetched image moments later. Bootstrap also fetched every image for the architecture, while the agent fetches only the images assigned to that node.
So rather than teach bootstrap to select a prefix, this removes the step. The agent is the right place: assigned subset only, and it maintains the sidecars that let a restart skip unchanged objects. Both bucket variables are required by the data-plane stacks and always written into the agent config, so image sync is never disabled and removal cannot strand a node. It also retires the GCP
set -estartup hazard outright instead of guarding it.Documentation
The images bucket is no longer architecture-specific — objects live under an
<arch>/prefix and the agent resolves the prefix from its own binary's architecture.Statements that became wrong rather than merely incomplete:
aws/variables.tf—node_ami_architectureclaimed it must match "the rootfs images ins3_images_bucket_id"aws/README.md— three mutually contradictory claims about whether switching to Graviton means repointing the images bucketpacker/aws/README.md— warned about pointing a node at "an arm64 rootfs bucket"aws/terraform.tfvars.example— the Graviton runbookUnder one bucket, only the AMI, instance type, and architecture setting change.
Also swept
iam-policies/aws/README.md,iam-policies/gcp/01-ci-images-gcs.md(including theAMD64_IMAGES_BUCKETplaceholder),gcp/variables.tf,images-infra/gcp/variables.tf, and the GCP tfvars example, perAGENTS.md's instruction to keepiam-policies/current.Not changed
No Terraform resource or variable definitions.
s3_images_bucket_idandimages_bucket_namewere already single-bucket variables. Node IAM is already bucket-wide —"${var.s3_images_bucket_arn}/*"(aws/iam.tf:67) androles/storage.objectViewer(gcp/iam.tf:5-9) — so key prefixes need no policy change.The AWS AMI row in
iam-policies/aws/README.mdstill says ARM64: that one is genuinely architecture-specific, describing the Graviton node image build rather than rootfs uploads.Companions
artemnikitin/firework-gitops-example#29 (publishes under
<arch>/) and artemnikitin/firework#45 (agent resolves the prefix). No live environments, so merge order is unconstrained — but publish images before deploying nodes, since a fresh node has no cache to fall back on.Testing
terraform fmt -check -recursive terraform/clean;terraform validatepasses for both data-plane stacks. Both bootstrap templates passbash -nwith Terraform interpolations stubbed. No orphaned variables left behind by the removal.