kubelet: group all container-runtime-specific flags/options into a separate struct #44061

yujuhong · 2017-04-04T22:00:32Z

They don't belong in the KubeletConfig.

This addresses #43253

k8s-reviewable · 2017-04-04T22:01:38Z

This change is

yujuhong · 2017-04-05T00:32:08Z

/cc @Random-Liu

supereagle · 2017-04-05T14:23:39Z

cmd/kubelet/app/options/options.go

@@ -80,6 +80,7 @@ type KubeletFlags struct {
 type KubeletServer struct {
 	KubeletFlags
 	componentconfig.KubeletConfiguration
+	DockerOptions *DockerOptions


Why not take DockerOptions as an embedded field of KubeletServer like KubeletFlags? Then the fields of DockerOptions can be directly accessed.

This is intentional. DockerOptions may be nil in the future.

supereagle · 2017-04-05T14:30:46Z

cmd/kubelet/app/server.go

@@ -156,7 +156,7 @@ func UnsecuredKubeletDeps(s *options.KubeletServer) (*kubelet.KubeletDeps, error
 		KubeClient:         nil,
 		ExternalKubeClient: nil,
 		Mounter:            mounter,
-		NetworkPlugins:     ProbeNetworkPlugins(s.NetworkPluginDir, s.CNIConfDir, s.CNIBinDir),
+		NetworkPlugins:     ProbeNetworkPlugins(s.DockerOptions.NetworkPluginDir, s.DockerOptions.CNIConfDir, s.DockerOptions.CNIBinDir),


If take DockerOptions as an embedded field of KubeletServer, these and other changes are not needed, as the fields of DockerOptions such as s.DockerOptions.NetworkPluginDir can be directly accessed by s.NetworkPluginDir like before.

yujuhong · 2017-04-05T16:55:06Z

Unfortunately some of the flags are shared by rktnetes, which complicates things. I am going to go through the list of options again. Tagging this as WIP.

xiangpengzhao · 2017-04-06T03:21:00Z

cmd/kubelet/app/options/docker.go

+	DockerEndpoint string
+	// networkPluginName is the name of the network plugin to be invoked for
+	// various events in kubelet/pod lifecycle
+	NetworkPluginName string


Why do we take these network flags as docker options?

In the new CRI world, network plugins are container runtime details. kubelet is not aware of them.

The tricky part is that these are still consumed by rkt since they haven't switched to CRI. That's why I marked this PR as WIP.

Thanks for explanation!

xiangpengzhao · 2017-04-06T03:24:04Z

cmd/kubelet/app/options/options.go

@@ -248,8 +241,6 @@ func (c *kubeletConfiguration) addFlags(fs *pflag.FlagSet) {
 	fs.BoolVar(&c.BabysitDaemons, "babysit-daemons", c.BabysitDaemons, "If true, the node has babysitter process monitoring docker and kubelet.")
 	fs.MarkDeprecated("babysit-daemons", "Will be removed in a future version.")
 	fs.Int32Var(&c.MaxPods, "max-pods", c.MaxPods, "Number of Pods that can run on this Kubelet.")
-	// TODO(#40229): Remove the docker-exec-handler flag.
-	fs.StringVar(&c.DockerExecHandlerName, "docker-exec-handler", c.DockerExecHandlerName, "Handler to use when executing a command in a container. Valid values are 'native' and 'nsenter'. Defaults to 'native'.")
 	fs.MarkDeprecated("docker-exec-handler", "this flag will be removed and only the 'native' handler will be supported in the future.")


delete this line?

As the issue states, we can't really remove this yet.

mtaufen · 2017-04-09T02:48:31Z

Should --enable-custom-metrics also get demoted to flag-only?

mtaufen · 2017-04-09T02:53:47Z

And what about fields like:

  // dockerEndpoint is the path to the docker endpoint to communicate with.
  DockerEndpoint string `json:"dockerEndpoint"`

  // dockerExecHandlerName is the handler to use when executing a command
  // in a container. Valid values are 'native' and 'nsenter'. Defaults to
  // 'native'.
  DockerExecHandlerName string `json:"dockerExecHandlerName"`

  // rktPath is the  path of rkt binary. Leave empty to use the first rkt in
  // $PATH.
  RktPath string `json:"rktPath"`

  // rktApiEndpoint is the endpoint of the rkt API service to communicate with.
  RktAPIEndpoint string `json:"rktAPIEndpoint"`

  // rktStage1Image is the image to use as stage1. Local paths and
  // http/https URLs are supported.
  RktStage1Image string `json:"rktStage1Image"

This stuff seems pretty runtime-specific and probably shouldn't be fed through the Kubelet.

yujuhong · 2017-04-13T18:16:59Z

And what about fields like:

// dockerEndpoint is the path to the docker endpoint to communicate with.
DockerEndpoint string json:"dockerEndpoint"

// dockerExecHandlerName is the handler to use when executing a command
// in a container. Valid values are 'native' and 'nsenter'. Defaults to
// 'native'.
DockerExecHandlerName string json:"dockerExecHandlerName"

// rktPath is the path of rkt binary. Leave empty to use the first rkt in
// $PATH.
RktPath string json:"rktPath"

// rktApiEndpoint is the endpoint of the rkt API service to communicate with.
RktAPIEndpoint string json:"rktAPIEndpoint"

// rktStage1Image is the image to use as stage1. Local paths and
// http/https URLs are supported.
RktStage1Image string `json:"rktStage1Image"
This stuff seems pretty runtime-specific and probably shouldn't be fed through the Kubelet.

The docker endpoint and exec handler are already included in the DockerOptions in this PR :-|
Not sure about the rkt fields yet. I am not too keen on creating a generic "container runtime options" since rkt and docker are integrated in very different ways (non-CRI vs. CRI) today.

mtaufen · 2017-04-25T22:35:04Z

Please ping me when this is ready for re-review.

mtaufen

We should also use this PR to mark deprecated any runtime-related flags that are planned for removal from the Kubelet.

yujuhong · 2017-05-25T19:30:25Z

@mtaufen I changed the PR to include all runtime-specific flags. It's not complete though. I think it's ok as a start. Let me know what you think.

mtaufen

Nice work, I had a few comments.

mtaufen · 2017-05-25T20:06:03Z

cmd/kubelet/app/options/options.go

@@ -87,6 +83,7 @@ type KubeletFlags struct {
 // a kubelet. These can either be set via command line or directly.
 type KubeletServer struct {
 	KubeletFlags
+	ContainerRuntimeOptions


Not in this PR, but very soon we'll have to actually name these sub-objects. It's getting very confusing what comes from where in KubeletServer.

Also, would it make sense for these to be subordinate to the KubeletFlags object, rather than directly on the KubeletServer, given that they all come in as flags-only?

I named it in the original PR but changed it to be consistent with other fields in the struct. Will switch it back.

Also, would it make sense for these to be subordinate to the KubeletFlags object, rather than directly on the KubeletServer, given that they all come in as flags-only?

Make sense. We still need to pass the container runtime options as a whole to kubelet, but I think that's fine. Done.

Yeah, that's fine. Thanks!

mtaufen · 2017-05-25T20:22:26Z

pkg/kubemark/hollow_kubelet.go

@@ -172,6 +175,6 @@ func GetHollowKubeletConfig(
 	//                have actually wanted the default).
 	//                The default will be present in the KubeletConfiguration contstructed above.


You can delete this TODO as well. It's old and irrelevant these days.

yujuhong · 2017-05-25T20:58:53Z

We should also use this PR to mark deprecated any runtime-related flags that are planned for removal from the Kubelet.

I'll continue evaluate other options to see what can be moved to this new struct, but I think getting this in would help prevent people from adding more fields to KubeletConfig.
As for deprecating the flags, it's to early to do that since we don't have replacement yet.

mtaufen · 2017-05-25T21:11:48Z

Yeah, SGTM

mtaufen · 2017-05-25T21:12:05Z

/lgtm
/approve

yujuhong · 2017-05-25T23:36:54Z

@k8s-bot node-e2e test this
@k8s-bot e2e-gce-etcd3 test this
@k8s-bot e2e-kops-aws test this

yujuhong · 2017-05-26T18:17:58Z

/cc @gmarek for approval.

yujuhong · 2017-05-26T19:26:10Z

@k8s-bot pull-kubernetes-unit test this

yujuhong · 2017-05-26T23:58:44Z

@k8s-bot pull-kubernetes-unit test this

yujuhong · 2017-05-27T00:14:48Z

@k8s-bot pull-kubernetes-e2e-kops-aws test this

I've been pasting the wrong one :-|

shyamjvs · 2017-05-27T00:29:43Z

/approve

yujuhong · 2017-05-27T00:30:09Z

Assigned to @shyamjvs. I found a new victim to approve the kubemark changes before I have to rebase again :-)

k8s-github-robot · 2017-05-27T00:39:05Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mtaufen, shyamjvs, yujuhong

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

~~cmd/kubelet/OWNERS~~ [yujuhong]
~~pkg/apis/componentconfig/OWNERS~~ [mtaufen]
~~pkg/kubelet/OWNERS~~ [yujuhong]
~~pkg/kubemark/OWNERS~~ [shyamjvs]

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

Do not store them in kubelet's configuration. Eventually, we would like to deprecate all these flags as they should not be part of kubelet.

yujuhong · 2017-05-30T15:11:42Z

Rebased and re-applying the lgtm & approved labels based on #44061 (comment) and #44061 (comment)

k8s-github-robot · 2017-05-31T00:43:51Z

Automatic merge from submit-queue

yujuhong added the release-note-none Denotes a PR that doesn't merit a release note. label Apr 4, 2017

yujuhong self-assigned this Apr 4, 2017

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Apr 4, 2017

k8s-github-robot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 4, 2017

yujuhong force-pushed the docker_flags branch from 9fa529a to 829f8f2 Compare April 4, 2017 22:03

yujuhong changed the title ~~[WIP] kubelet: group all docker flags/options into a separate struct~~ kubelet: group all docker flags/options into a separate struct Apr 5, 2017

yujuhong assigned mtaufen and dchen1107 and unassigned yujuhong Apr 5, 2017

supereagle reviewed Apr 5, 2017

View reviewed changes

yujuhong changed the title ~~kubelet: group all docker flags/options into a separate struct~~ [WIP] kubelet: group all docker flags/options into a separate struct Apr 5, 2017

xiangpengzhao reviewed Apr 6, 2017

View reviewed changes

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

mtaufen reviewed Apr 25, 2017

View reviewed changes

yujuhong force-pushed the docker_flags branch from 829f8f2 to 277312d Compare May 25, 2017 19:26

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

yujuhong changed the title ~~[WIP] kubelet: group all docker flags/options into a separate struct~~ kubelet: group all container-runtime-specific flags/options into a separate struct May 25, 2017

mtaufen reviewed May 25, 2017

View reviewed changes

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

yujuhong force-pushed the docker_flags branch from 0eb9cfc to bb59c48 Compare May 26, 2017 18:21

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

yujuhong force-pushed the docker_flags branch from bb59c48 to 90f5f6f Compare May 26, 2017 18:35

yujuhong assigned gmarek May 26, 2017

yujuhong assigned shyamjvs and unassigned gmarek and wojtek-t May 27, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 27, 2017

Group container-runtime-specific flags/options together

c823502

Do not store them in kubelet's configuration. Eventually, we would like to deprecate all these flags as they should not be part of kubelet.

yujuhong force-pushed the docker_flags branch from 90f5f6f to c823502 Compare May 30, 2017 15:10

yujuhong added approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed approved Indicates a PR has been approved by an approver from all required OWNERS files. labels May 30, 2017

k8s-github-robot merged commit da5edc1 into kubernetes:master May 31, 2017

supereagle mentioned this pull request Jun 1, 2017

mark --network-plugin-dir deprecated for kubelet #46327

Merged

yujuhong mentioned this pull request Jun 5, 2017

Extract container runtime-specific flags from kubelet #43253

Closed

yujuhong mentioned this pull request Aug 17, 2017

Remove legacy docker integration (a.k.a dockertools/docker_manager.go) #43234

Closed

3 tasks

yujuhong deleted the docker_flags branch September 11, 2017 20:34

tallclair mentioned this pull request Jan 22, 2024

[Proposal] Add most non-deprecated flags to the Kubelet configuration API #122916

Closed

14 tasks

		@@ -172,6 +175,6 @@ func GetHollowKubeletConfig(
		// have actually wanted the default).
		// The default will be present in the KubeletConfiguration contstructed above.

kubelet: group all container-runtime-specific flags/options into a separate struct #44061

kubelet: group all container-runtime-specific flags/options into a separate struct #44061

Conversation

yujuhong commented Apr 4, 2017 • edited

k8s-reviewable commented Apr 4, 2017

yujuhong commented Apr 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yujuhong commented Apr 5, 2017

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

mtaufen commented Apr 9, 2017

mtaufen commented Apr 9, 2017

yujuhong commented Apr 13, 2017

mtaufen commented Apr 25, 2017

mtaufen left a comment

Choose a reason for hiding this comment

yujuhong commented May 25, 2017

mtaufen left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yujuhong commented May 25, 2017

mtaufen commented May 25, 2017

mtaufen commented May 25, 2017

yujuhong commented May 25, 2017

yujuhong commented May 26, 2017

yujuhong commented May 26, 2017

yujuhong commented May 26, 2017

yujuhong commented May 27, 2017

shyamjvs commented May 27, 2017

yujuhong commented May 27, 2017

k8s-github-robot commented May 27, 2017

yujuhong commented May 30, 2017

k8s-github-robot commented May 31, 2017

yujuhong commented Apr 4, 2017 •

edited