Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Filter out IPV6 addresses from NodeAddresses() returned by vSphere #45181

Merged
merged 1 commit into from
May 10, 2017
Merged

Filter out IPV6 addresses from NodeAddresses() returned by vSphere #45181

merged 1 commit into from
May 10, 2017

Conversation

BaluDontu
Copy link
Contributor

@BaluDontu BaluDontu commented May 1, 2017 •
edited by enisoc
Loading

The vSphere CP returns both IPV6 and IPV4 addresses for a Node as part of NodeAddresses() implementation. However, Kubelet fails due to duplicate api.NodeAddress value when the node has an IPV6 address associated with it. This issue is tracked in #42690. The following are observed:

  • when we enabled the logs and checked the addresses sent by vSphere CP to Kubelet, we don't see any duplicate addresses at all.
  • Also, kubelet_node_status doesn’t receive any duplicate address from cloud provider.

However, when we filter out the IPV6 addresses and only return IPV4 addresses to the Kubelet, it works perfectly fine.

Even though the Kubelet receives the non-duplicate node-addresses, it still errors out with duplicate node addresses. It might be an issue when kubelet propagates these addresses to API server (or) API server is enable to handle IPV6 addresses.

@divyenpatel @abrarshivani @pdhamdhere @tusharnt

Release note:

vSphere cloud provider: Filter out IPV6 node addresses.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 1, 2017
@k8s-reviewable
Copy link

This change is Reviewable

@k8s-ci-robot
Copy link
Contributor

Hi @BaluDontu. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with @k8s-bot ok to test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

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. I understand the commands that are listed here.

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 1, 2017
@k8s-github-robot k8s-github-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. release-note-label-needed labels May 1, 2017
@BaluDontu
Copy link
Contributor Author

/assign @abrarshivani

@grodrigues3
Copy link
Contributor

@k8s-bot ok to test

@k8s-ci-robot k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 1, 2017
divyenpatel
divyenpatel reviewed May 2, 2017
View reviewed changes
pkg/cloudprovider/providers/vsphere/vsphere_util.go Outdated
@@ -63,3 +65,8 @@ func GetgovmomiClient(cfg *VSphereConfig) (*govmomi.Client, error) {
client, err := newClient(context.TODO(), cfg)
return client, err
}

func IsIpv4Address(str string) bool {
ip := net.ParseIP(str)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you use To4() method from net package.
ip := net.ParseIP(str).To4()

For ipv6 address ip will be set to nil.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed this in latest commit.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good.

@BaluDontu BaluDontu closed this May 2, 2017
@BaluDontu BaluDontu reopened this May 2, 2017
@BaluDontu BaluDontu mentioned this pull request May 2, 2017
Closed
@divyenpatel divyenpatel mentioned this pull request May 4, 2017
Merged
@BaluDontu
Copy link
Contributor Author

@k8s-bot verify test this

@abrarshivani
Copy link
Contributor

/approve

@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 5, 2017
@BaluDontu
Copy link
Contributor Author

@k8s-bot verify test this

@abrarshivani
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 9, 2017
@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: BaluDontu, abrarshivani

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-github-robot k8s-github-robot added the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label May 9, 2017
@k8s-github-robot k8s-github-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels May 9, 2017
@saad-ali saad-ali removed the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label May 10, 2017
@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 7c3f8c9 into kubernetes:master May 10, 2017
@divyenpatel divyenpatel mentioned this pull request May 15, 2017
Merged
@enisoc enisoc added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels May 15, 2017
k8s-github-robot pushed a commit that referenced this pull request May 17, 2017
Merge pull request #45834 from vmware/automated-cherry-pick-of-#45181-#…
78e3ccf 
…45569-kubernetes-release-1.6

Automatic merge from submit-queue

Automated cherry pick of #45181 #45569 upstream release 1.6

Cherry pick of #45181 #45569 on release-1.6.

#45181: Filter out IPV6 addresses from NodeAddresses() returned by vSphere
#45569:  Fixing VolumesAreAttached and DisksAreAttached functions in vSphere

@BaluDontu @luomiao  @tusharnt
@BaluDontu BaluDontu deleted the NodeAddressesIPV6IssueNew branch July 13, 2017 23:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@divyenpatel divyenpatel divyenpatel left review comments

Assignees

@abrarshivani abrarshivani

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.