-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Embed cri-dockerd in k3s #5741
Comments
I proposed doing this for 1.24 and got voted down. cri-dockerd also turned out to have a bunch of performance issues related to pulling stats that we had to call in some favors at Mirantis to get addressed. They apparently have only one person on cri-dockerd maintenance who only checks the issues/PRs when he gets around to it: Mirantis/cri-dockerd#38 (comment) I did get as far as actually implementing this before getting voted down though, and it worked fine: 6feaec2 |
Is there more context as why it got voted down. Is Rancher Desktop not going to use cri-dockerd? |
I think folks were mostly just ready to make a clean break from it at the same time as upstream did. With docker support on windows evaporating and cri-dockerd maintenance from Mirantis being so half-hearted it didn't seem worth carrying. Rancher Desktop is using it. They are actually packaging their own docker and containerd versions, along with a fork of cri-dockerd. |
Basically, if Rancher Desktop feels the need to package/fork/maintain cri-dockerd that basically proves there's value in doing that work upstream because docker is the primary solution on the desktop. So can we just shift the effort of maintaining it in Rancher Desktop and do it in k3s. |
Who are the gods of k3s that I need to convince that this is in fact a good idea to include cri-dockerd. Now that 1.24 is the stable release this is becoming more of a problem for me. |
I agree this needs to come back. Docker is still the preeminent way to do container/image development not having this feature in k3s makes using k3s for development difficult and hurts its place in the ecosystem. |
I believe @Oats87 is working on an issue related to cri-dockerd and CNI issues for 1.24 RKE. I would assume the issue could also exist here. |
6feaec2 should still apply mostly cleanly, it's just a matter of PM (or the team) deciding that it's something we want to take on as a support burden. |
The fork was only temporary while upstream was not applying PRs. We are bundling the latest upstream version of cri-dockerd now. For Rancher Desktop it will not matter if k3s starts bundling cri-dockerd or not. Since we support running all older versions of k3s, we will have to continue to ship it to support e.g. 1.24.1 to 1.24.3, which don't include it. |
everyone like docker, but everyone care when the time switch to contained. in my side project, I use nerdctl + contained in my k3s branch. many user complain why use nerdctl. they also like docker. this is true story. |
Anyone interested in this, please 👍🏻 the issue, and try out the linked PR to see if it meets your needs. |
@brandond once Derek's ADR reorg branch is merged, I think this is a perfect opportunity for an ADR to get public comment on |
ADR has been added to the PR. |
I blame Github more than anything for this, since it decided not to send me notifications or emails about... anything for a while, including PRs, so the fact that there was even an active base of people using/testing it that close to 1.24 was pretty surprising. I check it much more regularly now, and I'm not the only person (in theory). @Oats87 there was actually some work around CNI in 1.24 specifically last week. Mostly in better defaults so It's not really obvious to me whether this is still a concern on k3os/RancherOS anymore, but there's at least one The very first thing to do next (for 1.25, before, or whenever) is this, and I'd love to get more input about requirements. I can define requirements based loosely on what other CRIs do, but the input would be very welcomed. |
At least on the version of cri-dockerd we're about to ship, not using I will note that there are a couple extra commits from me on that branch; I'm not sure if any of them would be of any use to upstream? |
A little over a week ago, with Any and all patches are welcome :) The project was in a twilight zone for a little while, since upstream extracted it and handed it over, then decided to keep it in k8s anyway, so time was spent in significantly refactoring it to make more sense as a standalone repo (restructuring the modules, not importing huge portions of We're on a pretty regular release cadence now, the "chicken with its head cut off" reactive development to "upstream said they were going to remove this but actually didn't, so we need a fix" or "upstream changed this, so..." is over, and most of that was around "CNI by default". Nobody likes wrangling command line args, so adding a config file is a necessary change which makes everyone's lives easier, and Hyrum's Law makes it hard to know which flags people actually need until they're unceremoniously removed/changed and someone files an issue. I'm very much aiming for "less v0.x so the API may break" (the command-line args are more or less the API in this case) and some measure of "real" deprecation. cgroupsv2 are an obvious, and necessary target. User namespaces are something we'll need, but the story around that with upstream On the one hand, I've spent 15 years as an open source developer, and I 💯 understand that "we need this right now" or "we need to maintain our own fork so we can backport fixes into stable branches" is a simple fact of life. On the other hand, I've always been "upstream first", and I'd love to see |
Thanks for the commentary! I'll take a look at updating the embedded version for our next release cycle, and trying to push up anything that hasn't already been addressed. I think the major things were bumping the Kubernetes version and adding cgroupv2 support, but neither of those are huge lifts. The change to panic instead of fatal is mostly just so that we can rescue out of some (if not all) errors. |
As we're on this topic, thought I'd sneak in a question related to supporting alternative runtimes. I saw this article https://opni.io/v0.4/setup/gpu/#rke and it confirmed my fears that alternative runtimes are not supported with dockershim. Do you guys know if the cri-dockerd implementation supports them? I'm currently running k3s with |
Validated on master branch using commit id b14cabcEnvironment DetailsInfrastructure Node(s) CPU architecture, OS, and Version: Cluster Configuration: To validate:
Agent
Output:
Cluster Configuration:
|
Validated using rc v1.24.4-rc1+k3s1
|
@mateuszdrab |
I totally didn't realize how new support for alternative runtimes is in Docker. I could swear I tried to find a runtime flag a few weeks ago and couldn't, this explains it. To be honest I don't really have a use case to stick with dockershim... I need to try reconfiguring my setup to run without the flag, but I tested it on one of the hosts and had issues. In a way I don't want to run two instances of containerd, so I might need to configure K3s to use an external containerd socket. I'll test it out more when I have some more time... |
Is your feature request related to a problem? Please describe.
Bring back --docker
Describe the solution you'd like
I'd prefer k3s to vendor in cri-dockerd similar to all go stuff and run it when --docker is requested
Describe alternatives you've considered
Additional context
Docker is the number one container tool for development and I don't see that changing anytime soon. Running k3s with --docker gives you the very nice advantage of being able to run images from docker build with no push/pull cycle.
Backporting
The text was updated successfully, but these errors were encountered: