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 Quadlet support for Podman #722

Merged
merged 1 commit into from
Apr 21, 2024
Merged

Conversation

sshnaidm
Copy link
Member

Fix #671
Signed-off-by: Sagi Shnaidman sshnaidm@redhat.com

@sshnaidm sshnaidm force-pushed the quadlet branch 11 times, most recently from db2b584 to 1bcf5be Compare March 3, 2024 23:23
@markstos
Copy link

markstos commented Mar 4, 2024

I want comment just on the "quadlet" naming here.

Quadlet made sense as a code name as an external project, but now that it merged into Podman and it is the recommended way to integrate with systemd, the branding is counterproductive unnecessary to expose at the Ansible level.

The real beauty of of Quadlet is that there is hardly anything new to learn at all. If you know Podman and Systemd, there aren't really new concepts to learn. Yet Quadlet sounds like yet-another concept that needs to be learned to use Podman and Systemd together.

The name "Quadlet" appears to be mash-up of the word Quad, meaning "four" and "let" meaning "small", like a piglet is small pig or a parklet is small park, yet what it does has nothing to do with a "small four" as far as I can tell.

It's also confusing because there are no files related to Podman named Quadlet, no directories named quadlet, no directives named Quadlet-- it's just an abstract concept describing systemd integration.

The Podman networking features aren't called "octolet", they are described as Podman's networking features. There's no reason to add an extra label to these set of features either.

Let's dig into some specifics in this PR.

This adds a quadlet as container state:

'absent', 'present', 'stopped', 'started', 'created', 'quadlet'

I presume what this is trying to express is that the state of container is that it's managed by systemd through a file under /etc/containers/systemd?

That's a bit strange, because systemd services can themselves be present/absent/stopped/started as well. If there's not a clearer way to express this, I would support systemd as the name here.

quadlet_file_path=dict(type='path'),
quadlet_options=dict(type='list', elements='str'),

Suggested:

  • systemd_file_path
  • systemd_options

quadlet_file_path is required for Quadlet file generation

Suggest: systemd_file_path is required for systemd file generation.

In summary: I think this would be clearer if the term "systemd" was used everywhere that "quadlet" is used.

@nishipy
Copy link
Contributor

nishipy commented Mar 5, 2024

In my opinion, it looks clear to use the term quadlet here. As far as I know, some official docs and release notes use words including Quadlet, e.g. the terms like Podman Quadlet, Quadlet file, Quadlet unit file appear in https://docs.podman.io/en/latest/markdown/podman-systemd.unit.5.html. Podman also has env variables like QUADLET_UNIT_DIRS. In addition, state: systemd is confusing for me to distinguish from podman generate-systemd. Looking into the source code of Podman, we can find a directory or file named quadlet as of now.
Technically speaking, Quadlet is a systemd generator, and we will create qudlet unit files (this term can be found here) that Quadlet can translate into units with this module. So state: quadlet_unit would be more precise, but state: quadlet looks good as well.

@sshnaidm
Copy link
Member Author

sshnaidm commented Mar 5, 2024

@markstos The "Quadlet" file is not a Systemd unit file yet. The unit file is generated by running podman-systemd-generator or systemd reload.
As @nishipy mentioned the problem is that we have already systemd generation and it's going to be deprecated in favor of quadlets, but still active and we need to support version 4 and probably 3. So we'll need both systemd and quadlet generations to work in the same time.

@markstos
Copy link

markstos commented Mar 5, 2024

I understand that .container files are used to generate other systemd files. But it looks like a duck and quacks like a duck, and the old duck being deprecated, so I suggest it's called a duck.

@sshnaidm
Copy link
Member Author

sshnaidm commented Mar 6, 2024

I think we are going to leave it as quadlet now with keeping old (but not removed) systemd generaton finctionality. When systemd file generation will be deprecated finally and removed from Podman, we can do alias to systemd from quadlet.

@sshnaidm sshnaidm force-pushed the quadlet branch 5 times, most recently from 2713d76 to 51bab1e Compare March 23, 2024 11:37
@sshnaidm sshnaidm changed the title WIP add Quadlet support for Podman Add Quadlet support for Podman Mar 23, 2024
@sshnaidm sshnaidm force-pushed the quadlet branch 2 times, most recently from 94ebbed to d4853b2 Compare March 23, 2024 11:58
Copy link
Contributor

@nishipy nishipy left a comment

Choose a reason for hiding this comment

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

Leave a comment as follows

@sshnaidm sshnaidm force-pushed the quadlet branch 3 times, most recently from 3a0ad23 to 07e567a Compare April 12, 2024 17:21
@sshnaidm
Copy link
Member Author

I left optional quadlet_file (mandatory in case of kube) so that people can make custom service name.
quadlet_dir is by default one of root/non-root directories and also can be set.

@sshnaidm
Copy link
Member Author

@nishipy @alorle and all interested parties, please review, play with it and if no objections we'll merge this week.

Copy link
Contributor

@nishipy nishipy left a comment

Choose a reason for hiding this comment

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

@sshnaidm Thank you very much for your hard work! Leave a small comment.
Overall, it looks good to me. I understand that tests for now don't contain steps of systemctl --daemon-reload and checking if containers/pods/images/networks/volumes are created successfully, which might be added after further development.

plugins/modules/podman_image.py Show resolved Hide resolved
Signed-off-by: Sagi Shnaidman <sshnaidm@redhat.com>
@sshnaidm
Copy link
Member Author

Yes, it can be added in followups.

Copy link

@alorle alorle left a comment

Choose a reason for hiding this comment

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

Updated to the latest changes and all is working perfectly. I have tested continers, networks and volumes. Thanks for the hard work @sshnaidm

@sshnaidm
Copy link
Member Author

Ok, let's run with it.

@sshnaidm sshnaidm merged commit 1476ebe into containers:master Apr 21, 2024
36 checks passed
@sshnaidm sshnaidm deleted the quadlet branch April 21, 2024 22:13
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 new modules/options for Quadlet
4 participants