Skip to content

Publish every architecture to one bucket per cloud - #29

Merged
artemnikitin merged 5 commits into
mainfrom
feat/single-bucket-arch-prefix
Aug 14, 2026
Merged

Publish every architecture to one bucket per cloud#29
artemnikitin merged 5 commits into
mainfrom
feat/single-bucket-arch-prefix

Conversation

@artemnikitin

@artemnikitin artemnikitin commented Aug 8, 2026

Copy link
Copy Markdown
Owner

Publishes both architectures to one bucket per cloud, under an <arch>/ key prefix, instead of a separate bucket per architecture.

<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 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.

Images now upload under an <arch>/ key prefix instead of to a separate
bucket per architecture. The Firework agent resolves that prefix from the
node it runs on, so one bucket per cloud serves every architecture and a
mixed-architecture fleet needs no extra configuration.

The prefix uses the Go architecture vocabulary that TARGET_PLATFORM
already carries, not the AWS x86_64 spelling the agent would never look
under, and an unrecognised platform is rejected rather than published
without a prefix.

This removes the per-architecture bucket resolution added for the amd64
default: with the architecture in the key there is nothing to resolve,
both matrix legs upload unconditionally, and the warning about an
architecture that publishes nothing no longer describes a reachable
state.

Because both architectures previously produced identical object names,
publishing them to one bucket only became safe once the key carried the
architecture. The agent change must therefore ship after this one;
docs/ci-pipeline.md documents the ordering and the one-time full
re-download at cutover.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
artemnikitin and others added 3 commits August 8, 2026 12:49
The backend cases run the Makefile directly and never see the workflow,
so the env wiring that gives each matrix leg its architecture had no
coverage.

That gap matters more now than it would have before: both architectures
publish to one bucket under identical object names, so an upload step
missing TARGET_PLATFORM makes the arm64 leg publish under amd64/ and
overwrite the amd64 images. Nothing fails until a node boots a guest
built for the wrong architecture.

Verified by removing the env line and watching the check fail.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The workflow assertion only proved the TARGET_PLATFORM key was present in
each upload step. Hardcoding the value passed the whole suite while
pinning both matrix legs to one architecture, which is exactly the
overwrite the check exists to catch. It now matches the matrix
expression, verified against that bypass.

push-images.sh no longer defaults TARGET_PLATFORM. build-arm64 sets it
inside its own recipe while the push targets read it from the make
environment, so `make build-arm64 && make push-s3` published arm64
images under amd64/ and overwrote them in the now-shared bucket. The
Makefile default is removed too: build-images.sh already defaults on its
own, so `make build` is unchanged while an unset push fails loudly. CI
always sets it explicitly.

The migration runbook now diffs the flat and prefixed object sets rather
than asking for a visual check, because a node serving a cached image
will not report a gap, and step 4 deletes the flat objects.

README.md still described architecture-specific buckets with opt-in
ARM64, contradicting AGENTS.md in the same change.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Collapsing the per-architecture bucket resolution removed the warning
that fired when a publishing run had no bucket configured. Without it,
main, scheduled, and forced builds stay green while uploading nothing.
A publishing run now fails outright when neither bucket variable is set,
and warns per provider when only one is.

The migration runbook is replaced with a short note about leftover
root-level objects. There are no deployments on the previous layout, and
the runbook had four defects that only mattered if someone followed it:
it never told operators to repoint nodes that used a separate ARM64
bucket, it verified only amd64 and only S3, it required every flat object
to have a prefixed counterpart while acknowledging some are intentionally
obsolete, and its listing parsed `PRE <prefix>/` rows into empty entries
that made the pre-deletion check unreliable.

The retained deletion snippet uses list-objects-v2 with a delimiter, so
it returns root-level keys only and cannot produce those empty entries.

Also corrects the claim that both bucket variables are exported for every
build: each upload step now exports only its own provider's bucket, so
the stated rationale for requiring an explicit backend argument no longer
described the workflow.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Three places still said CI exports both bucket variables. Each upload
step now exports only its own provider's bucket, so the reason to require
an explicit backend argument is a local run with both configured, not the
workflow.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@artemnikitin
artemnikitin marked this pull request as ready for review August 14, 2026 16:22
@artemnikitin
artemnikitin merged commit 564f9a2 into main Aug 14, 2026
3 checks passed
@artemnikitin
artemnikitin deleted the feat/single-bucket-arch-prefix branch August 14, 2026 16:23
artemnikitin added a commit to artemnikitin/firework-deployment-example that referenced this pull request Aug 14, 2026
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** — `aws s3 sync` preserves 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.
- **GCP** — `gcloud storage`'s `*` does **not** cross `/`, so
`gs://bucket/*.ext4` matched nothing. Under `set -euo pipefail` that
aborted the startup script *before* `/etc/firework/agent.yaml` was
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 -e` startup
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_architecture` claimed it must match
"the rootfs images in `s3_images_bucket_id`"
- `aws/README.md` — three mutually contradictory claims about whether
switching to Graviton means repointing the images bucket
- `packer/aws/README.md` — warned about pointing a node at "an arm64
rootfs bucket"
- `aws/terraform.tfvars.example` — the Graviton runbook

Under 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 the
`AMD64_IMAGES_BUCKET` placeholder), `gcp/variables.tf`,
`images-infra/gcp/variables.tf`, and the GCP tfvars example, per
`AGENTS.md`'s instruction to keep `iam-policies/` current.

## Not changed

No Terraform resource or variable *definitions*. `s3_images_bucket_id`
and `images_bucket_name` were already single-bucket variables. Node IAM
is already bucket-wide — `"${var.s3_images_bucket_arn}/*"`
(`aws/iam.tf:67`) and `roles/storage.objectViewer` (`gcp/iam.tf:5-9`) —
so key prefixes need no policy change.

The AWS *AMI* row in `iam-policies/aws/README.md` still 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 validate`
passes for both data-plane stacks. Both bootstrap templates pass `bash
-n` with Terraform interpolations stubbed. No orphaned variables left
behind by the removal.

---------

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
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>
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.

1 participant