e2e: testing qemu provisioner + driver with jumpstarter-exec - #929
e2e: testing qemu provisioner + driver with jumpstarter-exec#929mangelajo wants to merge 8 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR changes QEMU Pods to use an exporter main container and native runtime sidecar. It adds runtime shutdown, terminal exporter recycling, image deployment wiring, shared e2e helpers, and ExporterSet QEMU tests. ChangesQEMU ExporterSet lifecycle
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
mangelajo
changed the title
Make the exporter the main container and jumpstarter-exec/runtime a native sidecar so default logs and Pod completion follow the exporter. Add jumpstarter-exec shutdown, terminal-Pod cleanup for ExitAndReplace, and ExporterSet QEMU coverage in the main e2e suite (flash/boot gated on #924). Co-authored-by: Cursor <cursoragent@cursor.com>
QEMU runs in the runtime container via jumpstarter-exec, so cloud-init vvfat content must live under /shared rather than the exporter's /tmp. Co-authored-by: Cursor <cursoragent@cursor.com>
… responsive Exercise j qemu power on/off under jmp shell, verify ExitAndReplace rotation, and chmod cidata dirs so the runtime sidecar can read them across UIDs. Co-authored-by: Cursor <cursoragent@cursor.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (2)
e2e/scripts/ensure-qemu-guest-image.sh (1)
42-46: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winHarden the download: timeouts, partial cleanup, and integrity check.
curlhas no--max-time/--connect-timeout, so a stalled CDN can hang the job, the.partialfile leaks on failure, and the "pinned" image is unverified.♻️ Suggested hardening
echo "Downloading Alpine guest image from ${ALPINE_IMAGE_URL}" >&2 tmp="${DEST}.partial" -curl -fL --retry 3 --retry-delay 2 -o "${tmp}" "${ALPINE_IMAGE_URL}" +trap 'rm -f "${tmp}"' EXIT +curl -fL --retry 3 --retry-delay 2 --connect-timeout 20 --max-time 900 \ + -o "${tmp}" "${ALPINE_IMAGE_URL}" +if [ -n "${JUMPSTARTER_E2E_QEMU_IMAGE_SHA256:-}" ]; then + echo "${JUMPSTARTER_E2E_QEMU_IMAGE_SHA256} ${tmp}" | sha256sum -c - +fi mv "${tmp}" "${DEST}"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/scripts/ensure-qemu-guest-image.sh` around lines 42 - 46, Harden the download flow around ALPINE_IMAGE_URL by adding curl connection and total-transfer timeouts, ensuring the temporary ${DEST}.partial file is removed whenever curl or the subsequent move fails, and verifying the downloaded image against the expected pinned checksum before moving it to DEST. Preserve the existing retry behavior and only output DEST after successful verification and installation.e2e/test/exporterset_qemu_test.go (1)
105-116: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReadiness assertion is weak;
ContainSubstring("true")passes when some containers are not ready.
containerStatuses[*].readyyields a space-joined list (e.g.true false). Preferkubectl wait --for=condition=Ready pod/<name>, which also fixes the "waiting for Pod Ready" label that currently only checks phaseRunning. Same pattern at Lines 205-208.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/test/exporterset_qemu_test.go` around lines 105 - 116, Replace the container readiness assertion in the pod-waiting flow with kubectl’s pod Ready condition check, using the existing exporterName and namespace arguments via the established Kubectl/Eventually pattern. Update the preceding phase-based wait and its label so the readiness wait uses `kubectl wait --for=condition=Ready pod/<name>`, and apply the same change to the corresponding flow around the second readiness assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/e2e.yaml:
- Around line 286-292: Increase the GitHub Actions job timeout in
`.github/workflows/e2e.yaml` at lines 286-292 so it exceeds the 90-minute Ginkgo
suite timeout and includes setup and log collection time. Alternatively, reduce
the full-suite Ginkgo timeout in `.github/workflows/e2e.yaml` lines 286-292 and
`e2e/lib/common.sh` lines 74-82 to preserve equivalent buffer; update both
affected sites consistently if choosing this approach.
- Around line 189-200: Update the cache key for the exporter image near the
“Cache exporter image” step to hash the complete exporter build context,
including Python package source files, rather than only configuration and lock
files. Also update the QEMU-runtime cache key near the corresponding QEMU
runtime cache step to include the complete runtime build context, including
runtime and shutdown implementation inputs; apply the required changes at
.github/workflows/e2e.yaml lines 189-200 and 223-234.
In `@e2e/scripts/ensure-qemu-guest-image.sh`:
- Around line 4-8: Update the resolution-order header in
ensure-qemu-guest-image.sh to state that JUMPSTARTER_E2E_QEMU_IMAGE accepts only
an absolute path, removing the unsupported “or URL” claim while preserving the
existing behavior.
In `@e2e/scripts/qemu_flash_boot.py`:
- Around line 20-21: Update the documented invocation in the jmp shell command
to use uv run python3 instead of the system Python interpreter, and note that
users should run make sync first when dependencies are not installed.
- Around line 1-109: Move the executable script currently defining main() into
the repository’s python/ workspace, preserving its CLI behavior and executable
entry point. Update any callers, references, or invocation documentation to use
the new location, and ensure the relocated module remains exposed through the
Python workspace conventions.
- Around line 48-64: Update the positional image argument in the argument parser
so it is optional, allowing invocations with --skip-flash and no image path.
Preserve the existing image value and flashing behavior when an image is
provided, while keeping the skip-flash path independent of args.image.
In `@e2e/test/exporterset_qemu_test.go`:
- Around line 120-135: Update the shared volume capacity check in the
exporterSetQemu test to query the sizeLimit once, then branch on that result.
Remove the preceding Eventually assertion requiring a non-empty value so the
existing Skip behavior for an empty sizeLimit or 100Mi remains reachable.
In
`@python/packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver_test.py`:
- Around line 221-234: Update
test_cidata_uses_shared_work_dir_with_launcher_socket to rely on the
launcher_socket path directly: keep the socket under shared, remove the
Qemu._work_dir patch, and call driver.cidata() unchanged. Preserve assertions
that cidata resides under shared with the expected permissions and files.
In `@python/packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver.py`:
- Around line 540-548: Update the cidata method so the TemporaryDirectory
remains 0o700 in local mode and is chmodded to 0o755 only when sidecar mode
requires cross-UID access. Use the existing sidecar-mode configuration or helper
visible in the surrounding driver code to guard the chmod, while preserving the
shared _work_dir behavior.
In `@python/packages/jumpstarter/jumpstarter/exporter/exporter.py`:
- Around line 44-105: Offload the blocking shutdown_runtime_sidecar invocation
from serve() to a worker thread using the existing async/AnyIO thread-offloading
mechanism, at both shutdown call sites around the task-group exit. Preserve the
function’s arguments, return handling, and shutdown sequencing while ensuring
subprocess.run never executes directly on the event loop.
- Around line 85-92: Update the subprocess.run exception handling in
shutdown_runtime_sidecar to catch PermissionError and other relevant OSError
failures, log the shutdown failure with the executable context, and return False
consistently with the existing FileNotFoundError and TimeoutExpired paths.
In `@rust/jumpstarter-exec/tests/integration.rs`:
- Around line 452-458: Update the server shutdown handling around server.wait()
to enforce a deadline: repeatedly poll server.try_wait(), preserving the
successful exit assertion when the process exits, and kill the child if the
deadline expires before it terminates. Ensure the timeout path fails the test
rather than blocking indefinitely.
---
Nitpick comments:
In `@e2e/scripts/ensure-qemu-guest-image.sh`:
- Around line 42-46: Harden the download flow around ALPINE_IMAGE_URL by adding
curl connection and total-transfer timeouts, ensuring the temporary
${DEST}.partial file is removed whenever curl or the subsequent move fails, and
verifying the downloaded image against the expected pinned checksum before
moving it to DEST. Preserve the existing retry behavior and only output DEST
after successful verification and installation.
In `@e2e/test/exporterset_qemu_test.go`:
- Around line 105-116: Replace the container readiness assertion in the
pod-waiting flow with kubectl’s pod Ready condition check, using the existing
exporterName and namespace arguments via the established Kubectl/Eventually
pattern. Update the preceding phase-based wait and its label so the readiness
wait uses `kubectl wait --for=condition=Ready pod/<name>`, and apply the same
change to the corresponding flow around the second readiness assertion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d5ba8535-d675-4bb6-b492-72c1c672a2c2
📒 Files selected for processing (32)
.dockerignore.github/actions/load-e2e-artifacts/action.yaml.github/workflows/e2e.yamlMakefilecontroller/hack/deploy_varscontroller/hack/deploy_with_operator.shcontroller/hack/sample-x86_64-kind.yamlcontroller/hack/utilscontroller/internal/exporterset/exporterconfig.gocontroller/internal/exporterset/provisioners/qemu/enrich_test.gocontroller/internal/exporterset/provisioners/qemu/qemu.gocontroller/internal/exporterset/provisioners/qemu/qemu_test.gocontroller/internal/exporterset/reconciler.gocontroller/internal/exporterset/reconciler_test.godocs/source/contributing/jeps/JEP-0014-virtual-scalable-exporters.mde2e/dex.values.yamle2e/lib/common.she2e/manifests/exporterset-qemu-kind.yamle2e/run-e2e.she2e/scripts/ensure-qemu-guest-image.she2e/scripts/qemu_flash_boot.pye2e/test/exporterset_qemu_test.goe2e/testdata/.gitignorepython/packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver.pypython/packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver_test.pypython/packages/jumpstarter/jumpstarter/exporter/exporter.pypython/packages/jumpstarter/jumpstarter/exporter/exporter_test.pyrust/jumpstarter-exec/src/client.rsrust/jumpstarter-exec/src/main.rsrust/jumpstarter-exec/src/protocol.rsrust/jumpstarter-exec/src/server.rsrust/jumpstarter-exec/tests/integration.rs
| except Exception as exc: # noqa: BLE001 - best-effort cleanup | ||
| print(f"power off after failure also failed: {exc}", flush=True) |
There was a problem hiding this comment.
poweroff should work, no best effort...
There was a problem hiding this comment.
Done in 45d69a6: success-path power.off() is no longer best-effort — failures propagate. Best-effort remains only on the failure/timeout cleanup branch.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/e2e.yaml (1)
184-187: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueNew checkout steps don't set
persist-credentials: false.Both new jobs checkout the repo without
persist-credentials: falsewhile also uploading artifacts in the same job (artipackedpattern). The actual leak surface here is limited since only/tmp/*.tar(outside the checkout) is uploaded, but setting it explicitly costs nothing and matches supply-chain best practice.🔒 Proposed fix
- name: Checkout repository uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7 with: fetch-depth: 0 + persist-credentials: falseAlso applies to: 218-221
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/e2e.yaml around lines 184 - 187, Update both new actions/checkout steps in the E2E workflow to set persist-credentials to false alongside fetch-depth, covering the checkout steps near the artifact-uploading jobs while preserving their existing checkout behavior.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In @.github/workflows/e2e.yaml:
- Around line 184-187: Update both new actions/checkout steps in the E2E
workflow to set persist-credentials to false alongside fetch-depth, covering the
checkout steps near the artifact-uploading jobs while preserving their existing
checkout behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0f328255-7c8c-43bf-a872-3349d455d2b6
📒 Files selected for processing (32)
.dockerignore.github/actions/load-e2e-artifacts/action.yaml.github/workflows/e2e.yamlMakefilecontroller/hack/deploy_varscontroller/hack/deploy_with_operator.shcontroller/hack/sample-x86_64-kind.yamlcontroller/hack/utilscontroller/internal/exporterset/exporterconfig.gocontroller/internal/exporterset/provisioners/qemu/enrich_test.gocontroller/internal/exporterset/provisioners/qemu/qemu.gocontroller/internal/exporterset/provisioners/qemu/qemu_test.gocontroller/internal/exporterset/reconciler.gocontroller/internal/exporterset/reconciler_test.godocs/source/contributing/jeps/JEP-0014-virtual-scalable-exporters.mde2e/dex.values.yamle2e/lib/common.she2e/manifests/exporterset-qemu-kind.yamle2e/run-e2e.she2e/scripts/ensure-qemu-guest-image.she2e/scripts/qemu_flash_boot.pye2e/test/exporterset_qemu_test.goe2e/testdata/.gitignorepython/packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver.pypython/packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver_test.pypython/packages/jumpstarter/jumpstarter/exporter/exporter.pypython/packages/jumpstarter/jumpstarter/exporter/exporter_test.pyrust/jumpstarter-exec/src/client.rsrust/jumpstarter-exec/src/main.rsrust/jumpstarter-exec/src/protocol.rsrust/jumpstarter-exec/src/server.rsrust/jumpstarter-exec/tests/integration.rs
🚧 Files skipped from review as they are similar to previous changes (26)
- e2e/testdata/.gitignore
- controller/hack/deploy_with_operator.sh
- controller/hack/deploy_vars
- .dockerignore
- .github/actions/load-e2e-artifacts/action.yaml
- rust/jumpstarter-exec/src/protocol.rs
- controller/hack/utils
- e2e/lib/common.sh
- python/packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver_test.py
- controller/internal/exporterset/reconciler_test.go
- controller/hack/sample-x86_64-kind.yaml
- rust/jumpstarter-exec/tests/integration.rs
- e2e/test/exporterset_qemu_test.go
- e2e/scripts/ensure-qemu-guest-image.sh
- controller/internal/exporterset/exporterconfig.go
- Makefile
- e2e/scripts/qemu_flash_boot.py
- rust/jumpstarter-exec/src/main.rs
- python/packages/jumpstarter/jumpstarter/exporter/exporter_test.py
- python/packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver.py
- controller/internal/exporterset/reconciler.go
- controller/internal/exporterset/provisioners/qemu/enrich_test.go
- controller/internal/exporterset/provisioners/qemu/qemu_test.go
- controller/internal/exporterset/provisioners/qemu/qemu.go
- rust/jumpstarter-exec/src/server.rs
- docs/source/contributing/jeps/JEP-0014-virtual-scalable-exporters.md
Raise diff-cover for exporter sidecar shutdown error/fallback branches, and centralize OIDC/legacy client-exporter create/login/delete in utils.go. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
python/packages/jumpstarter/jumpstarter/exporter/exporter_test.py (2)
1200-1208: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAssert that startup does not shut down the runtime.
Bind the mock and call
shutdown.assert_not_called()before cancelling the task group. The current test only verifies_stop_requested, so it would pass if processing the initial unleased status incorrectly invoked the new shutdown helper.As per coding guidelines,
python/**/*_test.pymust provide comprehensive package test coverage.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/packages/jumpstarter/jumpstarter/exporter/exporter_test.py` around lines 1200 - 1208, Update the test around exporter.serve to bind the shutdown_runtime_sidecar patch as a mock, then assert shutdown.assert_not_called() after the initial status is processed and before cancelling the task group. Keep the existing _stop_requested assertion and cancellation behavior unchanged.Source: Coding guidelines
1186-1188: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert one shutdown request and remove the duplicate call.
serve()callsshutdown_runtime_sidecar()on the lease-end branch, then unconditionally calls it again after the task group.assert_called()permits this duplicate side effect; requireassert_called_once()and make the cleanup path skip a shutdown already attempted for that transition.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/packages/jumpstarter/jumpstarter/exporter/exporter_test.py` around lines 1186 - 1188, Update the exporter.serve test and cleanup flow so shutdown_runtime_sidecar is invoked exactly once when the lease-end branch has already attempted shutdown. Replace the permissive assertion with assert_called_once(), and track or otherwise reuse that shutdown state to skip the unconditional post-task-group cleanup call for the same transition.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@python/packages/jumpstarter/jumpstarter/exporter/exporter_test.py`:
- Around line 1262-1278: Make test_falls_back_to_binary_beside_socket
deterministic by monkeypatching jumpstarter.exporter.exporter._DEFAULT_JMP_EXEC
to a nonexistent path under tmp_path before calling shutdown_runtime_sidecar.
Preserve the existing socket-adjacent binary setup and assertion while ensuring
the default binary path cannot exist in the test environment.
---
Outside diff comments:
In `@python/packages/jumpstarter/jumpstarter/exporter/exporter_test.py`:
- Around line 1200-1208: Update the test around exporter.serve to bind the
shutdown_runtime_sidecar patch as a mock, then assert
shutdown.assert_not_called() after the initial status is processed and before
cancelling the task group. Keep the existing _stop_requested assertion and
cancellation behavior unchanged.
- Around line 1186-1188: Update the exporter.serve test and cleanup flow so
shutdown_runtime_sidecar is invoked exactly once when the lease-end branch has
already attempted shutdown. Replace the permissive assertion with
assert_called_once(), and track or otherwise reuse that shutdown state to skip
the unconditional post-task-group cleanup call for the same transition.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1d9534a8-b747-4b1b-9ff8-7be52f382155
📒 Files selected for processing (6)
e2e/test/auth_logging_test.goe2e/test/exit_on_lease_end_test.goe2e/test/exporterset_qemu_test.goe2e/test/hooks_test.goe2e/test/utils.gopython/packages/jumpstarter/jumpstarter/exporter/exporter_test.py
🚧 Files skipped from review as they are similar to previous changes (1)
- e2e/test/exporterset_qemu_test.go
| minReplicas: 0 | ||
| maxReplicas: 1 |
There was a problem hiding this comment.
I think this works well for testing the replacement mechanism on exit but doesn't cover the warm pool behavior, specifically maintaining min replicas while instances are leased and the scale-up and scale-down mechanism. I know it's all covered by unit testing but maybe it's worth adding it also as an e2e test with minReplicas: 2 / maxReplicas: 3 to verify the scaling engine works correctly against a real cluster. (can be done also as a followup)
There was a problem hiding this comment.
Yes thanks!, I wanted to establish the base and then continue adding items like this. Also I will have to check how feasible it is without making the tests to way too heavy.
Adding this one to the list as well.
There was a problem hiding this comment.
Yes, I makes total sense, I will work on a follow up to not make this bigger.
I want to verify how does it behave in terms of pressure for the CI, it probably shouldn't be too bad if we don't start (power on) all the qemus at once.
| maxReplicas: 1 | ||
| minAvailableReplicas: 1 | ||
| scaleDownCooldown: 5m | ||
| recycleStrategy: ExitAndReplace |
There was a problem hiding this comment.
do we also want to test the recycleStrategy: InPlaceReuse flow?
There was a problem hiding this comment.
Yes, and many other things. That's a good topic for a follow up PR if that sounds good.
There was a problem hiding this comment.
Yes, will do an an independent PR.
| arch: x86_64 | ||
| smp: 1 | ||
| mem: 1G | ||
| disk_size: 10G |
There was a problem hiding this comment.
| disk_size: 10G | |
| disk_size: 2G |
May be more conservative like this, to make sure CI is ok.
| // Exporter (cascading the Pod) so scale-up can refill minAvailableReplicas. | ||
| // Without this, Offline/Succeeded instances inflate replicas, block warm-buffer | ||
| // refill at maxReplicas, and leave Completed Pods behind. | ||
| func (r *ExporterSetReconciler) cleanupTerminalExporters( |
There was a problem hiding this comment.
Follow-up (not blocking this PR): disable-before-delete grace for ExitAndReplace.
Today we delete as soon as Pods are terminal and LeaseRef is nil. That can race with the lease controller (separate process): after release the exporter may still look Online/Available long enough to accept a new lease while we delete it (worse if unregister/Offline lags).
Plan (tracked in #935):
- On terminal + unleased + still enabled → set
enabled=false, stamp a recycle annotation,RequeueAfter: 10s. - Delete only from
cleanupTerminalExportersonce Offline or disabled ≥ 10s (and still unleased). cleanupDisabledExportersmust skip recycle-stamped exporters so scale-down cleanup does not bypass the grace window.
Keeping that out of this PR to avoid growing the patch.
| ) | ||
| parser.add_argument( | ||
| "--disk-size", | ||
| default=os.environ.get("JUMPSTARTER_E2E_DISK_SIZE", "10G"), |
There was a problem hiding this comment.
| default=os.environ.get("JUMPSTARTER_E2E_DISK_SIZE", "10G"), | |
| default=os.environ.get("JUMPSTARTER_E2E_DISK_SIZE", "2G"), |
or consistent with any previous comments.
There was a problem hiding this comment.
Done in 45d69a6: default is now 2G (and the e2e flash invocation passes --disk-size 2G).
| ClientMessage::Shutdown => { | ||
| log.info("shutdown requested", &[("socket", json!(socket_path))]); | ||
| // Acknowledge before exiting so the client can observe success. | ||
| send(&writer, &ServerMessage::Exit { code: Some(0) })?; | ||
| // Drop the writer to flush the ack, remove the socket, then exit | ||
| // so the container's PID 1 terminates and Kubernetes replaces the Pod. | ||
| drop(writer); | ||
| let _ = std::fs::remove_file(socket_path); | ||
| std::process::exit(0); |
There was a problem hiding this comment.
std::process::exit(0) immediately terminates the process from a worker thread. Since serve_with spawns a new thread per connection, a Shutdown arriving while another connection is running an Exec session kills all child processes and I/O threads without signaling or draining.
Thus, if my brain does not trick me here Rust destructors are skipped, and since jumpstarter-exec runs as PID 1 in the container, orphaned children become zombies - I have seen entire clusters going doing from a misconfigured daemonset creating zombies ...
There was a problem hiding this comment.
Done in 45d69a6: Shutdown no longer calls process::exit. It acks the client, sets a shared flag, SIGTERMs tracked Exec children, and lets serve_with return cleanly so handlers can unwind.
| @@ -47,7 +47,7 @@ const ( | |||
| // configMountPath is where the ExporterConfig Secret is mounted. | |||
| configMountPath = "/etc/jumpstarter/exporters" | |||
There was a problem hiding this comment.
Consider exporting the constant from the exporterset package (e.g. ExporterConfigMountPath) and importing it in the QEMU provisioner.
There was a problem hiding this comment.
Exported ExporterConfigMountPath from exporterconfig.go in 45d69a6. The QEMU provisioner still uses a local exporterConfigPath constant (same value) — importing the parent exporterset package from provisioners/qemu creates a test import cycle (exporterset tests import qemu). Left a comment pointing at the shared constant.
| } | ||
|
|
||
| // listPodsGroupedByExporter maps Exporter names to their owned Pods. | ||
| func (r *ExporterSetReconciler) listPodsGroupedByExporter( |
There was a problem hiding this comment.
This new listPodsGroupedByExporter function (returns map[string][]corev1.Pod) issues the same List call with the same label selector and ownerReference filter as the existing listPodsByExporter (which returns map[string]struct{}). Both queries hit the API server or informer cache with identical parameters in a single reconcile cycle.
Keeping only listPodsGroupedByExporter and deriving the existence check at the call site would remove the duplication.
There was a problem hiding this comment.
Done in 45d69a6: listPodsByExporter now derives from listPodsGroupedByExporter (single List implementation).
|
|
||
| if self.exit_on_lease_end and previous_leased: | ||
| logger.info("Exporter configured to exit after lease, shutting down") | ||
| shutdown_runtime_sidecar() |
There was a problem hiding this comment.
Isnt this blocking for up to 10s?
There was a problem hiding this comment.
Yes — fixed in 45d69a6 by running it via await anyio.to_thread.run_sync(shutdown_runtime_sidecar) at the call sites.
| async with create_task_group() as tg: | ||
| tg.start_soon(exporter.serve) | ||
| # Give serve time to process the status | ||
| await anyio.sleep(0.1) |
There was a problem hiding this comment.
Using an event or condition variable to synchronize (e.g. wire _report_status to set an event after processing, then await that event before asserting) would make the test deterministic.
There was a problem hiding this comment.
Done in 45d69a6: exit-on-lease tests synchronize with an anyio.Event after statuses are processed instead of fixed sleeps.
| DumpOnFailure(250, tracker.DumpLogs) | ||
| // Stop any running exporter between tests | ||
| tracker.StopAll() | ||
| time.Sleep(time.Second) |
There was a problem hiding this comment.
Polling tracker.IsProcessRunning() with a short timeout via Eventually instead of a fixed sleep would be faster and more reliable.
There was a problem hiding this comment.
Done in 45d69a6: AfterEach uses Eventually until !tracker.IsProcessRunning() after StopAll().
Harden CI cache/timeouts, flip runtime/exporter UIDs for cidata access, make jumpstarter-exec Shutdown graceful, and tighten shutdown/e2e tests. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
controller/internal/exporterset/provisioners/qemu/qemu.go (1)
264-284: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winSet a shared Pod
fsGroupbefore the exporter writes/shared.
Qemu.cidata()creates a directory in/shared, but this Pod does not setspec.securityContext.fsGroup. WhenfsGroupis unset, Kubernetes does not modify volume ownership or permissions. The non-root exporter can then fail to create cidata in theemptyDir. Kubernetes also uses an implicit primary group whenrunAsGroupis unset. (kubernetes.io)Set
fsGroup: 65532and an explicit sharedrunAsGroup: 65532at Pod scope. Add assertions for both fields inqemu_test.go.Proposed fix
+ sharedGroup := exporterNonRootUID + pod := &corev1.Pod{ ObjectMeta: podMeta, Spec: corev1.PodSpec{ + SecurityContext: &corev1.PodSecurityContext{ + RunAsGroup: &sharedGroup, + FSGroup: &sharedGroup, + }, RestartPolicy: corev1.RestartPolicyNever,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@controller/internal/exporterset/provisioners/qemu/qemu.go` around lines 264 - 284, Update the QEMU Pod security context to set pod-level fsGroup and runAsGroup to 65532, ensuring the non-root exporter can create files under the shared emptyDir volume. Locate the Pod construction containing the sharedVolumeName volume and add assertions for both fields in qemu_test.go.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@e2e/test/exit_on_lease_end_test.go`:
- Around line 62-67: Update the shutdown verification around tracker.StopAll so
it observes the processes that were tracked before StopAll clears pt.pids.
Preserve a snapshot of the tracked PIDs and poll their termination, or have
StopAll return a termination result and assert it, ensuring the Eventually
assertion genuinely verifies exporter shutdown.
In `@rust/jumpstarter-exec/src/server.rs`:
- Around line 143-149: Synchronize shutdown state and child registration with a
shared lifecycle lock across the Shutdown handler and Exec flow. In the shutdown
logic around the children snapshot and state.shutdown.store, hold the lock while
marking shutdown and collecting PIDs; after spawn, register the PID under the
same lock only when shutdown is inactive. If shutdown is already active,
terminate and reap the newly spawned child before Exec returns.
---
Outside diff comments:
In `@controller/internal/exporterset/provisioners/qemu/qemu.go`:
- Around line 264-284: Update the QEMU Pod security context to set pod-level
fsGroup and runAsGroup to 65532, ensuring the non-root exporter can create files
under the shared emptyDir volume. Locate the Pod construction containing the
sharedVolumeName volume and add assertions for both fields in qemu_test.go.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 780cb051-d4e1-4134-9b22-94e05753b4fd
📒 Files selected for processing (18)
.github/workflows/e2e.yamlcontroller/Containerfile.qemu-runtimecontroller/hack/sample-x86_64-kind.yamlcontroller/internal/exporterset/exporterconfig.gocontroller/internal/exporterset/provisioners/qemu/qemu.gocontroller/internal/exporterset/provisioners/qemu/qemu_test.gocontroller/internal/exporterset/reconciler.goe2e/lib/common.she2e/scripts/ensure-qemu-guest-image.she2e/scripts/qemu_flash_boot.pye2e/test/exit_on_lease_end_test.goe2e/test/exporterset_qemu_test.gopython/packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver.pypython/packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver_test.pypython/packages/jumpstarter/jumpstarter/exporter/exporter.pypython/packages/jumpstarter/jumpstarter/exporter/exporter_test.pyrust/jumpstarter-exec/src/server.rsrust/jumpstarter-exec/tests/integration.rs
🚧 Files skipped from review as they are similar to previous changes (10)
- rust/jumpstarter-exec/tests/integration.rs
- python/packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver_test.py
- .github/workflows/e2e.yaml
- e2e/scripts/ensure-qemu-guest-image.sh
- controller/internal/exporterset/provisioners/qemu/qemu_test.go
- e2e/scripts/qemu_flash_boot.py
- python/packages/jumpstarter/jumpstarter/exporter/exporter.py
- e2e/test/exporterset_qemu_test.go
- controller/internal/exporterset/reconciler.go
- python/packages/jumpstarter/jumpstarter/exporter/exporter_test.py
| // Stop any running exporter between tests and wait until it is gone. | ||
| tracker.StopAll() | ||
| time.Sleep(time.Second) | ||
| Eventually(func() bool { | ||
| return tracker.IsProcessRunning() | ||
| }, 10*time.Second, 100*time.Millisecond).Should(BeFalse(), | ||
| "exporter process should stop after StopAll") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Make the shutdown assertion observe the tracked processes.
tracker.StopAll() clears pt.pids in e2e/test/utils.go:621-640. tracker.IsProcessRunning() only checks pt.pids in e2e/test/utils.go:651-663. Therefore, this Eventually assertion returns false immediately and does not verify exporter termination.
Preserve the PID snapshot for verification, or make StopAll return a termination result and assert that result here.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@e2e/test/exit_on_lease_end_test.go` around lines 62 - 67, Update the shutdown
verification around tracker.StopAll so it observes the processes that were
tracked before StopAll clears pt.pids. Preserve a snapshot of the tracked PIDs
and poll their termination, or have StopAll return a termination result and
assert it, ensuring the Eventually assertion genuinely verifies exporter
shutdown.
| let pids: Vec<u32> = state.children.lock().unwrap().iter().copied().collect(); | ||
| for pid in pids { | ||
| unsafe { | ||
| kill(pid as i32, 15); | ||
| } | ||
| } | ||
| state.shutdown.store(true, Ordering::SeqCst); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Synchronize shutdown state and child registration.
A concurrent Exec can spawn a child after Shutdown snapshots children. It can then insert its PID after the shutdown handler and the accept loop have completed their cleanup snapshots. That child does not receive SIGTERM.
Use one lifecycle lock for both the shutdown state and PID set. Set the shutdown state while holding that lock. After spawn, register the PID only if shutdown is not active. Otherwise, terminate and reap the new child before returning.
Also applies to: 215-216
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rust/jumpstarter-exec/src/server.rs` around lines 143 - 149, Synchronize
shutdown state and child registration with a shared lifecycle lock across the
Shutdown handler and Exec flow. In the shutdown logic around the children
snapshot and state.shutdown.store, hold the lock while marking shutdown and
collecting PIDs; after spawn, register the PID under the same lock only when
shutdown is inactive. If shutdown is already active, terminate and reap the
newly spawned child before Exec returns.
Runtime-as-root was creating QMP sockets the non-root exporter could not connect to. Clear umask / use 0666 on the launcher socket, and ship jumpstarter-exec from the qemu-runtime image so the fix is easy to rebuild. Co-authored-by: Cursor <cursoragent@cursor.com>
Reverts embedding jumpstarter-exec in qemu-runtime. The binary is built once in the exporter image and copied to /shared via init container, avoiding a Rust rebuild for every new runtime image. Co-authored-by: Cursor <cursoragent@cursor.com>
| done | ||
|
|
||
| - name: Prefetch Alpine guest image for ExporterSet QEMU | ||
| run: bash e2e/scripts/ensure-qemu-guest-image.sh |
There was a problem hiding this comment.
this must run from the setup-e2e make target, not be part of github ci yaml.
The ensure-qemu-guest-image.sh call now runs as part of setup-e2e.sh so both CI and local `make e2e-setup` prefetch the image consistently. Co-authored-by: Cursor <cursoragent@cursor.com>
3 participants
Summary
e2e-testsjob; recycle coverage runsj qemu power on/j qemu power offand re-checks the replacement. Flash/boot remains skipped until guest-disk capacity (Add storage/disk support for QEMU ExporterSet virtual targets #924).target-runtime(jumpstarter-exec) is a native sidecar: defaultkubectl logs,restartPolicy: Never, and exporter exit completes the Pod.jumpstarter-exec shutdownand best-effort runtime teardown from the exporter onexitOnLeaseEnd.Succeeded/Failed, then refillingminAvailableReplicas(avoids Completed/Offline zombies andmaxReplicasstalls).shutdown_runtime_sidecarerror/fallback paths for diff-cover; share OIDC/legacy client–exporter create/login/delete helpers ine2e/test/utils.go.Test plan
go test ./internal/exporterset/...(includes ExitAndReplace cleanup + RenderPod layout)make e2e-exporterset-qemu— Online/Ready + power cycle + recycle; flash/boot skipped on 100Mi SizeLimitmake -C python pkg-test-jumpstarter— includes new shutdown_runtime_sidecar coveragee2e-testson this PRcontroller/hack/sample-x86_64-kind.yaml, lease/release, confirm Completed Pod is deleted and a new Ready instance appears