[ipi] add machine net and vips according to dual or single stack #705
Conversation
📝 WalkthroughWalkthroughAdds release-version (4.12+) gating and per-IP-version conditional rendering to installer Jinja2 templates for machine/network/VIP fields, removes a default variable, and updates VIP validation conditions to match the new IPv4/IPv6/dual-stack logic. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
roles/installer/templates/install-config-virtualmedia.j2 (2)
12-16: SamemachineCIDRdual-stack issue as in bare-metal templateApply the guard proposed for
install-config.j2to avoid conflictingmachineCIDR+machineNetworkdeclarations.
53-65: Mirror the “safe-default” tweaks for VIP conditionalsThe undefined-flag risk and the corresponding
|default(false)|boolfix apply here too.Also applies to: 67-68
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
roles/installer/templates/install-config-virtualmedia.j2(2 hunks)roles/installer/templates/install-config.j2(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Sanity Check (stable-2.9)
- GitHub Check: Ansible-lint Check
- GitHub Check: Sanity Check (stable-2.18)
🔇 Additional comments (1)
roles/installer/templates/install-config.j2 (1)
53-65: Harden VIP conditional blocks against missing flag variablesSeveral tests (
ipv4_baremetal|bool,ipv6_enabled|bool) assume the flag is always defined. If a higher-level playbook omits one of them, the template will raise an UndefinedError and abort the run.Add safe defaults when casting to bool:
-{% if ipv4_baremetal|bool and apivip is defined and apivip|ipv4 %} +{% if (ipv4_baremetal|default(false)|bool) and apivip is defined and apivip|ipv4 %} -{% if ipv6_enabled|bool and apivip6 is defined and apivip6|ipv6 %} +{% if (ipv6_enabled|default(false)|bool) and apivip6 is defined and apivip6|ipv6 %} -{% if ipv4_baremetal|bool and ingressvip is defined and ingressvip|ipv4 %} +{% if (ipv4_baremetal|default(false)|bool) and ingressvip is defined and ingressvip|ipv4 %} -{% if ipv6_enabled|bool and ingressvip6 is defined and ingressvip6|ipv6 %} +{% if (ipv6_enabled|default(false)|bool) and ingressvip6 is defined and ingressvip6|ipv6 %} - apiVIP: {{ (ipv4_baremetal|bool) | ternary(apivip, apivip6) }} - ingressVIP: {{ (ipv4_baremetal|bool) | ternary(ingressvip, ingressvip6) }} + apiVIP: {{ (ipv4_baremetal|default(false)|bool) | ternary(apivip, apivip6) }} + ingressVIP: {{ (ipv4_baremetal|default(false)|bool) | ternary(ingressvip, ingressvip6) }}This small change makes the template resilient to callers that provide only one of the flags.
Also applies to: 67-68
|
Build succeeded. ✔️ dci-rpm-build-el8 SUCCESS in 2m 48s |
|
from change #705: |
manurodriguez
changed the title
manurodriguez
force-pushed
the
test-ipv6-only-ipi
branch
from
172ea18 to
da6424f
Compare
|
Build succeeded. ✔️ dci-rpm-build-el8 SUCCESS in 2m 51s |
|
from change #705: |
manurodriguez
force-pushed
the
test-ipv6-only-ipi
branch
from
da6424f to
860af3c
Compare
|
Build succeeded. ✔️ dci-rpm-build-el8 SUCCESS in 2m 50s |
|
from change #705: |
manurodriguez
force-pushed
the
test-ipv6-only-ipi
branch
from
860af3c to
09fe73c
Compare
|
Build succeeded. ✔️ dci-rpm-build-el8 SUCCESS in 2m 48s |
|
from change #705: |
manurodriguez
force-pushed
the
test-ipv6-only-ipi
branch
from
09fe73c to
2f208ea
Compare
|
Build succeeded. ✔️ dci-rpm-build-el8 SUCCESS in 2m 50s |
|
from change #705: |
manurodriguez
force-pushed
the
test-ipv6-only-ipi
branch
from
2f208ea to
984f439
Compare
|
Build succeeded. ✔️ dci-rpm-build-el8 SUCCESS in 2m 48s |
|
from change #705: |
| no_proxy_list: "" | ||
| http_proxy: "" | ||
| https_proxy: "" | ||
| ipv4_baremetal: false |
There was a problem hiding this comment.
Should ipv6_enabled have a default as ipv4_baremetal? I think it's needed to avoid breaking backward compatibility
|
@manurodriguez could you rebase this one? |
- OCP >= 4.12 can use either IPv4/IPv6 dual or single stack - anything before will default to IPv4 - use IPv4/IPv6 machineCIDR and VIPs accordingly
manurodriguez
force-pushed
the
test-ipv6-only-ipi
branch
from
984f439 to
f53d409
Compare
done, my bad, I'm resuming testing now |
|
Build succeeded. ✔️ dci-rpm-build-el8 SUCCESS in 2m 49s |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
roles/installer/templates/install-config.j2 (1)
69-70: Potential undefined VIP selection in fallback logicSame issue as in the virtualmedia template - the ternary operators could select undefined values if neither VIP is properly defined.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
roles/installer/templates/install-config-virtualmedia.j2(2 hunks)roles/installer/templates/install-config.j2(2 hunks)roles/node_prep/defaults/main.yml(0 hunks)roles/node_prep/tasks/10_validation.yml(1 hunks)
💤 Files with no reviewable changes (1)
- roles/node_prep/defaults/main.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- roles/node_prep/tasks/10_validation.yml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Sanity Check (stable-2.18)
- GitHub Check: Sanity Check (stable-2.9)
- GitHub Check: Ansible-lint Check
🔇 Additional comments (1)
roles/installer/templates/install-config-virtualmedia.j2 (1)
69-70: Please verify VIP validation for API and IngressI didn’t find any “required”, “assert” or “fail” checks for
apivip/apivip6oringressvip/ingressvip6in the roles, so the fallback ternaries may still produceundefined. Ensure that at least one of each pair is always defined—either by adding an Ansibleassert/failtask or by upstream variable validation.• File: roles/installer/templates/install-config-virtualmedia.j2
Lines 69–70apiVIP: {{ (apivip is defined and apivip|ipv4) | ternary(apivip, apivip6) }} ingressVIP: {{ (ingressvip is defined and ingressvip|ipv4) | ternary(ingressvip, ingressvip6) }}Please confirm that you have validation in place or introduce one to prevent both values being undefined.
| machineCIDR: {{ extcidrnet }} | ||
| {% endif %} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Missing IPv6 support for pre-4.12 versions
For OpenShift versions < 4.12, the template always sets machineCIDR to extcidrnet (IPv4). This doesn't handle IPv6-only deployments on older versions. Consider adding conditional logic similar to what was previously suggested.
{% else %}
+{% if ipv6_enabled|bool and not (dualstack_baremetal|bool or ipv4_baremetal|bool) %}
+ machineCIDR: {{ extcidrnet6 }}
+{% else %}
machineCIDR: {{ extcidrnet }}
+{% endif %}
{% endif %}📝 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.
| machineCIDR: {{ extcidrnet }} | |
| {% endif %} | |
| {% else %} | |
| - machineCIDR: {{ extcidrnet }} | |
| + {% if ipv6_enabled|bool and not (dualstack_baremetal|bool or ipv4_baremetal|bool) %} | |
| + machineCIDR: {{ extcidrnet6 }} | |
| + {% else %} | |
| + machineCIDR: {{ extcidrnet }} | |
| + {% endif %} | |
| {% endif %} |
🤖 Prompt for AI Agents
In roles/installer/templates/install-config.j2 around lines 22-23, the template
unconditionally sets machineCIDR to the IPv4 extcidrnet which breaks IPv6-only
pre-4.12 installs; modify the template to add a conditional branch for OpenShift
versions < 4.12 that uses the IPv6 CIDR variable when the cluster/network is
IPv6-only (e.g., check a provided ip_version or is_ipv6_only flag or the
existence of an extcidrnet_v6 variable) and fall back to extcidrnet for IPv4;
ensure the condition mirrors the earlier suggested logic so machineCIDR is set
to the IPv6 CIDR for IPv6-only clusters on older versions and to the IPv4
extcidrnet otherwise.
|
from change #705: |
|
This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 7 days |
|
Build succeeded. ✔️ dci-rpm-build-el8 SUCCESS in 2m 50s |
|
from change #705: |
|
This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 7 days |
SUMMARY
Add machine net and vips according to dual or single stack
- OCP >= 4.12 can use either IPv4/IPv6 dual or single stack
- anything before will default to IPv4
- use IPv4/IPv6 machineCIDR and VIPs accordingly
ISSUE TYPE
Enhanced Feature
Tests
TestBos2: virt