Warm Pool Feasibility Study [Spec]#3
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:
📝 WalkthroughWalkthroughThis PR adds warm-pool study documentation, Kubernetes experiment tooling and manifests, a readiness sidecar implementation, recorded results, an RFC proposal, repository ignore rules, and a workflow action update. ChangesWarm Pool Study Planning
Warm Pool Experiment Tooling
Results and Integration Proposal
Workflow Update
Estimated code review effort: 4 (Complex) | ~60 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Applied fixes from bot review comments: - Comment #3552516629: run sidecar container as non-root (USER 65532) - Comment #3552516599: handle kubectl exec failure in sidecar-readiness - Comment #3552515048: fix readiness gate condition type mismatch - Comment #3552514809: align CSV header labels with actual phase data Assisted-By: 🤖 Claude Code Signed-off-by: Roland Huß <rhuss@redhat.com>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Extract wait_pool_replenished to lib/common.sh and add replenishment checks before each claim in measure-combined.sh and measure-env-injection.sh. Without this, later runs may exhaust the pool and silently fall back to cold-start provisioning, corrupting warm pool measurement data. Assisted-By: 🤖 Claude Code Signed-off-by: Roland Huß <rhuss@redhat.com>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
22c9126 to
709b0f8
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
experiments/measure-cold-start.sh (1)
148-217: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winVanilla runner ignores
--imageoverride;SANDBOX_IMAGEis unused.The
--imageCLI option is documented inusage()andSANDBOX_IMAGEis defined at line 12, but the vanilla runner hardcodesregistry.k8s.io/pause:3.10in the inline manifest (line 175) instead of using$SANDBOX_IMAGE. The flag has no effect on this configuration, which is misleading for experimenters who pass--imageexpecting it to apply.🔧 Proposed fix: use SANDBOX_IMAGE in vanilla manifest
if ! kubectl apply -n "$NAMESPACE" -f - <<YAML apiVersion: agents.x-k8s.io/v1beta1 kind: Sandbox metadata: name: ${sandbox_name} spec: podTemplate: spec: containers: - name: sandbox - image: registry.k8s.io/pause:3.10 + image: ${SANDBOX_IMAGE} command: ["/pause"] YAML🤖 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 `@experiments/measure-cold-start.sh` around lines 148 - 217, The vanilla runner currently hardcodes the sandbox image in run_vanilla_config, so the --image override and SANDBOX_IMAGE setting are ignored for this path. Update the inline Sandbox manifest in run_vanilla_config to reference SANDBOX_IMAGE instead of the fixed pause image, and keep the rest of the create/wait/cleanup flow unchanged so the existing CLI option applies consistently to vanilla runs.
🧹 Nitpick comments (3)
rfc/NNNN-warm-pool-feasibility/README.md (1)
252-255: 🚀 Performance & Scalability | 🔵 TrivialCheck the operational cost of a 1s kubelet sync.
This target depends on a cluster-wide kubelet flag, not just an OpenShell setting. Verify that OpenShift can absorb the extra propagation churn; otherwise the Phase 1/2 latency goals should not assume sub-second annotation propagation.
🤖 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 `@rfc/NNNN-warm-pool-feasibility/README.md` around lines 252 - 255, The note in the projected-volume propagation guidance assumes sub-second annotation updates from kubelet, but the reviewer is asking to verify the operational impact of using a 1s kubelet sync before treating that latency as feasible. Update the section around the kubelet propagation and polling fallback text to explicitly state that `--sync-frequency=1s` is a cluster-wide kubelet setting, and qualify the Phase 1/2 latency goals unless OpenShift capacity/overhead has been validated; keep the fallback polling mention tied to the supervisor behavior.experiments/manifests/readiness-gate-pod.yaml (1)
9-15: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueAdd a
securityContextto the sandbox container.Static analysis flags missing
readOnlyRootFilesystem, default security context, and missingallowPrivilegeEscalation: false. For experiment pods these are non-exploitable, but adding a restrictive security context is cheap and aligns with Kubernetes hardening best practices.🛡️ Proposed security context
- name: sandbox image: registry.k8s.io/pause:3.10 command: ["/pause"] + securityContext: + readOnlyRootFilesystem: true + allowPrivilegeEscalation: false + runAsNonRoot: true + runAsUser: 65534 resources: requests: cpu: "100m" memory: "64Mi"🤖 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 `@experiments/manifests/readiness-gate-pod.yaml` around lines 9 - 15, Add a restrictive securityContext to the sandbox container in the readiness-gate pod manifest so the container uses a hardened default. Update the sandbox container spec to explicitly set readOnlyRootFilesystem and allowPrivilegeEscalation: false, and align any other default security settings needed for this pod. Use the sandbox container block in the manifest to locate the change.Source: Linters/SAST tools
experiments/manifests/sidecar-readiness.yaml (1)
27-36: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueAdd a
securityContextto the sandbox container.Same as the readiness-gate-pod manifest — static analysis flags missing
readOnlyRootFilesystemand default security context. Cheap to add for experiment pods.🛡️ Proposed security context
- name: sandbox image: registry.k8s.io/pause:3.10 command: ["/pause"] + securityContext: + readOnlyRootFilesystem: true + allowPrivilegeEscalation: false + runAsNonRoot: true + runAsUser: 65534 volumeMounts:🤖 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 `@experiments/manifests/sidecar-readiness.yaml` around lines 27 - 36, Add a securityContext to the sandbox container in the sidecar readiness manifest, matching the readiness-gate-pod manifest’s container hardening. Update the sandbox container definition to explicitly set the same default security settings, including readOnlyRootFilesystem, so the experiment pod passes static analysis and uses the intended secure runtime profile.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.
Inline comments:
In `@experiments/manifests/sidecar-readiness.yaml`:
- Around line 8-11: The sidecar manifest is using the wrong container
image/entrypoint for the readiness contract; replace the current agnhost netexec
setup with the custom readiness-sidecar image used by
experiments/sidecar/main.go so the container polls /tmp/signal/ready and only
serves /healthz after it exists. Update the image and command/args in the
sidecar manifest to point at that readiness-sidecar implementation, keeping the
pod behavior aligned with measure-sidecar-readiness.sh.
In `@rfc/NNNN-warm-pool-feasibility/README.md`:
- Around line 176-189: Close the claim/create race in the warm-pool fallback
logic by adding an explicit cancellation or terminal-state check before
switching to the cold-start path. Update the timeout handling around
wait_for_claim_ready and the delete_claim/create_sandbox_cold_start flow so the
driver revalidates the SandboxClaim state after timeout and only proceeds to
create_sandbox_cold_start when the claim is confirmed not to have bound or is
safely terminated.
- Around line 281-317: The warm-pool flow in the sandbox creation path records
only Pending and then returns before persisting the new warm-claim lifecycle
state, leaving WarmClaimed unused. Update the warm path around the
create_claim/wait_claim_bound/inject_identity sequence to transition the sandbox
store into WarmClaimed once the claim is bound and before returning, so the
SandboxState enum entry is actually reachable and status/cleanup logic can
distinguish it from Pending.
---
Outside diff comments:
In `@experiments/measure-cold-start.sh`:
- Around line 148-217: The vanilla runner currently hardcodes the sandbox image
in run_vanilla_config, so the --image override and SANDBOX_IMAGE setting are
ignored for this path. Update the inline Sandbox manifest in run_vanilla_config
to reference SANDBOX_IMAGE instead of the fixed pause image, and keep the rest
of the create/wait/cleanup flow unchanged so the existing CLI option applies
consistently to vanilla runs.
---
Nitpick comments:
In `@experiments/manifests/readiness-gate-pod.yaml`:
- Around line 9-15: Add a restrictive securityContext to the sandbox container
in the readiness-gate pod manifest so the container uses a hardened default.
Update the sandbox container spec to explicitly set readOnlyRootFilesystem and
allowPrivilegeEscalation: false, and align any other default security settings
needed for this pod. Use the sandbox container block in the manifest to locate
the change.
In `@experiments/manifests/sidecar-readiness.yaml`:
- Around line 27-36: Add a securityContext to the sandbox container in the
sidecar readiness manifest, matching the readiness-gate-pod manifest’s container
hardening. Update the sandbox container definition to explicitly set the same
default security settings, including readOnlyRootFilesystem, so the experiment
pod passes static analysis and uses the intended secure runtime profile.
In `@rfc/NNNN-warm-pool-feasibility/README.md`:
- Around line 252-255: The note in the projected-volume propagation guidance
assumes sub-second annotation updates from kubelet, but the reviewer is asking
to verify the operational impact of using a 1s kubelet sync before treating that
latency as feasible. Update the section around the kubelet propagation and
polling fallback text to explicitly state that `--sync-frequency=1s` is a
cluster-wide kubelet setting, and qualify the Phase 1/2 latency goals unless
OpenShift capacity/overhead has been validated; keep the fallback polling
mention tied to the supervisor behavior.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 06383420-8657-45a3-b9bd-9d7f8d47f939
📒 Files selected for processing (14)
experiments/RESULTS.mdexperiments/lib/common.shexperiments/manifests/image-prepull-daemonset.yamlexperiments/manifests/readiness-gate-pod.yamlexperiments/manifests/sandbox-template.yamlexperiments/manifests/sidecar-readiness.yamlexperiments/manifests/warm-pool.yamlexperiments/measure-cold-start.shexperiments/measure-combined.shexperiments/measure-env-injection.shexperiments/measure-readiness-gates.shexperiments/measure-sidecar-readiness.shexperiments/measure-warm-pool.shrfc/NNNN-warm-pool-feasibility/README.md
✅ Files skipped from review due to trivial changes (1)
- experiments/RESULTS.md
🚧 Files skipped from review as they are similar to previous changes (8)
- experiments/manifests/warm-pool.yaml
- experiments/manifests/image-prepull-daemonset.yaml
- experiments/measure-readiness-gates.sh
- experiments/measure-env-injection.sh
- experiments/measure-sidecar-readiness.sh
- experiments/lib/common.sh
- experiments/measure-combined.sh
- experiments/measure-warm-pool.sh
| When the warm pool has zero ready replicas or the SandboxClaim stays | ||
| Pending for longer than a configurable timeout (default: 5 seconds), the | ||
| driver falls back to the current cold-start path (creating a Sandbox | ||
| resource directly). | ||
|
|
||
| ```rust | ||
| match timeout(Duration::from_secs(5), wait_for_claim_ready(&claim)).await { | ||
| Ok(sandbox_name) => { /* warm path */ }, | ||
| Err(_) => { | ||
| delete_claim(&claim).await; | ||
| create_sandbox_cold_start(&sandbox).await; // existing path | ||
| } | ||
| } | ||
| ``` |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Close the claim/create race before falling back.
Deleting the claim on timeout and immediately creating a cold sandbox can double-provision if the claim binds just after the timeout, or leave warm capacity stranded. The fallback needs an explicit cancellation/terminal-state check before switching paths.
🤖 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 `@rfc/NNNN-warm-pool-feasibility/README.md` around lines 176 - 189, Close the
claim/create race in the warm-pool fallback logic by adding an explicit
cancellation or terminal-state check before switching to the cold-start path.
Update the timeout handling around wait_for_claim_ready and the
delete_claim/create_sandbox_cold_start flow so the driver revalidates the
SandboxClaim state after timeout and only proceeds to create_sandbox_cold_start
when the claim is confirmed not to have bound or is safely terminated.
| // Store sandbox as "pending" in the sandbox store | ||
| self.store.insert_pending(sandbox_id, &request.name); | ||
|
|
||
| // Try warm pool first | ||
| if let Some(pool) = self.driver.find_warm_pool(&namespace).await { | ||
| match self.driver.create_claim(&request.name, &pool).await { | ||
| Ok(claim) => { | ||
| let sandbox_name = self.driver.wait_claim_bound(&claim).await?; | ||
| self.driver.inject_identity(&sandbox_name, &sandbox_id).await?; | ||
| // Supervisor will call back via ConnectSupervisor | ||
| return Ok(sandbox_id); | ||
| } | ||
| Err(_) => { /* fall through to cold start */ } | ||
| } | ||
| } | ||
|
|
||
| // Cold-start fallback (existing path) | ||
| self.driver.create_sandbox(&sandbox_id, &request).await | ||
| } | ||
| ``` | ||
|
|
||
| #### 3.2 Sandbox Store Changes | ||
|
|
||
| The sandbox store needs to track the warm pool lifecycle: | ||
|
|
||
| ```rust | ||
| enum SandboxState { | ||
| Pending, // Created, waiting for compute | ||
| WarmClaimed, // Claim bound, waiting for supervisor activation | ||
| Connected, // Supervisor called ConnectSupervisor | ||
| Ready, // SSH available, sandbox usable | ||
| // ... existing states | ||
| } | ||
| ``` | ||
|
|
||
| The `WarmClaimed` state is new and represents the window between claim | ||
| binding and supervisor activation. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Persist the WarmClaimed transition.
The warm path inserts Pending, then returns after identity injection without ever recording the new warm-claim state. As written, the WarmClaimed state below is unreachable, and status/cleanup code will not be able to distinguish a bound warm claim from an ordinary pending sandbox.
🤖 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 `@rfc/NNNN-warm-pool-feasibility/README.md` around lines 281 - 317, The
warm-pool flow in the sandbox creation path records only Pending and then
returns before persisting the new warm-claim lifecycle state, leaving
WarmClaimed unused. Update the warm path around the
create_claim/wait_claim_bound/inject_identity sequence to transition the sandbox
store into WarmClaimed once the claim is bound and before returning, so the
SandboxState enum entry is actually reachable and status/cleanup logic can
distinguish it from Pending.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
experiments/manifests/sandbox-template.yaml (1)
15-15: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffConsider pinning image digests for a reproducibility study.
All three images (
sandboxes/base,agent,supervisor) use:latest. Since this template backs recorded latency results, a floating tag makes the measurements non-reproducible: a later:latestpush changes image size/init time and invalidates comparisons against the archived runs. Pinning to a digest (or a fixed tag) would tie results to a known image.Also applies to: 33-33, 89-89
🤖 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 `@experiments/manifests/sandbox-template.yaml` at line 15, Replace the floating :latest tags for the base, agent, and supervisor images in the sandbox manifest with immutable image digests or fixed version tags, keeping all three references consistent with the versions used for the recorded latency results.experiments/measure-warm-pool.sh (1)
313-315: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse
get_claimed_sandbox_namehere
This repeats the samejsonpath='{.status.sandbox.name}'lookup already wrapped by the helper, so using the helper keeps both call sites aligned if the field path changes.🤖 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 `@experiments/measure-warm-pool.sh` around lines 313 - 315, Replace the duplicated kubectl lookup in the claim-sandbox polling logic with the existing get_claimed_sandbox_name helper, passing claim_name and NAMESPACE as required; preserve the current empty-result behavior and use the helper’s returned value for subsequent processing.
🤖 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 `@experiments/manifests/sandbox-template.yaml`:
- Line 15: Replace the floating :latest tags for the base, agent, and supervisor
images in the sandbox manifest with immutable image digests or fixed version
tags, keeping all three references consistent with the versions used for the
recorded latency results.
In `@experiments/measure-warm-pool.sh`:
- Around line 313-315: Replace the duplicated kubectl lookup in the
claim-sandbox polling logic with the existing get_claimed_sandbox_name helper,
passing claim_name and NAMESPACE as required; preserve the current empty-result
behavior and use the helper’s returned value for subsequent processing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8e4a61d9-f455-4d24-a40e-95532a50bd1a
⛔ Files ignored due to path filters (8)
experiments/results/2026-07-09-rosa-hcp-422/cold-start-noprepull.csvis excluded by!**/*.csvexperiments/results/2026-07-09-rosa-hcp-422/cold-start-prepulled.csvis excluded by!**/*.csvexperiments/results/2026-07-09-rosa-hcp-422/cold-start-vanilla.csvis excluded by!**/*.csvexperiments/results/2026-07-09-rosa-hcp-422/env-injection-allowed.csvis excluded by!**/*.csvexperiments/results/2026-07-09-rosa-hcp-422/env-injection-disallowed.csvis excluded by!**/*.csvexperiments/results/2026-07-09-rosa-hcp-422/readiness-gates.csvis excluded by!**/*.csvexperiments/results/2026-07-09-rosa-hcp-422/warm-pool-burst.csvis excluded by!**/*.csvexperiments/results/2026-07-09-rosa-hcp-422/warm-pool-default.csvis excluded by!**/*.csv
📒 Files selected for processing (20)
experiments/RESULTS.mdexperiments/manifests/image-prepull-daemonset.yamlexperiments/manifests/readiness-gate-pod.yamlexperiments/manifests/sandbox-template.yamlexperiments/manifests/sidecar-readiness.yamlexperiments/measure-cold-start.shexperiments/measure-warm-pool.shexperiments/results/.gitignoreexperiments/results/2026-07-09-rosa-hcp-422/events-wp-default-run-1.jsonexperiments/results/2026-07-09-rosa-hcp-422/events-wp-default-run-10.jsonexperiments/results/2026-07-09-rosa-hcp-422/events-wp-default-run-2.jsonexperiments/results/2026-07-09-rosa-hcp-422/events-wp-default-run-3.jsonexperiments/results/2026-07-09-rosa-hcp-422/events-wp-default-run-4.jsonexperiments/results/2026-07-09-rosa-hcp-422/events-wp-default-run-5.jsonexperiments/results/2026-07-09-rosa-hcp-422/events-wp-default-run-6.jsonexperiments/results/2026-07-09-rosa-hcp-422/events-wp-default-run-7.jsonexperiments/results/2026-07-09-rosa-hcp-422/events-wp-default-run-8.jsonexperiments/results/2026-07-09-rosa-hcp-422/events-wp-default-run-9.jsonexperiments/results/README.mdrfc/NNNN-warm-pool-feasibility/README.md
✅ Files skipped from review due to trivial changes (13)
- experiments/results/README.md
- experiments/results/.gitignore
- experiments/results/2026-07-09-rosa-hcp-422/events-wp-default-run-2.json
- experiments/results/2026-07-09-rosa-hcp-422/events-wp-default-run-5.json
- experiments/results/2026-07-09-rosa-hcp-422/events-wp-default-run-9.json
- experiments/results/2026-07-09-rosa-hcp-422/events-wp-default-run-1.json
- experiments/results/2026-07-09-rosa-hcp-422/events-wp-default-run-7.json
- experiments/results/2026-07-09-rosa-hcp-422/events-wp-default-run-4.json
- experiments/results/2026-07-09-rosa-hcp-422/events-wp-default-run-3.json
- experiments/results/2026-07-09-rosa-hcp-422/events-wp-default-run-6.json
- experiments/results/2026-07-09-rosa-hcp-422/events-wp-default-run-10.json
- experiments/results/2026-07-09-rosa-hcp-422/events-wp-default-run-8.json
- rfc/NNNN-warm-pool-feasibility/README.md
🚧 Files skipped from review as they are similar to previous changes (3)
- experiments/manifests/readiness-gate-pod.yaml
- experiments/manifests/sidecar-readiness.yaml
- experiments/measure-cold-start.sh
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Applied fixes from bot review comments: - Fix CSV header mismatch in common.sh (init_ms/supervisor_ms/ssh_ms) - Fix (( elapsed++ )) under set -e crashing on first iteration (10 occurrences) - Fix usage() exiting 0 on error in all experiment scripts - Surface cleanup timeouts with log warning and return 1 Assisted-By: 🤖 Claude Code Signed-off-by: Roland Huß <rhuss@redhat.com>
Parent brainstorm (01) defines the layered measurement approach for evaluating Agent Sandbox warm pooling on OpenShift. Child documents cover cluster setup (02), measurements (03), and results synthesis (04). Assisted-By: 🤖 Claude Code
Assisted-By: 🤖 Claude Code Signed-off-by: Roland Huß <rhuss@redhat.com>
Assisted-By: 🤖 Claude Code Signed-off-by: Roland Huß <rhuss@redhat.com>
Feasibility study for warm pool integration with the Agent Sandbox operator on OpenShift (ROSA HCP 4.22). Key findings: - Claiming a pre-provisioned pod takes ~1.4s vs 16.7s cold start (12x) - Claim latency is image-independent (tested pause + full 3.2GB sandbox) - Identity binding via spec.env bypasses warm pool (creates fresh pod) Two RFCs for identity binding alternatives: - Annotation-based (~3s latency, ~18 points) - gRPC-push with always-on supervisor (sub-2s target, ~24 points) Prototype brainstorm (M1: ActivateSandbox gRPC PoC, M2: SandboxProfile entity with workspace-scoped pool lifecycle) incorporating NVIDIA feedback on unidentified supervisor state and SandboxProfile entity. Includes reproducible experiment scripts, measurement data, and analysis tooling. Assisted-By: 🤖 Claude Code Signed-off-by: Roland Huß <rhuss@redhat.com>
0b70b76 to
1635442
Compare
| burst 5 simultaneous claims to measure burst behavior and pool replenishment | ||
| all Run all configurations sequentially | ||
| EOF | ||
| exit 0 |
There was a problem hiding this comment.
🟡 Invalid command-line arguments silently succeed instead of reporting failure
The warm pool measurement script exits successfully when given unknown options (exit 0 at experiments/measure-warm-pool.sh:37), so wrapper scripts and CI pipelines that check exit codes will miss the misconfiguration.
Impact: Experiment runs with typos in flags appear to succeed, producing no measurements and no error signal.
Constitution Principle II violation: usage function unconditionally exits 0
The usage() function at experiments/measure-warm-pool.sh:37 uses exit 0 unconditionally. When called from the error path at line 49 (*) log "ERROR: Unknown option: $1"; usage ;;), the script exits with status 0 despite encountering an invalid argument.
The repository's constitution (.specify/memory/constitution.md:39-48) requires: "Usage/help functions called from error paths MUST exit with non-zero status. The recommended pattern is exit "${1:-0}" in the usage function body, with usage 0 for help requests and usage 1 for error call sites."
All five other experiment scripts in this PR correctly implement this pattern (e.g., experiments/measure-cold-start.sh:34 uses exit "${1:-0}" and line 44 calls usage 1). Only measure-warm-pool.sh deviates.
| exit 0 | |
| exit "${1:-0}" |
Was this helpful? React with 👍 or 👎 to provide feedback.
| --template) TEMPLATE_NAME="$2"; shift 2 ;; | ||
| --timeout) CLAIM_TIMEOUT="$2"; shift 2 ;; | ||
| -h|--help) usage ;; | ||
| *) log "ERROR: Unknown option: $1"; usage ;; |
There was a problem hiding this comment.
🟡 Error path for unknown options exits successfully instead of with failure status
The error handler for unrecognized flags calls the help function without requesting a non-zero exit (usage at experiments/measure-warm-pool.sh:49), so the script reports success even when invoked with invalid arguments.
Impact: Automated experiment harnesses cannot detect misconfigured measurement runs.
Constitution Principle II violation: error call site does not pass non-zero exit code
At experiments/measure-warm-pool.sh:49, the unknown-option handler calls usage without an argument. Even after fixing BUG-0001 to use exit "${1:-0}", calling usage without an argument defaults to exit 0. The error call site must pass 1 to signal failure, matching the pattern used in all other experiment scripts (e.g., experiments/measure-cold-start.sh:44: usage 1).
| *) log "ERROR: Unknown option: $1"; usage ;; | |
| *) log "ERROR: Unknown option: $1"; usage 1 ;; |
Was this helpful? React with 👍 or 👎 to provide feedback.
|
@copilot resolve the merge conflicts in this pull request |
| <<<<<<< HEAD | ||
| uses: astral-sh/setup-uv@f98e06938123ccabd21905ea5d0069192241f9f1 # v8.3.1 | ||
| ======= | ||
| uses: astral-sh/setup-uv@11f9893b081a58869d3b5fccaea48c9e9e46f990 # v8.3.2 | ||
| >>>>>>> origin/main |
There was a problem hiding this comment.
🔴 Unresolved merge conflict markers break the CI workflow file
Unresolved git merge conflict markers are left in the workflow file (<<<<<<< HEAD at .github/workflows/sync-docs.yml:96), so GitHub Actions will fail to parse the YAML and the sync-docs workflow will not run.
Impact: The documentation sync workflow is broken and will error on every trigger.
Merge conflict markers in YAML
Lines 96-100 contain literal <<<<<<< HEAD, =======, and >>>>>>> origin/main markers between two different setup-uv action SHA references. YAML parsers treat these as invalid syntax, causing the entire workflow to fail before any step executes.
<<<<<<< HEAD
uses: astral-sh/setup-uv@f98e06938123ccabd21905ea5d0069192241f9f1 # v8.3.1
=======
uses: astral-sh/setup-uv@11f9893b081a58869d3b5fccaea48c9e9e46f990 # v8.3.2
>>>>>>> origin/main| <<<<<<< HEAD | |
| uses: astral-sh/setup-uv@f98e06938123ccabd21905ea5d0069192241f9f1 # v8.3.1 | |
| ======= | |
| uses: astral-sh/setup-uv@11f9893b081a58869d3b5fccaea48c9e9e46f990 # v8.3.2 | |
| >>>>>>> origin/main | |
| uses: astral-sh/setup-uv@11f9893b081a58869d3b5fccaea48c9e9e46f990 # v8.3.2 |
Was this helpful? React with 👍 or 👎 to provide feedback.
| <<<<<<< HEAD | ||
| Last updated: 2026-07-11 | ||
| ======= | ||
| Last updated: 2026-07-10 | ||
| >>>>>>> origin/main |
There was a problem hiding this comment.
🟡 Unresolved merge conflict markers in brainstorm overview document
Unresolved git merge conflict markers are left throughout the brainstorm overview (<<<<<<< HEAD at brainstorm/00-overview.md:3), so the document renders with raw conflict syntax instead of coherent content.
Impact: The brainstorm overview is unreadable, showing duplicate conflicting sections with git markers.
Multiple conflict blocks in the file
The file contains conflict markers at lines 3-7, 17-23, 33-42, 51-62. Each block has both HEAD and origin/main versions of the content left unresolved. For example, the sessions table has two different sets of entries (sessions 05-07 from HEAD vs session 05 from origin/main).
Was this helpful? React with 👍 or 👎 to provide feedback.
Why This Change
OpenShell's Kubernetes driver creates a fresh Sandbox CR for every sandbox request, paying 8-12 seconds for pod scheduling, image pull, init container execution, supervisor startup, and gateway registration. For agent harnesses like OpenClaw that create sandboxes per tool call, this latency is unusable. The upstream Agent Sandbox project (v0.5.0) provides extension CRDs for warm pooling (SandboxTemplate, SandboxWarmPool, SandboxClaim), but OpenShell has no awareness of these. We need to know if warm pooling can deliver sub-2s sandbox startup on OpenShift before investing in integration work.
What Changes
This study will create an
experiments/directory with shell-based measurement scripts, Kubernetes manifests for warm pool configurations, and a minimal Go sidecar binary for readiness experiments. It will provision a short-lived ROSA HCP cluster, run 6 experiments measuring cold-start vs warm pool latency across readiness configurations, and produce an RFC inrfc/with data-backed architectural recommendations for OpenShell warm pool integration. No changes to OpenShell core code. This PR contains the specification artifacts only; implementation follows after spec approval.How It Works
The study uses a layered measurement approach across 4 execution phases:
Important
Review Guide contains the full review guidance: key decisions, scope boundaries, areas needing attention, and review checklist.
This PR contains the specification artifacts for Warm Pool Feasibility Study. Implementation follows after spec approval.
Assisted-By: 🤖 Claude Code
Summary by CodeRabbit