kubelet/kubenet: split hostport handling into separate module #26934

dcbw · 2016-06-07T02:59:23Z

This pulls the hostport functionality of kubenet out into a separate module so that it can be more easily tested and potentially used from other code (maybe CNI, maybe downstream consumers like OpenShift, etc). Couldn't find a mock iptables so I wrote one, but I didn't look very hard.

@freehan @thockin @bprashanth

dcbw · 2016-06-07T14:23:53Z

2016/06/07 07:20:44 e2e.go:212: Running: build-release
ERROR: gcloud crashed (error): [Errno 111] Connection refused

dcbw · 2016-06-07T14:43:35Z

@k8s-bot test this issue: #23545

dcbw · 2016-06-07T15:41:25Z

Sigh... a couple instances of:

  Timed out after 300.000s.
  Expected
      <*errors.errorString | 0xc820acd9b0>: {
          s: "expected metrics for kubelet",
      }
  to be nil

which seems to be flake #26431

dcbw · 2016-06-07T18:00:35Z

@k8s-bot test this issue: #26431

dcbw · 2016-06-07T18:04:18Z

cc @danwinship @pravisankar

freehan · 2016-06-07T18:13:24Z

pkg/kubelet/network/kubenet/kubenet_linux.go

+	// Open any hostports the pod's containers want
+	runningPods, err := plugin.getRunningPods()
+	if err == nil {
+		err = plugin.hostportHandler.OpenPodHostportsAndSync(pod, BridgeName, runningPods)


Say if the CNI ADD succeeded, but port is in use. SetUpPod will end up setting up the veth but fail at opening hostport. IIUC, kubelet will just kill infra container without calling TearDownPod (it assumes SetUpPod won't be partially successful). Then this Pod will stuck, because CNI ADD will always return failure since eth0 has been setup.

Need some restructure. Other parts looks good overall.

Say if the CNI ADD succeeded, but port is in use. SetUpPod will end up setting up the veth but fail at opening hostport. IIUC, kubelet will just kill infra container without calling TearDownPod (it assumes SetUpPod won't be partially successful). Then this Pod will stuck, because CNI ADD will always return failure since eth0 has been setup.

You are correct that this PR will exacerbate the situation a bit, so I'll try to fix that. However, it looks like all the network plugins have problems with this, since they don't necessarily clean up after themselves on errors in SetUpPod(). And since (at least) dockertools/manager.go never calls TearDownPod() when setup fails, I think there are already cases where failures will leave the pod network configured... I'll try to figure something out here.

I don't think the pod will actually get stuck, since eth0 will get destroyed along with the infra container's network namespace. But what won't be cleaned up is any IPAM leases or shaper stuff.

oh that is even worse, because kubelet will keep retry and used up all IPs. :(

Researching this more indicates that the docker runtime will eventually tear down the infra container. I'm not sure about the rkt runtime. But depending on the runtime to call TearDownPod() on a SetUpPod() failure seems pretty fragile, and I think the plugins should do it themsevles if they can.

Agreed. That is mostly because network plugin is event triggered.

I was thinking adding a Reconcile interface into network plugin. It can be triggered periodically and do whatever it want to ensure/cleanup network configuration.

dcbw · 2016-06-07T18:24:23Z

Still flaking on #26431...

danwinship · 2016-06-09T14:27:50Z

pkg/kubelet/network/hostport/fake_iptables.go

+	return false, nil
+}
+
+func normalizeRule(rule string) (string, error) {


do we have a shell-command-line-parsing utility somewhere?

maybe this function should panic/fail if it hits something that it's not going to parse correctly (eg, '\"' or '\' [that's backslash space])

oh, or just fix it to normalize the []string form rather than the string form

It's not easy to do []string normalization, because the rules coming from Save()/SaveAll() are already in string-form, and we'd need to split them apart into []string and handle quoted comments.

dcbw · 2016-06-13T22:21:57Z

@freehan @danwinship PTAL, thanks!

dcbw · 2016-06-15T14:48:07Z

go install ./cmd/...
Build was aborted
Aborted by Mike Danese
[xUnit] [INFO] - Starting to record.

dcbw · 2016-06-15T14:49:09Z

@k8s-bot test this issue: #IGNORE

freehan · 2016-06-15T18:25:32Z

pkg/kubelet/network/hostport/hostport.go

+	}
+}
+
+func NewTestHostportHandler(iptables utiliptables.Interface, portOpener hostportOpener) HostportHandler {


Maybe put this in hostport_test?

dcbw · 2016-06-15T20:54:49Z

@freehan @danwinship PTAL, thanks!

freehan · 2016-06-15T21:13:26Z

pkg/kubelet/network/kubenet/kubenet_linux.go

+	runningPods := make([]*hostport.RunningPod, 0)
+	for _, p := range pods {
+		for _, c := range p.Containers {
+			if c.Name != dockertools.PodInfraContainerName {


Add a comment here for something like "Assuming pod specs yield by runtime.GetPods include name and ID of containers in pod"

freehan · 2016-06-15T21:20:57Z

pkg/kubelet/network/kubenet/kubenet_linux.go

+	if err := plugin.setup(namespace, name, id, pod); err != nil {
+		// Make sure everything gets cleaned up on errors
+		podIP, _ := plugin.podIPs[id]
+		plugin.teardown(namespace, name, id, podIP)


Maybe log the err from teardown ?

freehan · 2016-06-15T21:23:50Z

I just have a bunch of nits.

Relying on the runtime to later call cleanup is fragile, so make sure that everything gets nicely cleaned up when setup errors occur.

dcbw · 2016-06-16T18:54:24Z

@freehan updated for your comments, PTAL

freehan · 2016-06-16T20:50:39Z

LGTM. This is great! We should get it in. Maybe for 1.3.1
Pending on tests.

k8s-bot · 2016-06-16T21:30:40Z

GCE e2e build/test passed for commit a519e8a.

k8s-github-robot · 2016-06-18T21:50:49Z

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

k8s-bot · 2016-06-18T22:24:34Z

GCE e2e build/test passed for commit a519e8a.

k8s-github-robot · 2016-06-18T22:24:56Z

Automatic merge from submit-queue

googlebot added the cla: yes label Jun 7, 2016

dcbw force-pushed the split-hostport branch from 1e4e02c to 00ffc2a Compare June 7, 2016 03:02

k8s-github-robot assigned dchen1107 Jun 7, 2016

k8s-github-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. release-note-label-needed labels Jun 7, 2016

dcbw force-pushed the split-hostport branch from 00ffc2a to 02f9d8e Compare June 7, 2016 14:18

dcbw force-pushed the split-hostport branch from 02f9d8e to f5e8568 Compare June 7, 2016 15:22

freehan reviewed Jun 7, 2016
View reviewed changes

freehan self-assigned this Jun 7, 2016

danwinship reviewed Jun 9, 2016
View reviewed changes

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 10, 2016

dcbw force-pushed the split-hostport branch from f5e8568 to 5ad019b Compare June 13, 2016 22:21

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 13, 2016

dcbw force-pushed the split-hostport branch 3 times, most recently from 200f879 to 841e320 Compare June 14, 2016 17:59

freehan reviewed Jun 15, 2016
View reviewed changes

dcbw added 2 commits June 16, 2016 13:44

kubelet/kubenet: split hostport handling into separate module

965492f

kubenet: clean up networking when setup errors occur

a519e8a

Relying on the runtime to later call cleanup is fragile, so make sure that everything gets nicely cleaned up when setup errors occur.

dcbw force-pushed the split-hostport branch from 841e320 to a519e8a Compare June 16, 2016 18:53

freehan added this to the v1.3 milestone Jun 16, 2016

k8s-github-robot merged commit d80b60e into kubernetes:master Jun 18, 2016

eparis removed the cherrypick-candidate label Jun 24, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kubelet/kubenet: split hostport handling into separate module #26934

kubelet/kubenet: split hostport handling into separate module #26934

dcbw commented Jun 7, 2016

dcbw commented Jun 7, 2016

dcbw commented Jun 7, 2016

dcbw commented Jun 7, 2016

dcbw commented Jun 7, 2016

dcbw commented Jun 7, 2016

freehan Jun 7, 2016 •

edited

Loading

freehan Jun 7, 2016

dcbw Jun 7, 2016

dcbw Jun 7, 2016

freehan Jun 7, 2016

dcbw Jun 13, 2016

freehan Jun 13, 2016

dcbw commented Jun 7, 2016

danwinship Jun 9, 2016 •

edited

Loading

danwinship Jun 9, 2016

dcbw Jun 10, 2016

dcbw commented Jun 13, 2016

dcbw commented Jun 15, 2016

dcbw commented Jun 15, 2016

freehan Jun 15, 2016

dcbw commented Jun 15, 2016

freehan Jun 15, 2016

freehan Jun 15, 2016

freehan commented Jun 15, 2016

dcbw commented Jun 16, 2016

freehan commented Jun 16, 2016

k8s-bot commented Jun 16, 2016

k8s-github-robot commented Jun 18, 2016

k8s-bot commented Jun 18, 2016

k8s-github-robot commented Jun 18, 2016

kubelet/kubenet: split hostport handling into separate module #26934

kubelet/kubenet: split hostport handling into separate module #26934

Conversation

dcbw commented Jun 7, 2016

dcbw commented Jun 7, 2016

dcbw commented Jun 7, 2016

dcbw commented Jun 7, 2016

dcbw commented Jun 7, 2016

dcbw commented Jun 7, 2016

freehan Jun 7, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcbw commented Jun 7, 2016

danwinship Jun 9, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcbw commented Jun 13, 2016

dcbw commented Jun 15, 2016

dcbw commented Jun 15, 2016

Choose a reason for hiding this comment

dcbw commented Jun 15, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

freehan commented Jun 15, 2016

dcbw commented Jun 16, 2016

freehan commented Jun 16, 2016

k8s-bot commented Jun 16, 2016

k8s-github-robot commented Jun 18, 2016

k8s-bot commented Jun 18, 2016

k8s-github-robot commented Jun 18, 2016

freehan Jun 7, 2016 •

edited

Loading

danwinship Jun 9, 2016 •

edited

Loading