update prune instructions to not prune minikube itself #10801
Conversation
|
[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
approved
|
/ok-to-test |
k8s-ci-robot
added
the
ok-to-test
|
Could you please provide more context? I don't yet agree with this PR.
|
@tstromberg I agree it is probably not an advice that will help everybody, but in some cases I have seen when users have hundreds of dangling images, docker ps and docker imgages is very slow (due to docker calucating hashesh) not strong opinion though, I am okay with removing the prune, wdyt ? |
@tstromberg this PR doesnt affec the Paused clusters, only adds more specific instructions for pruninng, so the user doesn't remove minikube itself in the process of prunning |
|
kvm2 Driver Times for Minikube (PR 10801): 63.7s 63.5s 65.0s Averages Time Per Log docker Driver Times for Minikube (PR 10801): 26.5s 26.6s 26.7s Averages Time Per Log |
| @@ -148,8 +148,14 @@ func runCmd(cmd *exec.Cmd, warnSlow ...bool) (*RunResult, error) { | |||
| out.WarningT(`Executing "{{.command}}" took an unusually long time: {{.duration}}`, out.V{"command": rr.Command(), "duration": elapsed}) | |||
| // Don't show any restarting hint, when running podman locally (on linux, with sudo). Only when having a service. | |||
| if cmd.Args[0] != "sudo" { | |||
There was a problem hiding this comment.
The logic to determine if this is Docker should be improved so as not to require the clarifying comment. It's pretty obscure.
There was a problem hiding this comment.
I agree this code seems weird, even for me each time I read it it feels odd, that we check for sudo.
This is a compromise that @afbjorklund suggested and implemented to avoid code complexity and since we do not allow running docker with “Sudo” , it is also most efficient way
There was a problem hiding this comment.
If the runner can't be made aware of which environment it is in, perhaps it's a good sign that it should be passing the signal upwards rather than providing advice for an environment. For instance, returning duration upwards in RunResult.
If that isn't feasible, lets simplify this check, and make it more accurate at the same time. Is there a way to check if the OCI is Docker (based on either the object, cmd.Args, or some other magic)? Ideally, this should say something like:
if isDocker() {
.. show docker advice ...
}
For instance, this function could simply check if cmd arg 0 or 1 is docker.
| @@ -148,8 +148,14 @@ func runCmd(cmd *exec.Cmd, warnSlow ...bool) (*RunResult, error) { | |||
| out.WarningT(`Executing "{{.command}}" took an unusually long time: {{.duration}}`, out.V{"command": rr.Command(), "duration": elapsed}) | |||
| // Don't show any restarting hint, when running podman locally (on linux, with sudo). Only when having a service. | |||
| if cmd.Args[0] != "sudo" { | |||
| out.ErrT(style.Tip, `Restarting the {{.name}} service may improve performance.`, out.V{"name": cmd.Args[0]}) | |||
| out.ErrT(style.Tip, `Restarting or pruning the {{.name}} service may improve performance.`, out.V{"name": cmd.Args[0]}) | |||
| out.Infof(`To prune run: $ docker system prune --filter="label!=created_by.minikube.sigs.k8s.io=true"`) | |||
There was a problem hiding this comment.
This still intuitively doesn't make much sense to me. Can you provide some additional context, such as a documentation from Docker or a benchmark? I want to make sure we aren't selling snake-oil here.
There was a problem hiding this comment.
One systems which user has hundreds of dangling images without owner, docker commands such as docker ps, do take longer.
(Which causes minikube to be slow too)
To replicate, you could do what an average user would do, go to random GitHub with READMe that says docker run.... And then forget about them. And many cases this might not help them at all, and same story for “restart” suggestion, I have noticed slow dockers that nothing helped them.
It is not really selling snake oil, It is the only suggestions that we got, and might help someone, specially if they wanna do prune on their own (such as tejal, we better at least give them the Right prune command)
I would be happy to change the wording to say, This MIGH not help u at all, might 10% chance this might be the problem. but that would make the dislclaimer text long
We already tell the suer “May improve”
And to be clear, on some slow dockers that I noticed, Restarting didn’t do anything either.
And if the user comes creates issue in github, I would tell them same suggestion, restart, prune,... there isn’t much else to do tbh
There was a problem hiding this comment.
Here's my recommendation then to take a data driven approach to this:
- Starting 10,000 abandoned containers.
- Run minikube and note the run durations mentioned in the log file (you mention
ps). minikube should complain about slowdowns. - prune
- Run minikube and note the run durations that changed since prune
- Adjust this logic to only suggest prune for the commands which would in fact be improved, such as
psas you have mentioned.
| @@ -35,6 +35,14 @@ The Docker driver allows you to install Kubernetes into an existing Docker insta | |||
|
|
|||
| [comment]: <> (this title is used in the docs links, don't change) | |||
|
|
|||
| ### Prune docker to avoid performance downgrade in minikube | |||
| the following command will prune all on unused/stopped containers, volumes on your docker. | |||
There was a problem hiding this comment.
Provide contextual documentation on why this improves performance.
Also, remove the leading whitespace and capitalize "the".
| @@ -173,8 +173,9 @@ var ( | |||
| ID: "RSRC_DOCKER_STORAGE", | |||
| ExitCode: ExInsufficientStorage, | |||
| Advice: `Try one or more of the following to free up space on the device: | |||
|
|
|||
| 1. Run "docker system prune" to remove unused Docker data (optionally with "-a") | |||
| 1. Remove unused Docker data (optionally with "-a") | |||
There was a problem hiding this comment.
Should this should say "Remove Docker daemon generated outside of minikube"?
There was a problem hiding this comment.
I am not sure if talking about outside and inside minikbue would make sense for the average end user. That might confuse them even more.
Since we give them the shell command with $ ... to copy paste.
If we wanted inside minikube we would give them this command
$ minikbue ssh docker system prune ...
There was a problem hiding this comment.
By outside of minikube, I mean images that were not created by minikube, as this command explicitly filters those out. I'm not sure of the exact best verbiage here, but the comment is no longer accurate to the command it describes.
| @@ -148,8 +148,14 @@ func runCmd(cmd *exec.Cmd, warnSlow ...bool) (*RunResult, error) { | |||
| out.WarningT(`Executing "{{.command}}" took an unusually long time: {{.duration}}`, out.V{"command": rr.Command(), "duration": elapsed}) | |||
| // Don't show any restarting hint, when running podman locally (on linux, with sudo). Only when having a service. | |||
| if cmd.Args[0] != "sudo" { | |||
There was a problem hiding this comment.
If the runner can't be made aware of which environment it is in, perhaps it's a good sign that it should be passing the signal upwards rather than providing advice for an environment. For instance, returning duration upwards in RunResult.
If that isn't feasible, lets simplify this check, and make it more accurate at the same time. Is there a way to check if the OCI is Docker (based on either the object, cmd.Args, or some other magic)? Ideally, this should say something like:
if isDocker() {
.. show docker advice ...
}
For instance, this function could simply check if cmd arg 0 or 1 is docker.
| @@ -148,8 +148,14 @@ func runCmd(cmd *exec.Cmd, warnSlow ...bool) (*RunResult, error) { | |||
| out.WarningT(`Executing "{{.command}}" took an unusually long time: {{.duration}}`, out.V{"command": rr.Command(), "duration": elapsed}) | |||
| // Don't show any restarting hint, when running podman locally (on linux, with sudo). Only when having a service. | |||
| if cmd.Args[0] != "sudo" { | |||
| out.ErrT(style.Tip, `Restarting the {{.name}} service may improve performance.`, out.V{"name": cmd.Args[0]}) | |||
| out.ErrT(style.Tip, `Restarting or pruning the {{.name}} service may improve performance.`, out.V{"name": cmd.Args[0]}) | |||
| out.Infof(`To prune run: $ docker system prune --filter="label!=created_by.minikube.sigs.k8s.io=true"`) | |||
There was a problem hiding this comment.
Here's my recommendation then to take a data driven approach to this:
- Starting 10,000 abandoned containers.
- Run minikube and note the run durations mentioned in the log file (you mention
ps). minikube should complain about slowdowns. - prune
- Run minikube and note the run durations that changed since prune
- Adjust this logic to only suggest prune for the commands which would in fact be improved, such as
psas you have mentioned.
| @@ -173,8 +173,9 @@ var ( | |||
| ID: "RSRC_DOCKER_STORAGE", | |||
| ExitCode: ExInsufficientStorage, | |||
| Advice: `Try one or more of the following to free up space on the device: | |||
|
|
|||
| 1. Run "docker system prune" to remove unused Docker data (optionally with "-a") | |||
| 1. Remove unused Docker data (optionally with "-a") | |||
There was a problem hiding this comment.
By outside of minikube, I mean images that were not created by minikube, as this command explicitly filters those out. I'm not sure of the exact best verbiage here, but the comment is no longer accurate to the command it describes.
closes #10773
Before this PR
After this PR
fix the suggestion not to prune minikube itself (if it is stopped)