kubeadm: reset: use crictl to reset containers #54721

runcom · 2017-10-27T21:36:41Z

Signed-off-by: Antonio Murdaca [email protected]

What this PR does / why we need it:

This patch makes kubeadm to try and reset containers using crictl first instead of docker. The reason is that kubeadm reset is ineffective with new container runtimes using the CRI (like CRI-O).
This patch uses crictl first and falls back to docker in case crictl isn't available.

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

Fix kubernetes/kubeadm#508

Special notes for your reviewer:

Release note:

kubeadm: reset: use crictl to reset containers

luxas

Generally LGTM, some nits/comments/questions

luxas · 2017-10-28T13:21:05Z

cmd/kubeadm/app/cmd/reset.go

-		fmt.Println("[reset] Removing kubernetes-managed containers.")
-		if err := exec.Command("sh", "-c", "docker ps -a --filter name=k8s_ -q | xargs -r docker rm --force --volumes").Run(); err != nil {
-			fmt.Println("[reset] Failed to stop the running containers.")
+	resetWithDocker := func() {


write this as a normal function top-level

luxas · 2017-10-28T13:21:47Z

cmd/kubeadm/app/cmd/reset.go

+				resetWithDocker()
+			}
+		} else {
+			fmt.Println("[reest] CRI socket path not provided for crictl. Trying docker instead.")


typo: reset

luxas · 2017-10-28T13:22:43Z

cmd/kubeadm/app/cmd/reset.go

+	crictlPath, err := exec.LookPath("crictl")
+	if err == nil {
+		if r.criSocketPath != "" {
+			if err := exec.Command("sh", "-c", crictlPath+" -r "+r.criSocketPath+" sandboxes --quiet | xargs -r "+crictlPath+" -r "+r.criSocketPath+" rms"); err != nil {


Logging here that "resetting foo env with tool bar and socket baz" would be helpful

luxas · 2017-10-28T13:23:30Z

cmd/kubeadm/app/cmd/reset.go

 		}
 	} else {
-		fmt.Println("[reset] Docker doesn't seem to be running. Skipping the removal of running Kubernetes containers.")
+		fmt.Println("[reset] crictl isn't available on your system. Trying using docker instead.")


not sure we need to log this, seems like an error to users, while everything's fine.

Yeah, I'm fine dropping this one.

luxas · 2017-10-28T13:24:03Z

cmd/kubeadm/app/cmd/reset.go

@@ -56,16 +57,22 @@ func NewCmdReset(out io.Writer) *cobra.Command {
 		"The path to the directory where the certificates are stored. If specified, clean this directory.",
 	)

+	cmd.PersistentFlags().StringVar(
+		&criSocketPath, "cri-socket", "",
+		"The path to the CRI socket to use with crictl when cleaning up containers.",


should we default to the dockershim socket?

runcom · 2017-10-28T13:48:28Z

@luxas addressed all your comments

luxas

Found two more things 😄

luxas · 2017-10-28T13:54:26Z

cmd/kubeadm/app/cmd/reset.go

+	if err == nil {
+		if r.criSocketPath != "" {
+			fmt.Println("[reset] Cleaning up running containers using crictl with socket %s", r.criSocketPath)
+			if err := exec.Command("sh", "-c", crictlPath+" -r "+r.criSocketPath+" sandboxes --quiet | xargs -r "+crictlPath+" -r "+r.criSocketPath+" rms"); err != nil {


for readability, can you please create a separate string for this and use fmt.Sprintf?

luxas · 2017-10-28T13:56:54Z

cmd/kubeadm/app/cmd/reset.go

+func resetWithDocker() {
+	dockerCheck := preflight.ServiceCheck{Service: "docker", CheckIfActive: true}
+	if _, errors := dockerCheck.Check(); len(errors) == 0 {
+		if err := exec.Command("sh", "-c", "docker ps -a --filter name=k8s_ -q | xargs -r docker rm --force --volumes").Run(); err != nil {


I know this wasn't here before sigh, but now I really think we should have unit tests for this, as the complexity grows.
Can you please use k8s.io/utils/exec that you inject to the functions here, and then fake it out and test the different cases out in unit tests?

yeah, I'll add a unit test for this function but it needs to mock out dockerCheck as well

runcom · 2017-10-29T16:28:13Z

@luxas reworked and added unit tests. PTAL

runcom · 2017-10-29T16:47:01Z

/retest

runcom · 2017-10-29T16:47:24Z

Looks like tests are failing cause they're using old code (???)

luxas · 2017-11-01T18:51:53Z

/retest

luxas

/approve

Bazel is failing with, I don't know why:

W1101 19:03:02.819] 2017/11/01 19:03:02 missing strict dependencies:
W1101 19:03:02.819]     cmd/kubeadm/app/cmd/reset.go: import of k8s.io/utils/exec, which is not a direct dependency

cc @sttts @ixdy

luxas · 2017-11-01T18:52:13Z

cmd/kubeadm/app/cmd/reset_test.go

@@ -181,3 +185,131 @@ func TestConfigDirCleaner(t *testing.T) {
 		}
 	}
 }
+
+// TODO(justinsb): Add test for nodePort conflict detection, once we have nodePort wired in


Mmm i don't know where that comes from, I'll fix remove it.

luxas · 2017-11-01T19:54:14Z

@sttts mentioned that bazel-contrib/rules_go#831 might be related/relevant for bazel failure

ixdy · 2017-11-01T20:29:28Z

it looks like you need to run hack/update-bazel.sh?

runcom · 2017-11-02T08:35:00Z

it looks like you need to run hack/update-bazel.sh?

done

runcom · 2017-11-02T14:10:28Z

/test pull-kubernetes-verify

Signed-off-by: Antonio Murdaca <[email protected]>

runcom · 2017-11-02T16:21:16Z

/test pull-kubernetes-verify

runcom · 2017-11-03T08:45:20Z

@luxas everything passed now

luxas

/lgtm

Thank you for this contribution!

k8s-github-robot · 2017-11-04T20:40:35Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: luxas, runcom

Associated issue: 508

The full list of commands accepted by this bot can be found here.

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

~~cmd/kubeadm/OWNERS~~ [luxas]

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

k8s-github-robot · 2017-11-04T21:04:47Z

/test all [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2017-11-04T21:49:51Z

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Oct 27, 2017

k8s-github-robot assigned errordeveloper and timothysc Oct 27, 2017

luxas reviewed Oct 28, 2017

View reviewed changes

luxas assigned luxas and unassigned errordeveloper Oct 28, 2017

runcom force-pushed the reset-remove-dockerism branch from 1ec2ea1 to abcafd6 Compare October 28, 2017 13:48

luxas reviewed Oct 28, 2017

View reviewed changes

runcom force-pushed the reset-remove-dockerism branch 2 times, most recently from e1e4cb1 to b60bbbc Compare October 29, 2017 16:27

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 29, 2017

luxas approved these changes Nov 1, 2017

View reviewed changes

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 1, 2017

runcom force-pushed the reset-remove-dockerism branch from b60bbbc to 32b129c Compare November 2, 2017 08:34

kubeadm: reset: use crictl to reset containers

bb0cd27

Signed-off-by: Antonio Murdaca <[email protected]>

runcom force-pushed the reset-remove-dockerism branch from 32b129c to bb0cd27 Compare November 2, 2017 14:59

luxas approved these changes Nov 4, 2017

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 4, 2017

k8s-github-robot merged commit 5691a8d into kubernetes:master Nov 4, 2017

runcom deleted the reset-remove-dockerism branch November 4, 2017 21:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kubeadm: reset: use crictl to reset containers #54721

kubeadm: reset: use crictl to reset containers #54721

runcom commented Oct 27, 2017

luxas left a comment

luxas Oct 28, 2017

luxas Oct 28, 2017

luxas Oct 28, 2017

luxas Oct 28, 2017

runcom Oct 28, 2017

luxas Oct 28, 2017

runcom commented Oct 28, 2017

luxas left a comment

luxas Oct 28, 2017

luxas Oct 28, 2017

runcom Oct 28, 2017

runcom commented Oct 29, 2017

runcom commented Oct 29, 2017

runcom commented Oct 29, 2017

luxas commented Nov 1, 2017

luxas left a comment

luxas Nov 1, 2017

runcom Nov 1, 2017

runcom Nov 2, 2017

luxas commented Nov 1, 2017

ixdy commented Nov 1, 2017

runcom commented Nov 2, 2017

runcom commented Nov 2, 2017

runcom commented Nov 2, 2017

runcom commented Nov 3, 2017

luxas left a comment

k8s-github-robot commented Nov 4, 2017

k8s-github-robot commented Nov 4, 2017

k8s-github-robot commented Nov 4, 2017

kubeadm: reset: use crictl to reset containers #54721

kubeadm: reset: use crictl to reset containers #54721

Conversation

runcom commented Oct 27, 2017

luxas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

runcom commented Oct 28, 2017

luxas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

runcom commented Oct 29, 2017

runcom commented Oct 29, 2017

runcom commented Oct 29, 2017

luxas commented Nov 1, 2017

luxas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luxas commented Nov 1, 2017

ixdy commented Nov 1, 2017

runcom commented Nov 2, 2017

runcom commented Nov 2, 2017

runcom commented Nov 2, 2017

runcom commented Nov 3, 2017

luxas left a comment

Choose a reason for hiding this comment

k8s-github-robot commented Nov 4, 2017

k8s-github-robot commented Nov 4, 2017

k8s-github-robot commented Nov 4, 2017