fix: improve sno-to-3node playbook reliability and idempotency + CR feedback#96
fix: improve sno-to-3node playbook reliability and idempotency + CR feedback#96jaypoulz wants to merge 2 commits into
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jaypoulz 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
|
@coderabbitai review |
WalkthroughThe SNO-to-3node transition workflow now uses readiness-based etcd checks, deterministic control-plane IP discovery, CCO topology fallback handling, HAProxy/DNS idempotency detection, stricter ingress validation, and non-suppressed DNS verification failures. ChangesSNO-to-3node transition
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Ansible
participant OpenShiftCLI
participant CCO
Ansible->>OpenShiftCLI: Check topology-transition command
OpenShiftCLI-->>Ansible: Command unavailable
Ansible->>CCO: Read topology transition state
CCO-->>Ansible: Return topology and progressing condition
Ansible->>Ansible: Classify state and continue or fail
Possibly related PRs
Suggested labels: Suggested reviewers: Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 1 warning)
✅ Passed checks (9 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/haproxy-setup.yml`:
- Line 225: Replace the HAProxy-only completion condition around
haproxy_configured_check with a comprehensive migration completion check that
independently validates stale /etc/hosts entries and the live libvirt network
DNS configuration. Ensure retries rerun any incomplete DNS migration tasks even
when HAProxy is already configured and running.
- Around line 8-12: Update the reuse probe in the haproxy setup task to search
for a marker that is actually rendered into /etc/haproxy/haproxy.cfg, replacing
the sno_cluster_domain check while preserving the existing three-master backend
validation. Use a stable configuration value or directive emitted by the
rendering block so valid existing configurations skip the disruptive
reconfiguration path.
In
`@deploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/main.yml`:
- Around line 14-15: Update the jq pod-readiness predicates in
deploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/main.yml
lines 14-15,
deploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/ingress-reconfigure.yml
lines 50-51, and
deploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/verify.yml
lines 53-54 to count each Running pod only when every container status has ready
== true, producing one count per pod rather than matching individual ready
containers.
In
`@deploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/preflight.yml`:
- Around line 76-84: Update the sno_cp_ips Jinja filtering expression to test
membership of "node-role.kubernetes.io/master" directly in each node’s
metadata.labels map, replacing the nested selectattr lookup. Preserve the
existing sorting and InternalIP extraction pipeline.
In
`@deploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/topology.yml`:
- Around line 35-36: Update the fallback handling around the topology status
command so an indeterminate result does not continue into the unconditional
success message. Poll until a terminal state is reached, or explicitly fail when
the status remains unknown, while preserving the existing failure and skip
transitions.
🪄 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: ee170393-fedb-43cd-a1a1-7469891de4e3
📒 Files selected for processing (7)
deploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/dns-transition.ymldeploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/haproxy-setup.ymldeploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/ingress-reconfigure.ymldeploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/main.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/verify.yml
💤 Files with no reviewable changes (1)
- deploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/dns-transition.yml
jaypoulz
force-pushed
the
add-oc-transition-command
branch
from
98fc421 to
e4cf3b2
Compare
Critical bug fixes: - Surface DNS verification failures instead of silently succeeding - Count only healthy etcd pods (Running + Ready) in main and verify phases - Delete ingress-operator pod before IngressController to force Infrastructure resync - Add proper error handling for oc/jq pipeline failures Improvements: - Add CCO fallback when oc adm transition topology unavailable - Make HAProxy setup idempotent with configuration checks - Gather control plane IPs in stable sorted order (no dependency on node[0]) - Add explicit firewalld zone parameter - Document NetworkManager dnsmasq file ownership Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
jaypoulz
force-pushed
the
add-oc-transition-command
branch
from
e4cf3b2 to
f635392
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/haproxy-setup.yml`:
- Around line 4-5: Update the HAProxy validation command in the migration setup
to require both an active and enabled systemd service, rather than checking only
runtime activity. Ensure the migration fails unless HAProxy is configured to
start automatically after reboot.
In
`@deploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/ingress-reconfigure.yml`:
- Around line 39-46: Update the ingress-operator deletion task to propagate
failures instead of suppressing them: remove the unconditional failure override
from the task registering delete_operator_pod. Keep the existing deletion
command, change detection, and needs_deletion condition unchanged so a failed
deletion stops the workflow before the wait and IngressController recreation
steps.
In
`@deploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/preflight.yml`:
- Around line 93-97: Update the set_fact block assigning sno_master1_ip and
sno_master2_ip to preserve the configured role input values from
defaults/main.yml when sno_cp_ips has fewer than three entries. Replace the
hardcoded fallback literals with the existing variables or dedicated fallback
variables, while keeping provided sno_cp_ips entries authoritative and avoiding
an unintended second source of truth.
- Around line 86-91: Update the control-plane IP validation in the preflight
assertion so it requires exactly one IP for a one-node cluster and exactly three
IPs for a three-node cluster, using the cluster topology value already available
in the task context. Remove the permissive 1–3 range that allows partial
membership, while preserving the existing failure message’s discovered-count
reporting.
- Around line 74-91: Update the control-plane IP extraction in the “Gather
control plane node IPs in stable order” task to exclude masters whose InternalIP
lookup yields no defined address, before populating sno_cp_ips. Ensure the
subsequent “Assert we have control plane IPs” check counts only valid IPs so
sno_master0_ip cannot receive an undefined value.
🪄 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: 164b47ba-c02b-47d1-a686-13ee6a1d536d
📒 Files selected for processing (7)
deploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/dns-transition.ymldeploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/haproxy-setup.ymldeploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/ingress-reconfigure.ymldeploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/main.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/verify.yml
💤 Files with no reviewable changes (1)
- deploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/dns-transition.yml
🚧 Files skipped from review as they are similar to previous changes (3)
- deploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/topology.yml
- deploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/main.yml
- deploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/verify.yml
| # Check if haproxy is running | ||
| systemctl is-active haproxy 2>/dev/null || exit 1 |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Require HAProxy to be enabled before declaring migration complete.
An active but disabled service passes this probe, skips the enabled: true task, and will not return after a reboot.
Proposed fix
systemctl is-active haproxy 2>/dev/null || exit 1
+ systemctl is-enabled --quiet haproxy 2>/dev/null || exit 1📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Check if haproxy is running | |
| systemctl is-active haproxy 2>/dev/null || exit 1 | |
| # Check if haproxy is running | |
| systemctl is-active haproxy 2>/dev/null || exit 1 | |
| systemctl is-enabled --quiet haproxy 2>/dev/null || exit 1 |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@deploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/haproxy-setup.yml`
around lines 4 - 5, Update the HAProxy validation command in the migration setup
to require both an active and enabled systemd service, rather than checking only
runtime activity. Ensure the migration fails unless HAProxy is configured to
start automatically after reboot.
| - name: "[ingress] Delete ingress-operator pod to force Infrastructure resync" | ||
| shell: | | ||
| oc delete pod -n openshift-ingress-operator -l name=ingress-operator | ||
| environment: "{{ sno_oc_env }}" | ||
| register: delete_operator_pod | ||
| changed_when: delete_operator_pod.rc == 0 | ||
| failed_when: false | ||
| when: "'needs_deletion=true' in ingress_check.stdout" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not suppress ingress-operator deletion failures.
If deletion fails while the existing pod remains ready, the wait task succeeds and the IngressController is recreated without the intended Infrastructure resync. Let the delete failure stop the workflow.
Proposed fix
register: delete_operator_pod
changed_when: delete_operator_pod.rc == 0
- failed_when: false
when: "'needs_deletion=true' in ingress_check.stdout"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: "[ingress] Delete ingress-operator pod to force Infrastructure resync" | |
| shell: | | |
| oc delete pod -n openshift-ingress-operator -l name=ingress-operator | |
| environment: "{{ sno_oc_env }}" | |
| register: delete_operator_pod | |
| changed_when: delete_operator_pod.rc == 0 | |
| failed_when: false | |
| when: "'needs_deletion=true' in ingress_check.stdout" | |
| - name: "[ingress] Delete ingress-operator pod to force Infrastructure resync" | |
| shell: | | |
| oc delete pod -n openshift-ingress-operator -l name=ingress-operator | |
| environment: "{{ sno_oc_env }}" | |
| register: delete_operator_pod | |
| changed_when: delete_operator_pod.rc == 0 | |
| when: "'needs_deletion=true' in ingress_check.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/ingress-reconfigure.yml`
around lines 39 - 46, Update the ingress-operator deletion task to propagate
failures instead of suppressing them: remove the unconditional failure override
from the task registering delete_operator_pod. Keep the existing deletion
command, change detection, and needs_deletion condition unchanged so a failed
deletion stops the workflow before the wait and IngressController recreation
steps.
| - name: "[preflight] Gather control plane node IPs in stable order" | ||
| set_fact: | ||
| sno_master0_ip: >- | ||
| {{ sno_master0_ip if sno_master0_ip | ||
| else (sno_nodes[0].status.addresses | ||
| | selectattr('type', 'equalto', 'InternalIP') | ||
| | map(attribute='address') | first) }} | ||
| sno_cp_ips: >- | ||
| {{ sno_nodes | ||
| | selectattr('metadata.labels["node-role.kubernetes.io/master"]', 'defined') | ||
| | sort(attribute='metadata.name') | ||
| | map(attribute='status.addresses') | ||
| | map('selectattr', 'type', 'equalto', 'InternalIP') | ||
| | map('map', attribute='address') | ||
| | map('first') | ||
| | list }} | ||
|
|
||
| - name: "[preflight] Assert we have control plane IPs" | ||
| assert: | ||
| that: | ||
| - sno_cp_ips | length >= 1 | ||
| - sno_cp_ips | length <= 3 | ||
| fail_msg: "Expected 1-3 control plane IPs, found {{ sno_cp_ips | length }}" |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
set -euo pipefail
printf '--- preflight.yml slice ---\n'
nl -ba deploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/preflight.yml | sed -n '1,170p'
printf '\n--- search for sno_cp_ips and sno_master ip consumers ---\n'
rg -n "sno_cp_ips|sno_master[0-9_]*ip|sno_master.*ip" deploy/openshift-clusters/roles/mutable-topology/sno-to-3node -SRepository: openshift-eng/two-node-toolbox
Length of output: 259
🏁 Script executed:
set -euo pipefail
pwd
lsRepository: openshift-eng/two-node-toolbox
Length of output: 302
🌐 Web query:
Jinja first filter empty sequence returns undefined
💡 Result:
When the Jinja 'first' filter is applied to an empty sequence, it returns a Jinja 'Undefined' object [1][2][3]. This is an intentional design choice to indicate that no value was found, rather than raising an immediate runtime exception [4][5]. By returning an 'Undefined' object, the filter allows for flexible handling of missing data. For example, you can chain the 'default' filter after 'first' to provide a fallback value if the sequence is empty [6]. Example usage: {{ my_list|first|default('fallback_value') }} In this case, if 'my_list' is empty, 'first' returns an 'Undefined' object, and the 'default' filter replaces it with 'fallback_value' [6]. Note that if you attempt to use an 'Undefined' object in a way that requires a concrete value (such as printing it or performing operations on it) without providing a default, Jinja will typically raise a 'jinja2.exceptions.UndefinedError' [6].
Citations:
- 1: https://jinja.palletsprojects.com/en/stable/api/
- 2: https://github.com/pallets/jinja/blob/main/src/jinja2/filters.py
- 3: https://github.com/pallets/jinja/blob/3.0.3/src/jinja2/filters.py
- 4: https://jinja.app/documentation/jinja-filters/jinja-first-filter-function-to-retrieve-first-item
- 5: https://digitalpomegranate.com/jinja-first-filter-function-to-retrieve-first-item/
- 6: Should Filters that work on lists should check for null value? pallets/jinja#1520
🏁 Script executed:
set -euo pipefail
echo '--- preflight.yml slice ---'
nl -ba deploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/preflight.yml | sed -n '1,170p'
echo
echo '--- search for sno_cp_ips and downstream IP consumers ---'
rg -n "sno_cp_ips|sno_master[0-9_]*ip|sno_master.*ip|InternalIP" deploy/openshift-clusters/roles/mutable-topology/sno-to-3node -SRepository: openshift-eng/two-node-toolbox
Length of output: 236
🏁 Script executed:
set -euo pipefail
echo '--- preflight.yml slice ---'
sed -n '1,170p' deploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/preflight.yml | cat -n
echo
echo '--- search for sno_cp_ips and downstream IP consumers ---'
rg -n "sno_cp_ips|sno_master[0-9_]*ip|sno_master.*ip|InternalIP" deploy/openshift-clusters/roles/mutable-topology/sno-to-3node -SRepository: openshift-eng/two-node-toolbox
Length of output: 12729
Reject masters without an InternalIP. first returns an undefined value for an empty address list, so a master without status.addresses[].type == InternalIP can still pass the length check and poison sno_master0_ip. Filter those nodes out or assert every extracted address is defined.
🤖 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 74 - 91, Update the control-plane IP extraction in the “Gather
control plane node IPs in stable order” task to exclude masters whose InternalIP
lookup yields no defined address, before populating sno_cp_ips. Ensure the
subsequent “Assert we have control plane IPs” check counts only valid IPs so
sno_master0_ip cannot receive an undefined value.
| - name: "[preflight] Assert we have control plane IPs" | ||
| assert: | ||
| that: | ||
| - sno_cp_ips | length >= 1 | ||
| - sno_cp_ips | length <= 3 | ||
| fail_msg: "Expected 1-3 control plane IPs, found {{ sno_cp_ips | length }}" |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Reject partial control-plane membership on three-node clusters.
A three-node cluster can pass with only two discovered control-plane IPs. Lines 96-97 then fill the missing slot with a fallback address, allowing HAProxy and libvirt to target an undiscovered node. Require exactly one control-plane IP for a one-node cluster and exactly three for a three-node cluster.
🤖 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 86 - 91, Update the control-plane IP validation in the preflight
assertion so it requires exactly one IP for a one-node cluster and exactly three
IPs for a three-node cluster, using the cluster topology value already available
in the task context. Remove the permissive 1–3 range that allows partial
membership, while preserving the existing failure message’s discovered-count
reporting.
| - name: "[preflight] Assign control plane IPs to master variables" | ||
| set_fact: | ||
| sno_master0_ip: "{{ sno_cp_ips[0] }}" | ||
| sno_master1_ip: "{{ sno_cp_ips[1] if sno_cp_ips | length > 1 else '192.168.111.21' }}" | ||
| sno_master2_ip: "{{ sno_cp_ips[2] if sno_cp_ips | length > 2 else '192.168.111.22' }}" |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Preserve configured fallback IPs.
defaults/main.yml exposes sno_master1_ip and sno_master2_ip as role inputs, but these lines replace them with hardcoded values whenever fewer than three addresses are found. That can assign unintended IPs to new VMs and downstream HAProxy/libvirt configuration. Use the existing variables, or dedicated fallback variables, instead of duplicating literals. set_fact creates high-precedence host variables, making this second source of truth especially risky. (docs.ansible.com)
🤖 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 93 - 97, Update the set_fact block assigning sno_master1_ip and
sno_master2_ip to preserve the configured role input values from
defaults/main.yml when sno_cp_ips has fewer than three entries. Replace the
hardcoded fallback literals with the existing variables or dedicated fallback
variables, while keeping provided sno_cp_ips entries authoritative and avoiding
an unintended second source of truth.
…e-topology Improve user experience by showing how to use EXTRA_ARGS with make: - Add inline examples in help output for sno-to-3node and clean-mutable-topology - Add Advanced Usage section explaining EXTRA_ARGS for Ansible commands - Update topology.yml stop message to show how to pass stop_before_transition_confirm This addresses confusion when users see variable names in error messages but don't know how to pass them via make commands. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
deploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/ingress-reconfigure.yml (1)
39-58: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winIngress-operator pod deletion failures are still suppressed, undermining the new wait step.
Line 45 keeps
failed_when: falseon the pod-deletion task. If the delete fails (e.g., permissions, pod already gone, API hiccup) while the existing pod remains Running/Ready, the task still reports success. The newly added wait task (Lines 48-58) then checks the same label selector for a Running+Ready pod — which the still-present old pod already satisfies — so the workflow proceeds as if the forced Infrastructure resync happened, when it never did.🐛 Proposed fix
register: delete_operator_pod changed_when: delete_operator_pod.rc == 0 - failed_when: false when: "'needs_deletion=true' in ingress_check.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/ingress-reconfigure.yml` around lines 39 - 58, The ingress-operator pod deletion task must no longer suppress failures. Update the task registering delete_operator_pod to remove failed_when: false or otherwise fail when the oc delete command fails, while preserving its existing conditional execution and changed_when behavior; keep the operator_pod_check wait task unchanged.deploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/haproxy-setup.yml (1)
4-5: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winHAProxy "enabled" state still not checked — and now the enable task is skipped entirely.
Line 5 only checks
systemctl is-active haproxy; an active-but-disabled service still passes this probe. This was flagged previously, but the impact is now worse: Line 224 gates theenabled: truetask onnot haproxy_migration_complete, so once the probe reports "complete," the task that would actually enable the service is skipped — the disabled service will never come back after a reboot.🔒️ Proposed fix
systemctl is-active haproxy 2>/dev/null || exit 1 + systemctl is-enabled --quiet haproxy 2>/dev/null || exit 1Also applies to: 223-230
🤖 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/haproxy-setup.yml` around lines 4 - 5, Update the HAProxy readiness probe to validate both active and enabled systemd states, not only that the service is running. Adjust the task guarded by haproxy_migration_complete so the enabled: true remediation still runs whenever HAProxy is disabled, including after migration is marked complete. Preserve the existing migration behavior when the service is already active and enabled.
🤖 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/haproxy-setup.yml`:
- Around line 21-34: Update the libvirt wildcard-DNS check in the “[haproxy]
Check if DNS migration is complete” task so its grep pattern matches the
rendered address format, including the slash between “address=” and “apps.”
while preserving the escaped sno_cluster_domain value. Keep the stale /etc/hosts
check and existing dns_migration_check behavior unchanged.
---
Duplicate comments:
In
`@deploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/haproxy-setup.yml`:
- Around line 4-5: Update the HAProxy readiness probe to validate both active
and enabled systemd states, not only that the service is running. Adjust the
task guarded by haproxy_migration_complete so the enabled: true remediation
still runs whenever HAProxy is disabled, including after migration is marked
complete. Preserve the existing migration behavior when the service is already
active and enabled.
In
`@deploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/ingress-reconfigure.yml`:
- Around line 39-58: The ingress-operator pod deletion task must no longer
suppress failures. Update the task registering delete_operator_pod to remove
failed_when: false or otherwise fail when the oc delete command fails, while
preserving its existing conditional execution and changed_when behavior; keep
the operator_pod_check wait task 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: 10cb3b18-180f-4687-b0a0-2dcc5dabe5eb
📒 Files selected for processing (7)
deploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/dns-transition.ymldeploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/haproxy-setup.ymldeploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/ingress-reconfigure.ymldeploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/main.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/verify.yml
💤 Files with no reviewable changes (1)
- deploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/dns-transition.yml
🚧 Files skipped from review as they are similar to previous changes (4)
- deploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/main.yml
- deploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/preflight.yml
- deploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/topology.yml
- deploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/verify.yml
| - name: "[haproxy] Check if DNS migration is complete" | ||
| shell: | | ||
| # Check /etc/hosts has no stale apps entries | ||
| ! grep -q "apps\.{{ sno_cluster_domain | regex_escape() }}" /etc/hosts 2>/dev/null || exit 1 | ||
|
|
||
| # Check libvirt network has wildcard DNS for apps | ||
| virsh net-dumpxml {{ sno_libvirt_network }} 2>/dev/null | \ | ||
| grep -q "address=/.apps.{{ sno_cluster_domain | regex_escape() }}" || exit 1 | ||
|
|
||
| # All checks passed | ||
| exit 0 | ||
| register: dns_migration_check | ||
| changed_when: false | ||
| failed_when: false |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
line="<dnsmasq:option value='address=/apps.example.com/192.168.111.1'/>"
pattern="address=/.apps.example.com"
if echo "$line" | grep -q "$pattern"; then
echo "MATCH (unexpected)"
else
echo "NO MATCH - confirms probe never succeeds against rendered template text"
fiRepository: openshift-eng/two-node-toolbox
Length of output: 240
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== haproxy-setup.yml ==\n'
sed -n '1,220p' deploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/haproxy-setup.yml
printf '\n== libvirt-network-3node.xml.j2 ==\n'
sed -n '1,220p' deploy/openshift-clusters/roles/mutable-topology/sno-to-3node/templates/libvirt-network-3node.xml.j2
printf '\n== search haproxy_migration_complete ==\n'
rg -n "haproxy_migration_complete|dns_migration_check|net-destroy|net-undefine|net-define|net-start|libvirtd" deploy/openshift-clusters/roles/mutable-topology/sno-to-3node -SRepository: openshift-eng/two-node-toolbox
Length of output: 11654
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== haproxy-setup.yml ==\n'
sed -n '1,220p' deploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/haproxy-setup.yml
printf '\n== libvirt-network-3node.xml.j2 ==\n'
sed -n '1,220p' deploy/openshift/eng? >/dev/nullRepository: openshift-eng/two-node-toolbox
Length of output: 6689
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,120p' deploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/haproxy-setup.yml | cat -n
printf '\n---\n'
sed -n '1,120p' deploy/openshift-clusters/roles/mutable-topology/sno-to-3node/templates/libvirt-network-3node.xml.j2 | cat -n
printf '\n---\n'
rg -n "haproxy_migration_complete|dns_migration_check|net-destroy|net-undefine|net-define|net-start|libvirtd" deploy/openshift-clusters/roles/mutable-topology/sno-to-3node -SRepository: openshift-eng/two-node-toolbox
Length of output: 10055
Fix the DNS migration probe regex
grep -q "address=/.apps.{{ sno_cluster_domain | regex_escape() }}" can’t match the rendered libvirt option (address=/apps.{{ sno_cluster_domain }}/...), so dns_migration_check never goes green and the HAProxy/libvirt reconfigure path keeps running.
🐛 Proposed fix
- grep -q "address=/.apps.{{ sno_cluster_domain | regex_escape() }}" || exit 1
+ grep -q "address=/apps\.{{ sno_cluster_domain | regex_escape() }}/" || exit 1📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: "[haproxy] Check if DNS migration is complete" | |
| shell: | | |
| # Check /etc/hosts has no stale apps entries | |
| ! grep -q "apps\.{{ sno_cluster_domain | regex_escape() }}" /etc/hosts 2>/dev/null || exit 1 | |
| # Check libvirt network has wildcard DNS for apps | |
| virsh net-dumpxml {{ sno_libvirt_network }} 2>/dev/null | \ | |
| grep -q "address=/.apps.{{ sno_cluster_domain | regex_escape() }}" || exit 1 | |
| # All checks passed | |
| exit 0 | |
| register: dns_migration_check | |
| changed_when: false | |
| failed_when: false | |
| - name: "[haproxy] Check if DNS migration is complete" | |
| shell: | | |
| # Check /etc/hosts has no stale apps entries | |
| ! grep -q "apps\.{{ sno_cluster_domain | regex_escape() }}" /etc/hosts 2>/dev/null || exit 1 | |
| # Check libvirt network has wildcard DNS for apps | |
| virsh net-dumpxml {{ sno_libvirt_network }} 2>/dev/null | \ | |
| grep -q "address=/apps\.{{ sno_cluster_domain | regex_escape() }}/" || exit 1 | |
| # All checks passed | |
| exit 0 | |
| register: dns_migration_check | |
| changed_when: false | |
| failed_when: false |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@deploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/haproxy-setup.yml`
around lines 21 - 34, Update the libvirt wildcard-DNS check in the “[haproxy]
Check if DNS migration is complete” task so its grep pattern matches the
rendered address format, including the slash between “address=” and “apps.”
while preserving the escaped sno_cluster_domain value. Keep the stale /etc/hosts
check and existing dns_migration_check behavior unchanged.
coderabbitai
Bot
added
the
ready-for-human-review
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/Makefile`:
- Around line 219-225: Remove the deploy EXTRA_ARGS bullet and example from the
Advanced Usage help text, leaving only examples for commands that actually
forward EXTRA_ARGS. Update the surrounding echo statements without changing the
documented sno-to-3node and clean-mutable-topology guidance.
🪄 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: 7509e454-a0fa-43d9-87a5-f7882ecddba8
📒 Files selected for processing (2)
deploy/Makefiledeploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/topology.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- deploy/openshift-clusters/roles/mutable-topology/sno-to-3node/tasks/topology.yml
| @echo "" | ||
| @echo "Advanced Usage:" | ||
| @echo " Some commands accept EXTRA_ARGS to pass additional options to underlying tools:" | ||
| @echo " - sno-to-3node and clean-mutable-topology: pass Ansible extra vars/verbosity" | ||
| @echo " Example: make sno-to-3node EXTRA_ARGS=\"-e my_var=value -vv\"" | ||
| @echo " - deploy: pass CloudFormation parameters" | ||
| @echo " Example: make deploy EXTRA_ARGS=\"key1=value1 key2=value2\"" |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C4 'EXTRA_ARGS|cloudformation|create-stack|deploy-stack|^deploy:|^create:' deploy/Makefile deploy/openshift-clusters/scriptsRepository: openshift-eng/two-node-toolbox
Length of output: 4028
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== deploy/Makefile targets ==\n'
sed -n '1,60p' deploy/Makefile
printf '\n== search for EXTRA_ARGS in deploy tree ==\n'
rg -n -C3 'EXTRA_ARGS|cloudformation|create-stack|deploy-stack|parameter|stack' deploy -g '!**/*.md'
printf '\n== scripts referenced by deploy target ==\n'
for f in deploy/aws-hypervisor/scripts/create.sh deploy/aws-hypervisor/scripts/init.sh deploy/aws-hypervisor/scripts/inventory.sh; do
echo "--- $f ---"
sed -n '1,220p' "$f"
doneRepository: openshift-eng/two-node-toolbox
Length of output: 46277
Remove the deploy EXTRA_ARGS example
make deploy EXTRA_ARGS="key1=value1 key2=value2" is ignored here: deploy only runs make create init inventory, and neither that recursion nor create.sh forwards arbitrary args to CloudFormation. Either thread EXTRA_ARGS through first or drop this example.
🤖 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 219 - 225, Remove the deploy EXTRA_ARGS bullet
and example from the Advanced Usage help text, leaving only examples for
commands that actually forward EXTRA_ARGS. Update the surrounding echo
statements without changing the documented sno-to-3node and
clean-mutable-topology guidance.
1 participant
Critical bug fixes:
Improvements:
Summary by CodeRabbit
EXTRA_ARGSguidance.