docker-env: restart dockerd inside minikube on failure #8239
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
k8s-ci-robot
added
the
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
size/S
k8s-ci-robot
added
the
approved
Codecov Report
@@ Coverage Diff @@
## master #8239 +/- ##
==========================================
- Coverage 34.48% 34.38% -0.10%
==========================================
Files 147 147
Lines 9418 9448 +30
==========================================
+ Hits 3248 3249 +1
- Misses 5771 5800 +29
Partials 399 399
|
medyagh
changed the title
sharifelgamal
approved these changes
May 21, 2020
tstromberg
suggested changes
May 21, 2020
cmd/minikube/cmd/docker-env.go
Outdated
| @@ -138,6 +139,12 @@ var dockerEnvCmd = &cobra.Command{ | |||
| out.V{"runtime": co.Config.KubernetesConfig.ContainerRuntime}) | |||
| } | |||
|
|
|||
| // on minikube stop, or computer restart the IP might change. | |||
| // reloads the certs to prevent #8185 | |||
| if err := sysinit.New(co.CP.Runner).Restart("docker"); err != nil { | |||
There was a problem hiding this comment.
I don't think this is the right place to restart Docker, mostly because it is very surprising behavior. If you really must restart within docker-env calls, please update the help documentation accordingly.
My preference would be to see this restart done elsewhere, such as fix.go or provision.go:
provisionalready restarts Docker if the systemd configuration has changed.- folks may be trying to access the docker daemon outside of the
docker-envcommand
One thought: if you include the IP address in the Docker config template, it will magically restart if the IP or Docker configuration changes. We could also do this more explicitly, of course.
There was a problem hiding this comment.
I refactored this PR since this review, please take another look
There was a problem hiding this comment.
As noted in #8235, it is getting very hard to debug all these auto-restart functions
k8s-ci-robot
added
size/M
medyagh
changed the title
Travis tests have failedHey @medyagh, 1st Buildmake test2nd Buildmake test3rd BuildmakeTravisBuddy Request Identifier: c8e33bb0-9bc1-11ea-9438-0b74518d636c |
|
I agree that this behaviour is surprising. I was OK with starting dockerd if running I don't think docker-machine has this issue, because the daemon is normally started with the VM and the certificates are regenerated by the provisioner when it starts up. It only warns about the address (minikube ip) being changed: Then there is I think the main reason we need this in minikube is because of 5cd7660 That's were we hard-coded the Provisioner and removed ConfigureAuth: - glog.Infof("Detecting provisioner ...")
- provisioner, err := provision.DetectProvisioner(h.Driver)
- if err != nil {
- return h, errors.Wrap(err, "detecting provisioner")- glog.Infof("Configuring auth for driver %s ...", h.Driver.DriverName())
- if err := h.ConfigureAuth(); err != nil {
- return h, &retry.RetriableError{Err: errors.Wrap(err, "Error configuring auth on host")}
- }This is also causing problems with some of my other later experiments, |
so the problem is, when you reconfigureAuth (that was refactored out of the code by thomas) it takes 5-6 times on each start (un-needed tax on minikube start) that is like we test the docker-env before returning and then restart if there is something wrong with docker. another thing that worth noting here is, docker does not need new Certs, it already does have Certs but it has loaded the wrong one ! ( I think this is because we Enable the docker-service in the kic base image for a faster start, so on first second it picks up the Old Certs and would hold on to them till we restart docker) |
Unless I am missing something, the IP only changes if |
tstromberg
suggested changes
May 26, 2020
cmd/minikube/cmd/podman-env.go
Outdated
| // on minikube stop, or computer restart the IP might change. | ||
| // reloads the certs to prevent #8185 | ||
| glog.Infof("will try to restart dockerd service...") | ||
| restartOrExitDaemon("docker", cname, co.CP.Runner) |
There was a problem hiding this comment.
I think this is a cut and paste error: there is no docker daemon to restart within podman.
cmd/minikube/cmd/podman-env.go
Outdated
| @@ -117,7 +118,8 @@ var podmanEnvCmd = &cobra.Command{ | |||
| } | |||
|
|
|||
| if ok := isPodmanAvailable(co.CP.Runner); !ok { | |||
| exit.WithCodeT(exit.Unavailable, `The podman service within '{{.cluster}}' is not active`, out.V{"cluster": cname}) | |||
| glog.Warningf("podman service inside minikube is not active will try to restart it...") | |||
| restartOrExitDaemon("podman", cname, co.CP.Runner) | |||
There was a problem hiding this comment.
There is no podman daemon
cmd/minikube/cmd/docker-env.go
Outdated
| @@ -119,6 +122,12 @@ func isDockerActive(r command.Runner) bool { | |||
| return sysinit.New(r).Active("docker") | |||
| } | |||
|
|
|||
| func restartOrExitDaemon(bin string, name string, runner command.Runner) { | |||
There was a problem hiding this comment.
nit: For idiomatic go, functions which may cause an exit should be named something like mustRestartDocker.
|
I like that this PR documents the problem that is trying to be solved, but what is missing is a mention of why this is the best way to fix it. |
medyagh
changed the title
medyagh
changed the title
medyagh
changed the title
Travis tests have failedHey @medyagh, 1st Buildmake testTravisBuddy Request Identifier: d7921600-9f8a-11ea-a34f-dd9ab62511aa |
tstromberg
suggested changes
May 26, 2020
cmd/minikube/cmd/docker-env.go
Outdated
| mustRestartDocker(cname, co.CP.Runner) | ||
| // TODO #8241: use kverify to wait for apisefver instead | ||
| // waiting for the basics like api-server to come up | ||
| time.Sleep(time.Second * 3) |
There was a problem hiding this comment.
Remove this sleep until it can be proved that this is helpful.
There was a problem hiding this comment.
done, but I am sure ppl will run into this, specially people who use this as part of another tool chain unvisible to the user
|
/ok-to-test |
k8s-ci-robot
added
the
ok-to-test
medyagh
force-pushed
the
verify_docker_env
branch
from
0ce9881 to
6ff8c9a
Compare
|
kvm2 Driver Times for Minikube (PR 8239): [64.05310125000001 65.99178446799999 67.524245977] Averages Time Per Log docker Driver Times for Minikube (PR 8239): [27.127987598999997 26.390889912 25.901410171000002] Averages Time Per Log |
Travis tests have failedHey @medyagh, 1st Buildmake testTravisBuddy Request Identifier: 7d274970-9f9d-11ea-a34f-dd9ab62511aa |
tstromberg
approved these changes
May 26, 2020
tstromberg
changed the title
tstromberg
changed the title
tstromberg
approved these changes
May 26, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Before this PR when running docker-env, if docker service is not running we exit with error,
for example if minikube IP changes, the docker inside minikube needs to be restart to pick up the certs that have the new IP added to them.
closes #8185
This PR is meant for the last effort, when user reaches out to docker-env, the root cause can be fixed by a different PR. that will be for another release.
Before this PR: docker env when IP changed after minikube start
After this PR: docker env when IP changed after minikube start