Add flag to set CNI bin dir, and use it on gci nodes #32151

bboreham · 2016-09-06T19:55:42Z

What this PR does / why we need it:

When using kube-up on GCE, following #31023 which moved the workers from debian to gci, CNI just isn't working. The root cause is basically as discussed in #28563: one flag (--network-plugin-dir) means two different things, and the configure-helper script uses it for the wrong purpose.

This PR adds a new flag --cni-bin-dir, then uses it to configure CNI as desired.

As discussed at #28563, I have also added a flag --cni-conf-dir so users can be explicit

Which issue this PR fixes : fixes #28563

Special notes for your reviewer:

I left the old flag largely alone for backwards-compatibility, with the exception that I stop setting the default when CNI is in use. The value of "/usr/libexec/kubernetes/kubelet-plugins/net/exec/" is unlikely to be what is wanted there.

Release note:

Added new kubelet flags `--cni-bin-dir` and `--cni-conf-dir` to specify where CNI files are located.
Fixed CNI configuration on GCI platform when using CNI.

This change is

k8s-ci-robot · 2016-09-06T19:55:42Z

Can a kubernetes member verify that this patch is reasonable to test? If so, please reply "ok to test".

Regular contributors should join the org to skip this step.

googlebot · 2016-09-07T10:06:35Z

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

googlebot · 2016-09-07T13:14:56Z

CLAs look good, thanks!

bboreham · 2016-09-07T13:18:26Z

Updated and rebased. @freehan this is the PR I mentioned in #27027.

bboreham · 2016-09-07T13:21:18Z

Oh, one question: the default for NetworkPluginDir is in pkg/apis/componentconfig/v1alpha1/defaults.go but the defaults for CNI dirs are in pkg/kubelet/network/cni/cni.go. Is it desirable to bring those into alignment?

errordeveloper · 2016-09-07T15:46:50Z

cmd/kubelet/app/options/options.go

@@ -147,7 +147,9 @@ func (s *KubeletServer) AddFlags(fs *pflag.FlagSet) {
 	fs.Int32Var(&s.LowDiskSpaceThresholdMB, "low-diskspace-threshold-mb", s.LowDiskSpaceThresholdMB, "The absolute free disk space, in MB, to maintain. When disk space falls below this threshold, new pods would be rejected. Default: 256")
 	fs.DurationVar(&s.VolumeStatsAggPeriod.Duration, "volume-stats-agg-period", s.VolumeStatsAggPeriod.Duration, "Specifies interval for kubelet to calculate and cache the volume disk usage for all pods and volumes.  To disable volume calculations, set to 0.  Default: '1m'")
 	fs.StringVar(&s.NetworkPluginName, "network-plugin", s.NetworkPluginName, "<Warning: Alpha feature> The name of the network plugin to be invoked for various events in kubelet/pod lifecycle")
-	fs.StringVar(&s.NetworkPluginDir, "network-plugin-dir", s.NetworkPluginDir, "<Warning: Alpha feature> The full path of the directory in which to search for network plugins")
+	fs.StringVar(&s.NetworkPluginDir, "network-plugin-dir", s.NetworkPluginDir, "<Warning: Alpha feature> The full path of the directory in which to search for network plugins or CNI config")


You probably want to mark it deprecated...

That would only be appropriate for the CNI usage of the flag; there is another meaning for non-CNI plugins.
Maybe non-CNI plugins should be deprecated, but that's another conversation.

errordeveloper · 2016-09-07T15:48:31Z

I think you also need to run make generated_files, as you have updated types.go.

bboreham · 2016-09-07T15:59:02Z

I think you also need to run make generated_files, as you have updated types.go

OK, have run that and checked in the resulting changes. Thanks!

caseydavenport · 2016-09-07T16:07:42Z

cmd/kubelet/app/options/options.go

@@ -147,7 +147,9 @@ func (s *KubeletServer) AddFlags(fs *pflag.FlagSet) {
 	fs.Int32Var(&s.LowDiskSpaceThresholdMB, "low-diskspace-threshold-mb", s.LowDiskSpaceThresholdMB, "The absolute free disk space, in MB, to maintain. When disk space falls below this threshold, new pods would be rejected. Default: 256")
 	fs.DurationVar(&s.VolumeStatsAggPeriod.Duration, "volume-stats-agg-period", s.VolumeStatsAggPeriod.Duration, "Specifies interval for kubelet to calculate and cache the volume disk usage for all pods and volumes.  To disable volume calculations, set to 0.  Default: '1m'")
 	fs.StringVar(&s.NetworkPluginName, "network-plugin", s.NetworkPluginName, "<Warning: Alpha feature> The name of the network plugin to be invoked for various events in kubelet/pod lifecycle")
-	fs.StringVar(&s.NetworkPluginDir, "network-plugin-dir", s.NetworkPluginDir, "<Warning: Alpha feature> The full path of the directory in which to search for network plugins")
+	fs.StringVar(&s.NetworkPluginDir, "network-plugin-dir", s.NetworkPluginDir, "<Warning: Alpha feature> The full path of the directory in which to search for network plugins or CNI config")
+	fs.StringVar(&s.CNIConfDir, "cni-conf-dir", s.CNIConfDir, "<Warning: Alpha feature> The full path of the directory in which to search for CNI config files")


Could we include the default values for these options? Most of the other options have them listed.

Sure; any views on my question about which file the default should be implemented in?

dcbw · 2016-09-08T23:02:35Z

Oh, one question: the default for NetworkPluginDir is in pkg/apis/componentconfig/v1alpha1/defaults.go but the defaults for CNI dirs are in pkg/kubelet/network/cni/cni.go. Is it desirable to bring those into alignment?

I feel like the defaults should be left up to each plugin type, either exec or CNI. I think that was just an omission in Rajat's original CNI branch. I don't think a kubelet global default is really appropriate here, since different plugins have different conventions. So maybe remove the default from defaults.go and put it into exec.go?

Edit: that said, I think we can do CNI defaults in the Flags() calls though, since these are obviously CNI specific. I just meant that the non-CNI defualt dir, which is left-over from exec, should be moved out of defaults.go and into exec.go somewhere, and --network-plugin-dir would be left without a default value, which each plugin could fill in by itself.

Also make clearer the function of --network-plugin-dir when using CNI

freehan · 2016-09-13T21:33:05Z

I think you have to run hack/update-codecgen.sh once again.

freehan · 2016-09-13T22:04:21Z

Thanks. LGTM. assuming tests will pass.

k8s-bot · 2016-09-13T22:23:40Z

GCE e2e build/test passed for commit 8a69683.

k8s-github-robot · 2016-09-13T22:59:21Z

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

k8s-bot · 2016-09-13T23:41:41Z

GCE e2e build/test passed for commit 8a69683.

k8s-github-robot · 2016-09-13T23:42:06Z

Automatic merge from submit-queue

Automatic merge from submit-queue Add flag to set CNI bin dir, and use it on gci nodes **What this PR does / why we need it**: When using `kube-up` on GCE, following kubernetes#31023 which moved the workers from debian to gci, CNI just isn't working. The root cause is basically as discussed in kubernetes#28563: one flag (`--network-plugin-dir`) means two different things, and the `configure-helper` script uses it for the wrong purpose. This PR adds a new flag `--cni-bin-dir`, then uses it to configure CNI as desired. As discussed at kubernetes#28563, I have also added a flag `--cni-conf-dir` so users can be explicit **Which issue this PR fixes** : fixes kubernetes#28563 **Special notes for your reviewer**: I left the old flag largely alone for backwards-compatibility, with the exception that I stop setting the default when CNI is in use. The value of `"/usr/libexec/kubernetes/kubelet-plugins/net/exec/"` is unlikely to be what is wanted there. **Release note**: ```release-note Added new kubelet flags `--cni-bin-dir` and `--cni-conf-dir` to specify where CNI files are located. Fixed CNI configuration on GCI platform when using CNI. ``` (cherry picked from commit c4893df)

k8s-cherrypick-bot · 2016-09-15T00:04:21Z

Commit found in the "release-1.4" branch appears to be this PR. Removing the "cherrypick-candidate" label. If this is an error find help to get your PR picked.

kubernetes/kubernetes#32151

New default (for k8s 1.4) set by this PR kubernetes/kubernetes#32151

kubernetes/kubernetes#32151

Automatic merge from submit-queue Add flag to set CNI bin dir, and use it on gci nodes **What this PR does / why we need it**: When using `kube-up` on GCE, following kubernetes#31023 which moved the workers from debian to gci, CNI just isn't working. The root cause is basically as discussed in kubernetes#28563: one flag (`--network-plugin-dir`) means two different things, and the `configure-helper` script uses it for the wrong purpose. This PR adds a new flag `--cni-bin-dir`, then uses it to configure CNI as desired. As discussed at kubernetes#28563, I have also added a flag `--cni-conf-dir` so users can be explicit **Which issue this PR fixes** : fixes kubernetes#28563 **Special notes for your reviewer**: I left the old flag largely alone for backwards-compatibility, with the exception that I stop setting the default when CNI is in use. The value of `"/usr/libexec/kubernetes/kubelet-plugins/net/exec/"` is unlikely to be what is wanted there. **Release note**: ```release-note Added new kubelet flags `--cni-bin-dir` and `--cni-conf-dir` to specify where CNI files are located. Fixed CNI configuration on GCI platform when using CNI. ``` (cherry picked from commit c4893df)

As per kubernetes/kubernetes#32151, kubelet doesn't search for CNI configs in /usr/libexec/kubernetes/kubelet-plugins/net/exec/ by default anymore. Signed-off-by: Martynas Pumputis <martynas@covalent.io>

As per kubernetes/kubernetes#32151, kubelet doesn't search for CNI configs in /usr/libexec/kubernetes/kubelet-plugins/net/exec/ by default anymore. Signed-off-by: Martynas Pumputis <m@lambda.lt>

googlebot added the cla: yes label Sep 6, 2016

k8s-github-robot assigned yujuhong Sep 6, 2016

k8s-github-robot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note-label-needed labels Sep 6, 2016

bboreham force-pushed the fix-cni-on-gci branch 3 times, most recently from e35fae3 to 4503359 Compare September 7, 2016 10:06

googlebot added cla: no and removed cla: yes labels Sep 7, 2016

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 7, 2016

bboreham force-pushed the fix-cni-on-gci branch from 4503359 to 144f06c Compare September 7, 2016 13:14

googlebot added cla: yes and removed cla: no labels Sep 7, 2016

k8s-github-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 Sep 7, 2016

errordeveloper reviewed Sep 7, 2016
View reviewed changes

caseydavenport reviewed Sep 7, 2016
View reviewed changes

bboreham added 3 commits September 9, 2016 11:43

Add options to set CNI config and binary directories

4e961e2

Also make clearer the function of --network-plugin-dir when using CNI

Move default directory for exec plugin into exec plugin

a2e583a

Use --cni-bin-dir flag when using CNI on GCI

fd4596b

Regenerate codecgen files

8a69683

freehan added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 13, 2016

k8s-github-robot mentioned this pull request Sep 13, 2016

[k8s.io] Kubectl client [k8s.io] Guestbook application should create and stop a working application [Conformance] {Kubernetes e2e suite} #31884

Closed

freehan mentioned this pull request Sep 13, 2016

Automated cherry pick of #32151 #32617

Closed

k8s-github-robot merged commit c4893df into kubernetes:master Sep 13, 2016

freehan added the cherrypick-candidate label Sep 14, 2016

eparis added the cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. label Sep 14, 2016

eparis mentioned this pull request Sep 14, 2016

Release 1.4 Cherrypick Batch #32717

Merged

k8s-cherrypick-bot removed the cherrypick-candidate label Sep 15, 2016

peebs pushed a commit to peebs/coreos-kubernetes that referenced this pull request Sep 19, 2016

all: replace --network-plugin-dir with new flag

8843a32

kubernetes/kubernetes#32151

tomdee added a commit to tomdee/minikube-iso that referenced this pull request Sep 21, 2016

rootfs: Use the correct location for CNI config

edf4da8

New default (for k8s 1.4) set by this PR kubernetes/kubernetes#32151

tomdee mentioned this pull request Sep 21, 2016

rootfs: Use the correct location for CNI config coreos/minikube-iso#23

Closed

errordeveloper mentioned this pull request Sep 22, 2016

CNI cosmetics kubernetes/release#113

Closed

bboreham mentioned this pull request Oct 20, 2016

Update documentation about CNI flags and requirements kubernetes/website#1516

Merged

mumoshu pushed a commit to mumoshu/coreos-kubernetes-filtered that referenced this pull request Oct 21, 2016

all: replace --network-plugin-dir with new flag

d592948

kubernetes/kubernetes#32151

bboreham mentioned this pull request Oct 23, 2016

Default plugin directory is /usr/libexec/kubernetes/kubelet-plugins/net/exec/ for CNI #25937

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add flag to set CNI bin dir, and use it on gci nodes #32151

Add flag to set CNI bin dir, and use it on gci nodes #32151

bboreham commented Sep 6, 2016 •

edited

Loading

k8s-ci-robot commented Sep 6, 2016

googlebot commented Sep 7, 2016

googlebot commented Sep 7, 2016

bboreham commented Sep 7, 2016

bboreham commented Sep 7, 2016

errordeveloper Sep 7, 2016

bboreham Sep 7, 2016

errordeveloper commented Sep 7, 2016

bboreham commented Sep 7, 2016

caseydavenport Sep 7, 2016

bboreham Sep 7, 2016

dcbw commented Sep 8, 2016 •

edited

Loading

freehan commented Sep 13, 2016

freehan commented Sep 13, 2016

k8s-bot commented Sep 13, 2016

k8s-github-robot commented Sep 13, 2016

k8s-bot commented Sep 13, 2016

k8s-github-robot commented Sep 13, 2016

k8s-cherrypick-bot commented Sep 15, 2016

Add flag to set CNI bin dir, and use it on gci nodes #32151

Add flag to set CNI bin dir, and use it on gci nodes #32151

Conversation

bboreham commented Sep 6, 2016 • edited Loading

k8s-ci-robot commented Sep 6, 2016

googlebot commented Sep 7, 2016

googlebot commented Sep 7, 2016

bboreham commented Sep 7, 2016

bboreham commented Sep 7, 2016

errordeveloper Sep 7, 2016

Choose a reason for hiding this comment

bboreham Sep 7, 2016

Choose a reason for hiding this comment

errordeveloper commented Sep 7, 2016

bboreham commented Sep 7, 2016

caseydavenport Sep 7, 2016

Choose a reason for hiding this comment

bboreham Sep 7, 2016

Choose a reason for hiding this comment

dcbw commented Sep 8, 2016 • edited Loading

freehan commented Sep 13, 2016

freehan commented Sep 13, 2016

k8s-bot commented Sep 13, 2016

k8s-github-robot commented Sep 13, 2016

k8s-bot commented Sep 13, 2016

k8s-github-robot commented Sep 13, 2016

k8s-cherrypick-bot commented Sep 15, 2016

bboreham commented Sep 6, 2016 •

edited

Loading

dcbw commented Sep 8, 2016 •

edited

Loading