none driver: look for cri-dockerd instead of hardcoding #15784

afbjorklund · 2023-02-05T10:50:34Z

Some people install the daemon in /usr/local/bin,
rather than under /usr/bin as the unit expects.

neersighted · 2023-02-05T11:02:54Z

pkg/minikube/cruntime/docker.go

@@ -688,6 +688,14 @@ const (
 	CNICacheDir = "/var/lib/cni/cache"
 )

+func getCriDockerdPath(cr CommandRunner) string {
+        rr, err := cr.RunCmd(exec.Command("which", "cri-dockerd"))


which is not a standard command, and parsing its output is dangerous at best. The stdlib os.LookPath is designed for this use-case.

How do you run LookPath over ssh ? We could run command -v, at least for the recently modern ones

Hmm, command -v is at least POSIX. I suppose the remote use-case makes this harder; but at the same time it feels very impure. The ability to go run over SSH might be nice in the end, but that's a lot of machinery to build out.

Does it make sense to abstract this a layer deeper and write a cr.LookPath that uses the external binary or os.LookPath based on the target?

Sure, but that is more of a separate refactor

neersighted · 2023-02-05T11:03:33Z

pkg/minikube/cruntime/docker.go

+func getCriDockerdPath(cr CommandRunner) string {
+        rr, err := cr.RunCmd(exec.Command("which", "cri-dockerd"))
+        if err != nil {
+                return "/usr/bin/cri-dockerd"


It's probably better to return an error/fail early, where we can describe what happened, rather than fail later in the systemd unit...

I think we already looked for cri-dockerd in the user-facing command code, so this is not supposed to fail (at all)

In that case, it seems better for everyone's collective sanity that an error is returned here, and that it does tear down the call stack/bubble up, as otherwise if the prelude to this code being invoked changes (and these assumptions are not obvious), severe bit-rot/surprising effects could ensue...

Well, it was not worse (than before)

@afbjorklund how about returning the current default value in case of "which" failing,
currently it assumes "/usr/bin/cri-dockerd" in case "which" fails we dont even try the old way.

That is the default.

Some people install the daemon in /usr/local/bin, rather than under /usr/bin as the unit expects.

medyagh · 2023-02-05T16:52:00Z

/ok-to-test

minikube-pr-bot · 2023-02-05T18:34:34Z

kvm2 driver with docker runtime

+----------------+----------+---------------------+
|    COMMAND     | MINIKUBE | MINIKUBE (PR 15784) |
+----------------+----------+---------------------+
| minikube start | 53.6s    | 53.1s               |
| enable ingress | 26.8s    | 26.5s               |
+----------------+----------+---------------------+

Times for minikube start: 53.6s 54.8s 52.2s 53.3s 53.9s
Times for minikube (PR 15784) start: 53.7s 53.9s 52.6s 53.3s 52.2s

Times for minikube (PR 15784) ingress: 28.2s 25.3s 27.7s 24.7s 26.3s
Times for minikube ingress: 26.2s 28.2s 26.8s 28.2s 24.7s

docker driver with docker runtime

+----------------+----------+---------------------+
|    COMMAND     | MINIKUBE | MINIKUBE (PR 15784) |
+----------------+----------+---------------------+
| minikube start | 26.3s    | 26.7s               |
| enable ingress | 33.3s    | 33.7s               |
+----------------+----------+---------------------+

Times for minikube start: 25.7s 26.1s 26.0s 26.6s 27.2s
Times for minikube (PR 15784) start: 26.8s 25.8s 27.4s 27.0s 26.7s

Times for minikube ingress: 49.6s 50.6s 25.1s 22.1s 19.1s
Times for minikube (PR 15784) ingress: 21.1s 83.6s 21.1s 21.1s 21.5s

docker driver with containerd runtime

+-------------------+----------+---------------------+
|      COMMAND      | MINIKUBE | MINIKUBE (PR 15784) |
+-------------------+----------+---------------------+
| minikube start    | 22.0s    | 22.0s               |
| ⚠️  enable ingress | 30.0s    | 54.8s ⚠️             |
+-------------------+----------+---------------------+

Times for minikube start: 21.0s 23.1s 22.0s 22.2s 22.0s
Times for minikube (PR 15784) start: 21.9s 22.4s 22.6s 21.4s 21.8s

Times for minikube ingress: 32.6s 31.6s 20.6s 32.6s 32.6s
Times for minikube (PR 15784) ingress: 79.6s 32.6s 47.6s 81.6s 32.6s

minikube-pr-bot · 2023-02-05T19:47:12Z

These are the flake rates of all failed tests.

Environment	Failed Tests	Flake Rate (%)
Hyperkit_macOS	TestRunningBinaryUpgrade (gopogh)	11.52 (chart)
Docker_Linux_containerd	TestDownloadOnlyKic (gopogh)	22.09 (chart)
Docker_Linux	TestDownloadOnlyKic (gopogh)	22.09 (chart)
Docker_macOS	TestIngressAddonLegacy/serial/ValidateIngressDNSAddonActivation (gopogh)	95.15 (chart)
Docker_macOS	TestIngressAddonLegacy/serial/ValidateIngressAddonActivation (gopogh)	100.00 (chart)
Docker_macOS	TestIngressAddonLegacy/serial/ValidateIngressAddons (gopogh)	100.00 (chart)
Docker_macOS	TestIngressAddonLegacy/StartLegacyK8sCluster (gopogh)	100.00 (chart)
Docker_macOS	TestKubernetesUpgrade (gopogh)	100.00 (chart)
Docker_macOS	TestMissingContainerUpgrade (gopogh)	100.00 (chart)
Docker_macOS	TestRunningBinaryUpgrade (gopogh)	100.00 (chart)
Docker_macOS	TestStartStop/group/old-k8s-version/serial/AddonExistsAfterStop (gopogh)	100.00 (chart)
Docker_macOS	TestStartStop/group/old-k8s-version/serial/DeployApp (gopogh)	100.00 (chart)
Docker_macOS	TestStartStop/group/old-k8s-version/serial/EnableAddonWhileActive (gopogh)	100.00 (chart)
Docker_macOS	TestStartStop/group/old-k8s-version/serial/FirstStart (gopogh)	100.00 (chart)
Docker_macOS	TestStartStop/group/old-k8s-version/serial/SecondStart (gopogh)	100.00 (chart)
Docker_macOS	TestStartStop/group/old-k8s-version/serial/UserAppExistsAfterStop (gopogh)	100.00 (chart)
Docker_macOS	TestStoppedBinaryUpgrade/Upgrade (gopogh)	100.00 (chart)
KVM_Linux_containerd	TestPreload (gopogh)	100.00 (chart)

To see the flake rates of all tests by environment, click here.

shu-mutou · 2023-02-06T08:47:16Z

Is it possible that what you look for with which is different from what is written in the systemd file?

shu-mutou · 2023-02-06T08:52:51Z

The current minikube code creates a 10-cni.conf file as one of the settings for the installed systemd file, so it would be safer to write the same as the existing systemd file, IMHO.

neersighted · 2023-02-06T15:27:40Z

There is a possibility of such, especially as the user (and not minikube) controls who you execute as and thus the content of the remote $PATH. That being said, I think this is much more sane than trying to parse a systemd unit, and if the user has an incorrect cri-dockerd in the path, minikube using it is not necessarily surprising. The results will be the same as sudo $(command -v cri-dockerd); if that gives unexpected results, then there is something more wrong with the system.

afbjorklund · 2023-02-06T15:29:03Z

Note that sudo does not include /usr/local/bin, on older systems. It failed crictl, earlier.

Use absolute path when calling crictl version #15642

medyagh

@afbjorklund sorry I changed my review

shu-mutou · 2023-02-12T05:39:48Z

@neersighted Thanks for your reply.

I've only used minikube with the none driver, so I don't really understand how it works with other drivers, but my understanding is that if cri-docker.service doesn't work, minikube won't work either. right?
If the path written in the existing systemd file is wrong, cri-docker.service will not work in the first place, so I think that's the cri-dockerd installation guide or the user's responsibility.

neersighted · 2023-02-12T07:44:50Z

The issue here is that cri-dockerd.service does work. Then minikube attempts to change the command line to set the CNI, and we end up here. This PR represents an attempt to prevent minikube from having strong opinions about what path cri-dockerd is available at.

shu-mutou · 2023-02-13T01:04:36Z

@neersighted Thank you for your explanations!
I understood the intention of minikube team and respect it.

k8s-ci-robot · 2023-02-17T17:55:33Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: afbjorklund, 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~~ [afbjorklund,medyagh]

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

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 5, 2023

k8s-ci-robot requested review from prezha and spowelljr February 5, 2023 10:50

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 Feb 5, 2023

neersighted suggested changes Feb 5, 2023

View reviewed changes

Look for cri-dockerd instead of hardcoding

90bc8a9

Some people install the daemon in /usr/local/bin, rather than under /usr/bin as the unit expects.

afbjorklund force-pushed the cri-dockerd-path branch from 177bb88 to 90bc8a9 Compare February 5, 2023 11:33

medyagh approved these changes Feb 5, 2023

View reviewed changes

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

medyagh requested changes Feb 8, 2023

View reviewed changes

medyagh approved these changes Feb 17, 2023

View reviewed changes

medyagh merged commit d8b4a2d into kubernetes:master Feb 17, 2023

medyagh changed the title ~~Look for cri-dockerd instead of hardcoding~~ none driver: look for cri-dockerd instead of hardcoding Feb 17, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

none driver: look for cri-dockerd instead of hardcoding #15784

none driver: look for cri-dockerd instead of hardcoding #15784

afbjorklund commented Feb 5, 2023

neersighted Feb 5, 2023 •

edited

Loading

afbjorklund Feb 5, 2023

neersighted Feb 5, 2023

afbjorklund Feb 5, 2023

neersighted Feb 5, 2023 •

edited

Loading

afbjorklund Feb 5, 2023

neersighted Feb 5, 2023

afbjorklund Feb 6, 2023

medyagh Feb 8, 2023

afbjorklund Feb 16, 2023

medyagh commented Feb 5, 2023

minikube-pr-bot commented Feb 5, 2023

minikube-pr-bot commented Feb 5, 2023

shu-mutou commented Feb 6, 2023

shu-mutou commented Feb 6, 2023 •

edited

Loading

neersighted commented Feb 6, 2023

afbjorklund commented Feb 6, 2023 •

edited

Loading

medyagh left a comment

shu-mutou commented Feb 12, 2023

neersighted commented Feb 12, 2023

shu-mutou commented Feb 13, 2023

k8s-ci-robot commented Feb 17, 2023

none driver: look for cri-dockerd instead of hardcoding #15784

none driver: look for cri-dockerd instead of hardcoding #15784

Conversation

afbjorklund commented Feb 5, 2023

neersighted Feb 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neersighted Feb 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

medyagh commented Feb 5, 2023

minikube-pr-bot commented Feb 5, 2023

minikube-pr-bot commented Feb 5, 2023

shu-mutou commented Feb 6, 2023

shu-mutou commented Feb 6, 2023 • edited Loading

neersighted commented Feb 6, 2023

afbjorklund commented Feb 6, 2023 • edited Loading

medyagh left a comment

Choose a reason for hiding this comment

shu-mutou commented Feb 12, 2023

neersighted commented Feb 12, 2023

shu-mutou commented Feb 13, 2023

k8s-ci-robot commented Feb 17, 2023

neersighted Feb 5, 2023 •

edited

Loading

neersighted Feb 5, 2023 •

edited

Loading

shu-mutou commented Feb 6, 2023 •

edited

Loading

afbjorklund commented Feb 6, 2023 •

edited

Loading