-
Notifications
You must be signed in to change notification settings - Fork 39.8k
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
kubelet: make dockershim.sock configurable #43914
Conversation
cc @mtaufen as this touches kubelet config |
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>
.
|
@yujuhong the reason I put it in If we put it in |
@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. |
@mtaufen's current PR passes individual args from 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. |
@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) |
@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. |
An annoying thing about the I've been passing in additional arguments in the meantime. I also want to avoid this, because it bloats If we could separate the static configuration from the dynamic configuration, I'd be comfortable exposing e.g. As long as we do something like this soon, I'm fine with additional args in |
b00c4bb
to
2c82578
Compare
@yujuhong updated based on our slack conversation - ptal! |
/approve nit: Add a description in the commit explaining what the change is. |
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.
2c82578
to
010b71a
Compare
@yujuhong commit comment updated. Does this work for you? |
/lgtm |
[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 |
Weird, it's still saying bazel isn't green when it just passed again. What's going on? |
Oh there it goes, nevermind |
Automatic merge from submit-queue (batch tested with PRs 43373, 41780, 44141, 43914, 44180) |
@ncdc I think the munger/SQ polls, so sometimes it can be a bit behind reality. |
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