refactor: podman quadlet sub-command#28335
Conversation
Luap99
left a comment
There was a problem hiding this comment.
Only a short high level look I am definitely in favour of this directory approach.
Question do we care about backwards compatibility? We still have 3-4 weeks till podman6 so we can break if wanted. And given quadlet install is a rather new command maybe reworking that now without worrying about compatibility is the easiest.
cc @mheon
|
Ummm. Given how new these commands are, it would not be unprecedented to break folks, but it's still a very bad user experience... Our migration options are also not particularly pretty though. I suppose we could detect legacy project files and convert to a directory and remove the file on finding them? |
Convert when? "inside quadlet"? when running "podman quadlet list/install/remove"? For install/remove it is not clear to me we should migrate other files. And on list it seems strange to do a rewrite of files, i.e. there is no sane way to make this in a atomic fashion so we risk leaving the files in a bad state when getting killed. |
|
I also like using directories instead of the Service naming conflictsBut this introduces the possibility of having multiple services of the same name. So I suppose it should either check for naming conflicts, or make the application name part of the service name, so that multiple quadlets with the same application-local name can exist independently. Implicit merge of applicationsI am also thinking that maybe it would be slightly more convenient for the user if they were notified that they are adding quadlets to an existing application. Now it happens implicitly, so it can hypothetically happen that the user: |
df7607c to
42c6321
Compare
|
Also took a mostly high level look, it looks nice overall. It does not look like it's ready to be merged, there are TODOs. My previous comment should probably be addressed, as installing multiple quadlets with the same name breaks. Removing the whole application keeps non-quadlet files and the directory itself. The word |
42c6321 to
5d12165
Compare
|
Hey @simonbrauner !
I removed the TODOs
This is something already possible with the current, so not sure how this could been addressed? Maybe we need a dedicated issue to try to address that?
It does if there is no errors while removing each quadlet in the application, the directory will be removed 👍 |
5d12165 to
af3ed8d
Compare
|
[NON-BLOCKING] Packit jobs failed. @containers/packit-build please check. Everyone else, feel free to ignore. |
|
Hello @axel7083.
There is still one about validation of application name https://github.com/containers/podman/pull/28335/changes#diff-98a6e1ecc741f449262cca173c6e557c64dbf2706e5b062967b07b76018b62adR10
I would say this is introduced by the possibility of having multiple quadlets of the same name across different applications. With the original approach, installing multiple quadlets of the same name fails: So I am thinking, the two ideas would be:
But I am not saying this is something that has to be done as part of this PR, I was just thinking of the consequences of your changes and this is what I thought of.
👍 |
I will try to add this tomorrow morning 👍 |
|
@simonbrauner I reviewed the problem, and the current behavior that you are seeing is not preventing "duplicate" quadlets with the same service name, but rather preventing you to override, the logic is technically still here. If you try to The problem of duplicates service name is a bit more annoying to solve, let me explain. A service name is either define by its name or by its The quadlet systemd generator is reading and parsing the unit file, and lookup for the But for us this is a little bit a challenge, as when should we fails? If the user is installing So my question would be, when should we fails? If the user already have multiple quadlets with the same service name, if we try to install another should we fail? Does the quadlet generator should fail if multiple quadlets have the same service name? Or just a warning in the stderr? I think we can parse every quadlets we try to install and check that in the list of quadlets we try to install (when passing a directory or a tar) we could read and parse them, but what if they are invalid? currently I don't think we are checking if the quadlets files are valid 🤔 . Anyway, I think this is going a bit out of scope of this PR, and as it is already big enough, I think we may skip the service name issue, wdyt? |
2fd9762 to
97fcbb6
Compare
Yes, sure. Initially I thought that this is strange behavior introduced by this PR, but if it's just something that was there already (just less likely to happen), then I have nothing against considering it out of scope. |
97fcbb6 to
c026ffc
Compare
|
Cockpit tests failed for commit c026ffc. @martinpitt, @jelly, @mvollmer please check. |
It is technically possible to have the same file names in many search directories so I think having some way to say this application does make sense. I am not sure how people plan to use a application to me there are one unit, added/removed as one. I think the require --recursive and delete the whole app as one is simple and logical to document, adding special cases makes this more complicated than it needs to be I think. |
|
I don't fully agree, but it is a reasonable starting point. We can always add the additional edge cases of removing individual quadlets out of an app, and similar things, in future versions. |
|
I agree with @Luap99, removing an application's single quadlet introduces new problems, and we should avoid that. But $ podman quadlet rm --recursive app/backend.volume
Error: quadlet "backend.volume" is part of the application "app" and can't be individually removed. Remove the application instead. |
It depends on what an application actually is. If it's something atomic which does not make sense unless it's complete, then yes, allowing partial removal only introduces problems. If it's just a group of quadlets then partial removal makes sense to me and it has clear semantics. If a user truly wants to remove a quadlet then I think they should be allowed to do so, and it's their responsibility that it may cause unused directory/broken dependency.
I don't think this is that tedious to write. I think it would be common to remove more quadlets at once, but many applications at once? But yes, I didn't think about it and it surely is something to consider. |
I have ideas on how people should use applications (see compose files and kubernetes deployments) and it's not a group of independent services. No, that's not what an application should be. |
Then it's good that we are making it clearer now. I probably have this mindset from the current implementation because |
ddc0ecb to
f680fee
Compare
|
During last Thursday's community call, we decided that:
Code, docs and tests have been updated accordingly. |
simonbrauner
left a comment
There was a problem hiding this comment.
Found inconsistency in install output, other than that, LGTM
| installReport.QuadletErrors[toInstall] = err | ||
| continue | ||
| } | ||
| installReport.InstalledQuadlets[toInstall] = installedPath |
There was a problem hiding this comment.
$ tree postgres-17-test
postgres-17-test
├── a
│ ├── E.container
│ └── README.md
├── A.container
├── B.container
└── README.md
$ podman quadlet install postgres-17-test --application 3
/home/sbrauner/.config/containers/systemd/3/README.md
postgres-17-test/a
/home/sbrauner/.config/containers/systemd/3/A.container
/home/sbrauner/.config/containers/systemd/3/B.container
The output of install does not seem to be recursive, it prints a folder name, in a different format even. I would expect:
/home/sbrauner/.config/containers/systemd/3/README.md
/home/sbrauner/.config/containers/systemd/3/A.container
/home/sbrauner/.config/containers/systemd/3/B.container
/home/sbrauner/.config/containers/systemd/3/a/E.container
/home/sbrauner/.config/containers/systemd/3/a/README.mdI'd propose that the installlQuadlet function could return a list of paths and collect them recursively, so that the output lists all quadlets, not only those that are in the topmost directory, to make it consistent with rm.
There was a problem hiding this comment.
Thanks for the review. This is indeed a bug. The output of the command doesn't include the nested files.
I had a look at the problem, and the fix isn't trivial. Considering that this is not critical, I would rather address it in a separate PR.
There was a problem hiding this comment.
I have opened a draft PR with the fix for this issue #28860
f680fee to
9911b41
Compare
Fixes: podman-container-tools#28118 Signed-off-by: axel7083 <42176370+axel7083@users.noreply.github.com>
9911b41 to
496646f
Compare
mheon
left a comment
There was a problem hiding this comment.
LGTM. Let's get this in so we have it in RC1.
Cleanup the code to install quadlets and fix the output of the command `podman quadlet install` when a folder is provided as argument. See the comment: podman-container-tools#28335 (comment) Signed-off-by: Mario Loriedo <mario.loriedo@gmail.com>
Cleanup the code to install quadlets and fix the output of the command `podman quadlet install` when a folder is provided as argument. See the comment: podman-container-tools#28335 (comment) Signed-off-by: Mario Loriedo <mario.loriedo@gmail.com>
- Cleanup the code to install quadlets - Fix `podman quadlet install` output message (see podman-container-tools#28335 (comment)) - Update libpod quadlet endpoint documentation Signed-off-by: Mario Loriedo <mario.loriedo@gmail.com>
- Cleanup the code to install quadlets - Fix `podman quadlet install` output message (see podman-container-tools#28335 (comment)) - Update libpod quadlet endpoint documentation Signed-off-by: Mario Loriedo <mario.loriedo@gmail.com>
- Cleanup the code to install quadlets - Fix `podman quadlet install` output message (see podman-container-tools#28335 (comment)) - Update libpod quadlet endpoint documentation Signed-off-by: Mario Loriedo <mario.loriedo@gmail.com>
- Cleanup the code to install quadlets - Fix `podman quadlet install` output message (see podman-container-tools#28335 (comment)) - Update libpod quadlet endpoint documentation Signed-off-by: Mario Loriedo <mario.loriedo@gmail.com>
- Cleanup the code to install quadlets - Fix `podman quadlet install` output message (see podman-container-tools#28335 (comment)) - Update libpod quadlet endpoint documentation Signed-off-by: Mario Loriedo <mario.loriedo@gmail.com>
- Cleanup the code to install quadlets - Fix `podman quadlet install` output message (see podman-container-tools#28335 (comment)) - Update libpod quadlet endpoint documentation Signed-off-by: Mario Loriedo <mario.loriedo@gmail.com>
|
Thanks for the directory-as-application refactor — dropping the marker files is a genuine simplification. Adding a feature suggestion for consideration: the "first directory = application" rule doesn't cover grouping apps under a parent directory (by team, owner or tenant): Here the natural application is each leaf ( Could an opt-in to set the application boundary (e.g. an install-time flag, or a small per-directory hint, rather than reintroducing Generated with Claude Opus 4.8. |
Checklist
Ensure you have completed the following checklist for your pull request to be reviewed:
commits. (
git commit -s). (If needed, usegit commit -s --amend). The author email must matchthe sign-off email address. See CONTRIBUTING.md
for more information.
Fixes: #00000in commit message (if applicable)make validatepr(format/lint checks)Noneif no user-facing changes)Does this PR introduce a user-facing change?
While working on #28117 and the comment from @Luap99 (#28117 (comment)) I had a lot of issues working around the
.appand.assetfile, a suggestion made was to get rid of those and consider an application as a folder.Here is a POC / proposal of how it could works
podman quadlet installWhen trying to install from a directory, you will need to specify
--applicationas we want to avoid dumping all the content of the directory at the root.Installing from a directory (support recursive)
$: podman quadlet install --application flask-redis ./flask-redis $: ree /home/axel7083/.config/containers/systemd /home/axel7083/.config/containers/systemd └── flask-redis ├── app │ ├── app.py │ ├── Containerfile │ └── requirements.txt ├── flask.kube ├── play.yaml └── README.mdpodman quadlet ls --format=jsonFollowing
flask-redisinstalled above we getNow let's add an
nginx.imagepodman quadlet rmI added a
--recursiveoption, the rm support two input, a quadlet file (E.g.foo.image) or an application name. However when trying t o delete an application it will throw an error$: /bin/podman quadlet rm flask-redis Error: unable to remove Quadlet: cannot find application "flask-redis"You need to specify the
--recursiveflag to delete the quadlets in the application