-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Bug 1957700: None Platform - set Kubelet's node-ip param based on service net CIDR #4987
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
@yboaron: This pull request references Bugzilla bug 1957700, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker. 3 validation(s) were run on this bug
No GitHub users were found matching the public email listed for the QA contact in Bugzilla (nshidlin@redhat.com), skipping review request. 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/test-infra repository. |
|
cc @celebdor |
cybertron
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't match the behavior of the masters in UPI: https://github.com/openshift/machine-config-operator/blob/847e2216cc324cfc17904e6afa6a18a47c565674/pkg/operator/render.go#L156 and https://github.com/openshift/machine-config-operator/blob/ecb3c0e03652049b5c37d9f5d5678e9d6dc24301/templates/common/_base/units/nodeip-configuration.service.yaml#L25
The difference is that on the nodes it only forces ipv6 for single-stack deployments. For dual-stack it will still use ipv4 (which should be fine since both are available). I'm not sure whether it matters since in dual-stack either should work, but I think it would be safer to be consistent.
Also, need to fix the fmt failure.
Let's stay consistent with what is done in nodeip-configuration and only force ipv6 in single-stack. |
|
Can you help me understand why we care about the service network for this? The errors in the bug are with connecting to the API server. The service network is not relevant to that. Why wouldn't we be looking at the machine network instead? |
|
/bugzilla refresh The main branch will open for development of next OCP version. Recalculating validity of PRs linked to this PR. |
|
@openshift-bot: This pull request references Bugzilla bug 1957700, 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 kubernetes/test-infra repository. |
|
/bugzilla refresh The main branch will open for development of next OCP version. Recalculating validity of PRs linked to this PR. |
|
@openshift-bot: This pull request references Bugzilla bug 1957700, 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 kubernetes/test-infra repository. |
e2e-metal-assisted-none-ipv6 jobs are failing for a long time. In order to reduce spamming on the CI channel, the alert would be removed until the job would return to work. The job should pass once openshift/installer#4987 is merged.
|
/test e2e-vsphere-upi I'm not entirely sure why we're using the service network for this, but it's what was used in MCO for determining whether kubelet should use ipv6. |
|
/uncc |
This PR addressed issue similar to [1] for the None platform, since API VIP isn't used for None platform, the UseIPv6ForNodeIP parameter should be calculated based on the content of service CIDR. [1] openshift#4756
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
|
/bugzilla refresh |
|
@yboaron: This pull request references Bugzilla bug 1957700, which is valid. 3 validation(s) were run on this bug
No GitHub users were found matching the public email listed for the QA contact in Bugzilla (nshidlin@redhat.com), skipping review request. 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/test-infra repository. |
|
/retest |
|
/bugzilla refresh The requirements for Bugzilla bugs have changed, recalculating validity. |
|
@openshift-merge-robot: This pull request references Bugzilla bug 1957700, 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 kubernetes/test-infra repository. |
|
@yboaron: 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/test-infra repository. I understand the commands that are listed here. |
|
/close |
|
@yboaron: PR needs rebase. 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/test-infra repository. |
|
@sadasu: Closed this PR. 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/test-infra repository. |
|
@yboaron: This pull request references Bugzilla bug 1957700. The bug has been updated to no longer refer to the pull request using the external bug tracker. All external bug links have been closed. The bug has been moved to the NEW state. 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/test-infra repository. |
This PR addressed issue similar to [1] for the None platform,
since API VIP isn't used for None platform, the UseIPv6ForNodeIP
parameter should be calculated based on the content of service CIDR.
[1] #4756