-
Notifications
You must be signed in to change notification settings - Fork 38.9k
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
Report node DNS info with --node-ip #63170
Conversation
f9b6f84
to
5446e2b
Compare
Thanks for the PR @micahhausler! There's some assumptions from nodecontroller that also overrides node addresses when kubelet sets |
/ok-to-test |
431e83a
to
2bdb719
Compare
@andrewsykim Done! |
glog.Errorf("Specified Node IP not found in cloudprovider") | ||
return | ||
} | ||
nodeAddresses = []v1.NodeAddress{*nodeIP} |
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.
sorry I wasn't clear earlier. I think we should still validate that nodeIP suggested by user exists, but we don't need to override the entire slice here with
nodeAddresses = []v1.NodeAddress{*nodeIP}
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.
You can probably just port the changes from #63101
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.
got it
2bdb719
to
a97ec28
Compare
```release-note Report node DNS info with --node-ip flag ```
a97ec28
to
1a218aa
Compare
/test pull-kubernetes-e2e-gce |
/lgtm @kubernetes/sig-node-pr-reviews PTAL |
/assign @yujuhong looks like a kubelet change |
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.
LGTM!
@@ -521,6 +521,7 @@ func NewMainKubelet(kubeCfg *kubeletconfiginternal.KubeletConfiguration, | |||
containerManager: kubeDeps.ContainerManager, | |||
containerRuntimeName: containerRuntime, | |||
nodeIP: parsedNodeIP, | |||
nodeIPValidator: validateNodeIP, |
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.
I think this is a pretty clean way of allowing validateNodeIP() to be mocked for testing. The alternative I can think of is to create a kubelet wrapper for net.InterfaceAddrs()
to allow you test get testing coverage for the validateNodeIP()
function itself.
/test pull-kubernetes-verify |
/test pull-kubernetes-e2e-gce |
4 similar comments
/test pull-kubernetes-e2e-gce |
/test pull-kubernetes-e2e-gce |
/test pull-kubernetes-e2e-gce |
/test pull-kubernetes-e2e-gce |
/test pull-kubernetes-local-e2e-containerized |
@bowei Can you PTAL? Thanks! |
@yujuhong - bumping this as I think bowei is ooo for a couple of days. Can you take a look at this? |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: andrewsykim, dchen1107, micahhausler The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/test all [submit-queue is verifying that this PR is safe to merge] |
/milestone v1.10 |
@micahhausler: You must be a member of the kubernetes-milestone-maintainers github team to set the milestone. In 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. |
@dchen1107 Can you add this to 1.10 so it can get backported? |
Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here. |
What this PR does / why we need it:
This PR adds
ExternalDNS
,InternalDNS
, andExternalIP
info for kubelets with the--nodeip
flag enabled.Which issue(s) this PR fixes
Fixes #63158
Special notes for your reviewer:
I added a field to the Kubelet to make IP validation more testable (
validateNodeIP
relies on thenet
package and the IP address of the host that is executing the test.) I also converted the test to use a table so new cases could be added more easily.Release Notes
@andrewsykim
@nckturner
/sig node
/sig network