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

update prune instructions to not prune minikube itself #10801

Closed
wants to merge 2 commits into from
Closed

update prune instructions to not prune minikube itself #10801

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
500e2cc
update prune instructionos to not prune minikube itself
medyagh Mar 12, 2021
9678029
wroding
medyagh Mar 12, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 7 additions & 1 deletion pkg/drivers/kic/oci/cli_runner.go
View file
Open in desktop
Expand Up @@ -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" {
Copy link
Contributor

Choose a reason for hiding this comment

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

The logic to determine if this is Docker should be improved so as not to require the clarifying comment. It's pretty obscure.

Copy link
Member Author

@medyagh medyagh Mar 23, 2021 •
edited

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

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"`)
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@medyagh medyagh Mar 23, 2021 •
edited

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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 ps as you have mentioned.

if runtime.GOOS != "linux" {
out.Infof(`To restart: Click Docker 🐳 in the system tray and then click "Restart Docker"`, out.V{"icon": style.Docker})
}

}

}
}

Expand Down
5 changes: 3 additions & 2 deletions pkg/minikube/reason/reason.go
View file
Open in desktop
Expand Up @@ -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")
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this should say "Remove Docker daemon generated outside of minikube"?

Copy link
Member Author

Choose a reason for hiding this comment

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

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 ...

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Run:
$ docker system prune --filter="label!=created_by.minikube.sigs.k8s.io=true"
2. Increase the storage allocated to Docker for Desktop by clicking on:
Docker icon > Preferences > Resources > Disk Image Size
3. Run "minikube ssh -- docker system prune" if using the Docker container runtime`,
Expand Down
10 changes: 9 additions & 1 deletion site/content/en/docs/drivers/docker.md
View file
Open in desktop
Expand Up @@ -15,7 +15,7 @@ The Docker driver allows you to install Kubernetes into an existing Docker insta

- Cross platform (linux, macOS, Windows)
- No hypervisor required when run on Linux
- Experimental support for [WSL2](https://docs.microsoft.com/en-us/windows/wsl/wsl2-install) on Windows 10
- Support for [WSL2](https://docs.microsoft.com/en-us/windows/wsl/wsl2-install) on Windows 10

## Known Issues

Expand All @@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

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

Provide contextual documentation on why this improves performance.

Also, remove the leading whitespace and capitalize "the".

```shell
docker system prune --filter="label!=created_by.minikube.sigs.k8s.io=true"
```
After pruning on MacOS or Windows consider restaring docker (Click Docker 🐳 in the system tray and then click "Restart Docker")


### Verify Docker container type is Linux

- On Windows, make sure Docker Desktop's container type setting is Linux and not windows. see docker docs on [switching container type](https://docs.docker.com/docker-for-windows/#switch-between-windows-and-linux-containers).
Expand Down