Support docker-env with cri-o runtime#22547
Support docker-env with cri-o runtime#22547afbjorklund wants to merge 1 commit intokubernetes:masterfrom
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: afbjorklund The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
This comment was marked as duplicate.
This comment was marked as duplicate.
There was a problem hiding this comment.
Pull request overview
This PR adds support for using docker-env command with the cri-o runtime, mirroring the existing experimental support for containerd. The implementation uses podman (via podman.socket) for cri-o, similar to how nerdctl is used for containerd, both utilizing SSH sockets for communication.
Changes:
- Added
startPodmandfunction to enable podman.socket and configure DOCKER_HOST for cri-o runtime - Extended
docker-envcommand to support cri-o runtime with experimental warning - Removed overly restrictive runtime validation that prevented non-docker/containerd runtimes
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| cmd/minikube/cmd/start.go | Added startPodmand function to start podman.socket and configure environment for cri-o runtime support |
| cmd/minikube/cmd/docker-env.go | Extended runtime support to include cri-o, added conditional logic to call startPodmand for cri-o, and removed restrictive validation |
Comments suppressed due to low confidence (1)
cmd/minikube/cmd/docker-env.go:685
- The removal of runtime validation for docker and containerd needs to be replaced with appropriate validation. While the function now checks for containerd with Docker driver compatibility at line 682-683, there's no equivalent driver validation for CRIO runtime. According to the PR description and the similar containerd implementation, CRIO should also only be supported with the Docker driver. Consider adding a similar check for CRIO.
func dockerEnvSupported(containerRuntime, driverName string) error {
// we only support containerd-env on the Docker driver
if containerRuntime == constants.Containerd && driverName != driver.Docker {
return fmt.Errorf("the docker-env command only supports the containerd runtime with the docker driver")
}
return nil
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
124dcfd to
cebcabb
Compare
cebcabb to
84f0cc1
Compare
|
Aargh, yet another runtime-specific hack: // TODO: refactor to work with docker, temp fix to resolve regression
if cr == constants.Containerd {
// eventually, run something similar to ssh --append-known
appendKnownHelper(nodeName, true)
} |
84f0cc1 to
0248b15
Compare
It is very similar to containerd, but with podman for cri-o instead of nerdctl for containerd. Both using ssh sockets.
0248b15 to
c3edc50
Compare
|
@afbjorklund: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
|
||
| // set up environment variable on remote machine. docker client uses 'non-login & non-interactive shell' therefore the only way is to modify .bashrc file of user 'docker' | ||
| // insert this at 4th line | ||
| envSetupCommand := exec.Command("/bin/bash", "-c", "sed -i '4i export DOCKER_HOST=unix:///run/podman/podman.sock' .bashrc") |
There was a problem hiding this comment.
@afbjorklund This line always adds export DOCKER_HOST=unix:///run/podman/podman.sock in the .bashrc file everytime when minikube docker-env gets executed. Can we check by first by echo $DOCKER_HOST
There was a problem hiding this comment.
Yeah. as I mentioned it was just copied. Probably would be a good thing to refactor these helpers

It is very similar to containerd, but with podman for cri-o instead of nerdctl for containerd. Both using ssh sockets.
There are plenty of refactoring opportunities (this is just copy/paste), for merging the nerdctl and podman support.
This will use BuildKit in a podman container, by default:
To switch to the legacy buildah, use DOCKER_BUILDKIT=0: