-
Notifications
You must be signed in to change notification settings - Fork 446
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
Update the default CRI server #599
Update the default CRI server #599
Conversation
/assign @feiskyer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see comment
Feedback from kubernetes-sigs#599 (review) Signed-off-by: Martin Hickey <martin.hickey@ie.ibm.com>
Thanks for the review and feedback @mikebrow. Updated and ready for review again. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see comment
I prefer we still use dockershim as the default CRI server because most Kubernetes users are still using dockershim. To switch to a different runtime, users could provide a config file for crictl.
This would introduce a few seconds latency for containerd and crio. @hickeyma @mikebrow do you think this is ok for users? I have another idea. @hickeyma @mikebrow what do you think if we add a sub-command to generate the config file based on runtime type? |
I don't think the intention is to change the default, or introduce any additional latency for the default or configured options or cli options. Though it would be good to put out a warning that automatically using docker as the default is being deprecated, please use the config or cli option. We are heading into an era where two or more of these container runtimes will always be running and the customer won't know which he's using. The idea here was to add loud warning messages when there is no config or cli, and further add more warning when the default docker fails but there exists a running sock from another container runtime besides the default docker, then go ahead and use the backup.. but yes the wasted time trying docker.. is just wasted time if crio or containerd is desired by the user/developer thus loud complaining required. If preferred could even "fail" hard with the message you need to put
There's a config sub-command that's supposed to have get and set options ala git config pattern... the get works the set isn't there. |
@mikebrow thanks for the clarification. |
Currently, the default CRI server is dockershim. This is a deprecated server with cri-tools. Also, does it make sense to even have a default as the user should explicitly define it. This PR adds `containerd` and `cri-o` to the default list and it checks each in order till one works. It also adds some looging that the defaults are being used. This at least provdes better usability to the user if they don't configure the server. Signed-off-by: Martin Hickey <martin.hickey@ie.ibm.com>
Feedback from kubernetes-sigs#599 (review) Signed-off-by: Martin Hickey <martin.hickey@ie.ibm.com>
08b102c
to
51ee1da
Compare
Feedback comment: kubernetes-sigs#599 (comment) Signed-off-by: Martin Hickey <martin.hickey@ie.ibm.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see comments/questions
Recaftor to remove defaultRuntimeEndpoint as it is no longer necessary as its is replaced to use array of default endpoints if endpoints are not set Signed-off-by: Martin Hickey <martin.hickey@ie.ibm.com>
@mikebrow I did a refactor to tidy up the logic around default eps. Let me know what you think. |
3fe53b1
to
0c5e4a6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
couple nits looking good!
0c5e4a6
to
21ef8f0
Compare
Thanks for review @mikebrow. Updated and ready for review again. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM(Non-Binding)
Used it in a couple environments, very nice indeed.
Thanks for the reviews and feedback @mikebrow |
2f83d2c
to
b37a0cb
Compare
Update following review on slack with mikebrow Signed-off-by: Martin Hickey <martin.hickey@ie.ibm.com>
b37a0cb
to
871ffe3
Compare
@feiskyer This is updated and ready for review again. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks
/lgtm
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: feiskyer, hickeyma The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
seems github's link to travis state got out of whack again.. |
yep, let me merge this manually |
Currently, the default CRI server is dockershim. This is a deprecated server with cri-tools. Also, does it make sense to even have a default as the user should explicitly define it.
This PR adds
containerd
andcri-o
to the default list and it checks each in order till one works. It also adds some logging that the defaults are being used. This at least provdes better usability to the user if they don't configure the server.Closes #597