Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

kubelet: make dockershim.sock configurable #43914

Merged

Conversation

ncdc
Copy link
Member

@ncdc ncdc commented Mar 31, 2017

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 ncdc added release-note-none Denotes a PR that doesn't merit a release note. sig/node Categorizes an issue or PR as relevant to SIG Node. labels Mar 31, 2017
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 31, 2017
@k8s-github-robot k8s-github-robot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Mar 31, 2017
@ncdc
Copy link
Member Author

ncdc commented Mar 31, 2017

cc @mtaufen as this touches kubelet config

@k8s-reviewable
Copy link

This change is Reviewable

@yujuhong
Copy link
Contributor

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
Copy link
Member Author

ncdc commented Mar 31, 2017 via email

@ncdc
Copy link
Member Author

ncdc commented Mar 31, 2017

@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
Copy link
Member

@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
Copy link
Contributor

@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
Copy link
Member Author

ncdc commented Mar 31, 2017

@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
Copy link
Contributor

@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
Copy link
Contributor

mtaufen commented Apr 1, 2017

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.

@yujuhong yujuhong added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Apr 3, 2017
@ncdc ncdc force-pushed the configurable-dockershim-socket branch from b00c4bb to 2c82578 Compare April 5, 2017 20:01
@k8s-github-robot k8s-github-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Apr 5, 2017
@ncdc
Copy link
Member Author

ncdc commented Apr 5, 2017

@yujuhong updated based on our slack conversation - ptal!

@ncdc
Copy link
Member Author

ncdc commented Apr 5, 2017

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

@yujuhong
Copy link
Contributor

yujuhong commented Apr 5, 2017

/approve
LGTM.

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

@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 5, 2017
Make the location of dockershim.sock configurable, so downstream
projects (such as OpenShift) can place it in a location that does not
require root access (e.g. for integration tests).

Make the kubelet respect and use the values of
--container-runtime-endpoint and --image-service-endpoint, if set. If
unset, the default value of /var/run/dockershim.sock is used.
@ncdc ncdc force-pushed the configurable-dockershim-socket branch from 2c82578 to 010b71a Compare April 6, 2017 16:08
@ncdc
Copy link
Member Author

ncdc commented Apr 6, 2017

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

@yujuhong
Copy link
Contributor

yujuhong commented Apr 6, 2017

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 6, 2017
@k8s-github-robot
Copy link

[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:

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

@ncdc
Copy link
Member Author

ncdc commented Apr 7, 2017

@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
Copy link
Member Author

ncdc commented Apr 7, 2017

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

@ncdc
Copy link
Member Author

ncdc commented Apr 7, 2017

Oh there it goes, nevermind

@k8s-github-robot
Copy link

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

@k8s-github-robot k8s-github-robot merged commit 19991ca into kubernetes:master Apr 7, 2017
@ixdy
Copy link
Member

ixdy commented Apr 7, 2017

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

@ncdc ncdc deleted the configurable-dockershim-socket branch April 18, 2017 18:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/node Categorizes an issue or PR as relevant to SIG Node. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants