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

Use socat while dial-stdio is not in VM image #12058

Closed
wants to merge 1 commit into from

Conversation

matejvasek
Copy link
Contributor

The podman version in VM doesn't contain patch with dial-stdio yet.
To workaround we can use socat.

Once podman containing dial-stdio is in VM image we can revert this.

The podman version in VM doesn't contain patch with dial-stdio yet.
To workaround we can use socat.

[NO TESTS NEEDED]

Signed-off-by: Matej Vasek <[email protected]>
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 21, 2021

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: matejvasek
To complete the pull request process, please assign saschagrunert after the PR has been reviewed.
You can assign the PR to them by writing /assign @saschagrunert in a comment when ready.

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@matejvasek
Copy link
Contributor Author

I don't know how release cycle of Fedora Core VM works. I guess that podman in it is one or two release behind. This PR helps fix docker CLI issues little bit sooner before #11819 will be part of VM.

@matejvasek matejvasek changed the title hack: use socat while dial-stdio is not in image Use socat while dial-stdio is not in VM image Oct 21, 2021
@afbjorklund
Copy link
Contributor

It worked with the earlier releases, since it had the real docker. It was broken in b3307bc, that replaced it with podman

/usr/local/bin/docker -> /usr/bin/podman

@matejvasek
Copy link
Contributor Author

It worked with the earlier releases, since it had the real docker. It was broken in b3307bc, that replaced it with podman

/usr/local/bin/docker -> /usr/bin/podman

@afbjorklund I didn't noticed when this changed. The podman system dial-stdio was merged only recently. Its advantage is that it should dial correct socket depending on user. Original docker CLI could do that too but you need set the DOCKER_HOST environment variable for user (like #12066).

@matejvasek
Copy link
Contributor Author

matejvasek commented Oct 24, 2021

So this PR should only bridge time between next release of podman and the release of fedora core image which would contain that podman. I think this should be fixed ASAP so people trying to switch to podman from docker desktop are not discouraged by the fact that docker CLI is not working.

@afbjorklund
Copy link
Contributor

afbjorklund commented Oct 25, 2021

I think this should be fixed ASAP

Probably needs to be backported to 3.4, then a package release and then two weeks waiting ?

But I think people trying to use podman machine are used to breakages after the JSON thing.

Before podman dial-stdio, this was.

@matejvasek
Copy link
Contributor Author

matejvasek commented Oct 25, 2021

Probably needs to be backported to 3.4, then a package release and then two weeks waiting ?

I guess it will be in 3.4.2 and then latter in fedora core I don't know what's the delay.
Maybe two weeks? But I think even two weeks are worth.

But I think people trying to use podman machine are used to breakages after the JSON thing.

I thought that had been fixed, wasn't it?

@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 28, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 28, 2021

@matejvasek: PR needs rebase.

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/test-infra repository.

@rhatdan
Copy link
Member

rhatdan commented Nov 2, 2021

I think I would rather just add the podman containing dial-stdio to the next podman 3.4.3 release, then add something that needs to be removed. Since this is a hidden feature, I don't see this as a big change.

@rhatdan
Copy link
Member

rhatdan commented Nov 2, 2021

@mheon WDYT?

@mheon
Copy link
Member

mheon commented Nov 2, 2021

I don't think we can, we're feature-frozen as of now and this is definitely a new feature.

@rhatdan
Copy link
Member

rhatdan commented Nov 2, 2021

Then let's cut a new release with just this feature, for the MAC. 3.5.

@github-actions
Copy link

github-actions bot commented Dec 3, 2021

A friendly reminder that this PR had no activity for 30 days.

@rhatdan
Copy link
Member

rhatdan commented Dec 3, 2021

@matejvasek are you still working on this?

@rhatdan rhatdan removed the stale-pr label Dec 3, 2021
@matejvasek
Copy link
Contributor Author

@rhatdan let me check current VM img, it may or may not be required anymore.

@matejvasek
Copy link
Contributor Author

matejvasek commented Dec 13, 2021

@rhatdan it looks like the dial-stdio (#11819) still hasn't been merged into some release, so this addition might be worthwhile.

@afbjorklund
Copy link
Contributor

afbjorklund commented Dec 13, 2021

It wasn't going to be added until 3.5, and VM has 3.4.2

There also is the issue of host keys (with Docker), so the workaround is tunneling the Unix socket... Also fixes those API clients that don't support ssh://

(Podman ignores ssh host keys, but Docker does not)

@github-actions
Copy link

A friendly reminder that this PR had no activity for 30 days.

@rhatdan
Copy link
Member

rhatdan commented Jan 13, 2022

@afbjorklund @matejvasek Should be rebased or closed?

@matejvasek matejvasek closed this Jan 13, 2022
@afbjorklund
Copy link
Contributor

I think unix:// tunneling is needed, the docker ssh:// support is not up to par.

Also it seemed like temporary hack, and one that was avoided intentionally by docker.

@matejvasek
Copy link
Contributor Author

I wonder why #11819 was added to podman 4.X and not packported to 3.X.
It wasn't breaking any compatibility was it?

@mheon
Copy link
Member

mheon commented Jan 28, 2022

It's a new feature (new command), and we generally don't backport features

@afbjorklund
Copy link
Contributor

It was going to be in podman 3.5, but I don't think it was important enough ?

@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 21, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. stale-pr
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants