-
Notifications
You must be signed in to change notification settings - Fork 5k
fix: Remove podman binary detection from podman-env command #21054
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
base: master
Are you sure you want to change the base?
fix: Remove podman binary detection from podman-env command #21054
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: elasticdotventures The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Welcome @elasticdotventures! |
Hi @elasticdotventures. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions 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. |
Can one of the admins verify this patch? |
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.
can you plz post before/after this PR in the description
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 don't think we should look for podman, or suggest to people to use the podman --remote
client. We should only give a path to a docker socket, for a docker client.
Podman Desktop does not use podman, and Podman Compose does not default to using podman-compose. Instead, they use the Docker API and docker-compose.
Using the podman-remote client, is what is causing these problems with using cri-o.
Error: unable to connect to Podman socket: server API version is too old. Client "4.0.0" server "3.4.4"
It is better to only use docker
as a legacy client, and minikube image
for the rest.
That is why it is called "compatibility socket", while the libpod client/connection keeps breaking the backwards API...
- Remove DockerBackendDetector and DetectDockerBackend function - Remove podman-specific logic from dockerEnvVars - Simplify docker-env to only handle Docker API compatibility
- Change podman-env to use Docker client against Podman's Docker-compatible socket - Remove SSH-based connectivity (Docker client doesn't support SSH keys) - Simplify podman-env to use standard Docker environment variables - Update tests to match new Docker API compatibility approach - Update documentation to clarify Docker client usage - Remove podman-specific test cases from docker-env_test.go This addresses the core review feedback about API compatibility issues and provides a cleaner separation between docker-env and podman-env.
8bfb441
to
2f07bfd
Compare
Remove unnecessary blank lines that were accidentally introduced when removing podman detection logic.
Extend podman-env compatibility to support both crio and docker container runtimes, providing users with more deployment flexibility while maintaining the core Docker API compatibility approach that eliminates API version conflicts. This change allows podman-env to work with: - Podman driver + crio runtime (original support) - Podman driver + docker runtime (new support) Both configurations use the same Docker API compatibility approach that was implemented to address reviewer feedback about SSH-based connectivity issues and API version mismatches.
Update: Added Docker Runtime SupportI've added an enhancement that extends podman-env compatibility to support both crio and docker container runtimes Changes in Latest Commit
New Supported Configurations
Both configurations use the same Docker API compatibility approach that eliminates the SSH-based connectivity issues and API version mismatches. TestingVerified that both runtime configurations work correctly:
|
Add practical example showing how to build images directly in minikube and deploy them to Kubernetes without needing a separate registry. This demonstrates the key value proposition of the Docker API compatibility approach.
@afbjorklund I just wanted to say thanks for really great advice & feedback. I also submitted a separate issue in CRIO (my attempt to isolate the reviews) |
I don't think that it should use the legacy tcp socket for Podman, if anything the ssh socket should be used for Docker... As far as I know there is no support for it upstream, and if there are issues with ssh it would be better to use local unix*. * probably tunneled over ssh, but anyway (like it is being done for Podman Desktop) Future versions of minikube should not even start the docker daemon or the podman service, by default. Instead they should be socket-activated on demand, and only containerd or crio needs to be running for CRI. But it is better to fix the functionality in Kubernetes does not need to use Docker at all, any more. |
@afbjorklund Thanks for the feedback. I've updated the PR to remove podman binary detection and version specificity as requested. Issue #9229 is separate work that shouldn't be muddled with this change. Hopefully this cleaner foundation makes that future SSH socket implementation a bit smaller. |
@elasticdotventures: Cannot trigger testing until a trusted user reviews the PR and leaves an In response to this:
Instructions 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. |
5b54d5b
to
e1e355c
Compare
Addresses feedback from reviewers by removing podman binary detection and clarifying the Docker client connectivity approach.
Key Changes
isPodmanAvailable()
function andwhich podman
checks per reviewer feedbackAddresses Review Feedback
Before/After
Before
After
Foundation for Issue #9229
This implementation provides a clean foundation for the future SSH socket approach described in issue #9229, but keeps the current changes minimal and focused. The SSH socket implementation (#9229) would be a separate, more substantial project that can build on this foundation.
Testing
This approach is more aligned with issue #9229's vision but represents a focused change that can be merged independently before tackling the larger SSH socket project.