Repository navigation
OSAC-3616: Netris MaaS/CaaS infra feats & fixes - #323
Conversation
|
@sruiz-rh: This pull request references OSAC-3616 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. |
WalkthroughThe Netris infrastructure adds suite-aware CaaS and MaaS deployment flows, snapshot restoration, cluster configuration, synchronous and force cleanup, OSAC refresh synchronization, and redeployment state handling. ChangesNetris suite lifecycle
Estimated code review effort: 5 (Critical) | ~100 minutes Sequence Diagram(s)sequenceDiagram
participant RedeployServer as redeploy-server.sh
participant Makefile
participant Ansible
RedeployServer->>Makefile: pass SUITE and OSAC_DEPLOY_MODE
Makefile->>Ansible: run suite-specific setup and deployment
Ansible-->>RedeployServer: report deployment status
sequenceDiagram
participant ForceDestroy as force-destroy-caas
participant Kubernetes
participant DiscoveryVMs as discovery-worker VMs
participant Netris
ForceDestroy->>Kubernetes: force-delete order resources
ForceDestroy->>DiscoveryVMs: stop and remove worker resources
ForceDestroy->>Netris: delete orphaned resources
Possibly related PRs
Suggested labels: Suggested reviewers: Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (3 errors, 2 warnings)
✅ Passed checks (6 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
🧪 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 |
|
Check the jira ticket for details on all the changes and why they are needed. |
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Nitpick comments (2)
infra/netris/roles/force-destroy-caas/files/netris_orphan_cleanup.py (1)
157-171: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe VPC filter is duplicated.
Lines 80-86 already computed
targets_vpcwith theisSystem,isDefault,Default, andocp-snoexclusions. Lines 160-163 repeat the same four conditions after a secondGET /api/v2/vpc. The refetch is intentional, because the earlier server-cluster deletions change VPC state. Extract the predicate so the two sites cannot drift.♻️ Proposed refactor
+def is_deletable_vpc(v): + name = v.get("name") or "" + return ( + should_delete(name) + and not v.get("isSystem") + and not v.get("isDefault") + and name not in ("Default", "ocp-sno") + ) + -targets_vpc = [ - v for v in vpcs - if should_delete(v.get("name")) - and not v.get("isSystem") - and not v.get("isDefault") - and v.get("name") not in ("Default", "ocp-sno") -] +targets_vpc = [v for v in vpcs if is_deletable_vpc(v)]Then use
if not is_deletable_vpc(v): continuein the final loop.🤖 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/force-destroy-caas/files/netris_orphan_cleanup.py` around lines 157 - 171, Extract the shared VPC eligibility logic into an is_deletable_vpc(v) predicate, including the should_delete name check and the isSystem, isDefault, Default, and ocp-sno exclusions. Use this predicate both when computing targets_vpc and in the final refetched VPC deletion loop, preserving the intentional second GET and existing deletion behavior.infra/netris/roles/force-destroy-caas/tasks/main.yml (1)
149-165: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffPrefer an Ansible
untilloop for the namespace finalize retry.The 10-iteration bash loop with
sleep 2reimplements a retry that Ansible expresses directly. Splitting the finalize step into its own task withuntil,retries, anddelayalso makes the wait visible in job output. This is optional, because the loop is bounded and the surrounding logic is one shell unit.As per coding guidelines: "Implement waits and retries with Ansible
untilloops using bothretriesanddelay."🤖 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/force-destroy-caas/tasks/main.yml` around lines 149 - 165, Replace the bounded bash retry loop around namespace finalization with a dedicated Ansible task using an until condition, retries, and delay. Preserve the existing checks, re-force behavior, and finalize operations by moving them into the task’s command or shell block, and expose each retry through Ansible output.Source: Coding guidelines
🤖 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/CLAUDE.md`:
- Around line 72-75: Update the service-model description in the repository
README to remove VMaaS and BMaaS, leaving only the implemented MaaS model. Keep
the documentation consistent with the status recorded in CLAUDE.md.
In `@infra/netris/netris-lab/roles/connectivity/tasks/vpn.yml`:
- Around line 22-27: Replace the unverified ssh-keygen removal and
StrictHostKeyChecking=no flow in the OpenVPN SSH check with trusted host-key
provisioning: obtain the redeployed mgmt-server key or fingerprint from the
authoritative source, install or validate it in known_hosts, and connect only
with SSH host-key verification enabled. Preserve the connectivity check while
ensuring the key is verified before authentication.
In `@infra/netris/roles/create-caas/tasks/main.yml`:
- Around line 36-45: Update the explicit cluster creation flow around the shell
command building the args array to pass the configured caas_cluster_version as
the MaaS release/version parameter, matching the field-based path’s behavior.
Ensure the value is included in caas_cluster_params or provided through an
equivalent --version/release_image argument so both creation paths use the
configured OCP release.
In `@infra/netris/roles/destroy-caas/tasks/main.yml`:
- Around line 8-10: The OSAC namespace selection in
infra/netris/roles/destroy-caas/tasks/main.yml lines 8-10 and
infra/netris/roles/force-destroy-caas/tasks/main.yml lines 8-11 must probe the
cluster instead of trusting snapshot_osac_namespace. Replace the destroy-caas
set_fact with the existing oc namespace probe pattern, store its result, and
derive _destroy_osac_ns from that probe; apply the same probe-based assignment
to _force_osac_ns in force-destroy-caas while leaving _force_agent_ns unchanged.
- Around line 41-69: Update the cluster lookup task registered as _cluster_id to
distinguish command or JSON parsing failures from a successful lookup with no
matching cluster. Do not allow failed_when: false to make failures appear as
empty stdout; explicitly fail or otherwise propagate lookup errors, and only run
“Report when named cluster is already absent” when the lookup succeeded and
returned no ID. Preserve the existing skip-delete behavior only for a valid
successful no-match result.
- Around line 71-88: Add an Ansible polling task after “Delete cluster via osac
CLI” and before Step 2, using osac get clusters for the target cluster and an
until condition that confirms the cluster is no longer present or teardown is
complete. Configure bounded retries and a delay, and ensure the poll is skipped
when _cluster_id is empty or deletion was skipped; keep the existing failure
warning behavior intact.
In `@infra/netris/roles/disk-setup/tasks/main.yml`:
- Around line 32-52: Update the disk-selection logic in the “Find largest unused
data disk” task to stop inferring ownership from mount state. Require the disk
to be explicitly listed in the inventory or to match an approved dedicated
partition label/UUID, and reject all other disks before any later mount or
reuse; remove the current unmounted-disk fallback while preserving root and nbd
exclusions.
In `@infra/netris/roles/force-destroy-caas/files/netris_orphan_cleanup.py`:
- Around line 111-118: Replace the substring-based name checks in the NAT
cleanup loop’s `hit` expression, and the corresponding vnet checks, with
boundary-aware exact order matching or rely only on `vpc.id` membership in
`orphan_vpc_ids`. Ensure short orphan names such as `order-ab` cannot match
longer live order names like `order-abcd`, while preserving deletion by
confirmed orphan VPC IDs.
- Around line 35-53: Update the authentication POST before get_list to capture
and validate the curl response HTTP status, reporting the response body and
exiting non-zero when login fails instead of continuing with an empty cookie
jar. In get_list, narrow the broad exception handler around json.loads to
json.JSONDecodeError and report the malformed response body before returning the
existing empty-list fallback.
In `@infra/netris/roles/force-destroy-caas/tasks/main.yml`:
- Around line 209-254: Resolve each entry in caas_discovery_vm_patterns to
matching libvirt domain names using the existing discovery approach, store the
exact names in _force_vms, and use loop: "{{ _force_vms }}" for both the “Wipe
discovery worker VMs” and “Delete any remaining Agent CRs” tasks. Keep dominfo,
disk cleanup, and agent hostname matching operating on each resolved VM name.
- Around line 14-58: The Bash-dependent shell tasks must explicitly run with
Bash rather than the default /bin/sh. Add args with executable set to /bin/bash
to the order collection task containing _force_orders_raw and the corresponding
per-order cleanup, VM wipe, and Agent cleanup tasks, preserving their existing
commands and failure behavior.
- Around line 274-286: Update the “Resolve Netris password” task to remove the
`${pass:-netris}` fallback and fail when the secret does not provide
NETRIS_PASSWORD. Preserve successful password resolution, but make the task
report a clear missing-secret error before the Netris cleanup path proceeds,
using the existing _netris_pass result and task failure controls.
In `@infra/netris/roles/patch-osac-refresh/tasks/sync-aap-project-pin.yml`:
- Around line 247-259: Before the POST task, query the Controller job-template
API for the job template named osac-config-as-code, filtering to the intended
organization and ensuring exactly the required template is selected. Store its
numeric ID or full name++organization URL identifier, then update the
_aap_cac_job launch request to use that resolved identifier instead of the
ambiguous name-only path.
In `@infra/netris/roles/setup-caas/tasks/main.yml`:
- Around line 266-269: Update the existing-entry validation around
NETRIS_RESOURCE_CLASS_MAP so the unchanged path only accepts an entry containing
valid server_cluster_template_id and vpc_interfaces fields. Treat dictionaries
missing either required field or containing malformed values as invalid, then
replace them with the generated host-type entry before cluster-fulfillment-ig
consumes the map; retain the current early exit only for fully valid entries.
---
Nitpick comments:
In `@infra/netris/roles/force-destroy-caas/files/netris_orphan_cleanup.py`:
- Around line 157-171: Extract the shared VPC eligibility logic into an
is_deletable_vpc(v) predicate, including the should_delete name check and the
isSystem, isDefault, Default, and ocp-sno exclusions. Use this predicate both
when computing targets_vpc and in the final refetched VPC deletion loop,
preserving the intentional second GET and existing deletion behavior.
In `@infra/netris/roles/force-destroy-caas/tasks/main.yml`:
- Around line 149-165: Replace the bounded bash retry loop around namespace
finalization with a dedicated Ansible task using an until condition, retries,
and delay. Preserve the existing checks, re-force behavior, and finalize
operations by moving them into the task’s command or shell block, and expose
each retry through Ansible output.
🪄 Autofix
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: CHILL
Plan: Pro Plus
Run ID: 30d3e2f4-0f68-435f-83f6-f51a008b1e85
📒 Files selected for processing (21)
infra/netris/CLAUDE.mdinfra/netris/Makefileinfra/netris/README.mdinfra/netris/inventory/group_vars/all.ymlinfra/netris/netris-lab/group_vars/all.ymlinfra/netris/netris-lab/roles/connectivity/tasks/vpn.ymlinfra/netris/playbooks/destroy-full.ymlinfra/netris/playbooks/force-destroy-caas.ymlinfra/netris/roles/create-caas/tasks/main.ymlinfra/netris/roles/destroy-caas/tasks/main.ymlinfra/netris/roles/discover-caas/tasks/main.ymlinfra/netris/roles/disk-setup/tasks/main.ymlinfra/netris/roles/force-destroy-caas/files/netris_orphan_cleanup.pyinfra/netris/roles/force-destroy-caas/tasks/main.ymlinfra/netris/roles/patch-osac-refresh/tasks/main.ymlinfra/netris/roles/patch-osac-refresh/tasks/sync-aap-project-pin.ymlinfra/netris/roles/restore-ocp/tasks/main.ymlinfra/netris/roles/setup-caas/tasks/main.ymlinfra/netris/roles/setup-infra/tasks/main.ymlinfra/netris/scripts/deploy-jump.shinfra/netris/scripts/redeploy-server.sh
| **MaaS:** deploy or deploy-fast → setup-maas → deploy-maas | ||
| (or `make redeploy-fresh SUITE=maas OSAC_DEPLOY_MODE=snapshot`) | ||
|
|
||
| **VMaaS / BMaaS:** not yet implemented |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Align the service-model status in both documents.
Line 75 states that VMaaS and BMaaS are not implemented. infra/netris/README.md line 3 still states that the repository runs VMaaS and BMaaS end-to-end tests. Update the README to list only implemented service models.
🤖 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/CLAUDE.md` around lines 72 - 75, Update the service-model
description in the repository README to remove VMaaS and BMaaS, leaving only the
implemented MaaS model. Keep the documentation consistent with the status
recorded in CLAUDE.md.
| - name: Remove stale SSH host key for mgmt-server | ||
| # After destroy/redeploy the mgmt-server VM gets a new host key; clear | ||
| # known_hosts so the OpenVPN SSH check below does not fail with | ||
| # "REMOTE HOST IDENTIFICATION HAS CHANGED". | ||
| shell: ssh-keygen -R 192.168.122.10 2>/dev/null || true | ||
| changed_when: false |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and inspect the VPN task file and nearby OpenVPN SSH checks without assuming repository size.
fd -a 'vpn.yml|openvpn|connectivity' . | sed 's#^\./##' | head -100
echo
echo "Target file lines:"
cat -n infra/netris/netris-lab/roles/connectivity/tasks/vpn.yml | sed -n '1,80p'
echo
echo "Search for managed host/key/fingerprint/known_hosts references:"
rg -n "192\.168\.122\.10|mgmt-server|StrictHostKeyChecking|ssh-keygen|host_certificate|FINGERPRINT|fingerprint|known_hosts|openssh|openvpn|OpenVPN" infra/netris/netris-lab/roles/connectivity -S || true
echo
echo "Commit/branch context:"
git rev-parse --show-toplevel
git status --short
git rev-parse --abbrev-ref HEADRepository: osac-project/osac-test-infra
Length of output: 7625
🌐 Web query:
OpenSSH ssh StrictHostKeyChecking=no disables host key verification manual
💡 Result:
Setting StrictHostKeyChecking=no in OpenSSH disables the interactive prompt that verifies a remote host's identity, effectively instructing the SSH client to automatically accept any host key presented by a server [1][2][3]. Behavior and Effects: When StrictHostKeyChecking is set to no (or off): - New Host Keys: The client automatically adds new host keys to the user's known_hosts file without user intervention [1][3]. - Changed Host Keys: The client will permit connections to hosts even if their host key has changed [1][3]. While the connection itself may proceed, it is important to note that the client may still flag the mismatch in its output [4]. Security Implications: Disabling host key verification significantly lowers the security of your SSH connection by making you vulnerable to Man-in-the-Middle (MITM) attacks [5][6][7]. Because the client no longer verifies that the server is the one you intend to connect to, an attacker could intercept the connection, present their own key, and potentially view or manipulate your traffic [6][7]. Critical Risk: This setting is particularly dangerous if you use SSH agent forwarding (e.g., ssh -A). If an attacker successfully intercepts your connection, they may be able to hijack your SSH agent to access other systems you have permissions for, which is a major security risk [6]. Manual Alternative: If your goal is to avoid annoying prompts while maintaining security, consider using the accept-new value instead of no [5]. - StrictHostKeyChecking=accept-new: This will automatically add new host keys to your known_hosts file but will still refuse to connect if a host key has changed, preserving protection against MITM attacks for existing hosts [1][3]. For automation purposes where you must avoid prompts but want to maintain a level of security, a common practice is to combine StrictHostKeyChecking=no with UserKnownHostsFile=/dev/null (to avoid cluttering your known_hosts file) while carefully considering the security risks in your specific environment [4].
Citations:
- 1: https://superuser.com/questions/1751932/what-are-the-restrictions-to-ssh-stricthostkeychecking-no
- 2: https://man.openbsd.org/ssh_config.5?lang=en
- 3: https://man7.org/linux/man-pages/man5/ssh_config.5.html
- 4: https://askubuntu.com/questions/1462362/for-ssh-command-line-tool-why-is-stricthostkeychecking-no-not-effective
- 5: https://linux-audit.com/ssh/config/client/option-stricthostkeychecking/
- 6: https://security.stackexchange.com/questions/39990/is-it-safe-to-disable-ssh-host-key-checking-if-key-based-authentication-is-used
- 7: https://stackoverflow.com/questions/30178936/jsch-sftp-security-with-session-setconfigstricthostkeychecking-no
Security Misconfiguration (CWE-295): Improper Certificate Validation
Preserve SSH server authentication after redeploy.
Lines 22-30 remove any known host key for 192.168.122.10, then connect with StrictHostKeyChecking=no. The OpenVPN status check can therefore contact an SSH server impersonating the management server. Provision a trusted replacement host key or fingerprint before connecting; do not accept the new SSH key without verification.
🤖 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/netris-lab/roles/connectivity/tasks/vpn.yml` around lines 22 -
27, Replace the unverified ssh-keygen removal and StrictHostKeyChecking=no flow
in the OpenVPN SSH check with trusted host-key provisioning: obtain the
redeployed mgmt-server key or fingerprint from the authoritative source, install
or validate it in known_hosts, and connect only with SSH host-key verification
enabled. Preserve the connectivity check while ensuring the key is verified
before authentication.
| - name: Create cluster (explicit -p params) | ||
| ansible.builtin.shell: | | ||
| set -euo pipefail | ||
| args=( | ||
| osac create cluster | ||
| --template "{{ caas_cluster_template }}" | ||
| --pull-secret-file "{{ pull_secret_path }}" | ||
| --ssh-public-key-file "{{ ssh_public_key_path }}" | ||
| --name "{{ caas_cluster_name }}" | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Compare common arguments between both cluster-creation paths.
rg -n -C 6 'osac create cluster|caas_cluster_version|caas_cluster_params' \
infra/netris/roles/create-caas/tasks/main.yml \
infra/netris/MakefileRepository: osac-project/osac-test-infra
Length of output: 5708
🌐 Web query:
osac create cluster --version --pull-secret-file --ssh-public-key-file option documentation
💡 Result:
The osac command-line interface (CLI) is a tool used within the Open Sovereign AI Cloud (OSAC) project to manage infrastructure, including OpenShift clusters [1][2]. Based on the current project structure and documentation [2], infrastructure creation (such as clusters) is primarily template-driven rather than managed by individual flags for parameters like pull secrets or SSH keys in the main command syntax [1]. Instead, these configurations are typically handled via template parameters [1]. Key points regarding the creation of clusters: 1. Template-Driven Approach: Clusters are created using templates [1]. The standard syntax for creating a cluster involves specifying the object type (cluster) and the template identifier, often accompanied by the --parameter or -p flag to pass specific configurations like pull secrets, SSH public keys, or release images [1]. Example usage: osac create cluster --template --name -p pull_secret= -p ssh_public_key= 2. Underlying Mechanism: The OSAC fulfillment service manages these requests and coordinates with the OSAC operator, which reconciles ClusterOrder custom resources [3][2]. In these underlying resources, fields such as pullSecret, sshPublicKey, and releaseImage are part of the specification [4]. 3. CLI Usage: While specific flags like --pull-secret-file or --ssh-public-key-file may not be native top-level flags on the osac command itself, parameters are passed as key-value pairs using the -p/--parameter flag to the template orchestration engine [1][5]. For the most accurate and up-to-date information, refer to the following resources: - The OSAC documentation repository: https://github.com/osac-project/docs [6] - The fulfillment-service/CLI repository: https://github.com/osac-project/fulfillment-service [7] If you are using a specific downstream version or customized deployment, ensure you check the specific template definitions in your environment's OSAC catalog, as they dictate which parameters are supported [1][5].
Citations:
- 1: https://github.com/osac-project/fulfillment-cli
- 2: https://github.com/osac-project/osac
- 3: https://github.com/osac-project/osac-operator/
- 4: OSAC-1323: Update cluster-fulfillment.md to match current codebase docs#32
- 5: OSAC-58: add demo_ocp_4_17 template role for catalog items demo osac-aap#318
- 6: https://github.com/osac-project/docs/
- 7: https://github.com/osac-project/fulfillment-service/
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== candidate files =="
git ls-files | rg 'osac|release|cluster|template|deploy|Makefile|requirements|ansible.cfg' | sed -n '1,200p'
echo
echo "== variable definitions/default values =="
rg -n -C 4 'caas_cluster_version|caas_cluster_template|caas_cluster_params|MAAS_PARAMS|MAAS_EXTRA' .
echo
echo "== osac references =="
rg -n -C 3 '"osac"|osac create|osac-command|osac-cli|--version|release' .Repository: osac-project/osac-test-infra
Length of output: 50385
🌐 Web query:
site:github.com/osac-project/osac "osac create cluster" "--version"
💡 Result:
The osac CLI tool, which accompanies the Open Sovereign AI Cloud (OSAC) fulfillment service, does not support a --version flag in the context of a create cluster command [1][2][3]. The osac create cluster command is used for provisioning infrastructure, such as OpenShift clusters, and typically relies on specific templates rather than version flags to define the cluster configuration [4][5][6]. To manage clusters, users interact with the fulfillment service through the osac CLI after authenticating [2]. Because osac is a command-line interface for the fulfillment service, information about the CLI tool's own version is typically managed through the build process or standard release binaries provided in the OSAC fulfillment-service repository [2][3]. For detailed usage and command references, you should consult the documentation within the fulfillment-service subdirectory of the official OSAC repository [1][2]. If you need to verify the version of the installed osac binary itself, it is generally standard to check if the tool supports a top-level --version flag (e.g., osac --version), though this is distinct from the cluster creation command syntax [2].
Citations:
- 1: https://github.com/osac-project/osac
- 2: https://github.com/osac-project/osac-installer
- 3: https://github.com/osac-project/fulfillment-service/
- 4: https://github.com/osac-project/osac-operator
- 5: OSAC-1829: remove legacy ocp_4_17_small cluster templates osac-aap#431
- 6: https://github.com/osac-project/osac-operator/
Put caas_cluster_version into the MaaS cluster creation parameters.
deploy-maas runs this path when caas_cluster_params is non-empty, but it only passes the listed -p arguments. The field path passes --version {{ caas_cluster_version }}; without it, MaaS may use a fallback version instead of the configured OCP release. Include release_image={{ caas_cluster_version }} in caas_cluster_params or add an equivalent version parameter so both paths use the configured release.
🤖 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/create-caas/tasks/main.yml` around lines 36 - 45, Update
the explicit cluster creation flow around the shell command building the args
array to pass the configured caas_cluster_version as the MaaS release/version
parameter, matching the field-based path’s behavior. Ensure the value is
included in caas_cluster_params or provided through an equivalent
--version/release_image argument so both creation paths use the configured OCP
release.
| - name: Resolve OSAC namespace for destroy | ||
| ansible.builtin.set_fact: | ||
| _destroy_osac_ns: "{{ snapshot_osac_namespace | default(osac_namespace) }}" |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Both roles trust snapshot_osac_namespace instead of probing the cluster. snapshot_osac_namespace | default(osac_namespace) falls back only when the variable is undefined. If the variable is defined in inventory but the namespace does not exist, both roles target a missing namespace. destroy-caas then fails at line 21 and stops the play before agents, InfraEnv, and VM disks are cleaned; force-destroy-caas finds nothing and reports success. infra/netris/roles/gather-caas/tasks/main.yml lines 2-12 already solve this with an oc get namespace probe.
infra/netris/roles/destroy-caas/tasks/main.yml#L8-L10: replace theset_factwith theoc get namespaceprobe plus aset_facton its result, and set_destroy_osac_nsfrom the probe.infra/netris/roles/force-destroy-caas/tasks/main.yml#L8-L11: set_force_osac_nsfrom the same probe, and keep_force_agent_nsunchanged.
📍 Affects 2 files
infra/netris/roles/destroy-caas/tasks/main.yml#L8-L10(this comment)infra/netris/roles/force-destroy-caas/tasks/main.yml#L8-L11
🤖 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 8 - 10, The OSAC
namespace selection in infra/netris/roles/destroy-caas/tasks/main.yml lines 8-10
and infra/netris/roles/force-destroy-caas/tasks/main.yml lines 8-11 must probe
the cluster instead of trusting snapshot_osac_namespace. Replace the
destroy-caas set_fact with the existing oc namespace probe pattern, store its
result, and derive _destroy_osac_ns from that probe; apply the same probe-based
assignment to _force_osac_ns in force-destroy-caas while leaving _force_agent_ns
unchanged.
| - name: Resolve cluster ID for {{ caas_cluster_name }} | ||
| ansible.builtin.shell: | | ||
| set -euo pipefail | ||
| osac get clusters -o json | python3 -c ' | ||
| import json, sys | ||
| raw = json.load(sys.stdin) | ||
| clusters = raw if isinstance(raw, list) else (raw.get("items") or [raw]) | ||
| name = sys.argv[1] | ||
|
|
||
| def cluster_name(c): | ||
| if not isinstance(c, dict): | ||
| return "" | ||
| # Table "NAME" column is metadata.name; top-level name is often unset. | ||
| meta = c.get("metadata") if isinstance(c.get("metadata"), dict) else {} | ||
| return (meta.get("name") or c.get("name") or "") | ||
|
|
||
| matches = [c.get("id", "") for c in clusters if cluster_name(c) == name and c.get("id")] | ||
| print(matches[0] if matches else "") | ||
| ' "{{ caas_cluster_name }}" | ||
| environment: | ||
| KUBECONFIG: /root/.kube/config | ||
| register: _cluster_id | ||
| changed_when: false | ||
| failed_when: false | ||
|
|
||
| # Fire-and-forget: async with no polling | ||
| - name: Delete cluster | ||
| - name: Report when named cluster is already absent | ||
| ansible.builtin.debug: | ||
| msg: "No osac cluster named '{{ caas_cluster_name }}' — skip osac delete" | ||
| when: (_cluster_id.stdout | trim | length) == 0 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
A failed cluster lookup is reported as "cluster already absent".
set -euo pipefail plus failed_when: false means any osac get clusters or JSON parse failure leaves _cluster_id.stdout empty. Line 69 then prints "already absent" and line 77 skips the delete. A login problem, an API error, or an unexpected payload shape silently turns destroy into a no-op — the exact failure mode the header comment says this change fixes.
Separate "lookup failed" from "no match".
🛠️ Proposed fix: distinguish lookup failure from no match
register: _cluster_id
changed_when: false
failed_when: false
+
+- name: Fail when cluster lookup did not complete
+ ansible.builtin.fail:
+ msg: >-
+ Cluster lookup failed (rc={{ _cluster_id.rc }}):
+ {{ _cluster_id.stderr | default('') | trim }}
+ when: _cluster_id.rc != 0
- name: Report when named cluster is already absent
ansible.builtin.debug:
msg: "No osac cluster named '{{ caas_cluster_name }}' — skip osac delete"
- when: (_cluster_id.stdout | trim | length) == 0
+ when:
+ - _cluster_id.rc == 0
+ - (_cluster_id.stdout | trim | 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: Resolve cluster ID for {{ caas_cluster_name }} | |
| ansible.builtin.shell: | | |
| set -euo pipefail | |
| osac get clusters -o json | python3 -c ' | |
| import json, sys | |
| raw = json.load(sys.stdin) | |
| clusters = raw if isinstance(raw, list) else (raw.get("items") or [raw]) | |
| name = sys.argv[1] | |
| def cluster_name(c): | |
| if not isinstance(c, dict): | |
| return "" | |
| # Table "NAME" column is metadata.name; top-level name is often unset. | |
| meta = c.get("metadata") if isinstance(c.get("metadata"), dict) else {} | |
| return (meta.get("name") or c.get("name") or "") | |
| matches = [c.get("id", "") for c in clusters if cluster_name(c) == name and c.get("id")] | |
| print(matches[0] if matches else "") | |
| ' "{{ caas_cluster_name }}" | |
| environment: | |
| KUBECONFIG: /root/.kube/config | |
| register: _cluster_id | |
| changed_when: false | |
| failed_when: false | |
| # Fire-and-forget: async with no polling | |
| - name: Delete cluster | |
| - name: Report when named cluster is already absent | |
| ansible.builtin.debug: | |
| msg: "No osac cluster named '{{ caas_cluster_name }}' — skip osac delete" | |
| when: (_cluster_id.stdout | trim | length) == 0 | |
| - name: Resolve cluster ID for {{ caas_cluster_name }} | |
| ansible.builtin.shell: | | |
| set -euo pipefail | |
| osac get clusters -o json | python3 -c ' | |
| import json, sys | |
| raw = json.load(sys.stdin) | |
| clusters = raw if isinstance(raw, list) else (raw.get("items") or [raw]) | |
| name = sys.argv[1] | |
| def cluster_name(c): | |
| if not isinstance(c, dict): | |
| return "" | |
| # Table "NAME" column is metadata.name; top-level name is often unset. | |
| meta = c.get("metadata") if isinstance(c.get("metadata"), dict) else {} | |
| return (meta.get("name") or c.get("name") or "") | |
| matches = [c.get("id", "") for c in clusters if cluster_name(c) == name and c.get("id")] | |
| print(matches[0] if matches else "") | |
| ' "{{ caas_cluster_name }}" | |
| environment: | |
| KUBECONFIG: /root/.kube/config | |
| register: _cluster_id | |
| changed_when: false | |
| failed_when: false | |
| - name: Fail when cluster lookup did not complete | |
| ansible.builtin.fail: | |
| msg: >- | |
| Cluster lookup failed (rc={{ _cluster_id.rc }}): | |
| {{ _cluster_id.stderr | default('') | trim }} | |
| when: _cluster_id.rc != 0 | |
| - name: Report when named cluster is already absent | |
| ansible.builtin.debug: | |
| msg: "No osac cluster named '{{ caas_cluster_name }}' — skip osac delete" | |
| when: | |
| - _cluster_id.rc == 0 | |
| - (_cluster_id.stdout | trim | 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/destroy-caas/tasks/main.yml` around lines 41 - 69, Update
the cluster lookup task registered as _cluster_id to distinguish command or JSON
parsing failures from a successful lookup with no matching cluster. Do not allow
failed_when: false to make failures appear as empty stdout; explicitly fail or
otherwise propagate lookup errors, and only run “Report when named cluster is
already absent” when the lookup succeeded and returned no ID. Preserve the
existing skip-delete behavior only for a valid successful no-match result.
| ansible.builtin.shell: | | ||
| set -euo pipefail | ||
| ns="{{ _force_osac_ns }}" | ||
| { | ||
| oc get clusterorder -n "$ns" -o jsonpath='{range .items[*]}{.metadata.name}{"\n"}{end}' 2>/dev/null || true | ||
| oc get hostedcluster -A --no-headers 2>/dev/null | awk '{print $2}' | grep '^order-' || true | ||
| oc get ns -o jsonpath='{range .items[*]}{.metadata.name}{"\n"}{end}' 2>/dev/null \ | ||
| | while read -r n; do | ||
| case "$n" in | ||
| klusterlet-order-*) echo "order-${n#klusterlet-order-}" ;; | ||
| "${ns}"-order-*-order-*) | ||
| # HCP ns: osac-devel-order-xxx-order-xxx | ||
| if [[ "$n" =~ ${ns}-(order-[a-z0-9]+)-order- ]]; then | ||
| echo "${BASH_REMATCH[1]}" | ||
| fi | ||
| ;; | ||
| "${ns}"-order-*) echo "${n#"${ns}-"}" ;; | ||
| esac | ||
| done | ||
| # Ghost ns: object gone but pods/deploys/HCP CRs remain | ||
| { | ||
| oc get pods -A --no-headers 2>/dev/null || true | ||
| oc get deploy -A --no-headers 2>/dev/null || true | ||
| oc get hostedcontrolplanes.hypershift.openshift.io -A --no-headers 2>/dev/null || true | ||
| } | awk -v pfx="${ns}-" '$1 ~ pfx"order-" {print $1}' | sort -u \ | ||
| | while read -r n; do | ||
| if [[ "$n" =~ ${ns}-(order-[a-z0-9]+)-order- ]]; then | ||
| echo "${BASH_REMATCH[1]}" | ||
| elif [[ "$n" =~ ${ns}-(order-[a-z0-9]+)$ ]]; then | ||
| echo "${BASH_REMATCH[1]}" | ||
| fi | ||
| done | ||
| # Leftover apps LB services in OSAC ns (order-xxx-apps-wa-lb) | ||
| oc get svc -n "$ns" -o jsonpath='{range .items[*]}{.metadata.name}{"\n"}{end}' 2>/dev/null \ | ||
| | while read -r svc; do | ||
| if [[ "$svc" =~ ^(order-[a-z0-9]+) ]]; then | ||
| echo "${BASH_REMATCH[1]}" | ||
| fi | ||
| done | ||
| } | grep -E '^order-' | sort -u | ||
| environment: | ||
| KUBECONFIG: /root/.kube/config | ||
| register: _force_orders_raw | ||
| changed_when: false | ||
| failed_when: false |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether other roles in this repo pin bash explicitly for shell blocks.
rg -n -B2 -A2 'executable: /bin/bash' infra/netris
rg -n 'ansible_shell_type|ansible_shell_executable' infra/netrisRepository: osac-project/osac-test-infra
Length of output: 10940
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== force-destroy-caas outline =="
wc -l infra/netris/roles/force-destroy-caas/tasks/main.yml
sed -n '1,250p' infra/netris/roles/force-destroy-caas/tasks/main.yml
echo
echo "== bash-specific constructs in target file =="
python3 - <<'PY'
from pathlib import Path
p=Path('infra/netris/roles/force-destroy-caas/tasks/main.yml')
s=p.read_text()
for i,line in enumerate(s.splitlines(),1):
if any(x in line for x in ['[[ ', ' =~ ', 'BASH_REMATCH', 'set -o pipefail', 'set -euo pipefail', ' <(' ]):
print(f'{i}: {line}')
PY
echo
echo "== Ansible shell modules without explicit executable nearby in target file =="
python3 - <<'PY'
from pathlib import Path
p=Path('infra/netris/roles/force-destroy-caas/tasks/main.yml')
lines=p.read_text().splitlines()
blocks=[]
for i,line in enumerate(lines,1):
if 'ansible.builtin.shell:' in line:
# find task args block until next same-level key or end
indent=20
bstart=None
btext=''
for j in range(i, min(len(lines), i+60)+1):
l=lines[j-1]
if j==i:
bstart=j; btext=l+'\n'
else:
# same or lower indent of sibling keys; ignore lines under shell itself
if l and not l.startswith(' ') and (l.startswith('name:') or l.startswith('ansible.') or l.startswith('vars:') or l.startswith('when:')):
break
btext+=l+'\n'
blocks.append((bstart,btext))
for bstart,btext in blocks:
print(f'--- shell task starting at line {bstart} ---')
print(btext[:1200])
print('has executable:', 'executable:' in btext)
PYRepository: osac-project/osac-test-infra
Length of output: 20995
Use Bash for Bash-only shell tasks.
ansible.builtin.shell defaults to /bin/sh, but several tasks here use Bash constructs such as [[ ]], =~, BASH_REMATCH, and process substitution. When /bin/sh is not Bash, those tasks fail immediately and can leave force-destroy incomplete; failed_when: false can mask the failure. Add args: {executable: /bin/bash} to the Bash-dependent shell tasks, especially the order collection task at line 14, the per-order cleanup task at line 71, the VM wipe task at line 210, and the Agent cleanup task at line 232.
🤖 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/force-destroy-caas/tasks/main.yml` around lines 14 - 58,
The Bash-dependent shell tasks must explicitly run with Bash rather than the
default /bin/sh. Add args with executable set to /bin/bash to the order
collection task containing _force_orders_raw and the corresponding per-order
cleanup, VM wipe, and Agent cleanup tasks, preserving their existing commands
and failure behavior.
| - name: Wipe discovery worker VMs (detach disks/ISO + remove qcow2) | ||
| ansible.builtin.shell: | | ||
| set -euo pipefail | ||
| vm="{{ item }}" | ||
| if ! virsh dominfo "$vm" >/dev/null 2>&1; then | ||
| echo "VM $vm not found — skip" | ||
| exit 0 | ||
| fi | ||
| if virsh list --name 2>/dev/null | grep -qx "$vm"; then | ||
| virsh destroy "$vm" 2>/dev/null || true | ||
| fi | ||
| while read -r target; do | ||
| [[ -z "$target" ]] && continue | ||
| virsh change-media "$vm" "$target" --eject --config 2>/dev/null || true | ||
| virsh detach-disk "$vm" "$target" --config 2>/dev/null || true | ||
| done < <(virsh domblklist "$vm" 2>/dev/null | awk 'NR>2 && NF>=1 {print $1}') | ||
| rm -f "/var/lib/libvirt/images/${vm}-disk.qcow2" | ||
| echo "$vm powered off; disks/ISO detached; qcow2 removed" | ||
| loop: "{{ caas_discovery_vm_patterns }}" | ||
| failed_when: false | ||
| changed_when: true | ||
|
|
||
| - name: Delete any remaining Agent CRs for wiped VMs | ||
| ansible.builtin.shell: | | ||
| set -euo pipefail | ||
| vm="{{ item }}" | ||
| agent_ns="{{ _force_agent_ns }}" | ||
| agent=$(oc get agent -n "$agent_ns" -o json 2>/dev/null | python3 -c ' | ||
| import json, sys | ||
| vm = sys.argv[1] | ||
| data = json.load(sys.stdin) | ||
| for a in data.get("items") or []: | ||
| host = ((a.get("status") or {}).get("inventory") or {}).get("hostname") or "" | ||
| if host == vm: | ||
| print(a["metadata"]["name"]) | ||
| break | ||
| ' "$vm" || true) | ||
| if [[ -n "$agent" ]]; then | ||
| oc delete agent "$agent" -n "$agent_ns" --wait=false 2>/dev/null || true | ||
| echo "deleted agent $agent ($vm)" | ||
| fi | ||
| environment: | ||
| KUBECONFIG: /root/.kube/config | ||
| loop: "{{ caas_discovery_vm_patterns }}" | ||
| failed_when: false | ||
| changed_when: true |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
caas_discovery_vm_patterns holds patterns, but these tasks treat the items as exact names.
infra/netris/roles/destroy-caas/tasks/main.yml line 151 resolves the same variable with virsh list --all --name | grep "{{ item }}", which confirms the items are substrings. Here line 213 calls virsh dominfo "$vm" and line 225 removes /var/lib/libvirt/images/${vm}-disk.qcow2, and line 242 compares the agent inventory hostname with host == vm. If an item is a pattern such as caas-worker, dominfo fails, the task prints "VM not found — skip", and no disk, no ISO, and no Agent CR is removed. Force destroy then reports success with the VMs intact.
Resolve the domain names first, then loop over the resolved names.
🛠️ Proposed fix: resolve names before wiping
+- name: Resolve discovery VM domain names
+ ansible.builtin.shell: |
+ set -euo pipefail
+ virsh list --all --name | grep -E "{{ item }}" || true
+ args:
+ executable: /bin/bash
+ register: _force_vm_lookup
+ changed_when: false
+ failed_when: false
+ loop: "{{ caas_discovery_vm_patterns }}"
+
+- name: Build discovery VM name list
+ ansible.builtin.set_fact:
+ _force_vms: "{{ _force_vm_lookup.results | map(attribute='stdout_lines') | flatten | select('ne', '') | unique | list }}"
+
- name: Wipe discovery worker VMs (detach disks/ISO + remove qcow2)Then use loop: "{{ _force_vms }}" in both the wipe task and the Agent CR task.
🤖 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/force-destroy-caas/tasks/main.yml` around lines 209 - 254,
Resolve each entry in caas_discovery_vm_patterns to matching libvirt domain
names using the existing discovery approach, store the exact names in
_force_vms, and use loop: "{{ _force_vms }}" for both the “Wipe discovery worker
VMs” and “Delete any remaining Agent CRs” tasks. Keep dominfo, disk cleanup, and
agent hostname matching operating on each resolved VM name.
| - name: Resolve Netris password | ||
| ansible.builtin.shell: | | ||
| set -euo pipefail | ||
| ns="{{ _force_osac_ns }}" | ||
| pass=$(oc get secret cluster-fulfillment-ig -n "$ns" \ | ||
| -o jsonpath='{.data.NETRIS_PASSWORD}' 2>/dev/null | base64 -d 2>/dev/null || true) | ||
| printf '%s' "${pass:-netris}" | ||
| environment: | ||
| KUBECONFIG: /root/.kube/config | ||
| register: _netris_pass | ||
| changed_when: false | ||
| failed_when: false | ||
| no_log: true |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for other hardcoded netris credential defaults in the repo.
rg -n -C2 "netris_password|NETRIS_PASSWORD|:-netris" infra/netrisRepository: osac-project/osac-test-infra
Length of output: 13823
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the immediate control flow around the password resolution and consumers of _netris_pass.
sed -n '240,320p' infra/netris/roles/force-destroy-caas/tasks/main.yml | cat -n -v
printf '\n--- _netris_pass and netris_password usages ---\n'
rg -n "_netris_pass|netris_password|NETRIS_PASSWORD" infra/netris/roles/force-destroy-caas/tasks/main.ymlRepository: osac-project/osac-test-infra
Length of output: 3789
Broken Authentication (CWE-798): Use of Hard-coded Credentials
Remove the hardcoded Netris password fallback.
If cluster-fulfillment-ig lacks NETRIS_PASSWORD, this task passes the credential netris to the Netris cleanup path instead of failing. Fail the task with a clear message so the operator sees the missing secret before Netris authentication proceeds.
🛠️ Proposed fix
- printf '%s' "${pass:-netris}"
+ if [[ -z "$pass" ]]; then
+ echo "NETRIS_PASSWORD not found in cluster-fulfillment-ig" >&2
+ exit 1
+ fi
+ printf '%s' "$pass"🤖 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/force-destroy-caas/tasks/main.yml` around lines 274 - 286,
Update the “Resolve Netris password” task to remove the `${pass:-netris}`
fallback and fail when the secret does not provide NETRIS_PASSWORD. Preserve
successful password resolution, but make the task report a clear missing-secret
error before the Netris cleanup path proceeds, using the existing _netris_pass
result and task failure controls.
Source: Coding guidelines
| - name: Launch osac-config-as-code (apply pin-driven Controller config) | ||
| ansible.builtin.uri: | ||
| url: "https://{{ _aap_pin_route.stdout }}/api/controller/v2/job_templates/osac-config-as-code/launch/" | ||
| method: POST | ||
| user: admin | ||
| password: "{{ _aap_pin_admin_pw.stdout }}" | ||
| force_basic_auth: true | ||
| validate_certs: false | ||
| status_code: [201] | ||
| register: _aap_cac_job | ||
| retries: 5 | ||
| delay: 20 | ||
| until: _aap_cac_job.status == 201 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
: "${CONTROLLER_URL:?Set the Controller API base URL}"
: "${CONTROLLER_OAUTH_TOKEN:?Set a read-only Controller OAuth token}"
curl --fail --silent --show-error \
-H "Authorization: Bearer ${CONTROLLER_OAUTH_TOKEN}" \
"${CONTROLLER_URL}/api/controller/v2/job_templates/?name=osac-config-as-code" |
jq '{count, results: [.results[] | {id, name, named_url}]}'Repository: osac-project/osac-test-infra
Length of output: 228
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the file and inspect the relevant task plus nearby variable setup.
fd -a 'sync-aap-project-pin.yml' .
file="$(fd 'sync-aap-project-pin.yml' . | head -n1)"
echo "FILE=${file}"
wc -l "$file"
sed -n '220,275p' "$file"
echo
echo "References to _aap_pin_route/_aap_pin_admin_pw/_aap_cac_job:"
rg -n "_aap_pin_route|_aap_pin_admin_pw|_aap_cac_job|osac-config-as-code|controller/v2/job_templates" "$file"Repository: osac-project/osac-test-infra
Length of output: 4484
🌐 Web query:
Ansible Automation Platform Controller API job templates launch URL named_url id launch documentation
💡 Result:
To launch a job template in the Ansible Automation Platform (AAP) Controller API, you must send an HTTP POST request to the launch endpoint associated with the specific job template [1][2]. The endpoint structure typically follows the pattern: /api/controller/v2/job_templates//launch/ (Note: Older versions or certain configurations may use /api/v2/job_templates//launch/ [3][4][5].) Key details for launching a job template via the API: 1. Method: HTTP POST [1][2]. 2. Authentication: The request requires valid authentication, such as an OAuth2 Bearer token [1][2]. 3. Request Body: You can optionally provide a JSON object in the request body to override template defaults, such as extra_vars, inventory, credentials, limit, or tags [6][2][5]. 4. Named URLs: The platform supports "named URLs," which allow you to reference resources by name rather than numeric ID [1]. For job templates, the format is generally ++<organization.name>. For example, you might use: /api/controller/v2/job_templates/my_template_name++my_org_name/launch/ [5]. Example using cURL: curl -k -X POST \ -H "Authorization: Bearer " \ -H "Content-Type: application/json" \ -d '{"extra_vars": {"key": "value"}}' \ https:///api/controller/v2/job_templates//launch/ [1][2]. If the launch is successful, the API returns a status code of 201 (Created) along with the details of the newly created job, including its unique job ID [7][6][2]. For bulk operations, AAP also provides a specific endpoint at /api/v2/bulk/job_launch [8][9].
Citations:
- 1: https://docs.redhat.com/en/documentation/red_hat_ansible_automation_platform/2.5/html-single/automation_execution_api_overview/index
- 2: https://www.ansiblepilot.com/articles/aap-2-6-rest-api-guide-automate-the-automation-platform
- 3: https://stackoverflow.com/questions/72492328/how-do-i-call-ansible-tower-rest-api-endpoint-to-launch-job-template-for-a-job-t
- 4: https://debugthis.dev/posts/awx/2020-02-28-ansible-awx-launching-a-job-template/
- 5: https://www.ansible-guru.nl/trigger_job_template_from_curl/
- 6: https://raw.githubusercontent.com/api-evangelist/red-hat/refs/heads/main/openapi/red-hat-ansible-automation-platform-openapi.yml
- 7: https://apis.io/apis/ansible-automation-platform/ansible-automation-platform-job-templates-api/
- 8: https://docs.redhat.com/en/documentation/red_hat_ansible_automation_platform/2.6/develop-proc_controller_launch_job_template
- 9: https://docs.redhat.com/en/documentation/red_hat_ansible_automation_platform/2.4/html/automation_controller_user_guide/controller-job-templates
🌐 Web query:
Ansible AWX Tower Controller job_templates [name] launch URI named URL name only
💡 Result:
To launch an Ansible AWX/Automation Platform Controller Job Template using its name instead of its numeric ID, you can use the API's Named URL feature [1]. The standard API endpoint for launching a job template is: /api/v2/job_templates/[ID]/launch/ [2][3][4] To use the name of the template, you must use the Named URL format, which is: /api/v2/job_templates/[JOB_NAME]++[ORGANIZATION_NAME]/launch/ [1] Key implementation details: 1. Format: The name must be combined with the organization name using a double-plus (++) separator [1]. 2. Method: You must perform a POST request to this URI to trigger the launch [5][1]. 3. Authentication: Ensure you provide valid credentials (e.g., Personal Access Token or basic auth) in the request headers [5][1][6]. 4. Payload: If the job template requires extra variables, inventory, or other parameters, include them in the JSON body of the POST request [5][3][7]. Example using curl: curl -X POST \ --url https://[AWX_HOST]/api/v2/job_templates/[JOB_NAME]++[ORG_NAME]/launch/ \ --header 'Authorization: Bearer [YOUR_TOKEN]' \ --header 'Content-Type: application/json' \ --data '{"extra_vars": {"key": "value"}}' [5][1] If you are using Ansible playbooks, the native awx.awx.job_launch module handles this lookup for you automatically when you specify the job_template name, avoiding the need to manually construct the URI [8][9].
Citations:
- 1: https://www.linkedin.com/pulse/calling-ansible-automation-platform-api-using-job-names-griffiths
- 2: Tower rest api job_templates launch endpoint is not working ansible/awx#12319
- 3: https://github.com/ansible/awx/blob/72a1922a/docs/prompting.md
- 4: https://www.dbi-services.com/blog/how-to-use-awx-rest-api-to-execute-jobs/
- 5: https://www.andthenyoudie.fr/posts/launch-awx_tower-job-by-name/
- 6: https://radeksprta.eu/posts/start-awx-job-via-api/
- 7: https://stackoverflow.com/questions/72492328/how-do-i-call-ansible-tower-rest-api-endpoint-to-launch-job-template-for-a-job-t
- 8: https://docs.ansible.com/projects/ansible/latest/collections/awx/awx/job_launch_module.html
- 9: https://ansible-awx.mintlify.app/guides/running-jobs
🌐 Web query:
Controller API job templates name field named_url API launch JSON field name
💡 Result:
In the Ansible Automation Platform (AAP) and AWX Controller API, the named_url field provides a human-readable, unique identifier for resources, including job templates [1][2]. Named URL Structure: The format for job templates is defined as ++<organization.name> [1][2][3]. This structure allows for multiple job templates with the same name to coexist, provided they belong to different organizations [2]. While the newer named URL format includes the organization name, the legacy scheme using only the job template is often still supported for backward compatibility, though it may return the oldest matching template if duplicates exist [2]. API Usage: 1. Identifying Named URLs: The named_url field is available as a read-only property in the detailed view of a resource (e.g., /api/v2/job_templates/{id}/) [1][3][4]. You can use this field to construct API requests by replacing the numeric primary key (PK) with the encoded named URL string [1][4][5]. 2. Launching Jobs: To launch a job template, you perform a POST request to the /api/v2/job_templates/{id_or_named_url}/launch/ endpoint [6][7]. 3. Launch JSON Fields: When launching a job, the JSON payload may include various fields for prompting, such as extra_vars (as a JSON dictionary), limit, credentials (as a list of IDs), and instance_groups [6][5]. Only fields that have "Prompt on Launch" enabled on the job template can be successfully overridden via this API call [8][6][5]. Because the named URL includes special characters like "+" and potentially "/", ensure that these characters are correctly URL-encoded when used in an API path (e.g., using %2B for "+" and %2F for "/") [5].
Citations:
- 1: https://docs.redhat.com/en/documentation/red_hat_ansible_automation_platform/2.5/html/automation_execution_api_overview/controller-api-access-resources
- 2: https://github.com/ansible/awx/blob/devel/docs/named_url.md
- 3: https://docs.redhat.com/en/documentation/red_hat_ansible_automation_platform/2.6/develop-assembly_controller_api_browsing_api
- 4: Give job templates (and other models) a slug ansible/awx#1035
- 5: http://www.automate.nyc/ansible/launch-job-via-rest-api/
- 6: https://github.com/ansible/awx/blob/72a1922a/docs/prompting.md
- 7: https://debugthis.dev/posts/awx/2020-03-31-ansible-awx-using-python-to-launch-a-job-template/
- 8: https://docs.redhat.com/en/documentation/red_hat_ansible_automation_platform/2.6/develop-proc_controller_launch_job_template
🌐 Web query:
AWX named_url validation accepts only name for job_templates without double plus old API
💡 Result:
In AWX, the named URL for job templates is defined by the format <name>++<organization.name> [1][2][3]. While the standard format requires the double-plus (++) separator to uniquely identify a job template within an organization, AWX maintains backwards compatibility for older API usage [1][4]. Consequently, you can still reference job templates using only the <name> scheme (i.e., /api/v2/job_templates/<name>/) [1][4]. When the older <name> scheme is used, if multiple job templates exist with the same name, AWX returns the oldest one [1]. The formal move to the <name>++<organization.name> format was implemented to support environments where multiple job templates share the same name but belong to different organizations, as uniqueness constraints were loosened from just the name field to the (organization, name) pair [1][4]. Internal API logic handles the resolution of these URL formats to the underlying database primary key [5][6].
Citations:
- 1: https://github.com/ansible/awx/blob/devel/docs/named_url.md
- 2: https://docs.ansible.com/projects/awx/en/24.6.1/rest%5Fapi/access%5Fresources.html
- 3: https://docs.redhat.com/en/documentation/red_hat_ansible_automation_platform/2.7/develop-assembly_controller_api_browsing_api
- 4: Loosen uniqueness on job templates ansible/awx#4008
- 5: https://github.com/ansible/awx/blob/72a1922a/awx/main/tests/functional/test_named_url.py
- 6: https://github.com/ansible/awx/blob/72a1922a/awx/api/serializers.py
🌐 Web query:
Red Hat AAP Controller API named_url /api/controller/v2/job_templates [name] valid old name only
💡 Result:
In the Red Hat Ansible Automation Platform (AAP) Controller API, the named URL feature allows you to access resources using human-readable identifiers rather than strictly relying on numeric primary keys [1][2]. For job templates, the named URL format is defined as ++<organization.name> [1][3]. This format is generated based on the current name and the organization name at the time of the request [1][4]. If a job template is renamed, its named URL will update to reflect the new name, as the named URL is a derived identifier [1][4]. Consequently, using an old, outdated name in the named URL path will result in a 404 Not Found error because the API expects the identifier to match the current, valid name of the resource [1][2]. To reliably access a job template, you should use the named_url field provided in the resource's detail view, which displays the accurately generated, current named URL for that specific object [1][5]. Resources that support named URLs are listed in the NAMED_URL_FORMATS configuration setting, which can be inspected at /api/controller/v2/settings/named-url/ [1][4].
Citations:
- 1: https://docs.redhat.com/en/documentation/red_hat_ansible_automation_platform/2.6/develop-assembly_controller_api_browsing_api
- 2: https://docs.redhat.com/en/documentation/red_hat_ansible_automation_platform/2.5/html-single/automation_execution_api_overview/index
- 3: https://docs.redhat.com/en/documentation/red_hat_ansible_automation_platform/2.5/pdf/automation_execution_api_overview/Red_Hat_Ansible_Automation_Platform-2.5-Automation_execution_API_overview-en-US.pdf
- 4: https://docs.redhat.com/en/documentation/red_hat_ansible_automation_platform/2.5/html/automation_execution_api_overview/controller-api-access-resources
- 5: https://docs.redhat.com/en/documentation/red_hat_ansible_automation_platform/2.7/develop-assembly_controller_api_browsing_api
Query osac-config-as-code before launching.
Controller launch paths should use the numeric job-template ID or the full name++organization named URL. osac-config-as-code alone may resolve ambiguously or fail when multiple templates share the name; add a filtered job-template lookup before this POST and build the launch URL from the required template.
🧰 Tools
🪛 Checkov (3.3.9)
[medium] 247-262: 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/patch-osac-refresh/tasks/sync-aap-project-pin.yml` around
lines 247 - 259, Before the POST task, query the Controller job-template API for
the job template named osac-config-as-code, filtering to the intended
organization and ensuring exactly the required template is selected. Store its
numeric ID or full name++organization URL identifier, then update the
_aap_cac_job launch request to use that resolved identifier instead of the
ambiguous name-only path.
| name = os.environ["NETRIS_RC_NAME"] | ||
| if name in data and isinstance(data[name], dict) and data[name]: | ||
| print(f"unchanged {name} already in NETRIS_RESOURCE_CLASS_MAP") | ||
| sys.exit(0) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Validate the existing host-type entry before retaining it.
Line 267 accepts any nonempty dictionary as valid. For example, {"g5":{"mgmt_interface":"ens4"}} skips the patch even though it lacks server_cluster_template_id and vpc_interfaces. Validate the required fields and replace malformed entries before cluster-fulfillment-ig consumes the map.
🤖 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/setup-caas/tasks/main.yml` around lines 266 - 269, Update
the existing-entry validation around NETRIS_RESOURCE_CLASS_MAP so the unchanged
path only accepts an entry containing valid server_cluster_template_id and
vpc_interfaces fields. Treat dictionaries missing either required field or
containing malformed values as invalid, then replace them with the generated
host-type entry before cluster-fulfillment-ig consumes the map; retain the
current early exit only for fully valid entries.
eliorerz
left a comment
There was a problem hiding this comment.
Please add a proper commit and PR description
|
/retest |
|
Re-triggered failed runs:
|
🚨 Credential leak scan found secrets in e2e run logs/artifactsWorkflow: E2E VMaaS 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).
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
infra/netris/roles/discover-caas/tasks/main.yml (1)
205-206: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRead vCPU and memory maximums separately before applying overrides.
virsh vcpucountonly has--maximumhere, but later tasks reuse_current_vcpusforvirsh setvcpus --config, which changes the current configured vCPU count, not the maximum. If the target exceeds the old maximum, this should change the maximum first.
virsh dommemstat actualreports a dynamic balloon/pressure metric, not the configured memory maximum. Use the configured maximum forvirsh setmaxmem --config; use current configured memory only forvirsh setmem --configif that is intended.🤖 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/discover-caas/tasks/main.yml` around lines 205 - 206, Update the discovery tasks around the vCPU and memory override flows to read configured maximums separately from current configured values. Use the maximum vCPU value for the initial virsh setvcpus --maximum update before applying any configured-current change, and replace dommemstat actual with the configured memory maximum for setmaxmem --config; retain current configured memory only for the setmem --config step if needed.
🤖 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/discover-caas/tasks/main.yml`:
- Around line 230-250: Update the final debug task to report the resolved per-VM
vCPU and memory targets using the same override maps and defaults applied by the
resource-setting tasks, rather than only caas_discovery_vcpu and
caas_discovery_memory_mb. Ensure each VM’s message reflects its effective
values, including overrides for h02 and h03.
---
Outside diff comments:
In `@infra/netris/roles/discover-caas/tasks/main.yml`:
- Around line 205-206: Update the discovery tasks around the vCPU and memory
override flows to read configured maximums separately from current configured
values. Use the maximum vCPU value for the initial virsh setvcpus --maximum
update before applying any configured-current change, and replace dommemstat
actual with the configured memory maximum for setmaxmem --config; retain current
configured memory only for the setmem --config step if needed.
🪄 Autofix
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: CHILL
Plan: Pro Plus
Run ID: bd738401-0dfb-4205-b552-956d8d03aa00
📒 Files selected for processing (6)
infra/netris/CLAUDE.mdinfra/netris/Makefileinfra/netris/README.mdinfra/netris/inventory/group_vars/all.ymlinfra/netris/roles/discover-caas/tasks/main.ymlinfra/netris/roles/setup-caas/tasks/main.yml
🚧 Files skipped from review as they are similar to previous changes (5)
- infra/netris/inventory/group_vars/all.yml
- infra/netris/CLAUDE.md
- infra/netris/README.md
- infra/netris/roles/setup-caas/tasks/main.yml
- infra/netris/Makefile
| cmd: >- | ||
| virsh setmaxmem {{ item.item }} | ||
| {{ caas_discovery_memory_mb_overrides[item.item] | default(caas_discovery_memory_mb) }}M | ||
| --config | ||
| when: >- | ||
| item.stdout | int != | ||
| (caas_discovery_memory_mb_overrides[item.item] | default(caas_discovery_memory_mb)) | int | ||
| loop: "{{ _current_memory.results }}" | ||
| loop_control: | ||
| label: "{{ item.item }}" | ||
| changed_when: true | ||
|
|
||
| - name: Set memory | ||
| ansible.builtin.command: | ||
| cmd: virsh setmem {{ item.item }} {{ caas_discovery_memory_mb }}M --config | ||
| when: item.stdout | int != caas_discovery_memory_mb | int | ||
| cmd: >- | ||
| virsh setmem {{ item.item }} | ||
| {{ caas_discovery_memory_mb_overrides[item.item] | default(caas_discovery_memory_mb) }}M | ||
| --config | ||
| when: >- | ||
| item.stdout | int != | ||
| (caas_discovery_memory_mb_overrides[item.item] | default(caas_discovery_memory_mb)) | int |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Report the resolved per-VM values.
The final debug task at Lines [324]-[325] still prints caas_discovery_vcpu and caas_discovery_memory_mb. With per-VM overrides, the message can report default values while h02 and h03 use larger targets. Print the resolved values for each VM, or print the override maps.
🤖 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/discover-caas/tasks/main.yml` around lines 230 - 250,
Update the final debug task to report the resolved per-VM vCPU and memory
targets using the same override maps and defaults applied by the
resource-setting tasks, rather than only caas_discovery_vcpu and
caas_discovery_memory_mb. Ensure each VM’s message reflects its effective
values, including overrides for h02 and h03.
🚨 Credential leak scan found secrets in e2e run logs/artifactsWorkflow: E2E BMaaS 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).
|
🚨 Credential leak scan found secrets in e2e run logs/artifactsWorkflow: E2E VMaaS 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).
|
🚨 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).
|
MaaS suite (SUITE / setup-maas / disk grow / g5 labeling / resource map / cluster params), CaaS reliability (agent wait, HCP PSS, AAP pin sync, disk-setup, VPN host-key, destroy/force-destroy), and docs. Signed-off-by: Sergio Ruiz <sruiz@redhat.com> Assisted-by: Cursor Co-authored-by: Cursor <cursoragent@cursor.com>
Keep h01 at 16GiB when growing h02/h03 for real-model headroom, and publish catalog items with ClusterTemplateReference id (OSAC-1330). Signed-off-by: Sergio Ruiz <sruiz@redhat.com> Assisted-by: Cursor Co-authored-by: Cursor <cursoragent@cursor.com>
SNAT softgate pairing did not uniquely fix the SoftGate VRF issue; keep Dan's general/general/general/general default. Signed-off-by: Sergio Ruiz <sruiz@redhat.com> Assisted-by: Cursor Co-authored-by: Cursor <cursoragent@cursor.com>
5377a9b to
c726cdd
Compare
The PR and commit descriptions have been updated
Remove trailing spaces on the MaaS workflow line so pre-commit trim-trailing-whitespace passes in CI. Signed-off-by: Sergio Ruiz <sruiz@redhat.com> Assisted-by: Cursor Co-authored-by: Cursor <cursoragent@cursor.com>
|
@sruiz-rh: This pull request references OSAC-3616 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.1.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: danmanor, sruiz-rh 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 |
Summary
Netris Spectrum-X lab infra for Demo 7 MaaS, plus shared CaaS reliability fixes and destroy / force-destroy so labs can be cleaned and re-run.
Tracking: OSAC-3616 (parent epic OSAC-1862). Full need/fix narrative lives on the ticket; this PR ships the
infra/netrischanges onfeat/maas-and-fixes.A. MaaS deployment suite
SUITE=caas|maason redeploy (Makefile,redeploy-server.sh,restore-ocp);OSAC_DEPLOY_MODE=fresh|snapshotfor OSAC install pathmake setup-maas/deploy-maas/destroy-maas/force-destroy-maaswith Demo 7 defaults (maas-cluster,g5,ocp_4_20_ai_maas, enable flags, discovery sizing)ocp_snapshot_disk_gb: 150) for MaaS/AI image pressure on the management clusterresourceClass=g5labeling (caas_resource_class_hostnames), NetrisNETRIS_RESOURCE_CLASS_MAPmerge forg5, andcaas_cluster_paramsfor template create-pflagsB. CaaS / shared reliability
osac get clustersparsing; HCP PodSecurity privileged labels; honest Progressing/Failed reporting after createosac-test-infra; safer LVM-rootdisk-setupsync-aap-project-pin.yml) soocp_4_20_ai_maasis publishable after restoregeneral/general/general/general(triedgeneral/general/snat/snat; SoftGate/VRF failures also occur with SNAT pairing — not a unique fix)metadata.name+template: { id: … }(ObjectMeta + OSAC-1330 typed reference)C. Destroy / force-destroy (+ MaaS)
destroy-caas/force-destroy-caas(+ MaaS wrappers): osac login + delete, agent/InfraEnv cleanup, leftover HC/ClusterOrder/namespace wipe, Netris orphan sweep (OSAC-1873; merged with ideas from NO-ISSUE: bare-metal lab tooling for persistent RHEL servers #267)destroy-caasreliability: probe live OSAC namespace (don’t trust snapshot default), log in beforeosac get/delete, distinguish lookup failures from “cluster absent”, poll until cluster teardown completesD. Rebase / CI hygiene
main; Makefile/README conflict resolution keeps upstreamdestroy-setup,force-destroy-caas, and BMaaS targets (setup-bmaas,destroy-bmaas,gather-bmaas) while adding MaaS wrappers in a dedicated Makefile section below BMaaSinfra/netris/CLAUDE.mdCommits
OSAC-3616: Netris MaaS/CaaS infra feats & fixes— bulk MaaS suite + CaaS reliability + destroyOSAC-3616: Add per-VM memory overrides for MaaS discovery— memory overrides + catalogtemplate.idOSAC-3616: Restore all-general softgate roles— keepgeneral×4after SNAT pairing did not uniquely fix SoftGate/VRFOSAC-3616: Fix trailing whitespace in infra/netris/CLAUDE.md— satisfy pre-committrim-trailing-whitespaceon the MaaS workflow doc lineTest plan
make redeploy-fresh(defaultSUITE=caas) → lab → OCP → OSAC → setup-caas → deploy-caas (no inventory KeyError / missingg5map / restricted HCP PodSecurity)make redeploy-fresh SUITE=maasgrows h00 disk as needed, runs setup-maas/deploy-maas withocp_4_20_ai_maas+ enable flags, labels only h02/h03 withg5. Depends on not-yet-merged osac#122 (BM MaaS /ocp_4_20_ai_maastemplate fixes)deploy-osac, AAP Project matchesconfig-as-code-ig, CaC succeeds; mono-repo pins have JT playbooks underosac-aap/;ocp_4_20_ai_maaspublishablefacebook-opt-125m-simulated/facebook/opt-125m), create subscriptions/auth as needed, and validate inference — those post-cluster steps are manual ops, not covered by this Ansiblemake destroy-caas/force-destroy-caas(and-maas) clear leftovers for a clean next redeploy. Force-destroy is required while MaaS leave-behinds persist (e.g. OSAC-3737) and for older CaaS cleanup classes already addressed in OSAC-2025make disk-setupon LVM-root BM selects data disk (not OS) and mounts/mnt/datageneral/general/general/generalin in-tree netris-lab varsmetadata.name+template.idMAAS_DISCOVERY_MEMORY_MB(default 20480; override e.g. 49152 for real-model headroom)pre-commit run --all-filespasses (trailing whitespace ininfra/netris/CLAUDE.md)Assisted-by: Cursor