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 👍 podman/pkg/domain/infra/abi/quadlet.go Lines 626 to 637 in 5d12165 |
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 podman/pkg/domain/infra/abi/quadlet.go Lines 285 to 287 in af3ed8d 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 podman/pkg/systemd/quadlet/quadlet.go Lines 1580 to 1590 in 2cc3be7 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. |
|
[NON-BLOCKING] Packit jobs failed. @containers/packit-build please check. Everyone else, feel free to ignore. |
|
@axel7083 I had similar CI failures and it got solved by rebasing on the latest |
c026ffc to
8f0ef82
Compare
e2a13c5 to
2317792
Compare
|
|
||
| #### **--recursive** | ||
|
|
||
| When an application name is specified, you need to specify --recursive flag to remove all the Quadlets which belongs to that specific application. |
There was a problem hiding this comment.
This reads a bit confusing. Is this mandatory when an application name is specified, and does nothing otherwise? If so, do we need the dedicated argument? User intent in specifying an application seems pretty clear that they want everything gone
There was a problem hiding this comment.
I agree, this is probably an option that can be on by default if an application requires it it makes sense to have it on and documented
There was a problem hiding this comment.
Thank you for you feedback and for what it's worth I think that's confusing too.
The flag is required when the user removes an application (the folder, recursively). And it's not required when the user wants to delete a single quadlet (i.e. podman quadlet rm flask.container flask.volume works fine).
The rationale for requiring this flag in the case of an application is to ensure that the user understands the implications of the removal: rm -rf <app-folder>.
There was a problem hiding this comment.
Hm. As long as we have clear error messages telling folks to use --recursive when they try and remove an app, I can live with it.
There was a problem hiding this comment.
This is the current error:
$ podman quadlet rm foo
Error: unable to remove Quadlet: refusing to remove application "foo": recursive option is not setThere was a problem hiding this comment.
And I have simplified the documentation of the --recursive flag:
#### **--recursive**
Required when removing applications (default false)
inknos
left a comment
There was a problem hiding this comment.
what happens if you do
podman quadlet install --application nameofapp.container directory/ and you have installed nameofapp.container already?
I know that disambiguation is an issue we'll have to deal with eventually out of this PR, but it makes sense to me to at least check for valid application names that can collide with quadlet files. I could definitely see a user calling "container" their application and running into an unexpected collision.
|
|
||
| #### **--recursive** | ||
|
|
||
| When an application name is specified, you need to specify --recursive flag to remove all the Quadlets which belongs to that specific application. |
There was a problem hiding this comment.
I agree, this is probably an option that can be on by default if an application requires it it makes sense to have it on and documented
Do you mean we should avoid installing an application whose name would be a valid quadlet name? Or that we should check the names of the existing quadlets (not services) and avoid collisions? |
Avoid the application has a valid quadlet name, like it should not end with |
2317792 to
73ce7ff
Compare
I have added this condition to the function that validates application names and a new unit test as well. |
Fixes: containers#28118 Signed-off-by: axel7083 <42176370+axel7083@users.noreply.github.com>
73ce7ff to
6079157
Compare
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