fix default cni selection for cri-docker(d)

Author: prezha

pkg/minikube/cni/cni.go

@@ -27,6 +27,7 @@ import (
 	"strings"
 	"time"
 
+	"github.com/blang/semver/v4"
 	"github.com/pkg/errors"
 	"k8s.io/klog/v2"
 	"k8s.io/minikube/pkg/kapi"
@@ -36,6 +37,7 @@ import (
 	"k8s.io/minikube/pkg/minikube/constants"
 	"k8s.io/minikube/pkg/minikube/driver"
 	"k8s.io/minikube/pkg/minikube/vmpath"
+	"k8s.io/minikube/pkg/util"
 )
 
 const (
@@ -160,6 +162,18 @@ func chooseDefault(cc config.ClusterConfig) Manager {
 		return Bridge{cc: cc}
 	}
 
+	// for docker container runtime and k8s v1.24+ where dockershim and kubenet were removed, we fallback to bridge cni for cri-docker(d)
+	// ref: https://github.com/Mirantis/cri-dockerd#important
+	// ref: https://github.com/Mirantis/cri-dockerd#to-use-with-kubernetes
+	// note: currently, default cni that we "distribute" (in /etc/cni/net.d) is based on cri-o bridge, and
+	// because it does not currently use portmap plugin, we pick "our" bridge instead (cri-o one will be disabled automatically)
+	// ref: https://github.com/cri-o/cri-o/blob/f317b267ddef21aee5ffc92d890a77112b006815/contrib/cni/10-crio-bridge.conflist
+	kv, err := util.ParseKubernetesVersion(cc.KubernetesConfig.KubernetesVersion)
+	if err == nil && kv.GTE(semver.MustParse("1.24.0-alpha.2")) {
+		klog.Infof("%q driver + %s runtime found, recommending bridge", cc.Driver, cc.KubernetesConfig.ContainerRuntime)
+		return Bridge{cc: cc}
+	}
+
 	klog.Infof("CNI unnecessary in this configuration, recommending no CNI")
 	return Disabled{cc: cc}
 }

pkg/minikube/cruntime/containerd.go

@@ -229,7 +229,7 @@ func (r *Containerd) Enable(disOthers bool, cgroupDriver string, inUserNamespace
 	currentVersion, err := r.Version()
 	if err == nil && semver.MustParse(targetVersion).GT(semver.MustParse(currentVersion)) {
 		klog.Infof("replacing original containerd with v%s-%s-%s", targetVersion, runtime.GOOS, runtime.GOARCH)
-		r.Init.ForceStop("containerd")
+		_ = r.Init.ForceStop("containerd")
 		if err := updateContainerdBinary(r.Runner, targetVersion, runtime.GOOS, runtime.GOARCH); err != nil {
 			klog.Warningf("unable to replace original containerd with v%s-%s-%s: %v", targetVersion, runtime.GOOS, runtime.GOARCH, err)
 		}

pkg/minikube/cruntime/cri.go

@@ -236,7 +236,7 @@ func stopCRIContainers(cr CommandRunner, ids []string) error {
 	// - docker stop --help => -t, --time int   Seconds to wait for stop before killing it (default 10)
 	// - crictl stop --help => --timeout value, -t value  Seconds to wait to kill the container after a graceful stop is requested (default: 0)
 	// to prevent "stuck" containers blocking ports (eg, "[ERROR Port-2379|2380]: Port 2379|2380 is in use" for etcd during "live" k8s upgrade)
-	args := append([]string{crictl, "stop", "--timeout", "10"}, ids...)
+	args := append([]string{crictl, "stop", "--timeout=10"}, ids...)
 	c := exec.Command("sudo", args...)
 	if _, err := cr.RunCmd(c); err != nil {
 		return errors.Wrap(err, "crictl")

pkg/minikube/cruntime/cruntime_test.go

@@ -460,7 +460,7 @@ func (f *FakeRunner) crictl(args []string, _ bool) (string, error) {
 			return strings.Join(ids, "\n"), nil
 		}
 	case "stop":
-		for _, id := range args[1:] {
+		for _, id := range args[2:] {
 			f.t.Logf("fake crictl: Stopping id %q", id)
 			if f.containers[id] == "" {
 				return "", fmt.Errorf("no such container")

pkg/minikube/node/start.go

@@ -423,17 +423,16 @@ func configureRuntimes(runner cruntime.CommandRunner, cc config.ClusterConfig, k
 	if err := cni.ConfigureLoopback(runner); err != nil {
 		klog.Warningf("unable to name loopback interface in dockerConfigureNetworkPlugin: %v", err)
 	}
-	// ensure all default CNI(s) are properly configured on each and every node (re)start
-	// make sure container runtime is restarted afterwards for these changes to take effect
-	if err := cni.ConfigureDefaultBridgeCNIs(runner, cc.KubernetesConfig.NetworkPlugin); err != nil {
-		klog.Errorf("unable to disable preinstalled bridge CNI(s): %v", err)
-	}
-
 	if kv.GTE(semver.MustParse("1.24.0-alpha.2")) {
 		if err := cruntime.ConfigureNetworkPlugin(cr, runner, cc.KubernetesConfig.NetworkPlugin); err != nil {
 			exit.Error(reason.RuntimeEnable, "Failed to configure network plugin", err)
 		}
 	}
+	// ensure all default CNI(s) are properly configured on each and every node (re)start
+	// make sure container runtime is restarted afterwards for these changes to take effect
+	if err := cni.ConfigureDefaultBridgeCNIs(runner, cc.KubernetesConfig.NetworkPlugin); err != nil {
+		klog.Errorf("unable to disable preinstalled bridge CNI(s): %v", err)
+	}
 
 	// Preload is overly invasive for bare metal, and caching is not meaningful.
 	// KIC handles preload elsewhere.

test/integration/addons_test.go

@@ -71,7 +71,7 @@ func TestAddons(t *testing.T) {
 		// MOCK_GOOGLE_TOKEN forces the gcp-auth webhook to use a mock token instead of trying to get a valid one from the credentials.
 		os.Setenv("MOCK_GOOGLE_TOKEN", "true")
 
-		// for some reason, (Docker_Cloud_Shell) sets 'MINIKUBE_FORCE_SYSTEMD=true' while having cgroupfs set in docker (and probably os itself), which might make it unstable and occasionaly fail:
+		// for some reason, (Docker_Cloud_Shell) sets 'MINIKUBE_FORCE_SYSTEMD=true' while having cgroupfs set in docker (and probably os itself), which might make it unstable and occasionally fail:
 		// - I1226 15:05:24.834294   11286 out.go:177]   - MINIKUBE_FORCE_SYSTEMD=true
 		// - I1226 15:05:25.070037   11286 info.go:266] docker info: {... CgroupDriver:cgroupfs ...}
 		// ref: https://storage.googleapis.com/minikube-builds/logs/15463/27154/Docker_Cloud_Shell.html