kubelet: make dockershim.sock configurable

[ncdc]

What this PR does / why we need it: allow the path to dockershim.sock to be configurable, so downstream projects such as OpenShift can run integration tests without needing to run them as root

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged):

Special notes for your reviewer:

Release note:

cc @derekwaynecarr @sttts @kubernetes/rh-cluster-infra @kubernetes/sig-node-pr-reviews

[ncdc]

cc @mtaufen as this touches kubelet config

[k8s-reviewable]

This change is 

[yujuhong]

I understand this should be configurable, but I am not sure if it belongs to the KubeletConfiguration.
I think we can add it to KubeletFlags (#40117) for now, and work on extracting docker-specific flags/configurations later (#43253). WDYT?

[ncdc]

I haven't kept up with where things are headed so I'll defer to sig node for where to put it. Thanks!

…

On Fri, Mar 31, 2017 at 1:53 PM Yu-Ju Hong ***@***.***> wrote: I understand this should be configurable, but I am not sure if it belongs to the KubeletConfiguration. I think we can add it to KubeletFlags (#40117 <#40117>) for now, and work on extracting docker-specific flags/configurations later (#43253 <#43253>). WDYT? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#43914 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAABYsghrgZMC6jF_LmkkNBDmJDrGKaJks5rrT2HgaJpZM4Mvzcf> .

[ncdc]

@yujuhong the reason I put it in KubeletConfiguration is because, at least for now, that's what is passed in to NewMainKubelet (along with KubeletDeps), and it made more sense to me to put it into the former.

If we put it in KubeletFlags, how would we get the value down where it needs to go? Would you see NewMainKubelet taking in flags, config, and deps?

[derekwaynecarr]

@ncdc same question. i need to catch up on the flags pr, but at first glance, its not clear how that would propagate down unless we also exposed it on kubelet configuration.

[yujuhong]

@mtaufen's current PR passes individual args from KubeletFlags to NewMainKubelet, but I don't see why we can't pass a separate struct to the function (yet). Maybe @mtaufen have more thoughts on this...

I'd like to keep dockershim configuration out of kubelet if possible. Since the shim can be started and configured separately (theoretically), it doesn't seem right to store it as part of the kubelet config. Suggestions are welcome though since I haven't spent too much thought on this.

[ncdc]

@yujuhong is this code that's currently in the kubelet moving somewhere else?

kubeCfg.RemoteRuntimeEndpoint, kubeCfg.RemoteImageEndpoint = kubeCfg.DockerShimSocket, kubeCfg.DockerShimSocket

glog.V(2).Infof("Starting the GRPC server for the docker CRI shim.")
server := dockerremote.NewDockerServer(kubeCfg.DockerShimSocket, ds)

[yujuhong]

@ncdc not yet, but it's a possibility. I'd like to try cleaning up and decoupling the dockershim and kubelet code further in Q2, but not sure how much we can actually achieve.

[mtaufen]

An annoying thing about the cmd/pkg split is that "commands have flags" so if you want the KubeletFlags struct to be defined in a reasonable place (cmd) and you also want to use it in pkg (a NewMainKubelet arg), you have to create a circular relationship. It's not so circular that it angers the compiler, but it's still something I'd rather avoid.

I've been passing in additional arguments in the meantime. I also want to avoid this, because it bloats NewMainKubelet's args and I did a ton of work last year to cut those down to the few left today.

If we could separate the static configuration from the dynamic configuration, I'd be comfortable exposing e.g. StaticKubeletConfiguration and DynamicKubeletConfiguration from pkg/kubelet and only ever mapping flags to things in StaticKubeletConfiguration. In fact I'd like to do this soon, because KubeletBuilder is exposed from pkg and its signature requires the same information as NewMainKubelet's - which means people depend on a thing that will likely bloat along with NewMainKubelet.

As long as we do something like this soon, I'm fine with additional args in NewMainKubelet for now, as a matter of expedience.

[ncdc]

@yujuhong updated based on our slack conversation - ptal!

[ncdc]

@k8s-bot non-cri e2e test this #43977

[yujuhong]

/approve
LGTM.

nit: Add a description in the commit explaining what the change is.

[ncdc]

@yujuhong commit comment updated. Does this work for you?

[yujuhong]

/lgtm

[k8s-github-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ncdc, yujuhong

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

• pkg/kubelet/OWNERS [yujuhong]

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

[ncdc]

@ixdy @spxtr @apelisse for some reason the current status is Required Github CI test is not green: Jenkins Bazel Build when it says Jenkins Bazel Build — Build succeeded.. I'm not sure what's up so I'm rekicking bazel.

@k8s-bot bazel test this

[ncdc]

Weird, it's still saying bazel isn't green when it just passed again. What's going on?

[ncdc]

Oh there it goes, nevermind

[k8s-github-robot]

Automatic merge from submit-queue (batch tested with PRs 43373, 41780, 44141, 43914, 44180)

[ixdy]

@ncdc I think the munger/SQ polls, so sometimes it can be a bit behind reality.