SDN options and CNI directory cleanup #18404

dcbw · 2018-02-01T23:15:30Z

Remove un-needed kubelet flags and rationalize paths that kubelet and the SDN use.

@knobunc @openshift/networking @danwinship

pravisankar

Instead of hard-coding cniDirPath="/etc/cni/net.d" in pkg/network/node/node.go,
we can actually get the CNI conf dir:

  cniDirPath = kubeletArguments["cni-conf-dir"][0] if present, otherwise kubeletcni.DefaultNetDir

By doing this, we do not have to change anything in openshift-sdn when upstream changes these default values.

pravisankar · 2018-02-02T00:31:01Z

Similar to DefaultCNIDir in k8s.io/kubernetes/pkg/kubelet/network/kubenet/kubenet_linux.go, I think we should expose defaultIPAMDir in upstream and reuse it in:
pkg/network/node/metrics.go: items, err := ioutil.ReadDir("/var/lib/cni/networks/openshift-sdn/")
Otherwise, if upstream changes this default value for some reason we may not notice until we see some breakage.

pravisankar · 2018-02-02T00:47:02Z

vendor/github.com/containernetworking/plugins/plugins/main/loopback/loopback.go

@@ -1,72 +0,0 @@
-// Copyright 2016 CNI authors


Do we want to remove these files from vendor dir? Removing this plugin from openshift packaging/installation is not sufficient?
I see other plugins under vendor/github.com/containernetworking/plugins/plugins/main which are not directly used by openshift SDN.

@pravisankar yeah, those exist there. we don't build or install them though, so we could either remove them from Godeps or just leave them for now. No reason to have those sources around if we don't use them I think.

pravisankar · 2018-02-02T00:48:32Z

/retest

danwinship · 2018-02-02T15:29:37Z

Similar to DefaultCNIDir in k8s.io/kubernetes/pkg/kubelet/network/kubenet/kubenet_linux.go, I think we should expose defaultIPAMDir in upstream and reuse it in:
pkg/network/node/metrics.go: items, err := ioutil.ReadDir("/var/lib/cni/networks/openshift-sdn/")

Even if they expose defaultIPAMDir, we'd still be making assumptions about the format it stores the data in. So we'd need them to expose a "getMetrics()" function or something.

Another possibility is to just add a check to the extended networking test that just verifies that the data there is what we expect.

The CNI directory arguments are already the defaults in cni.go and we shouldn't need to change them (unless upstream changes them, at which point we should probably match). The Hairpin mode has been ignored upstream for CNI plugins since the switch to CRI. It's up to CNI network plugins to do what they need to do WRT allowing pods to talk back to themselves via the service IP. (note that hairpin is still used for --network-plugin=kubenet and for the old rkt runtime).

dcbw · 2018-02-02T17:35:11Z

@pravisankar @danwinship so I forgot that the kubelet CNI driver uses loopback, so I've dropped that commit.

pravisankar · 2018-02-02T20:16:55Z

pkg/network/node/pod.go

@@ -153,7 +154,8 @@ func getIPAMConfig(clusterNetworks []common.ClusterNetwork, localSubnet string)
 		Name:       "openshift-sdn",
 		Type:       "openshift-sdn",
 		IPAM: &hostLocalIPAM{
-			Type: "host-local",
+			Type:    "host-local",
+			DataDir: hostLocalDataDir,


Where is hostLocalIPAM.DataDir used?

@pravisankar it's serialized into JSON and passed to the host-local plugin, which then puts its IPAM data there instead of the default. In reality hostLocalDataDir is already the same as the host-local plugin default so no change. But the path will be consistent now.

pravisankar

host-local plugin bin dir path "/opt/cni/bin" is used in createIPAMArgs, ipamAdd and ipamDel methods. May be we could define hostLocalBinDir="/opt/cni/bin"?
Rest of the changes looks good.

Instead of defining things in a couple places, just use the Kubelet CNI driver conf dir define. If that's overridden via the kubelet --cni-conf-dir argument, use that instead. Also pass a known IPAM data dir to the host-local plugin, and use that in the metrics code, to protect against upstream changes to the defaults.

dcbw · 2018-02-02T22:36:10Z

host-local plugin bin dir path "/opt/cni/bin" is used in createIPAMArgs, ipamAdd and ipamDel methods. May be we could define hostLocalBinDir="/opt/cni/bin"?

@pravisankar done

pravisankar · 2018-02-03T00:30:46Z

pkg/network/node/pod.go

@@ -42,6 +43,7 @@ import (

 const (
 	podInterfaceName = knetwork.DefaultInterfaceName
+	cniBinPath       = kcni.DefaultCNIDir


As a follow up, we could add a validation to check kubeletArguments["cni-bin-path"] is not set when openshift network plugin is used.

Addressed in #18464

pravisankar · 2018-02-03T00:32:38Z

/lgtm
/approve

pravisankar · 2018-02-03T00:34:56Z

/retest

dcbw · 2018-02-20T22:45:26Z

/approve

openshift-ci-robot · 2018-02-20T22:45:50Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dcbw, pravisankar

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

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

~~pkg/cmd/OWNERS~~ [dcbw]
~~pkg/network/OWNERS~~ [dcbw,pravisankar]

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

openshift-merge-robot · 2018-02-20T22:47:46Z

/test all [submit-queue is verifying that this PR is safe to merge]

openshift-merge-robot · 2018-02-21T00:18:41Z

Automatic merge from submit-queue.

Data directory passed to the CNI host-local delegate plugin should be /var/lib/cni/networks, but the original patch missed one place to append "/networks" (the one that mattered). Fix-up for openshift#18404 Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1549491

Automatic merge from submit-queue. sdn: fix CNI IPAM data dir Data directory passed to the CNI host-local delegate plugin should be /var/lib/cni/networks, but the original patch missed one place to append "/networks" (the one that mattered). Fix-up for #18404 Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1549491 @openshift/networking

Data directory passed to the CNI host-local delegate plugin should be /var/lib/cni/networks, but the original patch missed one place to append "/networks" (the one that mattered). Fix-up for openshift#18404 Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1549491

Data directory passed to the CNI host-local delegate plugin should be /var/lib/cni/networks, but the original patch missed one place to append "/networks" (the one that mattered). Fix-up for openshift/origin#18404 Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1549491

openshift-ci-robot requested review from danwinship, JacobTanenbaum, liggitt, soltysh and stevekuznetsov February 1, 2018 23:15

openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Feb 1, 2018

openshift-merge-robot added the vendor-update Touching vendor dir or related files label Feb 1, 2018

pravisankar added the component/networking label Feb 1, 2018

pravisankar reviewed Feb 2, 2018

View reviewed changes

dcbw changed the title ~~SDN options and loopback plugin cleanup~~ SDN options and CNI directory cleanup Feb 2, 2018

dcbw force-pushed the sdn-options-loopback-cleanup branch from e8de175 to 9341a78 Compare February 2, 2018 19:01

openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 2, 2018

openshift-merge-robot removed the vendor-update Touching vendor dir or related files label Feb 2, 2018

dcbw force-pushed the sdn-options-loopback-cleanup branch from 9341a78 to 89afcf3 Compare February 2, 2018 20:09

pravisankar reviewed Feb 2, 2018

View reviewed changes

dcbw force-pushed the sdn-options-loopback-cleanup branch from 89afcf3 to 10c212d Compare February 2, 2018 22:30

pravisankar reviewed Feb 3, 2018

View reviewed changes

pravisankar approved these changes Feb 3, 2018

View reviewed changes

openshift-ci-robot assigned pravisankar Feb 3, 2018

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 3, 2018

pravisankar added the kind/bug Categorizes issue or PR as related to a bug. label Feb 3, 2018

soltysh removed their request for review February 5, 2018 15:28

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 20, 2018

openshift-merge-robot merged commit 2919077 into openshift:master Feb 21, 2018

dcbw mentioned this pull request Feb 28, 2018

sdn: fix CNI IPAM data dir #18779

Merged

SDN options and CNI directory cleanup #18404

SDN options and CNI directory cleanup #18404

Uh oh!

Conversation

dcbw commented Feb 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pravisankar left a comment

Choose a reason for hiding this comment

Uh oh!

pravisankar commented Feb 2, 2018

Uh oh!

pravisankar Feb 2, 2018

Choose a reason for hiding this comment

Uh oh!

dcbw Feb 2, 2018

Choose a reason for hiding this comment

Uh oh!

pravisankar commented Feb 2, 2018

Uh oh!

danwinship commented Feb 2, 2018

Uh oh!

dcbw commented Feb 2, 2018

Uh oh!

pravisankar Feb 2, 2018

Choose a reason for hiding this comment

Uh oh!

dcbw Feb 2, 2018

Choose a reason for hiding this comment

Uh oh!

pravisankar left a comment

Choose a reason for hiding this comment

Uh oh!

dcbw commented Feb 2, 2018

Uh oh!

pravisankar Feb 3, 2018

Choose a reason for hiding this comment

Uh oh!

pravisankar Feb 6, 2018

Choose a reason for hiding this comment

Uh oh!

pravisankar commented Feb 3, 2018

Uh oh!

pravisankar commented Feb 3, 2018

Uh oh!

dcbw commented Feb 20, 2018

Uh oh!

openshift-ci-robot commented Feb 20, 2018

Uh oh!

openshift-merge-robot commented Feb 20, 2018

Uh oh!

openshift-merge-robot commented Feb 21, 2018

Uh oh!

Uh oh!

dcbw commented Feb 1, 2018 •

edited

Loading