[DEBUG][DO NOT MERGE] - #82632
Conversation
…azure-ipi-kata Add new step-registry ref that installs the OSC operator and configures the cluster using Helm charts (rhdh-e2e-runner base image, git clone pattern). Enable OSC_INSTALL=true in candidate422 azure-ipi-kata with test scenario C00102. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
openshift-ci
Bot
added
the
do-not-merge/hold
|
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:
WalkthroughThe pull request adds a CI step that installs the OpenShift Sandboxed Containers operator and operands with Helm, configures workload and peer-pod modes, waits for readiness, records installation state, and enables the step in the sandboxed-containers pre-chain and Azure test job. ChangesOSC operator installation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CI
participant OSCInstallStep
participant Helm
participant OpenShift
CI->>OSCInstallStep: execute with OSC_INSTALL and workload settings
OSCInstallStep->>Helm: fetch and render OSC charts
Helm-->>OSCInstallStep: return rendered manifests
OSCInstallStep->>OpenShift: apply operator manifests
OpenShift-->>OSCInstallStep: report operator resources ready
OSCInstallStep->>OpenShift: apply operands and peer-pods secret
OpenShift-->>OSCInstallStep: report KataConfig ready
OSCInstallStep->>OpenShift: patch osc-config with oscInstalled=true
Suggested labels: Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 inconclusive)
✅ Passed checks (13 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 |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: tbuskey The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
|
@tbuskey: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (8)
ci-operator/step-registry/sandboxed-containers-operator/install-osc-operator/sandboxed-containers-operator-install-osc-operator-commands.sh (6)
88-95: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
retry()appears unused.No call sites in this script; drop it or wire it into the flaky
oc apply/git clonepaths.🤖 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 `@ci-operator/step-registry/sandboxed-containers-operator/install-osc-operator/sandboxed-containers-operator-install-osc-operator-commands.sh` around lines 88 - 95, Remove the unused retry function from the script, since no call sites invoke retry and the comment does not require integrating it into other commands.Source: Linters/SAST tools
97-124: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoff
evalof an interpolated condition string is fragile.Every call site embeds
${OSC_NAMESPACE}/${installplan_ref}/${kataconfig_name}into a string that is re-parsed by the shell, requiring the hand-escaped\"\$(...)\"gymnastics seen at lines 383, 404, 411, 582. A value containing whitespace or shell metacharacters (e.g. a mistypedOSC_NAMESPACEin a job config) changes the parse rather than failing cleanly. Passing the condition as an array (wait_until desc timeout interval -- oc get ...) and invoking"$@"removes theevalentirely.Not exploitable as written since all inputs come from trusted job config, so this is a hardening/readability suggestion rather than a live vulnerability.
🤖 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 `@ci-operator/step-registry/sandboxed-containers-operator/install-osc-operator/sandboxed-containers-operator-install-osc-operator-commands.sh` around lines 97 - 124, Update the wait_until function to accept the condition command and its arguments as separate parameters, invoke them directly with "$@" instead of eval, and adjust all call sites to pass commands and interpolated values as distinct arguments. Remove the embedded escaping and preserve the existing timeout, progress, success, and failure behavior.Source: Linters/SAST tools
298-303: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winMissing
peerpods-param-cmshould be fatal whenENABLEPEERPODS=true.Continuing with a warning renders peer-pods config with chart defaults only (no subnet/NSG/resource group), so the failure surfaces much later as an opaque KataConfig or pod-VM launch timeout after up to two hours of waiting. Fail fast here instead.
♻️ Suggested change
else - echo ">>> WARNING: peerpods-param-cm not found in default namespace" >&2 + echo ">>> ERROR: peerpods-param-cm not found in default namespace but ENABLEPEERPODS=true" >&2 + return 1 fi🤖 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 `@ci-operator/step-registry/sandboxed-containers-operator/install-osc-operator/sandboxed-containers-operator-install-osc-operator-commands.sh` around lines 298 - 303, Update the ENABLEPEERPODS=true branch in the peerpods-param-cm lookup so a missing ConfigMap exits with a nonzero status instead of only emitting a warning; preserve the existing helm_args behavior for found configuration and the peerpods.enabled=false path when peer pods are disabled.
628-631: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
|| truethen an unconditional success message.If the patch fails, the log still claims
oscInstalled=truewas set while downstream tests will re-install or misdetect state. Let the patch failure surface.♻️ Suggested change
- oc patch configmap osc-config -n default --type merge \ - -p '{"data":{"oscInstalled":"true"}}' || true - - echo ">>> osc-config patched with oscInstalled=true" + if ! oc patch configmap osc-config -n default --type merge \ + -p '{"data":{"oscInstalled":"true"}}'; then + echo ">>> ERROR: failed to patch osc-config with oscInstalled=true" >&2 + return 1 + fi + + echo ">>> osc-config patched with oscInstalled=true"🤖 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 `@ci-operator/step-registry/sandboxed-containers-operator/install-osc-operator/sandboxed-containers-operator-install-osc-operator-commands.sh` around lines 628 - 631, Update the configmap patch command in the osc operator installation flow to remove the unconditional `|| true`, allowing patch failures to propagate and stop the step. Keep the success message only after a successfully completed `oc patch` invocation.
345-378: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winGating on every cluster CatalogSource is too broad.
An unrelated degraded catalog (community-operators, a mirror catalog, a leftover dev catalog from another step) blocks this step for 600s and then hard-fails the job even though the OSC subscription's own catalog is healthy. Restrict the wait to the catalog actually referenced by the subscription (
osc-operator-dev-catalogorredhat-operators) — Stage 1 already covers the dev case.🤖 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 `@ci-operator/step-registry/sandboxed-containers-operator/install-osc-operator/sandboxed-containers-operator-install-osc-operator-commands.sh` around lines 345 - 378, Update the Stage 0 CatalogSource readiness check to monitor only the catalog referenced by the OSC subscription: osc-operator-dev-catalog or redhat-operators. Exclude unrelated CatalogSources from catalog_states, readiness counts, progress output, and failure diagnostics, while preserving the existing wait and success behavior for the selected catalog.
409-418: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
.items[0]picks an arbitrary CSV in the namespace.There is no name filter, so any additional CSV in
${OSC_NAMESPACE}(a dependency, or a staleReplacingcopy) can be inspected instead of the sandboxed-containers CSV, and the gate then passes or fails on the wrong object. Select by name prefix instead.♻️ Suggested change
- if ! wait_until "CSV Succeeded" 600 5 \ - "[[ \"\$(oc get csv -n '${OSC_NAMESPACE}' -o jsonpath='{.items[0].status.phase}' 2>/dev/null)\" == \"Succeeded\" ]]"; then + if ! wait_until "CSV Succeeded" 600 5 \ + "[[ \"\$(oc get csv -n '${OSC_NAMESPACE}' -o jsonpath='{.items[?(@.spec.displayName==\"OpenShift sandboxed containers Operator\")].status.phase}' 2>/dev/null)\" == \"Succeeded\" ]]"; thenAlternatively resolve the CSV name from
subscription.status.currentCSVand wait on that object directly, which is the most robust option.🤖 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 `@ci-operator/step-registry/sandboxed-containers-operator/install-osc-operator/sandboxed-containers-operator-install-osc-operator-commands.sh` around lines 409 - 418, Update the Stage 4 CSV checks in the sandboxed-containers installation flow to target the sandboxed-containers CSV by name prefix instead of using arbitrary `.items[0]` entries. Apply the same selector when checking the phase and retrieving `csv_name`, while preserving the existing wait, diagnostic dump, and success behavior.ci-operator/config/openshift/sandboxed-containers-operator/openshift-sandboxed-containers-operator-devel__downstream-candidate422.yaml (1)
51-52: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPin the catalog image instead of
:latest.
osc-test-fbc:latestis a mutable Konflux tag, so this job's operator-under-test changes whenever that image is rebuilt — failures become unattributable to any commit in this repo and results aren't reproducible. Prefer a digest (@sha256:…) or a dated/versioned tag.🤖 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 `@ci-operator/config/openshift/sandboxed-containers-operator/openshift-sandboxed-containers-operator-devel__downstream-candidate422.yaml` around lines 51 - 52, Update OSC_CATALOG_SOURCE_IMAGE in the sandboxed-containers operator configuration to use an immutable image digest or dated/versioned tag instead of the mutable :latest tag. Keep OSC_INSTALL unchanged and ensure the pinned reference identifies a specific catalog image for reproducible runs.ci-operator/step-registry/sandboxed-containers-operator/install-osc-operator/sandboxed-containers-operator-install-osc-operator-ref.yaml (1)
7-12: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNo explicit
timeoutdespite multi-hour waits.
wait_for_kataconfigalone allows 7200s, plus ~2400s of operator/catalog waits. Without atimeouton the ref, the step inherits the default (which may be shorter than the script's worst case), andgrace_period: 10monly covers post-SIGTERM cleanup. Consider settingtimeoutexplicitly so the ref's contract matches the script's wait budget.⏱️ Suggested addition
grace_period: 10m + timeout: 3h0m0s commands: sandboxed-containers-operator-install-osc-operator-commands.sh🤖 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 `@ci-operator/step-registry/sandboxed-containers-operator/install-osc-operator/sandboxed-containers-operator-install-osc-operator-ref.yaml` around lines 7 - 12, Add an explicit timeout to the sandboxed-containers-operator install OSC operator ref configuration, alongside grace_period and commands, sized to cover the script’s maximum multi-hour wait budget plus cleanup time. Keep the existing resource requests and grace_period unchanged.
🤖 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
`@ci-operator/step-registry/sandboxed-containers-operator/install-osc-operator/sandboxed-containers-operator-install-osc-operator-commands.sh`:
- Around line 201-208: Update render_osc_operator_chart and
render_osc_operands_chart in
ci-operator/step-registry/sandboxed-containers-operator/install-osc-operator/sandboxed-containers-operator-install-osc-operator-commands.sh
at lines 201-208 and 305-312 to capture helm template stdout only; redirect
stderr to a temporary file used solely for failure diagnostics, while preserving
manifest output and nonzero error handling.
- Around line 515-537: Validate client_id, client_secret, and tenant_id after
extraction in the peer-pods secret setup before writing or creating
peer-pods-secret. If any required value is empty, fail the setup with a clear
non-sensitive error and avoid echoing credentials or creating the secret;
preserve the existing creation flow when all values are present.
- Around line 357-365: Fix the ready-catalog count assignment in the catalog
readiness loop so a zero-match grep result produces exactly one numeric value.
Update the logic around catalog_states, ready_catalogs, and the ready_catalogs
-eq total_catalogs comparison without appending a fallback line, preserving the
existing readiness behavior.
- Around line 253-296: Update the Helm argument construction in the provider
case branches to use --set-literal instead of --set for cloud configuration
values, especially AWS_SG_IDS and any other values that may contain commas.
Preserve the existing conditional inclusion and value paths while ensuring each
cloud value is passed as one literal Helm assignment; only retain --set if the
supported Helm version is below 3.12.
- Around line 130-151: Update fetch_osc_charts to support OSC_CHARTS_REF values
that are branches, tags, or commit SHAs: clone the repository, then resolve and
check out the requested ref explicitly when needed instead of passing a raw SHA
to git clone --branch. Remove the redundant mkdir -p before rm -rf, while
preserving the existing charts-directory selection and failure handling.
- Around line 580-588: The KataConfig readiness gate in the wait_until condition
must require both InProgress=False and completed node installation. Add a
positive readiness check by comparing status.kataNodes.readyNodeCount with the
expected worker-node count, while preserving the existing timeout and diagnostic
handling.
In
`@ci-operator/step-registry/sandboxed-containers-operator/install-osc-operator/sandboxed-containers-operator-install-osc-operator-ref.yaml`:
- Around line 3-6: Update the from_image configuration for the OSC install step
to use an OSC/kata-owned runtime image or a stable tag controlled by this
workflow instead of ci/rhdh-e2e-runner:main, while preserving access to helm,
oc, git, and jq.
---
Nitpick comments:
In
`@ci-operator/config/openshift/sandboxed-containers-operator/openshift-sandboxed-containers-operator-devel__downstream-candidate422.yaml`:
- Around line 51-52: Update OSC_CATALOG_SOURCE_IMAGE in the sandboxed-containers
operator configuration to use an immutable image digest or dated/versioned tag
instead of the mutable :latest tag. Keep OSC_INSTALL unchanged and ensure the
pinned reference identifies a specific catalog image for reproducible runs.
In
`@ci-operator/step-registry/sandboxed-containers-operator/install-osc-operator/sandboxed-containers-operator-install-osc-operator-commands.sh`:
- Around line 88-95: Remove the unused retry function from the script, since no
call sites invoke retry and the comment does not require integrating it into
other commands.
- Around line 97-124: Update the wait_until function to accept the condition
command and its arguments as separate parameters, invoke them directly with "$@"
instead of eval, and adjust all call sites to pass commands and interpolated
values as distinct arguments. Remove the embedded escaping and preserve the
existing timeout, progress, success, and failure behavior.
- Around line 298-303: Update the ENABLEPEERPODS=true branch in the
peerpods-param-cm lookup so a missing ConfigMap exits with a nonzero status
instead of only emitting a warning; preserve the existing helm_args behavior for
found configuration and the peerpods.enabled=false path when peer pods are
disabled.
- Around line 628-631: Update the configmap patch command in the osc operator
installation flow to remove the unconditional `|| true`, allowing patch failures
to propagate and stop the step. Keep the success message only after a
successfully completed `oc patch` invocation.
- Around line 345-378: Update the Stage 0 CatalogSource readiness check to
monitor only the catalog referenced by the OSC subscription:
osc-operator-dev-catalog or redhat-operators. Exclude unrelated CatalogSources
from catalog_states, readiness counts, progress output, and failure diagnostics,
while preserving the existing wait and success behavior for the selected
catalog.
- Around line 409-418: Update the Stage 4 CSV checks in the sandboxed-containers
installation flow to target the sandboxed-containers CSV by name prefix instead
of using arbitrary `.items[0]` entries. Apply the same selector when checking
the phase and retrieving `csv_name`, while preserving the existing wait,
diagnostic dump, and success behavior.
In
`@ci-operator/step-registry/sandboxed-containers-operator/install-osc-operator/sandboxed-containers-operator-install-osc-operator-ref.yaml`:
- Around line 7-12: Add an explicit timeout to the sandboxed-containers-operator
install OSC operator ref configuration, alongside grace_period and commands,
sized to cover the script’s maximum multi-hour wait budget plus cleanup time.
Keep the existing resource requests and grace_period unchanged.
🪄 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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: f0df4582-84bd-4cd5-a84b-9b3f63ae37dd
📒 Files selected for processing (7)
ci-operator/config/openshift/sandboxed-containers-operator/openshift-sandboxed-containers-operator-devel__downstream-candidate422.yamlci-operator/step-registry/sandboxed-containers-operator/install-osc-operator/OWNERSci-operator/step-registry/sandboxed-containers-operator/install-osc-operator/README.mdci-operator/step-registry/sandboxed-containers-operator/install-osc-operator/sandboxed-containers-operator-install-osc-operator-commands.shci-operator/step-registry/sandboxed-containers-operator/install-osc-operator/sandboxed-containers-operator-install-osc-operator-ref.metadata.jsonci-operator/step-registry/sandboxed-containers-operator/install-osc-operator/sandboxed-containers-operator-install-osc-operator-ref.yamlci-operator/step-registry/sandboxed-containers-operator/pre/sandboxed-containers-operator-pre-chain.yaml
| function fetch_osc_charts() { | ||
| local charts_dir="${SCRATCH}/charts" | ||
|
|
||
| echo ">>> Fetching OSC charts from: ${OSC_CHARTS_REPO} (ref: ${OSC_CHARTS_REF})" >&2 | ||
|
|
||
| mkdir -p "${charts_dir}" | ||
| rm -rf "${charts_dir}" | ||
| git clone --depth 1 --branch "${OSC_CHARTS_REF}" "${OSC_CHARTS_REPO}" "${charts_dir}" | ||
|
|
||
| if [[ ! -d "${charts_dir}" ]]; then | ||
| echo ">>> ERROR: Failed to clone charts repository" >&2 | ||
| exit 1 | ||
| fi | ||
|
|
||
| echo ">>> Charts fetched" >&2 | ||
| if [[ -d "${charts_dir}/charts" ]]; then | ||
| echo "${charts_dir}/charts" | ||
| else | ||
| echo "${charts_dir}" | ||
| fi | ||
| return 0 | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
--branch does not accept a commit SHA, but the env contract advertises commits.
OSC_CHARTS_REF is documented as "branch/tag/commit" in the ref (line 26) and README (line 31), yet git clone --depth 1 --branch <sha> fails for a raw commit SHA. Either narrow the documentation to branch/tag, or fetch the SHA explicitly.
Also note mkdir -p "${charts_dir}" on line 135 is immediately undone by rm -rf on line 136.
🔧 Suggested fix supporting any ref
- mkdir -p "${charts_dir}"
rm -rf "${charts_dir}"
- git clone --depth 1 --branch "${OSC_CHARTS_REF}" "${OSC_CHARTS_REPO}" "${charts_dir}"
+ git init -q "${charts_dir}"
+ git -C "${charts_dir}" remote add origin "${OSC_CHARTS_REPO}"
+ git -C "${charts_dir}" fetch --depth 1 origin "${OSC_CHARTS_REF}"
+ git -C "${charts_dir}" checkout -q FETCH_HEAD📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function fetch_osc_charts() { | |
| local charts_dir="${SCRATCH}/charts" | |
| echo ">>> Fetching OSC charts from: ${OSC_CHARTS_REPO} (ref: ${OSC_CHARTS_REF})" >&2 | |
| mkdir -p "${charts_dir}" | |
| rm -rf "${charts_dir}" | |
| git clone --depth 1 --branch "${OSC_CHARTS_REF}" "${OSC_CHARTS_REPO}" "${charts_dir}" | |
| if [[ ! -d "${charts_dir}" ]]; then | |
| echo ">>> ERROR: Failed to clone charts repository" >&2 | |
| exit 1 | |
| fi | |
| echo ">>> Charts fetched" >&2 | |
| if [[ -d "${charts_dir}/charts" ]]; then | |
| echo "${charts_dir}/charts" | |
| else | |
| echo "${charts_dir}" | |
| fi | |
| return 0 | |
| } | |
| function fetch_osc_charts() { | |
| local charts_dir="${SCRATCH}/charts" | |
| echo ">>> Fetching OSC charts from: ${OSC_CHARTS_REPO} (ref: ${OSC_CHARTS_REF})" >&2 | |
| rm -rf "${charts_dir}" | |
| git init -q "${charts_dir}" | |
| git -C "${charts_dir}" remote add origin "${OSC_CHARTS_REPO}" | |
| git -C "${charts_dir}" fetch --depth 1 origin "${OSC_CHARTS_REF}" | |
| git -C "${charts_dir}" checkout -q FETCH_HEAD | |
| if [[ ! -d "${charts_dir}" ]]; then | |
| echo ">>> ERROR: Failed to clone charts repository" >&2 | |
| exit 1 | |
| fi | |
| echo ">>> Charts fetched" >&2 | |
| if [[ -d "${charts_dir}/charts" ]]; then | |
| echo "${charts_dir}/charts" | |
| else | |
| echo "${charts_dir}" | |
| fi | |
| return 0 | |
| } |
🤖 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
`@ci-operator/step-registry/sandboxed-containers-operator/install-osc-operator/sandboxed-containers-operator-install-osc-operator-commands.sh`
around lines 130 - 151, Update fetch_osc_charts to support OSC_CHARTS_REF values
that are branches, tags, or commit SHAs: clone the repository, then resolve and
check out the requested ref explicitly when needed instead of passing a raw SHA
to git clone --branch. Remove the redundant mkdir -p before rm -rf, while
preserving the existing charts-directory selection and failure handling.
| local helm_output | ||
| if ! helm_output=$(helm template "${helm_args[@]}" 2>&1); then | ||
| echo ">>> ERROR: helm template failed" >&2 | ||
| echo "$helm_output" >&2 | ||
| return 1 | ||
| fi | ||
|
|
||
| echo "$helm_output" |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Both helm renderers merge stderr into the captured manifest. The shared root cause is helm_output=$(helm template ... 2>&1): on success, helm's stderr warnings (deprecation, coalesce, --set notices) land inside the captured YAML, which is then written to a manifest file and fed to oc apply, yielding a parse failure.
ci-operator/step-registry/sandboxed-containers-operator/install-osc-operator/sandboxed-containers-operator-install-osc-operator-commands.sh#L201-L208: capture stdout only inrender_osc_operator_chart, redirecting stderr to a temp file used solely for the failure diagnostic.ci-operator/step-registry/sandboxed-containers-operator/install-osc-operator/sandboxed-containers-operator-install-osc-operator-commands.sh#L305-L312: apply the identical change inrender_osc_operands_chart.
📍 Affects 1 file
ci-operator/step-registry/sandboxed-containers-operator/install-osc-operator/sandboxed-containers-operator-install-osc-operator-commands.sh#L201-L208(this comment)ci-operator/step-registry/sandboxed-containers-operator/install-osc-operator/sandboxed-containers-operator-install-osc-operator-commands.sh#L305-L312
🤖 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
`@ci-operator/step-registry/sandboxed-containers-operator/install-osc-operator/sandboxed-containers-operator-install-osc-operator-commands.sh`
around lines 201 - 208, Update render_osc_operator_chart and
render_osc_operands_chart in
ci-operator/step-registry/sandboxed-containers-operator/install-osc-operator/sandboxed-containers-operator-install-osc-operator-commands.sh
at lines 201-208 and 305-312 to capture helm template stdout only; redirect
stderr to a temporary file used solely for failure diagnostics, while preserving
manifest output and nonzero error handling.
| vxlan_port=$(echo "${cm_data}" | jq -r '.data.VXLAN_PORT // ""') | ||
| proxy_timeout=$(echo "${cm_data}" | jq -r '.data.PROXY_TIMEOUT // ""') | ||
| [[ -n "${vxlan_port}" ]] && helm_args+=("--set" "peerpods.providersConfigs.all.VXLAN_PORT=${vxlan_port}") | ||
| [[ -n "${proxy_timeout}" ]] && helm_args+=("--set" "peerpods.providersConfigs.all.PROXY_TIMEOUT=${proxy_timeout}") | ||
|
|
||
| case "${provider}" in | ||
| azure) | ||
| local azure_subnet_id azure_nsg_id azure_resource_group azure_region azure_instance_size | ||
| azure_subnet_id=$(echo "${cm_data}" | jq -r '.data.AZURE_SUBNET_ID // ""') | ||
| azure_nsg_id=$(echo "${cm_data}" | jq -r '.data.AZURE_NSG_ID // ""') | ||
| azure_resource_group=$(echo "${cm_data}" | jq -r '.data.AZURE_RESOURCE_GROUP // ""') | ||
| azure_region=$(echo "${cm_data}" | jq -r '.data.AZURE_REGION // ""') | ||
| azure_instance_size=$(echo "${cm_data}" | jq -r '.data.AZURE_INSTANCE_SIZE // ""') | ||
| [[ -n "${azure_subnet_id}" ]] && helm_args+=("--set" "peerpods.providersConfigs.azure.AZURE_SUBNET_ID=${azure_subnet_id}") | ||
| [[ -n "${azure_nsg_id}" ]] && helm_args+=("--set" "peerpods.providersConfigs.azure.AZURE_NSG_ID=${azure_nsg_id}") | ||
| [[ -n "${azure_resource_group}" ]] && helm_args+=("--set" "peerpods.providersConfigs.azure.AZURE_RESOURCE_GROUP=${azure_resource_group}") | ||
| [[ -n "${azure_region}" ]] && helm_args+=("--set" "peerpods.providersConfigs.azure.AZURE_REGION=${azure_region}") | ||
| # Let chart auto-set AZURE_INSTANCE_SIZE based on confidential mode if not explicitly set | ||
| [[ -n "${azure_instance_size}" ]] && helm_args+=("--set" "peerpods.providersConfigs.azure.AZURE_INSTANCE_SIZE=${azure_instance_size}") | ||
| ;; | ||
| aws) | ||
| local aws_region aws_subnet_id aws_vpc_id aws_sg_ids podvm_instance_type | ||
| aws_region=$(echo "${cm_data}" | jq -r '.data.AWS_REGION // ""') | ||
| aws_subnet_id=$(echo "${cm_data}" | jq -r '.data.AWS_SUBNET_ID // ""') | ||
| aws_vpc_id=$(echo "${cm_data}" | jq -r '.data.AWS_VPC_ID // ""') | ||
| aws_sg_ids=$(echo "${cm_data}" | jq -r '.data.AWS_SG_IDS // ""') | ||
| podvm_instance_type=$(echo "${cm_data}" | jq -r '.data.PODVM_INSTANCE_TYPE // ""') | ||
| [[ -n "${aws_region}" ]] && helm_args+=("--set" "peerpods.providersConfigs.aws.AWS_REGION=${aws_region}") | ||
| [[ -n "${aws_subnet_id}" ]] && helm_args+=("--set" "peerpods.providersConfigs.aws.AWS_SUBNET_ID=${aws_subnet_id}") | ||
| [[ -n "${aws_vpc_id}" ]] && helm_args+=("--set" "peerpods.providersConfigs.aws.AWS_VPC_ID=${aws_vpc_id}") | ||
| [[ -n "${aws_sg_ids}" ]] && helm_args+=("--set" "peerpods.providersConfigs.aws.AWS_SG_IDS=${aws_sg_ids}") | ||
| [[ -n "${podvm_instance_type}" ]] && helm_args+=("--set" "peerpods.providersConfigs.aws.PODVM_INSTANCE_TYPE=${podvm_instance_type}") | ||
| ;; | ||
| gcp) | ||
| local gcp_project_id gcp_zone gcp_network gcp_machine_type | ||
| gcp_project_id=$(echo "${cm_data}" | jq -r '.data.GCP_PROJECT_ID // ""') | ||
| gcp_zone=$(echo "${cm_data}" | jq -r '.data.GCP_ZONE // ""') | ||
| gcp_network=$(echo "${cm_data}" | jq -r '.data.GCP_NETWORK // ""') | ||
| gcp_machine_type=$(echo "${cm_data}" | jq -r '.data.GCP_MACHINE_TYPE // ""') | ||
| [[ -n "${gcp_project_id}" ]] && helm_args+=("--set" "peerpods.providersConfigs.gcp.GCP_PROJECT_ID=${gcp_project_id}") | ||
| [[ -n "${gcp_zone}" ]] && helm_args+=("--set" "peerpods.providersConfigs.gcp.GCP_ZONE=${gcp_zone}") | ||
| [[ -n "${gcp_network}" ]] && helm_args+=("--set" "peerpods.providersConfigs.gcp.GCP_NETWORK=${gcp_network}") | ||
| [[ -n "${gcp_machine_type}" ]] && helm_args+=("--set" "peerpods.providersConfigs.gcp.GCP_MACHINE_TYPE=${gcp_machine_type}") | ||
| ;; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
helm --set-string versus --set comma escaping list values
💡 Result:
When using Helm's --set and --set-string flags, both interpret commas as separators for multiple key-value pairs by default [1][2]. This often causes issues when a value itself contains a comma, as Helm will incorrectly split the value at the comma and attempt to parse the remainder as a new key-value pair [3][4]. To include a comma within a value, you must escape it using a backslash (\,) [3][4]. Key considerations: 1. Escaping: Using \, instructs Helm's parser to treat the comma as a literal character rather than a separator [3][4]. For example: helm install --set greeting='hello, world'. [4] 2. --set vs --set-string: Both flags exhibit this behavior because they share the same underlying parsing logic for command-line arguments [5][6]. There is no difference in how they handle comma escaping [5]. 3. Alternative - --set-literal: If you need to pass a literal string without worrying about complex escaping rules, Helm now supports the --set-literal flag [1][7]. This flag is designed to treat the provided value strictly as a string, avoiding the parsing issues associated with commas and other special characters [1][7]. This is generally the recommended approach if your Helm version supports it [7]. 4. Other Workarounds: If you find command-line escaping difficult or unreliable (especially with complex values or lists), consider using: - Values files: Use the --values (or -f) flag to pass a YAML file containing your values [8][1]. This avoids command-line parsing issues entirely [5][4]. - --set-file: Use this to read the value from a file, which is useful if the content is long or contains many special characters [8][1]. In summary, while \, is the standard escape method, using --set-literal is often a cleaner solution for passing strings containing commas [1][7].
Citations:
- 1: https://github.com/helm/helm-www/blob/main/docs/helm/helm_install.md
- 2: https://github.com/helm/helm-www/blob/main/versioned_docs/version-3/helm/helm_install.md
- 3: helm CLI incorrectly splits on commas even when quoted helm/helm#1556
- 4: helm install/upgrade does not support commas in set flag helm/helm#2952
- 5: Provide an option or alternative for --set and/or --set-string to take a value literally helm/helm#4030
- 6: Helm is interpreting commas in passwords wrongly when using --set or --set-string helm/helm#10663
- 7: Provide an alternative for --set and/or --set-string to take a value literally helm/helm#9182
- 8: https://helm.sh/docs/helm/helm_install/
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="ci-operator/step-registry/sandboxed-containers-operator/install-osc-operator/sandboxed-containers-operator-install-osc-operator-commands.sh"
echo "== file exists and line count =="
wc -l "$FILE"
echo "== relevant function excerpt =="
sed -n '220,305p' "$FILE" | cat -n
echo "== occurrences of helm_args and provider keys =="
rg -n 'helm_args|providersConfigs|VXLAN_PORT|PROXY_TIMEOUT|AWS_SG_IDS|AZURE|GCP' "$FILE"
echo "== any values files or literals in this script/package =="
git ls-files ci-operator/step-registry/sandboxed-containers-operator/install-osc-operator | xargs -r rg -n 'helm |values|ConfigMap|peerpods|providersConfigs|secret|AWS_SG_IDS|VXLAN_PORT'Repository: openshift/release
Length of output: 26941
🌐 Web query:
Helm command line flag --set-literal introduced version
💡 Result:
The Helm command line flag --set-literal was introduced in Helm version 3.12.0 [1][2]. This flag allows users to set values as literal strings on the command line without the special character interpretation or escaping typically required by the --set or --set-string flags [3][4][2].
Citations:
- 1: https://stackoverflow.com/questions/79272151/how-to-pass-brackets-and-other-special-characters-using-set-in-helm
- 2: https://github.com/helm/helm/releases/tag/v3.12.0
- 3: Provide an alternative for --set and/or --set-string to take a value literally helm/helm#9182
- 4: helm/helm@4516039
🌐 Web query:
site:github.com/helm/helm --set-string comma escaping
💡 Result:
When using Helm, the --set and --set-string flags treat commas as special characters used for internal list parsing, which can cause them to split values incorrectly [1][2][3]. To handle commas in your values, you have two primary options: 1. Use the --set-literal flag (Recommended) This flag was introduced specifically to treat values as literal strings without interpreting special characters like commas, backslashes, or brackets [4][5]. Example: helm install --set-literal mykey='value,with,commas' mychart 2. Escape the comma with a backslash If you must use --set or --set-string, you can escape the comma by preceding it with a backslash [3][6][7]. Note that depending on your shell environment, you may need to wrap the value in quotes to ensure the backslash is passed correctly to the Helm CLI [3][8]. Example: helm install --set mykey='value,with,commas' mychart If you find that escaping becomes too complex, especially with many special characters, the best practice is to provide the values via a separate YAML file using the --values (-f) flag instead [2][3][8][7].
Citations:
- 1: Helm is interpreting commas in passwords wrongly when using --set or --set-string helm/helm#10663
- 2: Provide an option or alternative for --set and/or --set-string to take a value literally helm/helm#4030
- 3: helm CLI incorrectly splits on commas even when quoted helm/helm#1556
- 4: Provide an alternative for --set and/or --set-string to take a value literally helm/helm#9182
- 5: helm/helm@4516039
- 6: Add
--set-valueflag that requires less escaping helm/helm#4406 - 7: helm install/upgrade does not support commas in set flag helm/helm#2952
- 8: helm CLI incorrectly splits on commas even when quoted helm/helm#1556
Pass peerpods cloud values as Helm literals.
--set and --set-string parse commas as separators, so a comma-separated AWS_SG_IDS=sg-1,sg-2 is split into separate assignments. In this CI context, use --set-literal for these values unless the Helm version is constrained below 3.12.
🤖 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
`@ci-operator/step-registry/sandboxed-containers-operator/install-osc-operator/sandboxed-containers-operator-install-osc-operator-commands.sh`
around lines 253 - 296, Update the Helm argument construction in the provider
case branches to use --set-literal instead of --set for cloud configuration
values, especially AWS_SG_IDS and any other values that may contain commas.
Preserve the existing conditional inclusion and value paths while ensuring each
cloud value is passed as one literal Helm assignment; only retain --set if the
supported Helm version is below 3.12.
| if [[ -n "${sp_json}" ]]; then | ||
| local decoded | ||
| decoded=$(echo "${sp_json}" | base64 -d) | ||
| local client_id client_secret tenant_id | ||
| client_id=$(echo "${decoded}" | jq -r '.clientId // .azure.azure_client_id // ""') | ||
| client_secret=$(echo "${decoded}" | jq -r '.clientSecret // .azure.azure_client_secret // ""') | ||
| tenant_id=$(echo "${decoded}" | jq -r '.tenantId // .azure.azure_tenant_id // ""') | ||
|
|
||
| local subscription_id | ||
| subscription_id=$(oc get secret azure-credentials -n kube-system -o jsonpath='{.data.azure_subscription_id}' 2>/dev/null | base64 -d || echo "") | ||
|
|
||
| cat > "${SCRATCH}/peer-pods-secret.json" <<EOF | ||
| { | ||
| "clientId": "${client_id}", | ||
| "clientSecret": "${client_secret}", | ||
| "tenantId": "${tenant_id}", | ||
| "subscriptionId": "${subscription_id}" | ||
| } | ||
| EOF | ||
| oc create secret generic peer-pods-secret \ | ||
| -n "${OSC_NAMESPACE}" \ | ||
| --from-file="${SCRATCH}/peer-pods-secret.json" | ||
| rm -f "${SCRATCH}/peer-pods-secret.json" |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Empty extracted credentials still produce a peer-pods-secret.
If neither key layout matches (.clientId / .azure.azure_client_id), jq returns "" and the secret is created with blank values; the failure then surfaces as an unexplained pod-VM creation error much later. Validate before creating, without echoing the values.
🛡️ Suggested guard
+ if [[ -z "${client_id}" || -z "${client_secret}" || -z "${tenant_id}" || -z "${subscription_id}" ]]; then
+ echo ">>> ERROR: incomplete Azure credentials extracted (empty clientId/clientSecret/tenantId/subscriptionId)" >&2
+ return 1
+ fi
cat > "${SCRATCH}/peer-pods-secret.json" <<EOF📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if [[ -n "${sp_json}" ]]; then | |
| local decoded | |
| decoded=$(echo "${sp_json}" | base64 -d) | |
| local client_id client_secret tenant_id | |
| client_id=$(echo "${decoded}" | jq -r '.clientId // .azure.azure_client_id // ""') | |
| client_secret=$(echo "${decoded}" | jq -r '.clientSecret // .azure.azure_client_secret // ""') | |
| tenant_id=$(echo "${decoded}" | jq -r '.tenantId // .azure.azure_tenant_id // ""') | |
| local subscription_id | |
| subscription_id=$(oc get secret azure-credentials -n kube-system -o jsonpath='{.data.azure_subscription_id}' 2>/dev/null | base64 -d || echo "") | |
| cat > "${SCRATCH}/peer-pods-secret.json" <<EOF | |
| { | |
| "clientId": "${client_id}", | |
| "clientSecret": "${client_secret}", | |
| "tenantId": "${tenant_id}", | |
| "subscriptionId": "${subscription_id}" | |
| } | |
| EOF | |
| oc create secret generic peer-pods-secret \ | |
| -n "${OSC_NAMESPACE}" \ | |
| --from-file="${SCRATCH}/peer-pods-secret.json" | |
| rm -f "${SCRATCH}/peer-pods-secret.json" | |
| if [[ -n "${sp_json}" ]]; then | |
| local decoded | |
| decoded=$(echo "${sp_json}" | base64 -d) | |
| local client_id client_secret tenant_id | |
| client_id=$(echo "${decoded}" | jq -r '.clientId // .azure.azure_client_id // ""') | |
| client_secret=$(echo "${decoded}" | jq -r '.clientSecret // .azure.azure_client_secret // ""') | |
| tenant_id=$(echo "${decoded}" | jq -r '.tenantId // .azure.azure_tenant_id // ""') | |
| local subscription_id | |
| subscription_id=$(oc get secret azure-credentials -n kube-system -o jsonpath='{.data.azure_subscription_id}' 2>/dev/null | base64 -d || echo "") | |
| if [[ -z "${client_id}" || -z "${client_secret}" || -z "${tenant_id}" || -z "${subscription_id}" ]]; then | |
| echo ">>> ERROR: incomplete Azure credentials extracted (empty clientId/clientSecret/tenantId/subscriptionId)" >&2 | |
| return 1 | |
| fi | |
| cat > "${SCRATCH}/peer-pods-secret.json" <<EOF | |
| { | |
| "clientId": "${client_id}", | |
| "clientSecret": "${client_secret}", | |
| "tenantId": "${tenant_id}", | |
| "subscriptionId": "${subscription_id}" | |
| } | |
| EOF | |
| oc create secret generic peer-pods-secret \ | |
| -n "${OSC_NAMESPACE}" \ | |
| --from-file="${SCRATCH}/peer-pods-secret.json" | |
| rm -f "${SCRATCH}/peer-pods-secret.json" |
🤖 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
`@ci-operator/step-registry/sandboxed-containers-operator/install-osc-operator/sandboxed-containers-operator-install-osc-operator-commands.sh`
around lines 515 - 537, Validate client_id, client_secret, and tenant_id after
extraction in the peer-pods secret setup before writing or creating
peer-pods-secret. If any required value is empty, fail the setup with a clear
non-sensitive error and avoid echoing credentials or creating the secret;
preserve the existing creation flow when all values are present.
| # Wait for KataConfig InProgress condition to be False (7200s / 2h) | ||
| if ! wait_until "KataConfig ready (InProgress=False)" 7200 30 \ | ||
| "[[ \"\$(oc get kataconfig '${kataconfig_name}' -o jsonpath='{.status.conditions[?(@.type==\"InProgress\")].status}' 2>/dev/null)\" == \"False\" ]]"; then | ||
| echo ">>> ERROR: KataConfig not ready after 2 hours" | ||
| oc get kataconfig "${kataconfig_name}" -o yaml || true | ||
| oc get nodes || true | ||
| oc get mcp || true | ||
| return 1 | ||
| fi |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# How do existing sandboxed-containers steps assert KataConfig readiness?
rg -n 'kataconfig' ci-operator/step-registry/sandboxed-containers-operator -i -C3Repository: openshift/release
Length of output: 22758
🌐 Web query:
KataConfig readyNodeCount condition status readyNodeCount
💡 Result:
In the context of the KataConfig custom resource (CR) used by OpenShift sandboxed containers, readyNodeCount is a field within the status.kataNodes stanza that represents the number of worker nodes that have successfully completed the installation of the Kata Containers runtime and are prepared to execute related workloads [1][2][3]. The status of the KataConfig CR provides visibility into the installation or uninstallation progress of Kata Containers across a cluster [1][4]. During the installation process: 1. The nodeCount field reflects the total number of worker nodes targeted for the installation [1][2]. 2. The readyNodeCount field increments as individual nodes complete their installation [1][3]. 3. Until a node is fully installed, it is not counted in the readyNodeCount [1][5]. 4. The installation is considered successful when all target nodes are listed as installed and the InProgress condition in the status transitions to False [1][3][6]. If readyNodeCount is 0, it indicates that none of the nodes have yet reached a state where they are ready to run Kata-based workloads [1][7]. This often occurs at the very beginning of the installation process or if the installation has not yet succeeded on any nodes [1][5]. You can monitor these values by inspecting the status field of the KataConfig resource using the command: oc get KataConfig -o yaml [4].
Citations:
- 1: https://docs.redhat.com/en/documentation/openshift_sandboxed_containers/1.11/html/deploying_red_hat_openshift_sandboxed_containers/kataconfig-status-messages
- 2: https://docs.redhat.com/en/documentation/openshift_sandboxed_containers/1.10/html/deploying_openshift_sandboxed_containers/kataconfig-status-messages
- 3: https://docs.redhat.com/en/documentation/openshift_sandboxed_containers/1.9/html/user_guide/kataconfig-status-messages
- 4: https://docs.redhat.com/en/documentation/openshift_container_platform/4.8/html-single/sandboxed_containers_support_for_openshift/index
- 5: https://docs.redhat.com/en/documentation/openshift_sandboxed_containers/1.6/html/user_guide/kataconfig-status-messages
- 6: https://docs.redhat.com/en/documentation/openshift_sandboxed_containers/1.9/pdf/user_guide/OpenShift_sandboxed_containers-1.9-User_guide-en-US.pdf
- 7: https://docs.redhat.com/en/documentation/openshift_sandboxed_containers/1.8/html/user_guide/kataconfig-status-messages
🌐 Web query:
KataConfig InProgress condition status False operator Ready condition KataConfig
💡 Result:
For a KataConfig custom resource (CR) in OpenShift Sandboxed Containers, the InProgress condition status of False (with no specified reason) indicates that the Kata runtime is successfully installed and configured on the cluster [1][2][3]. When the status of all worker nodes listed under kataNodes is set to installed and the InProgress condition is False, the installation is complete [1][2][3]. If the InProgress condition remains True or the installation appears stuck, it typically suggests that the operator is still reconciling, or that there is an underlying issue such as a node failing to become Ready [4][5][6]. In such cases, checking the operator logs and the status of the worker nodes is the recommended troubleshooting step [4][5][6].
Citations:
- 1: https://docs.redhat.com/en/documentation/openshift_sandboxed_containers/1.9/pdf/user_guide/OpenShift_sandboxed_containers-1.9-User_guide-en-US.pdf
- 2: https://docs.redhat.com/en/documentation/openshift_sandboxed_containers/1.12/html/deploying_openshift_sandboxed_containers_on_bare-metal_servers/configure-osc-overview_metal-osc
- 3: https://docs.redhat.com/en/documentation/openshift_sandboxed_containers/1.11/html/deploying_red_hat_openshift_sandboxed_containers/kataconfig-status-messages
- 4: https://www.redhat.com/en/blog/sandboxed-containers-operator-from-zero-to-hero-the-hard-way-part-2
- 5: https://raw.githubusercontent.com/validatedpatterns/docs/main/content/patterns/coco-pattern/coco-pattern-troubleshooting.adoc
- 6: https://validatedpatterns.io/patterns/coco-pattern/coco-pattern-troubleshooting/
🌐 Web query:
github openshift-sandboxed-containers-operator readyNodeCount InProgress False condition
💡 Result:
In the context of the OpenShift sandboxed containers (OSC) Operator, the conditions in the KataConfig custom resource (CR) provide insight into the installation status of the Kata runtime [1][2]. When the installation is complete, the InProgress condition transitions to False (with no specified reason), and all worker nodes under kataNodes are listed as installed with their readyNodeCount equal to the total nodeCount [1][3]. If you see InProgress: False but the installation appears incomplete or stuck, consider the following points based on authoritative documentation and troubleshooting guidance: 1. Understanding Status Transitions: During installation, the operator updates readyNodeCount incrementally as each worker node completes its configuration [1][3]. If readyNodeCount does not match your expected node count, the operator may still be working, or a node may have failed to reach a 'Ready' state [1][4]. 2. Potential Causes for "Stuck" States: - Node Readiness: The operator relies on the Machine Config Operator (MCO) to apply configurations. If a node remains in a NotReady state or the MachineConfigPool (MCP) is stuck in an UPDATING status, the Kata installation will appear stalled [4]. - Resource Issues: Check the node console or system logs for boot-time errors, and ensure virtualization is enabled, as this is a common cause for node ineligibility [5][4]. - Reconcile Delays: Occasionally, the operator may require additional time to reconcile. If the status is not updating after an extended period, you can manually trigger reconciliation by adding an annotation to the KataConfig CR [6]: oc annotate kataconfig reconcile-trigger="$(date)" --overwrite 3. Troubleshooting Steps: - Inspect the KataConfig CR for detailed status messages: oc describe kataconfig -n openshift-sandboxed-containers-operator [6]. - Check the operator logs for errors or stuck processes: oc logs -n openshift-sandboxed-containers-operator -l name=openshift-sandboxed-containers-operator -f [6]. - Verify the state of the MachineConfigPools to ensure configuration changes are propagating correctly [4]. - If issues persist, use the oc adm must-gather command with the appropriate OSC image to collect comprehensive diagnostic data [7][8].
Citations:
- 1: https://docs.redhat.com/en/documentation/openshift_sandboxed_containers/1.10/html/deploying_openshift_sandboxed_containers/kataconfig-status-messages
- 2: https://docs.redhat.com/pt-br/documentation/openshift_container_platform/4.10/html/sandboxed_containers_support_for_openshift/understanding-sandboxed-containers
- 3: https://docs.redhat.com/en/documentation/openshift_sandboxed_containers/1.5/html/openshift_sandboxed_containers_user_guide/deploying-sandboxed-containers-workloads
- 4: https://www.redhat.com/en/blog/sandboxed-containers-operator-from-zero-to-hero-the-hard-way-part-2
- 5: https://docs.redhat.com/it/documentation/openshift_container_platform/4.10/htmlsingle/sandboxed_containers_support_for_openshift/index
- 6: https://raw.githubusercontent.com/validatedpatterns/docs/main/content/patterns/coco-pattern/coco-pattern-troubleshooting.adoc
- 7: https://docs.redhat.com/en/documentation/openshift_container_platform/4.10/html/sandboxed_containers_support_for_openshift/troubleshooting-sandboxed-containers
- 8: https://docs.redhat.com/en/documentation/openshift_sandboxed_containers/1.11/html/deploying_red_hat_openshift_sandboxed_containers/troubleshooting
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the local wait_for_kataconfig function and nearby logic enough to verify whether
# the current gate has any built-in coverage for kataNodes/readyNodeCount or Ready.
sed -n '550,605p' ci-operator/step-registry/sandboxed-containers-operator/install-osc-operator/sandboxed-containers-operator-install-osc-operator-commands.sh
printf '\nRelevant status/status conditions usages in install step:\n'
rg -n 'kataNodes|readyNodeCount|In Progress|InProgress|condition|ready' ci-operator/step-registry/sandboxed-containers-operator/install-osc-operator/sandboxed-containers-operator-install-osc-operator-commands.sh || trueRepository: openshift/release
Length of output: 3985
Make the KataConfig readiness gate depend on node installation progress.
The current guard can pass as soon as status.conditions[?(@.type=="InProgress")].status is False, even if the operator has not yet installed Kata on the target nodes. Require a positive readiness indicator as the terminal condition, such as matching status.kataNodes.readyNodeCount to the expected worker-node count, in addition to InProgress=False.
🤖 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
`@ci-operator/step-registry/sandboxed-containers-operator/install-osc-operator/sandboxed-containers-operator-install-osc-operator-commands.sh`
around lines 580 - 588, The KataConfig readiness gate in the wait_until
condition must require both InProgress=False and completed node installation.
Add a positive readiness check by comparing status.kataNodes.readyNodeCount with
the expected worker-node count, while preserving the existing timeout and
diagnostic handling.
|
/pj-rehearse list |
|
/pj-rehearse periodic-ci-openshift-sandboxed-containers-operator-devel-downstream-candidate422-azure-ipi-kata |
|
/pj-rehearse list |
|
@tbuskey: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
2 similar comments
|
@tbuskey: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
@tbuskey: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
@tbuskey: job(s): periodic-ci-openshift-sandboxed-containers-operator-devel-downstream-candidate422-azure-ipi-kata either don't exist or were not found to be affected, and cannot be rehearsed |
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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
`@ci-operator/config/openshift/sandboxed-containers-operator/openshift-sandboxed-containers-operator-devel__downstream-candidate422.yaml`:
- Line 37: Update the azure-ipi-kata job configuration so
restrict_network_access is disabled, preserving external chart access required
by OSC_INSTALL and target C00102; only use an approved internal chart mirror if
that is the established alternative.
🪄 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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 4857b283-b832-4454-a5ba-e33748dc2084
📒 Files selected for processing (1)
ci-operator/config/openshift/sandboxed-containers-operator/openshift-sandboxed-containers-operator-devel__downstream-candidate422.yaml
|
/pj-rehearse periodic-ci-openshift-sandboxed-containers-operator-devel-downstream-candidate422-azure-ipi-kata |
|
/pj-rehearse list |
|
@tbuskey: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
1 similar comment
|
@tbuskey: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
The Helm chart's namespace template renders empty, so the namespace must be created explicitly before oc apply. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
/pj-rehearse periodic-ci-openshift-sandboxed-containers-operator-devel-downstream-candidate422-azure-ipi-kata |
|
@tbuskey: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
/pj-rehearse periodic-ci-openshift-sandboxed-containers-operator-devel-downstream-candidate422-azure-ipi-kata |
|
@tbuskey: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
Signed-off-by: Tom Buskey <tbuskey@redhat.com>
Fix fetch_osc_charts: use correct path for DEBUG startingCSV patch, add missing fi, redirect DEBUG echo to stderr, and move final echo to end of function. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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
`@ci-operator/step-registry/sandboxed-containers-operator/install-osc-operator/sandboxed-containers-operator-install-osc-operator-commands.sh`:
- Around line 152-155: Update the subscription.yaml patching block in the
install script to stop masking sed failures: remove the unconditional “|| true”
from the sed command so a failed edit aborts the step, or explicitly verify
afterward that startingCSV was removed before reporting the patch as successful.
- Around line 253-255: Remove the make -C "${operands_chart}" ssh-keys
invocation from the command script; do not execute any target from the cloned
chart repository with CI credentials in scope. Generate the required ed25519
keys directly within this trusted script, writing them to the chart’s files/
directory, or invoke only an immutable integrity-verified helper with tracing
disabled for the minimal sensitive scope.
🪄 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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 6bbfa0da-57ed-4733-b83b-b9c530a487a0
📒 Files selected for processing (4)
ci-operator/config/openshift/sandboxed-containers-operator/openshift-sandboxed-containers-operator-devel__downstream-candidate422.yamlci-operator/step-registry/sandboxed-containers-operator/install-osc-operator/README.mdci-operator/step-registry/sandboxed-containers-operator/install-osc-operator/sandboxed-containers-operator-install-osc-operator-commands.shci-operator/step-registry/sandboxed-containers-operator/install-osc-operator/sandboxed-containers-operator-install-osc-operator-ref.yaml
💤 Files with no reviewable changes (1)
- ci-operator/config/openshift/sandboxed-containers-operator/openshift-sandboxed-containers-operator-devel__downstream-candidate422.yaml
| # DEBUG until https://github.com/confidential-devhub/charts/pull/3 merges | ||
| if grep -q 'startingCSV.*$' "${result_dir}/osc-operator/templates/subscription.yaml" 2>/dev/null; then | ||
| sed -ie 's/startingCSV.*$//' "${result_dir}/osc-operator/templates/subscription.yaml" || true | ||
| echo ">>> DEBUG: subscription.yaml patched. PR 3 not merged yet." >&2 |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Do not mask a failed chart patch.
sed ... || true lets the script report a successful patch even when the edit fails. Helm then receives the unpatched template and the installation fails later with a misleading error. Let the edit failure abort the step or verify the postcondition explicitly.
🤖 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
`@ci-operator/step-registry/sandboxed-containers-operator/install-osc-operator/sandboxed-containers-operator-install-osc-operator-commands.sh`
around lines 152 - 155, Update the subscription.yaml patching block in the
install script to stop masking sed failures: remove the unconditional “|| true”
from the sed command so a failed edit aborts the step, or explicitly verify
afterward that startingCSV was removed before reporting the patch as successful.
| # Generate SSH keys via the chart's Makefile (ed25519, into files/ for .Files.Get) | ||
| make -C "${operands_chart}" ssh-keys >&2 | ||
|
|
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="ci-operator/step-registry/sandboxed-containers-operator/install-osc-operator/sandboxed-containers-operator-install-osc-operator-commands.sh"
printf 'File exists: '
test -f "$FILE" && echo yes || echo no
printf '\nRelevant lines 200-280:\n'
sed -n '200,280p' "$FILE" | nl -ba -v200
printf '\nSearch OSC_CHARTS_REPO/REF/chart/operands_chart references:\n'
rg -n "OSC_CHARTS_REPO|OSC_CHARTS_REF|OPERAND_CHART|operands_chart|ssh-keys|make -C" "$FILE"
printf '\nGit diff stat/name:\n'
git diff --stat -- "$FILE" || true
git diff -- "$FILE" | sed -n '1,220p' || trueRepository: openshift/release
Length of output: 238
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="ci-operator/step-registry/sandboxed-containers-operator/install-osc-operator/sandboxed-containers-operator-install-osc-operator-commands.sh"
printf 'Git status/stat:\n'
git status --short -- "$FILE" || true
git diff HEAD -- "$FILE" | sed -n '1,260p' || true
printf '\nRelevant lines 200-280:\n'
awk 'NR>=200 && NR<=280 { printf "%d:\t%s\n", NR, $0 }' "$FILE"
printf '\nSearch OSC_CHARTS_REPO/REF/chart/operands_chart/ssh-keys/make references:\n'
grep -nE "OSC_CHARTS_REPO|OSC_CHARTS_REF|OPERAND_CHART|operands_chart|ssh-keys|make -C" "$FILE" || true
printf '\nTraces/env/secrets around relevant section:\n'
grep -nE "set .*x|OSC_|osd-admin/kubeconfig|kubeconfig|credentials|make -C|ssh-keys|operands_chart|tmp|SHARED_DIR" "$FILE" | sed -n '1,220p'Repository: openshift/release
Length of output: 10376
Do not execute the chart Makefile in this command script.
OSC_CHARTS_REPO/OSC_CHARTS_REF control the repository cloned at line 137, then make -C "${operands_chart}" ssh-keys runs an arbitrary target from that repository with CI credentials in scope. Generate the SSH keys inside this trusted script instead, or use an immutable, integrity-verified helper with minimal tracing disabled scope.
🤖 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
`@ci-operator/step-registry/sandboxed-containers-operator/install-osc-operator/sandboxed-containers-operator-install-osc-operator-commands.sh`
around lines 253 - 255, Remove the make -C "${operands_chart}" ssh-keys
invocation from the command script; do not execute any target from the cloned
chart repository with CI credentials in scope. Generate the required ed25519
keys directly within this trusted script, writing them to the chart’s files/
directory, or invoke only an immutable integrity-verified helper with tracing
disabled for the minimal sensitive scope.
|
/pj-rehearse list |
|
@tbuskey: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
/pj-rehearse periodic-ci-openshift-sandboxed-containers-operator-devel-downstream-candidate422-azure-ipi-kata |
|
@tbuskey: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
[REHEARSALNOTIFIER]
A total of 56 jobs have been affected by this change. The above listing is non-exhaustive and limited to 25 jobs. A full list of affected jobs can be found here Interacting with pj-rehearseComment: Once you are satisfied with the results of the rehearsals, comment: |
1 participant
…azure-ipi-kata
Add new step-registry ref that installs the OSC operator and configures the cluster using Helm charts (rhdh-e2e-runner base image, git clone pattern). Enable OSC_INSTALL=true in candidate422 azure-ipi-kata with test scenario C00102.
/hold
Summary by CodeRabbit
sandboxed-containers-operator-install-osc-operator) that conditionally installs the OpenShift Sandboxed Containers (OSC) operator and configures operands/feature-gates via Helm (supportingkata,peer-pods, andcoco), including provider-specificpeer-pods-secrethandling for Azure/AWS and optional custom OSC operatorCatalogSourceoverride viaCATALOG_SOURCE_IMAGE.OSC_INSTALL, defaultfalse), and supporting parameters/metadata (OWNERS, ref yaml + metadata, README), plus updates the step tooling to useCATALOG_SOURCE_IMAGE(renaming from the prior OSC-specific variable).sandboxed-containers-operator-prechain ahead of the existingsandboxed-containers-operator-install-trustee-operatorstep.azure-ipi-kataconfiguration forcandidate422to enable OSC installation for scenarioC00102:OSC_INSTALL: "true",SLEEP_DURATION: 8h,TEST_SCENARIOS: C00102restrict_network_accesstotrueCATALOG_SOURCE_IMAGE(and removes any prior OSC-prefixed catalog source env var from the job env)oc create namespace "${OSC_NAMESPACE}").