Remove the ethtool kubelet dependency; use ip link instead of ethtool to determine pair interface. #30356

luxas · 2016-08-10T11:17:31Z

@kubernetes/sig-network does this seem worthwhile?
I noticed that the ip link inside a container is named eth0@if{host_pair_interface}. This PR removes the dependency on ethtool that does that lookup as well.

Can you reproduce and verify?
ref #26093

$ ip link
1: lo: <LOOPBACK,UP,LOWER_UP> mtu 65536 qdisc noqueue state UNKNOWN mode DEFAULT group default 
    link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00
2: eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc pfifo_fast state UP mode DEFAULT group default qlen 1000
    link/ether f0:76:1c:62:f1:36 brd ff:ff:ff:ff:ff:ff
3: wlan0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc mq state UP mode DORMANT group default qlen 1000
    link/ether d0:53:49:0a:a9:8f brd ff:ff:ff:ff:ff:ff
209: flannel0: <POINTOPOINT,MULTICAST,NOARP,UP,LOWER_UP> mtu 1472 qdisc pfifo_fast state UNKNOWN mode DEFAULT group default qlen 500
    link/none 
210: docker0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1472 qdisc noqueue state UP mode DEFAULT group default 
    link/ether 02:42:5a:c2:48:48 brd ff:ff:ff:ff:ff:ff
212: vethb860bf4@if211: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1472 qdisc noqueue master docker0 state UP mode DEFAULT group default 
    link/ether f6:e0:1a:a4:a6:ec brd ff:ff:ff:ff:ff:ff link-netnsid 1
214: veth345cf11@if213: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1472 qdisc noqueue master docker0 state UP mode DEFAULT group default 
    link/ether c6:55:3a:33:89:71 brd ff:ff:ff:ff:ff:ff link-netnsid 0
$ sudo nsenter -t 22525 -n -- ip link
1: lo: <LOOPBACK,UP,LOWER_UP> mtu 65536 qdisc noqueue state UNKNOWN mode DEFAULT group default 
    link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00
211: eth0@if212: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1472 qdisc noqueue state UP mode DEFAULT group default 
    link/ether 02:42:0a:01:56:02 brd ff:ff:ff:ff:ff:ff link-netnsid 0

This change is

yujuhong · 2016-08-10T22:40:17Z

Assigned to @matchstick to review or delegate.

thockin · 2016-08-11T06:34:53Z

pkg/kubelet/network/hairpin/hairpin.go

@@ -37,7 +37,7 @@ const (
 )

 var (
-	ethtoolOutputRegex = regexp.MustCompile("peer_ifindex: (\\d+)")
+	ipLinkOutputRegex = regexp.MustCompile("eth0@if(\\d+)")


where does this come from? "eth0" is not a given...

Right now eth0 is hardcoded in an other file with a todo to make it configurable. I may fetch the value from there instead.

Do you still think it may be something other than eth0 (on kubenet for example, which I haven't tested much)

kubenet is not depending on the kubelet hairpin logic now. The CNI bridge plugin does the job.

Can we delete any of the old (pre-kubenet) code now before 1.4?

@thockin I think we should keep it in v1.4, but after the release we might remove it if it's unneeded.

freehan · 2016-08-12T23:14:50Z

Since kubenet is on by default now. I do not think the auto e2e test will cover the code changes here. I would recommend setting the NETWORK_PROVIDER=none for complete e2e test.

… to determine pair interface.

Random-Liu · 2016-08-15T22:51:46Z

@k8s-bot test this issue #29932.

freehan · 2016-08-16T00:26:08Z

Okay. LGTM.

Please revert the test commit and self apply LGTM label.

luxas · 2016-08-16T11:16:48Z

Thanks for the review!
Self-applying now

luxas · 2016-08-16T15:04:05Z

@k8s-bot smoke test this please github issue: #IGNORE

luxas · 2016-08-16T19:44:34Z

@k8s-bot test this please github issue: #IGNORE

k8s-bot · 2016-08-16T20:25:49Z

GCE e2e build/test passed for commit ae97cda.

freehan · 2016-08-17T23:53:41Z

hmmm. I just came across a counter case.

$ nsenter -t 6066 -n -F -- ip link show eth0
3: eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1460 qdisc noqueue state UP mode DEFAULT 
    link/ether 26:03:86:7f:02:8e brd ff:ff:ff:ff:ff:ff

It looks like we still cannot get rid of ethtool completely. What if we fall back to use ethtool if pattern not matched?

luxas · 2016-08-18T21:11:14Z

Ah, how bad! How did you produce that behaviour?
I'll update the PR to fallback on ethtool

thockin · 2016-08-18T23:16:39Z

Can we just do the things that ethtool is doing under the covers? I know
one of the original ethtool authors and he says it is pretty
straightforward..

On Thu, Aug 18, 2016 at 2:12 PM, Lucas Käldström notifications@github.com
wrote:

Ah, how bad! How did you produce that behaviour?
I'll update the PR to fallback on ethtool

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#30356 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AFVgVD7mREQ_G-teQhEekNM6Mt9ep9WDks5qhMqngaJpZM4JhCYs
.

thockin · 2016-08-18T23:31:44Z

https://raw.githubusercontent.com/safchain/ethtool/master/ethtool.go

I just ran this, it works.

On Thu, Aug 18, 2016 at 4:16 PM, Tim Hockin thockin@google.com wrote:

Can we just do the things that ethtool is doing under the covers? I know
one of the original ethtool authors and he says it is pretty
straightforward..

On Thu, Aug 18, 2016 at 2:12 PM, Lucas Käldström <notifications@github.com

wrote:

Ah, how bad! How did you produce that behaviour?
I'll update the PR to fallback on ethtool

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#30356 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AFVgVD7mREQ_G-teQhEekNM6Mt9ep9WDks5qhMqngaJpZM4JhCYs
.

luxas · 2016-08-19T22:02:55Z

@thockin That's a good idea, but we can't.
We're using nsenter to exec into the net namespace, and we can't do that with go, so the go ethtool code would always be running in the host netns :(

Guess we have to fallback on ethtool

k8s-bot · 2016-08-30T00:32:51Z

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message will repeat several times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

luxas · 2016-10-16T19:12:44Z

@freehan How did you procude the above condition?
Was it with the old networking mode (none)?

If so, we can rely on ip link after #34906 is merged then...

luxas · 2016-11-11T21:54:43Z

Hah @thockin this was the one I was talking about :)
This should be safe to do now when we've got rid of the legacy networking mode.
PTAL

luxas · 2016-11-16T17:58:51Z

We actually might want to consider this for v1.5 as a bug fix given kubernetes/kops#879

thockin · 2016-11-16T18:48:01Z

Do we really understand the escape that @freehan showed? What makes us think we really understand it? Are we sure all versions of kernels and ip link work properly?

Is this veth867530@if9 syntax specified or at least DOCUMENTED somewhere? a link to kernel code or ip tool code might convince me it was reliable.

My fear is we are replacing something known to work (but a dependency) with something new and undocumented. It looks like a side-effect, not an API. I want to know it is reliable.

bboreham · 2016-11-16T20:45:26Z

I agree that relying on the output format of ip link is unwise.

We're using nsenter to exec into the net namespace, and we can't do that with go

A trick used in various places (e.g. Docker) is to re-exec the current executable with arguments to do just the one task we need done in a different namespace.

jellonek · 2016-11-17T11:12:31Z

Can not we use there directly vishvananda/netlink instead calling external command? We have it already in our sources...
https://github.com/kubernetes/kubernetes/tree/master/vendor/github.com/vishvananda/netlink

It should be really easy to replicate what @thockin
@luxas PTAL on https://github.com/containernetworking/cni/tree/master/pkg/ns - so we can do something in particular ns while being still in go.

bboreham · 2016-11-17T11:20:40Z

@jellonek it is necessary to look in the pod's network namespace, so vishvananda/netlink is insufficient.

If you add vishvananda/ns there is enough code to do the task, but you still have to exec a separate process because of vishvananda/netns#17

Edit: One more thing: the CNI page qualifies its recommendation with "All code dependent on a particular network namespace should be wrapped in the ns.Do() method". Although it sounds simple, it implies a vast amount of work to wrap every network call in Kubernetes and all of its dependent libraries to ensure it runs in the root namespace; it is simpler to exec a separate process when you specifically need to be in a non-root namespace.

ref: kubernetes#26093 ref: kubernetes#30356

jellonek · 2016-11-28T11:38:20Z

@bboreham: PTAL on linked PR in which I shown how this could be done - IMO it's a bit simpler than using that two external binaries (nsenter, ethtool).

@thockin btw. doing my PR i also found what you were asking for there

@luxas: WDYT about my implementation?

bboreham · 2016-11-28T12:03:09Z

@jellonek please read up the issues I have linked. Your approach (which I also once thought was safe) has caused many weird failures in Docker, Weave Net and other places.

It is not possible with the current Go runtime to have one goroutine change namespace without sometimes affecting other goroutines. You must use a separate process to change namespace.

jellonek · 2016-11-28T12:07:52Z

@bboreham ack. will definitely read both of mentioned issues.

jellonek · 2016-11-28T12:17:58Z

Ok. So with current version of golang runtime we can not use such approach. I'll close my PR.

k8s-github-robot · 2016-12-28T12:19:42Z

This PR hasn't been active in 30 days. It will be closed in 59 days (Feb 26, 2017).

cc @luxas @thockin

You can add 'keep-open' label to prevent this from happening, or add a comment to keep it open another 90 days

luxas · 2017-01-08T21:08:55Z

Closing this as I don't have time to look into it. Can maybe be revisited in the future

googlebot added the cla: yes label Aug 10, 2016

k8s-github-robot assigned yujuhong Aug 10, 2016

k8s-github-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. release-note-label-needed labels Aug 10, 2016

yujuhong assigned matchstick and unassigned yujuhong Aug 10, 2016

thockin reviewed Aug 11, 2016
View reviewed changes

freehan assigned freehan and unassigned matchstick Aug 12, 2016

Remove the ethtool kubelet dependency; use ip link instead of ethtool…

ae97cda

… to determine pair interface.

luxas force-pushed the remove_ethtool branch from 83655b1 to ae97cda Compare August 13, 2016 18:42

luxas added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels Aug 13, 2016

luxas force-pushed the remove_ethtool branch from ebb625d to ae97cda Compare August 16, 2016 11:14

luxas added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 16, 2016

luxas mentioned this pull request Aug 16, 2016

Identify, document and maybe remove kubelet dependencies #26093

Closed

freehan removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 17, 2016

This was referenced Oct 25, 2016

Fix the kube-flannel.yaml in order to work flannel-io/flannel#518

Merged

hyperkube images take forever to build #35058

Closed

luxas assigned thockin and unassigned freehan Nov 11, 2016

luxas mentioned this pull request Nov 16, 2016

Weave issues with '--topology private' kubernetes/kops#879

Closed

jellonek added a commit to jellonek/kubernetes that referenced this pull request Nov 28, 2016

kubelet: Remove dependency on ethtool

26f931e

ref: kubernetes#26093 ref: kubernetes#30356

jellonek mentioned this pull request Nov 28, 2016

kubelet: Remove dependency on ethtool #37548

Closed

luxas closed this Jan 8, 2017

itskingori mentioned this pull request Feb 10, 2017

Install any additional requirements in nodeup kubernetes/kops#1830

Closed

aespinosa mentioned this pull request Sep 22, 2017

Updates for the latest k8s and etcd aespinosa/cookbook-kube#17

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove the ethtool kubelet dependency; use ip link instead of ethtool to determine pair interface. #30356

Remove the ethtool kubelet dependency; use ip link instead of ethtool to determine pair interface. #30356

luxas commented Aug 10, 2016 •

edited by k8s-oncall

Loading

yujuhong commented Aug 10, 2016

thockin Aug 11, 2016

luxas Aug 11, 2016

freehan Aug 12, 2016

thockin Aug 13, 2016

luxas Aug 13, 2016

freehan commented Aug 12, 2016

Random-Liu commented Aug 15, 2016

freehan commented Aug 16, 2016 •

edited

Loading

luxas commented Aug 16, 2016

luxas commented Aug 16, 2016

luxas commented Aug 16, 2016

k8s-bot commented Aug 16, 2016

freehan commented Aug 17, 2016

luxas commented Aug 18, 2016

thockin commented Aug 18, 2016

thockin commented Aug 18, 2016

luxas commented Aug 19, 2016

k8s-bot commented Aug 30, 2016

luxas commented Oct 16, 2016

luxas commented Nov 11, 2016

luxas commented Nov 16, 2016

thockin commented Nov 16, 2016

bboreham commented Nov 16, 2016

jellonek commented Nov 17, 2016

bboreham commented Nov 17, 2016 •

edited

Loading

jellonek commented Nov 28, 2016

bboreham commented Nov 28, 2016

jellonek commented Nov 28, 2016

jellonek commented Nov 28, 2016

k8s-github-robot commented Dec 28, 2016

luxas commented Jan 8, 2017

Remove the ethtool kubelet dependency; use ip link instead of ethtool to determine pair interface. #30356

Remove the ethtool kubelet dependency; use ip link instead of ethtool to determine pair interface. #30356

Conversation

luxas commented Aug 10, 2016 • edited by k8s-oncall Loading

yujuhong commented Aug 10, 2016

thockin Aug 11, 2016

Choose a reason for hiding this comment

luxas Aug 11, 2016

Choose a reason for hiding this comment

freehan Aug 12, 2016

Choose a reason for hiding this comment

thockin Aug 13, 2016

Choose a reason for hiding this comment

luxas Aug 13, 2016

Choose a reason for hiding this comment

freehan commented Aug 12, 2016

Random-Liu commented Aug 15, 2016

freehan commented Aug 16, 2016 • edited Loading

luxas commented Aug 16, 2016

luxas commented Aug 16, 2016

luxas commented Aug 16, 2016

k8s-bot commented Aug 16, 2016

freehan commented Aug 17, 2016

luxas commented Aug 18, 2016

thockin commented Aug 18, 2016

thockin commented Aug 18, 2016

luxas commented Aug 19, 2016

k8s-bot commented Aug 30, 2016

luxas commented Oct 16, 2016

luxas commented Nov 11, 2016

luxas commented Nov 16, 2016

thockin commented Nov 16, 2016

bboreham commented Nov 16, 2016

jellonek commented Nov 17, 2016

bboreham commented Nov 17, 2016 • edited Loading

jellonek commented Nov 28, 2016

bboreham commented Nov 28, 2016

jellonek commented Nov 28, 2016

jellonek commented Nov 28, 2016

k8s-github-robot commented Dec 28, 2016

luxas commented Jan 8, 2017

luxas commented Aug 10, 2016 •

edited by k8s-oncall

Loading

freehan commented Aug 16, 2016 •

edited

Loading

bboreham commented Nov 17, 2016 •

edited

Loading