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

Add podman remote support #61

Merged
merged 13 commits into from
May 4, 2024
Merged

Add podman remote support #61

merged 13 commits into from
May 4, 2024

Conversation

ewchong
Copy link
Contributor

@ewchong ewchong commented Apr 25, 2024

Close #27

Changes introduced with this PR

  • Add support for running remote podman commands using --connection

By contributing to this repository, I agree to the contribution guidelines.

connector.go Outdated Show resolved Hide resolved
internal/cliwrapper/cliwrapper.go Outdated Show resolved Hide resolved
internal/cliwrapper/cliwrapper.go Outdated Show resolved Hide resolved
internal/cliwrapper/cliwrapper.go Outdated Show resolved Hide resolved
@ewchong
Copy link
Contributor Author

ewchong commented Apr 25, 2024

I do see a weird behavior that seems podman related when running more complex workflows on a local VM such as serial-wait_for. The behavior is the same if I set the default podman connection to the VM and use Arcaflow without the new argument.

2024-04-25T15:16:39-04:00	warning	source=plugin-provider	Plugin step example/hello-world deploy failed. error while determining if image exists. Stdout: '', Stderr: 'Cannot connect to Podman. Please verify your connection to the Linux system using `podman system connection list`, or try `podman machine init` and `podman machine start` to manage a new Linux VM
Error: unable to connect to Podman socket: failed to connect: ssh: handshake failed: write tcp 127.0.0.1:64464->127.0.0.1:2222: write: broken pipe
', Cmd error: (exit status 125)

This doesn't seem reproduce when using an actual baremetal remote machine.

@ewchong ewchong marked this pull request as ready for review April 25, 2024 20:32
Copy link
Contributor

@webbnh webbnh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Ed for bringing this forward.

Before we merge this, I think the invocation of the Podman command needs to be further encapsulated (and I'll work up a PR for your branch for that).

Also, I think we should try to come up with a way to test this new code with something other than an empty value. (I'll ponder that, but please do as well.)

internal/cliwrapper/cliwrapper.go Outdated Show resolved Hide resolved
internal/cliwrapper/cliwrapper.go Outdated Show resolved Hide resolved
internal/cliwrapper/cliwrapper.go Outdated Show resolved Hide resolved
internal/cliwrapper/cliwrapper_test.go Outdated Show resolved Hide resolved
webbnh

This comment was marked as outdated.

@webbnh

This comment was marked as outdated.

webbnh

This comment was marked as outdated.

Copy link
Contributor

@webbnh webbnh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good, now, Ed (if I do say so myself 😉) -- the tests are all passing and your additions are nicely localized.

However, speaking of tests, is there some way that we can set up a context for the local host so that we can test the changes you've made here with an actual --connection in the invocation?

internal/cliwrapper/cliwrapper.go Outdated Show resolved Hide resolved
@ewchong ewchong marked this pull request as draft May 1, 2024 02:10
@ewchong ewchong marked this pull request as ready for review May 2, 2024 01:42
Copy link
Contributor

@webbnh webbnh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ed, this looks great! Thanks again for doing this.

However, I think the test cleanup should be made more rigorous (I sent you a PR (ewchong#2) for your branch), there are some lint complaints, and I have another suggestion (along with a few nits).

factory.go Outdated Show resolved Hide resolved
internal/cliwrapper/cliwrapper.go Outdated Show resolved Hide resolved
internal/cliwrapper/cliwrapper_test.go Outdated Show resolved Hide resolved
internal/cliwrapper/cliwrapper_test.go Outdated Show resolved Hide resolved
internal/cliwrapper/cliwrapper_test.go Outdated Show resolved Hide resolved
internal/cliwrapper/cliwrapper_test.go Outdated Show resolved Hide resolved
internal/cliwrapper/cliwrapper_test.go Outdated Show resolved Hide resolved
internal/cliwrapper/cliwrapper_test.go Outdated Show resolved Hide resolved
Use cleanup handlers to tear down the temporary Podman API service.
Pass the connection via a pointer to a string to make handling
  null values more graceful.
Tweak some names and text.
Pick lint.
internal/cliwrapper/cliwrapper_test.go Outdated Show resolved Hide resolved
internal/cliwrapper/cliwrapper_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

@webbnh webbnh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Thanks, again, Ed.

Copy link
Contributor

@jaredoconnell jaredoconnell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find the specifier ConnectionName a little confusing, as it sounds more like an address to me, but other than that, looks good.

@mfleader mfleader self-requested a review May 3, 2024 17:19
@ewchong
Copy link
Contributor Author

ewchong commented May 3, 2024

I find the specifier ConnectionName a little confusing, as it sounds more like an address to me, but other than that, looks good.

What you are referring to in Podman is called URI. In this case the ConnectionName represents the name column of the following table:

podman system connection list
Name                         URI                                                         Identity                                     Default     ReadWrite
podman-machine-default       ssh://core@localhost:55635/run/user/501/podman/podman.sock  /Users/cloud171/.ssh/podman-machine-default  true        false
podman-machine-default-root  ssh://root@localhost:55635/run/podman/podman.sock           /Users/cloud171/.ssh/podman-machine-default  false       false

The plugin itself doesn't do anything with the URI and has to be setup by the user beforehand.

@jaredoconnell
Copy link
Contributor

I find the specifier ConnectionName a little confusing, as it sounds more like an address to me, but other than that, looks good.

What you are referring to in Podman is called URI. In this case the ConnectionName represents the name column of the following table:

podman system connection list
Name                         URI                                                         Identity                                     Default     ReadWrite
podman-machine-default       ssh://core@localhost:55635/run/user/501/podman/podman.sock  /Users/cloud171/.ssh/podman-machine-default  true        false
podman-machine-default-root  ssh://root@localhost:55635/run/podman/podman.sock           /Users/cloud171/.ssh/podman-machine-default  false       false

The plugin itself doesn't do anything with the URI and has to be setup by the user beforehand.

Thanks for clarifying. Unfortunately that requires pre-setup before the workflow is run, but that's okay.

@webbnh webbnh merged commit a2333c1 into arcalot:main May 4, 2024
2 checks passed
@webbnh
Copy link
Contributor

webbnh commented May 4, 2024

Unfortunately that requires pre-setup before the workflow is run, but that's okay.

We should be able to build on this functionality to add the ability for the workflow to set the URI.

It is pretty much always going to be the case that the Podman listener has to be setup independent of the workflow, so some pre-setup will generally be required in order to use this feature, and I think that's totally reasonable. However, I can see how it might be awkward to configure the Podman connection outside of the workflow (depending on where and how the workflow is being run), in which case it would be handy to be able to provide it via the workflow. (And, I don't think it would be hard.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for podman --remote
4 participants