Repository navigation
OSAC-2801: Split deploy targets, add e2e-caas-netris job - #268
Merged
Merged
Conversation
|
@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. |
|
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 PR adds reusable pull-request CaaS/Netris E2E workflows, refactors infrastructure and OSAC orchestration, introduces CaaS discovery and cleanup flows, expands diagnostics collection, and adds OCP deployment sequencing with snapshot-cache support. ChangesCaaS Netris E2E pipeline
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 1 warning)
✅ Passed checks (9 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 41
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
infra/netris/roles/prep-refresh-osac/tasks/main.yml (1)
135-149: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winThe new
osac_ui_imageoverride is immediately nullified by the next task.Lines 142-148 unconditionally flip
enabled: true→falsefor theui:block, and Line 151 scales the deployment to zero. So settingosac_ui_imagerewrites an image reference for a component that then never runs — the override is dead code as written. Gate the disable on the override being absent.🐛 Suggested fix
- name: Disable osac-ui (image not published yet) ansible.builtin.shell: cmd: | if grep -q 'osac-ui' {{ osac_installer_dir }}/{{ snapshot_osac_values_file }}; then sed -i '/^ui:/,/^[a-z]/{s/enabled: true/enabled: false/}' \ {{ osac_installer_dir }}/{{ snapshot_osac_values_file }} fi changed_when: false + when: osac_ui_image | default('') | length == 0Line 151's scale-to-zero needs the same guard.
🤖 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 - 149, Update the “Disable osac-ui (image not published yet)” task and its associated scale-to-zero operation to run only when osac_ui_image is absent or empty. Preserve the image override task’s behavior when osac_ui_image is provided so the configured UI image remains enabled and deployable.infra/netris/README.md (1)
239-272: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winThe old "Key Variables" table survived the reorg and now contradicts the new tables.
ocp_versionis4.22here but4.21at Line 285;snapshot_flavor_imageisquay.io/osac-project/...here butquay.io/rh-ee-ovishlit/...at Line 331;caas_cluster_templateisocp_ci_smallhere butosac.templates.ocp_ci_smallat Line 341. The example block at Line 268 also duplicates Line 235. Delete Lines 239-272 (keeping one example block) so the categorized tables are the single source of truth.🤖 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 239 - 272, Remove the obsolete “Key Variables” table and its duplicate example block from the README, preserving the newer categorized variable tables and the single remaining example block as the authoritative documentation. Do not alter the categorized tables or their values.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/e2e-caas-netris-caller.yml:
- Around line 8-9: Restrict the pull_request-triggered job in the workflow to
same-repository branches by adding a head-repository guard at the job level, and
add a paths filter for files relevant to the E2E run so documentation-only
changes are skipped. Keep the existing pull_request trigger and prevent fork PR
code from executing on the provisioned host.
In @.github/workflows/e2e-caas-netris-full-install-caller.yml:
- Around line 24-26: Remove the duplicated command-string default from the
caller workflow input declaration, and rely on the reusable workflow’s existing
default/fallback for empty inputs, including pull_request events. Update the
input definition while preserving its description and optional status; use the
corresponding fallback near the caller invocation as the single source of truth.
- Around line 8-9: Gate the pull_request-triggered provisioning workflow before
it allocates c5n.metal capacity, restricting execution to PRs whose head
repository matches github.repository. Apply the condition to the provisioning
job or workflow so forked PRs skip all AWS setup while same-repository PRs
retain the existing behavior.
In @.github/workflows/e2e-caas-netris-full-install.yml:
- Around line 390-395: Update the deploy-ocp steps in both
.github/workflows/e2e-caas-netris-full-install.yml lines 390-395 and
.github/workflows/e2e-caas-netris.yml lines 390-395: increase or remove the
60-minute timeout to accommodate fresh SNO installation within the job’s
360-minute limit, and rename the step to describe both snapshot and fresh
deployment modes.
In `@infra/netris/Makefile`:
- Around line 10-12: Export both OSAC_DEPLOY_MODE and EXTRA_VARS in their
variable definitions so recursive $(MAKE) invocations, including refresh-osac,
inherit the values assigned by this Makefile or included files. Keep
ANSIBLE_EXTRA’s existing expansion and osac_deploy_mode argument behavior
unchanged.
- Around line 85-91: Update the destroy-osac target to stop redirecting make
uninstall stderr to /dev/null while retaining its best-effort behavior with ||
true; remove the @ prefix from that command or use explicit error handling. Also
remove /opt/osac-installer-full from the cleanup command if this flow no longer
produces it.
In `@infra/netris/playbooks/destroy-infra.yml`:
- Around line 1-6: Add become: true to the “Destroy infrastructure” play in
destroy-infra.yml so the destroy-infra role executes with root privileges and
can remove the protected libvirt and dnsmasq paths.
In `@infra/netris/playbooks/post-osac.yml`:
- Line 57: Keep the os_images entry filtering in filtered based on the minor {{
ocp_version }} and x86_64 architecture. Update the post-OSAC OS image validation
to compare openshift_version against the release-image value {{ ocp_version
}}.0, and restrict validation to x86_64 entries when multi-arch entries may be
present.
In `@infra/netris/README.md`:
- Around line 331-332: Update the default values for snapshot_flavor_image and
snapshot_recert_image to use the corresponding images in the
quay.io/osac-project organization instead of the personal rh-ee-ovishlit
namespace. Keep the documented image descriptions and defaults-only behavior
unchanged.
In `@infra/netris/roles/caas-create/tasks/main.yml`:
- Around line 37-53: Update the “Wait for cluster to be ready” task so a
CLUSTER_STATE_FAILED result terminates polling immediately rather than relying
on failed_when during retries. Preserve retries for non-ready states and ensure
the subsequent “Fail if cluster ended in FAILED state” task remains reachable to
report the failure.
In `@infra/netris/roles/caas-discovery/tasks/main.yml`:
- Around line 28-72: Replace every hardcoded 192.168.16.1 reference in the
“Configure OCP DNS on mgmt-server for agent discovery” task with a single
role/group variable such as netris_mgmt_server_ip, including the SSH target,
dnsmasq listen-address, and DHCP grep/sed expressions. Define or reuse that
variable through the role’s existing variable mechanism and preserve the current
command behavior.
- Around line 169-178: Update the “Build VM name list” task using
_discovery_vm_lookup.results so multiple stdout_lines matches are not silently
truncated. Either flatten all matching VM names into caas_discovery_vms or add
validation requiring exactly one match per caas_discovery_vm_patterns entry,
causing unexpected multi-matches to fail loudly.
- Around line 10-26: Remove the notify directive from the “Ensure dnsmasq
listens on br-mgmt for agent DNS” lineinfile task, leaving the registered
_dnsmasq_mgmt condition and explicit “Restart dnsmasq if config changed” systemd
task unchanged.
- Around line 55-62: Update the DHCP DNS configuration block in the task so it
adds the required option when no existing “option domain-name-servers” directive
is present, while continuing to rewrite an incorrect existing directive. Ensure
the file is changed, isc-dhcp-server is restarted, and changed is set when
either operation inserts or updates the directive.
- Around line 221-226: Update the “Get current max memory” task to read the
persistent memory value from each VM’s XML using virsh dumpxml and an XML
extraction command, rather than virsh dommemstat. Preserve the existing
_current_memory registration, loop over caas_discovery_vms, and unchanged-task
behavior for the idempotency check.
- Around line 109-124: Update the “Ensure pull-secret exists in InfraEnv
namespace” shell task to avoid passing the decoded secret through --from-literal
or any command-line argument. Write the decoded _pull_secret content to a
securely managed temporary file, create the secret using a file-based input, and
remove the temporary file afterward while preserving no_log, retry behavior, and
changed detection.
In `@infra/netris/roles/caas-setup/tasks/main.yml`:
- Line 1: Replace the disabled TLS validation in the five identified HTTP tasks,
including the “Create host type,” “Launch publish-templates job,” polling,
“Create cluster catalog item,” and “Download discovery ISO” operations, with
validation against the cluster’s trusted internal CA bundle. If that bundle
cannot be wired in for this lab, document the isolated-environment rationale
beside each affected setting while preserving secure defaults elsewhere.
- Around line 187-196: Add no_log: true to the “Launch publish-templates job”
task using _aap_admin_pw.stdout, matching the existing “Wait for
publish-templates job to complete” task and preventing the AAP admin password
from appearing in task output.
- Around line 3-10: Update the “Wait for expected number of agents” task to set
environment KUBECONFIG to /root/.kube/config for its oc invocation, matching the
other oc tasks in the file. Keep the existing agent-count command and retry
behavior unchanged.
In `@infra/netris/roles/deploy-infra/tasks/main.yml`:
- Around line 9-16: Update the “Ensure br-mgmt bridge exists” shell task so
bridge creation remains conditional, but assigning mgmt_bridge_cidr to
mgmt_bridge_name and bringing the bridge up occur unconditionally. Make the
address assignment idempotent by avoiding failure when the address already
exists, while preserving the existing bridge creation behavior.
In `@infra/netris/roles/deploy-infra/tasks/preflight.yml`:
- Around line 36-38: Update the “Check AWS credentials” set_fact expression to
safely handle undefined aws_access_key_id and aws_secret_access_key values,
treating missing variables as invalid credentials rather than raising an error.
Preserve the existing _aws_creds_ok boolean contract so the aggregated preflight
failure handling can report the problem.
- Around line 22-27: Update the “Check KVM support” task to recognize both Intel
vmx and AMD svm virtualization flags, preferably by validating /dev/kvm when
appropriate, while preserving the existing preflight gate behavior. Also revise
the related failure message to use generic KVM/virtualization wording instead of
referring only to vmx.
- Around line 28-30: Update the _mem_total_gb assignment in the “Check minimum
memory (16GB)” task so the division result is explicitly grouped before applying
round(1). Keep the existing memory calculation and downstream threshold logic
unchanged, ensuring failure messages use the rounded gigabyte value.
In `@infra/netris/roles/destroy-caas/tasks/main.yml`:
- Around line 90-92: Update the “Build VM name list” set_fact for
caas_discovery_vms to flatten all stdout_lines from every matching
_discovery_vm_lookup result, rather than applying map('first'). Preserve
filtering out results with empty stdout and ensure every matched VM name reaches
the subsequent destroy and disk-removal tasks.
- Around line 14-21: Update the “Delete cluster (fire-and-forget)” task’s async
configuration to use a suitable timeout substantially longer than one second,
while preserving poll: 0 so the command remains non-blocking.
In `@infra/netris/roles/destroy-infra/tasks/main.yml`:
- Around line 37-40: Update the dnsmasq paths in
infra/netris/roles/destroy-infra/tasks/main.yml lines 37-40 and
infra/netris/roles/gather-infra/tasks/main.yml line 199 to use /etc/dnsmasq.d/{{
ocp_cluster_name | default('ocp-sno') }}.conf. This ensures destroy-infra
removes the variable-named drop-in and triggers its existing restart condition,
while gather-infra reads the matching deployed configuration.
In `@infra/netris/roles/gather-caas/tasks/main.yml`:
- Around line 11-13: Update the _osac_namespace set_fact in
infra/netris/roles/gather-caas/tasks/main.yml:11-13 and
infra/netris/roles/gather-infra/tasks/main.yml:11-13 to use the default filter’s
true flag, so undefined or empty _detected_osac_ns.stdout falls back to
osac_namespace. Keep both identical namespace-detection tasks consistent; shared
extraction is optional.
- Around line 318-333: Update the “Extract hosted cluster kubeconfig” shell task
to create /tmp/caas-hosted-kubeconfig with restrictive permissions, such as by
setting its mode to owner-only before or during the base64 decode. Preserve the
existing extraction and status behavior.
In `@infra/netris/roles/gather-infra/tasks/main.yml`:
- Around line 166-182: Update the four Netris gathering tasks—“Gather Netris
VPCs”, “Gather Netris VNets”, “Gather Netris subnets”, and “Gather Netris NAT
rules”—to stop interpolating _gather_netris_cookie.stdout into their shell
commands. Pass the session cookie through environment: and configure each task
with no_log: true, following the existing fulfillment-API pattern while
preserving the current curl behavior.
In `@infra/netris/roles/netris-configure/tasks/main.yml`:
- Around line 89-98: Guard the template-existence check using a default empty
list when `_sct_list.json.data` is null or missing, so the `selectattr`
expression falls through to template creation. Update the related condition near
the existing lookup logic while preserving the current `no_log` setting and
selection behavior for valid response data.
In `@infra/netris/roles/ocp-dns/tasks/main.yml`:
- Around line 41-46: Replace the shell-based Route 53 invocation in the “Create
OCP DNS records in Route 53” task with an argv-safe ansible.builtin.command or a
temporary batch file passed via file://, so _r53_batch is not embedded in
single-quoted shell text and apostrophes in derived names remain valid.
- Around line 2-14: Update the “Get Route 53 hosted zone ID” task to filter the
AWS CLI response for an exact hosted-zone name match, rather than selecting
HostedZones[0] from the lexicographically ordered results. Ensure the task fails
explicitly when no exact match exists, so subsequent UPSERTs cannot target an
unrelated zone.
In `@infra/netris/roles/post-refresh-osac/tasks/main.yml`:
- Around line 95-107: Replace the shell command in the secret-update task with a
secure temporary env-file workflow: create the file using ansible.builtin.copy
with mode "0600" and no_log enabled, pass its values to the secret-generation
command without embedding secrets in command-line arguments, and remove the file
afterward. Preserve the existing secret names, namespace, SSH key inputs,
KUBECONFIG environment, and apply behavior.
- Around line 20-30: Update the “Discover Netris controller URL” task around
_traefik_nodeport to retry until the kubectl command succeeds and produces
non-empty stdout, matching the existing retry behavior in the sibling tasks,
with 10 retries. Ensure _netris_ctl_url is set only after a valid nodePort is
discovered.
- Around line 59-62: The _netris_resource_class_map value is currently stored as
a JSON string and then serialized again by to_json. Update the “Build resource
class map” set_fact to construct an Ansible dictionary instead of a folded JSON
string, and apply the same correction to the corresponding raw-string map in the
prep-osac task so downstream consumers receive a single correctly encoded
object.
In `@infra/netris/roles/prep-osac/tasks/main.yml`:
- Around line 113-116: Change the “Build resource class map” task so
_netris_resource_class_map is stored as a native Ansible mapping rather than a
pre-serialized JSON string, allowing the existing to_json usage at the later
serialization point to encode it exactly once. Preserve the current
resource-class keys and values.
- Around line 207-211: Update the “Write patched values file” task to set the
generated file mode to 0600, protecting the injected SSH private key and AWS
credentials. Also set mode 0600 on the pull-secret.json task near the existing
key-file mode configuration, since it contains registry credentials.
- Around line 192-195: Update the osac_operator_image tag extraction in the
set_fact expression to use regex_findall or split the image string, ensuring
untagged references do not call first on None. Apply a true default so the tag
resolves to latest when no match or an empty result is returned, while
preserving tagged-image behavior.
In `@infra/netris/roles/prep-refresh-osac/tasks/main.yml`:
- Around line 245-253: Update the “Wait for cert-manager webhook to be ready”
task using the existing `_certmgr_rollout` registration and rollout command,
adding `changed_when: false` so successful readiness checks do not report
changes.
In `@infra/netris/roles/snapshot-pull/tasks/main.yml`:
- Around line 2-8: Update the snapshot cache flow around _flavor_cached and the
disk reassembly task: resolve and record the digest of snapshot_flavor_image in
a .cached-image marker, and only treat the cache as valid when that marker
matches the current resolved digest. During reassembly, write each disk to
disk_file.partial, remove or replace any stale partial, and atomically move it
to disk_file only after successful completion; create the matching cache marker
after all disks finish.
- Around line 98-109: Update the chunk reconstruction loops in the snapshot pull
task to sort matching chunk filenames with version-aware ordering before
decompression, ensuring chunk-2 precedes chunk-10. Preserve or explicitly define
the required per-layer ordering between .zst and .gz chunks, and add size
validation with wc -c if the rebuilt qcow2 depends on complete output.
---
Outside diff comments:
In `@infra/netris/README.md`:
- Around line 239-272: Remove the obsolete “Key Variables” table and its
duplicate example block from the README, preserving the newer categorized
variable tables and the single remaining example block as the authoritative
documentation. Do not alter the categorized tables or their values.
In `@infra/netris/roles/prep-refresh-osac/tasks/main.yml`:
- Around line 135-149: Update the “Disable osac-ui (image not published yet)”
task and its associated scale-to-zero operation to run only when osac_ui_image
is absent or empty. Preserve the image override task’s behavior when
osac_ui_image is provided so the configured UI image remains enabled and
deployable.
🪄 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: 87d3e908-0487-47e5-a25d-fb06ed1fe370
📒 Files selected for processing (55)
.github/workflows/e2e-caas-netris-caller.yml.github/workflows/e2e-caas-netris-full-install-caller.yml.github/workflows/e2e-caas-netris-full-install.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/gather.yml
- infra/netris/playbooks/gather-lab.yml
- infra/netris/roles/vm_resize/tasks/main.yml
- infra/netris/playbooks/destroy.yml
- infra/netris/roles/osac_install/tasks/main.yml
- infra/netris/roles/assisted_service/tasks/main.yml
- infra/netris/playbooks/setup-ocp.yml
- infra/netris/playbooks/site.yml
- infra/netris/roles/ocp_install/tasks/main.yml
Comment on lines
+90
to
+92
| - 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 }}" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Only the first VM per pattern survives this list build — the rest leak.
map(attribute='stdout_lines') | map('first') discards every match after the first, so if a pattern matches several discovery VMs only one gets virsh destroy'd and only one disk is removed. Flatten instead.
🐛 Proposed fix
- 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 | unique | list }}"📝 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 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: Build VM name list | |
| ansible.builtin.set_fact: | |
| caas_discovery_vms: "{{ _discovery_vm_lookup.results | selectattr('stdout', 'ne', '') | map(attribute='stdout_lines') | flatten | select | unique | list }}" |
🤖 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 90 - 92, Update
the “Build VM name list” set_fact for caas_discovery_vms to flatten all
stdout_lines from every matching _discovery_vm_lookup result, rather than
applying map('first'). Preserve filtering out results with empty stdout and
ensure every matched VM name reaches the subsequent destroy and disk-removal
tasks.
Comment on lines
+113
to
+116
| - 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"]}} |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
Double JSON encoding of NETRIS_RESOURCE_CLASS_MAP (same defect as the snapshot path).
_netris_resource_class_map is built as a JSON string, then to_json at Line 159 re-encodes it into a quoted, escaped string. See the consolidated note for the fix.
Also applies to: 159-159
🤖 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 113 - 116, Change
the “Build resource class map” task so _netris_resource_class_map is stored as a
native Ansible mapping rather than a pre-serialized JSON string, allowing the
existing to_json usage at the later serialization point to encode it exactly
once. Preserve the current resource-class keys and values.
Comment on lines
+192
to
+195
| | 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) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm behavior of regex_search with a replacement arg on no-match, and the group-list return shape
rg -n 'regex_search' -C3 infra/netrisRepository: osac-project/osac-test-infra
Length of output: 3311
🌐 Web query:
Ansible Jinja2 ansible.utils regex_search replacement argument returns list of groups no match None behavior
💡 Result:
In Ansible, the regex_search filter behavior regarding return values and capture groups is as follows: When using regex_search without additional capture group arguments, it returns the matched string if a match is found, or None if no match is found [1][2][3]. Note that in some older documentation or specific environments, this may have been described as returning an empty string, but recent versions and official clarification confirm that None is the standard return value for no match [2][3]. When you provide capture group arguments (e.g., regex_search('pattern', '\1')), the filter returns a list containing the matched groups [4][5]. A common, effective pattern to extract a specific captured group is to chain the filter with the first filter to obtain the string directly: {{ 'input' | regex_search('pattern', '\1') | first }} [4][5]. However, this idiom can cause errors when no match occurs because regex_search returns None, not a list, making it impossible to apply the first filter (resulting in a 'NoneType' object is not iterable error) [4]. To handle this, you can provide a default value that acts as a safe, empty iterable for the first filter, such as: {{ 'input' | regex_search('pattern', '\1') | default([None], True) | first }} [4] Alternatively, you can use the regex_findall filter if you always expect a list return value, as it returns an empty list [] instead of None when no matches are found [4].
Citations:
- 1: https://docs.ansible.com/ansible/latest/collections/ansible/builtin/regex_search_filter.html
- 2: regex_search returns None when there is no match, and not an empty string ansible/ansible#84397
- 3: https://docs.ansible.com/projects/ansible-core/2.18/collections/ansible/builtin/regex_search_filter.html
- 4: https://stackoverflow.com/questions/78023340/ansible-regex-search-with-capture-groups-when-no-matches
- 5: https://serverfault.com/questions/1033936/extracting-part-of-the-string-using-ansible-regex-search-and-save-the-output-as
🌐 Web query:
ansible jinja2 first filter value None default filter documentation
💡 Result:
In Jinja2 and Ansible, the first and default filters interact in a specific way when handling sequences and undefined or null values. The first filter returns the first item of a sequence [1][2]. If the sequence is empty (or the input is None where a sequence is expected), it returns a Jinja Undefined object [2][3][4]. Because first returns Undefined rather than None when no item is found, you can use the default filter to provide a fallback value [2][4]. Example usage: {{ (my_list | first) | default('my_default_value') }} When my_list is empty, first returns Undefined, which triggers the default filter to return 'my_default_value' [2][4]. Key considerations: 1. Default Filter Behavior: By default, the default filter only replaces values that are Undefined [5]. If you need it to also replace "falsy" values (like empty strings or None), you must set the second argument to true (e.g., {{ my_var | default('default', true) }}) [6][5]. 2. None vs. Empty String in Ansible: A known historical behavior in Ansible is that the templating engine often converts None into an empty string [6]. If you are chaining filters and specifically require None to be passed through, you may need to use default(None) or enable jinja2_native mode in your ansible.cfg to preserve null/None types rather than converting them to strings [6]. 3. Chaining Omit: If you are chaining filters and want to use the omit sentinel to skip a task or parameter when no value is found, a common pattern is: {{ foo | default(None) | some_filter or omit }} [7][8]. This uses None as an intermediate value to trigger failure in later filters, which the or omit logic then catches [7][8].
Citations:
- 1: https://docs.ansible.com/ansible/latest/collections/ansible/builtin/first_filter.html
- 2: https://jinja.app/documentation/jinja-filters/jinja-first-filter-function-to-retrieve-first-item
- 3: https://github.com/pallets/jinja/blob/main/src/jinja2/filters.py
- 4: https://digitalpomegranate.com/jinja-first-filter-function-to-retrieve-first-item/
- 5: https://nikhilism.com/post/2016/understanding-ansible-jinja2-default-filter/
- 6: default filter behaves strangely with
nonetype ansible/ansible#57438 - 7: https://docs.ansible.com/projects/ansible/latest/playbook_guide/playbooks_filters.html
- 8: https://docs.ansible.com/projects/ansible-core/2.20/playbook%5Fguide/playbooks%5Ffilters.html
Use regex_findall for tag extraction or split the image string.
With replacement capture groups, regex_search returns None on no match, so None | first fails before default('latest') is evaluated. An untagged reference like quay.io/foo/osac-operator will crash this set_fact; regex_findall(':([^/]+)$') and then applying first | default('latest', true) avoids this failure mode.
🤖 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 192 - 195, Update
the osac_operator_image tag extraction in the set_fact expression to use
regex_findall or split the image string, ensuring untagged references do not
call first on None. Apply a true default so the tag resolves to latest when no
match or an empty result is returned, while preserving tagged-image behavior.
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
🛑 Comments failed to post (26)
infra/netris/roles/caas-create/tasks/main.yml (1)
37-53: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Retry loop won't fail fast on
CLUSTER_STATE_FAILED.
until/retriesonly stops looping once theuntilcondition is met or retries are exhausted — afailed_whenmatch on an individual attempt does not break the loop early. So once the cluster hitsCLUSTER_STATE_FAILED, this task keeps polling for up to 120×30s (~1 hour) before finally failing, instead of aborting immediately. As a side effect, the "Fail if cluster ended in FAILED state" task below is unreachable dead code in every code path.🐛 Proposed fix: break the retry loop immediately on FAILED state
- name: Wait for cluster to be ready ansible.builtin.shell: cmd: | STATE=$(osac get clusters {{ caas_cluster_id }} -o json | python3 -c "import sys,json; print(json.load(sys.stdin).get('status',{}).get('state',''))") echo "$STATE" - if [ "$STATE" = "CLUSTER_STATE_FAILED" ]; then exit 2; fi register: _cluster_state - until: _cluster_state.stdout == "CLUSTER_STATE_READY" + until: _cluster_state.stdout in ["CLUSTER_STATE_READY", "CLUSTER_STATE_FAILED"] retries: 120 delay: 30 - failed_when: _cluster_state.rc == 2 changed_when: false - name: Fail if cluster ended in FAILED state ansible.builtin.fail: msg: "CaaS cluster '{{ caas_cluster_name }}' ({{ caas_cluster_id }}) is in FAILED state" when: _cluster_state.stdout == "CLUSTER_STATE_FAILED"📝 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.- name: Wait for cluster to be ready ansible.builtin.shell: cmd: | STATE=$(osac get clusters {{ caas_cluster_id }} -o json | python3 -c "import sys,json; print(json.load(sys.stdin).get('status',{}).get('state',''))") echo "$STATE" register: _cluster_state until: _cluster_state.stdout in ["CLUSTER_STATE_READY", "CLUSTER_STATE_FAILED"] retries: 120 delay: 30 changed_when: false - name: Fail if cluster ended in FAILED state ansible.builtin.fail: msg: "CaaS cluster '{{ caas_cluster_name }}' ({{ caas_cluster_id }}) is in FAILED state" when: _cluster_state.stdout == "CLUSTER_STATE_FAILED"🤖 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/caas-create/tasks/main.yml` around lines 37 - 53, Update the “Wait for cluster to be ready” task so a CLUSTER_STATE_FAILED result terminates polling immediately rather than relying on failed_when during retries. Preserve retries for non-ready states and ensure the subsequent “Fail if cluster ended in FAILED state” task remains reachable to report the failure.infra/netris/roles/caas-discovery/tasks/main.yml (6)
10-26: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Redundant dnsmasq restart (notify handler + immediate task).
The
lineinfiletask bothnotifys therestart dnsmasqhandler and is immediately followed by an explicitsystemd: restartedtask gated on the same "changed" condition. This restarts dnsmasq twice per run — once immediately, once again when handlers flush at the end of the play — causing an unnecessary extra DNS/DHCP blip during agent discovery boot.♻️ Drop the redundant notify (the explicit task already restarts immediately)
- name: Ensure dnsmasq listens on br-mgmt for agent DNS ansible.builtin.lineinfile: path: /etc/dnsmasq.d/ocp-sno.conf line: "{{ item }}" state: present loop: - "interface=br-mgmt" - "no-dhcp-interface=br-mgmt" - "bind-interfaces" register: _dnsmasq_mgmt - notify: restart dnsmasq📝 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.- name: Ensure dnsmasq listens on br-mgmt for agent DNS ansible.builtin.lineinfile: path: /etc/dnsmasq.d/ocp-sno.conf line: "{{ item }}" state: present loop: - "interface=br-mgmt" - "no-dhcp-interface=br-mgmt" - "bind-interfaces" register: _dnsmasq_mgmt - name: Restart dnsmasq if config changed ansible.builtin.systemd: name: dnsmasq state: restarted when: _dnsmasq_mgmt is changed🤖 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/caas-discovery/tasks/main.yml` around lines 10 - 26, Remove the notify directive from the “Ensure dnsmasq listens on br-mgmt for agent DNS” lineinfile task, leaving the registered _dnsmasq_mgmt condition and explicit “Restart dnsmasq if config changed” systemd task unchanged.
28-72: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Hardcoded mgmt-server IP repeated 5 times.
192.168.16.1is hardcoded in the SSH target,listen-address, and DHCP grep/sed checks. If the mgmt-server address ever changes, every occurrence must be updated in lockstep or the discovery flow silently breaks.Extract to a role/group var (e.g.
netris_mgmt_server_ip) and interpolate it in each location instead.🤖 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/caas-discovery/tasks/main.yml` around lines 28 - 72, Replace every hardcoded 192.168.16.1 reference in the “Configure OCP DNS on mgmt-server for agent discovery” task with a single role/group variable such as netris_mgmt_server_ip, including the SSH target, dnsmasq listen-address, and DHCP grep/sed expressions. Define or reuse that variable through the role’s existing variable mechanism and preserve the current command behavior.
55-62: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
DHCP DNS option silently not added when the line doesn't already exist.
sed -i 's/option domain-name-servers .*/.../ 'only rewrites an existing line; if/etc/dhcp/dhcpd.confhas nooption domain-name-serversdirective at all (plausible on a fresh mgmt-server), the substitution matches nothing and nothing is written — agents keep resolving via the default DHCP DNS instead of the local dnsmasq, with no error surfaced (changed=falsereported, task succeeds).🐛 Ensure the line exists, don't rely solely on rewriting it
- if grep -q 'option domain-name-servers 192.168.16.1' /etc/dhcp/dhcpd.conf; then + if grep -q 'option domain-name-servers 192.168.16.1' /etc/dhcp/dhcpd.conf; then : + elif grep -q '^option domain-name-servers' /etc/dhcp/dhcpd.conf; then + sed -i 's/option domain-name-servers .*/option domain-name-servers 192.168.16.1, 8.8.8.8;/' /etc/dhcp/dhcpd.conf + systemctl restart isc-dhcp-server 2>/dev/null || true + changed=true else - sed -i 's/option domain-name-servers .*/option domain-name-servers 192.168.16.1, 8.8.8.8;/' /etc/dhcp/dhcpd.conf + echo 'option domain-name-servers 192.168.16.1, 8.8.8.8;' >> /etc/dhcp/dhcpd.conf systemctl restart isc-dhcp-server 2>/dev/null || true changed=true fi📝 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.# Point DHCP DNS to mgmt-server so agents use local dnsmasq if grep -q 'option domain-name-servers 192.168.16.1' /etc/dhcp/dhcpd.conf; then : elif grep -q '^option domain-name-servers' /etc/dhcp/dhcpd.conf; then sed -i 's/option domain-name-servers .*/option domain-name-servers 192.168.16.1, 8.8.8.8;/' /etc/dhcp/dhcpd.conf systemctl restart isc-dhcp-server 2>/dev/null || true changed=true else echo 'option domain-name-servers 192.168.16.1, 8.8.8.8;' >> /etc/dhcp/dhcpd.conf systemctl restart isc-dhcp-server 2>/dev/null || true changed=true 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 `@infra/netris/roles/caas-discovery/tasks/main.yml` around lines 55 - 62, Update the DHCP DNS configuration block in the task so it adds the required option when no existing “option domain-name-servers” directive is present, while continuing to rewrite an incorrect existing directive. Ensure the file is changed, isc-dhcp-server is restarted, and changed is set when either operation inserts or updates the directive.
169-178: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Multi-match VM patterns silently lose extra VMs.
stdout_lines | map('first')keeps only the first matching VM name per pattern. If acaas_discovery_vm_patternsentry matches more than onevirsh listline, the rest are silently dropped fromcaas_discovery_vmswith no warning, understating how many VMs get configured/booted.Consider asserting
stdout_lines | length == 1per pattern (or flattening all matches) so unexpected multi-matches fail loudly instead of silently truncating.🤖 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/caas-discovery/tasks/main.yml` around lines 169 - 178, Update the “Build VM name list” task using _discovery_vm_lookup.results so multiple stdout_lines matches are not silently truncated. Either flatten all matching VM names into caas_discovery_vms or add validation requiring exactly one match per caas_discovery_vm_patterns entry, causing unexpected multi-matches to fail loudly.
221-226: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "== candidate file excerpt ==" sed -n '170,240p' infra/netris/roles/caas-discovery/tasks/main.yml 2>/dev/null || true echo "== relevant pattern searches ==" rg -n "Shut down running VMs|dommemstat|dumpxml|vcpucount|caas_discovery_vms" infra/netris/roles/caas-discovery/tasks/main.yml infra/netris/roles/caas-discovery 2>/dev/null || true echo "== libvirt virsh manpage availability ==" man virsh dommemstat 2>/tmp/virsh_dommemstat.html >/tmp/virsh_dommemstat.txt || true [ -f /tmp/virsh_dommemstat.txt ] && sed -n '/NAME/,/DESCRIPTION/p' /tmp/virsh_dommemstat.txt | head -80 [ -f /tmp/virsh_dommemstat.txt ] && sed -n '/DESCRIPTION/p;/dommemstat/p' /tmp/virsh_dommemstat.txt | head -120 || true echo "== installed virsh availability ==" command -v virsh && virsh --version 2>/tmp/virsh_version.txt || true cat /tmp/virsh_version.txt 2>/dev/null || trueRepository: osac-project/osac-test-infra
Length of output: 5366
🌐 Web query:
libvirt virsh dommemstat inactive domain shuts down domain not running💡 Result:
The command 'virsh dommemstat' is designed to retrieve memory statistics for a running guest virtual machine [1][2][3]. Because it relies on active communication with the guest's balloon driver (typically via the hypervisor), it cannot function on a domain that is inactive or shut down [1][2]. If you attempt to run 'virsh dommemstat' on an inactive domain, libvirt will typically return an error because the domain is not running and has no active processes or balloon driver to query [4][5][2]. Key points regarding this behavior: 1. Requirement for Active State: Memory statistics, such as those provided by 'dommemstat', represent real-time data from the guest's memory management system (like the virtio-balloon driver) [6][1]. When a domain is in a shut-off or inactive state, this guest-side driver is not running, and therefore, no real-time metrics exist to be collected [1][2]. 2. Command Limitation: Unlike some other virsh commands that support an '--inactive' flag to query the persistent XML configuration (such as 'domblklist' or 'domiflist') [1][7][2], 'dommemstat' is strictly for live, running domains [1][2][3]. 3. Error Handling: In many libvirt versions, querying an inactive domain for live statistics will trigger an error indicating that the domain is not running (e.g., 'error: Domain is not running') [4][5]. This is standard behavior; if you require information about a domain that is not currently running, you should instead inspect its configuration via 'virsh dumpxml ' or similar management commands [7].
Citations:
- 1: https://docs.redhat.com/en/documentation/red_hat_enterprise_linux/7/html/virtualization_deployment_and_administration_guide/sect-statlists
- 2: https://linux.die.net/man/1/virsh
- 3: https://download.libvirt.org/virshcmdref/html/sect-dommemstat.html
- 4: https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/message/3UO2SCUXQ752I6UWBUJXGCJMMQ6X55CE/
- 5: https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/thread/S2FVPCZLAF5GPORMMADZFIQTTT7IUI5U/
- 6: https://lists.libvirt.org/archives/list/users@lists.libvirt.org/thread/ELTTSESGYVG3DBYOXYGOFW5H744TUFV3/
- 7: https://man.archlinux.org/man/virsh.1.en
🌐 Web query:
libvirt virsh dommemstat active domain not running error💡 Result:
The error "domain is not running" when executing the
virsh dommemstatcommand occurs because this command is specifically designed to retrieve memory statistics from a guest virtual machine that is currently active (running) [1][2]. In libvirt,dommemstatrelies on the hypervisor and, in many cases, the guest's balloon driver to report real-time memory information; these components are only functional while the domain is in a running state [1][3]. If you encounter this error, verify the status of your domain usingvirsh domstate <domain>[4]. If the state is anything other than "running" (e.g., "shut off", "paused", "crashed"), the command will fail [4][5]. Key points regarding this behavior: - Functional Requirement: The command requires an active domain because it queries the hypervisor for live memory usage data [1][2]. - Consistent Error Handling: The "domain is not running" message is a standard response indicating that the operation (querying live memory stats) is invalid for the current lifecycle state of the guest [5]. - Troubleshooting Steps: - Check the domain state:virsh domstate <domain_name>[4]. - If the domain is shut off, start it first:virsh start <domain_name>. - If the domain is supposed to be running but shows as shut off or crashed, inspect the libvirt domain logs (usually located in/var/log/libvirt/qemu/<domain_name>.log) to diagnose unexpected shutdowns [4].Citations:
- 1: https://docs.redhat.com/fr/documentation/red_hat_enterprise_linux/7/html/virtualization_deployment_and_administration_guide/sect-statlists
- 2: https://download.libvirt.org/virshcmdref/html/sect-dommemstat.html
- 3: https://lists.xenproject.org/archives/html/xen-devel/2015-11/msg02209.html
- 4: https://lists.libvirt.org/archives/list/users@lists.libvirt.org/thread/VSSQ3FKD3MVCQ7RIA6V4637LGVJOLKV5/
- 5: https://lists.libvirt.org/archives/list/users@lists.libvirt.org/thread/SL32VLI3RGAHO7M4X3SGE6DO3GMVSX5A/
🌐 Web query:
libvirt virsh vcpucount --config --maximum inactive domain source memory target vcpus💡 Result:
The virsh vcpucount command is used to display information about the virtual CPU counts of a specified domain [1][2]. Regarding the specific flags mentioned: --maximum: This flag requests information on the maximum cap of vCPUs that a domain can have defined [2][3]. It is commonly used to see the upper limit for vCPU hot-plugging [4][3]. --config: This flag requests information from the persistent XML configuration of the domain, which will take effect the next time the domain is booted [2][3]. When used together, virsh vcpucount --config --maximum [domain] displays the maximum number of vCPUs currently configured for the domain to be used on its next boot [1][2]. Important notes on usage: 1. Mutually Exclusive Flags: The flags --config (next boot), --live (current running state), and --current (current state) are mutually exclusive [2]. Therefore, you cannot combine --config with --live or --current in a single vcpucount command [2]. 2. --maximum vs --active: The --maximum and --active flags are also mutually exclusive; you must choose one to indicate whether you are querying the maximum capacity or the current usage [2]. 3. Command Structure: The command typically takes either zero flags (to display all information in a table) or exactly two flags (one from the {maximum/active} group and one from the {config/live/current} group) [2]. If you are attempting to modify these values rather than just viewing them, you must use the virsh setvcpus command, which follows similar logic (e.g., --maximum must be used with --config) [5][4][6].
Citations:
- 1: https://docs.redhat.com/en/documentation/red_hat_enterprise_linux/7/html/virtualization_deployment_and_administration_guide/sect-Managing_guest_virtual_machines_with_virsh-Displaying_per_guest_virtual_machine_information
- 2: https://linux.die.net/man/1/virsh
- 3: https://docs.redhat.com/en/documentation/red_hat_enterprise_linux/7/html/virtualization_deployment_and_administration_guide/sect-managing_guest_virtual_machines_with_virsh-displaying_per_guest_virtual_machine_information
- 4: https://docs.oracle.com/en/operating-systems/oracle-linux/kvm-user/kvm-WorkingWithMemoryandCPUAllocation.html
- 5: https://www.ibm.com/docs/en/linux-on-systems?topic=cpus-modifying-number-virtual
- 6: https://bugzilla.redhat.com/show_bug.cgi?id=1204033
🌐 Web query:
libvirt virsh setmaxmem max current state shut off setmem --config inactive domain💡 Result:
To modify the memory configuration of a virtual machine using virsh, the behavior depends on whether you are modifying the maximum memory limit or the current memory allocation, and which flags are used. 1. Maximum Memory Allocation (setmaxmem): The command virsh setmaxmem domain size --config allows you to modify the maximum memory limit for an inactive (shut off) domain [1]. When the --config flag is used, the change is applied to the persistent configuration of the domain and will take effect the next time the domain is booted [1]. 2. Current Memory Allocation (setmem): The command virsh setmem is primarily intended for changing the memory allocation of a running guest domain [1][2][3]. Historically, attempts to run virsh setmem on an inactive domain have failed with an error because the operation requires the domain to be running [3]. 3. Managing Inactive Domains: If you need to change the memory settings of a domain that is currently in a shut off state, the standard approach is to edit the domain's XML configuration directly using the virsh edit command [4][1]. This allows you to update both the current and maximum memory values in the persistent definition of the domain, which will then be applied when the domain is subsequently started. Summary of Flags: - --config: Affects the persistent configuration of the domain (applies to the next boot) [1]. - --live: Affects a running guest domain immediately [1][2]. - --current: Affects the current state of the domain [1][2]. In summary, for inactive domains, use virsh edit to modify the domain's persistent XML configuration for both memory parameters, as commands like setmem are designed for active domains [1][3].
Citations:
- 1: https://linux.die.net/man/1/virsh
- 2: https://docs.oracle.com/en/operating-systems/oracle-linux/10/kvm-user/kvm-WorkingWithMemoryandCPUAllocation.html
- 3: https://access.redhat.com/solutions/35824
- 4: https://www.libvirt.org/manpages/virsh.html
Read max memory from the VM XML instead of
virsh dommemstat.This runs after the VMs are destroyed, and
virsh dommemstatrequires a running domain, so inactive discovery VMs will not produce theactualstat needed for the idempotency check. Use the persistent configured value instead, e.g.virsh dumpxml {{ item }} | xmllint --xpath 'string(//memory)' -.🤖 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/caas-discovery/tasks/main.yml` around lines 221 - 226, Update the “Get current max memory” task to read the persistent memory value from each VM’s XML using virsh dumpxml and an XML extraction command, rather than virsh dommemstat. Preserve the existing _current_memory registration, loop over caas_discovery_vms, and unchanged-task behavior for the idempotency check.
109-124: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Pull secret exposed via CLI argument (
--from-literal).The decoded pull secret is passed as a literal shell command-line argument. Even with
no_log: truesuppressing it from Ansible's output, the remote process's argv (visible viaps//proc/<pid>/cmdlineto other local users during execution) still leaks the secret.🔒 Pass the secret via a temp file instead of an inline literal
- oc get secret pull-secret -n {{ caas_discovery_infraenv_namespace }} || - oc create secret generic pull-secret -n {{ caas_discovery_infraenv_namespace }} - --from-literal=.dockerconfigjson='{{ _pull_secret.content | b64decode | trim }}' - --type=kubernetes.io/dockerconfigjson + oc get secret pull-secret -n {{ caas_discovery_infraenv_namespace }} || + (umask 077 && echo '{{ _pull_secret.content | b64decode | trim }}' > /tmp/.pull-secret.json && + oc create secret generic pull-secret -n {{ caas_discovery_infraenv_namespace }} + --from-file=.dockerconfigjson=/tmp/.pull-secret.json + --type=kubernetes.io/dockerconfigjson && + shred -u /tmp/.pull-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 `@infra/netris/roles/caas-discovery/tasks/main.yml` around lines 109 - 124, Update the “Ensure pull-secret exists in InfraEnv namespace” shell task to avoid passing the decoded secret through --from-literal or any command-line argument. Write the decoded _pull_secret content to a securely managed temporary file, create the secret using a file-based input, and remove the temporary file afterward while preserving no_log, retry behavior, and changed detection.infra/netris/roles/caas-setup/tasks/main.yml (3)
1-1: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
TLS certificate validation disabled across every CaaS/fulfillment/AAP HTTP(S) call. Checkov (CKV_ANSIBLE_1/CKV_ANSIBLE_2) flags
validate_certs: falseon five separateuri/get_urltasks spanning both files — one root cause (self-signed in-cluster certs in this lab), not five independent issues.
infra/netris/roles/caas-setup/tasks/main.yml#L151-165: "Create host type"uricall to the fulfillment API disables cert validation.infra/netris/roles/caas-setup/tasks/main.yml#L187-196: "Launch publish-templates job"uricall to the AAP controller disables cert validation.infra/netris/roles/caas-setup/tasks/main.yml#L198-210: "Wait for publish-templates job to complete"uripolling call disables cert validation.infra/netris/roles/caas-setup/tasks/main.yml#L227-252: "Create cluster catalog item"uricall disables cert validation.infra/netris/roles/caas-discovery/tasks/main.yml#L158-168: "Download discovery ISO"get_urlcall disables cert validation.Since this is an ephemeral, single-tenant lab environment the risk is limited, but consider trusting the cluster's internal CA bundle (e.g. via
ca-bundle.crtfrom theopenshift-confignamespace) instead of blanket-disabling validation, or at minimum document why it's safe here so it isn't copy-pasted into a less isolated environment later.🤖 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/caas-setup/tasks/main.yml` at line 1, Replace the disabled TLS validation in the five identified HTTP tasks, including the “Create host type,” “Launch publish-templates job,” polling, “Create cluster catalog item,” and “Download discovery ISO” operations, with validation against the cluster’s trusted internal CA bundle. If that bundle cannot be wired in for this lab, document the isolated-environment rationale beside each affected setting while preserving secure defaults elsewhere.Source: Linters/SAST tools
3-10: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Missing
environment: KUBECONFIGon the firstoccall.Every other
oc-invoking task in this file explicitly setsenvironment: KUBECONFIG: /root/.kube/config, but this first task omits it. If the ambient shell's kubeconfig context differs,oc get agentcould target the wrong cluster or fail silently (stderr redirected to/dev/null), leaving_agent_count.stdoutat 0 and spinning through the full 120×15s retry window before failing with a confusing error.🐛 Add the missing environment block
- name: Wait for expected number of agents ansible.builtin.shell: cmd: oc get agent -n {{ caas_agent_namespace }} --no-headers 2>/dev/null | wc -l + environment: + KUBECONFIG: /root/.kube/config register: _agent_count📝 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.- name: Wait for expected number of agents ansible.builtin.shell: cmd: oc get agent -n {{ caas_agent_namespace }} --no-headers 2>/dev/null | wc -l environment: KUBECONFIG: /root/.kube/config register: _agent_count until: _agent_count.stdout | int >= caas_agents | length retries: 120 delay: 15 changed_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/caas-setup/tasks/main.yml` around lines 3 - 10, Update the “Wait for expected number of agents” task to set environment KUBECONFIG to /root/.kube/config for its oc invocation, matching the other oc tasks in the file. Keep the existing agent-count command and retry behavior unchanged.
187-196: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Inconsistent
no_logaround the AAP admin password.The following task ("Wait for publish-templates job to complete") sets
no_log: truewhen using_aap_admin_pw.stdout, but this "Launch publish-templates job" task does not, despite passing the same password.🔒 Add no_log for consistency
validate_certs: false status_code: [201] register: _publish_job + 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.- name: Launch publish-templates job ansible.builtin.uri: url: "https://{{ _aap_route.stdout }}/api/controller/v2/job_templates/osac-publish-templates/launch/" method: POST user: admin password: "{{ _aap_admin_pw.stdout }}" force_basic_auth: true validate_certs: false status_code: [201] register: _publish_job no_log: true🧰 Tools
🪛 Checkov (3.3.8)
[medium] 187-199: Ensure that certificate validation isn't disabled with uri
(CKV_ANSIBLE_1)
🤖 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/caas-setup/tasks/main.yml` around lines 187 - 196, Add no_log: true to the “Launch publish-templates job” task using _aap_admin_pw.stdout, matching the existing “Wait for publish-templates job to complete” task and preventing the AAP admin password from appearing in task output.infra/netris/roles/deploy-infra/tasks/main.yml (1)
9-16: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
mgmt_bridge_cidris only assigned when the bridge is created.If
br-mgmtalready exists but has no address (pre-created by libvirt, a prior partial run, or a reboot-restored link without config), theip addr addinside the||group never runs, and downstream tasks fail far from the cause. Make address assignment unconditional and idempotent.♻️ Suggested fix
- name: Ensure br-mgmt bridge exists ansible.builtin.shell: | + set -eo pipefail ip link show {{ mgmt_bridge_name }} 2>/dev/null || { ip link add name {{ mgmt_bridge_name }} type bridge - ip addr add {{ mgmt_bridge_cidr }} dev {{ mgmt_bridge_name }} } + ip addr replace {{ mgmt_bridge_cidr }} dev {{ mgmt_bridge_name }} ip link set dev {{ mgmt_bridge_name }} up changed_when: false + args: + executable: /bin/bash📝 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.- name: Ensure br-mgmt bridge exists ansible.builtin.shell: | set -eo pipefail ip link show {{ mgmt_bridge_name }} 2>/dev/null || { ip link add name {{ mgmt_bridge_name }} type bridge } ip addr replace {{ mgmt_bridge_cidr }} dev {{ mgmt_bridge_name }} ip link set dev {{ mgmt_bridge_name }} up changed_when: false args: executable: /bin/bash🤖 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/deploy-infra/tasks/main.yml` around lines 9 - 16, Update the “Ensure br-mgmt bridge exists” shell task so bridge creation remains conditional, but assigning mgmt_bridge_cidr to mgmt_bridge_name and bringing the bridge up occur unconditionally. Make the address assignment idempotent by avoiding failure when the address already exists, while preserving the existing bridge creation behavior.infra/netris/roles/deploy-infra/tasks/preflight.yml (3)
22-27: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
grep -c vmxfails preflight on AMD hosts.AMD CPUs expose
svm, notvmx. On an AMD bare-metal host with virtualization fully enabled, this gate hard-fails the deploy with "KVM (vmx) not available". Match either flag, or better, check/dev/kvm.🐛 Suggested fix
- name: Check KVM support - ansible.builtin.command: grep -c vmx /proc/cpuinfo + ansible.builtin.command: grep -cE 'vmx|svm' /proc/cpuinfo register: _kvm_check changed_when: false failed_when: falseAnd update the failure text at Line 49 to drop the
vmx-only wording.📝 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.- name: Check KVM support ansible.builtin.command: grep -cE 'vmx|svm' /proc/cpuinfo register: _kvm_check changed_when: false 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/deploy-infra/tasks/preflight.yml` around lines 22 - 27, Update the “Check KVM support” task to recognize both Intel vmx and AMD svm virtualization flags, preferably by validating /dev/kvm when appropriate, while preserving the existing preflight gate behavior. Also revise the related failure message to use generic KVM/virtualization wording instead of referring only to vmx.
28-30: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Filter precedence:
round(1)is applied to1024, not to the quotient.Jinja binds filters tighter than arithmetic, so this evaluates as
ansible_memtotal_mb / (1024 | round(1)). The threshold check still works, but the failure message at Line 50 prints an unrounded float like15.6318359375GB.🐛 Suggested fix
- _mem_total_gb: "{{ ansible_memtotal_mb / 1024 | round(1) }}" + _mem_total_gb: "{{ (ansible_memtotal_mb / 1024) | round(1) }}"📝 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.- name: Check minimum memory (16GB) ansible.builtin.set_fact: _mem_total_gb: "{{ (ansible_memtotal_mb / 1024) | round(1) }}"🤖 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/deploy-infra/tasks/preflight.yml` around lines 28 - 30, Update the _mem_total_gb assignment in the “Check minimum memory (16GB)” task so the division result is explicitly grouped before applying round(1). Keep the existing memory calculation and downstream threshold logic unchanged, ensuring failure messages use the rounded gigabyte value.
36-38: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Undefined AWS vars crash the preflight instead of reporting a clean failure.
If
aws_access_key_id/aws_secret_access_keyaren't defined at all (rather than empty), this task raises an undefined-variable error, defeating the purpose of the aggregated failure list at Line 52.🛡️ Suggested fix
- _aws_creds_ok: "{{ aws_access_key_id | length > 0 and aws_secret_access_key | length > 0 }}" + _aws_creds_ok: >- + {{ (aws_access_key_id | default('') | length > 0) + and (aws_secret_access_key | default('') | length > 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.- name: Check AWS credentials ansible.builtin.set_fact: _aws_creds_ok: >- {{ (aws_access_key_id | default('') | length > 0) and (aws_secret_access_key | default('') | length > 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 `@infra/netris/roles/deploy-infra/tasks/preflight.yml` around lines 36 - 38, Update the “Check AWS credentials” set_fact expression to safely handle undefined aws_access_key_id and aws_secret_access_key values, treating missing variables as invalid credentials rather than raising an error. Preserve the existing _aws_creds_ok boolean contract so the aggregated preflight failure handling can report the problem.infra/netris/roles/destroy-infra/tasks/main.yml (1)
37-40: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
/etc/dnsmasq.d/ocp-sno.confis hardcoded in two places while the cluster name is a variable. Everything else derives fromocp_cluster_name | default('ocp-sno'), so a non-default cluster name means teardown leaves the drop-in behind (dnsmasq keeps serving stale records) and gather reads the wrong file.
infra/netris/roles/destroy-infra/tasks/main.yml#L37-L40: template the path as/etc/dnsmasq.d/{{ ocp_cluster_name | default('ocp-sno') }}.confso the removal — and the conditional dnsmasq restart below it — actually fire.infra/netris/roles/gather-infra/tasks/main.yml#L199-L199: use the same templated path in thecatso the collected config matches the deployed cluster.📍 Affects 2 files
infra/netris/roles/destroy-infra/tasks/main.yml#L37-L40(this comment)infra/netris/roles/gather-infra/tasks/main.yml#L199-L199🤖 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-infra/tasks/main.yml` around lines 37 - 40, Update the dnsmasq paths in infra/netris/roles/destroy-infra/tasks/main.yml lines 37-40 and infra/netris/roles/gather-infra/tasks/main.yml line 199 to use /etc/dnsmasq.d/{{ ocp_cluster_name | default('ocp-sno') }}.conf. This ensures destroy-infra removes the variable-named drop-in and triggers its existing restart condition, while gather-infra reads the matching deployed configuration.infra/netris/roles/gather-caas/tasks/main.yml (2)
11-13: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Duplicated namespace-detection fallback that can't catch an empty string.
stdout | default(osac_namespace)only applies whenstdoutis undefined; an empty probe result yields_osac_namespace: ''and every downstreamoc -n ''silently targets nothing.
infra/netris/roles/gather-caas/tasks/main.yml#L11-L13: switch todefault(osac_namespace, true)so empty output falls back.infra/netris/roles/gather-infra/tasks/main.yml#L11-L13: apply the same change; consider extracting this two-task pair into a shared include, since it's byte-identical in both roles.📍 Affects 2 files
infra/netris/roles/gather-caas/tasks/main.yml#L11-L13(this comment)infra/netris/roles/gather-infra/tasks/main.yml#L11-L13🤖 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/gather-caas/tasks/main.yml` around lines 11 - 13, Update the _osac_namespace set_fact in infra/netris/roles/gather-caas/tasks/main.yml:11-13 and infra/netris/roles/gather-infra/tasks/main.yml:11-13 to use the default filter’s true flag, so undefined or empty _detected_osac_ns.stdout falls back to osac_namespace. Keep both identical namespace-detection tasks consistent; shared extraction is optional.
318-333: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Hosted-cluster admin kubeconfig lands in
/tmpwith default permissions.
base64 -d > /tmp/caas-hosted-kubeconfigcreates a world-readable file holding cluster-admin credentials for the hosted cluster, and it stays there until the cleanup task at line 383 (which is skipped only on failure paths, but the window is the whole gather run). Tighten the mode when writing it.🔒 Proposed fix
+ umask 077 oc get secret "${SECRET_NAME}" -n "{{ _hc_ns.stdout }}" -o jsonpath='{.data.kubeconfig}' 2>/dev/null \ | base64 -d > /tmp/caas-hosted-kubeconfig 2>&1📝 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.- name: Extract hosted cluster kubeconfig ansible.builtin.shell: | SECRET_NAME=$(oc get hostedclusters.hypershift.openshift.io --all-namespaces -o jsonpath='{.items[0].status.kubeconfig.name}' 2>/dev/null) if [ -z "${SECRET_NAME}" ]; then echo "kubeconfig secret not available yet" > {{ _gather_dir }}/caas/hosted-cluster/status exit 0 fi umask 077 oc get secret "${SECRET_NAME}" -n "{{ _hc_ns.stdout }}" -o jsonpath='{.data.kubeconfig}' 2>/dev/null \ | base64 -d > /tmp/caas-hosted-kubeconfig 2>&1 echo "kubeconfig extracted from secret ${SECRET_NAME}" > {{ _gather_dir }}/caas/hosted-cluster/status environment: KUBECONFIG: /root/.kube/config changed_when: false failed_when: false no_log: true when: _hc_ns.stdout | default('') | length > 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 `@infra/netris/roles/gather-caas/tasks/main.yml` around lines 318 - 333, Update the “Extract hosted cluster kubeconfig” shell task to create /tmp/caas-hosted-kubeconfig with restrictive permissions, such as by setting its mode to owner-only before or during the base64 decode. Preserve the existing extraction and status behavior.infra/netris/roles/gather-infra/tasks/main.yml (1)
166-182: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Netris session cookie is interpolated into four un-
no_log'd shell commands.The login task at lines 155-164 correctly sets
no_log: true, then these four tasks paste the resultingconnect.sidstraight into the command line without it. Any verbose run or callback plugin output captures a live session credential into logs that get uploaded as CI artifacts. Pass it viaenvironment:and mark the tasksno_log: true, matching the fulfillment-API pattern ininfra/netris/roles/gather-caas/tasks/main.yml.🔒 Proposed fix (apply to all four)
- name: Gather Netris VPCs - ansible.builtin.shell: "curl -sf -b 'connect.sid={{ _gather_netris_cookie.stdout }}' {{ _gather_netris_url }}/api/v2/vpc 2>&1 | python3 -m json.tool > {{ _gather_dir }}/lab/netris/vpcs 2>&1 || echo 'unavailable' > {{ _gather_dir }}/lab/netris/vpcs" + ansible.builtin.shell: "curl -sf -b \"connect.sid=${NETRIS_COOKIE}\" {{ _gather_netris_url }}/api/v2/vpc 2>&1 | python3 -m json.tool > {{ _gather_dir }}/lab/netris/vpcs 2>&1 || echo 'unavailable' > {{ _gather_dir }}/lab/netris/vpcs" + environment: + NETRIS_COOKIE: "{{ _gather_netris_cookie.stdout }}" + no_log: true changed_when: false 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/gather-infra/tasks/main.yml` around lines 166 - 182, Update the four Netris gathering tasks—“Gather Netris VPCs”, “Gather Netris VNets”, “Gather Netris subnets”, and “Gather Netris NAT rules”—to stop interpolating _gather_netris_cookie.stdout into their shell commands. Pass the session cookie through environment: and configure each task with no_log: true, following the existing fulfillment-API pattern while preserving the current curl behavior.infra/netris/roles/netris-configure/tasks/main.yml (1)
89-98: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Guard the template-existence lookup, and reconsider
no_logon the GET.Two related concerns on this pair:
- If the controller returns
data: null(or a different envelope),_sct_list.json.data | selectattr(...)raises instead of falling through to creation. Default it to[].no_log: trueon the GET hides nothing sensitive beyond the cookie already inheaders, but it does make a failedwhenat Line 120 opaque. Keeping it is fine; just be aware you'll be debugging blind.🛡️ Suggested fix
- when: _sct_list.json.data | selectattr('name', 'equalto', caas_server_cluster_template_name) | list | length == 0 + when: >- + _sct_list.json.data | default([], true) + | selectattr('name', 'equalto', caas_server_cluster_template_name) + | list | length == 0Also applies to: 120-120
🧰 Tools
🪛 Checkov (3.3.8)
[medium] 89-101: Ensure that certificate validation isn't disabled with uri
(CKV_ANSIBLE_1)
[medium] 89-101: Ensure that HTTPS url is used with uri
(CKV2_ANSIBLE_1)
🤖 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/netris-configure/tasks/main.yml` around lines 89 - 98, Guard the template-existence check using a default empty list when `_sct_list.json.data` is null or missing, so the `selectattr` expression falls through to template creation. Update the related condition near the existing lookup logic while preserving the current `no_log` setting and selection behavior for valid response data.infra/netris/roles/ocp-dns/tasks/main.yml (2)
2-14: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
HostedZones[0]can be the wrong zone — records may land in someone else's hosted zone.
list-hosted-zones-by-name --dns-namereturns zones lexicographically at or after the given name; it is not an exact-match lookup. Ifdns_hosted_zonedoesn't exist in the account (typo, wrong account/creds),HostedZones[0].Idsilently resolves to the next zone, and the UPSERTs at Line 41 writeapi/api-int/*.appsrecords into an unrelated zone. Filter on the exact name and fail loudly when nothing matches.🛡️ Suggested fix
- name: Get Route 53 hosted zone ID ansible.builtin.shell: cmd: >- aws route53 list-hosted-zones-by-name --dns-name {{ dns_hosted_zone }} - --query "HostedZones[0].Id" + --query "HostedZones[?Name=='{{ dns_hosted_zone }}.'].Id | [0]" --output text environment: AWS_ACCESS_KEY_ID: "{{ aws_access_key_id }}" AWS_SECRET_ACCESS_KEY: "{{ aws_secret_access_key }}" AWS_DEFAULT_REGION: "{{ aws_region }}" register: _r53_zone_id changed_when: false + +- name: Fail if hosted zone was not found + ansible.builtin.fail: + msg: "Hosted zone {{ dns_hosted_zone }} not found in this AWS account" + when: _r53_zone_id.stdout | trim in ['', 'None']📝 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.- name: Get Route 53 hosted zone ID ansible.builtin.shell: cmd: >- aws route53 list-hosted-zones-by-name --dns-name {{ dns_hosted_zone }} --query "HostedZones[?Name=='{{ dns_hosted_zone }}.'].Id | [0]" --output text environment: AWS_ACCESS_KEY_ID: "{{ aws_access_key_id }}" AWS_SECRET_ACCESS_KEY: "{{ aws_secret_access_key }}" AWS_DEFAULT_REGION: "{{ aws_region }}" register: _r53_zone_id changed_when: false - name: Fail if hosted zone was not found ansible.builtin.fail: msg: "Hosted zone {{ dns_hosted_zone }} not found in this AWS account" when: _r53_zone_id.stdout | trim in ['', 'None']🤖 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/ocp-dns/tasks/main.yml` around lines 2 - 14, Update the “Get Route 53 hosted zone ID” task to filter the AWS CLI response for an exact hosted-zone name match, rather than selecting HostedZones[0] from the lexicographically ordered results. Ensure the task fails explicitly when no exact match exists, so subsequent UPSERTs cannot target an unrelated zone.
41-46: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low value
Single-quoted
--change-batchbreaks on an apostrophe in derived names.
ocp_cluster_name/ocp_base_domaincome fromLAB_NAME; any single quote terminates the shell quoting and mangles the command.ansible.builtin.commandwith an argv-safe temp file, or writing the batch viacopyand passing--change-batch file://..., removes the shell entirely.🤖 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/ocp-dns/tasks/main.yml` around lines 41 - 46, Replace the shell-based Route 53 invocation in the “Create OCP DNS records in Route 53” task with an argv-safe ansible.builtin.command or a temporary batch file passed via file://, so _r53_batch is not embedded in single-quoted shell text and apostrophes in derived names remain valid.infra/netris/roles/post-refresh-osac/tasks/main.yml (3)
20-30: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
This nodePort discovery lacks the retry loop its two siblings have.
prep-osac/tasks/main.yml(Lines 65-74) andnetris-configure/tasks/main.yml(Lines 2-11) both wrap the identical command inuntil: rc == 0 and stdout | length > 0with 10 retries. Here, a not-yet-ready traefik service yields empty stdout,changed_when: falsehides nothing, and_netris_ctl_urlbecomeshttp://192.168.16.254:— which then gets baked into the ConfigMap at Line 76 and the auth role at Line 36. Fail fast or retry.🛡️ Suggested fix
register: _traefik_nodeport changed_when: false + until: _traefik_nodeport.rc == 0 and _traefik_nodeport.stdout | length > 0 + retries: 10 + delay: 15📝 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.- name: Discover Netris controller URL ansible.builtin.shell: cmd: kubectl get svc traefik -n kube-system -o jsonpath='{.spec.ports[?(@.name=="web")].nodePort}' environment: KUBECONFIG: /etc/rancher/k3s/k3s.yaml register: _traefik_nodeport changed_when: false until: _traefik_nodeport.rc == 0 and _traefik_nodeport.stdout | length > 0 retries: 10 delay: 15 - name: Set Netris controller URL ansible.builtin.set_fact: _netris_ctl_url: "http://192.168.16.254:{{ _traefik_nodeport.stdout }}"🤖 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/post-refresh-osac/tasks/main.yml` around lines 20 - 30, Update the “Discover Netris controller URL” task around _traefik_nodeport to retry until the kubectl command succeeds and produces non-empty stdout, matching the existing retry behavior in the sibling tasks, with 10 retries. Ensure _netris_ctl_url is set only after a valid nodePort is discovered.
59-62: 🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Confirm jinja2_native is not enabled (which would make set_fact yield a dict, not a str) fd -H '^ansible\.cfg$' --exec grep -Hn -iE 'jinja2_native|native' {} \; fd -H '^ansible\.cfg$' --exec cat -n {} \; # Show both construction sites and their consumers rg -n 'netris_resource_class_map|NETRIS_RESOURCE_CLASS_MAP' -C2Repository: osac-project/osac-test-infra
Length of output: 2451
🏁 Script executed:
#!/bin/bash set -u echo "== files ==" git ls-files | rg 'ansible|infra/netris/roles/(post-refresh-osac|prep-osac)/tasks/main.yml' echo "== post-refresh map construction and consumers with context ==" sed -n '45,95p' infra/netris/roles/post-refresh-osac/tasks/main.yml rg -n 'netris_resource_class_map|NETRIS_RESOURCE_CLASS_MAP' -C4 echo "== prep-osac relevant lines if present ==" if [ -f infra/netris/roles/prep-osac/tasks/main.yml ]; then nl -ba infra/netris/roles/prep-osac/tasks/main.yml | sed -n '90,170p' fi echo "== search jinja_native in repo ==" rg -n 'jinja2_native|native' .Repository: osac-project/osac-test-infra
Length of output: 50384
🏁 Script executed:
#!/bin/bash set -u echo "== post-refresh map construction with context ==" nl -ba infra/netris/roles/post-refresh-osac/tasks/main.yml | sed -n '45,95p' echo "== prep-osac map construction with context ==" nl -ba infra/netris/roles/prep-osac/tasks/main.yml | sed -n '100,170p' echo "== exact consumers of netris_resource_class_map / NETRIS_RESOURCE_CLASS_MAP ==" rg -n 'netris_resource_class_map|NETRIS_RESOURCE_CLASS_MAP' infra netris 2>/dev/null || true echo "== jinja_native settings in role/config files only ==" rg -n 'jinja2_native|native' ansible.cfg infra/netris/ansible.cfg infra/netris/netris-lab/ansible.cfg 2>/dev/null || trueRepository: osac-project/osac-test-infra
Length of output: 919
🏁 Script executed:
#!/bin/bash set -u echo "== post-refresh map construction with context ==" awk '{printf "%6d %s\n", NR, $0}' infra/netris/roles/post-refresh-osac/tasks/main.yml | sed -n '45,95p' echo "== prep-osac map construction with context ==" awk '{printf "%6d %s\n", NR, $0}' infra/netris/roles/prep-osac/tasks/main.yml | sed -n '100,170p' echo "== exact consumers of netris_resource_class_map / NETRIS_RESOURCE_CLASS_MAP ==" rg -n 'netris_resource_class_map|NETRIS_RESOURCE_CLASS_MAP' infraRepository: osac-project/osac-test-infra
Length of output: 7488
Fix double JSON serialization for the Netris resource class map.
set_factwith a>-block stores a JSON string, so applying| to_jsonatpost-refresh-osac/tasks/main.yml:83encodes the string again. Consumers receive"{\"ci-worker\": ...}"instead of the intended object. Build the map as an Ansible dict and keepto_json, or store the raw string withoutto_json. The same pattern also serializes the raw string atprep-osac/tasks/main.yml:159.🤖 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/post-refresh-osac/tasks/main.yml` around lines 59 - 62, The _netris_resource_class_map value is currently stored as a JSON string and then serialized again by to_json. Update the “Build resource class map” set_fact to construct an Ansible dictionary instead of a folded JSON string, and apply the same correction to the corresponding raw-string map in the prep-osac task so downstream consumers receive a single correctly encoded object.
95-107: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Secrets are passed on the command line, so they leak to
/procand any shell tracing.
no_log: truehides them from Ansible output but not from the process table on the target host, nor from unquoted-expansion hazards (an AWS secret containing$, backtick, or whitespace breaks or injects here). Prefer an env-file written with mode0600, or feed literals via stdin.🔒️ Suggested direction
cmd: | + set -eo pipefail oc create secret generic cluster-fulfillment-ig -n {{ snapshot_osac_namespace }} \ - --from-literal=NETRIS_PASSWORD={{ netris_password }} \ - --from-literal=AWS_ACCESS_KEY_ID={{ aws_access_key_id }} \ - --from-literal=AWS_SECRET_ACCESS_KEY={{ aws_secret_access_key }} \ + --from-env-file=/dev/fd/3 \ --from-file=SERVER_SSH_KEY={{ ssh_public_key_path | regex_replace('\.pub$', '') }} \ --from-file=SERVER_SSH_BASTION_KEY={{ ssh_public_key_path | regex_replace('\.pub$', '') }} \ --dry-run=client -o yaml | oc apply -f -(Write the env file with
ansible.builtin.copy+mode: "0600"+no_log: truebeforehand, and remove it afterwards.)🤖 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/post-refresh-osac/tasks/main.yml` around lines 95 - 107, Replace the shell command in the secret-update task with a secure temporary env-file workflow: create the file using ansible.builtin.copy with mode "0600" and no_log enabled, pass its values to the secret-generation command without embedding secrets in command-line arguments, and remove the file afterward. Preserve the existing secret names, namespace, SSH key inputs, KUBECONFIG environment, and apply behavior.infra/netris/roles/snapshot-pull/tasks/main.yml (2)
2-8: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
The cache sentinel is both image-agnostic and set by partial writes.
Two failure modes from using bare
disk-0.qcow2existence as the cache key:
- Stale flavor. Change
snapshot_flavor_image(e.g.:caas→ a PR build, or the mutable:caastag itself moves) and the role happily reports "already cached" while deploying the previous flavor's disks. Extremely hard to debug downstream.- Truncated disk. The reassembly at Line 90 appends into
disk-0.qcow2incrementally. If it dies mid-way (OOM, disk full, killed run), the next invocation sees the file and skips repair, then boots a corrupt qcow2.Record the resolved image digest alongside the cache and compare it, and assemble to a
.partialfile renamed only on success.🛡️ Suggested direction
+- name: Read cached flavor image reference + ansible.builtin.slurp: + src: "{{ snapshot_flavor_dir }}/.cached-image" + register: _cached_ref + failed_when: false + - name: Check if flavor is already cached ansible.builtin.stat: path: "{{ snapshot_flavor_dir }}/disk-0.qcow2" register: _flavor_cached - name: Pull and extract flavor image - when: not _flavor_cached.stat.exists + when: >- + not _flavor_cached.stat.exists + or (_cached_ref.content | default('') | b64decode | trim) != snapshot_flavor_image block:And in the reassembly, write to
"${disk_file}.partial"andmvinto place as the final command, plus drop a.cached-imagemarker after the loop.Also applies to: 90-113
🤖 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/snapshot-pull/tasks/main.yml` around lines 2 - 8, Update the snapshot cache flow around _flavor_cached and the disk reassembly task: resolve and record the digest of snapshot_flavor_image in a .cached-image marker, and only treat the cache as valid when that marker matches the current resolved digest. During reassembly, write each disk to disk_file.partial, remove or replace any stale partial, and atomically move it to disk_file only after successful completion; create the matching cache marker after all disks finish.
98-109: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Look for the flavor manifest schema / chunk naming to confirm one-chunk-per-layer rg -n 'chunk_count|chunks/|chunk-' -C3 infra/netris fd -H 'manifest.json' infra/netris --exec cat -n {} \; # Any tooling in-repo that builds these flavor images? rg -n 'cluster-flavors|zstd|pigz' -C3 --iglob '!**/vendor/**'Repository: osac-project/osac-test-infra
Length of output: 5490
🏁 Script executed:
#!/bin/bash set -u echo "== file outline ==" ast-grep outline infra/netris/roles/snapshot-pull/tasks/main.yml --view expanded || true echo "== relevant task section ==" sed -n '60,115p' infra/netris/roles/snapshot-pull/tasks/main.yml | cat -n echo "== chunk_count usages ==" rg -n 'chunk_count|chunk-' -C2 --iglob '!**/vendor/**' . || true echo "== git tracked netris files with likely manifest names ==" git ls-files 'infra/netris/**' | rg 'manifest|disk|flavor|snapshot|oci' || true echo "== deterministic glob expansion comparison for numeric chunk names ==" python3 - <<'PY' from pathlib import Path import tempfile, glob, subprocess with tempfile.TemporaryDirectory() as d: p=Path(d) names=["chunk-1.zst","chunk-10.zst","chunk-2.zst","chunk-1.gz","chunk-10.gz","chunk-2.gz"] for n in names: (p/n).touch() print("glob *.zst:", glob.glob(str(p/"*.zst"))) print("shell wildcard *.zst:", subprocess.check_output(['sh','-c',"printf '%s\\n' $TMPDIR/chunks/*.zst"],env={'TMPDIR':str(p)}).decode().strip().split()) PYRepository: osac-project/osac-test-infra
Length of output: 4354
🏁 Script executed:
#!/bin/bash set -u echo "== shell wildcard numeric order ==" tmp="$(mktemp -d)" mkdir -p "$tmp/chunks" # Intentionally touch files so the shell expands non-null patterns for f in chunk-1.zst chunk-10.zst chunk-2.zst chunk-1.gz chunk-10.gz chunk-2.gz; do touch "$tmp/chunks/$f" done printf 'nullglob off, *.zst:\n' ( shopt -s ; printf '%s\n' "$tmp/chunks"/*.zst ) printf 'nullglob off, *.gz:\n' ( shopt -s ; printf '%s\n' "$tmp/chunks"/*.gz ) printf 'globbed arrays in bash:\n' bash -c " shopt -s nullglob files=(\"$tmp/chunks\"/*.zst) printf '%s\n' \"\${files[@]}\" " printf 'sort -V glob results:\n' bash -c " for chunk in $(printf '%s\n' \"$tmp/chunks\"/*.zst \"$tmp/chunks\"/*.gz | sort -V); do printf '%s\n' \"\$chunk\" done " printf 'count assertion suggestion output:\n' printf '%s\n' \ "<(cat \"$tmp/chunks\"/*.zst \"$tmp/chunks\"/*.gz 2>/dev/null | wc -c) == disk_size_expected"Repository: osac-project/osac-test-infra
Length of output: 1457
Enforce a deterministic chunk rebuild order.
In Bash,
/*.zstexpands in shell-dependent alphabetized order, sochunk-10can come beforechunk-2;sort -Vbefore decompression would prevent that. Also keep.zstbefore.gzonly if that is required per layer; otherwise pick and document the intended order. Addwc -c/size validation if this is relied on to produce a valid qcow2.🤖 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/snapshot-pull/tasks/main.yml` around lines 98 - 109, Update the chunk reconstruction loops in the snapshot pull task to sort matching chunk filenames with version-aware ordering before decompression, ensuring chunk-2 precedes chunk-10. Preserve or explicitly define the required per-layer ordering between .zst and .gz chunks, and add size validation with wc -c if the rebuilt qcow2 depends on complete output.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
infra/netris/roles/prep-refresh-osac/tasks/main.yml (1)
160-165: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not disable
osac-uiwhen an override image is supplied.The following task always sets
ui.enabled: false, soosac_ui_imagenever deploys. Disable the UI only when no override is configured.Proposed fix
- name: Disable osac-ui (image not published yet) ansible.builtin.shell: cmd: | if grep -q 'osac-ui' {{ osac_installer_dir }}/{{ snapshot_osac_values_file }}; then sed -i '/^ui:/,/^[a-z]/{s/enabled: true/enabled: false/}' \ {{ osac_installer_dir }}/{{ snapshot_osac_values_file }} fi changed_when: false + when: osac_ui_image | default('') | length == 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 `@infra/netris/roles/prep-refresh-osac/tasks/main.yml` around lines 160 - 165, Update the task that sets the OSAC UI enabled state to disable it only when osac_ui_image is empty or unset; when an override image is supplied, preserve or set ui.enabled to true so the image deploys. Use the existing osac_ui_image condition and the relevant UI values-editing task rather than changing the image replacement task.
🤖 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-refresh-osac/tasks/main.yml`:
- Around line 281-293: Update the shell command in the “Re-adopt postgres-init
secret for Helm release rename” task so failures from both oc annotate and oc
label propagate and cause the Ansible task to fail. Remove the explicit “||
true” masking and enable fail-fast behavior for the shell sequence while
preserving the existing conditional secret lookup and changed_when behavior.
In `@infra/netris/roles/restore-snapshot/tasks/main.yml`:
- Around line 731-740: Update the webhook pod readiness check to treat an empty
pods list as unavailable: after parsing the JSON in the inline Python script,
print NOT_READY and exit when pods is empty, while preserving the existing
container readiness checks for non-empty results.
---
Outside diff comments:
In `@infra/netris/roles/prep-refresh-osac/tasks/main.yml`:
- Around line 160-165: Update the task that sets the OSAC UI enabled state to
disable it only when osac_ui_image is empty or unset; when an override image is
supplied, preserve or set ui.enabled to true so the image deploys. Use the
existing osac_ui_image condition and the relevant UI values-editing task rather
than changing the image replacement task.
🪄 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: 274133aa-36de-4b4f-93bb-d740785bf31a
📒 Files selected for processing (2)
infra/netris/roles/prep-refresh-osac/tasks/main.ymlinfra/netris/roles/restore-snapshot/tasks/main.yml
Comment on lines
+731
to
+740
| oc get pods -n cert-manager -l app.kubernetes.io/component=webhook -o json | python3 -c " | ||
| import json, sys | ||
| pods = json.load(sys.stdin)['items'] | ||
| for pod in pods: | ||
| for cs in pod.get('status', {}).get('containerStatuses', []): | ||
| if not cs.get('ready', False): | ||
| print('NOT_READY') | ||
| sys.exit(0) | ||
| print('OK') | ||
| " |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Treat zero webhook pods as unavailable.
When the selector returns no pods, this loop prints OK, so recovery is skipped although the webhook is unavailable. Mark an empty pod list as NOT_READY (or explicitly validate the deployment’s Available condition).
🤖 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/restore-snapshot/tasks/main.yml` around lines 731 - 740,
Update the webhook pod readiness check to treat an empty pods list as
unavailable: after parsing the JSON in the inline Python script, print NOT_READY
and exit when pods is empty, while preserving the existing container readiness
checks for non-empty results.
Contributor
Author
|
/retest |
|
Re-triggered failed runs:
|
Contributor
Author
|
/retest |
|
Re-triggered failed runs:
|
Contributor
Author
|
/hold |
Contributor
Author
|
/unhold |
🚨 Credential leak scan found secrets in e2e run logs/artifactsWorkflow: E2E CaaS Netris Full Install Tainted raw logs and/or artifacts for that e2e run were deleted to close the exposure window. Rotate any credential(s) below that are real (not test/dev-only values).
|
Contributor
Author
|
/retest |
|
Re-triggered failed runs:
|
omer-vishlitzky
requested changes
Aug 2, 2026
omer-vishlitzky
left a comment
omer-vishlitzky
left a comment
Contributor
There was a problem hiding this comment.
Verdict: REQUEST CHANGES
Two blocking issues (security + correctness), five significant concerns.
Critical Issues (blocks merge)
1. .gitmodules points to personal fork
infra/netris/.gitmodules:3 — https://github.com/danmanor/netris-lab.git
Should be upstream repo URL, not author's personal fork. If submodule is initialized, it clones from a fork that could be deleted, force-pushed, or modified. Supply-chain risk.
2. pull_request trigger without fork guard
.github/workflows/e2e-caas-netris-full-install-caller.yml:8-9
Any PR — including from a fork — spins up a c5n.metal EC2 instance (~$3.89/hr) and stages Vault-sourced secrets (pull secret, AAP/Netris licenses, Route53 AWS creds) onto a box running the fork's code. Add:
jobs:
e2e-caas-netris:
if: github.event_name != 'pull_request' || github.event.pull_request.head.repo.full_name == github.repositorySignificant Concerns (should fix before merge)
3. No timeout on Deploy OSAC step — the slowest step at 49m
.github/workflows/e2e-caas-netris-full-install.yml:341-345 — "Deploy OSAC" has no timeout-minutes. Compare: "Deploy OCP from snapshot" on line 337 has timeout-minutes: 60. If make install hangs, it burns the full 360m job timeout. Add timeout-minutes: 90.
4. .env.infra written after failed install
infra/netris/Makefile:37-43 — deploy-osac runs install-osac or refresh-osac, then unconditionally writes .env.infra. If the install fails partway, the env file signals "deployment succeeded" to downstream validation (_validate-suite-contract in root Makefile:53-57). Move the write inside the fresh/snapshot targets as a final step, or guard on exit code.
5. Duplicate osac mono-repo clone
setup-infra/tasks/main.yml:129 clones osac to ~/.local/src/osac (build CLI). Then prep-osac/tasks/main.yml:23-28 clones the same repo to /opt/osac (for installer). On a c5n.metal box this wastes ~2 minutes + doubles disk usage. Either reuse the first clone or build CLI from the /opt/osac clone in prep-osac.
6. ~70-line inline Jinja expression for values overrides
prep-osac/tasks/main.yml:112-180 — Single set_fact with nested dictionaries, conditional combine(), regex, and secret interpolation. Extremely fragile: Jinja syntax error, missing pipe, or YAML indentation slip produces silent data corruption (wrong values file, missing secrets). Extract to a template file or split into multiple set_fact tasks for debuggability.
7. gather-mgmt-cluster bypasses root Makefile
.github/workflows/e2e-caas-netris-full-install.yml:376 calls make -C infra/netris -f contract.mk gather-mgmt-cluster directly, while gather-infra (line 370) and gather-suite (line 382) route through root Makefile targets. Breaks backend abstraction — add gather-mgmt-cluster to root Makefile or route through an existing target.
| @@ -0,0 +1,3 @@ | |||
| [submodule "netris-lab"] | |||
| path = netris-lab | |||
| url = https://github.com/danmanor/netris-lab.git | |||
Contributor
There was a problem hiding this comment.
Critical: Supply-chain risk. This points to a personal fork (danmanor/netris-lab), not the upstream repo. If anyone initializes this submodule (or CI does), they clone from a fork that could be deleted, force-pushed, or modified at any time.
🚨 Credential leak scan found secrets in e2e run logs/artifactsWorkflow: E2E CaaS Full Install Tainted raw logs and/or artifacts for that e2e run were deleted to close the exposure window. Rotate any credential(s) below that are real (not test/dev-only values).
|
omer-vishlitzky
requested changes
Aug 3, 2026
omer-vishlitzky
left a comment
omer-vishlitzky
left a comment
Contributor
There was a problem hiding this comment.
Re-review (commit 934ba28)
Previous findings checked against updated code. One dropped (.env.infra write is actually guarded by make's error handling). Remaining issues persist.
Critical Issues (blocks merge)
1. .gitmodules points to personal fork
infra/netris/.gitmodules:3 — https://github.com/danmanor/netris-lab.git
Supply-chain risk. If submodule is initialized, clones from a personal account that could be deleted, force-pushed, or modified. Should use an org-owned repo.
2. pull_request trigger without fork guard
.github/workflows/e2e-caas-netris-full-install-caller.yml:8-9
Any fork PR spins up a c5n.metal EC2 instance (~$3.89/hr) and stages Vault-sourced secrets (pull secret, AAP/Netris licenses, Route53 AWS creds) onto a box running the fork's code.
Fix:
jobs:
e2e-caas-netris:
if: github.event_name != 'pull_request' || github.event.pull_request.head.repo.full_name == github.repositoryAlso consider a paths filter to skip metal for doc-only changes.
Significant Concerns (should fix before merge)
3. No timeout on Deploy OSAC step — the slowest step at 49m
.github/workflows/e2e-caas-netris-full-install.yml:341-345 — "Deploy OSAC" has no timeout-minutes. Compare: "Deploy OCP from snapshot" on line 336 has timeout-minutes: 60. If make install hangs, it burns the full 360m job timeout. Add timeout-minutes: 90.
4. Duplicate osac mono-repo clone
setup-infra/tasks/main.yml:133 clones osac to ~/.local/src/osac (build CLI). Then prep-osac/tasks/main.yml:23 clones the same repo again to /opt/osac (for installer). ~2 minutes wasted + doubled disk on a c5n.metal box. Reuse the first clone or build CLI from /opt/osac.
5. ~72-line inline Jinja expression for values overrides
prep-osac/tasks/main.yml:120-191 — Single set_fact with nested dictionaries, conditional combine(), regex, and secret interpolation. A Jinja syntax error, missing pipe, or YAML indentation slip produces silent data corruption. Extract to a template file or split into multiple set_fact tasks for debuggability.
6. gather-mgmt-cluster and deploy-caas bypass root Makefile
Workflow line 376 calls make -C infra/netris gather-mgmt-cluster directly, and line 358 calls make -C infra/netris deploy-caas directly. Meanwhile gather-infra (line 370), gather-suite (line 382), setup-suite (line 351), and all deploy targets go through root Makefile. Inconsistent — breaks the backend abstraction that the rest of the workflow follows.
Dropped from previous review
.env.infrawritten after failed install — invalid. Make's error handling prevents theprintffrom executing if theif/firecipe line exits non-zero.
| @@ -0,0 +1,3 @@ | |||
| [submodule "netris-lab"] | |||
| path = netris-lab | |||
| url = https://github.com/danmanor/netris-lab.git | |||
Contributor
There was a problem hiding this comment.
Critical: Supply-chain risk. Points to a personal fork (danmanor/netris-lab). Should use an org-owned repo — personal accounts can be deleted, force-pushed, or modified without review.
| # e2e-caas-netris-caller.yml (which only added its own schedule once that | ||
| # mechanism was proven in real dispatches). | ||
| on: | ||
| pull_request: |
Contributor
There was a problem hiding this comment.
Critical: No fork guard. Fork PRs spin up c5n.metal + stage Vault secrets onto a box running fork code.
jobs:
e2e-caas-netris:
if: github.event_name != 'pull_request' || github.event.pull_request.head.repo.full_name == github.repository| repo: "{{ osac_repo }}" | ||
| dest: "{{ osac_dir }}" | ||
| version: "{{ osac_branch }}" | ||
| force: true |
Contributor
There was a problem hiding this comment.
Duplicate clone. setup-infra already clones osac to ~/.local/src/osac. This clones it again to /opt/osac. ~2 min wasted + doubled disk. Reuse first clone or build CLI from this one.
| if osac_aap_image | default('') | length > 0 else {}), recursive=true) | ||
| | combine(({'images': {'ui': osac_ui_image}} | ||
| if osac_ui_image | default('') | length > 0 else {}), recursive=true) | ||
| }} |
Contributor
There was a problem hiding this comment.
72-line inline Jinja. Nested dicts + conditional combine() + regex + secret interpolation in one set_fact. Silent data corruption on any syntax slip. Extract to template or split into multiple tasks.
Contributor
Author
|
Addressed the re-review findings in 9565e7a: Fixed:
Not changing:
|
…ll path Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: danmanor, omer-vishlitzky 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 |
Contributor
Author
|
/retest |
|
Re-triggered failed runs:
|
2 tasks done
This was referenced Aug 5, 2026
This branch was previously 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
3 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-ocpanddeploy-osactargetsOSAC_DEPLOY_MODEtoggle (fresh/snapshot) — fresh installs OSAC from scratch via Helm, snapshot refreshes a baked-in OSACosac_repo/osac_branch— the mono-repo (osac-project/osac) contains osac-installer, osac-operator, fulfillment-service, and osac-aap as subdirectories, so we clone once and use subdirectories directlyOverride osac-operator code,Override fulfillment-service code, etc.) — no longer needed with the mono-repoosac_operator_image,fulfillment_service_image,osac_aap_image) as separate variablesosac-repo/osac-branchinstead of per-component repo/branch)Variables (before → after)
osac_installer_repo+osac_installer_branchosac_repo+osac_branchosac_operator_repo+osac_operator_branchfulfillment_service_repo+fulfillment_service_branchosac_aap_repo+osac_aap_branchosac_installer_dir=/opt/osac-installerosac_dir=/opt/osac,osac_installer_dir=/opt/osac/osac-installerPipeline (fresh mode, default)
Removed
setup-ocp.yml/install-ocp.yml)assisted_service,ocp_install,vm_resizedeploy,deploy-fast,deploy-ocp-snapshot,snapshot-refreshdestroy_caasduplicate role (underscored copy)Test plan
ansible-playbook --syntax-checkmake -n)prep-osacclonesosac-project/osacto/opt/osacdeploy-ocp+deploy-osac+setup-caas(in progress)Assisted-by: Claude Code noreply@anthropic.com