Skip to content
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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 3 commits into from
Jun 22, 2021
Merged

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

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b0b4cbb
Do not return an error from Systemd.ForceStop(svc) if svc is already …
ilya-zuyev Jun 15, 2021
5a3ad1e
Merge branch 'master' into fix_docker_stop_log
ilya-zuyev Jun 22, 2021
654a963
fix error processing
ilya-zuyev Jun 22, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion pkg/minikube/sysinit/systemd.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,9 @@ package sysinit

import (
"errors"
"fmt"
"os/exec"
"strings"

"k8s.io/minikube/pkg/minikube/assets"
)
Expand Down Expand Up @@ -123,7 +125,14 @@ func (s *Systemd) Stop(svc string) error {

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

_, 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 {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@medyagh medyagh Jun 18, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@medyagh medyagh Jun 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Contributor Author

@ilya-zuyev ilya-zuyev Jun 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@medyagh medyagh Jun 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return nil
}
if strings.Contains(rr.Output(), fmt.Sprintf("Unit %s not loaded", svc)) {
// already stopped
return nil
}
return err
}

Expand Down