improve how cni and cruntimes work together #11185

prezha · 2021-04-23T22:34:06Z

as stated in the issue's description:

crio is the only container runtime atm that allows specifying cni network that should be used (via 'cni_default_network' config param) and, unlike containerd or docker, it doesn't need custom kubelet's '--cni-conf-dir' flag to avoid conflicting CNI configs in default /etc/cni/net.d, so we can used that to improve how cni & cruntime work together (especially for multinode), while also avoiding later runtime reconfiguration and restart

medyagh · 2021-04-23T22:53:23Z

/ok-to-test

minikube-pr-bot · 2021-04-24T00:08:37Z

kvm2 driver with docker runtime

+----------------+----------+---------------------+
|    COMMAND     | MINIKUBE | MINIKUBE (PR 11185) |
+----------------+----------+---------------------+
| minikube start | 49.5s    | 50.9s               |
| enable ingress | 35.3s    | 34.6s               |
+----------------+----------+---------------------+

Times for minikube start: 48.8s 48.4s 51.2s 50.5s 48.8s
Times for minikube (PR 11185) start: 54.8s 47.9s 50.5s 47.7s 53.7s

Times for minikube (PR 11185) ingress: 36.3s 34.3s 33.7s 33.7s 35.2s
Times for minikube ingress: 34.8s 36.3s 34.3s 35.2s 35.8s

docker driver with docker runtime

+----------------+----------+---------------------+
|    COMMAND     | MINIKUBE | MINIKUBE (PR 11185) |
+----------------+----------+---------------------+
| minikube start | 21.9s    | 21.6s               |
| enable ingress | 30.8s    | 33.2s               |
+----------------+----------+---------------------+

Times for minikube start: 23.0s 21.9s 21.7s 21.4s 21.7s
Times for minikube (PR 11185) start: 21.5s 21.2s 21.4s 22.0s 21.8s

Times for minikube ingress: 27.5s 41.0s 29.5s 27.6s 28.5s
Times for minikube (PR 11185) ingress: 42.0s 30.0s 30.0s 34.0s 30.0s

docker driver with containerd runtime
error collecting results for docker driver: timing run 0 with minikube: timing cmd: [out/minikube addons enable ingress]: waiting for minikube: exit status 10

minikube-pr-bot · 2021-04-24T03:28:55Z

kvm2 driver with docker runtime

+----------------+----------+---------------------+
|    COMMAND     | MINIKUBE | MINIKUBE (PR 11185) |
+----------------+----------+---------------------+
| minikube start | 50.6s    | 49.6s               |
| enable ingress | 34.9s    | 35.0s               |
+----------------+----------+---------------------+

Times for minikube start: 54.8s 51.8s 46.9s 51.5s 48.1s
Times for minikube (PR 11185) start: 47.4s 48.7s 50.0s 51.6s 50.5s

Times for minikube ingress: 34.3s 37.7s 34.2s 34.3s 33.8s
Times for minikube (PR 11185) ingress: 34.3s 37.3s 34.4s 33.9s 35.3s

docker driver with docker runtime

+----------------+----------+---------------------+
|    COMMAND     | MINIKUBE | MINIKUBE (PR 11185) |
+----------------+----------+---------------------+
| minikube start | 22.1s    | 21.2s               |
| enable ingress | 30.5s    | 31.2s               |
+----------------+----------+---------------------+

Times for minikube start: 22.4s 21.9s 22.1s 22.0s 22.1s
Times for minikube (PR 11185) start: 21.0s 21.8s 21.3s 21.1s 20.8s

Times for minikube ingress: 33.0s 32.0s 28.5s 30.0s 29.0s
Times for minikube (PR 11185) ingress: 29.5s 29.5s 28.5s 38.0s 30.5s

docker driver with containerd runtime
error collecting results for docker driver: timing run 0 with minikube: timing cmd: [out/minikube addons enable ingress]: waiting for minikube: exit status 10

medyagh · 2021-04-25T03:30:54Z

/retest-this-please

minikube-pr-bot · 2021-04-25T05:18:02Z

kvm2 driver with docker runtime

+----------------+----------+---------------------+
|    COMMAND     | MINIKUBE | MINIKUBE (PR 11185) |
+----------------+----------+---------------------+
| minikube start | 48.5s    | 49.8s               |
| enable ingress | 34.6s    | 34.4s               |
+----------------+----------+---------------------+

Times for minikube (PR 11185) ingress: 34.3s 34.7s 34.2s 34.2s 34.7s
Times for minikube ingress: 34.3s 34.8s 35.8s 34.2s 33.7s

Times for minikube start: 51.9s 47.1s 46.5s 49.2s 48.1s
Times for minikube (PR 11185) start: 49.7s 46.1s 51.3s 48.1s 53.9s

docker driver with docker runtime

+----------------+----------+---------------------+
|    COMMAND     | MINIKUBE | MINIKUBE (PR 11185) |
+----------------+----------+---------------------+
| minikube start | 21.7s    | 21.5s               |
| enable ingress | 32.0s    | 30.5s               |
+----------------+----------+---------------------+

Times for minikube ingress: 29.0s 31.4s 32.5s 36.5s 30.5s
Times for minikube (PR 11185) ingress: 34.0s 29.5s 29.5s 30.5s 29.0s

Times for minikube (PR 11185) start: 22.4s 21.4s 20.8s 21.2s 21.6s
Times for minikube start: 22.9s 20.4s 22.5s 21.0s 21.6s

docker driver with containerd runtime
error collecting results for docker driver: timing run 0 with minikube: timing cmd: [out/minikube addons enable ingress]: waiting for minikube: exit status 10

medyagh · 2021-04-25T16:03:46Z

@prezha needs a rebase

cmd/minikube/cmd/start_flags.go

pkg/minikube/bootstrapper/bsutil/kubeadm.go

pkg/minikube/cni/cni.go

medyagh · 2021-04-25T16:25:56Z

pkg/minikube/cruntime/crio.go

-		return errors.Wrap(err, "parse cidr")
-	}
-
-	oldNet := "10.88.0.0/16"


Are we sure we dont need this ?
@afbjorklund

I think the idea was to stop kubernetes and podman/crio from trying to use the same network.

Some conflict with the default 10.85.0.0/16 and 10.88.0.0/16 subnets ? #8570

@prezha shouldn’t we keep this ? To avoid the conflict ?

medyagh · 2021-04-25T16:26:47Z

@afbjorklund what is your guidance on this PR ?

@prezha now that we merged the bump kindnet, could we expect the containerd test for this PR not to timeout ?

prezha · 2021-04-25T17:16:01Z

@prezha now that we merged the bump kindnet, could we expect the containerd test for this PR not to timeout ?

i'm not sure if it will work without #11106 being merged first

minikube-pr-bot · 2021-04-25T17:28:49Z

kvm2 driver with docker runtime

+----------------+----------+---------------------+
|    COMMAND     | MINIKUBE | MINIKUBE (PR 11185) |
+----------------+----------+---------------------+
| minikube start | 50.5s    | 50.8s               |
| enable ingress | 40.5s    | 40.9s               |
+----------------+----------+---------------------+

Times for minikube start: 54.5s 50.2s 49.3s 50.5s 48.1s
Times for minikube (PR 11185) start: 55.3s 48.2s 47.8s 55.0s 47.5s

Times for minikube (PR 11185) ingress: 42.3s 35.8s 44.3s 42.8s 39.3s
Times for minikube ingress: 42.8s 36.2s 42.8s 43.3s 37.2s

docker driver with docker runtime

+----------------+----------+---------------------+
|    COMMAND     | MINIKUBE | MINIKUBE (PR 11185) |
+----------------+----------+---------------------+
| minikube start | 21.7s    | 21.2s               |
| enable ingress | 35.8s    | 37.3s               |
+----------------+----------+---------------------+

Times for minikube start: 23.2s 20.8s 21.8s 21.4s 21.1s
Times for minikube (PR 11185) start: 21.8s 21.1s 20.1s 21.2s 21.5s

Times for minikube ingress: 34.5s 40.0s 32.5s 37.6s 34.5s
Times for minikube (PR 11185) ingress: 33.0s 41.5s 37.5s 37.5s 37.0s

docker driver with containerd runtime
error collecting results for docker driver: timing run 0 with minikube: timing cmd: [out/minikube addons enable ingress]: waiting for minikube: exit status 10

medyagh · 2021-04-25T21:37:44Z

@prezha now that we merged the bump kindnet, could we expect the containerd test for this PR not to timeout ?

i'm not sure if it will work without #11106 being merged first

How about pull all of them in that PR just to prove first it works, if it does we can merge the smaller PRs ?
Currently I yet have not seen one of them that doesn’t time out and I don’t wanna risk reverting them if proved that it didn’t make difference

medyagh · 2021-04-26T19:57:12Z

@prezha @afbjorklund I confirm this PR fixes
the coredns NOT coming up

Before

$ kc get pods -A 
NAMESPACE     NAME                               READY   STATUS              RESTARTS   AGE
kube-system   coredns-74ff55c5b-2hchh            0/1     ContainerCreating   0          77s
kube-system   etcd-minikube                      1/1     Running             0          89s
kube-system   kindnet-rwvfz                      1/1     Running             0          77s
kube-system   kube-apiserver-minikube            1/1     Running             0          89s
kube-system   kube-controller-manager-minikube   1/1     Running             0          89s
kube-system   kube-proxy-8hphs                   1/1     Running             0          77s
kube-system   kube-scheduler-minikube            1/1     Running             0          89s
kube-system   storage-provisioner                1/1     Running             1          90s

#after

$ kc get pods -A
NAMESPACE     NAME                               READY   STATUS    RESTARTS   AGE
kube-system   coredns-74ff55c5b-cs6st            0/1     Running   0          36s
kube-system   etcd-minikube                      1/1     Running   0          47s
kube-system   kindnet-tz8qc                      1/1     Running   0          36s
kube-system   kube-apiserver-minikube            1/1     Running   0          47s
kube-system   kube-controller-manager-minikube   0/1     Running   0          47s
kube-system   kube-proxy-tnsqx                   1/1     Running   0          36s
kube-system   kube-scheduler-minikube            1/1     Running   0          47s
kube-system   storage-provisioner                1/1     Running   0          47s

medyagh · 2021-04-26T21:58:24Z

pkg/minikube/cni/cni.go

+// Note: currently, this change affects only Kindnet CNI (and all multinodes using it), but it can be easily expanded to other/all CNIs if needed.
+// Note2: Cilium does not need workaround as they automatically restart pods after CNI is successfully deployed.
+func configureCNI(cc *config.ClusterConfig, cnm Manager) error {
+	if _, kindnet := cnm.(KindNet); kindnet {


on the crio KVM test I see these failing https://storage.googleapis.com/minikube-builds/logs/11189/cc9afff/KVM_Linux_crio.html

214 | TestNetworkPlugins/group/auto/HairPin | 0.24 -- | -- | -- 237 | TestNetworkPlugins/group/calico/DNS | 395.35 241 | TestNetworkPlugins/group/false/KubeletFlags | 0.24 252 | TestNetworkPlugins/group/flannel/DNS | 377.81

@ilya-zuyev do u midn taking a look ?

k8s-ci-robot · 2021-04-29T22:01:22Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ilya-zuyev, medyagh, prezha

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~~ [ilya-zuyev,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 cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 23, 2021

k8s-ci-robot requested review from blueelvis and medyagh April 23, 2021 22:34

prezha mentioned this pull request Apr 23, 2021

fix wait for terminated pods or for minion nodes in the just-restarted cluster #11106

Closed

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

prezha force-pushed the workout-cni-and-cruntimes branch from bd72030 to ce9ce17 Compare April 24, 2021 01:17

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 25, 2021

medyagh requested a review from afbjorklund April 25, 2021 16:06