OCPEDGE-2498: Add migration support#83
Open
eggfoobar wants to merge 5 commits into
Open
Conversation
|
@eggfoobar: This pull request references OCPEDGE-2498 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 spike 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. |
openshift-ci-robot
added
the
jira/valid-reference
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: eggfoobar The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
Contributor
|
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:
WalkthroughThis PR adds an Ansible workflow to transition an OpenShift single-node cluster to three nodes, including entrypoints, VM provisioning, ignition generation, topology updates, readiness waits, DNS configuration, verification, and guarded cleanup. ChangesSNO to 3-node mutable topology
Estimated code review effort: 4 (Complex) | ~75 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5 | ❌ 6❌ Failed checks (6 inconclusive)
✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
deploy/Makefile (1)
122-130: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDocument the new
sno-ipiandsno-agenttargets inmake help.Lines 79-83 add both commands, but this help block only advertises
sno-to-3node.make helpwill miss the new entrypoints 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 `@deploy/Makefile` around lines 122 - 130, Update the Makefile help output so the new `sno-ipi` and `sno-agent` targets are listed alongside the existing deployment entries in `make help`. Add matching `@echo` lines in the help block near the `sno-to-3node` entry, using the same wording/style as the other target descriptions so the new entrypoints are discoverable.deploy/openshift-clusters/scripts/deploy-cluster.sh (1)
62-69: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReject the unsupported
sno + kclicombination explicitly.This validation now allows
--topology snofor every method, so a directdeploy-cluster.sh --topology sno --method kclifalls through tokcli-install.yml. The PR only adds SNO support through the install-dev/setup flow, so this opens an unsupported path instead of failing fast.Suggested fix
if [[ "${METHOD}" != "ipi" && "${METHOD}" != "agent" && "${METHOD}" != "kcli" ]]; then echo "Error: Invalid method '${METHOD}'. Must be 'ipi', 'agent', or 'kcli'." exit 1 fi + +if [[ "${TOPOLOGY}" == "sno" && "${METHOD}" == "kcli" ]]; then + echo "Error: Topology 'sno' only supports 'ipi' or 'agent'." + exit 1 +fiAlso applies to: 87-104
🤖 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 `@deploy/openshift-clusters/scripts/deploy-cluster.sh` around lines 62 - 69, The validation in deploy-cluster.sh currently permits every METHOD with TOPOLOGY=sno, which allows an unsupported sno + kcli path to continue into the kcli install flow. Update the argument checks around the TOPOLOGY/METHOD validation blocks to explicitly reject the unsupported combination before any install logic runs, and keep the failure consistent with the existing validation messages. Reference the validation section and the kcli-install path so the guard is placed where the topology and method are first checked.deploy/openshift-clusters/roles/dev-scripts/install-dev/tasks/config.yml (1)
26-32: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winInclude the SNO scenario in the missing-variable error.
Line 31 still only tells agent users to add
TNF_IPV4orTNA_IPV4. Now thatsnois a supported prefix on Line 43, a missingAGENT_E2E_TEST_SCENARIOfor SNO will produce the wrong remediation.Proposed fix
- AGENT_E2E_TEST_SCENARIO="TNF_IPV4" (fencing) or "TNA_IPV4" (arbiter) to - the config file. + AGENT_E2E_TEST_SCENARIO="TNF_IPV4" (fencing), "TNA_IPV4" (arbiter), or + "SNO_IPV4" (sno) to the config file.Also applies to: 43-43
🤖 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 `@deploy/openshift-clusters/roles/dev-scripts/install-dev/tasks/config.yml` around lines 26 - 32, The missing-AGENT_E2E_TEST_SCENARIO failure message in the config validation task should be updated to include the SNO option now supported by the method-prefix logic. Adjust the fail message in the config validation flow so it lists the correct remediation for all supported prefixes, including the new sno case alongside the existing agent scenarios, and make sure the guidance matches the prefix handling used in the task that selects by method.
🧹 Nitpick comments (2)
deploy/openshift-clusters/scripts/sno-to-3node.sh (1)
1-2: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the repository-standard Bash shebang.
This new wrapper still uses
#!/bin/bash, but the repo shell-script rule requires#!/usr/bin/bash. As per coding guidelines, "Shell scripts should use#!/usr/bin/bashshebang".🤖 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 `@deploy/openshift-clusters/scripts/sno-to-3node.sh` around lines 1 - 2, The wrapper script currently uses the non-standard Bash shebang, so update the script header to match the repository convention by changing the shebang at the top of sno-to-3node.sh to the repo-standard Bash path. Keep the rest of the script unchanged, and verify the file still starts with the expected shebang followed by set -euo pipefail.Source: Coding guidelines
deploy/openshift-clusters/scripts/clean-mutable-topology.sh (1)
1-2: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the repository-standard Bash shebang.
This new wrapper still uses
#!/bin/bash, but the repo shell-script rule requires#!/usr/bin/bash. As per coding guidelines, "Shell scripts should use#!/usr/bin/bashshebang".🤖 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 `@deploy/openshift-clusters/scripts/clean-mutable-topology.sh` around lines 1 - 2, The clean-mutable-topology.sh wrapper uses the wrong Bash shebang, so update the script header to match the repository standard. Replace the current shebang at the top of the file with the repo-required Bash interpreter line, keeping the rest of the script unchanged; this is the only fix needed in the script’s startup header.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 `@deploy/openshift-clusters/clean-mutable-topology.yml`:
- Around line 35-39: The cleanup playbook currently imports
mutable-topology/sno-to-3node tasks/clean.yml unconditionally, which can destroy
a healthy HA control plane. Add a topology check in clean-mutable-topology.yml
before the import_role so it only runs when the cluster is not already HA, or
require an explicit force_cleanup=true override to proceed. Use the existing
import_role task as the entry point and gate it with a condition or precheck
that prevents destructive cleanup unless the override is set.
In
`@deploy/openshift-clusters/roles/dev-scripts/install-dev/files/config_sno_example.sh`:
- Around line 35-40: This example hardcodes an x86_64 release image and leaves
out the ARM64-specific Metal3 overrides, so update the config example to
explicitly document how to switch to aarch64/Graviton. Use
OPENSHIFT_RELEASE_IMAGE as the payload architecture selector and add the
required Metal3 container override guidance (IRONIC_IMAGE, VBMC_IMAGE,
SUSHY_TOOLS_IMAGE) in the example so the install path is correct for ARM64
hypervisors. Refer to the config_sno_example.sh example block and keep the
existing installer/image-policy comments consistent with the new ARM64 notes.
In
`@deploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/clean.yml`:
- Around line 68-72: The cleanup task in clean.yml is deleting the original SNO
node’s api-int DNS entry, which can break the surviving cluster after cleanup.
Update the [clean] Remove api-int from libvirt network DNS task to target only
mutable-topology-added resources associated with sno_new_nodes, and avoid
removing the api-int record for sno_master0_ip in the libvirt network update
logic.
- Around line 37-43: The embedded Python in the cleanup task scripts is
indented, which causes the inline `python3 -c` snippets used in `clean.yml` to
fail before the DHCP cleanup logic runs. Update the scripts in the task blocks
that use `EXISTING_MAC` and the lease cleanup command so the Python content is
left-aligned or otherwise dedented, and keep the logic under the same task names
so `|| true` / `failed_when: false` no longer hides a parse failure.
- Around line 26-30: The per-node ISO cleanup in the [clean] task is using a
hostname-based filename that can collide across clusters on the same hypervisor.
Update the ISO naming scheme in the related ISO creation and cleanup flow to use
a cluster-scoped identifier, such as item.name or a sno_cluster_name-prefixed
name, and make the file path in the cleanup task match that same unique pattern.
In
`@deploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/create-vms.yml`:
- Around line 27-32: The DNS cleanup in the `create-vms.yml` task is still
targeting `{{ item.ip }}`, so stale `dns-host` records for `master-1`/`master-2`
with old IPs are not removed before `virsh net-update add dns-host` runs. Update
the cleanup logic around the `virsh net-update` calls to delete entries by
hostname instead of by IP, and then tighten the `dig` verification so it checks
that the resolved answers actually contain `{{ item.ip }}` rather than only a
non-empty response.
In
`@deploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/ignition.yml`:
- Around line 19-24: The MCS reachability check in the ignition task is
currently allowed to fail and continue, which defers a required configuration
error until boot. Update the existing "[ignition] Verify MCS reachability" step
so it aborts when the curl probe to the MCS health endpoint fails, and only keep
a non-failing path if you explicitly add a break-glass override. Use the
existing mcs_health probe logic as the place to enforce fail-fast behavior.
- Around line 13-18: The ignition generation step writes /tmp/master.ign but
never populates sno_master_ign_b64, so auto-install.ign.j2 cannot render
reliably. Update the relevant task flow in ignition.yml to read back the
generated master.ign and set sno_master_ign_b64 before the template that
consumes it, using the existing ignition generation and auto-install rendering
steps as the anchor points. Ensure the variable is produced within the role
rather than assuming an external caller supplies it.
In
`@deploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/mco-rollout.yml`:
- Around line 68-81: The MCO rollout polling in the mco-rollout task currently
suppresses timeout failures by using failed_when: false, which allows the play
to continue even if mcp_poll never reports completion. Update the
polling/verification logic in the mcp_poll task so a timeout is fatal by
default, or make the non-fatal continuation explicitly opt-in via a best-effort
mode. Keep the success check based on mcp_poll.stdout and ensure the final MCO
rollout result step reflects failure when the rollout does not complete.
In
`@deploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/preflight.yml`:
- Around line 80-120: The preflight checks in the etcd probe and the
cluster-operator health check are only logging warnings instead of stopping the
topology transition, which makes this phase non-gating. Update the preflight
logic so the tasks for the etcd pod check and the degraded CO check fail the
play by default, removing the unconditional `|| true` and the `failed_when:
false` behavior; keep any bypass only behind an explicit break-glass override.
Use the existing task names like "[preflight] Check etcd pods" and "[preflight]
Check for degraded cluster operators" to locate the affected steps.
- Around line 129-154: The preflight repair flow for api-int only deletes
entries that already match sno_master0_ip and the verification step only checks
for any DNS answer, so stale mappings can remain undetected. Update the shell
logic in the api-int repair task to remove existing api-int entries by
hostname/FQDN regardless of IP, then make the dns_apiint_verify check confirm
the resolved result includes sno_master0_ip rather than just being non-empty.
Use the existing preflight tasks and variables like sno_libvirt_network,
sno_master0_ip, and sno_cluster_domain to locate and adjust the repair and
verification steps.
In
`@deploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/update-dns.yml`:
- Around line 24-31: The dnsmasq config in the update-dns task is incorrectly
pinning the wildcard apps record to sno_all_node_ips[0], which can vary because
node ordering is not stable. Update the logic in the sno_dnsmasq_lines
generation to use a stable ingress target such as a VIP/load balancer, or
otherwise emit all intended ingress node IPs instead of selecting the first node
IP. Make the change in the set_fact block that builds the DNS entries so the
*.apps record does not depend on array order.
In
`@deploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/wait-etcd.yml`:
- Around line 28-48: The wait-etcd task only checks for 3 Ready pods, while the
etcd membership and endpoint health probes in the same flow still suppress
failures via failed_when: false and fallback echo text. Update the wait logic in
wait-etcd.yml so the shell checks using oc exec and etcdctl member list /
endpoint health are allowed to fail the task, or fold their success into the
retry condition, so the role does not proceed to topology.yml until etcd quorum
and health are actually good.
In
`@deploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/wait-nodes.yml`:
- Around line 4-21: The CSR approval logic in the wait-nodes task is too broad
because it approves every pending request returned by oc get csr. Update the
pending-CSR collection and approval loop in the shell block so it only selects
kubelet client/server CSRs associated with sno_new_nodes, and then approve only
those entries before calling oc adm certificate approve. Use the existing
PENDING/CSR handling in wait-nodes.yml as the place to narrow the filter.
In `@deploy/openshift-clusters/roles/mutable-topology/sno-to-3node/vars/main.yml`:
- Around line 2-5: The kubeconfig path in sno_kubeconfig_resolved is incorrectly
derived from the default sno_cluster_name, so it can point to the wrong cluster
before discovery happens. Update the mutable-topology SNO-to-3node vars so
sno_cluster_name is not assumed here; instead require sno_cluster_name or
sno_kubeconfig to be provided up front, or change the resolution logic to use a
source that does not depend on the default value. Keep the fix localized to the
sno_kubeconfig_resolved expression and related variable handling.
---
Outside diff comments:
In `@deploy/Makefile`:
- Around line 122-130: Update the Makefile help output so the new `sno-ipi` and
`sno-agent` targets are listed alongside the existing deployment entries in
`make help`. Add matching `@echo` lines in the help block near the
`sno-to-3node` entry, using the same wording/style as the other target
descriptions so the new entrypoints are discoverable.
In `@deploy/openshift-clusters/roles/dev-scripts/install-dev/tasks/config.yml`:
- Around line 26-32: The missing-AGENT_E2E_TEST_SCENARIO failure message in the
config validation task should be updated to include the SNO option now supported
by the method-prefix logic. Adjust the fail message in the config validation
flow so it lists the correct remediation for all supported prefixes, including
the new sno case alongside the existing agent scenarios, and make sure the
guidance matches the prefix handling used in the task that selects by method.
In `@deploy/openshift-clusters/scripts/deploy-cluster.sh`:
- Around line 62-69: The validation in deploy-cluster.sh currently permits every
METHOD with TOPOLOGY=sno, which allows an unsupported sno + kcli path to
continue into the kcli install flow. Update the argument checks around the
TOPOLOGY/METHOD validation blocks to explicitly reject the unsupported
combination before any install logic runs, and keep the failure consistent with
the existing validation messages. Reference the validation section and the
kcli-install path so the guard is placed where the topology and method are first
checked.
---
Nitpick comments:
In `@deploy/openshift-clusters/scripts/clean-mutable-topology.sh`:
- Around line 1-2: The clean-mutable-topology.sh wrapper uses the wrong Bash
shebang, so update the script header to match the repository standard. Replace
the current shebang at the top of the file with the repo-required Bash
interpreter line, keeping the rest of the script unchanged; this is the only fix
needed in the script’s startup header.
In `@deploy/openshift-clusters/scripts/sno-to-3node.sh`:
- Around line 1-2: The wrapper script currently uses the non-standard Bash
shebang, so update the script header to match the repository convention by
changing the shebang at the top of sno-to-3node.sh to the repo-standard Bash
path. Keep the rest of the script unchanged, and verify the file still starts
with the expected shebang followed by set -euo pipefail.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: d1bb47f1-0e94-4c52-bf4c-ea4c9efe1020
📒 Files selected for processing (27)
deploy/Makefiledeploy/openshift-clusters/clean-mutable-topology.ymldeploy/openshift-clusters/roles/dev-scripts/install-dev/files/.gitignoredeploy/openshift-clusters/roles/dev-scripts/install-dev/files/config_sno_example.shdeploy/openshift-clusters/roles/dev-scripts/install-dev/handlers/main.ymldeploy/openshift-clusters/roles/dev-scripts/install-dev/tasks/config.ymldeploy/openshift-clusters/roles/dev-scripts/install-dev/vars/main.ymldeploy/openshift-clusters/roles/mutable-topology/sno-to-3node/defaults/main.ymldeploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/boot-nodes.ymldeploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/clean.ymldeploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/create-vms.ymldeploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/ignition.ymldeploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/main.ymldeploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/mco-rollout.ymldeploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/preflight.ymldeploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/topology.ymldeploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/update-dns.ymldeploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/verify.ymldeploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/wait-etcd.ymldeploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/wait-nodes.ymldeploy/openshift-clusters/roles/mutable-topology/sno-to-3node/templates/auto-install.ign.j2deploy/openshift-clusters/roles/mutable-topology/sno-to-3node/templates/master.ign.j2deploy/openshift-clusters/roles/mutable-topology/sno-to-3node/vars/main.ymldeploy/openshift-clusters/scripts/clean-mutable-topology.shdeploy/openshift-clusters/scripts/deploy-cluster.shdeploy/openshift-clusters/scripts/sno-to-3node.shdeploy/openshift-clusters/sno-to-3node.yml
eggfoobar
force-pushed
the
add-migration-support
branch
from
497dfcc to
134e627
Compare
coderabbitai
Bot
added
the
ready-for-human-review
Contributor
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
deploy/Makefile (1)
135-144: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd the new SNO deploy targets to
make help.
make helpdocuments the migration and cleanup entrypoints, butsno-ipiandsno-agentare still missing from the deployment list, so the new deploy flow is easy to miss.Suggested patch
`@echo` " fencing-assisted - Deploy hub + spoke TNF cluster via assisted installer" + `@echo` " sno-ipi - Deploy SNO IPI cluster (non-interactive)" + `@echo` " sno-agent - Deploy SNO Agent cluster (non-interactive)" `@echo` " sno-to-3node - Transition existing SNO cluster to 3-node HA (platform: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 `@deploy/Makefile` around lines 135 - 144, The help output in Makefile is missing the new SNO deployment entrypoints, so update the `make help` target’s OpenShift Cluster Management section to include `sno-ipi` and `sno-agent` alongside the existing deploy/cleanup commands. Keep the new entries grouped with the other deployment targets and use the same `@echo` pattern in the `help` recipe so the new flow is discoverable.
🧹 Nitpick comments (3)
deploy/openshift-clusters/roles/dev-scripts/install-dev/files/config_sno_example.sh (1)
1-41: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake this file clearly a config fragment or a real script.
It currently has a Bash shebang, but it does not follow the repo’s shell-script contract (
#!/usr/bin/bashandset -euo pipefail). Either drop the shebang and document it as a sourced config example, or make it conform to the script standard. As per coding guidelines, "Shell scripts should use#!/usr/bin/bashshebang" and "Include error handling withset -euo pipefail".🤖 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 `@deploy/openshift-clusters/roles/dev-scripts/install-dev/files/config_sno_example.sh` around lines 1 - 41, The file is currently ambiguous between a sourced config fragment and an executable shell script, and its header does not match the repo shell-script contract. Either remove the shebang and clearly treat it as a sourced config example, or convert it into a proper script by updating the header in config_sno_example.sh to use the required shell standard and add strict error handling near the top. Make the choice consistent with how this file is used, and keep the existing export-based config values under that contract.Source: Coding guidelines
deploy/openshift-clusters/scripts/sno-to-3node.sh (1)
1-2: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the repository Bash shebang.
This wrapper already follows the repo's strict-mode pattern, but the shebang should be
#!/usr/bin/bashto match the shell-script standard.Suggested patch
-#!/bin/bash +#!/usr/bin/bashAs per coding guidelines, "Shell scripts should use
#!/usr/bin/bashshebang".🤖 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 `@deploy/openshift-clusters/scripts/sno-to-3node.sh` around lines 1 - 2, The shell script header in sno-to-3node.sh should use the repository-standard Bash interpreter path instead of the generic Bash shebang. Update the script’s shebang at the top of the file to match the shell-script convention used across the repo, while keeping the existing strict-mode settings unchanged.Source: Coding guidelines
deploy/openshift-clusters/scripts/clean-mutable-topology.sh (1)
1-2: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the repository Bash shebang.
This wrapper already follows the repo's strict-mode pattern, but the shebang should be
#!/usr/bin/bashto match the shell-script standard.Suggested patch
-#!/bin/bash +#!/usr/bin/bashAs per coding guidelines, "Shell scripts should use
#!/usr/bin/bashshebang".🤖 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 `@deploy/openshift-clusters/scripts/clean-mutable-topology.sh` around lines 1 - 2, The script uses the wrong Bash shebang at the top of clean-mutable-topology.sh; update the shebang to the repository-standard /usr/bin/bash while keeping the existing strict-mode settings intact. Make this change at the script header so the wrapper matches the repo shell-script convention used alongside set -euo pipefail.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 `@deploy/openshift-clusters/roles/dev-scripts/install-dev/handlers/main.yml`:
- Around line 15-19: The `oc project` retry block in `main.yml` is swallowing
failures because `failed_when: false` lets the handler succeed even after all
retries are exhausted. Update the handler logic around `oc_project_result` so
that a nonzero `rc` after the final retry actually fails the task, or at minimum
emits an explicit warning/error on the last attempt; keep the retry behavior
intact but make the terminal failure visible.
In `@deploy/openshift-clusters/roles/dev-scripts/install-dev/tasks/config.yml`:
- Line 43: The missing-scenario remediation for agent methods is incomplete
because the guidance still only mentions TNF_IPV4 and TNA_IPV4 even though sno
is now supported; update the failure message in the agent method validation
logic to include SNO and point users to SNO_IPV4 as well. Make the change in the
same place that checks AGENT_E2E_TEST_SCENARIO so the message stays aligned with
the scenario mapping used by sno.
In
`@deploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/boot-nodes.yml`:
- Around line 57-58: The cleanup in boot-nodes is leaving UEFI guest definitions
behind because the `virsh undefine` call in the VM teardown sequence does not
remove the OVMF NVRAM varstore. Update the `virsh undefine` usage in the
`boot-nodes` task so it includes `--nvram` for these guests, keeping the
existing `virsh destroy`/cleanup flow in place and ensuring the old domain is
fully removed before the next `virt-install` runs.
In
`@deploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/clean.yml`:
- Around line 114-124: The cleanup in the `clean.yml` task only reloads libvirt
dnsmasq, but it leaves the NetworkManager dnsmasq override created by
`update-dns.yml` in place. Update the `[clean]` flow to also remove `{{
sno_nm_dnsmasq_conf }}` and then reload or restart `NetworkManager` so stale
local DNS entries for `api`, `api-int`, and `*.apps` are cleared. Use the
existing cleanup block near `[clean] Reload libvirt dnsmasq` and `[clean]
Cleanup complete` to add this DNS teardown step.
In
`@deploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/create-vms.yml`:
- Around line 130-137: The "[create-vms] Add DNS entries to libvirt network"
task currently hides failures by setting failed_when: false on the virsh
net-update add dns-host call. Remove the failure masking so the task in
create-vms.yml fails when virsh net-update cannot update dns-host, and keep the
existing loop over sno_new_nodes so each hostname update is validated. If a
fallback is needed, handle it explicitly in a separate step rather than allowing
this task to report success when the libvirt DNS update did not apply.
In
`@deploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/ignition.yml`:
- Around line 2-7: The `ignition.yml` probes are unbounded and can hang the role
during API or DNS stalls. Update the `"[ignition] Extract MCS CA certificate"`
task and the later `curl` probe task to use explicit timeouts for the `oc` and
`curl` commands, keeping the existing task structure and symbols like
`mcs_ca_raw` and the probe step intact. Ensure both commands fail quickly
instead of waiting indefinitely.
In
`@deploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/mco-rollout.yml`:
- Around line 42-64: The drain auto-fix currently applies to every cordoned node
in the mco-rollout task, which can bypass normal MCO drains or intentional
maintenance cordons. Update the logic around CORDONED_NODES so it only runs when
there is a stale MCO-drain signal, or change sno_auto_fix_drain to be opt-in by
default instead of default(true). Keep the uncordon/lastAppliedDrain/MCC restart
flow behind that stricter gate so normal SchedulingDisabled nodes are not
touched.
In
`@deploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/preflight.yml`:
- Around line 135-143: The embedded Python passed to python3 -c in the preflight
task is not indented under the shell block, so the YAML parser can misread it as
new keys and fail to load the file. Fix the shell: | content in preflight.yml by
indenting the full Python snippet consistently beneath the CURRENT_IP assignment
while keeping the same logic in the task that queries virsh net-dumpxml for
api-int.{{ sno_cluster_domain }}.
In
`@deploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/topology.yml`:
- Around line 2-5: The CVO disable step in the topology transition flow is not
protected by a restore path, so a failure can leave the cluster without a
running CVO. Update the SNO-to-3node topology task sequence around the
scale-down step to use a block with an always section that scales
cluster-version-operator back up, or add an explicit restore task after the
rollout phases if CVO must remain down temporarily. Make sure the fix is tied to
the existing topology task(s) that perform the CVO scale-down so the recovery
runs even when later phases fail.
In
`@deploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/update-dns.yml`:
- Around line 57-69: The DNS verification in update-dns.yml is too weak because
the API check only tests for any non-empty result and the apps check never
fails, so phase 8 can pass with stale or partial records. Tighten the validation
in the existing dns verification tasks by comparing the dig results against
sno_all_node_ips[0] for api and sno_all_node_ips for test.apps, and make the
checks fail/retry until the returned records match instead of accepting any
response.
In
`@deploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/verify.yml`:
- Around line 91-111: The final verification in the [verify] task only checks
the number of node rows and can pass even if some nodes are NotReady. Update the
existing node verification around verify_nodes so it explicitly asserts all
three nodes are Ready before the migration success message is emitted, and keep
the HA/etcd checks as-is. Use the verify_nodes assertion block and the "[verify]
SNO to 3-node transition verified successfully" debug step to locate where to
add the readiness condition.
In `@deploy/openshift-clusters/scripts/deploy-cluster.sh`:
- Around line 62-63: The topology validation in deploy-cluster.sh currently
allows sno unconditionally, which lets unsupported kcli SNO requests pass into
the playbook and fail later. Update the TOPOLOGY check in the deploy-cluster.sh
flow so sno is only accepted for supported deployment methods, using the
existing method selection logic in the script, or alternatively align
kcli-install/tasks/validate.yml so kcli explicitly supports sno. Keep the fix
centered around the topology validation gate and the kcli validation path.
---
Outside diff comments:
In `@deploy/Makefile`:
- Around line 135-144: The help output in Makefile is missing the new SNO
deployment entrypoints, so update the `make help` target’s OpenShift Cluster
Management section to include `sno-ipi` and `sno-agent` alongside the existing
deploy/cleanup commands. Keep the new entries grouped with the other deployment
targets and use the same `@echo` pattern in the `help` recipe so the new flow is
discoverable.
---
Nitpick comments:
In
`@deploy/openshift-clusters/roles/dev-scripts/install-dev/files/config_sno_example.sh`:
- Around line 1-41: The file is currently ambiguous between a sourced config
fragment and an executable shell script, and its header does not match the repo
shell-script contract. Either remove the shebang and clearly treat it as a
sourced config example, or convert it into a proper script by updating the
header in config_sno_example.sh to use the required shell standard and add
strict error handling near the top. Make the choice consistent with how this
file is used, and keep the existing export-based config values under that
contract.
In `@deploy/openshift-clusters/scripts/clean-mutable-topology.sh`:
- Around line 1-2: The script uses the wrong Bash shebang at the top of
clean-mutable-topology.sh; update the shebang to the repository-standard
/usr/bin/bash while keeping the existing strict-mode settings intact. Make this
change at the script header so the wrapper matches the repo shell-script
convention used alongside set -euo pipefail.
In `@deploy/openshift-clusters/scripts/sno-to-3node.sh`:
- Around line 1-2: The shell script header in sno-to-3node.sh should use the
repository-standard Bash interpreter path instead of the generic Bash shebang.
Update the script’s shebang at the top of the file to match the shell-script
convention used across the repo, while keeping the existing strict-mode settings
unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 2df4a7c9-40e6-4c41-b37a-324e24187240
📒 Files selected for processing (27)
deploy/Makefiledeploy/openshift-clusters/clean-mutable-topology.ymldeploy/openshift-clusters/roles/dev-scripts/install-dev/files/.gitignoredeploy/openshift-clusters/roles/dev-scripts/install-dev/files/config_sno_example.shdeploy/openshift-clusters/roles/dev-scripts/install-dev/handlers/main.ymldeploy/openshift-clusters/roles/dev-scripts/install-dev/tasks/config.ymldeploy/openshift-clusters/roles/dev-scripts/install-dev/vars/main.ymldeploy/openshift-clusters/roles/mutable-topology/sno-to-3node/defaults/main.ymldeploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/boot-nodes.ymldeploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/clean.ymldeploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/create-vms.ymldeploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/ignition.ymldeploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/main.ymldeploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/mco-rollout.ymldeploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/preflight.ymldeploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/topology.ymldeploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/update-dns.ymldeploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/verify.ymldeploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/wait-etcd.ymldeploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/wait-nodes.ymldeploy/openshift-clusters/roles/mutable-topology/sno-to-3node/templates/auto-install.ign.j2deploy/openshift-clusters/roles/mutable-topology/sno-to-3node/templates/master.ign.j2deploy/openshift-clusters/roles/mutable-topology/sno-to-3node/vars/main.ymldeploy/openshift-clusters/scripts/clean-mutable-topology.shdeploy/openshift-clusters/scripts/deploy-cluster.shdeploy/openshift-clusters/scripts/sno-to-3node.shdeploy/openshift-clusters/sno-to-3node.yml
Contributor
Author
|
/label tide/merge-method-squash |
openshift-ci
Bot
added
the
tide/merge-method-squash
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
deploy/openshift-clusters/scripts/deploy-cluster.sh (1)
52-63: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winKeep
--helpaligned with the accepted topologies.After allowing
snohere, the file header and the-h|--helpbranch still advertise onlyarbiter|fencing.deploy-cluster.sh --helpnow contradicts the actual interface.🤖 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 `@deploy/openshift-clusters/scripts/deploy-cluster.sh` around lines 52 - 63, Update the CLI documentation in deploy-cluster.sh so it matches the validation logic that now accepts sno. In the script header and the -h|--help handling, replace any arbiter|fencing-only wording with arbiter|fencing|sno, and ensure the usage/examples for topology reference the same accepted values as the TOPOLOGY validation block.
♻️ Duplicate comments (1)
deploy/openshift-clusters/roles/dev-scripts/install-dev/handlers/main.yml (1)
15-31: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winStill fail the handler after the last
oc projectretry.Line 19 keeps the task successful even when all retries are exhausted, and Lines 22-31 only log a warning. That still hides real kubeconfig/auth/API failures and lets the role continue in a bad state.
Proposed fix
- name: Run oc project command: oc --kubeconfig="{{ kubeconfig_path }}" project openshift-machine-api when: - kubeconfig_path is defined - kubeconfig_stat.stat.exists | default(false) changed_when: false register: oc_project_result retries: 5 delay: 15 until: oc_project_result.rc == 0 - failed_when: false + failed_when: oc_project_result.rc != 0 listen: Set OCP project - -- name: Warn if oc project failed after all retries - debug: - msg: >- - WARNING: 'oc project openshift-machine-api' still failing after all retries - (rc={{ oc_project_result.rc | default('n/a') }}). - stderr: {{ oc_project_result.stderr | default('') | trim }} - when: - - kubeconfig_path is defined - - kubeconfig_stat.stat.exists | default(false) - - oc_project_result.rc | default(0) != 0 - listen: Set OCP project🤖 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 `@deploy/openshift-clusters/roles/dev-scripts/install-dev/handlers/main.yml` around lines 15 - 31, The handler currently masks a failed oc_project_result by setting failed_when: false and only emitting a warning in the follow-up debug task, so the role can continue after all retries are exhausted. Update the Set OCP project handler logic to use oc_project_result in a failure condition after the retries are done, and keep the warning only as additional context; ensure the failure is tied to the existing oc_project_result check and the Set OCP project handler rather than just logging in the debug task.
🧹 Nitpick comments (2)
deploy/openshift-clusters/scripts/sno-to-3node.sh (1)
1-2: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the repository-standard Bash shebang.
This new script uses
#!/bin/bash, but the repo rule for*.shis#!/usr/bin/bash.As per coding guidelines,
**/*.sh:Shell scripts should use #!/usr/bin/bash shebang.🤖 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 `@deploy/openshift-clusters/scripts/sno-to-3node.sh` around lines 1 - 2, The new shell script uses the wrong Bash shebang; update the top of sno-to-3node.sh to match the repository-standard `#!/usr/bin/bash` used for `*.sh` scripts. Keep the rest of the script unchanged and make sure the entrypoint shebang is consistent with the repo’s shell script convention.Source: Coding guidelines
deploy/openshift-clusters/scripts/clean-mutable-topology.sh (1)
1-2: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the repository-standard Bash shebang.
This new script uses
#!/bin/bash, but the repo rule for*.shis#!/usr/bin/bash.As per coding guidelines,
**/*.sh:Shell scripts should use #!/usr/bin/bash shebang.🤖 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 `@deploy/openshift-clusters/scripts/clean-mutable-topology.sh` around lines 1 - 2, The new shell script is using the wrong Bash interpreter path; update the shebang in the script header to match the repository-standard format used for .sh files. Locate the script by its top-level shebang and replace the current Bash path with the repo-required /usr/bin/bash form so it aligns with the shell script convention.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 `@deploy/Makefile`:
- Around line 79-84: The new deploy targets sno-ipi and sno-agent are missing
from the make help output, so update the help/documentation logic in
deploy/Makefile to advertise them alongside sno-to-3node. Add these targets to
the same help entry list or help-generation block used by make help so users can
discover the new entrypoints from the main CLI.
In `@deploy/openshift-clusters/clean-mutable-topology.yml`:
- Around line 35-53: The topology guard in the live cluster check is too
permissive because it allows cleanup to continue when the detected
controlPlaneTopology is Unknown. Update the logic around the live_topology check
and the Abort if cluster is HA and force_cleanup is not set step so destructive
cleanup only proceeds when the topology is explicitly SingleReplica, or when
force_cleanup is true; otherwise fail closed on Unknown or any non-explicit
result. Use the existing live_topology, clean.yml, and controlPlaneTopology
handling to make the condition explicit and prevent master-1/master-2 deletion
unless the topology is confirmed safe.
In
`@deploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/clean.yml`:
- Around line 127-130: The cleanup task still only reloads NetworkManager, so
the old dnsmasq state may survive and keep stale DNS overrides. Update the
cleanup step in the NetworkManager reset flow to use a full restart after
removing `{{ sno_nm_dnsmasq_conf }}`, matching the behavior used in
`update-dns.yml`; keep the change in the `clean.yml` task that manages
NetworkManager so the stale `api`/`api-int`/`*.apps` answers are fully cleared.
In
`@deploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/create-vms.yml`:
- Around line 2-9: The stale VM cleanup in the create-vms task is missing the
OVMF-specific NVRAM removal flag, which can leave domain state behind and
interfere with recreating the VM. Update the `virsh undefine` call in the
`"[create-vms] Cleanup stale VMs"` task to include `--nvram`, keeping the rest
of the cleanup flow in `create-vms.yml` unchanged.
In
`@deploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/preflight.yml`:
- Around line 155-167: The DNS verification in the preflight task is using
substring matching, which can incorrectly accept a different IP when one answer
prefixes another. Update the checks in the preflight shell task and the
`dns_apiint_verify` validation to compare against trimmed `stdout_lines` entries
instead of `in dns_apiint.stdout`, so the `dig` result for `api-int` must match
the exact `sno_master0_ip` value.
In
`@deploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/verify.yml`:
- Around line 47-78: The verification step in the cluster operator summary only
reports degraded/unavailable operators and never blocks completion, so the
migration can still finish in a bad state. Update the verify task that runs the
oc get co summary and stores it in verify_co to explicitly fail when any
operator is Degraded or Available=False, using the existing degraded and
unavailable lists as the gate, so the role stops before emitting MIGRATION
COMPLETE.
---
Outside diff comments:
In `@deploy/openshift-clusters/scripts/deploy-cluster.sh`:
- Around line 52-63: Update the CLI documentation in deploy-cluster.sh so it
matches the validation logic that now accepts sno. In the script header and the
-h|--help handling, replace any arbiter|fencing-only wording with
arbiter|fencing|sno, and ensure the usage/examples for topology reference the
same accepted values as the TOPOLOGY validation block.
---
Duplicate comments:
In `@deploy/openshift-clusters/roles/dev-scripts/install-dev/handlers/main.yml`:
- Around line 15-31: The handler currently masks a failed oc_project_result by
setting failed_when: false and only emitting a warning in the follow-up debug
task, so the role can continue after all retries are exhausted. Update the Set
OCP project handler logic to use oc_project_result in a failure condition after
the retries are done, and keep the warning only as additional context; ensure
the failure is tied to the existing oc_project_result check and the Set OCP
project handler rather than just logging in the debug task.
---
Nitpick comments:
In `@deploy/openshift-clusters/scripts/clean-mutable-topology.sh`:
- Around line 1-2: The new shell script is using the wrong Bash interpreter
path; update the shebang in the script header to match the repository-standard
format used for .sh files. Locate the script by its top-level shebang and
replace the current Bash path with the repo-required /usr/bin/bash form so it
aligns with the shell script convention.
In `@deploy/openshift-clusters/scripts/sno-to-3node.sh`:
- Around line 1-2: The new shell script uses the wrong Bash shebang; update the
top of sno-to-3node.sh to match the repository-standard `#!/usr/bin/bash` used
for `*.sh` scripts. Keep the rest of the script unchanged and make sure the
entrypoint shebang is consistent with the repo’s shell script convention.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: bd4f4653-4596-463f-be41-02bedea644dd
📒 Files selected for processing (27)
deploy/Makefiledeploy/openshift-clusters/clean-mutable-topology.ymldeploy/openshift-clusters/roles/dev-scripts/install-dev/files/.gitignoredeploy/openshift-clusters/roles/dev-scripts/install-dev/files/config_sno_example.shdeploy/openshift-clusters/roles/dev-scripts/install-dev/handlers/main.ymldeploy/openshift-clusters/roles/dev-scripts/install-dev/tasks/config.ymldeploy/openshift-clusters/roles/dev-scripts/install-dev/vars/main.ymldeploy/openshift-clusters/roles/mutable-topology/sno-to-3node/defaults/main.ymldeploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/boot-nodes.ymldeploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/clean.ymldeploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/create-vms.ymldeploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/ignition.ymldeploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/main.ymldeploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/mco-rollout.ymldeploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/preflight.ymldeploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/topology.ymldeploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/update-dns.ymldeploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/verify.ymldeploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/wait-etcd.ymldeploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/wait-nodes.ymldeploy/openshift-clusters/roles/mutable-topology/sno-to-3node/templates/auto-install.ign.j2deploy/openshift-clusters/roles/mutable-topology/sno-to-3node/templates/master.ign.j2deploy/openshift-clusters/roles/mutable-topology/sno-to-3node/vars/main.ymldeploy/openshift-clusters/scripts/clean-mutable-topology.shdeploy/openshift-clusters/scripts/deploy-cluster.shdeploy/openshift-clusters/scripts/sno-to-3node.shdeploy/openshift-clusters/sno-to-3node.yml
Comment on lines
+35
to
+53
| - name: Check live cluster topology | ||
| shell: | | ||
| KUBECONFIG={{ dev_scripts_path | default('openshift-metal3/dev-scripts') }}/ocp/{{ sno_cluster_name | default('ostest') }}/auth/kubeconfig \ | ||
| oc get infrastructure cluster \ | ||
| -o jsonpath='{.status.controlPlaneTopology}' 2>/dev/null || echo "Unknown" | ||
| register: live_topology | ||
| changed_when: false | ||
| failed_when: false | ||
| ignore_errors: true | ||
|
|
||
| - name: Abort if cluster is HA and force_cleanup is not set | ||
| ansible.builtin.fail: | ||
| msg: >- | ||
| Cluster controlPlaneTopology is '{{ live_topology.stdout | trim }}'. | ||
| Refusing to remove master-1 and master-2 from a live HA cluster. | ||
| Re-run with -e "force_cleanup=true" to override. | ||
| when: | ||
| - live_topology.stdout | trim == "HighlyAvailable" | ||
| - not (force_cleanup | default(false) | bool) |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Fail closed when topology detection is unavailable.
This still proceeds on Unknown. If oc/kubeconfig access is broken on an already-HA cluster, the playbook skips the guard and drops into clean.yml, which deletes master-1 and master-2 anyway. Destructive cleanup should only run when the topology is explicitly SingleReplica, or when force_cleanup=true is set.
Suggested guard change
- - name: Abort if cluster is HA and force_cleanup is not set
+ - name: Abort unless cluster is SingleReplica or force_cleanup is set
ansible.builtin.fail:
msg: >-
Cluster controlPlaneTopology is '{{ live_topology.stdout | trim }}'.
- Refusing to remove master-1 and master-2 from a live HA cluster.
+ Refusing to remove master-1 and master-2 unless the live cluster is
+ explicitly SingleReplica.
Re-run with -e "force_cleanup=true" to override.
when:
- - live_topology.stdout | trim == "HighlyAvailable"
- not (force_cleanup | default(false) | bool)
+ - live_topology.stdout | trim != "SingleReplica"📝 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: Check live cluster topology | |
| shell: | | |
| KUBECONFIG={{ dev_scripts_path | default('openshift-metal3/dev-scripts') }}/ocp/{{ sno_cluster_name | default('ostest') }}/auth/kubeconfig \ | |
| oc get infrastructure cluster \ | |
| -o jsonpath='{.status.controlPlaneTopology}' 2>/dev/null || echo "Unknown" | |
| register: live_topology | |
| changed_when: false | |
| failed_when: false | |
| ignore_errors: true | |
| - name: Abort if cluster is HA and force_cleanup is not set | |
| ansible.builtin.fail: | |
| msg: >- | |
| Cluster controlPlaneTopology is '{{ live_topology.stdout | trim }}'. | |
| Refusing to remove master-1 and master-2 from a live HA cluster. | |
| Re-run with -e "force_cleanup=true" to override. | |
| when: | |
| - live_topology.stdout | trim == "HighlyAvailable" | |
| - not (force_cleanup | default(false) | bool) | |
| - name: Abort unless cluster is SingleReplica or force_cleanup is set | |
| ansible.builtin.fail: | |
| msg: >- | |
| Cluster controlPlaneTopology is '{{ live_topology.stdout | trim }}'. | |
| Refusing to remove master-1 and master-2 unless the live cluster is | |
| explicitly SingleReplica. | |
| Re-run with -e "force_cleanup=true" to override. | |
| when: | |
| - not (force_cleanup | default(false) | bool) | |
| - live_topology.stdout | trim != "SingleReplica" |
🤖 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 `@deploy/openshift-clusters/clean-mutable-topology.yml` around lines 35 - 53,
The topology guard in the live cluster check is too permissive because it allows
cleanup to continue when the detected controlPlaneTopology is Unknown. Update
the logic around the live_topology check and the Abort if cluster is HA and
force_cleanup is not set step so destructive cleanup only proceeds when the
topology is explicitly SingleReplica, or when force_cleanup is true; otherwise
fail closed on Unknown or any non-explicit result. Use the existing
live_topology, clean.yml, and controlPlaneTopology handling to make the
condition explicit and prevent master-1/master-2 deletion unless the topology is
confirmed safe.
Comment on lines
+127
to
+130
| - name: "[clean] Reload NetworkManager to clear stale DNS overrides" | ||
| systemd: | ||
| name: NetworkManager | ||
| state: reloaded |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Restart NetworkManager here too.
update-dns.yml had to restart NetworkManager to apply {{ sno_nm_dnsmasq_conf }}, but cleanup only reloads after deleting that file. If the embedded dnsmasq process keeps the old config across reload, stale api/api-int/*.apps answers survive cleanup and poison the next run.
Proposed fix
- name: "[clean] Reload NetworkManager to clear stale DNS overrides"
systemd:
name: NetworkManager
- state: reloaded
+ state: restarted
become: true
failed_when: false📝 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: "[clean] Reload NetworkManager to clear stale DNS overrides" | |
| systemd: | |
| name: NetworkManager | |
| state: reloaded | |
| - name: "[clean] Reload NetworkManager to clear stale DNS overrides" | |
| systemd: | |
| name: NetworkManager | |
| state: restarted |
🤖 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
`@deploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/clean.yml`
around lines 127 - 130, The cleanup task still only reloads NetworkManager, so
the old dnsmasq state may survive and keep stale DNS overrides. Update the
cleanup step in the NetworkManager reset flow to use a full restart after
removing `{{ sno_nm_dnsmasq_conf }}`, matching the behavior used in
`update-dns.yml`; keep the change in the `clean.yml` task that manages
NetworkManager so the stale `api`/`api-int`/`*.apps` answers are fully cleared.
Comment on lines
+155
to
+167
| when: dns_apiint.stdout | trim == "" or sno_master0_ip not in dns_apiint.stdout | ||
| changed_when: true | ||
|
|
||
| - name: "[preflight] Verify api-int DNS after fix" | ||
| shell: | | ||
| dig +short api-int.{{ sno_cluster_domain }} @192.168.111.1 | ||
| register: dns_apiint_verify | ||
| changed_when: false | ||
| failed_when: sno_master0_ip not in dns_apiint_verify.stdout | ||
| retries: 3 | ||
| delay: 2 | ||
| until: sno_master0_ip in dns_apiint_verify.stdout | ||
| when: dns_apiint.stdout | trim == "" or sno_master0_ip not in dns_apiint.stdout |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Match DNS answers exactly, not by substring.
in dns_apiint.stdout can accept the wrong IP when one address is a prefix of another. Compare against trimmed stdout_lines instead.
Proposed fix
- when: dns_apiint.stdout | trim == "" or sno_master0_ip not in dns_apiint.stdout
+ when: dns_apiint.stdout | trim == "" or sno_master0_ip not in (dns_apiint.stdout_lines | map('trim') | list)
@@
- failed_when: sno_master0_ip not in dns_apiint_verify.stdout
+ failed_when: sno_master0_ip not in (dns_apiint_verify.stdout_lines | map('trim') | list)
@@
- until: sno_master0_ip in dns_apiint_verify.stdout
+ until: sno_master0_ip in (dns_apiint_verify.stdout_lines | map('trim') | 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
| when: dns_apiint.stdout | trim == "" or sno_master0_ip not in dns_apiint.stdout | |
| changed_when: true | |
| - name: "[preflight] Verify api-int DNS after fix" | |
| shell: | | |
| dig +short api-int.{{ sno_cluster_domain }} @192.168.111.1 | |
| register: dns_apiint_verify | |
| changed_when: false | |
| failed_when: sno_master0_ip not in dns_apiint_verify.stdout | |
| retries: 3 | |
| delay: 2 | |
| until: sno_master0_ip in dns_apiint_verify.stdout | |
| when: dns_apiint.stdout | trim == "" or sno_master0_ip not in dns_apiint.stdout | |
| when: dns_apiint.stdout | trim == "" or sno_master0_ip not in (dns_apiint.stdout_lines | map('trim') | list) | |
| changed_when: true | |
| - name: "[preflight] Verify api-int DNS after fix" | |
| shell: | | |
| dig +short api-int.{{ sno_cluster_domain }} `@192.168.111.1` | |
| register: dns_apiint_verify | |
| changed_when: false | |
| failed_when: sno_master0_ip not in (dns_apiint_verify.stdout_lines | map('trim') | list) | |
| retries: 3 | |
| delay: 2 | |
| until: sno_master0_ip in (dns_apiint_verify.stdout_lines | map('trim') | list) | |
| when: dns_apiint.stdout | trim == "" or sno_master0_ip not in (dns_apiint.stdout_lines | map('trim') | 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
`@deploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/preflight.yml`
around lines 155 - 167, The DNS verification in the preflight task is using
substring matching, which can incorrectly accept a different IP when one answer
prefixes another. Update the checks in the preflight shell task and the
`dns_apiint_verify` validation to compare against trimmed `stdout_lines` entries
instead of `in dns_apiint.stdout`, so the `dig` result for `api-int` must match
the exact `sno_master0_ip` value.
Comment on lines
+47
to
+78
| - name: "[verify] Get cluster operator summary" | ||
| shell: | | ||
| oc get co -o json | python3 -c " | ||
| import json, sys | ||
| data = json.load(sys.stdin) | ||
| total = len(data['items']) | ||
| degraded = [] | ||
| unavailable = [] | ||
| for co in data['items']: | ||
| name = co['metadata']['name'] | ||
| conds = {c['type']: c['status'] for c in co.get('status', {}).get('conditions', [])} | ||
| if conds.get('Degraded') == 'True': | ||
| degraded.append(name) | ||
| if conds.get('Available') == 'False': | ||
| unavailable.append(name) | ||
| print(f'Total COs: {total}') | ||
| if degraded: | ||
| print(f'Degraded: {chr(44).join(degraded)}') | ||
| else: | ||
| print('Degraded: none') | ||
| if unavailable: | ||
| print(f'Unavailable: {chr(44).join(unavailable)}') | ||
| else: | ||
| print('Unavailable: none') | ||
| " | ||
| environment: "{{ sno_oc_env }}" | ||
| register: verify_co | ||
| changed_when: false | ||
|
|
||
| - name: "[verify] Cluster operators" | ||
| debug: | ||
| msg: "{{ verify_co.stdout }}" |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Fail the migration when cluster operators are still degraded or unavailable.
This block only prints the degraded/unavailable operator list, then the role still emits MIGRATION COMPLETE. Since mco-rollout.yml is best-effort and main.yml ends after this phase, there is no later hard gate that proves post-transition convergence.
Proposed fix
- name: "[verify] Cluster operators"
debug:
msg: "{{ verify_co.stdout }}"
+- name: "[verify] Assert cluster operators healthy"
+ assert:
+ that:
+ - "'Degraded: none' in verify_co.stdout"
+ - "'Unavailable: none' in verify_co.stdout"
+ fail_msg: "{{ verify_co.stdout }}"
+
- name: "[verify] Get MCP master status"
shell: |
oc get mcp master --no-headersAlso applies to: 111-116
🤖 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
`@deploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/verify.yml`
around lines 47 - 78, The verification step in the cluster operator summary only
reports degraded/unavailable operators and never blocks completion, so the
migration can still finish in a bad state. Update the verify task that runs the
oc get co summary and stores it in verify_co to explicitly fail when any
operator is Degraded or Available=False, using the existing degraded and
unavailable lists as the gate, so the role stops before emitting MIGRATION
COMPLETE.
e1bf815 to
f8eef94
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
deploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/clean.yml (1)
127-132: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winStill reloading instead of restarting NetworkManager.
update-dns.ymlrestarts NetworkManager to applysno_nm_dnsmasq_conf, but this cleanup step only reloads (state: reloaded) after deleting the file, which was previously flagged as insufficient to guarantee the embedded dnsmasq process drops the stale config.♻️ Proposed fix
- name: "[clean] Reload NetworkManager to clear stale DNS overrides" systemd: name: NetworkManager - state: reloaded + state: restarted become: true failed_when: false🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/clean.yml` around lines 127 - 132, The cleanup task still uses a NetworkManager reload, which is not enough to ensure the embedded dnsmasq process drops the removed DNS override. Update the task named “[clean] Reload NetworkManager to clear stale DNS overrides” in clean.yml to perform a restart via the systemd task instead of a reload, matching the behavior used in update-dns.yml and keeping the existing become and failed_when handling.deploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/verify.yml (1)
47-90: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winCluster operator and MCP health are still not gated before declaring success.
The
[verify] Cluster operatorsand[verify] MCP statusblocks only printverify_co.stdout/verify_mcp.stdout; there is noassertthat fails the play when operators are Degraded/Unavailable or the MCP is not Updated.main.ymltreatsverify.ymlas the terminal "Phase 9" step, so the role can still report a successful migration with degraded operators or a stuck MCP rollout.♻️ Proposed fix
- name: "[verify] Cluster operators" debug: msg: "{{ verify_co.stdout }}" +- name: "[verify] Assert cluster operators healthy" + assert: + that: + - "'Degraded: none' in verify_co.stdout" + - "'Unavailable: none' in verify_co.stdout" + fail_msg: "{{ verify_co.stdout }}" + - name: "[verify] Get MCP master status" shell: | oc get mcp master --no-headers environment: "{{ sno_oc_env }}" register: verify_mcp changed_when: false - name: "[verify] MCP status" debug: msg: "{{ verify_mcp.stdout }}" + +- name: "[verify] Assert MCP updated" + assert: + that: "'True' in verify_mcp.stdout" + fail_msg: "MCP master not fully updated:\n{{ verify_mcp.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 `@deploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/verify.yml` around lines 47 - 90, The verify tasks currently only report Cluster Operator and MCP output, but do not fail the play when health is bad. Update the verify flow in the existing `verify_co`/`verify_mcp` checks so it asserts no operators are Degraded or Unavailable and that the master MCP is fully updated/ready before success is declared. Use the existing `verify_co` and `verify_mcp` registered results to add explicit failure conditions after the `"[verify] Cluster operators"` and `"[verify] MCP status"` steps.
🧹 Nitpick comments (3)
deploy/openshift-clusters/scripts/sno-to-3node.sh (1)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueShebang should be
#!/usr/bin/bash.As per coding guidelines,
**/*.shfiles "should use#!/usr/bin/bashshebang". This script uses#!/bin/bash.diff
-#!/bin/bash +#!/usr/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 `@deploy/openshift-clusters/scripts/sno-to-3node.sh` at line 1, Update the script header to use the required bash interpreter path: this file currently starts with the wrong shebang, so replace the existing `#!/bin/bash` at the top of `sno-to-3node.sh` with `#!/usr/bin/bash` to match the shell script guideline.Source: Coding guidelines
deploy/openshift-clusters/scripts/clean-mutable-topology.sh (2)
1-22: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate logic with
sno-to-3node.sh.This script is identical to
deploy/openshift-clusters/scripts/sno-to-3node.shaside from the playbook filename. Consider extracting the shared instance/inventory validation and invocation logic into a common helper (e.g., alongsidecommon.sh) to avoid divergence over time.🤖 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 `@deploy/openshift-clusters/scripts/clean-mutable-topology.sh` around lines 1 - 22, The validation and ansible invocation logic in clean-mutable-topology.sh is duplicated in sno-to-3node.sh, so extract the shared checks and playbook runner into a common helper sourced by both scripts. Move the instance-dir and inventory.ini validation plus the cd/ansible-playbook flow into a reusable function in common.sh or a nearby shared script, and keep only the playbook-specific filename in each wrapper. Use the existing get_node_dir and DEPLOY_DIR setup to preserve behavior while removing the duplicate shell logic.
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueShebang should be
#!/usr/bin/bash.As per coding guidelines,
**/*.shfiles "should use#!/usr/bin/bashshebang". This script uses#!/bin/bash.diff
-#!/bin/bash +#!/usr/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 `@deploy/openshift-clusters/scripts/clean-mutable-topology.sh` at line 1, The script uses the wrong shebang and should follow the shell script guideline by switching the top-of-file interpreter line in clean-mutable-topology.sh from the current bash path to usr/bin/bash. Update the shebang only, keeping the rest of the script unchanged.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 `@deploy/openshift-clusters/clean-mutable-topology.yml`:
- Around line 16-39: The cleanup probes are rebuilding KUBECONFIG from
dev_scripts_path and sno_cluster_name instead of honoring the sno_kubeconfig
override, which can point them at the wrong cluster. Update the two shell probes
in the cleanup workflow to use sno_kubeconfig when it is defined, and only fall
back to the existing computed kubeconfig path when the override is absent. Keep
the existing detection logic in the domain and topology steps, but make sure
both oc invocations reference the same kubeconfig source consistently.
In
`@deploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/preflight.yml`:
- Around line 122-124: The preflight DNS verification is hardcoded to query a
fixed dnsmasq server, which can point at the wrong libvirt network. Update the
`dig` checks in the preflight tasks to use the DNS server associated with `{{
sno_libvirt_network }}` instead of `@192.168.111.1`, and make the same change in
both the `api-int` and related verification steps so the check matches the
repair path.
In
`@deploy/openshift-clusters/roles/mutable-topology/sno-to-3node/templates/auto-install.ign.j2`:
- Around line 8-12: The `/etc/master.ign` entry in `auto-install.ign.j2` is too
permissive for rendered ignition content that may include sensitive
control-plane credentials. Update the file mode for this object to restrict
access to the installer service only, keeping the change localized to the
`/etc/master.ign` stanza and preserving the existing `contents.source` handling.
---
Duplicate comments:
In
`@deploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/clean.yml`:
- Around line 127-132: The cleanup task still uses a NetworkManager reload,
which is not enough to ensure the embedded dnsmasq process drops the removed DNS
override. Update the task named “[clean] Reload NetworkManager to clear stale
DNS overrides” in clean.yml to perform a restart via the systemd task instead of
a reload, matching the behavior used in update-dns.yml and keeping the existing
become and failed_when handling.
In
`@deploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/verify.yml`:
- Around line 47-90: The verify tasks currently only report Cluster Operator and
MCP output, but do not fail the play when health is bad. Update the verify flow
in the existing `verify_co`/`verify_mcp` checks so it asserts no operators are
Degraded or Unavailable and that the master MCP is fully updated/ready before
success is declared. Use the existing `verify_co` and `verify_mcp` registered
results to add explicit failure conditions after the `"[verify] Cluster
operators"` and `"[verify] MCP status"` steps.
---
Nitpick comments:
In `@deploy/openshift-clusters/scripts/clean-mutable-topology.sh`:
- Around line 1-22: The validation and ansible invocation logic in
clean-mutable-topology.sh is duplicated in sno-to-3node.sh, so extract the
shared checks and playbook runner into a common helper sourced by both scripts.
Move the instance-dir and inventory.ini validation plus the cd/ansible-playbook
flow into a reusable function in common.sh or a nearby shared script, and keep
only the playbook-specific filename in each wrapper. Use the existing
get_node_dir and DEPLOY_DIR setup to preserve behavior while removing the
duplicate shell logic.
- Line 1: The script uses the wrong shebang and should follow the shell script
guideline by switching the top-of-file interpreter line in
clean-mutable-topology.sh from the current bash path to usr/bin/bash. Update the
shebang only, keeping the rest of the script unchanged.
In `@deploy/openshift-clusters/scripts/sno-to-3node.sh`:
- Line 1: Update the script header to use the required bash interpreter path:
this file currently starts with the wrong shebang, so replace the existing
`#!/bin/bash` at the top of `sno-to-3node.sh` with `#!/usr/bin/bash` to match
the shell script guideline.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 5541d134-f639-4040-b324-49e50d05bf1c
📒 Files selected for processing (23)
deploy/Makefiledeploy/openshift-clusters/clean-mutable-topology.ymldeploy/openshift-clusters/roles/dev-scripts/install-dev/handlers/main.ymldeploy/openshift-clusters/roles/dev-scripts/install-dev/tasks/config.ymldeploy/openshift-clusters/roles/mutable-topology/sno-to-3node/defaults/main.ymldeploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/boot-nodes.ymldeploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/clean.ymldeploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/create-vms.ymldeploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/ignition.ymldeploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/main.ymldeploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/mco-rollout.ymldeploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/preflight.ymldeploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/topology.ymldeploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/update-dns.ymldeploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/verify.ymldeploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/wait-etcd.ymldeploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/wait-nodes.ymldeploy/openshift-clusters/roles/mutable-topology/sno-to-3node/templates/auto-install.ign.j2deploy/openshift-clusters/roles/mutable-topology/sno-to-3node/templates/master.ign.j2deploy/openshift-clusters/roles/mutable-topology/sno-to-3node/vars/main.ymldeploy/openshift-clusters/scripts/clean-mutable-topology.shdeploy/openshift-clusters/scripts/sno-to-3node.shdeploy/openshift-clusters/sno-to-3node.yml
Comment on lines
+16
to
+39
| - name: Detect cluster domain from kubeconfig (best-effort) | ||
| shell: | | ||
| KUBECONFIG={{ dev_scripts_path | default('openshift-metal3/dev-scripts') }}/ocp/{{ sno_cluster_name | default('ostest') }}/auth/kubeconfig \ | ||
| oc get infrastructure cluster -o jsonpath='{.status.apiServerInternalURI}' 2>/dev/null \ | ||
| | sed 's|^https://api-int\.||; s|:6443$||' | ||
| register: detected_domain | ||
| changed_when: false | ||
| failed_when: false | ||
| ignore_errors: true | ||
|
|
||
| - name: Set cluster domain (use detected, variable override, or default) | ||
| set_fact: | ||
| sno_cluster_domain: >- | ||
| {{ sno_cluster_domain | ||
| if (sno_cluster_domain is defined and sno_cluster_domain) | ||
| else (detected_domain.stdout | trim | ||
| if (detected_domain.stdout is defined and detected_domain.stdout | trim) | ||
| else 'ostest.test.metalkube.org') }} | ||
|
|
||
| - name: Check live cluster topology | ||
| shell: | | ||
| KUBECONFIG={{ dev_scripts_path | default('openshift-metal3/dev-scripts') }}/ocp/{{ sno_cluster_name | default('ostest') }}/auth/kubeconfig \ | ||
| oc get infrastructure cluster \ | ||
| -o jsonpath='{.status.controlPlaneTopology}' 2>/dev/null || echo "Unknown" |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Honor sno_kubeconfig in the cleanup probes.
Both oc probes rebuild KUBECONFIG from dev_scripts_path/sno_cluster_name, but this role now exposes sno_kubeconfig as the override for non-standard installs. When that override is used, domain detection falls back and topology detection degrades to Unknown, so this cleanup path can make decisions against the wrong cluster state.
Suggested fix
+ - name: Resolve kubeconfig path for mutable-topology checks
+ ansible.builtin.set_fact:
+ effective_sno_kubeconfig: >-
+ {{ sno_kubeconfig
+ if (sno_kubeconfig is defined and sno_kubeconfig)
+ else (dev_scripts_path | default('openshift-metal3/dev-scripts')) ~ '/ocp/' ~ (sno_cluster_name | default('ostest')) ~ '/auth/kubeconfig' }}
+
- name: Detect cluster domain from kubeconfig (best-effort)
shell: |
- KUBECONFIG={{ dev_scripts_path | default('openshift-metal3/dev-scripts') }}/ocp/{{ sno_cluster_name | default('ostest') }}/auth/kubeconfig \
+ KUBECONFIG={{ effective_sno_kubeconfig | quote }} \
oc get infrastructure cluster -o jsonpath='{.status.apiServerInternalURI}' 2>/dev/null \
| sed 's|^https://api-int\.||; s|:6443$||'
@@
- name: Check live cluster topology
shell: |
- KUBECONFIG={{ dev_scripts_path | default('openshift-metal3/dev-scripts') }}/ocp/{{ sno_cluster_name | default('ostest') }}/auth/kubeconfig \
+ KUBECONFIG={{ effective_sno_kubeconfig | quote }} \
oc get infrastructure cluster \
-o jsonpath='{.status.controlPlaneTopology}' 2>/dev/null || echo "Unknown"📝 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: Detect cluster domain from kubeconfig (best-effort) | |
| shell: | | |
| KUBECONFIG={{ dev_scripts_path | default('openshift-metal3/dev-scripts') }}/ocp/{{ sno_cluster_name | default('ostest') }}/auth/kubeconfig \ | |
| oc get infrastructure cluster -o jsonpath='{.status.apiServerInternalURI}' 2>/dev/null \ | |
| | sed 's|^https://api-int\.||; s|:6443$||' | |
| register: detected_domain | |
| changed_when: false | |
| failed_when: false | |
| ignore_errors: true | |
| - name: Set cluster domain (use detected, variable override, or default) | |
| set_fact: | |
| sno_cluster_domain: >- | |
| {{ sno_cluster_domain | |
| if (sno_cluster_domain is defined and sno_cluster_domain) | |
| else (detected_domain.stdout | trim | |
| if (detected_domain.stdout is defined and detected_domain.stdout | trim) | |
| else 'ostest.test.metalkube.org') }} | |
| - name: Check live cluster topology | |
| shell: | | |
| KUBECONFIG={{ dev_scripts_path | default('openshift-metal3/dev-scripts') }}/ocp/{{ sno_cluster_name | default('ostest') }}/auth/kubeconfig \ | |
| oc get infrastructure cluster \ | |
| -o jsonpath='{.status.controlPlaneTopology}' 2>/dev/null || echo "Unknown" | |
| - name: Resolve kubeconfig path for mutable-topology checks | |
| ansible.builtin.set_fact: | |
| effective_sno_kubeconfig: >- | |
| {{ sno_kubeconfig | |
| if (sno_kubeconfig is defined and sno_kubeconfig) | |
| else (dev_scripts_path | default('openshift-metal3/dev-scripts')) ~ '/ocp/' ~ (sno_cluster_name | default('ostest')) ~ '/auth/kubeconfig' }} | |
| - name: Detect cluster domain from kubeconfig (best-effort) | |
| shell: | | |
| KUBECONFIG={{ effective_sno_kubeconfig | quote }} \ | |
| oc get infrastructure cluster -o jsonpath='{.status.apiServerInternalURI}' 2>/dev/null \ | |
| | sed 's|^https://api-int\.||; s|:6443$||' | |
| register: detected_domain | |
| changed_when: false | |
| failed_when: false | |
| ignore_errors: true | |
| - name: Set cluster domain (use detected, variable override, or default) | |
| set_fact: | |
| sno_cluster_domain: >- | |
| {{ sno_cluster_domain | |
| if (sno_cluster_domain is defined and sno_cluster_domain) | |
| else (detected_domain.stdout | trim | |
| if (detected_domain.stdout is defined and detected_domain.stdout | trim) | |
| else 'ostest.test.metalkube.org') }} | |
| - name: Check live cluster topology | |
| shell: | | |
| KUBECONFIG={{ effective_sno_kubeconfig | quote }} \ | |
| oc get infrastructure cluster \ | |
| -o jsonpath='{.status.controlPlaneTopology}' 2>/dev/null || echo "Unknown" |
🤖 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 `@deploy/openshift-clusters/clean-mutable-topology.yml` around lines 16 - 39,
The cleanup probes are rebuilding KUBECONFIG from dev_scripts_path and
sno_cluster_name instead of honoring the sno_kubeconfig override, which can
point them at the wrong cluster. Update the two shell probes in the cleanup
workflow to use sno_kubeconfig when it is defined, and only fall back to the
existing computed kubeconfig path when the override is absent. Keep the existing
detection logic in the domain and topology steps, but make sure both oc
invocations reference the same kubeconfig source consistently.
Comment on lines
+122
to
+124
| - name: "[preflight] Verify api-int DNS on hypervisor" | ||
| shell: | | ||
| dig +short api-int.{{ sno_cluster_domain }} @192.168.111.1 |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Stop querying a fixed DNS server here.
Both dig checks hardcode @192.168.111.1, but the repair path updates {{ sno_libvirt_network }}. On any non-default libvirt network, preflight can inspect the wrong dnsmasq instance and either repair unnecessarily or fail verification after a successful virsh net-update.
Also applies to: 158-160
🤖 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
`@deploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/preflight.yml`
around lines 122 - 124, The preflight DNS verification is hardcoded to query a
fixed dnsmasq server, which can point at the wrong libvirt network. Update the
`dig` checks in the preflight tasks to use the DNS server associated with `{{
sno_libvirt_network }}` instead of `@192.168.111.1`, and make the same change in
both the `api-int` and related verification steps so the check matches the
repair path.
Comment on lines
+8
to
+12
| "path": "/etc/master.ign", | ||
| "mode": 420, | ||
| "contents": { | ||
| "source": "data:text/plain;charset=utf-8;base64,{{ sno_master_ign_b64 }}" | ||
| } |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Tighten /etc/master.ign permissions.
master.ign can carry the rendered control-plane ignition, including pull-secret and cluster credential material. 0644 makes it world-readable in the live ISO environment for no benefit; 0600 is enough for the installer service.
Suggested fix
- "mode": 420,
+ "mode": 384,As per coding guidelines, "Handle secrets and credentials securely."
📝 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
| "path": "/etc/master.ign", | |
| "mode": 420, | |
| "contents": { | |
| "source": "data:text/plain;charset=utf-8;base64,{{ sno_master_ign_b64 }}" | |
| } | |
| "path": "/etc/master.ign", | |
| "mode": 384, | |
| "contents": { | |
| "source": "data:text/plain;charset=utf-8;base64,{{ sno_master_ign_b64 }}" | |
| } |
🤖 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
`@deploy/openshift-clusters/roles/mutable-topology/sno-to-3node/templates/auto-install.ign.j2`
around lines 8 - 12, The `/etc/master.ign` entry in `auto-install.ign.j2` is too
permissive for rendered ignition content that may include sensitive
control-plane credentials. Update the file mode for this object to restrict
access to the installer service only, keeping the change localized to the
`/etc/master.ign` stanza and preserving the existing `contents.source` handling.
Source: Coding guidelines
eggfoobar
force-pushed
the
add-migration-support
branch
from
f8eef94 to
c5d09b6
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
deploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/boot-nodes.yml (1)
80-81: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winKeep
virsh undefinealigned with UEFI/NVRAM guests.This is still missing
--nvram; with the OVMF varstore configured byvirt-install, undefine can fail and leave the old domain behind before the next create.Suggested fix
sudo virsh destroy "$VM_NAME" 2>/dev/null || true - sudo virsh undefine "$VM_NAME" 2>/dev/null || true + sudo virsh undefine "$VM_NAME" --nvram 2>/dev/null || true🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/boot-nodes.yml` around lines 80 - 81, Update the cleanup in the boot-nodes task so `virsh undefine` handles UEFI/NVRAM guests created by `virt-install`; specifically, in the `boot-nodes.yml` teardown block where `sudo virsh destroy` and `sudo virsh undefine` run, add the NVRAM-aware option to the `virsh undefine` call so the old domain is fully removed before recreation.
🧹 Nitpick comments (1)
deploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/boot-nodes.yml (1)
60-68: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPreserve idempotent task status for the installer check.
This task reports changed even when
coreos-installeris already installed. Addchanged_whenso reruns don’t show a false mutation.Suggested fix
- name: "[boot] Ensure coreos-installer is available" shell: | if ! command -v coreos-installer &>/dev/null; then echo "coreos-installer not found, installing via dnf..." sudo dnf install -y coreos-installer else echo "coreos-installer already installed: $(command -v coreos-installer)" fi + register: coreos_installer_check + changed_when: "'installing via dnf' in coreos_installer_check.stdout"As per coding guidelines, deployments should be idempotent.
🤖 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 `@deploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/boot-nodes.yml` around lines 60 - 68, The boot-nodes.yml task that checks for coreos-installer is reporting changed even on no-op reruns. Update the shell task named "[boot] Ensure coreos-installer is available" to set changed_when so it only reports changed when the dnf install branch runs, and stays unchanged when command -v coreos-installer already succeeds. Make sure the task remains idempotent while preserving the current install behavior.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.
Duplicate comments:
In
`@deploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/boot-nodes.yml`:
- Around line 80-81: Update the cleanup in the boot-nodes task so `virsh
undefine` handles UEFI/NVRAM guests created by `virt-install`; specifically, in
the `boot-nodes.yml` teardown block where `sudo virsh destroy` and `sudo virsh
undefine` run, add the NVRAM-aware option to the `virsh undefine` call so the
old domain is fully removed before recreation.
---
Nitpick comments:
In
`@deploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/boot-nodes.yml`:
- Around line 60-68: The boot-nodes.yml task that checks for coreos-installer is
reporting changed even on no-op reruns. Update the shell task named "[boot]
Ensure coreos-installer is available" to set changed_when so it only reports
changed when the dnf install branch runs, and stays unchanged when command -v
coreos-installer already succeeds. Make sure the task remains idempotent while
preserving the current install behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 2c26fbf7-9252-4826-94ce-3ec2d575b695
📒 Files selected for processing (1)
deploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/boot-nodes.yml
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
deploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/topology.yml (2)
68-73: 🗄️ Data Integrity & Integration | 🔵 TrivialManual status patch bypasses the controller that normally owns this field.
Directly patching
infrastructure/cluster.status(as the commit message notes, "simulating controller" until CCO integration exists) risks being overwritten if any controller later reconciles this resource and expects a different flow to set status, or races with a controller that also writes to this subresource. This is explicitly called out as temporary, so flagging for tracking rather than as a blocking issue — worth a TODO/tracking reference to the follow-up CCO integration work so it isn't forgotten.🤖 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 `@deploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/topology.yml` around lines 68 - 73, The manual status patch in the topology task is a temporary controller bypass and should be tracked explicitly so it is not forgotten. Update the existing topology patch step (the one that runs the oc patch against infrastructure/cluster status) to include a clear TODO or tracking reference for the follow-up CCO integration work, and keep the note tied to the topology_cmd_check flow so the temporary nature is obvious in the task definition.
3-4: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valuePrefer
commandmodule overshellfor static, non-piped invocations.None of these commands use shell features (pipes, redirection, globbing, or variable expansion requiring a shell), so
ansible.builtin.commandwould be a safer, lower-overhead choice thanshell. Theoc patchtask on lines 69-71 legitimately needsshellfor its quoted JSON payload, but the others (help check, preflight, transition initiate, spec/status get) don't.Also applies to: 16-17, 37-38, 57-58, 76-77
🤖 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 `@deploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/topology.yml` around lines 3 - 4, Replace the static `shell` invocations in the topology tasks with `ansible.builtin.command` where no shell features are needed. Update the `oc adm transition topology --help`, preflight, transition initiate, and spec/status get tasks to use `command` instead of `shell`, keeping `shell` only for the `oc patch` task with the quoted JSON payload. Use the task names/commands in `topology.yml` to locate each affected block.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@deploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/topology.yml`:
- Around line 68-73: The manual status patch in the topology task is a temporary
controller bypass and should be tracked explicitly so it is not forgotten.
Update the existing topology patch step (the one that runs the oc patch against
infrastructure/cluster status) to include a clear TODO or tracking reference for
the follow-up CCO integration work, and keep the note tied to the
topology_cmd_check flow so the temporary nature is obvious in the task
definition.
- Around line 3-4: Replace the static `shell` invocations in the topology tasks
with `ansible.builtin.command` where no shell features are needed. Update the
`oc adm transition topology --help`, preflight, transition initiate, and
spec/status get tasks to use `command` instead of `shell`, keeping `shell` only
for the `oc patch` task with the quoted JSON payload. Use the task
names/commands in `topology.yml` to locate each affected block.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: a3ebbebd-efc9-40a8-81e2-df4f3ac00b88
📒 Files selected for processing (3)
deploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/topology.ymldeploy/openshift-clusters/roles/mutable-topology/sno-to-3node/vars/main.ymldeploy/openshift-clusters/sno-to-3node.yml
🚧 Files skipped from review as they are similar to previous changes (2)
- deploy/openshift-clusters/sno-to-3node.yml
- deploy/openshift-clusters/roles/mutable-topology/sno-to-3node/vars/main.yml
openshift-ci
Bot
added
the
needs-rebase
Add a new Ansible role and make target to perform a one-shot migration of a platform:None SNO cluster to a 3-node HA cluster. The role creates two new control-plane VMs, boots them with RHCOS via coreos-installer ISO ignition embed, waits for nodes to join and etcd to scale, then transitions the topology to HighlyAvailable. Usage: make sno-to-3node Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: ehila <ehila@redhat.com>
pull rhcos-live.iso from payload if not present install coreos-installer if not present Signed-off-by: ehila <ehila@redhat.com>
eggfoobar
force-pushed
the
add-migration-support
branch
from
d334b22 to
5f7b69d
Compare
openshift-ci
Bot
removed
the
needs-rebase
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@deploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/clean.yml`:
- Around line 2-8: Update the “[clean] Stop and undefine new VMs” task to stop
suppressing failures from `virsh undefine`; preserve the existing destroy
handling if needed, but allow undefine errors to fail the cleanup before the
subsequent disk-removal task runs. Keep the change scoped to the shell commands
and task error behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 6338c6e7-8221-4182-b0fb-45a98b32b4d3
📒 Files selected for processing (21)
deploy/Makefiledeploy/openshift-clusters/clean-mutable-topology.ymldeploy/openshift-clusters/roles/mutable-topology/sno-to-3node/defaults/main.ymldeploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/boot-nodes.ymldeploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/clean.ymldeploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/create-vms.ymldeploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/ignition.ymldeploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/main.ymldeploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/mco-rollout.ymldeploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/preflight.ymldeploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/topology.ymldeploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/update-dns.ymldeploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/verify.ymldeploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/wait-etcd.ymldeploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/wait-nodes.ymldeploy/openshift-clusters/roles/mutable-topology/sno-to-3node/templates/auto-install.ign.j2deploy/openshift-clusters/roles/mutable-topology/sno-to-3node/templates/master.ign.j2deploy/openshift-clusters/roles/mutable-topology/sno-to-3node/vars/main.ymldeploy/openshift-clusters/scripts/clean-mutable-topology.shdeploy/openshift-clusters/scripts/sno-to-3node.shdeploy/openshift-clusters/sno-to-3node.yml
🚧 Files skipped from review as they are similar to previous changes (13)
- deploy/openshift-clusters/sno-to-3node.yml
- deploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/ignition.yml
- deploy/openshift-clusters/scripts/sno-to-3node.sh
- deploy/openshift-clusters/roles/mutable-topology/sno-to-3node/templates/auto-install.ign.j2
- deploy/openshift-clusters/scripts/clean-mutable-topology.sh
- deploy/openshift-clusters/roles/mutable-topology/sno-to-3node/templates/master.ign.j2
- deploy/openshift-clusters/roles/mutable-topology/sno-to-3node/defaults/main.yml
- deploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/wait-etcd.yml
- deploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/wait-nodes.yml
- deploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/create-vms.yml
- deploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/preflight.yml
- deploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/boot-nodes.yml
- deploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/main.yml
Comment on lines
+2
to
+8
| - name: "[clean] Stop and undefine new VMs" | ||
| shell: | | ||
| sudo virsh destroy {{ item.name }} 2>/dev/null || true | ||
| sudo virsh undefine {{ item.name }} --remove-all-storage 2>/dev/null || true | ||
| loop: "{{ sno_new_nodes }}" | ||
| failed_when: false | ||
| changed_when: true |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Do not mask failed VM undefines before deleting their disks.
Line 5 suppresses virsh undefine failures, but the following task removes the qcow2 anyway. A libvirt failure can leave a defined VM pointing to a deleted disk while cleanup reports success.
Proposed fix
- sudo virsh destroy {{ item.name }} 2>/dev/null || true
- sudo virsh undefine {{ item.name }} --remove-all-storage 2>/dev/null || true
+ if sudo virsh dominfo {{ item.name }} >/dev/null 2>&1; then
+ sudo virsh destroy {{ item.name }} >/dev/null 2>&1 || true
+ sudo virsh undefine {{ item.name }} --remove-all-storage
+ fi
loop: "{{ sno_new_nodes }}"
- failed_when: false
changed_when: true📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| - name: "[clean] Stop and undefine new VMs" | |
| shell: | | |
| sudo virsh destroy {{ item.name }} 2>/dev/null || true | |
| sudo virsh undefine {{ item.name }} --remove-all-storage 2>/dev/null || true | |
| loop: "{{ sno_new_nodes }}" | |
| failed_when: false | |
| changed_when: true | |
| - name: "[clean] Stop and undefine new VMs" | |
| shell: | | |
| if sudo virsh dominfo {{ item.name }} >/dev/null 2>&1; then | |
| sudo virsh destroy {{ item.name }} >/dev/null 2>&1 || true | |
| sudo virsh undefine {{ item.name }} --remove-all-storage | |
| fi | |
| loop: "{{ sno_new_nodes }}" | |
| changed_when: true |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@deploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/clean.yml`
around lines 2 - 8, Update the “[clean] Stop and undefine new VMs” task to stop
suppressing failures from `virsh undefine`; preserve the existing destroy
handling if needed, but allow undefine errors to fail the cleanup before the
subsequent disk-removal task runs. Keep the change scoped to the shell commands
and task error behavior.
- avoid cleanup if HA already set unless forced - add comment about cluster name - add cleanup for cluster ip Signed-off-by: ehila <ehila@redhat.com>
- handlers/main.yml: emit explicit WARNING debug if oc project fails after all retries, keeping failed_when: false to avoid play abortion - tasks/config.yml: add SNO_IPV4 to the AGENT_E2E_TEST_SCENARIO guidance message so all three supported scenarios are listed - boot-nodes.yml: add --nvram to virsh undefine so UEFI varstore is fully removed before virt-install recreates the domain - clean.yml: remove NetworkManager dnsmasq override file and reload NM after libvirt dnsmasq reload to clear stale local DNS entries - create-vms.yml: remove failed_when: false from virsh net-update dns-host add so DNS update failures surface instead of being silently swallowed - ignition.yml: add --request-timeout=30s to oc get configmap and --max-time 10 to curl so probes fail quickly on API/DNS stalls - mco-rollout.yml: tighten drain auto-fix gate to only fire on nodes with a stale MCO desiredDrain annotation (drain-* prefix, != lastAppliedDrain); change sno_auto_fix_drain default to false (opt-in); restart MCC only once per cycle rather than unconditionally - preflight.yml: indent Python snippet inside shell: | block to column 4+ so all lines remain within the block scalar per YAML spec - update-dns.yml: tighten API DNS check to require a known node IP via intersect filter; replace failed_when: false on apps check with a real assertion against sno_all_node_ips[0]; add retries to both checks - verify.yml: add Assert all nodes Ready assertion before the success message so NotReady nodes cause a hard failure instead of passing silently Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: ehila <ehila@redhat.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
deploy/openshift-clusters/clean-mutable-topology.yml (2)
35-43:⚠️ Potential issue | 🟠 MajorHonor the effective kubeconfig override.
This topology probe rebuilds
KUBECONFIGfromdev_scripts_pathandsno_cluster_nameinstead of usingsno_kubeconfig. Non-standard installs can therefore inspect the wrong cluster before destructive cleanup. Reuse one override-aware kubeconfig path for both cleanup probes.🤖 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 `@deploy/openshift-clusters/clean-mutable-topology.yml` around lines 35 - 43, The “Check live cluster topology” task should honor the effective kubeconfig override instead of reconstructing it from dev_scripts_path and sno_cluster_name. Reuse the same override-aware kubeconfig path used by the other cleanup probe, specifically sno_kubeconfig when provided, so both probes target the same cluster.
45-53:⚠️ Potential issue | 🟠 MajorFail closed when topology detection is unknown.
Because the probe suppresses errors,
Unknown, empty, or malformed topology values currently bypass this guard and proceed to destructive cleanup. Allow cleanup only for an explicitSingleReplicatopology unlessforce_cleanup=true.Suggested guard
when: - - live_topology.stdout | trim == "HighlyAvailable" - not (force_cleanup | default(false) | bool) + - live_topology.stdout | trim != "SingleReplica"🤖 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 `@deploy/openshift-clusters/clean-mutable-topology.yml` around lines 45 - 53, Update the “Abort if cluster is HA and force_cleanup is not set” guard to fail closed: when force_cleanup is false, permit destructive cleanup only if the trimmed live_topology.stdout equals exactly “SingleReplica”. Ensure Unknown, empty, malformed, and HighlyAvailable values all trigger ansible.builtin.fail, while preserving the force_cleanup=true override.
🤖 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
`@deploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/update-dns.yml`:
- Around line 62-69: Update the `failed_when` and `until` conditions for
`dns_api_verify` so they require every IP in `sno_all_node_ips` to be present in
the normalized DNS response, rather than accepting any intersection; preserve
the non-empty response check and ensure both conditions use the same
complete-list comparison.
---
Duplicate comments:
In `@deploy/openshift-clusters/clean-mutable-topology.yml`:
- Around line 35-43: The “Check live cluster topology” task should honor the
effective kubeconfig override instead of reconstructing it from dev_scripts_path
and sno_cluster_name. Reuse the same override-aware kubeconfig path used by the
other cleanup probe, specifically sno_kubeconfig when provided, so both probes
target the same cluster.
- Around line 45-53: Update the “Abort if cluster is HA and force_cleanup is not
set” guard to fail closed: when force_cleanup is false, permit destructive
cleanup only if the trimmed live_topology.stdout equals exactly “SingleReplica”.
Ensure Unknown, empty, malformed, and HighlyAvailable values all trigger
ansible.builtin.fail, while preserving the force_cleanup=true override.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: e7ae353b-645c-455a-a98b-3cac5d3aa7f9
📒 Files selected for processing (12)
deploy/openshift-clusters/clean-mutable-topology.ymldeploy/openshift-clusters/roles/dev-scripts/install-dev/handlers/main.ymldeploy/openshift-clusters/roles/dev-scripts/install-dev/tasks/config.ymldeploy/openshift-clusters/roles/mutable-topology/sno-to-3node/defaults/main.ymldeploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/boot-nodes.ymldeploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/clean.ymldeploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/create-vms.ymldeploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/ignition.ymldeploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/mco-rollout.ymldeploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/preflight.ymldeploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/update-dns.ymldeploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/verify.yml
💤 Files with no reviewable changes (1)
- deploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/create-vms.yml
🚧 Files skipped from review as they are similar to previous changes (7)
- deploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/ignition.yml
- deploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/clean.yml
- deploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/verify.yml
- deploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/mco-rollout.yml
- deploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/preflight.yml
- deploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/boot-nodes.yml
- deploy/openshift-clusters/roles/mutable-topology/sno-to-3node/defaults/main.yml
Comment on lines
+62
to
+69
| failed_when: >- | ||
| dns_api_verify.stdout | trim == "" or | ||
| sno_all_node_ips | intersect(dns_api_verify.stdout_lines) | length == 0 | ||
| retries: 6 | ||
| delay: 5 | ||
| until: >- | ||
| dns_api_verify.stdout | trim != "" and | ||
| sno_all_node_ips | intersect(dns_api_verify.stdout_lines) | length > 0 |
Contributor
There was a problem hiding this comment.
Require API DNS to contain all expected node IPs.
The current condition succeeds when only one IP intersects the response, so stale or partially loaded dnsmasq configuration can still pass. Compare the normalized DNS result with the complete sno_all_node_ips list in both failed_when and until.
Suggested tightening
- failed_when: >-
- dns_api_verify.stdout | trim == "" or
- sno_all_node_ips | intersect(dns_api_verify.stdout_lines) | length == 0
+ failed_when: >-
+ (dns_api_verify.stdout_lines | sort) != (sno_all_node_ips | sort)
...
- until: >-
- dns_api_verify.stdout | trim != "" and
- sno_all_node_ips | intersect(dns_api_verify.stdout_lines) | length > 0
+ until: >-
+ (dns_api_verify.stdout_lines | sort) == (sno_all_node_ips | sort)🤖 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
`@deploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/update-dns.yml`
around lines 62 - 69, Update the `failed_when` and `until` conditions for
`dns_api_verify` so they require every IP in `sno_all_node_ips` to be present in
the normalized DNS response, rather than accepting any intersection; preserve
the non-empty response check and ensure both conditions use the same
complete-list comparison.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit