-
Notifications
You must be signed in to change notification settings - Fork 9
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 support for Podman pod feature and for RHEL/CentOS 8 #1
base: master
Are you sure you want to change the base?
Conversation
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.
Hey @jwhb thanks for the pull request.
If you want to split out the RedHat family support into a separate PR I'd be happy to merge that right away. I think we're going to have to go back and forth a bit on the --pod
support before I'd be happy merging it.
set_fact: | ||
podman_pods: "{{ podman_pods | default({}) | combine({service.pod.name: {'name': service.pod.name, 'ports': service.pod.ports | default([]) + (podman_pods[service.pod.name]['ports'] | default([]))}}) }}" | ||
|
||
- name: Delete pods with missing port bindings |
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.
The reason I never added pod support was because it would require some gnarly use of the shell module like this.
I keep waiting for upstream Ansible to a module like podman_pod
support but that may never happen.
I have two real requests when it comes to this:
-
What does
--pod
provide over--network
? -
If this is to get merged it will need a way to be idempotent. As it stands if I utilize the
pod
option for any of my services this will always reportchanged
. This breaks one of the core promises of Configuration Management.
Point number 2 is why I never did this, you can do some wicked things with args
and shell like:
shell: echo "something" > this_file
args:
creates: this_file
To make it "idempotent"-ish and I think I'd be willing to accept that but this is already a pretty gnarly script.
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 anticipated that it could be problematic to put both changes in one PR and to implement the pod feature in that way. For my use case (Apache Guacamole) I needed an automation to connect 3 containers and publish some of their ports. Neither using pods nor network was possible with the existing Ansible modules, so I picked the pod option.
I don't think we can change the pod feature to have the quality (i.e. idempotency) you require for this project. While we wait for a native Ansible module to cover this feature I'll keep the branch published in case somebody needs to configure pods.
I split the pod feature into #2. Also you can close this PR.
Thanks for your comments and excellent work!
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 had a similar problem when trying to deploy Nextcloud without pods I eventually gave up in the end and just exposed all the ports for everything and networked them the old-school way.
I think it's a very valid use case and I would love to see this feature through. With the new Ansible split into modules I'm not even sure where to report this upstream.
I also do not know if we can ship custom modules with roles on Galaxy if so that might be an option?
Perhaps an alternative is to do a little more abusing of the shell module. I've also attempted to make pods be SystemD template units in the past which almost worked but I didn't need pods bad enough to push it over the line.
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.
Shipping custom modules on Galaxy is made possible by Collections.
I just stumbled upon this collection on Galaxy: https://galaxy.ansible.com/containers/podman. The project provides additional modules for pods and networks. However it seems that it doesn't let you create networks.
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.
Perusing the source code I found https://github.com/containers/ansible-podman-collections/blob/master/plugins/modules/podman_network.py
Which does appear to let us create networks. I bet we could pull this as a dependency and make it work.
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.
Hi @chasinglogic @jwhb ,
I'm really interested in this feature. Do you think this is something you can work on ?
Best,
Jerome
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.
Hey @jrevillard,
I'm not sure when I can get to working on this
* Add apt resource recomended bey podman dev
The role currently uses
dnf
to install Podman tools. The packages are first available on Fedora 28 anddnf
was introduced to RHEL/CentOS with major version 8. Therefore we check against Fedora 28 and RHEL/CentOS 8 when including the dnf tasks.This PR also adds the ability to use the Podman Pod feature. The role user will want to put the containers created by this role (or rather systemd) into Podman Pods. These have to be created beforehand. I implemented task logic to first check for existing pods with the same name. If a pod matches and doesn't have the published ports as requested by the role variables the pod will be deleted.
The tasks will always try to create the pod with the specified ports, if it doesn't exist.
The containers will then be assigned to a pod using the
--pod
argument in the systemd unit.I had to use a different format for the published port mapping (dict vs. string) to properly detected existing pods with "outdated" port mappings.
I tested the role against CentOS 8 with different pod configurations (no pod assigned, pod assigned with no published ports, pod assigned with multiple ports).
What do you think about this contribution?