wip: Fix stop containers for containerd. #7545
Conversation
k8s-ci-robot
added
cncf-cla: yes
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: medyagh 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 |
k8s-ci-robot
added
the
approved
medyagh
changed the title
medyagh
changed the title
|
/ok-to-test |
k8s-ci-robot
added
the
ok-to-test
|
Error: running mkcmp |
Codecov Report
@@ Coverage Diff @@
## master #7545 +/- ##
=======================================
Coverage 36.69% 36.69%
=======================================
Files 146 146
Lines 8988 8988
=======================================
Hits 3298 3298
Misses 5297 5297
Partials 393 393
|
|
Times for minikube: [69.642782215 66.478358537 65.91416543] Times for Minikube (PR 7545): [68.559998493 66.82173064599999 66.26329182399999] Averages Time Per Log |
|
Times for minikube: [69.018850534 68.66266183 66.928241411] Times for Minikube (PR 7545): [68.642177001 68.525873759 66.414367633] Averages Time Per Log |
|
Times for minikube: [65.634584023 66.59397426899999 68.001445409] Times for Minikube (PR 7545): [65.555876831 65.131053814 66.36321817000001] Averages Time Per Log |
|
@medyagh: PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
k8s-ci-robot
added
the
needs-rebase
| @@ -316,33 +316,40 @@ func (d *Driver) Start() error { | |||
| func (d *Driver) Stop() error { | |||
| // on init this doesn't get filled when called from cmd | |||
| d.exec = command.NewKICRunner(d.MachineName, d.OCIBinary) | |||
| // docker does not send right SIG for systemd to know to stop the systemd. | |||
| // to avoid bind address be taken on an upgrade. more info https://github.com/kubernetes/minikube/issues/7171 | |||
| if err := kubelet.Stop(d.exec); err != nil { | |||
There was a problem hiding this comment.
Why did this move? I think the kubelet should stop first - otherwise it may restart containers on you.
| } | ||
| } | ||
| // to avoid https://github.com/kubernetes/minikube/issues/7521 | ||
| if _, err := d.exec.RunCmd(exec.Command("sudo", "/bin/bash", "-c", "kill -9 $(pgrep kube-apiserver)")); err != nil { |
There was a problem hiding this comment.
Shouldn't stopping the container take care of this? Do we have proof that this helps?
| @@ -195,7 +195,7 @@ func stopCRIContainers(cr CommandRunner, ids []string) error { | |||
| glog.Infof("Stopping containers: %s", ids) | |||
|
|
|||
| crictl := getCrictlPath(cr) | |||
| args := append([]string{crictl, "rm"}, ids...) | |||
| args := append([]string{crictl, "stop", "--timeout=2"}, ids...) | |||
There was a problem hiding this comment.
default time out is 10seconds, I thought it would better if we tweak it so we dont wait long ... but maybe not.
|
Times for minikube: [66.508048593 69.124683805 67.763322201] Times for Minikube (PR 7545): [66.539862787 64.451360596 67.12617210500001] Averages Time Per Log |
|
Times for minikube: [70.46217426 69.362016633 71.168487795] Times for Minikube (PR 7545): [65.5233439 66.94999124300001 69.086857996] Averages Time Per Log |
|
Times for minikube: [65.159318801 69.44426764 67.36778160299998] Times for Minikube (PR 7545): [64.854058299 65.75686646299998 66.536224421] Averages Time Per Log |
|
Times for minikube: [66.535640968 66.615333903 65.26584139799999] Times for Minikube (PR 7545): [65.60682616199998 67.84454655799999 71.294241319] Averages Time Per Log |
|
This PR have a very positive impact. Looking forward to it! PS - I had no idea that the Docker driver used containerd inside. |
medyagh
changed the title
k8s-ci-robot
added
the
do-not-merge/work-in-progress
hopefully closes #7521
which after miniube was stopped for containerd, on the start you would sometimes see this in the logs:
the problem was in kic we were trying to stop containers before kic stop to avoid
bind address problem
but the command stop container was wrong in the lib, it was rm
and even if the lib writer meant remove, u can not remove a not stopped container. so I changed to stop, we might need to do both stop and rm but I dont think rm is needed.
this doesnt work in crictl:
but this works: