Update the default CRI server by hickeyma · Pull Request #599 · kubernetes-sigs/cri-tools

hickeyma · 2020-04-30T21:13:00Z

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

hickeyma · 2020-04-30T21:15:48Z

/assign @feiskyer

mikebrow

see comment

Feedback from kubernetes-sigs#599 (review) Signed-off-by: Martin Hickey <[email protected]>

hickeyma · 2020-05-01T12:20:19Z

Thanks for the review and feedback @mikebrow. Updated and ready for review again.

mikebrow

see comment

feiskyer · 2020-05-02T13:07:15Z

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.

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 PR adds containerd and cri-o to the default list and it checks each in order till one works.

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?

mikebrow · 2020-05-02T14:50:45Z

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.

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 PR adds containerd and cri-o to the default list and it checks each in order till one works.

This would introduce a few seconds latency for containerd and crio. @hickeyma @mikebrow do you think this is ok for users?

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 ....sock in the config file.

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?

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. crictl config - Get and set crictl options

#602

feiskyer · 2020-05-04T12:49:05Z

@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 <[email protected]>

Feedback from kubernetes-sigs#599 (review) Signed-off-by: Martin Hickey <[email protected]>

Feedback comment: kubernetes-sigs#599 (comment) Signed-off-by: Martin Hickey <[email protected]>

hickeyma · 2020-05-05T13:44:15Z

Thanks for the reviews @feiskyer @mikebrow. Updated and ready for review again.

mikebrow

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 <[email protected]>

hickeyma · 2020-05-06T14:28:12Z

@mikebrow I did a refactor to tidy up the logic around default eps. Let me know what you think.

mikebrow

couple nits looking good!

hickeyma · 2020-05-06T19:14:02Z

Thanks for review @mikebrow. Updated and ready for review again.

mikebrow

LGTM(Non-Binding)
Used it in a couple environments, very nice indeed.

hickeyma · 2020-05-06T19:38:48Z

Thanks for the reviews and feedback @mikebrow

Update following review on slack with mikebrow Signed-off-by: Martin Hickey <[email protected]>

hickeyma · 2020-05-07T07:08:42Z

@feiskyer This is updated and ready for review again.

feiskyer

Thanks

/lgtm
/approve

k8s-ci-robot · 2020-05-07T09:08:44Z

[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

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [feiskyer]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

mikebrow · 2020-05-07T12:10:25Z

seems github's link to travis state got out of whack again..

feiskyer · 2020-05-07T12:25:46Z

yep, let me merge this manually

hickeyma · 2020-05-07T12:33:04Z

Thanks @feiskyer and @mikebrow for sorting this and for the merger @feiskyer

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

k8s-ci-robot requested review from Random-Liu and mrunalp April 30, 2020 21:13

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Apr 30, 2020

hickeyma mentioned this pull request Apr 30, 2020

Update the default CRI server defaults #598

Closed

k8s-ci-robot assigned feiskyer Apr 30, 2020

hickeyma mentioned this pull request Apr 30, 2020

Should there be a default for the CRI server endpoint? #597

Closed

mikebrow reviewed Apr 30, 2020

View reviewed changes

Comment thread cmd/crictl/main_unix.go Outdated

Comment thread cmd/crictl/main_windows.go Outdated

hickeyma added a commit to hickeyma/cri-tools that referenced this pull request May 1, 2020

Change defaults to be its own array

08b102c

Feedback from kubernetes-sigs#599 (review) Signed-off-by: Martin Hickey <[email protected]>

mikebrow reviewed May 1, 2020

View reviewed changes

Comment thread cmd/crictl/main.go Outdated

hickeyma added 2 commits May 5, 2020 11:09

Change defaults to be its own array

51ee1da

Feedback from kubernetes-sigs#599 (review) Signed-off-by: Martin Hickey <[email protected]>

hickeyma force-pushed the hickeyma/cri-default-server branch from 08b102c to 51ee1da Compare May 5, 2020 11:10

Add flag to check that endpoints have been set

b1429c4

Feedback comment: kubernetes-sigs#599 (comment) Signed-off-by: Martin Hickey <[email protected]>

hickeyma requested a review from feiskyer May 5, 2020 13:43

mikebrow reviewed May 5, 2020

View reviewed changes

Comment thread cmd/crictl/main_unix.go

Comment thread cmd/crictl/main.go

Remove defaultRuntimeEndpoint

0188e62

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 <[email protected]>

hickeyma force-pushed the hickeyma/cri-default-server branch from 3fe53b1 to 0c5e4a6 Compare May 6, 2020 17:26

k8s-ci-robot removed the do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. label May 6, 2020

mikebrow reviewed May 6, 2020

View reviewed changes

Comment thread docs/crictl.md Outdated

Comment thread docs/crictl.md Outdated

mikebrow reviewed May 6, 2020

View reviewed changes

Comment thread cmd/crictl/main.go Outdated

hickeyma force-pushed the hickeyma/cri-default-server branch from 0c5e4a6 to 21ef8f0 Compare May 6, 2020 19:11

mikebrow approved these changes May 6, 2020

View reviewed changes

hickeyma force-pushed the hickeyma/cri-default-server branch 2 times, most recently from 2f83d2c to b37a0cb Compare May 6, 2020 19:55

Refactor following review

871ffe3

Update following review on slack with mikebrow Signed-off-by: Martin Hickey <[email protected]>

hickeyma force-pushed the hickeyma/cri-default-server branch from b37a0cb to 871ffe3 Compare May 6, 2020 20:15

feiskyer reviewed May 7, 2020

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 7, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 7, 2020

feiskyer merged commit 0fdfd4b into kubernetes-sigs:master May 7, 2020

hickeyma deleted the hickeyma/cri-default-server branch May 7, 2020 12:32

hickeyma mentioned this pull request Aug 28, 2020

REQUEST: New membership for hickeyma kubernetes/org#2146

Closed

6 tasks

This was referenced Aug 31, 2020

crictl 1.19.0 fishworks/fish-food#918

Merged

critest 1.19.0 fishworks/fish-food#919

Merged

SergeyKanzhelev mentioned this pull request Jan 13, 2022

Remove dockershim from crictl #871

Closed

afbjorklund mentioned this pull request Jan 13, 2022

[1.24] Remove dockershim from crictl code and docs #870

Closed

Conversation

hickeyma commented Apr 30, 2020

Uh oh!

hickeyma commented Apr 30, 2020

Uh oh!

mikebrow left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

hickeyma commented May 1, 2020

Uh oh!

mikebrow left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

feiskyer commented May 2, 2020

Uh oh!

mikebrow commented May 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

feiskyer commented May 4, 2020

Uh oh!

hickeyma commented May 5, 2020

Uh oh!

mikebrow left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

hickeyma commented May 6, 2020

Uh oh!

mikebrow left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hickeyma commented May 6, 2020

Uh oh!

mikebrow left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hickeyma commented May 6, 2020

Uh oh!

hickeyma commented May 7, 2020

Uh oh!

feiskyer left a comment

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented May 7, 2020

Uh oh!

mikebrow commented May 7, 2020

Uh oh!

feiskyer commented May 7, 2020

Uh oh!

hickeyma commented May 7, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mikebrow commented May 2, 2020 •

edited

Loading

mikebrow left a comment •

edited

Loading