Repository navigation
Conversation
Key changes: - Migrate OSAC deployment from make install to setup.sh with retry logic - Add OSAC-2529 workaround for Helm ownership on vast-tenant-config-shared secret - Add destroy-caas playbook and role for CaaS teardown - Replace single values-file patching with overlay-based approach (osac-devel) - Use separate caas_ocp_version variable for CaaS discovery ISO - Remove LVM disk recreation from destroy-ocp - Remove cherry-pick workarounds from osac_refresh (fixed upstream) - Update README to reflect setup.sh-based deployment flow - Bump vendored ansible.posix, ansible.utils, netris.controller collections Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Dan Manor <dmanor@redhat.com>
- Fix missing newline at end of README.md (pre-commit end-of-file-fixer) - Revert vendored collection diffs (ansible.posix, ansible.utils, netris.controller) — not part of this sync Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Dan Manor <dmanor@redhat.com>
Rename role directories to use consistent hyphen-case naming. Remove assisted_service, ocp_install, vm_resize roles (Assisted Installer path removed). Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Dan Manor <dmanor@redhat.com>
Create hyphenated playbooks aligned with Make targets. Delete old underscore-named and Assisted-Installer-path playbooks (setup-ocp.yml, install-ocp.yml, site.yml). Update stale role references in prep-refresh-osac comments. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Dan Manor <dmanor@redhat.com>
Align Make targets with contract names. Add deploy-ocp target. Add OSAC_DEPLOY_MODE toggle (fresh/snapshot) for deploy-osac. Remove Assisted Installer targets (deploy, deploy-ocp, deploy-fast), snapshot-refresh sub-targets, and workaround targets. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Dan Manor <dmanor@redhat.com>
Add deploy-ocp and destroy-ocp targets to root Makefile. Update e2e pipeline: setup-infra -> deploy-infra -> deploy-ocp -> deploy-osac -> setup-suite -> run-tests. Add osac_deploy_mode variable (fresh/snapshot) with flavor derivation. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Dan Manor <dmanor@redhat.com>
Split the deploy-osac CI step into deploy-ocp (snapshot restore) and deploy-osac (OSAC install), matching the updated contract. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Dan Manor <dmanor@redhat.com>
The prep-osac role uses osac_values_file to determine where to copy license and pull-secret files, but it defaulted to values/development/values.yaml while the Makefile's run-osac-setup passes VALUES_FILE=values/caas-ci/values.yaml. This caused the license file to be copied to the wrong directory, breaking install-secrets. Align both defaults to values/caas-ci/values.yaml and osac-e2e-ci namespace, matching what CI actually uses. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Dan Manor <dmanor@redhat.com>
|
@danmanor: This pull request references OSAC-2801 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: danmanor 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 |
|
Warning Review limit reached
Next review available in: 36 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository: osac-project/coderabbit/.coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
WalkthroughThe PR adds a reusable CaaS/Netris E2E workflow, separates OCP and OSAC deployment phases, supports fresh and snapshot OSAC modes, adds CaaS provisioning and teardown automation, renames infrastructure roles and playbooks, and expands diagnostics collection. ChangesNetris E2E lifecycle
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (3 errors)
✅ Passed checks (8 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 |
Add .superpowers/ to .gitignore to prevent plan/review artifacts from being committed. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Dan Manor <dmanor@redhat.com>
Replace the skeleton caller with a real workflow that triggers on PRs to main. Calls e2e-caas-netris.yml (which now uses the deploy-ocp + deploy-osac fresh install path) with concurrency groups to cancel in-progress runs when a PR is updated. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Dan Manor <dmanor@redhat.com>
In fresh mode, the osac-prereqs chart creates AgentServiceConfig with only the target OCP version's osImage entry, so infrastructure-operator never populates OS_IMAGES with all default versions. Scaling it down breaks setup-caas which needs it running to regenerate the assisted-service auth keypair. In snapshot mode, the baked-in snapshot has all default OS_IMAGES, so the scale-down and filtering remain necessary. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Dan Manor <dmanor@redhat.com>
Create the reusable workflow (copy of e2e-caas-netris.yml with updated name) and update the caller to point at it with pull_request trigger for presubmit CI. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Dan Manor <dmanor@redhat.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 33
🤖 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 `@infra/netris/inventory/group_vars/all.yml`:
- Around line 73-81: The snapshot flavor cache identity must change when the
resolved snapshot_flavor_image changes. Update the snapshot-flavor caching logic
used by roles/snapshot-pull to include a sanitized image digest/tag in the cache
path or persist and compare the resolved image before reusing
snapshot_flavor_dir/disk-0.qcow2, while preserving the existing cache reuse
behavior for matching images.
In `@infra/netris/Makefile`:
- Around line 85-91: Update the destroy-osac target to stop suppressing
uninstall errors and remove the unconditional “|| true” behavior. Preserve
success when the OSAC release is explicitly absent, but propagate all other
uninstall failures and only remove the installer directories and environment
file after a successful or explicitly absent uninstall.
- Around line 10-12: Synchronize the deploy-mode contract around
OSAC_DEPLOY_MODE, EXTRA_VARS, and ANSIBLE_EXTRA: validate OSAC_DEPLOY_MODE as an
accepted mode before selecting the deployment branch, and always pass its value
to Ansible as osac_deploy_mode. Ensure this explicit Make value takes precedence
over any EXTRA_VARS value and is included in every relevant Ansible invocation,
including setup-infra.
In `@infra/netris/README.md`:
- Around line 221-228: The fresh-flow documentation must match the defaults used
by make deploy-osac. Update the README’s prep-osac, run-osac-setup, and
post-osac descriptions to state that post-osac is skipped in fresh mode and that
MCE scale-down and image filtering apply only to snapshot mode; replace the
outdated osac-devel namespace and development values-file references with
osac-e2e-ci and values/caas-ci/values.yaml.
In `@infra/netris/roles/caas-create/tasks/main.yml`:
- Around line 1-28: Consolidate the cluster lookup so the initial `osac get
clusters` task parses and registers the existing cluster’s ID (or an absent
marker) in `_existing_cluster`, then use that result for the create condition
and downstream ID reuse. Remove the separate “Get existing cluster ID” task and
replace references to `_existing_cluster_id` with `_existing_cluster.stdout`,
preserving creation only when the named cluster is absent.
- Around line 37-54: Update the “Wait for cluster to be ready” task’s until
condition to stop retrying when the state is any terminal value, including
CLUSTER_STATE_READY and CLUSTER_STATE_FAILED, matching the established pattern
used by _publish_status. Keep the separate “Fail if cluster ended in FAILED
state” task to report failure after polling stops, and remove the rc/failed_when
early-exit mechanism if it is no longer needed.
In `@infra/netris/roles/caas-discovery/tasks/main.yml`:
- Around line 10-26: Remove the redundant dnsmasq restart by updating the
“Ensure dnsmasq listens on br-mgmt for agent DNS” task and its related restart
flow. Keep a single restart when _dnsmasq_mgmt reports changes, and ensure the
existing systemd restart behavior remains intact without triggering both the
notify handler and explicit task.
In `@infra/netris/roles/caas-setup/tasks/main.yml`:
- Around line 187-196: Add no_log: true to the “Launch publish-templates job”
task that invokes ansible.builtin.uri with _aap_admin_pw.stdout, matching the
adjacent task’s secret-handling configuration.
- Around line 58-73: Update the “Label agent with netris server name” task to
safely handle an empty selectattr result before accessing netris_server_name.
Add a clear diagnostic and fail deliberately when an agent hostname has no
matching caas_agents vm_pattern; otherwise preserve the existing label command
behavior.
- Around line 140-165: Add no_log: true to the “Get service account token” task
registering _sa_token and to both consuming tasks, “Create host type” and
“Create cluster catalog item,” so the bearer token and related request output
are suppressed. Preserve the existing token retrieval, Authorization headers,
and task behavior.
In `@infra/netris/roles/deploy-infra/tasks/main.yml`:
- Around line 9-16: Update the task around “Ensure br-mgmt bridge exists” so
bridge creation remains conditional, but management address configuration runs
on every invocation. Replace the creation-only ip addr add behavior with address
reconciliation, such as ip addr replace using mgmt_bridge_cidr, while preserving
the final bridge-up operation.
In `@infra/netris/roles/deploy-infra/tasks/preflight.yml`:
- Line 23: Update the KVM preflight checks using the visible grep commands so
they recognize both Intel vmx and AMD svm virtualization flags, while preserving
the existing validation behavior. Apply the same change to both occurrences, or
replace the CPU-flag checks with validation that /dev/kvm is available.
In `@infra/netris/roles/destroy-caas/tasks/main.yml`:
- Around line 82-105: Update the discovery VM teardown tasks to process every VM
returned for each pattern, rather than reducing each stdout_lines result with
map('first'). After force-stopping each VM in the existing “Shut down running
VMs” flow, explicitly undefine its libvirt domain before removing its disk,
using the VM names collected in caas_discovery_vms.
- Around line 14-21: Update the “Delete cluster (fire-and-forget)” task to use a
realistic async timeout by increasing its async value from 1 to 600, while
preserving poll: 0 and the existing command and failure behavior.
In `@infra/netris/roles/destroy-infra/tasks/main.yml`:
- Around line 37-41: Replace the hardcoded dnsmasq config path in
infra/netris/roles/destroy-infra/tasks/main.yml lines 37-41 with
/etc/dnsmasq.d/{{ ocp_cluster_name | default('ocp-sno') }}.conf, matching the
ISO naming logic. Apply the same derived path in
infra/netris/roles/gather-infra/tasks/main.yml lines 198-201 so diagnostics
collect the deployed cluster’s config.
- Around line 2-4: Add error-tolerant handling to the netris-lab variable
loading task using ansible.builtin.include_vars, matching the existing guard in
gather/tasks/main.yml. Ensure a missing group_vars/all.yml does not abort the
teardown and allows subsequent cleanup tasks to run.
In `@infra/netris/roles/gather-caas/tasks/main.yml`:
- Around line 2-13: The namespace-detection block is duplicated and should run
once in the parent gather role. In infra/netris/roles/gather-caas/tasks/main.yml
lines 2-13, remove the probe and _osac_namespace set_fact, then consume the
parent-provided _osac_namespace; make the same removal and consumption change in
infra/netris/roles/gather-infra/tasks/main.yml lines 2-13. Add the shared
_osac_namespace computation alongside _gather_dir in
infra/netris/roles/gather/tasks/main.yml so both sub-roles use the single
detected value.
- Around line 31-36: Update the shell command fallbacks in the gather-caas tasks
and corresponding gather-infra tasks so a failed oc command preserves its
captured stdout/stderr instead of overwriting the artifact with a placeholder.
Replace the clobbering fallback redirection in each repeated “no X” pattern with
an append operation, while retaining the existing output paths and failure
behavior.
- Around line 318-333: Update the “Extract hosted cluster kubeconfig” shell task
to remove any existing /tmp/caas-hosted-kubeconfig before checking for the
secret, write newly extracted credentials with restrictive permissions (for
example by creating the file with mode 0600), and ensure failed or unavailable
extraction cannot leave a stale file for later tasks to use. Preserve the
existing status-file messages and successful extraction flow.
In `@infra/netris/roles/gather-infra/tasks/main.yml`:
- Around line 146-164: Use the defined netris_username variable in the Login to
Netris controller task instead of netris_login, preserving the existing password
and request behavior. Update the _gather_netris_url construction to apply the
fallback when _gather_traefik_np.stdout is empty as well as undefined, so an
empty nodePort yields port 0.
- Around line 416-424: Update the _sno_ssh definition used by the gather tasks
to include non-interactive BatchMode=yes and disable known-host persistence with
UserKnownHostsFile=/dev/null, while retaining the existing connection timeout.
Ensure all kubelet, crio, and other SNO SSH diagnostics inherit these options so
authentication failures and recreated VM host keys cannot block or abort the
gather.
- Around line 166-184: Update the “Gather Netris VPCs”, “Gather Netris VNets”,
“Gather Netris subnets”, and “Gather Netris NAT rules” tasks to pass the session
cookie through an environment variable, following the existing GATHER_TOKEN
pattern in the CaaS gather tasks. Remove _gather_netris_cookie.stdout from each
shell command and reference the environment variable in curl, ensuring the
cookie is not exposed in rendered command or failure output.
In `@infra/netris/roles/netris-configure/tasks/main.yml`:
- Line 120: Update the condition in the server-cluster-template task to
normalize both response shapes before filtering: use the direct JSON list when
`_sct_list.json` is a list, otherwise use `_sct_list.json.data`. Apply the
existing name match against that normalized template collection so an existing
template skips the POST in either response format.
In `@infra/netris/roles/ocp-dns/tasks/main.yml`:
- Around line 16-39: Extend the Route 53 preflight around the existing
_r53_existing check to query api, api-int, and *.apps records that the later
UPSERT modifies. Validate each returned value against ocp_dnat_ip, and fail with
the existing ownership-protection behavior if any record exists with a different
value, while allowing absent records or matching values.
In `@infra/netris/roles/post-refresh-osac/tasks/main.yml`:
- Around line 2-16: Extract the duplicated socat env-file and systemd setup from
the refresh role into a shared task file, then include that file from both the
prep and refresh role task flows. Preserve the existing task names, service
name, configuration values, and startup behavior while ensuring both roles use
the same shared implementation.
- Around line 20-26: Add the same retry behavior used by the equivalent Traefik
discovery task in prep-osac to the “Discover Netris controller URL” task: wait
until _traefik_nodeport.stdout is non-empty, retry up to 10 times with a
15-second delay, and preserve the existing command, environment, registration,
and changed_when settings.
- Around line 64-108: Quote or otherwise safely shell-escape all secret
interpolations in the cluster-fulfillment-ig secret task, especially
netris_password, aws_access_key_id, and aws_secret_access_key, so values cannot
alter command parsing. Add pipefail handling to both cluster-fulfillment-ig
configmap and secret shell commands before their oc create | oc apply pipelines,
preserving no_log on the secret task.
In `@infra/netris/roles/prep-osac/tasks/main.yml`:
- Around line 111-149: Fix double-encoding of _netris_resource_class_map in
infra/netris/roles/prep-osac/tasks/main.yml:111-149 and
infra/netris/roles/post-refresh-osac/tasks/main.yml:59-83 by removing the
to_json filter when assigning NETRIS_RESOURCE_CLASS_MAP, since the folded
set_fact already stores a JSON string. Keep the resulting configuration as valid
JSON without added quotes or escaping at both affected sites.
- Around line 123-191: Add no_log: true to the “Build values overrides” Ansible
set_fact task that constructs _values_overrides, ensuring its rendered
secret-bearing values are suppressed from verbose and CI logs.
In `@infra/netris/roles/prep-refresh-osac/tasks/main.yml`:
- Around line 18-27: Update the “Cherry-pick upstream fixes for
refresh-after-snapshot” task to fail when the cherry-pick command fails and
clean up the repository by aborting the cherry-pick or resetting the partial
state before the play stops. Preserve the existing skip condition and successful
cherry-pick behavior, and add a tracking JIRA comment for the temporary
branch/commit workaround.
- Around line 29-53: Replace the unverified in-place patching in the “Patch
refresh-after-snapshot.py for snapshot compatibility” task with guarded
transformations that validate each required source pattern or marker before
editing, including the upgrade_osac/helm region check without unsafe split
indexing. Fail clearly when an expected pattern is absent, and set changed
status based on whether the file was actually modified rather than
unconditionally using changed_when: true.
In `@infra/netris/roles/snapshot-pull/tasks/main.yml`:
- Around line 1-6: Update the _flavor_cached validation and its downstream
checks to confirm the entire snapshot flavor is complete, rather than only
statting disk-0.qcow2. Verify every expected disk or use a completion marker
created only after all disks are successfully reassembled, and ensure the
pull/reassemble flow remains active when any disk is missing or incomplete.
- Around line 90-114: Update the “Reassemble disk images from compressed chunks”
shell task to process .zst and .gz chunks in explicit numeric order rather than
relying on glob expansion, preserving the intended disk sequence across layers.
After reassembly, add an integrity check for each generated disk-*.qcow2 and
fail the task if validation fails.
🪄 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: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f59e21e1-07cf-45e1-90f0-86b96cac083a
📒 Files selected for processing (53)
.github/workflows/e2e-caas-netris-full-install-caller.yml.github/workflows/e2e-caas-netris.yml.gitignoreMakefileinfra/netris/Makefileinfra/netris/README.mdinfra/netris/contract.mkinfra/netris/inventory/group_vars/all.ymlinfra/netris/playbooks/connectivity.ymlinfra/netris/playbooks/deploy-caas.ymlinfra/netris/playbooks/deploy-infra.ymlinfra/netris/playbooks/deploy-ocp.ymlinfra/netris/playbooks/destroy-caas.ymlinfra/netris/playbooks/destroy-infra.ymlinfra/netris/playbooks/destroy-ocp.ymlinfra/netris/playbooks/destroy.ymlinfra/netris/playbooks/gather-caas.ymlinfra/netris/playbooks/gather-infra.ymlinfra/netris/playbooks/gather-lab.ymlinfra/netris/playbooks/gather.ymlinfra/netris/playbooks/install-ocp.ymlinfra/netris/playbooks/post-osac.ymlinfra/netris/playbooks/post-refresh-osac.ymlinfra/netris/playbooks/prep-osac.ymlinfra/netris/playbooks/prep-refresh-osac.ymlinfra/netris/playbooks/setup-caas.ymlinfra/netris/playbooks/setup-infra.ymlinfra/netris/playbooks/setup-ocp.ymlinfra/netris/playbooks/site.ymlinfra/netris/roles/assisted_service/tasks/main.ymlinfra/netris/roles/caas-create/tasks/main.ymlinfra/netris/roles/caas-discovery/handlers/main.ymlinfra/netris/roles/caas-discovery/tasks/main.ymlinfra/netris/roles/caas-discovery/templates/infraenv.yml.j2infra/netris/roles/caas-setup/tasks/main.ymlinfra/netris/roles/deploy-infra/tasks/main.ymlinfra/netris/roles/deploy-infra/tasks/preflight.ymlinfra/netris/roles/destroy-caas/tasks/main.ymlinfra/netris/roles/destroy-infra/tasks/main.ymlinfra/netris/roles/gather-caas/tasks/main.ymlinfra/netris/roles/gather-infra/tasks/main.ymlinfra/netris/roles/gather/tasks/main.ymlinfra/netris/roles/netris-configure/tasks/main.ymlinfra/netris/roles/ocp-dns/handlers/main.ymlinfra/netris/roles/ocp-dns/tasks/main.ymlinfra/netris/roles/ocp_install/tasks/main.ymlinfra/netris/roles/osac_install/tasks/main.ymlinfra/netris/roles/post-refresh-osac/tasks/main.ymlinfra/netris/roles/prep-osac/tasks/main.ymlinfra/netris/roles/prep-refresh-osac/tasks/main.ymlinfra/netris/roles/setup-infra/tasks/main.ymlinfra/netris/roles/snapshot-pull/tasks/main.ymlinfra/netris/roles/vm_resize/tasks/main.yml
💤 Files with no reviewable changes (10)
- infra/netris/playbooks/install-ocp.yml
- infra/netris/playbooks/destroy.yml
- infra/netris/roles/vm_resize/tasks/main.yml
- infra/netris/playbooks/setup-ocp.yml
- infra/netris/playbooks/gather-lab.yml
- infra/netris/playbooks/gather.yml
- infra/netris/playbooks/site.yml
- infra/netris/roles/assisted_service/tasks/main.yml
- infra/netris/roles/osac_install/tasks/main.yml
- infra/netris/roles/ocp_install/tasks/main.yml
Comment on lines
85
to
+91
| destroy-osac: | ||
| @echo "=== Tearing down OSAC ===" | ||
| cd /opt/osac-installer && source /tmp/osac-setup.env && \ | ||
| EXTRA_SERVICES=true ./scripts/teardown.sh || true | ||
| oc delete namespace osac-devel shared --ignore-not-found --wait=false 2>/dev/null || true | ||
| rm -rf /opt/osac-installer | ||
| rm -f /tmp/osac-setup.env | ||
| cd /opt/osac-installer && make uninstall \ | ||
| INSTALLER_NAMESPACE=$(OSAC_NAMESPACE) \ | ||
| VALUES_FILE=$(OSAC_VALUES_FILE) 2>/dev/null || true | ||
| rm -rf /opt/osac-installer /opt/osac-installer-full | ||
| @rm -f $(ENV_INFRA) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Do not hide failed OSAC uninstalls.
2>/dev/null || true turns Helm failures into a successful teardown, then removes the installer and environment file. Subsequent deployments can collide with orphaned releases and the failure is no longer diagnosable. Only ignore an explicitly absent release; otherwise fail the target.
🧰 Tools
🪛 checkmake (0.3.2)
[warning] 85-85: Target body for "destroy-osac" exceeds allowed length of 5 lines (6).
(maxbodylength)
🤖 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 `@infra/netris/Makefile` around lines 85 - 91, Update the destroy-osac target
to stop suppressing uninstall errors and remove the unconditional “|| true”
behavior. Preserve success when the OSAC release is explicitly absent, but
propagate all other uninstall failures and only remove the installer directories
and environment file after a successful or explicitly absent uninstall.
Comment on lines
+221
to
+228
| 1. **`prep-osac`** (Ansible) — clones osac-installer, patches the Helm values file with all CI-specific settings (Netris controller URL/credentials, DNS/AWS settings, SSH keys, instance group config for cluster/network fulfillment), enables required operators (LVMS, MetalLB, CNV, MCE, bundled PostgreSQL), copies license and pull secret to the values directory, and sets up a socat forwarder for OCP ingress on port 9444. | ||
|
|
||
| 2. **`run-osac-setup`** (shell) — runs `setup.sh` directly in the terminal with live output. This installs OCP operators (LVMS, MetalLB, CNV, cert-manager, Authorino, Keycloak, AAP), deploys OSAC via Helm, applies AAP configuration, and runs post-install setup (AAP token, hub registration, tenant creation). Retries up to 10 times with a 3-minute delay. | ||
| 2. **`run-osac-setup`** (shell) — runs `make install` in the osac-installer repo, which executes three Helm install phases: | ||
| - **Phase 1** (`install-operators`) — installs OLM subscriptions for cert-manager, AAP, LVMS, MetalLB, CNV, and MCE via the `osac-operators` chart | ||
| - **Phase 2** (`install-prereqs`) — deploys Keycloak, CA certificates, trust-manager bundles, and operator CRD instances via the `osac-prereqs` chart | ||
| - **Phase 3** (`install-osac`) — deploys the OSAC umbrella chart (osac-operator, fulfillment-service, osac-aap, osac-ui) with post-install hooks for hub creation and template publishing | ||
|
|
||
| 3. **`post-osac`** (Ansible) — scales down MCE operators (infrastructure-operator, multicluster-engine-operator) to prevent them from resetting OS images, then filters `OS_IMAGES` in the assisted-service ConfigMap and `RHCOS_VERSIONS` in the assisted-image-service StatefulSet to only the target OCP version (`caas_ocp_version`, x86_64). Verifies the image-service pod contains only the expected version. | ||
| 3. **`post-osac`** (Ansible) — scales down MCE operators (infrastructure-operator, multicluster-engine-operator) to prevent them from resetting OS images, then filters `OS_IMAGES` in the assisted-service ConfigMap and `RHCOS_VERSIONS` in the assisted-image-service StatefulSet to only the target OCP version (`ocp_version`, x86_64). Verifies the image-service pod contains only the expected version. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Align the fresh-flow documentation with actual behavior and defaults.
post-osac is skipped in fresh mode; the MCE scale-down and image filtering are snapshot-only. Also, the documented osac-devel namespace and development values file conflict with the current osac-e2e-ci / values/caas-ci/values.yaml defaults, so following this table describes a different deployment than make deploy-osac runs.
Also applies to: 308-310
🤖 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 `@infra/netris/README.md` around lines 221 - 228, The fresh-flow documentation
must match the defaults used by make deploy-osac. Update the README’s prep-osac,
run-osac-setup, and post-osac descriptions to state that post-osac is skipped in
fresh mode and that MCE scale-down and image filtering apply only to snapshot
mode; replace the outdated osac-devel namespace and development values-file
references with osac-e2e-ci and values/caas-ci/values.yaml.
Comment on lines
+14
to
+21
| - name: Delete cluster (fire-and-forget) | ||
| ansible.builtin.command: | ||
| cmd: osac delete cluster {{ _cluster_id.stdout }} | ||
| when: _cluster_id.stdout | length > 0 | ||
| async: 1 | ||
| poll: 0 | ||
| changed_when: true | ||
| failed_when: false |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Ansible async 1 poll 0 does the async_wrapper kill the job after the timeout
💡 Result:
Yes, when you run an Ansible task with async and poll: 0, the async_wrapper process does kill the job after the defined timeout [1]. In this configuration, Ansible spawns a supervisory process (the async_wrapper) that monitors the execution of your task [1]. The wrapper creates a child process to run the module and watches it [1]. The wrapper tracks the remaining time against the value specified in your async parameter [1]. Once the time limit is reached, the wrapper sends a SIGKILL signal to the process group associated with the task to terminate it [1]. It is important to note that when using poll: 0, Ansible considers the task to have started successfully and immediately moves to the next task in the playbook without waiting for the async job to finish [2][3]. Because of this "fire and forget" behavior, the playbook run concludes without checking the status of that task [2][3]. If you need to ensure the task finishes within a specific timeframe or handle its output, you must use the async_status module in a later task to check on the job or clean it up [2][4][3]. Additionally, since the async_wrapper leaves behind a job cache file, you should manually clean this up using async_status with mode: cleanup if the task is intended to be fire-and-forget [3][5].
Citations:
- 1: https://github.com/ansible/ansible-modules-core/blob/devel/utilities/logic/async_wrapper.py
- 2: https://docs.ansible.com/projects/ansible/latest/playbook_guide/playbooks_async.html
- 3: https://docs.ansible.com/projects/ansible-core/2.18/playbook%5Fguide/playbooks%5Fasync.html
- 4: https://github.com/ansible/ansible/blob/devel/lib/ansible/modules/async_status.py
- 5: https://docs.ansible.com/projects/ansible/13/playbook%5Fguide/playbooks_async.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="infra/netris/roles/destroy-caas/tasks/main.yml"
if [ -f "$file" ]; then
echo "== file excerpt =="
nl -ba "$file" | sed -n '1,40p'
echo
echo "== relevant async/poll usages in repo =="
rg -n "^\s*(async|poll|failed_when|changed_when):" infra/netris || true
else
echo "missing: $file"
echo "candidate files:"
fd -a 'main\.yml$|main\.yaml$' . | sed 's#^\./##' | grep 'destroy-caas/tasks/main\.ya?ml$' || true
fiRepository: osac-project/osac-test-infra
Length of output: 225
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="infra/netris/roles/destroy-caas/tasks/main.yml"
if [ -f "$file" ]; then
echo "== file excerpt =="
awk '{printf "%6d\t%s\n", NR, $0}' "$file" | sed -n '1,40p'
echo
echo "== relevant async/poll usages in repo =="
grep -RInE '^\s*(async|poll|failed_when|changed_when):\s*[^#]' infra/netris || true
else
echo "missing: $file"
echo "candidate files:"
find . -type f \( -path '*/destroy-caas/tasks/main.yml' -o -path '*/destroy-caas/tasks/main.yaml' \) -printf '%p\n'
fiRepository: osac-project/osac-test-infra
Length of output: 46244
**Use a realistic timeout for the async delete.
With poll: 0, the async wrapper still enforces async and SIGKILLs the task after that duration. A 1-second budget is too short for osac delete cluster, so the “fire-and-forget” teardown can be killed before the cluster actually starts being deleted. Set a longer async value, such as 600.
🔧 Proposed fix
when: _cluster_id.stdout | length > 0
- async: 1
+ async: 600
poll: 0📝 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.
Suggested change
| - name: Delete cluster (fire-and-forget) | |
| ansible.builtin.command: | |
| cmd: osac delete cluster {{ _cluster_id.stdout }} | |
| when: _cluster_id.stdout | length > 0 | |
| async: 1 | |
| poll: 0 | |
| changed_when: true | |
| failed_when: false | |
| - name: Delete cluster (fire-and-forget) | |
| ansible.builtin.command: | |
| cmd: osac delete cluster {{ _cluster_id.stdout }} | |
| when: _cluster_id.stdout | length > 0 | |
| async: 600 | |
| poll: 0 | |
| changed_when: true | |
| failed_when: 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 `@infra/netris/roles/destroy-caas/tasks/main.yml` around lines 14 - 21, Update
the “Delete cluster (fire-and-forget)” task to use a realistic async timeout by
increasing its async value from 1 to 600, while preserving poll: 0 and the
existing command and failure behavior.
Comment on lines
+82
to
+105
| - name: Find discovery VMs | ||
| ansible.builtin.shell: | ||
| cmd: virsh list --all --name | grep "{{ item }}" | ||
| register: _discovery_vm_lookup | ||
| changed_when: false | ||
| failed_when: false | ||
| loop: "{{ caas_discovery_vm_patterns }}" | ||
|
|
||
| - name: Build VM name list | ||
| ansible.builtin.set_fact: | ||
| caas_discovery_vms: "{{ _discovery_vm_lookup.results | selectattr('stdout', 'ne', '') | map(attribute='stdout_lines') | map('first') | list }}" | ||
|
|
||
| - name: Shut down running VMs | ||
| ansible.builtin.command: | ||
| cmd: virsh destroy {{ item }} | ||
| loop: "{{ caas_discovery_vms }}" | ||
| failed_when: false | ||
| changed_when: true | ||
|
|
||
| - name: Remove discovery VM disks | ||
| ansible.builtin.file: | ||
| path: /var/lib/libvirt/images/{{ item }}-disk.qcow2 | ||
| state: absent | ||
| loop: "{{ caas_discovery_vms }}" |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Discovery VMs are destroyed but never undefined, and only the first match per pattern is handled.
virsh destroy force-stops the domain; the definition survives. You then delete its backing qcow2, leaving a defined-but-broken domain that will collide on the next run. lab_destroy cleans orphans, but this role runs as an independent teardown path.
Separately, map('first') on stdout_lines drops all but the first VM matching each pattern.
🔧 Proposed fix
- name: Build VM name list
ansible.builtin.set_fact:
- caas_discovery_vms: "{{ _discovery_vm_lookup.results | selectattr('stdout', 'ne', '') | map(attribute='stdout_lines') | map('first') | list }}"
+ caas_discovery_vms: "{{ _discovery_vm_lookup.results | selectattr('stdout', 'ne', '') | map(attribute='stdout_lines') | flatten | select | list }}"
- name: Shut down running VMs
ansible.builtin.command:
cmd: virsh destroy {{ item }}
loop: "{{ caas_discovery_vms }}"
failed_when: false
changed_when: true
+
+- name: Undefine discovery VMs
+ ansible.builtin.command:
+ cmd: virsh undefine {{ item }} --nvram
+ loop: "{{ caas_discovery_vms }}"
+ failed_when: false
+ changed_when: true📝 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.
Suggested change
| - name: Find discovery VMs | |
| ansible.builtin.shell: | |
| cmd: virsh list --all --name | grep "{{ item }}" | |
| register: _discovery_vm_lookup | |
| changed_when: false | |
| failed_when: false | |
| loop: "{{ caas_discovery_vm_patterns }}" | |
| - name: Build VM name list | |
| ansible.builtin.set_fact: | |
| caas_discovery_vms: "{{ _discovery_vm_lookup.results | selectattr('stdout', 'ne', '') | map(attribute='stdout_lines') | map('first') | list }}" | |
| - name: Shut down running VMs | |
| ansible.builtin.command: | |
| cmd: virsh destroy {{ item }} | |
| loop: "{{ caas_discovery_vms }}" | |
| failed_when: false | |
| changed_when: true | |
| - name: Remove discovery VM disks | |
| ansible.builtin.file: | |
| path: /var/lib/libvirt/images/{{ item }}-disk.qcow2 | |
| state: absent | |
| loop: "{{ caas_discovery_vms }}" | |
| - name: Find discovery VMs | |
| ansible.builtin.shell: | |
| cmd: virsh list --all --name | grep "{{ item }}" | |
| register: _discovery_vm_lookup | |
| changed_when: false | |
| failed_when: false | |
| loop: "{{ caas_discovery_vm_patterns }}" | |
| - name: Build VM name list | |
| ansible.builtin.set_fact: | |
| caas_discovery_vms: "{{ _discovery_vm_lookup.results | selectattr('stdout', 'ne', '') | map(attribute='stdout_lines') | flatten | select | list }}" | |
| - name: Shut down running VMs | |
| ansible.builtin.command: | |
| cmd: virsh destroy {{ item }} | |
| loop: "{{ caas_discovery_vms }}" | |
| failed_when: false | |
| changed_when: true | |
| - name: Undefine discovery VMs | |
| ansible.builtin.command: | |
| cmd: virsh undefine {{ item }} --nvram | |
| loop: "{{ caas_discovery_vms }}" | |
| failed_when: false | |
| changed_when: true | |
| - name: Remove discovery VM disks | |
| ansible.builtin.file: | |
| path: /var/lib/libvirt/images/{{ item }}-disk.qcow2 | |
| state: absent | |
| loop: "{{ caas_discovery_vms }}" |
🤖 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 `@infra/netris/roles/destroy-caas/tasks/main.yml` around lines 82 - 105, Update
the discovery VM teardown tasks to process every VM returned for each pattern,
rather than reducing each stdout_lines result with map('first'). After
force-stopping each VM in the existing “Shut down running VMs” flow, explicitly
undefine its libvirt domain before removing its disk, using the VM names
collected in caas_discovery_vms.
Comment on lines
+111
to
+149
| - name: Build resource class map | ||
| ansible.builtin.set_fact: | ||
| _netris_resource_class_map: >- | ||
| {"{{ caas_host_type_id }}":{"server_cluster_template_id":{{ _netris_sct_id }},"mgmt_interface":"ens4","vpc_interfaces":["ens5"]}} | ||
|
|
||
| # --- Patch values file with all settings --- | ||
|
|
||
| - name: Read current values file | ||
| ansible.builtin.slurp: | ||
| src: "{{ osac_installer_dir }}/{{ osac_values_file }}" | ||
| register: _values_raw | ||
|
|
||
| - name: Build values overrides | ||
| ansible.builtin.set_fact: | ||
| _values_overrides: >- | ||
| {{ {'bundledPostgres': {'enabled': true}, | ||
| 'bmf': {'enabled': false}, | ||
| 'lvms': {'enabled': true, 'channel': 'stable-' + ocp_version}, | ||
| 'metallb': {'enabled': true}, | ||
| 'cnv': {'enabled': true}, | ||
| 'mce': {'enabled': true}, | ||
| 'operator': {'aap': {'tokenSecret': {'name': 'osac-aap-api-token', 'key': 'token'}}}, | ||
| 'aap': {'instanceGroups': {'clusterFulfillment': {'enabled': true, 'config': { | ||
| 'NETWORK_CLASS': 'netris', | ||
| 'NETWORK_STEPS_COLLECTION': 'netris.steps', | ||
| 'EXTERNAL_ACCESS_BASE_DOMAIN': ocp_base_domain, | ||
| 'EXTERNAL_ACCESS_SUPPORTED_BASE_DOMAINS': ocp_base_domain, | ||
| 'EXTERNAL_ACCESS_API_INTERNAL_NETWORK': 'hypershift', | ||
| 'HOSTED_CLUSTER_BASE_DOMAIN': ocp_base_domain, | ||
| 'HOSTED_CLUSTER_CONTROLLER_AVAILABILITY_POLICY': 'SingleReplica', | ||
| 'HOSTED_CLUSTER_INFRASTRUCTURE_AVAILABILITY_POLICY': 'SingleReplica', | ||
| 'NETRIS_CONTROLLER_URL': _netris_ctl_url, | ||
| 'NETRIS_USERNAME': netris_username, | ||
| 'NETRIS_SITE_ID': _netris_site_id | string, | ||
| 'NETRIS_TENANT_ID': '1', | ||
| 'NETRIS_TENANT_NAME': 'Admin', | ||
| 'NETRIS_MGMT_VPC_ID': _netris_mgmt_vpc_id | string, | ||
| 'NETRIS_MGMT_VPC_NAME': ocp_vpc_name, | ||
| 'NETRIS_RESOURCE_CLASS_MAP': _netris_resource_class_map | to_json, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Reproduce the double-encoding behavior Ansible's to_json (json.dumps) applies to an already-JSON string
python3 -c "
import json
s = '{\"gp3.large\": {\"server_cluster_template_id\": 42, \"mgmt_interface\": \"ens4\", \"vpc_interfaces\": [\"ens5\"]}}'
print('input :', s)
print('to_json(input):', json.dumps(s))
"Repository: osac-project/osac-test-infra
Length of output: 412
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- target files ---\n'
for f in infra/netris/roles/prep-osac/tasks/main.yml infra/netris/roles/post-refresh-osac/tasks/main.yml; do
if [ -f "$f" ]; then
echo "FOUND $f"
wc -l "$f"
else
echo "MISSING $f"
fi
done
printf '\n--- prep-osac relevant block ---\n'
sed -n '100,160p' infra/netris/roles/prep-osac/tasks/main.yml
printf '\n--- post-refresh-osac relevant block ---\n'
sed -n '48,92p' infra/netris/roles/post-refresh-osac/tasks/main.yml
printf '\n--- deterministic Jinja/text-behavior probe ---\n'
python3 - <<'PY'
import re
folded = '''$'-
{"{{ caas_host_type_id }}":{"server_cluster_template_id":{{ _netris_sct_id }},"mgmt_interface":"ens4","vpc_interfaces":["ens5"]}}
'''
print(folded)
print("contains literal string-like scalar:", "{", "\"", "}}" in folded)
print("would become JSON string after placeholder substitution:", '{"caas_host_type_id":{"server_cluster_template_id":42,"mgmt_interface":"ens4","vpc_interfaces":["ens5"]}}')
PYRepository: osac-project/osac-test-infra
Length of output: 257
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- target files ---'
for f in infra/netris/roles/prep-osac/tasks/main.yml infra/netris/roles/post-refresh-osac/tasks/main.yml; do
if [ -f "$f" ]; then
echo "FOUND $f"
wc -l "$f"
else
echo "MISSING $f"
fi
done
printf '\n%s\n' '--- prep-osac relevant block ---'
sed -n '100,160p' infra/netris/roles/prep-osac/tasks/main.yml
printf '\n%s\n' '--- post-refresh-osac relevant block ---'
sed -n '48,92p' infra/netris/roles/post-refresh-osac/tasks/main.yml
printf '\n%s\n' '--- deterministic Jinja/text-behavior probe ---'
python3 - <<'PY'
import json
already_json = '{"caas_host_type_id":{"server_cluster_template_id":42,"mgmt_interface":"ens4","vpc_interfaces":["ens5"]}}'
print("input:", already_json)
print("to_json(input):", json.dumps(already_json))
PYRepository: osac-project/osac-test-infra
Length of output: 5999
Avoid double-encoding _netris_resource_class_map. It is rendered as a JSON string by the folded set_fact, so applying | to_json at use time wraps it in extra quotes and escapes it. Drop the filter on the existing string, or construct _netris_resource_class_map as a native fact and call to_json once where it flows into NETRIS_RESOURCE_CLASS_MAP.
📍 Affects 2 files
infra/netris/roles/prep-osac/tasks/main.yml#L111-L149(this comment)infra/netris/roles/post-refresh-osac/tasks/main.yml#L59-L83
🤖 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 `@infra/netris/roles/prep-osac/tasks/main.yml` around lines 111 - 149, Fix
double-encoding of _netris_resource_class_map in
infra/netris/roles/prep-osac/tasks/main.yml:111-149 and
infra/netris/roles/post-refresh-osac/tasks/main.yml:59-83 by removing the
to_json filter when assigning NETRIS_RESOURCE_CLASS_MAP, since the folded
set_fact already stores a JSON string. Keep the resulting configuration as valid
JSON without added quotes or escaping at both affected sites.
Comment on lines
+123
to
+191
| - name: Build values overrides | ||
| ansible.builtin.set_fact: | ||
| _values_overrides: >- | ||
| {{ {'bundledPostgres': {'enabled': true}, | ||
| 'bmf': {'enabled': false}, | ||
| 'lvms': {'enabled': true, 'channel': 'stable-' + ocp_version}, | ||
| 'metallb': {'enabled': true}, | ||
| 'cnv': {'enabled': true}, | ||
| 'mce': {'enabled': true}, | ||
| 'operator': {'aap': {'tokenSecret': {'name': 'osac-aap-api-token', 'key': 'token'}}}, | ||
| 'aap': {'instanceGroups': {'clusterFulfillment': {'enabled': true, 'config': { | ||
| 'NETWORK_CLASS': 'netris', | ||
| 'NETWORK_STEPS_COLLECTION': 'netris.steps', | ||
| 'EXTERNAL_ACCESS_BASE_DOMAIN': ocp_base_domain, | ||
| 'EXTERNAL_ACCESS_SUPPORTED_BASE_DOMAINS': ocp_base_domain, | ||
| 'EXTERNAL_ACCESS_API_INTERNAL_NETWORK': 'hypershift', | ||
| 'HOSTED_CLUSTER_BASE_DOMAIN': ocp_base_domain, | ||
| 'HOSTED_CLUSTER_CONTROLLER_AVAILABILITY_POLICY': 'SingleReplica', | ||
| 'HOSTED_CLUSTER_INFRASTRUCTURE_AVAILABILITY_POLICY': 'SingleReplica', | ||
| 'NETRIS_CONTROLLER_URL': _netris_ctl_url, | ||
| 'NETRIS_USERNAME': netris_username, | ||
| 'NETRIS_SITE_ID': _netris_site_id | string, | ||
| 'NETRIS_TENANT_ID': '1', | ||
| 'NETRIS_TENANT_NAME': 'Admin', | ||
| 'NETRIS_MGMT_VPC_ID': _netris_mgmt_vpc_id | string, | ||
| 'NETRIS_MGMT_VPC_NAME': ocp_vpc_name, | ||
| 'NETRIS_RESOURCE_CLASS_MAP': _netris_resource_class_map | to_json, | ||
| 'SERVER_MGMT_ROUTE_DESTINATION': '192.168.16.0/20', | ||
| 'SERVER_MGMT_ROUTE_GATEWAY': '192.168.16.1', | ||
| 'SERVER_SSH_BASTION_HOST': '192.168.16.254', | ||
| 'SERVER_SSH_BASTION_USER': 'root', | ||
| 'SERVER_SSH_USER': 'root', | ||
| 'DNS_CLASS': 'dns.route53.dns', | ||
| 'DNS_ZONE': dns_hosted_zone | ||
| }, 'secret': { | ||
| 'NETRIS_PASSWORD': netris_password, | ||
| 'AWS_ACCESS_KEY_ID': aws_access_key_id, | ||
| 'AWS_SECRET_ACCESS_KEY': aws_secret_access_key | ||
| }}, | ||
| 'networkFulfillment': {'enabled': true, 'config': { | ||
| 'NETRIS_CONTROLLER_URL': _netris_ctl_url, | ||
| 'NETRIS_USERNAME': netris_username, | ||
| 'NETRIS_SITE_ID': _netris_site_id | string, | ||
| 'NETRIS_TENANT_ID': '1', | ||
| 'NETRIS_TENANT_NAME': 'Admin' | ||
| }, 'secret': { | ||
| 'NETRIS_PASSWORD': netris_password | ||
| }} | ||
| }}, | ||
| 'clusterFulfillment': {'config': { | ||
| 'DNS_CLASS': 'dns.route53.dns', | ||
| 'DNS_ZONE': dns_hosted_zone | ||
| }, 'secret': { | ||
| 'AWS_ACCESS_KEY_ID': aws_access_key_id, | ||
| 'AWS_SECRET_ACCESS_KEY': aws_secret_access_key | ||
| }} | ||
| } | ||
| | combine(({'operator': {'image': { | ||
| 'repository': osac_operator_image | regex_replace(':([^/]+)$', ''), | ||
| 'tag': (osac_operator_image | regex_search(':([^/]+)$', '\1') | first | default('latest')) | ||
| }}} if osac_operator_image | default('') | length > 0 else {}), recursive=true) | ||
| | combine(({'service': {'images': {'service': fulfillment_service_image}}, | ||
| 'dbMigrate': {'image': fulfillment_service_image}} | ||
| if fulfillment_service_image | default('') | length > 0 else {}), recursive=true) | ||
| | combine(({'aap': {'configAsCode': {'projectGitBranch': osac_aap_branch}}} | ||
| if osac_aap_branch | default('') | length > 0 else {}), recursive=true) | ||
| | combine(({'aap': {'bootstrap': {'image': osac_aap_image}}} | ||
| if osac_aap_image | default('') | length > 0 else {}), recursive=true) | ||
| }} |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Add no_log: true to the secret-bearing "Build values overrides" task.
This set_fact embeds netris_password, aws_access_key_id, and aws_secret_access_key in plaintext inside _values_overrides. The follow-up "Write patched values file" task correctly sets no_log: true, but this one doesn't — running with -v/-vvv (or CI log capture) will print the rendered fact, leaking credentials into logs.
🔒 Proposed fix
- name: Build values overrides
ansible.builtin.set_fact:
_values_overrides: >-
...
+ no_log: true📝 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.
Suggested change
| - name: Build values overrides | |
| ansible.builtin.set_fact: | |
| _values_overrides: >- | |
| {{ {'bundledPostgres': {'enabled': true}, | |
| 'bmf': {'enabled': false}, | |
| 'lvms': {'enabled': true, 'channel': 'stable-' + ocp_version}, | |
| 'metallb': {'enabled': true}, | |
| 'cnv': {'enabled': true}, | |
| 'mce': {'enabled': true}, | |
| 'operator': {'aap': {'tokenSecret': {'name': 'osac-aap-api-token', 'key': 'token'}}}, | |
| 'aap': {'instanceGroups': {'clusterFulfillment': {'enabled': true, 'config': { | |
| 'NETWORK_CLASS': 'netris', | |
| 'NETWORK_STEPS_COLLECTION': 'netris.steps', | |
| 'EXTERNAL_ACCESS_BASE_DOMAIN': ocp_base_domain, | |
| 'EXTERNAL_ACCESS_SUPPORTED_BASE_DOMAINS': ocp_base_domain, | |
| 'EXTERNAL_ACCESS_API_INTERNAL_NETWORK': 'hypershift', | |
| 'HOSTED_CLUSTER_BASE_DOMAIN': ocp_base_domain, | |
| 'HOSTED_CLUSTER_CONTROLLER_AVAILABILITY_POLICY': 'SingleReplica', | |
| 'HOSTED_CLUSTER_INFRASTRUCTURE_AVAILABILITY_POLICY': 'SingleReplica', | |
| 'NETRIS_CONTROLLER_URL': _netris_ctl_url, | |
| 'NETRIS_USERNAME': netris_username, | |
| 'NETRIS_SITE_ID': _netris_site_id | string, | |
| 'NETRIS_TENANT_ID': '1', | |
| 'NETRIS_TENANT_NAME': 'Admin', | |
| 'NETRIS_MGMT_VPC_ID': _netris_mgmt_vpc_id | string, | |
| 'NETRIS_MGMT_VPC_NAME': ocp_vpc_name, | |
| 'NETRIS_RESOURCE_CLASS_MAP': _netris_resource_class_map | to_json, | |
| 'SERVER_MGMT_ROUTE_DESTINATION': '192.168.16.0/20', | |
| 'SERVER_MGMT_ROUTE_GATEWAY': '192.168.16.1', | |
| 'SERVER_SSH_BASTION_HOST': '192.168.16.254', | |
| 'SERVER_SSH_BASTION_USER': 'root', | |
| 'SERVER_SSH_USER': 'root', | |
| 'DNS_CLASS': 'dns.route53.dns', | |
| 'DNS_ZONE': dns_hosted_zone | |
| }, 'secret': { | |
| 'NETRIS_PASSWORD': netris_password, | |
| 'AWS_ACCESS_KEY_ID': aws_access_key_id, | |
| 'AWS_SECRET_ACCESS_KEY': aws_secret_access_key | |
| }}, | |
| 'networkFulfillment': {'enabled': true, 'config': { | |
| 'NETRIS_CONTROLLER_URL': _netris_ctl_url, | |
| 'NETRIS_USERNAME': netris_username, | |
| 'NETRIS_SITE_ID': _netris_site_id | string, | |
| 'NETRIS_TENANT_ID': '1', | |
| 'NETRIS_TENANT_NAME': 'Admin' | |
| }, 'secret': { | |
| 'NETRIS_PASSWORD': netris_password | |
| }} | |
| }}, | |
| 'clusterFulfillment': {'config': { | |
| 'DNS_CLASS': 'dns.route53.dns', | |
| 'DNS_ZONE': dns_hosted_zone | |
| }, 'secret': { | |
| 'AWS_ACCESS_KEY_ID': aws_access_key_id, | |
| 'AWS_SECRET_ACCESS_KEY': aws_secret_access_key | |
| }} | |
| } | |
| | combine(({'operator': {'image': { | |
| 'repository': osac_operator_image | regex_replace(':([^/]+)$', ''), | |
| 'tag': (osac_operator_image | regex_search(':([^/]+)$', '\1') | first | default('latest')) | |
| }}} if osac_operator_image | default('') | length > 0 else {}), recursive=true) | |
| | combine(({'service': {'images': {'service': fulfillment_service_image}}, | |
| 'dbMigrate': {'image': fulfillment_service_image}} | |
| if fulfillment_service_image | default('') | length > 0 else {}), recursive=true) | |
| | combine(({'aap': {'configAsCode': {'projectGitBranch': osac_aap_branch}}} | |
| if osac_aap_branch | default('') | length > 0 else {}), recursive=true) | |
| | combine(({'aap': {'bootstrap': {'image': osac_aap_image}}} | |
| if osac_aap_image | default('') | length > 0 else {}), recursive=true) | |
| }} | |
| - name: Build values overrides | |
| ansible.builtin.set_fact: | |
| _values_overrides: >- | |
| {{ {'bundledPostgres': {'enabled': true}, | |
| 'bmf': {'enabled': false}, | |
| 'lvms': {'enabled': true, 'channel': 'stable-' + ocp_version}, | |
| 'metallb': {'enabled': true}, | |
| 'cnv': {'enabled': true}, | |
| 'mce': {'enabled': true}, | |
| 'operator': {'aap': {'tokenSecret': {'name': 'osac-aap-api-token', 'key': 'token'}}}, | |
| 'aap': {'instanceGroups': {'clusterFulfillment': {'enabled': true, 'config': { | |
| 'NETWORK_CLASS': 'netris', | |
| 'NETWORK_STEPS_COLLECTION': 'netris.steps', | |
| 'EXTERNAL_ACCESS_BASE_DOMAIN': ocp_base_domain, | |
| 'EXTERNAL_ACCESS_SUPPORTED_BASE_DOMAINS': ocp_base_domain, | |
| 'EXTERNAL_ACCESS_API_INTERNAL_NETWORK': 'hypershift', | |
| 'HOSTED_CLUSTER_BASE_DOMAIN': ocp_base_domain, | |
| 'HOSTED_CLUSTER_CONTROLLER_AVAILABILITY_POLICY': 'SingleReplica', | |
| 'HOSTED_CLUSTER_INFRASTRUCTURE_AVAILABILITY_POLICY': 'SingleReplica', | |
| 'NETRIS_CONTROLLER_URL': _netris_ctl_url, | |
| 'NETRIS_USERNAME': netris_username, | |
| 'NETRIS_SITE_ID': _netris_site_id | string, | |
| 'NETRIS_TENANT_ID': '1', | |
| 'NETRIS_TENANT_NAME': 'Admin', | |
| 'NETRIS_MGMT_VPC_ID': _netris_mgmt_vpc_id | string, | |
| 'NETRIS_MGMT_VPC_NAME': ocp_vpc_name, | |
| 'NETRIS_RESOURCE_CLASS_MAP': _netris_resource_class_map | to_json, | |
| 'SERVER_MGMT_ROUTE_DESTINATION': '192.168.16.0/20', | |
| 'SERVER_MGMT_ROUTE_GATEWAY': '192.168.16.1', | |
| 'SERVER_SSH_BASTION_HOST': '192.168.16.254', | |
| 'SERVER_SSH_BASTION_USER': 'root', | |
| 'SERVER_SSH_USER': 'root', | |
| 'DNS_CLASS': 'dns.route53.dns', | |
| 'DNS_ZONE': dns_hosted_zone | |
| }, 'secret': { | |
| 'NETRIS_PASSWORD': netris_password, | |
| 'AWS_ACCESS_KEY_ID': aws_access_key_id, | |
| 'AWS_SECRET_ACCESS_KEY': aws_secret_access_key | |
| }}, | |
| 'networkFulfillment': {'enabled': true, 'config': { | |
| 'NETRIS_CONTROLLER_URL': _netris_ctl_url, | |
| 'NETRIS_USERNAME': netris_username, | |
| 'NETRIS_SITE_ID': _netris_site_id | string, | |
| 'NETRIS_TENANT_ID': '1', | |
| 'NETRIS_TENANT_NAME': 'Admin' | |
| }, 'secret': { | |
| 'NETRIS_PASSWORD': netris_password | |
| }} | |
| }}, | |
| 'clusterFulfillment': {'config': { | |
| 'DNS_CLASS': 'dns.route53.dns', | |
| 'DNS_ZONE': dns_hosted_zone | |
| }, 'secret': { | |
| 'AWS_ACCESS_KEY_ID': aws_access_key_id, | |
| 'AWS_SECRET_ACCESS_KEY': aws_secret_access_key | |
| }} | |
| } | |
| | combine(({'operator': {'image': { | |
| 'repository': osac_operator_image | regex_replace(':([^/]+)$', ''), | |
| 'tag': (osac_operator_image | regex_search(':([^/]+)$', '\1') | first | default('latest')) | |
| }}} if osac_operator_image | default('') | length > 0 else {}), recursive=true) | |
| | combine(({'service': {'images': {'service': fulfillment_service_image}}, | |
| 'dbMigrate': {'image': fulfillment_service_image}} | |
| if fulfillment_service_image | default('') | length > 0 else {}), recursive=true) | |
| | combine(({'aap': {'configAsCode': {'projectGitBranch': osac_aap_branch}}} | |
| if osac_aap_branch | default('') | length > 0 else {}), recursive=true) | |
| | combine(({'aap': {'bootstrap': {'image': osac_aap_image}}} | |
| if osac_aap_image | default('') | length > 0 else {}), recursive=true) | |
| }} | |
| no_log: 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 `@infra/netris/roles/prep-osac/tasks/main.yml` around lines 123 - 191, Add
no_log: true to the “Build values overrides” Ansible set_fact task that
constructs _values_overrides, ensuring its rendered secret-bearing values are
suppressed from verbose and CI logs.
Comment on lines
+18
to
+27
| - name: Cherry-pick upstream fixes for refresh-after-snapshot | ||
| ansible.builtin.shell: | ||
| cmd: | | ||
| git fetch origin fix/config-as-code-ig-helm-conflict | ||
| git cherry-pick --no-commit b0b2cfb | ||
| chdir: "{{ osac_installer_dir }}" | ||
| register: _cherry_pick | ||
| changed_when: _cherry_pick.rc == 0 | ||
| failed_when: false | ||
| when: not (osac_installer_skip_clone | default(false) | bool) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 💤 Low value
Cherry-pick failure is silently swallowed with no cleanup.
failed_when: false means a conflicting cherry-pick (partial apply, conflict markers left in working tree) won't fail the play, and there's no git cherry-pick --abort/reset on failure. If refresh-after-snapshot.py (patched in the very next task) is among the conflicted files, the following naive string-replace patches will silently no-op against files containing <<<<<<< conflict markers, and the corrupted script will be used later during the actual snapshot refresh — a much harder failure to diagnose than a loud cherry-pick error here.
🔧 Proposed fix
- name: Cherry-pick upstream fixes for refresh-after-snapshot
ansible.builtin.shell:
cmd: |
git fetch origin fix/config-as-code-ig-helm-conflict
- git cherry-pick --no-commit b0b2cfb
+ git cherry-pick --no-commit b0b2cfb || { git cherry-pick --abort; exit 1; }
chdir: "{{ osac_installer_dir }}"
register: _cherry_pick
changed_when: _cherry_pick.rc == 0
- failed_when: false
+ failed_when: _cherry_pick.rc != 0
when: not (osac_installer_skip_clone | default(false) | bool)Separately, the hardcoded branch/commit hash is a temporary upstream-fix workaround with no tracking comment (unlike other workarounds in this file, e.g. OSAC-2115, OSAC-1624) — consider adding a JIRA reference for when it can be dropped.
📝 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.
Suggested change
| - name: Cherry-pick upstream fixes for refresh-after-snapshot | |
| ansible.builtin.shell: | |
| cmd: | | |
| git fetch origin fix/config-as-code-ig-helm-conflict | |
| git cherry-pick --no-commit b0b2cfb | |
| chdir: "{{ osac_installer_dir }}" | |
| register: _cherry_pick | |
| changed_when: _cherry_pick.rc == 0 | |
| failed_when: false | |
| when: not (osac_installer_skip_clone | default(false) | bool) | |
| - name: Cherry-pick upstream fixes for refresh-after-snapshot | |
| ansible.builtin.shell: | |
| cmd: | | |
| git fetch origin fix/config-as-code-ig-helm-conflict | |
| git cherry-pick --no-commit b0b2cfb || { git cherry-pick --abort; exit 1; } | |
| chdir: "{{ osac_installer_dir }}" | |
| register: _cherry_pick | |
| changed_when: _cherry_pick.rc == 0 | |
| failed_when: _cherry_pick.rc != 0 | |
| when: not (osac_installer_skip_clone | default(false) | bool) |
🤖 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 `@infra/netris/roles/prep-refresh-osac/tasks/main.yml` around lines 18 - 27,
Update the “Cherry-pick upstream fixes for refresh-after-snapshot” task to fail
when the cherry-pick command fails and clean up the repository by aborting the
cherry-pick or resetting the partial state before the play stops. Preserve the
existing skip condition and successful cherry-pick behavior, and add a tracking
JIRA comment for the temporary branch/commit workaround.
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 `@infra/netris/roles/prep-osac/tasks/main.yml`:
- Around line 23-62: The KubeletConfig apply task should control whether MCP
transition waits run, rather than always reporting changed. Register the `oc
apply` result, remove the unconditional `changed_when: true`, and add a
condition to both MCP wait tasks to execute only when the result indicates
`created` or `configured`.
In `@infra/netris/roles/prep-refresh-osac/tasks/main.yml`:
- Around line 135-140: Update the “Override osac-ui image in values file” task
so it replaces the complete YAML image scalar, including registry ports and
optional digest suffixes, rather than relying on the current narrow sed pattern.
Use a YAML-aware replacement, or add validation that confirms the original
scalar was fully replaced and no dangling digest or unrelated text remains,
while preserving the existing osac_ui_image guard.
🪄 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: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 8b3b5959-bfb7-4c9b-9375-356fa0bd42f3
📒 Files selected for processing (4)
infra/netris/Makefileinfra/netris/inventory/group_vars/all.ymlinfra/netris/roles/prep-osac/tasks/main.ymlinfra/netris/roles/prep-refresh-osac/tasks/main.yml
Comment on lines
+23
to
+62
| - name: Apply KubeletConfig for maxPods 500 | ||
| ansible.builtin.shell: | ||
| cmd: | | ||
| cat <<'EOF' | oc apply -f - | ||
| apiVersion: machineconfiguration.openshift.io/v1 | ||
| kind: KubeletConfig | ||
| metadata: | ||
| name: set-master-max-pods | ||
| spec: | ||
| machineConfigPoolSelector: | ||
| matchLabels: | ||
| pools.operator.machineconfiguration.openshift.io/master: "" | ||
| kubeletConfig: | ||
| maxPods: 500 | ||
| EOF | ||
| environment: | ||
| KUBECONFIG: /root/.kube/config | ||
| changed_when: true | ||
|
|
||
| - name: Wait for MCP master to start updating | ||
| ansible.builtin.shell: | ||
| cmd: oc get mcp/master -o 'jsonpath={.status.conditions[?(@.type=="Updating")].status}' | ||
| environment: | ||
| KUBECONFIG: /root/.kube/config | ||
| register: _mcp_updating | ||
| until: _mcp_updating.stdout == 'True' | ||
| retries: 60 | ||
| delay: 10 | ||
| changed_when: false | ||
|
|
||
| - name: Wait for MCP master to finish updating (tolerates API outage during SNO reboot) | ||
| ansible.builtin.shell: | ||
| cmd: oc wait --for=condition=Updated mcp/master --timeout=300s | ||
| environment: | ||
| KUBECONFIG: /root/.kube/config | ||
| changed_when: false | ||
| register: _mcp_updated | ||
| until: _mcp_updated.rc == 0 | ||
| retries: 10 | ||
| delay: 30 |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Repository files matching target:"
fd -a 'main.yml$' . | sed 's#^\./##' | rg '(^|/)infra/netris/roles/prep-osac/tasks/main.yml$|prep-osac/tasks/main.yml|roles/prep-osac/tasks/main.yml' || true
target="$(fd 'tasks/main.yml$' . | rg '(^|/)infra/netris/roles/prep-osac/tasks/main.yml$' | head -n 1 || true)"
if [ -z "$target" ]; then
echo "Target file not found"
exit 0
fi
echo "Target file path: $target"
wc -l "$target"
echo
echo "Relevant lines 1-80:"
sed -n '1,90p' "$target" | nl -ba
echo
echo "Search KubeletConfig occurrences:"
rg -n "KubeletConfig|Kubelet|machineconfiguration|oc apply|MCP|mcp/master|changed_when" "$target" || true
echo
echo "Check Ansible availability and docs context:"
which ansible-playbook || true
ansible --version 2>/dev/null | head -5 || true
echo
echo "Behavioral probe: parse the apply task and conditional waits"
python3 - <<'PY'
import re
from pathlib import Path
target = Path(sys.argv[1]) if len(sys.argv)>1 else next(Path('.').rglob('infra/netris/roles/prep-osac/tasks/main.yml'), Path())
text = target.read_text()
for idx, line in enumerate(text.splitlines(), 1):
if 'KubeletConfig' in line or 'oc apply' in line:
print(f"{target}:{idx}:{line.strip()}")
PYRepository: osac-project/osac-test-infra
Length of output: 285
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files under infra/netris:"
git ls-files 'infra/netris/**' | head -80
echo
echo "Locate prep-osac role files:"
git ls-files | rg 'prep-osac|roles/prep-osac|KubeletConfig|maxPods|mcp/master' || true
echo
echo "Show file if present:"
if [ -f infra/netris/roles/prep-osac/tasks/main.yml ]; then
wc -l infra/netris/roles/prep-osac/tasks/main.yml
sed -n '1,100p' infra/netris/roles/prep-osac/tasks/main.yml | nl -ba
echo
rg -n "KubeletConfig|oc apply|mcp/master|changed_when|register:_mcp|retries|delay" infra/netris/roles/frontend/tasks/main.yml || true
else
echo "target not present"
fi
echo
echo "Current commit / diff stat:"
git rev-parse --short HEAD
git diff --stat || true
git diff --name-only || true
git diff -- infra/netris/roles/prep-osac/tasks/main.yml | sed -n '1,180p' || trueRepository: osac-project/osac-test-infra
Length of output: 4092
🌐 Web query:
OpenShift MachineConfigPoolUpdating status created or configured oc apply result oc apply -f - changed_when true
💡 Result:
The status UPDATING=True in an OpenShift MachineConfigPool (MCP) indicates that the Machine Config Operator (MCO) is actively applying a new machine configuration to at least one node in that pool [1][2][3]. When an MCP is in this state, nodes may be cordoned, drained, and rebooted as part of the update process [1][3]. Regarding the context of Ansible and "changed_when": 1. UPDATING status: If an MCP remains in UPDATING=True for an extended period, it typically suggests a bottleneck, such as a node failing to drain (often due to Pod Disruption Budgets), a machine configuration failure, or a configuration error [4][3]. You can investigate by running oc get mcp and checking the status conditions [4][1], or by examining the MachineConfigDaemon logs on the affected nodes [4]. 2. "changed_when true" in Ansible: The use of "changed_when: true" in an Ansible task involving an "oc apply" command is an explicit instruction to Ansible to always report the task as "changed," regardless of whether the command actually modified the cluster state [5][6]. Because "oc apply" is a declarative command that may result in no actual changes if the cluster is already in the desired state, Ansible's command/shell modules cannot always determine if a real change occurred [5]. Developers often add "changed_when: true" to force Ansible to treat the task as a change-triggering event (e.g., to notify downstream handlers like service restarts) [5]. Conversely, if you want to ensure the task remains idempotent and only reports "changed" when a modification actually occurs, you would typically use "changed_when: false" for read-only operations or use conditional logic based on the command output [5][6]. Using "changed_when: true" effectively disables the command's natural ability to be idempotent in the context of Ansible's reporting [5].
Citations:
- 1: https://docs.okd.io/4.20/machine_configuration/index.html
- 2: https://docs.redhat.com/en/documentation/openshift_container_platform/4.21/observability/machine_configuration/machine-config-daemon-metrics
- 3: https://kubernetes.recipes/recipes/configuration/openshift-machineconfig-mcp-guide/
- 4: https://kubernetes.recipes/recipes/troubleshooting/mcp-blocked-stale-update/
- 5: https://docs.ansible.com/projects/lint/rules/no-changed-when/
- 6: redhatci/ansible-collection-redhatci-ocp@f5e3741
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "All git-tracked files mentioning prep-osac:"
git ls-files | rg 'prep-osac|prepusac|prep_osac|OSAC|Prep' || true
echo
echo "Inspect current git state:"
git status --short
git diff --stat
git diff --name-only
echo
echo "Read target if it is git-tracked:"
if git ls-files --error-unmatch 'infra/netris/roles/prep-osac/tasks/main.yml' >/dev/null 2>&1; then
wc -l infra/netris/roles/prep-osac/tasks/main.yml
sed -n '1,90p' infra/netris/roles/prep-osac/tasks/main.yml | nl -ba
echo
rg -n "KubeletConfig|oc apply|changed_when|register:_mcp|retries|delay|mcp/master" infra/netris/roles/prep-osac/tasks/main.yml || true
else
echo "infra/netris/roles/prep-osac/tasks/main.yml is not git-tracked"
fi
echo
echo "Search KubeletConfig anywhere in git-track:"
git ls-files -z | xargs -0 rg -n "KubeletConfig|Kubelet|maxPods|oc apply -f -|mcp/master|Machine Config Pool|MachineConfigPool"Repository: osac-project/osac-test-infra
Length of output: 443
🏁 Script executed:
#!/bin/bash
set -euo pipefail
target="infra/netris/roles/prep-osac/tasks/main.yml"
echo "Target file summary"
wc -l "$target"
echo
echo "Relevant task lines"
awk 'NR>=1 && NR<=90 {printf "%6d %s\n", NR, $0}' "$target"
echo
echo "Specific occurrences"
rg -n "KubeletConfig|oc apply|mcp/master|changed_when|register|retries|delay|wait for" "$target" || true
echo
echo "Read prep playbook include"
awk 'NR>=1 && NR<=120 {printf "%6d %s\n", NR, $0}' infra/netris/playbooks/prep-osac.ymlRepository: osac-project/osac-test-infra
Length of output: 4509
Make the MCP transition waits conditional on an actual configuration change.
changed_when: true reports a change even when oc apply makes no modification, so reruns still enter the Updating == True loop. Register the apply result and run the MCP update/wait tasks only when the resource reports created or configured.
🤖 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 `@infra/netris/roles/prep-osac/tasks/main.yml` around lines 23 - 62, The
KubeletConfig apply task should control whether MCP transition waits run, rather
than always reporting changed. Register the `oc apply` result, remove the
unconditional `changed_when: true`, and add a condition to both MCP wait tasks
to execute only when the result indicates `created` or `configured`.
Comment on lines
+135
to
+140
| - name: Override osac-ui image in values file | ||
| ansible.builtin.shell: | ||
| cmd: | | ||
| sed -i 's|[a-zA-Z0-9./_-]*osac-ui:[a-zA-Z0-9._-]*|{{ osac_ui_image }}|g' \ | ||
| {{ osac_installer_dir }}/{{ snapshot_osac_values_file }} | ||
| when: osac_ui_image | default('') | length > 0 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate file =="
git ls-files | grep -F 'infra/netris/roles/prep-refresh-osac/tasks/main.yml' || true
echo "== relevant task section =="
if [ -f infra/netris/roles/prep-refresh-osac/tasks/main.yml ]; then
nl -ba infra/netris/roles/prep-refresh-osac/tasks/main.yml | sed -n '110,160p'
fi
echo "== search osac_ui related usage =="
rg -n "osac_ui_image|osac-ui|snapshot_osac_values_file|disable.*ui|ui:" infra/netris/roles/prep-refresh-osac -S || true
echo "== regex behavioral probe =="
python3 - <<'PY'
import re
pattern = r'[a-zA-Z0-9./_-]*osac-ui:[a-zA-Z8-9._-]*' # literal from comment? inspect actual below
# We'll model the actual behavior for representative strings if we discover it.
samples = [
"osac-ui:old",
"registry.example:5000/osac-ui:old",
"registry.example:5000/osac-ui:old@sha256:abcdef",
"ghcr.io/org/osac-ui:old@sha256:abcdef",
"localhost:5000/osac-ui:old",
]
for s in samples:
print(s, re.sub(pattern, "NEW_IMAGE", s))
PY
echo "== yaml image scalar extraction hints =="
python3 - <<'PY'
import re
samples = [
"osac-ui:old",
"registry.example:5000/osac-ui:old",
"registry.example:5000/osac-ui:old@sha256:abcdef",
]
actual = r'[a-zA-Z0-9./_-]*osac-ui:[a-zA-Z0-9._-]*'
for s in samples:
print("input=", s, "full_match=", bool(re.search(actual, s)), "last_match=", s[s.rfind('osac-ui'):] if 'osac-ui:' in s else None)
PYRepository: osac-project/osac-test-infra
Length of output: 304
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate file =="
git ls-files | grep -F 'infra/netris/roles/prep-refresh-osac/tasks/main.yml' || true
echo "== relevant task section =="
if [ -f infra/netris/roles/prep-refresh-osac/tasks/main.yml ]; then
nl -ba infra/netris/roles/prep-refresh-osac/tasks/main.yml | sed -n '110,160p'
fi
echo "== search osac_ui related usage =="
rg -n "osac_ui_image|osac-ui|snapshot_osac_values_file|disable.*ui|ui:" infra/netris/roles/prep-refresh-osac -S || true
echo "== regex behavioral probe =="
python3 - <<'PY'
samples = [
"registry.example:5000/osac-ui:old",
"registry.example:5000/osac-ui:old@sha256:abcdef",
]
import re
pattern = r'[a-zA-Z0-9./_-]*osac-ui:[a-zA-Z0-9._-]*'
for s in samples:
print(s, "=>", re.sub(pattern, "NEW_IMAGE", s))
PYRepository: osac-project/osac-test-infra
Length of output: 304
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== relevant task lines =="
if command -v nl >/dev/null 2>&1; then
nl -ba infra/netris/roles/prep-refresh-osac/tasks/main.yml | sed -n '120,155p'
else
cat -n infra/netris/roles/prep-refresh-osac/tasks/main.yml | sed -n '120,155p'
fi
echo "== search osac_ui related usage =="
rg -n "osac_ui_image|osac-ui|snapshot_osac_values_file|disable.*ui|ui:" infra/netris/roles/prep-refresh-osac -S || true
echo "== regex behavioral probe =="
python3 - <<'PY'
import re
samples = [
"registry.example:5000/osac-ui:old",
"registry.example:5000/osac-ui:old@sha256:abcdef",
"ghcr.io/org/osac-ui:old@sha256:abcdef",
"localhost:5000/osac-ui:old",
"osac-ui:old",
]
pattern = r'[a-zA-Z0-9./_-]*osac-ui:[a-zA-Z0-9._-]*'
for s in samples:
print(f"{s!r} => {re.sub(pattern, 'NEW_IMAGE', s)!r} full={bool(re.search(pattern, s))}")
PYRepository: osac-project/osac-test-infra
Length of output: 4521
Handle complete osac-ui image scalars, not only simple name:tag values.
The sed regex does not include registry ports before osac-ui or the digest suffix after the tag. With inputs like registry.example:5000/osac-ui:old and registry-hosted digest refs, the replacement can leave unrelated text or a dangling digest (for non-local registries) instead of applying ${osac_ui_image} to the full image scalar. Use a YAML-aware replacement or validate that the original scalar was fully replaced.
🤖 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 `@infra/netris/roles/prep-refresh-osac/tasks/main.yml` around lines 135 - 140,
Update the “Override osac-ui image in values file” task so it replaces the
complete YAML image scalar, including registry ports and optional digest
suffixes, rather than relying on the current narrow sed pattern. Use a
YAML-aware replacement, or add validation that confirms the original scalar was
fully replaced and no dangling digest or unrelated text remains, while
preserving the existing osac_ui_image guard.
The KubeletConfig change triggers a node reboot on SNO. During the reboot the API server is unreachable, causing oc wait to fail immediately instead of waiting. Use ansible retries around a shorter oc wait timeout so connection failures during the reboot get retried rather than failing the task. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Dan Manor <dmanor@redhat.com>
danmanor
force-pushed
the
feat/deploy-split
branch
from
July 28, 2026 05:59
a7f2137 to
a516ccf
Compare
The sno-4-22 snapshot flavor already has maxPods: 500 configured, eliminating the need to apply a KubeletConfig and wait for the MCP reboot during deploy-osac. This saves ~10 minutes of reboot time. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Dan Manor <dmanor@redhat.com>
danmanor
force-pushed
the
feat/deploy-split
branch
from
July 28, 2026 07:38
e0e99ab to
31870bc
Compare
The gather-infra playbook was missing the _gather_dir set_fact and directory creation that gather-caas already had, causing the role to fail with '_gather_dir' is undefined. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Dan Manor <dmanor@redhat.com>
Use absolute path via github.workspace so upload-artifact doesn't nest the logs under a redundant osac-test-infra/osac-test-infra/ prefix. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Dan Manor <dmanor@redhat.com>
Enable presubmit CI for the snapshot-refresh CaaS Netris flow. Add concurrency group to cancel in-progress runs on PR update. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Dan Manor <dmanor@redhat.com>
Contributor
Author
|
Reopening as PR from origin branch to enable workflow dispatch testing |
4 of 5 tasks
This was referenced Aug 9, 2026
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
deploy-ocp(snapshot restore) anddeploy-osac(OSAC install) targetsOSAC_DEPLOY_MODEtoggle (fresh/snapshot) — fresh installs OSAC from scratch via Helm, snapshot refreshes a baked-in OSACfreshmode using the OCP-only snapshot flavor (sno-4.19-x86_64)deploy-ocpas a new contract target in root MakefilePipeline (fresh mode, default)
Removed
deploy-ocpviasetup-ocp.yml/install-ocp.yml)assisted_service,ocp_install,vm_resizedeploy,deploy-fast,deploy-ocp-snapshot,snapshot-refreshTest plan
ansible-playbook --syntax-checkmake -n)OSAC_DEPLOY_MODE=snapshotselects refresh pathdeploy-ocp+deploy-osacsucceededAssisted-by: Claude Code noreply@anthropic.com
Summary by CodeRabbit