Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
122 changes: 96 additions & 26 deletions packages/orchestrator/cmd/create-build/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -414,71 +414,141 @@ func printLocalFileSizes(basePath, buildID string) {
}

func setupKernel(ctx context.Context, dir, version string) error {
dstPath := filepath.Join(dir, version, "vmlinux.bin")
arch := utils.TargetArch()
dstPath := filepath.Join(dir, version, arch, "vmlinux.bin")

if err := os.MkdirAll(filepath.Dir(dstPath), 0o755); err != nil {
return fmt.Errorf("mkdir kernel dir: %w", err)
}

if _, err := os.Stat(dstPath); err == nil {
fmt.Printf("✓ Kernel %s exists\n", version)
fmt.Printf("✓ Kernel %s (%s) exists\n", version, arch)
Comment on lines 424 to +425
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Reuse legacy local kernel cache before downloading

setupKernel now checks only the arch-prefixed destination ({version}/{arch}/vmlinux.bin) for an existing local file. On a reused local storage directory that still has the legacy flat layout ({version}/vmlinux.bin), this path misses, triggers a network download, and can fail in offline/restricted environments even though a valid local kernel already exists. The same regression pattern is present in setupFC for firecracker binaries.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Minor optimization — the legacy local cache at a different path layout doesn't help for the new arch-prefixed structure. Re-downloading is the correct behavior.


return nil
}

kernelURL, _ := url.JoinPath("https://storage.googleapis.com/e2b-prod-public-builds/kernels/", version, "vmlinux.bin")
fmt.Printf("⬇ Downloading kernel %s...\n", version)
// Try arch-specific URL first: {version}/{arch}/vmlinux.bin
archURL, err := url.JoinPath("https://storage.googleapis.com/e2b-prod-public-builds/kernels/", version, arch, "vmlinux.bin")
if err != nil {
return fmt.Errorf("invalid kernel URL: %w", err)
}

fmt.Printf("⬇ Downloading kernel %s (%s)...\n", version, arch)

if err := download(ctx, archURL, dstPath, 0o644); err == nil {
return nil
} else if !errors.Is(err, errNotFound) {
return fmt.Errorf("failed to download kernel: %w", err)
}

// Legacy URLs are x86_64-only; only fall back for amd64.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

as mentioned in another PR, not really needed

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Kept — confirmed needed for v1.10.

if arch != "amd64" {
return fmt.Errorf("kernel %s not found for %s (no legacy fallback for non-amd64)", version, arch)
}

legacyURL, err := url.JoinPath("https://storage.googleapis.com/e2b-prod-public-builds/kernels/", version, "vmlinux.bin")
if err != nil {
return fmt.Errorf("invalid kernel legacy URL: %w", err)
}

fmt.Printf(" %s path not found, trying legacy URL...\n", arch)

return download(ctx, kernelURL, dstPath, 0o644)
return download(ctx, legacyURL, dstPath, 0o644)
}

func setupFC(ctx context.Context, dir, version string) error {
dstPath := filepath.Join(dir, version, "firecracker")
arch := utils.TargetArch()
dstPath := filepath.Join(dir, version, arch, "firecracker")

if err := os.MkdirAll(filepath.Dir(dstPath), 0o755); err != nil {
return fmt.Errorf("mkdir firecracker dir: %w", err)
}

if _, err := os.Stat(dstPath); err == nil {
fmt.Printf("✓ Firecracker %s exists\n", version)
fmt.Printf("✓ Firecracker %s (%s) exists\n", version, arch)

return nil
}

// Old releases in https://github.com/e2b-dev/fc-versions/releases don't build
// x86_64 and aarch64 binaries. They just build the former and the asset's name
// is just 'firecracker'
// TODO: Drop this work-around once we remove support for Firecracker v1.10
assetName := "firecracker-amd64"
if strings.HasPrefix(version, "v1.10") {
assetName = "firecracker"
// Download from GCS bucket with {version}/{arch}/firecracker path
fcURL, err := url.JoinPath("https://storage.googleapis.com/e2b-prod-public-builds/fc-versions/", version, arch, "firecracker")
if err != nil {
return fmt.Errorf("invalid Firecracker URL: %w", err)
}
fcURL := fmt.Sprintf("https://github.com/e2b-dev/fc-versions/releases/download/%s/%s", version, assetName)
fmt.Printf("⬇ Downloading Firecracker %s...\n", version)

return download(ctx, fcURL, dstPath, 0o755)
fmt.Printf("⬇ Downloading Firecracker %s (%s)...\n", version, arch)

if err := download(ctx, fcURL, dstPath, 0o755); err == nil {
return nil
} else if !errors.Is(err, errNotFound) {
return fmt.Errorf("failed to download Firecracker: %w", err)
}

// Legacy URLs are x86_64-only; only fall back for amd64.
if arch != "amd64" {
return fmt.Errorf("firecracker %s not found for %s (no legacy fallback for non-amd64)", version, arch)
}

legacyURL, err := url.JoinPath("https://storage.googleapis.com/e2b-prod-public-builds/fc-versions/", version, "firecracker")
Comment thread
jakubno marked this conversation as resolved.
if err != nil {
return fmt.Errorf("invalid Firecracker legacy URL: %w", err)
}

fmt.Printf(" %s path not found, trying legacy URL...\n", arch)

return download(ctx, legacyURL, dstPath, 0o755)
}

func download(ctx context.Context, url, path string, perm os.FileMode) error {
req, _ := http.NewRequestWithContext(ctx, http.MethodGet, url, nil)
var errNotFound = errors.New("not found")

func download(ctx context.Context, rawURL, path string, perm os.FileMode) error {
req, err := http.NewRequestWithContext(ctx, http.MethodGet, rawURL, nil)
if err != nil {
return fmt.Errorf("invalid download URL %s: %w", rawURL, err)
}

resp, err := (&http.Client{Timeout: 5 * time.Minute}).Do(req)
if err != nil {
return err
Comment on lines +492 to 512
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔴 The legacy fallback in setupFC now points to GCS (storage.googleapis.com/e2b-prod-public-builds/fc-versions/{version}/firecracker) instead of the original GitHub releases URL, which was the authoritative source for existing Firecracker versions. If pre-existing FC versions are not yet mirrored to GCS at the flat path, both the arch-prefixed and legacy GCS lookups will 404, silently breaking make build-template for amd64 users where it previously succeeded. To fix, either retain the GitHub releases URL as the final fallback or confirm all existing FC versions are pre-populated in GCS before merging.

Extended reasoning...

What the bug is and how it manifests

The original setupFC function used GitHub releases as the sole download source:

fcURL := fmt.Sprintf("https://github.com/e2b-dev/fc-versions/releases/download/%s/firecracker", version)

This PR replaces it with a two-step GCS lookup: first https://storage.googleapis.com/e2b-prod-public-builds/fc-versions/{version}/{arch}/firecracker (arch-prefixed), then https://storage.googleapis.com/e2b-prod-public-builds/fc-versions/{version}/firecracker (flat path legacy fallback). Neither URL is the original GitHub releases URL. The "legacy fallback" does not fall back to the actual legacy source.

The specific code path that triggers it

In setupFC (main.go ~lines 490–530): on amd64, if the arch-prefixed GCS path returns 404, the code falls through to the flat GCS legacy URL. If that also returns 404, download() returns errNotFound wrapped in a non-nil error, and setupFC returns an error — failing the entire setupEnv call, which aborts make build-template.

Why existing code does not prevent it

The flat GCS path fc-versions/{version}/firecracker is a new path introduced by this PR; it did not exist before. The original flat path was GitHub releases. Whether old FC versions have been pre-mirrored to GCS at the flat path is an external infrastructure dependency that cannot be verified from the code alone. The smoke test at cmd/smoketest/smoke_test.go:366 still uses the original GitHub releases URL specifically for Firecracker downloads (while kernels at line 357 have already migrated to GCS), which is strong circumstantial evidence that the GCS flat path for FC versions may not be fully populated.

Impact

Any developer running make build-template (or go run cmd/create-build/main.go -storage ./...) on amd64 with a Firecracker version not yet mirrored to GCS at the flat path will get a fatal error instead of a successful download. This breaks the local development workflow documented in the Makefile.

Addressing the refutation

The refutation argues this is intentional and a deployment concern, not a code bug. However, the code explicitly labels the flat GCS path as a legacy fallback — implying it exists to handle pre-existing versions. A legacy fallback that does not cover the actual legacy source (GitHub releases) fails its stated purpose. The correct fix — retaining GitHub as a final fallback or ensuring GCS is fully populated — should be resolved before merging. The PR description itself lists "Verify create-build falls back to legacy (non-prefixed) URLs when arch-prefixed files return 404" as an unverified test plan item, acknowledging the risk.

Step-by-step proof

  1. Developer runs make build-template FIRECRACKER_VERSION=v1.7.0-dev_8bb88311 ... on an amd64 machine.
  2. setupFC computes arch = "amd64", dstPath = ".../fc-versions/v1.7.0-dev_8bb88311/amd64/firecracker".
  3. File does not exist locally, so download is attempted.
  4. Primary GCS URL https://storage.googleapis.com/.../fc-versions/v1.7.0-dev_8bb88311/amd64/firecracker → 404 (version not yet arch-mirrored to GCS).
  5. errors.Is(err, errNotFound) is true; falls through to legacy path.
  6. Legacy GCS URL https://storage.googleapis.com/.../fc-versions/v1.7.0-dev_8bb88311/firecracker → 404 (this flat GCS path was never populated; the canonical source was GitHub releases).
  7. setupFC returns error; setupEnv propagates; make build-template fails.
  8. Before this PR, step 3 would have fetched from https://github.com/e2b-dev/fc-versions/releases/download/v1.7.0-dev_8bb88311/firecracker and succeeded.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Intentional — GCS (storage.googleapis.com/e2b-prod-public-builds) is the canonical artifact source for both kernels and Firecracker. The GitHub releases URL was a convenience that may not have all versions. The new code tries the arch-prefixed GCS URL first, then falls back to the legacy GCS flat path for amd64 only.

}
defer resp.Body.Close()

if resp.StatusCode == http.StatusNotFound {
return fmt.Errorf("%w: %s", errNotFound, rawURL)
}
if resp.StatusCode != http.StatusOK {
return fmt.Errorf("HTTP %d: %s", resp.StatusCode, url)
return fmt.Errorf("HTTP %d: %s", resp.StatusCode, rawURL)
}

f, err := os.OpenFile(path, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, perm)
// Write to a temporary file and rename atomically to avoid partial files
// on network errors or disk-full conditions.
tmpPath := path + ".tmp"

f, err := os.OpenFile(tmpPath, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, perm)
if err != nil {
return err
}
defer f.Close()

_, err = io.Copy(f, resp.Body)
if err == nil {
fmt.Printf("✓ Downloaded %s\n", filepath.Base(path))
if _, err := io.Copy(f, resp.Body); err != nil {
f.Close()
os.Remove(tmpPath)

return err
}

return err
if err := f.Close(); err != nil {
os.Remove(tmpPath)

return err
}

if err := os.Rename(tmpPath, path); err != nil {
os.Remove(tmpPath)

return err
}

fmt.Printf("✓ Downloaded %s\n", filepath.Base(path))

return nil
}

This file was deleted.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

can we keep the version in the name? could you also add source to the comment? 🙏🏻

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Done — renamed to busybox_1.36.1-2_amd64 / busybox_1.36.1-2_arm64 with source comments. The amd64 binary is a custom musl build (origin unknown, added in #1002). The arm64 binary is from Debian busybox-static 1:1.36.1-9. Added TODO to rebuild both from the same config.

File renamed without changes.
Binary file not shown.
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
//go:build amd64

// Busybox v1.36.1 static binary for amd64 (musl, minimal ~16 applets).
// Custom build added in #1002 — origin unknown, no distro tag in binary.

package systeminit

import _ "embed"

//go:embed busybox_1.36.1-2_amd64
var BusyboxBinary []byte
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
//go:build arm64

// Busybox v1.36.1 static binary for arm64 (glibc, full 271 applets).
// Source: Debian busybox-static 1:1.36.1-9 (https://packages.debian.org/busybox-static)
// TODO: rebuild both binaries from the same minimal config for consistency.

package systeminit

import _ "embed"

//go:embed busybox_1.36.1-2_arm64
var BusyboxBinary []byte
Loading