-
Notifications
You must be signed in to change notification settings - Fork 308
feat: arch-aware downloads in create-build and fetch-busybox target #2260
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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) | ||
|
|
||
| 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. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. as mentioned in another PR, not really needed
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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") | ||
|
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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 The legacy fallback in Extended reasoning...What the bug is and how it manifests The original 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 The specific code path that triggers it In Why existing code does not prevent it The flat GCS path Impact Any developer running 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
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? 🙏🏻
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done — renamed to |
| 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
setupKernelnow 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 insetupFCfor firecracker binaries.Useful? React with 👍 / 👎.
There was a problem hiding this comment.
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.