Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
@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. |
WalkthroughThe node IP wait script now 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 Possibly related PRs
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
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
dbdb79b to
ea7ccf0
Compare
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 "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.
There was a problem hiding this comment.
Yeah, that probably makes sense now that we know which IP families should exist.
|
/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. |
|
/unhold Kube rebase landed |
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?
Now that we know which IP families we should be using Fail if we can't bind to each family.
ea7ccf0 to
9e0cbde
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
@rbbratta: This pull request references Jira Issue OCPBUGS-63152, which is invalid:
Comment 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. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
templates/common/_base/files/wait-for-node-ip.yaml (1)
41-58: 🎯 Functional Correctness | 🔵 Trivial | 🏗️ Heavy liftAdd execution coverage for the new failure path.
The new blocks change boot behavior: a missing selected-family address exits with status 1. Add a test that executes or simulates the rendered script for IPv4-only, IPv6-only, and dual-stack configurations. Assert the selected-family failure and confirm that single-stack configurations do not require the opposite family. The supplied
pkg/controller/template/render_test.gotest checks rendered text only.🤖 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 - 58, Add execution-based test coverage for the rendered wait-for-node-IP script, covering IPv4-only, IPv6-only, and dual-stack configurations. Verify each selected family exits with status 1 when its address is missing, while single-stack cases succeed without requiring the non-selected family; retain the existing render-text assertions in render_test.go.
🤖 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 `@templates/common/_base/files/wait-for-node-ip.yaml`:
- Around line 41-58: Add execution-based test coverage for the rendered
wait-for-node-IP script, covering IPv4-only, IPv6-only, and dual-stack
configurations. Verify each selected family exits with status 1 when its address
is missing, while single-stack cases succeed without requiring the non-selected
family; retain the existing render-text assertions in render_test.go.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 44f58ea9-04b9-4c07-95c4-0b4c4f2d9133
📒 Files selected for processing (2)
pkg/controller/template/render_test.gotemplates/common/_base/files/wait-for-node-ip.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/controller/template/render_test.go
|
/jira refresh |
|
@rbbratta: This pull request references Jira Issue OCPBUGS-63152, which is invalid:
Comment 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. |
|
/jira refresh |
|
@rbbratta: This pull request references Jira Issue OCPBUGS-63152, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: 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: GitHub didn't allow me to request PR reviews from the following users: rbbratta. Note that only openshift members and repo collaborators can review this PR, and authors cannot review their own PRs. 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 kubernetes-sigs/prow repository. |
|
Scheduling tests matching the |
|
/verified by @rbbratta |
|
@rbbratta: This PR has been marked as verified by DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cybertron, isabella-janssen, rbbratta 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 |
|
/retest-required |
|
/retest-required |
|
/retest-required |
|
/test unit |
|
/hold Revision 9e0cbde was retested 3 times: holding |
|
/unhold |
|
@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 Revision 9e0cbde was retested 3 times: holding |
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