OCPBUGS-63152: use IPFamilies template for wait-for-node-ip - #6289
OCPBUGS-63152: use IPFamilies template for wait-for-node-ip#6289rbbratta wants to merge 1 commit into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
jira/valid-reference
|
@rbbratta: This pull request references Jira Issue OCPBUGS-63152, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: rbbratta The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
WalkthroughThe node IP wait script now conditionally performs IPv4 and IPv6 bind checks based on configured IP families. A table-driven test verifies primary, IPv4, and IPv6 bind blocks for single-stack and dual-stack configurations. ChangesNode IP family binding
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Caution Review failedAn error occurred during the review process. Please try again later. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@cybertron @mko can we use the |
cybertron
left a comment
cybertron
left a comment
There was a problem hiding this comment.
Would like to see the duplication reduced, but otherwise this makes sense to me.
| # We only want to make sure that if nodeip-configuration detected an IP address, the | ||
| # address is usable. | ||
| # Family-specific checks follow ControllerConfig IPFamilies (network.config serviceNetwork; same as kubelet --node-ip). | ||
| {{- if or (eq .IPFamilies "DualStack") (eq .IPFamilies "DualStackIPv6Primary") }} |
There was a problem hiding this comment.
I don't really like the duplication here. We can just wrap the individual checks in ifs that look for single stack of the opposite family. I.e.
if ipfamilies != ipv6
check for ipv4
if ipfamilies != ipv4
check for ipv6
See if we can use the kubelet dualstack node-ip templating info to help with wait-for-node-ip.sh. If kubelet is not trying to bind to an IPv6 address, then we don't need to wait for the IPv6 address?
dbdb79b to
ea7ccf0
Compare
![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
🤖 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 `@templates/common/_base/files/wait-for-node-ip.yaml`:
- Around line 41-56: Indent both conditional template blocks, including their
if/end actions, to the same level as the inline block scalar content in the
wait-for-node-IP template. Keep the shell commands and IPv4/IPv6 conditions
unchanged while ensuring all four template actions remain inside inline: | so
the rendered YAML passes yamllint.
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ceff1f02-b7a7-49fa-b694-08bf141f5b86
📒 Files selected for processing (2)
pkg/controller/template/render_test.gotemplates/common/_base/files/wait-for-node-ip.yaml
| {{- if ne .IPFamilies "IPv6" }} | ||
| ip=$(cat /run/nodeip-configuration/ipv4 || echo "") | ||
| if [[ "${ip}" == "" ]]; then | ||
| echo "No ipv4 to bind was found" | ||
| else | ||
| wait_for_ip_bind "${ip}" | ||
| fi | ||
|
|
||
| {{- end }} | ||
| {{- if ne .IPFamilies "IPv4" }} | ||
| ip=$(cat /run/nodeip-configuration/ipv6 || echo "") | ||
| if [[ "${ip}" == "" ]]; then | ||
| echo "No ipv6 to bind was found" | ||
| else | ||
| wait_for_ip_bind "${ip}" | ||
| fi | ||
| {{- end }} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Keep the template YAML-valid.
The column-zero template actions terminate inline: |, so yamllint fails at Line 42. Indent all four actions to the block-scalar level.
Proposed fix
-{{- if ne .IPFamilies "IPv6" }}
+ {{- if ne .IPFamilies "IPv6" }}
...
-{{- end }}
+ {{- end }}📝 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.
| {{- if ne .IPFamilies "IPv6" }} | |
| ip=$(cat /run/nodeip-configuration/ipv4 || echo "") | |
| if [[ "${ip}" == "" ]]; then | |
| echo "No ipv4 to bind was found" | |
| else | |
| wait_for_ip_bind "${ip}" | |
| fi | |
| {{- end }} | |
| {{- if ne .IPFamilies "IPv4" }} | |
| ip=$(cat /run/nodeip-configuration/ipv6 || echo "") | |
| if [[ "${ip}" == "" ]]; then | |
| echo "No ipv6 to bind was found" | |
| else | |
| wait_for_ip_bind "${ip}" | |
| fi | |
| {{- end }} | |
| {{- if ne .IPFamilies "IPv6" }} | |
| ip=$(cat /run/nodeip-configuration/ipv4 || echo "") | |
| if [[ "${ip}" == "" ]]; then | |
| echo "No ipv4 to bind was found" | |
| else | |
| wait_for_ip_bind "${ip}" | |
| fi | |
| {{- end }} | |
| {{- if ne .IPFamilies "IPv4" }} | |
| ip=$(cat /run/nodeip-configuration/ipv6 || echo "") | |
| if [[ "${ip}" == "" ]]; then | |
| echo "No ipv6 to bind was found" | |
| else | |
| wait_for_ip_bind "${ip}" | |
| fi | |
| {{- end }} |
🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 42-42: syntax error: could not find expected ':'
(syntax)
🤖 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 `@templates/common/_base/files/wait-for-node-ip.yaml` around lines 41 - 56,
Indent both conditional template blocks, including their if/end actions, to the
same level as the inline block scalar content in the wait-for-node-IP template.
Keep the shell commands and IPv4/IPv6 conditions unchanged while ensuring all
four template actions remain inside inline: | so the rendered YAML passes
yamllint.
Source: Linters/SAST tools
| {{- if ne .IPFamilies "IPv4" }} | ||
| ip=$(cat /run/nodeip-configuration/ipv6 || echo "") | ||
| if [[ "${ip}" == "" ]]; then | ||
| echo "No ipv6 to bind was found" |
There was a problem hiding this comment.
maybe we should fail here. We are expecting an IPv6 address to bind to.
|
@rbbratta: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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 kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
/hold Holding to allow the Kube rebase to land in #6321. Please ensure this will not cause merge conflicts for the Kube rebase before unholding this PR. |
openshift-ci
Bot
added
the
do-not-merge/hold
4 participants
See if we can use the kubelet dualstack node-ip templating info to help with wait-for-node-ip.sh.
If kubelet is not trying to bind to an IPv6 address, then we don't need to wait for the IPv6 address?
Cursor AI assisted.
Summary by CodeRabbit