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

feat: add support for quadlet, secrets #78

Merged
merged 1 commit into from
Jul 19, 2023

Conversation

richm
Copy link
Contributor

@richm richm commented Jun 28, 2023

Feature: Add support for quadlets. User can pass in quadlet units using
podman_quadlet_units. Add support for secrets. User can pass in
Ansible Vault encrypted secrets using podman_secrets.

Reason: quadlets are the new way to implement applications in podman that
use systemd services. quadlets allow you to specify everything you need
to run your application - containers, services, volumes, networks, and
more - using simple, systemd style unit files. Secrets such as passwords,
tokens, keys, etc. are an important part of application configuration, so
the role now allows those to be specified.

Result: Users can deploy entire, complex applications using the podman
system role using quadlet units.

@richm
Copy link
Contributor Author

richm commented Jun 29, 2023

[citest]

1 similar comment
@richm
Copy link
Contributor Author

richm commented Jun 29, 2023

[citest]

Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

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

This is really great work, @richm !

@ygalblum PTAL

@@ -8,6 +8,10 @@ run `podman` containers.
## Requirements

The role requires podman version 4.2 or later.
The role requires podman version 4.4 or later for quadlet support and secret
support.
The role requires podman version 4.5 or later for support for using healthchecks
Copy link
Member

Choose a reason for hiding this comment

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

Nice catch!

README.md Outdated Show resolved Hide resolved
@richm
Copy link
Contributor Author

richm commented Jun 29, 2023

[citest]

@richm
Copy link
Contributor Author

richm commented Jun 29, 2023

[citest]

@richm
Copy link
Contributor Author

richm commented Jun 29, 2023

@vrothberg @ygalblum the quadlet demo is not working on EL8 - looks like podman there was recently updated to podman-3:4.5.1-5.module+el8.9.0+19106+3ac0000c.x86_64 - EL9 works fine with podman-2:4.5.1-4.el9.x86_64
Here is the error - from podman logs the-wordpress-container:

Warning: mysqli::mysqli(): php_network_getaddresses: getaddrinfo failed: Name or service not known in - on line 22
Warning: mysqli::mysqli(): (HY000/2002): php_network_getaddresses: getaddrinfo failed: Name or service not known in - on line 22
MySQL Connection Error: (2002) php_network_getaddresses: getaddrinfo failed: Name or service not known

all other pod logs/journalctl look fine

I'm willing to chalk this up to a bug that snuck into the podman update in EL8 that is hopefully known and will be fixed shortly

@vrothberg
Copy link
Member

@richm a bug with a reproducer would be great. Thank you!

README.md Outdated
from the file name given in `file`, `file_src`, or `template_src`.

By default, the files will be copied to or created in
`/etc/containers/systemd/$name.$type` on the managed node. You can provide a
Copy link

Choose a reason for hiding this comment

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

This path is correct for root. However, for users the location is $HOME/.config/containers/systemd/

Comment on lines 2 to 13
- name: Check if user is lingering
stat:
path: "/var/lib/systemd/linger/{{ __podman_user }}"
register: __podman_user_lingering
when: __podman_rootless | bool

- name: Enable lingering if needed
command: loginctl enable-linger {{ __podman_user }}
when:
- __podman_rootless | bool
- not __podman_user_lingering.stat.exists
changed_when: true
Copy link

Choose a reason for hiding this comment

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

Consider replacing into one step:

Suggested change
- name: Check if user is lingering
stat:
path: "/var/lib/systemd/linger/{{ __podman_user }}"
register: __podman_user_lingering
when: __podman_rootless | bool
- name: Enable lingering if needed
command: loginctl enable-linger {{ __podman_user }}
when:
- __podman_rootless | bool
- not __podman_user_lingering.stat.exists
changed_when: true
- name: Enable lingering for ansible_user
ansible.builtin.command: loginctl enable-linger {{ __podman_user }}
args:
creates: '/var/lib/systemd/linger/{{ __podman_user }}'
when: __podman_rootless | bool

owner: "{{ __podman_user }}"
group: "{{ __podman_group }}"
mode: "0755"
when: not __podman_quadlet_file is none
Copy link

Choose a reason for hiding this comment

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

What if it's an empty string (like you check in the following cases)?
In addition, can't you just do when: __podman_quadlet_file? AFAIK this covers all the false cases (none, empty string)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed all of these. I believe we had to use the in [none, ""] form with older versions of ansible and/or jinja. I think we don't need to use this anymore.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, there is a bit of a wrinkle - if you use when: __some_string_valued_variable, you will get the "bare variable" warning - https://github.com/redhat-cop/automation-good-practices/tree/main/coding_style#ansible-guidelines - the recommendation is to use when: __some_string_valued_variable | bool - this works fine if the variable is empty or none - however, when it is a non-zero length string, when: __some_string_valued_variable | bool will evaluate to false because it is doing some sort of string-to-bool conversion - so you must use when: __some_string_valued_variable | length > 0 to check for a non-empty string.

or __podman_template_file is changed
- __podman_activate_systemd_unit | bool

- name: Enable service # noqa no-handler
Copy link

Choose a reason for hiding this comment

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

Currently, you cannot enable services generated by Quadlet. The reason is that since the systemd unit file is a generated one, systemd considers it as transient and therefore does not allow enabling it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok - I commented this out so we can remember to revisit if/when enable service is supported

Copy link

Choose a reason for hiding this comment

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

OK

@@ -0,0 +1,13 @@
---
# Primarily for testing unreleased versions
- name: Enable podman copr on EL
Copy link

Choose a reason for hiding this comment

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

Did you consider using community.general.copr instead of command?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would prefer to keep external dependencies to the absolute minimum - it is problematic for downstream productization, especially unsupported community collections like community.general - also, this code is strictly for developer testing - not something that an end user should ever use, and will not be supported

Copy link

Choose a reason for hiding this comment

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

OK

- podman_version is version("4.4", "<")
- podman_fail_if_too_old | d(true)

- name: Podman package version must be 4.4 or later for quadlet, secrets
Copy link

Choose a reason for hiding this comment

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

Assuming I'm not misreading the when clause here, it is the same as the previous one. Now, since the previous one is a fail then the flow will never reach this stage. So, if you want the second one to happen, you should reorder them.
Another suggestion is to combine them using block so that you can write the when clause only once.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What I'm trying to do here is - if podman_fail_if_too_old is True, then the role will fail if podman is "too old". This is the default case, and one that production users will hit if they try to use newer features with an older version of podman. If podman_fail_if_too_old is False, then I want to just skip the rest of the play - this is useful for testing purposes, not something a production user would or should ever use.


- name: Check systemd
# noqa command-instead-of-module
shell: set -euo pipefail; systemctl list-units | grep quadlet
Copy link

Choose a reason for hiding this comment

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

Quadlet is a generator executed by systemd, it is not a unit and therefore this check actually fails.
This also raises the question of what it the purpose of of these steps if failed_when is set to false

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Quadlet is a generator executed by systemd, it is not a unit and therefore this check actually fails.

Hmm - well it does print something.

This also raises the question of what it the purpose of of these steps if failed_when is set to false

This isn't really an assertion test - it's really so I can see what's going on - I guess I should convert this into a "real" test, or just get rid of it, once the quadlet stuff is working and reviewed.

changed_when: false
when: __web_status is success

- name: Show errors
Copy link

Choose a reason for hiding this comment

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

Consider combining into block in order to check when only once

@richm
Copy link
Contributor Author

richm commented Jul 5, 2023

[citest]

1 similar comment
@richm
Copy link
Contributor Author

richm commented Jul 5, 2023

[citest]

@richm
Copy link
Contributor Author

richm commented Jul 5, 2023

@ygalblum looks like installing podman-plugins works on all platforms - ok to leave it in? Or will the podman package in rhel8 be fixed to require this?

@ygalblum
Copy link

ygalblum commented Jul 6, 2023

LGTM for the code.
As for podman-plugins, I think @Luap99 knows better than me

@Luap99
Copy link

Luap99 commented Jul 6, 2023

LGTM for the code. As for podman-plugins, I think @Luap99 knows better than me

I cannot comment on whenever RHEL 8 is supposed to install it by default or not. I think fedora had a recommends before we switched to netavark.

Also I have no idea what adding podman-plugins here does, is this only installed for CI testing or by default for all users? This package should no longer be installed by default on distros who ship netavark/aardvark-dns. And also this is not even a general package name by any means. Every distro decided to name the dnsname plugin package differently, if this role only targets fedora/centos/rhel it is fine but if you need support for other distros it would fail there.

@richm
Copy link
Contributor Author

richm commented Jul 6, 2023

LGTM for the code. As for podman-plugins, I think @Luap99 knows better than me

I cannot comment on whenever RHEL 8 is supposed to install it by default or not. I think fedora had a recommends before we switched to netavark.

Also I have no idea what adding podman-plugins here does, is this only installed for CI testing or by default for all users?

I am proposing that this will be the default for runtime - any user using this role will have podman-plugins installed. I suppose I could change it so that it only installs podman-plugins if using any of the quadlet features, but that seems unnecessarily complex.

This package should no longer be installed by default on distros who ship netavark/aardvark-dns. And also this is not even a general package name by any means. Every distro decided to name the dnsname plugin package differently, if this role only targets fedora/centos/rhel it is fine but if you need support for other distros it would fail there.

The problem is that the RHEL8 package podman-3:4.5.1-5.module+el8.9.0+19106+3ac0000c.x86_64 does not have a dependency on podman-plugins, and attempting to use quadlet networking without podman-plugins fails. The RHEL9, Fedora, and rhcontainer copr podman packages do not have this problem. I do not know if they have an explicit dependency on podman-plugins, or otherwise do not have this problem because DNS is handled in a different way on RHEL9 and later.

I am currently not worried about supporting platforms other than RedHat OS family, but if we do decide to support those platforms, we can easily do that, and have different packages for different distro/version combinations.

@Luap99
Copy link

Luap99 commented Jul 6, 2023

Can you make it conditional on a podman command output podman info --format {{.Host.NetworkBackend}}?
If this says CNI you will need podman-plugins installed for dns, if it says netavark you need aardvark-dns installed (that should be default on RHEL 9 newer).

I think it is worth to file a bz against RHEL 8 and ask for a recommends on podman-plugins there. I am not aware of any reason why it should not be installed.

@richm
Copy link
Contributor Author

richm commented Jul 6, 2023

Can you make it conditional on a podman command output podman info --format {{.Host.NetworkBackend}}? If this says CNI you will need podman-plugins installed for dns, if it says netavark you need aardvark-dns installed (that should be default on RHEL 9 newer).

Do we ask RHEL customer who want to use podman + quadlet to do this? If so, then I can add this to the role. If not, then I don't think the role should do this, it should be handled by a podman package dependency?

I think it is worth to file a bz against RHEL 8 and ask for a recommends

Why a recommends instead of a requires?

on podman-plugins there. I am not aware of any reason why it should not be installed.

I guess what I am asking is - is this a bug in the podman packaging on RHEL8? It seems like it to me, unless we are going to tell our RHEL8 podman users that they must first install podman-plugins if they want to use quadlet features?

@vrothberg
Copy link
Member

I guess what I am asking is - is this a bug in the podman packaging on RHEL8? It seems like it to me, unless we are going to tell our RHEL8 podman users that they must first install podman-plugins if they want to use quadlet features?

That seems like a fair expectation to me.

@Luap99
Copy link

Luap99 commented Jul 6, 2023

DNS is always optional, podman will work fine without it.
The only thing you are missing out is name resolution for container names. That is why I don't think it is required per definition, but many people likely prefer it so a recommends sounds good to me.

This has nothing to do with quadlet features at all. The quadlet example uses names to communicate that's all, a normal podman container may wish to do the same. I agree that this is a problem out of scope for this role. Either RHEL 8 says we install it by default or not. I don't see why this role specially has to enable that so that is way I suggest to create a bz were we can have this discussion.

@richm
Copy link
Contributor Author

richm commented Jul 6, 2023

@richm
Copy link
Contributor Author

richm commented Jul 6, 2023

[citest]

1 similar comment
@richm
Copy link
Contributor Author

richm commented Jul 6, 2023

[citest]

@richm
Copy link
Contributor Author

richm commented Jul 11, 2023

[citest]

@richm richm force-pushed the quadlet branch 3 times, most recently from fd15f7f to 19c05d7 Compare July 13, 2023 16:35
@TomSweeneyRedHat
Copy link

ETA for this? We'd hoped to have it delivered for July 17 if possible.

Feature: Add support for quadlets.  User can pass in quadlet units using
`podman_quadlet_units`.  Add support for secrets.  User can pass in
Ansible Vault encrypted secrets using `podman_secrets`.

Reason: quadlets are the new way to implement applications in podman that
use systemd services.  quadlets allow you to specify everything you need
to run your application - containers, services, volumes, networks, and
more - using simple, systemd style unit files.  Secrets such as passwords,
tokens, keys, etc. are an important part of application configuration, so
the role now allows those to be specified.

Result: Users can deploy entire, complex applications using the podman
system role using quadlet units.
@richm richm merged commit 0ef00f3 into linux-system-roles:main Jul 19, 2023
6 checks passed
@richm richm deleted the quadlet branch July 19, 2023 20:00
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.

None yet

5 participants