Fix ipvs proxier nodeport #56685

m1093782566 · 2017-12-01T09:31:02Z

What this PR does / why we need it:

Fix ipvs proxier nodeport.

Which issue(s) this PR fixes:
Fixes #55923

Special notes for your reviewer:

We bump the netwlink version in the 1st commit because:

We call netlink.RouteListFiltered() to filter LOCAL type addresses from kernel route table. netlink.RouteListFiltered() exists in newer-version netlink package
newer-version netlink package migrate syscall to golang.org/x/sys/unix, k8s cross-build can benefit from it as well. The Go doc for syscall says:

NOTE: This package is locked down. Code outside the standard Go repository should be migrated to use the corresponding package in the golang.org/x/sys repository. That is also where updates required by new systems or versions should be applied. See https://golang.org/s/go1.4-syscall for more information.

Release note:

Fix ipvs proxier nodeport eth* assumption

k8s-ci-robot · 2017-12-01T09:31:44Z

@m1093782566: GitHub didn't allow me to assign the following users: brendanburns.

Note that only kubernetes members can be assigned.

In response to this:

/assign @thockin @brendanburns

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.

k8s-ci-robot · 2017-12-01T09:32:20Z

@m1093782566: GitHub didn't allow me to assign the following users: brendanburns.

Note that only kubernetes members can be assigned.

In response to this:

/assign @brendanburns

/unassign @lavalamp

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.

thockin · 2017-12-04T18:00:27Z

pkg/proxy/ipvs/proxier.go

@@ -171,36 +170,27 @@ type IPGetter interface {
 	NodeIPs() ([]net.IP, error)
 }

+// realIPGetter is a real NodeIP handler, it implements IPGetter.
 type realIPGetter struct{}

 func (r *realIPGetter) NodeIPs() (ips []net.IP, err error) {


This function could use a comment explaining what it does, and what the heuristic is.

It also seems to have no tests

This function could use a comment explaining what it does, and what the heuristic is.

Sounds fair, added now. PTAL.

It also seems to have no tests

It's fixed now. PTAL.

thockin · 2017-12-04T18:01:49Z

pkg/proxy/ipvs/netlink_linux.go

+// 10.0.0.10  scope host  src 10.0.0.10
+// Then cut the unique src IP fields,
+// --> result set: [10.0.0.1, 10.0.0.10]
+func (h *netlinkHandle) GetLocalAddresses(filterDev string) (sets.String, error) {


Where are the tests?

Believe me, I am 100% want to add tests.

The reason why I didn't do that is I am unable to do that. GetLocalAddresses() will revoke netlink.LinkByName() to get a REAL network interface and netlink.RouteListFiltered() to list REAL route table. In order to testing them, I have to create a REAL network interface and give the test code root privilege. Unfortunately, the UT codes run in upstream CI can NOT acquire such privileges. Am I missing something?

Or, Should we check the user id before running tests? e.g.

if user.Current().Uid == 0 { do test }

??

It seems odd.

In my view, we can add tests for the caller side: NodeIPs()?

thockin · 2017-12-04T18:06:47Z

I don't like having to ask for tests. I know they are tedious, but they really are required. The project in general, and this effort (ipvs overall) in particular are too important.

m1093782566 · 2017-12-05T09:10:01Z

I apologize - if the missing tests make you unhappy.

Tests are already there. Please take a look again when you have time.

thockin · 2017-12-15T05:47:59Z

/lgtm
/approve

fejta-bot · 2017-12-15T09:06:36Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

…iltered

m1093782566 · 2017-12-18T06:24:29Z

PR rebased to resolve conflicts.... It needs re-lgtm.

brendandburns · 2017-12-18T21:21:37Z

/lgtm

k8s-ci-robot · 2017-12-18T21:21:44Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: brendandburns, m1093782566, thockin

Associated issue: #55923

The full list of commands accepted by this bot can be found here.

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

~~OWNERS~~ [brendandburns,thockin]

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 · 2017-12-18T22:45:41Z

Automatic merge from submit-queue (batch tested with PRs 54379, 56593, 56685, 54174, 57309). If you want to cherry-pick this change to another branch, please follow the instructions here.

gyliu513 · 2018-01-12T09:13:32Z

@m1093782566 can you please back merge this to 1.9.0? As in 1.9.0, ipvs claimed it is already beta, but NodePort service does not work, this is not acceptable.

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Dec 1, 2017

k8s-github-robot assigned lavalamp and thockin Dec 1, 2017

k8s-ci-robot unassigned lavalamp Dec 1, 2017

k8s-ci-robot assigned brendandburns Dec 1, 2017

k8s-ci-robot added the area/ipvs label Dec 1, 2017

m1093782566 mentioned this pull request Dec 1, 2017

ipvs NodePort is not working if host not using eth{i} as nic name #55923

Closed

m1093782566 force-pushed the fix-nodeport branch 3 times, most recently from 642eb52 to 9da9b34 Compare December 2, 2017 06:46

k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 2, 2017

m1093782566 force-pushed the fix-nodeport branch from 9da9b34 to 4aa6fc6 Compare December 2, 2017 06:54

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Dec 2, 2017

thockin reviewed Dec 4, 2017

View reviewed changes

m1093782566 force-pushed the fix-nodeport branch from 9286de9 to 75b21fe Compare December 5, 2017 09:06

m1093782566 force-pushed the fix-nodeport branch from 4f0d379 to 21f682b Compare December 6, 2017 01:31

kubernetes deleted a comment from k8s-github-robot Dec 6, 2017

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Dec 15, 2017

bump netlink version because of we need to use new version RouteListF…

f93d842

…iltered

fix ipvs proxier nodeport

a9b67a4

m1093782566 force-pushed the fix-nodeport branch from 21f682b to 2689955 Compare December 16, 2017 08:27

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 16, 2017

m1093782566 added 3 commits December 18, 2017 11:03

test get node IP

094eaf4

add fake ut

8c4bfd0

update bazel BUILD

22a4edc

m1093782566 force-pushed the fix-nodeport branch from 2689955 to 22a4edc Compare December 18, 2017 03:04

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 18, 2017

k8s-github-robot merged commit 6719e7a into kubernetes:master Dec 18, 2017

Lion-Wei mentioned this pull request Jan 13, 2018

ipvs NodePort is not working if host not using eth{i} as nic name in 1.9.0 #58202

Closed

This was referenced Jan 23, 2018

Automated cherry pick of #56685: Fix ipvs proxier nodeport #58612

Closed

Automated cherry pick of #56685: Fix ipvs proxier nodeport #58842

Closed

m1093782566 mentioned this pull request Feb 22, 2018

kube-proxy in ipvs mode doesn not correctly work with load balancer #59976

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix ipvs proxier nodeport #56685

Fix ipvs proxier nodeport #56685

m1093782566 commented Dec 1, 2017 •

edited

Loading

k8s-ci-robot commented Dec 1, 2017

k8s-ci-robot commented Dec 1, 2017

thockin Dec 4, 2017

thockin Dec 4, 2017

m1093782566 Dec 5, 2017

m1093782566 Dec 5, 2017

thockin Dec 4, 2017

m1093782566 Dec 5, 2017 •

edited

Loading

m1093782566 Dec 5, 2017

thockin commented Dec 4, 2017

m1093782566 commented Dec 5, 2017

thockin commented Dec 15, 2017

fejta-bot commented Dec 15, 2017

m1093782566 commented Dec 18, 2017 •

edited

Loading

brendandburns commented Dec 18, 2017

k8s-ci-robot commented Dec 18, 2017

k8s-github-robot commented Dec 18, 2017

gyliu513 commented Jan 12, 2018

Fix ipvs proxier nodeport #56685

Fix ipvs proxier nodeport #56685

Conversation

m1093782566 commented Dec 1, 2017 • edited Loading

k8s-ci-robot commented Dec 1, 2017

k8s-ci-robot commented Dec 1, 2017

thockin Dec 4, 2017

Choose a reason for hiding this comment

thockin Dec 4, 2017

Choose a reason for hiding this comment

m1093782566 Dec 5, 2017

Choose a reason for hiding this comment

m1093782566 Dec 5, 2017

Choose a reason for hiding this comment

thockin Dec 4, 2017

Choose a reason for hiding this comment

m1093782566 Dec 5, 2017 • edited Loading

Choose a reason for hiding this comment

m1093782566 Dec 5, 2017

Choose a reason for hiding this comment

thockin commented Dec 4, 2017

m1093782566 commented Dec 5, 2017

thockin commented Dec 15, 2017

fejta-bot commented Dec 15, 2017

m1093782566 commented Dec 18, 2017 • edited Loading

brendandburns commented Dec 18, 2017

k8s-ci-robot commented Dec 18, 2017

k8s-github-robot commented Dec 18, 2017

gyliu513 commented Jan 12, 2018

m1093782566 commented Dec 1, 2017 •

edited

Loading

m1093782566 Dec 5, 2017 •

edited

Loading

m1093782566 commented Dec 18, 2017 •

edited

Loading