fix: Phase 0 — Critical bug fixes + provisioner resilience + E2E tests#622
Merged
ArangoGutierrez merged 1 commit intoFeb 9, 2026
Merged
Conversation
There was a problem hiding this comment.
Pull request overview
Phase 0 hardening pass: fixes a few critical provisioning/AWS bugs, improves apt/package/driver installation resilience, and introduces a real AWS-backed E2E suite.
Changes:
- Add Ginkgo/Gomega AWS E2E tests + test environment fixtures.
- Improve provisioner robustness (retry logic, driver install fallback chain, fix duplicate template execution, safer SSH output handling).
- Tighten file permissions for cached state and kubeconfig output; address AWS storage size mutation and extend termination waiter.
Reviewed changes
Copilot reviewed 17 out of 632 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/e2e_test.go | Adds Ginkgo suite bootstrap + env initialization for E2E runs |
| tests/aws_test.go | Implements real AWS lifecycle E2E coverage (create → provision → validate → cleanup) |
| tests/common/common.go | Adds helper for generating unique IDs for E2E resource names |
| tests/data/test_aws.yml | E2E environment fixture for containerd + kubeadm + K8s |
| tests/data/test_aws_docker.yml | E2E environment fixture for Docker + NVIDIA runtime stack |
| pkg/provisioner/templates/nv-driver.go | Adds arch detection + multi-stage driver fallback strategy |
| pkg/provisioner/templates/docker.go | Wraps apt-get update with retry |
| pkg/provisioner/templates/containerd.go | Aligns to “latest” default behavior + improves install steps |
| pkg/provisioner/templates/container-toolkit.go | Adds retry install + fixes duplicate template execution |
| pkg/provisioner/templates/common.go | Makes apt installs resilient via broader retries + dpkg repair steps |
| pkg/provisioner/provisioner.go | Replaces log.Fatalf in goroutine with error propagation + sync |
| pkg/provider/aws/status.go | Removes redundant file creation; tightens cache file permissions |
| pkg/provider/aws/delete.go | Extends EC2 termination wait timeout |
| pkg/provider/aws/create.go | Removes global storageSizeGB pointer mutation by using local variable |
| cmd/cli/create/create.go | Writes kubeconfig with 0600 permissions |
| cmd/action/ci/entrypoint.go | Writes kubeconfig with 0600 permissions |
| go.mod | Updates Go version + adds E2E test dependencies |
Comment on lines
+36
to
+41
| func TestMain(t *testing.T) { | ||
| suiteName := "E2E Holodeck" | ||
|
|
||
| RegisterFailHandler(Fail) | ||
| RunSpecs(t, suiteName) | ||
| } |
There was a problem hiding this comment.
TestMain has a special meaning in Go (func TestMain(m *testing.M)), and using that name for a normal test function is confusing. Consider renaming this to something like TestE2E/TestE2ESuite to avoid ambiguity and make intent clearer.
| GinkgoWriter.Println("=== Starting test:", config.name, "===") | ||
|
|
||
| uniqueID := common.GenerateUID() | ||
| artifactDir := filepath.Join(LogArtifactDir, config.name+"-"+uniqueID) |
There was a problem hiding this comment.
If LOG_ARTIFACT_DIR is unset/empty, filepath.Join("", ...) will create artifacts relative to the repo working directory, which can dirty the workspace in CI/dev runs. Consider defaulting LogArtifactDir to a temp dir (e.g., os.MkdirTemp("", "holodeck-e2e-*")) when the env var is empty, and cleaning it up on success.
Suggested change
| artifactDir := filepath.Join(LogArtifactDir, config.name+"-"+uniqueID) | |
| logArtifactDir := LogArtifactDir | |
| if logArtifactDir == "" { | |
| tmpDir, err := os.MkdirTemp("", "holodeck-e2e-*") | |
| Expect(err).NotTo(HaveOccurred(), "Failed to create temporary artifact directory") | |
| logArtifactDir = tmpDir | |
| defer func() { | |
| // only clean up temporary artifacts on successful test runs | |
| if CurrentSpecReport().Failed() { | |
| return | |
| } | |
| err := os.RemoveAll(tmpDir) | |
| Expect(err).NotTo(HaveOccurred(), "Failed to clean up temporary artifact directory") | |
| }() | |
| } | |
| artifactDir := filepath.Join(logArtifactDir, config.name+"-"+uniqueID) |
Collaborator
Author
There was a problem hiding this comment.
In CI, LOG_ARTIFACT_DIR is always set. For local runs, the relative path behavior is intentional — it keeps artifacts visible for debugging. Adding temp dir logic would hide artifacts by default, making local E2E debugging harder.
Comment on lines
+22
to
+31
| "log" | ||
| ) | ||
|
|
||
| func GenerateUID() string { | ||
| b := make([]byte, 4) | ||
| _, err := rand.Read(b) | ||
| if err != nil { | ||
| log.Fatalf("failed to generate secure random UID: %v", err) | ||
| } | ||
| return hex.EncodeToString(b) |
There was a problem hiding this comment.
Calling log.Fatalf in a helper makes failures unrecoverable and can skip Ginkgo cleanup (leaking AWS resources). Prefer returning (string, error) and failing the spec at the call site (e.g., with Expect(err).NotTo(HaveOccurred())), so defers/cleanup can still run.
Suggested change
| "log" | |
| ) | |
| func GenerateUID() string { | |
| b := make([]byte, 4) | |
| _, err := rand.Read(b) | |
| if err != nil { | |
| log.Fatalf("failed to generate secure random UID: %v", err) | |
| } | |
| return hex.EncodeToString(b) | |
| "fmt" | |
| ) | |
| func GenerateUID() (string, error) { | |
| b := make([]byte, 4) | |
| if _, err := rand.Read(b); err != nil { | |
| return "", fmt.Errorf("failed to generate secure random UID: %w", err) | |
| } | |
| return hex.EncodeToString(b), nil |
| type: g4dn.xlarge | ||
| region: us-west-1 | ||
| ingressIpRanges: | ||
| - "0.0.0.0/0" |
There was a problem hiding this comment.
Opening ingress to 0.0.0.0/0 in E2E fixtures is risky for real AWS runs (exposes SSH to the internet). Consider restricting this to the CI runner’s public IP (passed via env/CI metadata) or making the allowed CIDR configurable with a safe default.
Suggested change
| - "0.0.0.0/0" | |
| - "203.0.113.0/24" |
Collaborator
Author
There was a problem hiding this comment.
These are ephemeral E2E security groups created and destroyed within a single test run (~10 min). The instances use short-lived, scoped credentials in isolated test accounts. Restricting to runner IP adds complexity (runner IP detection varies by CI provider) for minimal security gain given the short-lived nature. Added a comment documenting this rationale in e3a4ec1.
| } | ||
|
|
||
| return nil | ||
| return os.WriteFile(cachePath, data, 0600) |
There was a problem hiding this comment.
os.WriteFile(..., 0600) only applies the permission bits on file creation; if the cache file already exists with broader permissions from previous runs, its mode may remain unchanged. Consider explicitly enforcing file mode after writing (e.g., os.Chmod(cachePath, 0600)) or writing to a temp file with 0600 and atomically renaming it.
Suggested change
| return os.WriteFile(cachePath, data, 0600) | |
| if err := os.WriteFile(cachePath, data, 0600); err != nil { | |
| return err | |
| } | |
| return os.Chmod(cachePath, 0600) |
Comment on lines
68
to
71
| }, 10*time.Minute, waiterOptions...); err != nil { | ||
| a.fail() | ||
| return fmt.Errorf("error waiting for instance to be terminated: %v", err) | ||
| } |
There was a problem hiding this comment.
This returns a newly formatted error but drops the original error for callers that want to unwrap/inspect it. Prefer wrapping with %w (and keep error strings consistent with the rest of the PR’s failed to ...: %w style).
Comment on lines
243
to
246
| localFile, err := os.OpenFile(opts.kubeconfig, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0600) | ||
| if err != nil { | ||
| return fmt.Errorf("error creating local file: %v", err) | ||
| } |
There was a problem hiding this comment.
The operation is no longer strictly “creating” the file (it may also truncate an existing file). Consider updating the message to reflect open/write intent (and wrap with %w for consistency).
ArangoGutierrez
added a commit
to ArangoGutierrez/holodeck
that referenced
this pull request
Feb 7, 2026
- Rename TestMain to TestE2E to avoid Go testing convention conflict - Change GenerateUID to return (string, error) with 16 bytes entropy, replacing log.Fatalf that bypassed Ginkgo cleanup - Add os.Chmod after os.WriteFile for cache file permission enforcement - Replace %v with %w in error wrapping for proper error chain propagation - Update error message in create.go to reflect OpenFile intent - Add documentation comments to E2E test fixtures explaining 0.0.0.0/0 ingress rationale (ephemeral security groups, isolated test accounts) Signed-off-by: Carlos Eduardo Arango Gutierrez <eduardoa@nvidia.com> Co-authored-by: Cursor <cursoragent@cursor.com>
ArangoGutierrez
force-pushed
the
fix/phase0-critical-bugs
branch
from
February 9, 2026 13:11
e3a4ec1 to
cd6f624
Compare
Pull Request Test Coverage Report for Build 21830413696Details
💛 - Coveralls |
ArangoGutierrez
enabled auto-merge (squash)
February 9, 2026 13:29
ArangoGutierrez
force-pushed
the
fix/phase0-critical-bugs
branch
3 times, most recently
from
February 9, 2026 14:34
964f55d to
9dc7a41
Compare
…e permissions - Use local variable for storageSizeGB instead of taking address of package-level var, preventing data race and pointer aliasing with AWS SDK - Replace log.Fatalf in provisioner goroutine with proper error propagation via channel and sync.WaitGroup, preventing process crash and goroutine leaks - Simplify status.go update() to MkdirAll + WriteFile + Chmod(0600), removing redundant os.Create(0666) that left cache files world-readable Re-implemented against current upstream/main (a19ea84). Signed-off-by: Carlos Eduardo Arango Gutierrez <eduardoa@nvidia.com> Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: Carlos Eduardo Arango Gutierrez <eduardoa@nvidia.com> Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: Carlos Eduardo Arango Gutierrez <eduardoa@nvidia.com> Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: Carlos Eduardo Arango Gutierrez <eduardoa@nvidia.com> Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: Carlos Eduardo Arango Gutierrez <eduardoa@nvidia.com> Co-authored-by: Cursor <cursoragent@cursor.com>
ArangoGutierrez
force-pushed
the
fix/phase0-critical-bugs
branch
from
February 9, 2026 15:07
9dc7a41 to
4f550ee
Compare
ArangoGutierrez
added a commit
to ArangoGutierrez/holodeck
that referenced
this pull request
Feb 10, 2026
…e permissions (NVIDIA#622) - Use local variable for storageSizeGB instead of taking address of package-level var, preventing data race and pointer aliasing with AWS SDK - Replace log.Fatalf in provisioner goroutine with proper error propagation via channel and sync.WaitGroup, preventing process crash and goroutine leaks - Simplify status.go update() to MkdirAll + WriteFile + Chmod(0600), removing redundant os.Create(0666) that left cache files world-readable Re-implemented against current upstream/main (a19ea84). Signed-off-by: Carlos Eduardo Arango Gutierrez <eduardoa@nvidia.com> Co-authored-by: Cursor <cursoragent@cursor.com>
ArangoGutierrez
added a commit
to ArangoGutierrez/holodeck
that referenced
this pull request
Feb 10, 2026
…e permissions (NVIDIA#622) - Use local variable for storageSizeGB instead of taking address of package-level var, preventing data race and pointer aliasing with AWS SDK - Replace log.Fatalf in provisioner goroutine with proper error propagation via channel and sync.WaitGroup, preventing process crash and goroutine leaks - Simplify status.go update() to MkdirAll + WriteFile + Chmod(0600), removing redundant os.Create(0666) that left cache files world-readable Re-implemented against current upstream/main (a19ea84). Signed-off-by: Carlos Eduardo Arango Gutierrez <eduardoa@nvidia.com> Co-authored-by: Cursor <cursoragent@cursor.com>
ArangoGutierrez
added a commit
to ArangoGutierrez/holodeck
that referenced
this pull request
Feb 10, 2026
…e permissions (NVIDIA#622) - Use local variable for storageSizeGB instead of taking address of package-level var, preventing data race and pointer aliasing with AWS SDK - Replace log.Fatalf in provisioner goroutine with proper error propagation via channel and sync.WaitGroup, preventing process crash and goroutine leaks - Simplify status.go update() to MkdirAll + WriteFile + Chmod(0600), removing redundant os.Create(0666) that left cache files world-readable Re-implemented against current upstream/main (a19ea84). Signed-off-by: Carlos Eduardo Arango Gutierrez <eduardoa@nvidia.com> Co-authored-by: Cursor <cursoragent@cursor.com>
ArangoGutierrez
added a commit
to ArangoGutierrez/holodeck
that referenced
this pull request
Feb 12, 2026
…e permissions (NVIDIA#622) - Use local variable for storageSizeGB instead of taking address of package-level var, preventing data race and pointer aliasing with AWS SDK - Replace log.Fatalf in provisioner goroutine with proper error propagation via channel and sync.WaitGroup, preventing process crash and goroutine leaks - Simplify status.go update() to MkdirAll + WriteFile + Chmod(0600), removing redundant os.Create(0666) that left cache files world-readable Re-implemented against current upstream/main (a19ea84). Signed-off-by: Carlos Eduardo Arango Gutierrez <eduardoa@nvidia.com> Co-authored-by: Cursor <cursoragent@cursor.com>
ArangoGutierrez
added a commit
to ArangoGutierrez/holodeck
that referenced
this pull request
Feb 13, 2026
…e permissions (NVIDIA#622) - Use local variable for storageSizeGB instead of taking address of package-level var, preventing data race and pointer aliasing with AWS SDK - Replace log.Fatalf in provisioner goroutine with proper error propagation via channel and sync.WaitGroup, preventing process crash and goroutine leaks - Simplify status.go update() to MkdirAll + WriteFile + Chmod(0600), removing redundant os.Create(0666) that left cache files world-readable Re-implemented against current upstream/main (a19ea84). Signed-off-by: Carlos Eduardo Arango Gutierrez <eduardoa@nvidia.com> Co-authored-by: Cursor <cursoragent@cursor.com>
ArangoGutierrez
added a commit
to ArangoGutierrez/holodeck
that referenced
this pull request
Feb 13, 2026
…e permissions (NVIDIA#622) - Use local variable for storageSizeGB instead of taking address of package-level var, preventing data race and pointer aliasing with AWS SDK - Replace log.Fatalf in provisioner goroutine with proper error propagation via channel and sync.WaitGroup, preventing process crash and goroutine leaks - Simplify status.go update() to MkdirAll + WriteFile + Chmod(0600), removing redundant os.Create(0666) that left cache files world-readable Re-implemented against current upstream/main (a19ea84). Signed-off-by: Carlos Eduardo Arango Gutierrez <eduardoa@nvidia.com> Co-authored-by: Cursor <cursoragent@cursor.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.
Summary
Phase 0 of the 360-degree codebase evaluation. Fixes critical bugs and hardens provisioning infrastructure.
Critical Bug Fixes
Provisioner Resilience
E2E Test Suite (NEW)
Test Plan
go vet ./...passes-s -S(DCO + SSH)Made with Cursor