-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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.
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
improve how cni and cruntimes work together #11185
improve how cni and cruntimes work together #11185
Conversation
/ok-to-test |
kvm2 driver with docker runtime
Times for minikube start: 48.8s 48.4s 51.2s 50.5s 48.8s Times for minikube (PR 11185) ingress: 36.3s 34.3s 33.7s 33.7s 35.2s docker driver with docker runtime
Times for minikube start: 23.0s 21.9s 21.7s 21.4s 21.7s Times for minikube ingress: 27.5s 41.0s 29.5s 27.6s 28.5s docker driver with containerd runtime |
bd72030
to
ce9ce17
Compare
kvm2 driver with docker runtime
Times for minikube start: 54.8s 51.8s 46.9s 51.5s 48.1s Times for minikube ingress: 34.3s 37.7s 34.2s 34.3s 33.8s docker driver with docker runtime
Times for minikube start: 22.4s 21.9s 22.1s 22.0s 22.1s Times for minikube ingress: 33.0s 32.0s 28.5s 30.0s 29.0s docker driver with containerd runtime |
/retest-this-please |
kvm2 driver with docker runtime
Times for minikube (PR 11185) ingress: 34.3s 34.7s 34.2s 34.2s 34.7s Times for minikube start: 51.9s 47.1s 46.5s 49.2s 48.1s docker driver with docker runtime
Times for minikube ingress: 29.0s 31.4s 32.5s 36.5s 30.5s Times for minikube (PR 11185) start: 22.4s 21.4s 20.8s 21.2s 21.6s docker driver with containerd runtime |
@prezha needs a rebase |
return errors.Wrap(err, "parse cidr") | ||
} | ||
|
||
oldNet := "10.88.0.0/16" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we sure we dont need this ?
@afbjorklund
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@prezha shouldn’t we keep this ? To avoid the conflict ?
@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 ? |
ce9ce17
to
e3b4566
Compare
kvm2 driver with docker runtime
Times for minikube start: 54.5s 50.2s 49.3s 50.5s 48.1s Times for minikube (PR 11185) ingress: 42.3s 35.8s 44.3s 42.8s 39.3s docker driver with docker runtime
Times for minikube start: 23.2s 20.8s 21.8s 21.4s 21.1s Times for minikube ingress: 34.5s 40.0s 32.5s 37.6s 34.5s docker driver with containerd runtime |
How about pull all of them in that PR just to prove first it works, if it does we can merge the smaller PRs ? |
@prezha @afbjorklund I confirm this PR fixes Before
#after
|
// 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 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ?
[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:
Approvers can indicate their approval by writing |
fixes #11184
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