Skip to content
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

Always use cni unless running with dockershim #14780

Merged
merged 3 commits into from
Jun 14, 2023

Conversation

afbjorklund
Copy link
Collaborator

@afbjorklund afbjorklund commented Aug 11, 2022

Since cri-dockerd 0.2.5, it now defaults to cni.

So when using cri (not dockershim), use cni too.


Closes #14760

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 11, 2022
@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 11, 2022
@afbjorklund
Copy link
Collaborator Author

/ok-to-test

@k8s-ci-robot k8s-ci-robot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Aug 11, 2022
@minikube-pr-bot

This comment has been minimized.

@afbjorklund
Copy link
Collaborator Author

/retest-required

@medyagh
Copy link
Member

medyagh commented Aug 17, 2022

Thank you for the PR
@afbjorklund mind taking a look at this failure to see if it is related to this PR or not ?
https://storage.googleapis.com/minikube-builds/logs/14780/25250/Docker_Linux.html#fail_TestNetworkPlugins%2fgroup%2fauto%2fHairPin

@afbjorklund
Copy link
Collaborator Author

mind taking a look at this failure to see if it is related to this PR or not ?

it is related, it was hardcoding "not Docker" in the expectations.

{"auto", []string{}, "", "", ContainerRuntime() != "docker"},

@minikube-pr-bot

This comment has been minimized.

@minikube-pr-bot

This comment has been minimized.

@spowelljr
Copy link
Member

/retest

@spowelljr
Copy link
Member

@minikube-pr-bot

This comment has been minimized.

@minikube-pr-bot

This comment has been minimized.

@mgabeler-lee-6rs
Copy link

With the latest release of crictl, minikube with the none driver no longer works without a newer cri-dockerd, which doesn't work without this PR, and I believe #14703.

Any outlook on when those two (and perhaps #14827 which is related) might be ready to merge?

@afbjorklund
Copy link
Collaborator Author

afbjorklund commented Dec 20, 2022

There is a new cri-dockerd 0.3.0 out, with support for Kubernetes 1.26.0. Hopefully it can include the other fixes... ?

But I don't think the CNI bugs are related to the CRI upgrade, even if both of them should be done (both are ancient)

@mgabeler-lee-6rs
Copy link

Hmm, at one point I needed some combination of this & the other two PRs I listed to address #14724 -- that specifically lists #14703 more than this one for that issue, but I'd been testing with a local build merging all 3. Perhaps some further changes in cri-dockerd have made that not critical now, as it seems to work out of the box again if I grab the latest releases of minikube & cri-dockerd & crictl.

@maxb
Copy link

maxb commented Dec 29, 2022

How will the changes here impact users like me, who just want to run with the none driver and no network plugin at all, for a simple single-node cluster on a development laptop? I currently do this by adding the --network-plugin= option to the cri-dockerd command line.

@afbjorklund
Copy link
Collaborator Author

afbjorklund commented Dec 29, 2022

How will the changes here impact users like me, who just want to run with the none driver and no network plugin at all, for a simple single-node cluster on a development laptop? I currently do this by adding the --network-plugin= option to the cri-dockerd command line.

The no-op plugin (--network-plugin=) and kubenet has been removed from upstream (retro-actively), so since Kubernetes 1.24 it is now mandatory to use CRI and CNI... But you can still use a simple single-node network if you want to, by using --cni=bridge (it is rather similar).

{
  "cniVersion": "0.3.1",
  "name": "bridge",
  "plugins": [
    {
      "type": "bridge",
      "bridge": "bridge",
      "addIf": "true",
      "isDefaultGateway": true,
      "forceAddress": false,
      "ipMasq": true,
      "hairpinMode": true,
      "ipam": {
          "type": "host-local",
          "subnet": "10.244.0.0/16"
      }
    },
    {
      "type": "portmap",
      "capabilities": {
          "portMappings": true
      }
    }
  ]
}

@maxb
Copy link

maxb commented Dec 29, 2022

Currently I have a functional single node cluster with no CNI at all, using Kubernetes 1.26.0, minikube 1.28.0, cri-dockerd 0.3.0, passing --network-plugin= to cri-dockerd.

@afbjorklund
Copy link
Collaborator Author

afbjorklund commented Dec 29, 2022

If you really want it, it would be possible to make a special case for docker. The default would still be to use CNI (for 1.24+), but to keep the --network-plugin option around...

Anyhow, it was supposed to be possible already. Just need to be a little more explicit about it, and possibly we need to add "" as a special case (or something similar to "no-op" perhaps?)

minikube start --container-runtime=docker --network-plugin=no-op

It is likely to break down in the future, but seems to still work for 1.26

@afbjorklund
Copy link
Collaborator Author

afbjorklund commented Dec 29, 2022

Previously the code used "" to mean Docker container runtime, or "" to mean No-op network plugin.

But now they mean "use the default" (whatever that happens to be), so need explicit strings for the old values.

Slightly complicated that dockershim uses kubernetes.io/no-op instead.

Maybe hardcoding the empty string would work better, for that special case.

@maxb
Copy link

maxb commented Dec 30, 2022

My main interest is that the learning curve towards operating a single node driver=none setup, should avoid extra complexity where practical. It has already grown a fair bit recently, (cri-dockerd, crictl, needing to research the requirement to add --network-plugin=) but I think there's still value in allowing people to leave out CNI where not necessary, and defer tangling with that until a later stage of their Kubernetes educational journey.

I recognise driver=none is never going to be the preferred option for "just make it work, as quickly as possible" use-cases, but it really is great for people willing to put in a minor additional setup cost, in exchange for easier educational tinkering.

@afbjorklund
Copy link
Collaborator Author

afbjorklund commented Dec 30, 2022

My main interest is that the learning curve towards operating a single node driver=none setup, should avoid extra complexity where practical. It has already grown a fair bit recently, (cri-dockerd, crictl, needing to research the requirement to add --network-plugin=) but I think there's still value in allowing people to leave out CNI where not necessary, and defer tangling with that until a later stage of their Kubernetes educational journey.

The simple days of Docker are gone, upstream has decided to make CRI mandatory (in 1.24) and to make CNI the default (in 1.25, retroactively also for 1.24). And on the latest release day, using Docker was not even possible (for 1.26).

So unfortunately people now have to learn both CRI and CNI, before the cluster will even start. It is possible that this will also include CSI in the future, but for now storage remains "optional" for a single-node cluster (and NFS still works...)

I fully assume that we will have to change to containerd as the default for 1.27, simple because dockerd isn't tested.
That will make life even worse, since you have to learn about containerd and buildkitd and other low-level stuff.

The nerdctl tools makes them slightly more comfortable to use, but it is not fully compatible with the old tools ?
You can also go the podman route (with CRI-O), if you prefer to use something "different". But still not Docker.

I recognise driver=none is never going to be the preferred option for "just make it work, as quickly as possible" use-cases, but it really is great for people willing to put in a minor additional setup cost, in exchange for easier educational tinkering.

The "none" driver is used in both learning and testing environments, and it remains an important target.

I detailed some of the worst current bugs with it here:

The lack of packaging and the lack of documentation makes it more cumbersome, than what it needed to be.

With Docker no longer being Open Source (with Desktop, and Machine no longer being developed), it is "gone".
I personally think that Lima is the best successor, after losing faith in Podman Desktop. Your mileage may vary.

So it seems that Kubernetes is now more Linux, in that you can tinker with it yourself or you can use a distribution ?
Hopefully, minikube start will continue to be such a Kubernetes distribution - with all the batteries included.

Anyway, this feature (i.e. network-plugin) will remain - while available.

For the future of minikube, there is always the Slack channel: #minikube

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 20, 2023
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 26, 2023
@minikube-pr-bot

This comment has been minimized.

@minikube-pr-bot

This comment has been minimized.

@medyagh
Copy link
Member

medyagh commented Mar 22, 2023

@afbjorklund the only test failing is

TestNetworkPlugins/group/false


TestNetworkPlugins/group/false

could u take a look if we fix that we could merge

@minikube-pr-bot

This comment has been minimized.

@minikube-pr-bot

This comment has been minimized.

Since cri-dockerd 0.25, it now defaults to cni.

So when using cri (not dockershim), use cni too.
You can still use other network plugins, with it.

Just that cni is now used by default, for 1.24 up.
@minikube-pr-bot
Copy link

kvm2 driver with docker runtime

+----------------+----------+---------------------+
|    COMMAND     | MINIKUBE | MINIKUBE (PR 14780) |
+----------------+----------+---------------------+
| minikube start | 52.3s    | 50.6s               |
| enable ingress | 27.6s    | 27.3s               |
+----------------+----------+---------------------+

Times for minikube ingress: 27.7s 27.8s 27.8s 27.2s 27.2s
Times for minikube (PR 14780) ingress: 28.3s 27.2s 27.2s 25.2s 28.7s

Times for minikube start: 51.6s 51.3s 51.8s 54.5s 52.2s
Times for minikube (PR 14780) start: 49.7s 50.2s 51.5s 50.3s 51.1s

docker driver with docker runtime

+-------------------+----------+---------------------+
|      COMMAND      | MINIKUBE | MINIKUBE (PR 14780) |
+-------------------+----------+---------------------+
| minikube start    | 23.1s    | 23.6s               |
| ⚠️  enable ingress | 20.9s    | 48.9s ⚠️             |
+-------------------+----------+---------------------+

Times for minikube ingress: 20.9s 20.9s 20.9s 20.9s 20.9s
Times for minikube (PR 14780) ingress: 48.4s 49.4s 49.4s 48.9s 48.3s

Times for minikube start: 21.4s 23.0s 25.1s 25.2s 21.0s
Times for minikube (PR 14780) start: 22.7s 23.0s 25.3s 22.4s 24.8s

docker driver with containerd runtime

+----------------+----------+---------------------+
|    COMMAND     | MINIKUBE | MINIKUBE (PR 14780) |
+----------------+----------+---------------------+
| minikube start | 23.3s    | 20.9s               |
| enable ingress | 31.1s    | 26.5s               |
+----------------+----------+---------------------+

Times for minikube start: 23.9s 22.6s 22.8s 23.5s 23.8s
Times for minikube (PR 14780) start: 20.9s 22.4s 21.7s 20.2s 19.5s

Times for minikube ingress: 30.3s 31.3s 31.3s 31.4s 31.3s
Times for minikube (PR 14780) ingress: 31.3s 19.3s 31.4s 18.4s 32.3s

@minikube-pr-bot
Copy link

These are the flake rates of all failed tests.

Environment Failed Tests Flake Rate (%)
KVM_Linux TestFunctional/parallel/MountCmd/specific-port (gopogh) 0.65 (chart)
KVM_Linux TestStartStop/group/no-preload/serial/DeployApp (gopogh) 0.65 (chart)
KVM_Linux TestStartStop/group/no-preload/serial/EnableAddonWhileActive (gopogh) 0.65 (chart)
KVM_Linux TestStartStop/group/no-preload/serial/FirstStart (gopogh) 0.65 (chart)
KVM_Linux TestStartStop/group/no-preload/serial/VerifyKubernetesImages (gopogh) 2.61 (chart)
KVM_Linux TestStartStop/group/no-preload/serial/AddonExistsAfterStop (gopogh) 3.27 (chart)
KVM_Linux TestStartStop/group/no-preload/serial/Pause (gopogh) 3.27 (chart)
KVM_Linux TestStartStop/group/no-preload/serial/SecondStart (gopogh) 3.27 (chart)
KVM_Linux TestStartStop/group/no-preload/serial/UserAppExistsAfterStop (gopogh) 3.27 (chart)

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

@spowelljr spowelljr merged commit 72ca8a2 into kubernetes:master Jun 14, 2023
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: afbjorklund, medyagh, spowelljr

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,spowelljr]

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

@tuanaac
Copy link

tuanaac commented Sep 18, 2023

@afbjorklund could you please advise me how to change the subnet?

{
"cniVersion": "0.3.1",
"name": "bridge",
"plugins": [
{
"type": "bridge",
"bridge": "bridge",
"addIf": "true",
"isDefaultGateway": true,
"forceAddress": false,
"ipMasq": true,
"hairpinMode": true,
"ipam": {
"type": "host-local",
"subnet": "10.244.0.0/16"
}
},
{
"type": "portmap",
"capabilities": {
"portMappings": true
}
}
]
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Default CNI to enabled, even for Docker runtime
8 participants