Do not return an error from Systemd.ForceStop(svc) if svc is already stopped #11667

ilya-zuyev · 2021-06-15T22:02:20Z

…stoppedf

ilya-zuyev · 2021-06-15T22:02:40Z

/ok-to-test

sharifelgamal · 2021-06-15T23:47:56Z

pkg/minikube/sysinit/systemd.go

@@ -123,7 +125,14 @@ func (s *Systemd) Stop(svc string) error {

 // ForceStop terminates a service with prejudice
 func (s *Systemd) ForceStop(svc string) error {
-	_, err := s.r.RunCmd(exec.Command("sudo", "systemctl", "stop", "-f", svc))
+	rr, err := s.r.RunCmd(exec.Command("sudo", "systemctl", "stop", "-f", svc))
+	if err == nil {


this if statement isn't really necessary, return err will return nil if err is nil

agree with @sharifelgamal this "if" is not needed !

This if lets us to avoid matching substrings in strings.Contains. Also the whole logic is clearer with this shortcut - if systemctl returned ok - return immediately as no extra processing required

this "if statement" adds to the code complexity in terms of number of If statments. and makes the code harder to read without gaining anything meaningful in terms of cpu cycles.

this is not inline with go style used in minikube, I suggest getting rid of it

just curious, how it makes the code harder to read?

@ilya-zuyev
because following the code we dont expect to check for err nil and return while it already returns,
a typical go reader with following the styles is used to look at the return and it is obvisous if it is nil it returns nil too.
while this might be easier for the one writing this code to explicitly add the If statement here, for the go reviewer is not common to see it

"Gocyclo calculates cyclomatic complexities of functions in Go source code. The cyclomatic complexity of a function is calculated according to the following rules: 1 is the base complexity of a function +1 for each 'if', 'for', 'case', '&&' or '||' Go Report Card warns on functions with cyclomatic complexity > 1"

plus it adds to the go "gocyclo" score https://goreportcard.com/report/github.com/kubernetes/minikube
so the less If statments, the better

because following the code we dont expect to check for err nil and return while it already returns, a typical go reader with following the styles is used to look at the return and it is obvisous if it is nil it returns nil too. I don't understand this sentence

Cyclomatic complexity metric evaluates the number of branches inside the function, its goal is to prevent functions overgrow. This method is 7 lines long, talking about cyclomatic complexity makes no sense here.

this is not inline with minikube code style, it doesn't make sense to check for "nil" and return while we are returning in next line anyways, this an example of code seems easier to "write" but overtime will make it harder to "maintain"

I suggest reading more minikube code or https://www.kubernetes.dev/docs/guide/coding-convention/

I don't personally think this is more complex, it's just an awkward line of code for a small optimization.

minikube-pr-bot · 2021-06-16T00:09:21Z

kvm2 driver with docker runtime

+----------------+----------+---------------------+
|    COMMAND     | MINIKUBE | MINIKUBE (PR 11667) |
+----------------+----------+---------------------+
| minikube start | 47.4s    | 47.4s               |
| enable ingress | 34.5s    | 35.6s               |
+----------------+----------+---------------------+

Times for minikube (PR 11667) start: 47.3s 47.2s 46.3s 46.1s 50.1s
Times for minikube start: 47.8s 47.0s 46.4s 47.1s 48.6s

Times for minikube ingress: 36.3s 34.2s 34.7s 33.8s 33.6s
Times for minikube (PR 11667) ingress: 36.2s 34.7s 35.7s 37.2s 34.2s

docker driver with docker runtime

+----------------+----------+---------------------+
|    COMMAND     | MINIKUBE | MINIKUBE (PR 11667) |
+----------------+----------+---------------------+
| minikube start | 21.7s    | 21.7s               |
| enable ingress | 28.9s    | 29.2s               |
+----------------+----------+---------------------+

Times for minikube start: 23.4s 20.6s 21.5s 21.5s 21.4s
Times for minikube (PR 11667) start: 21.3s 22.4s 21.5s 21.0s 22.2s

Times for minikube ingress: 27.0s 32.0s 28.0s 29.0s 28.5s
Times for minikube (PR 11667) ingress: 29.0s 30.5s 30.5s 28.0s 28.0s

docker driver with containerd runtime

+----------------+----------+---------------------+
|    COMMAND     | MINIKUBE | MINIKUBE (PR 11667) |
+----------------+----------+---------------------+
| minikube start | 43.8s    | 45.6s               |
| enable ingress |          |                     |
+----------------+----------+---------------------+

Times for minikube start: 32.0s 48.9s 47.2s 43.2s 47.4s
Times for minikube (PR 11667) start: 46.8s 42.5s 47.6s 43.5s 47.7s

medyagh · 2021-06-16T22:01:07Z

pkg/minikube/sysinit/systemd.go

@@ -123,7 +125,14 @@ func (s *Systemd) Stop(svc string) error {

 // ForceStop terminates a service with prejudice
 func (s *Systemd) ForceStop(svc string) error {
-	_, err := s.r.RunCmd(exec.Command("sudo", "systemctl", "stop", "-f", svc))
+	rr, err := s.r.RunCmd(exec.Command("sudo", "systemctl", "stop", "-f", svc))
+	if err == nil {


agree with @sharifelgamal this "if" is not needed !

medyagh · 2021-06-16T22:02:00Z

pkg/minikube/sysinit/systemd.go

@@ -123,7 +125,14 @@ func (s *Systemd) Stop(svc string) error {

 // ForceStop terminates a service with prejudice
 func (s *Systemd) ForceStop(svc string) error {


consider adding this to openRC too
https://github.com/medyagh/minikube/blob/1bac30bbe7caa5b8c64b35cfb97c197ffe08840a/pkg/minikube/sysinit/openrc.go#L166

Not sure what to do there. This PR is targeting a very specific systemd issue

medyagh · 2021-06-18T15:46:34Z

pkg/minikube/sysinit/systemd.go

@@ -123,7 +125,14 @@ func (s *Systemd) Stop(svc string) error {

 // ForceStop terminates a service with prejudice
 func (s *Systemd) ForceStop(svc string) error {
-	_, err := s.r.RunCmd(exec.Command("sudo", "systemctl", "stop", "-f", svc))
+	rr, err := s.r.RunCmd(exec.Command("sudo", "systemctl", "stop", "-f", svc))
+	if err == nil {


this "if statement" adds to the code complexity in terms of number of If statments. and makes the code harder to read without gaining anything meaningful in terms of cpu cycles.

this is not inline with go style used in minikube, I suggest getting rid of it

k8s-ci-robot · 2021-06-21T21:30:10Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ilya-zuyev, 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:

~~OWNERS~~ [ilya-zuyev,medyagh]

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

medyagh · 2021-06-21T21:30:49Z

plz pull upstream and then it is good to go

minikube-pr-bot · 2021-06-22T02:18:47Z

kvm2 driver with docker runtime

+----------------+----------+---------------------+
|    COMMAND     | MINIKUBE | MINIKUBE (PR 11667) |
+----------------+----------+---------------------+
| minikube start | 47.6s    | 47.1s               |
| enable ingress | 34.4s    | 34.4s               |
+----------------+----------+---------------------+

Times for minikube start: 48.6s 45.8s 45.8s 46.4s 51.6s
Times for minikube (PR 11667) start: 47.1s 46.9s 47.2s 47.1s 47.4s

Times for minikube ingress: 32.8s 35.2s 33.2s 35.7s 35.2s
Times for minikube (PR 11667) ingress: 35.2s 35.2s 34.2s 34.2s 33.2s

docker driver with docker runtime

+----------------+----------+---------------------+
|    COMMAND     | MINIKUBE | MINIKUBE (PR 11667) |
+----------------+----------+---------------------+
| minikube start | 21.7s    | 21.2s               |
| enable ingress | 31.1s    | 31.9s               |
+----------------+----------+---------------------+

Times for minikube start: 22.9s 21.0s 21.4s 22.0s 21.2s
Times for minikube (PR 11667) start: 20.6s 21.1s 21.5s 21.2s 21.4s

Times for minikube ingress: 28.5s 31.0s 28.5s 37.0s 30.5s
Times for minikube (PR 11667) ingress: 29.0s 30.5s 32.5s 31.0s 36.5s

docker driver with containerd runtime

+----------------+----------+---------------------+
|    COMMAND     | MINIKUBE | MINIKUBE (PR 11667) |
+----------------+----------+---------------------+
| minikube start | 41.6s    | 44.7s               |
| enable ingress |          |                     |
+----------------+----------+---------------------+

Times for minikube start: 31.8s 43.5s 46.7s 43.0s 42.8s
Times for minikube (PR 11667) start: 46.6s 42.8s 43.2s 47.1s 43.9s

minikube-bot · 2021-06-22T02:40:58Z

These are the flake rates of all failed tests on Docker_Linux_containerd.

Failed Tests	Flake Rate (%)
TestPause/serial/Pause	14.29 (chart)
TestPause/serial/VerifyStatus	14.29 (chart)
TestPause/serial/PauseAgain	19.48 (chart)

minikube-bot · 2021-06-22T02:55:05Z

These are the flake rates of all failed tests on KVM_Linux_containerd.

Failed Tests	Flake Rate (%)
TestErrorSpam/setup	100.00 (chart)

medyagh · 2021-06-22T04:03:36Z

@ilya-zuyev
this PR doesnt seem to fix the failed test
https://storage.googleapis.com/minikube-builds/logs/11667/5a3ad1e/KVM_Linux_containerd.html#fail_TestErrorSpam%2fsetup

error_spam_test.go:108: minikube stderr:
E0622 01:56:15.187503    9483 docker.go:159] "Failed to stop" err="sudo systemctl stop -f docker.service: Process exited with status 5\nstdout:\n\nstderr:\nFailed to stop docker.service: Unit docker.service not loaded.\n" service="docker.service"

minikube-pr-bot · 2021-06-22T07:37:51Z

kvm2 driver with docker runtime

+----------------+----------+---------------------+
|    COMMAND     | MINIKUBE | MINIKUBE (PR 11667) |
+----------------+----------+---------------------+
| minikube start | 52.4s    | 49.7s               |
| enable ingress | 35.2s    | 36.0s               |
+----------------+----------+---------------------+

Times for minikube ingress: 35.8s 35.7s 33.8s 34.7s 35.8s
Times for minikube (PR 11667) ingress: 40.4s 35.7s 35.2s 34.2s 34.7s

Times for minikube start: 56.0s 51.8s 51.5s 51.9s 51.0s
Times for minikube (PR 11667) start: 51.5s 49.0s 47.2s 51.6s 49.0s

docker driver with docker runtime

+----------------+----------+---------------------+
|    COMMAND     | MINIKUBE | MINIKUBE (PR 11667) |
+----------------+----------+---------------------+
| minikube start | 22.1s    | 22.1s               |
| enable ingress | 29.2s    | 31.0s               |
+----------------+----------+---------------------+

Times for minikube (PR 11667) start: 21.7s 21.3s 22.8s 22.7s 22.2s
Times for minikube start: 22.3s 22.7s 21.7s 22.4s 21.4s

Times for minikube ingress: 30.5s 29.0s 29.0s 28.6s 29.1s
Times for minikube (PR 11667) ingress: 29.5s 33.5s 32.5s 31.0s 28.5s

docker driver with containerd runtime

+----------------+----------+---------------------+
|    COMMAND     | MINIKUBE | MINIKUBE (PR 11667) |
+----------------+----------+---------------------+
| minikube start | 45.3s    | 45.1s               |
| enable ingress |          |                     |
+----------------+----------+---------------------+

Times for minikube start: 47.7s 44.4s 47.5s 43.5s 43.4s
Times for minikube (PR 11667) start: 47.2s 43.2s 47.7s 43.4s 44.1s

Do not return an error from Systemd.ForceStop(svc) if svc is already …

b0b4cbb

…stoppedf

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 15, 2021

k8s-ci-robot requested review from RA489 and tstromberg June 15, 2021 22:02

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jun 15, 2021

k8s-ci-robot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Jun 15, 2021

sharifelgamal reviewed Jun 15, 2021

View reviewed changes

medyagh requested changes Jun 16, 2021

View reviewed changes

medyagh requested changes Jun 18, 2021

View reviewed changes

medyagh approved these changes Jun 21, 2021

View reviewed changes

Merge branch 'master' into fix_docker_stop_log

5a3ad1e

medyagh self-requested a review June 22, 2021 04:03

fix error processing

654a963

ilya-zuyev merged commit d4fe773 into kubernetes:master Jun 22, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not return an error from Systemd.ForceStop(svc) if svc is already stopped #11667

Do not return an error from Systemd.ForceStop(svc) if svc is already stopped #11667

ilya-zuyev commented Jun 15, 2021

ilya-zuyev commented Jun 15, 2021

sharifelgamal Jun 15, 2021

medyagh Jun 16, 2021

ilya-zuyev Jun 17, 2021

medyagh Jun 18, 2021 •

edited

Loading

ilya-zuyev Jun 18, 2021

medyagh Jun 21, 2021 •

edited

Loading

ilya-zuyev Jun 21, 2021 •

edited

Loading

medyagh Jun 21, 2021 •

edited

Loading

sharifelgamal Jun 21, 2021

minikube-pr-bot commented Jun 16, 2021

medyagh Jun 16, 2021

medyagh Jun 16, 2021

ilya-zuyev Jun 17, 2021

medyagh Jun 18, 2021 •

edited

Loading

k8s-ci-robot commented Jun 21, 2021

medyagh commented Jun 21, 2021

minikube-pr-bot commented Jun 22, 2021

minikube-bot commented Jun 22, 2021

minikube-bot commented Jun 22, 2021

medyagh commented Jun 22, 2021

minikube-pr-bot commented Jun 22, 2021

		@@ -123,7 +125,14 @@ func (s *Systemd) Stop(svc string) error {

		// ForceStop terminates a service with prejudice
		func (s *Systemd) ForceStop(svc string) error {

Do not return an error from Systemd.ForceStop(svc) if svc is already stopped #11667

Do not return an error from Systemd.ForceStop(svc) if svc is already stopped #11667

Conversation

ilya-zuyev commented Jun 15, 2021

ilya-zuyev commented Jun 15, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

medyagh Jun 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

medyagh Jun 21, 2021 • edited Loading

Choose a reason for hiding this comment

ilya-zuyev Jun 21, 2021 • edited Loading

Choose a reason for hiding this comment

medyagh Jun 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

minikube-pr-bot commented Jun 16, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

medyagh Jun 18, 2021 • edited Loading

Choose a reason for hiding this comment

k8s-ci-robot commented Jun 21, 2021

medyagh commented Jun 21, 2021

minikube-pr-bot commented Jun 22, 2021

minikube-bot commented Jun 22, 2021

minikube-bot commented Jun 22, 2021

medyagh commented Jun 22, 2021

minikube-pr-bot commented Jun 22, 2021

medyagh Jun 18, 2021 •

edited

Loading

medyagh Jun 21, 2021 •

edited

Loading

ilya-zuyev Jun 21, 2021 •

edited

Loading

medyagh Jun 21, 2021 •

edited

Loading

medyagh Jun 18, 2021 •

edited

Loading