Various quadlet updates #16237
Various quadlet updates #16237
Conversation
| #### `Network=` | ||
|
|
||
| Specify a custom network for the container. This has the same format as the `--network` option | ||
| to `podman run`. For example, use `host` to use the host network in the container, or `none` to | ||
| not set up networking in the container. |
There was a problem hiding this comment.
You can set --network multiple times when you specify network names. I think you should allow that too.
There was a problem hiding this comment.
Added in a separate commit
alexlarsson
force-pushed
the
quadlet-updates
branch
2 times, most recently
from
6510a06 to
7c4ce0d
Compare
|
Hmm, for some reason all the |
cmd/quadlet/main.go
Outdated
| if !ok { | ||
| return | ||
| } | ||
| if strings.Contains(imageName, "@") { |
There was a problem hiding this comment.
Use image/pkg/shortname IsShortName(imageName)
There was a problem hiding this comment.
That pulls in 47 megabytes of dependencies or something...
There was a problem hiding this comment.
Actually its not that bad, but it goes from 2.6BM to 3.1MB (stripped 1.8-2.21MB), and it pulls in things like containers/image/docker/reference with its 1.3 meg of regexps.
Also adds these new deps:
libsubid.so.4 => /lib64/libsubid.so.4 (0x00007fb97a1ae000)
libaudit.so.1 => /lib64/libaudit.so.1 (0x00007fb979fa8000)
libselinux.so.1 => /lib64/libselinux.so.1 (0x00007fb979f7b000)
libsemanage.so.2 => /lib64/libsemanage.so.2 (0x00007fb979f3a000)
libcrypt.so.2 => /lib64/libcrypt.so.2 (0x00007fb979efe000)
libacl.so.1 => /lib64/libacl.so.1 (0x00007fb979ef4000)
libattr.so.1 => /lib64/libattr.so.1 (0x00007fb979eec000)
libcap-ng.so.0 => /lib64/libcap-ng.so.0 (0x00007fb979ee2000)
libpcre2-8.so.0 => /lib64/libpcre2-8.so.0 (0x00007fb979e45000)
libsepol.so.2 => /lib64/libsepol.so.2 (0x00007fb979d8d000)
There was a problem hiding this comment.
Not to say any of this is horrible. But the generators are run during very early boot and block systemd from starting to read the unit files, so any slowness blocks the entire boot process.
There was a problem hiding this comment.
I’d prefer using docker/reference but I’m not going to get into a huge fight about this…
Way too often, such handcrafted parsers are not actually correct (a confusion about the three uses of : is very common). That’s, AFAICS, not the case here, but it’s just a bad habit to get into. “Always parse the data using the provided API, never deal with raw strings” is easier to enforce with fewer counterexamples in the codebase.
From a quick check it seems that something like distribution/distribution#3566 might bring us from 43 to 7 regexp compiles.
#16205 already talks about reducing the overhead of the reference parser even further. Would someone be willing to actually do that work?
Would one or two of the above be good enough?
The SELinux dependency is a bit surprising. How does that come about? Would moving (some variant of) IsShortName from c/image/pkg/shortnames to c/image/docker/reference, dropping things like c/storage/pkg/homedir (and the required rootless infrastructure), help?
The concept of short names is orthogonal to digested references. What is this function actually warning about? I.e. “ideally use a fully-qualified name” does not motivate users to change things unless it says why they should do that extra work.
There was a problem hiding this comment.
The SELinux dependency is a bit surprising. How does that come about? Would moving (some variant of) IsShortName from c/image/pkg/shortnames to c/image/docker/reference, dropping things like c/storage/pkg/homedir (and the required rootless infrastructure), help?
Yes, that would certainly help.
The concept of short names is orthogonal to digested references. What is this function actually warning about? I.e. “ideally use a fully-qualified name” does not motivate users to change things unless it says why they should do that extra work.
User may well just ignore the warning. I would go as far as throwing an error if we want to take the issue really serious.
There was a problem hiding this comment.
I'm generally in favour of using dependencies with tested code, but the generator code runs during very special circumstances, and affect system-wide boot performace heavily, so in this particular case I'm against it.
In particular because this is really only a warning. The initial reason for recommending the use of fqn is that using a shortname on rhel unnecessary parses 300k of shorname aliases, which adds > 60mesec to boot. But it is also generally wise to use a fqn to make sure you're running exactly the image you expected.
There was a problem hiding this comment.
I don't think we should fail for a shortname, that seems unnecessarily harsh. However, giving a warning for something that is potentially quite slow at least gives users the possibility of acting on this.
| @@ -128,13 +128,25 @@ setuid and file capabilities. | |||
|
|
|||
| Drop these capabilities from the default container capability set. The default is `all`, allowing | |||
| addition of capabilities with `AddCapability`. Set this to empty to drop no capabilities. | |||
There was a problem hiding this comment.
This is misleading, since I believe DropCapability="" will run with the Podman default capabilities as defined in containers.conf.
There was a problem hiding this comment.
Yeah, that is true. Will fix the wording.
There was a problem hiding this comment.
Added a commit fixing this
|
Ping? |
There was a problem hiding this comment.
The short-name parser seems to only be fed with localhost/imagename. If we decide going down this road, I suggest using the existing unit tests (see https://github.com/containers/image/blob/main/pkg/shortnames/shortnames_test.go#L14).
cmd/quadlet/main.go
Outdated
| if !ok { | ||
| return | ||
| } | ||
| if strings.Contains(imageName, "@") { |
There was a problem hiding this comment.
The SELinux dependency is a bit surprising. How does that come about? Would moving (some variant of) IsShortName from c/image/pkg/shortnames to c/image/docker/reference, dropping things like c/storage/pkg/homedir (and the required rootless infrastructure), help?
Yes, that would certainly help.
The concept of short names is orthogonal to digested references. What is this function actually warning about? I.e. “ideally use a fully-qualified name” does not motivate users to change things unless it says why they should do that extra work.
User may well just ignore the warning. I would go as far as throwing an error if we want to take the issue really serious.
|
Maybe I should move the shortname commit to a separate PR? That seems least important and most contentious. |
Sounds good 👍 |
We were looking at stdout, not stderr, and one of the testcases were wrong. Signed-off-by: Alexander Larsson <[email protected]>
Otherwise the noimage test doesn't look at the stderr assertion. Signed-off-by: Alexander Larsson <[email protected]>
This makees much more sense for typical service loads, and can easily be reverted by `ReadOnly=no`. Also updates and adds various tests for this. Signed-off-by: Alexander Larsson <[email protected]>
This is much better for the systemd case becase we pass the journal socket fds directly to the container. This means less copying of the logs, but it also means the journal will correctly get the peer process id when it tries to extract things like the name of what is logging something. With this we correctly name the logging process rather than claim everything comes from conmon. Signed-off-by: Alexander Larsson <[email protected]>
This was added in the old quadlet to work around issues with podman not passing on notify fds and pids. However, these are now fixed with: containers#11316 openSUSE/catatonit#15 So, remove this key (which was never in a podman release anyway) Signed-off-by: Alexander Larsson <[email protected]>
The binary name is not the same as in the old quadlet, and can anyway differ in system and user runs, so use os.Args[0] to get the right name in the comment. Signed-off-by: Alexander Larsson <[email protected]>
You can still use multiple lines, but this is not necessary. Signed-off-by: Alexander Larsson <[email protected]>
Signed-off-by: Alexander Larsson <[email protected]>
This lets you add custom device nodes into the container Signed-off-by: Alexander Larsson <[email protected]>
This just fixes the indentation which was previously breaking the list such that the various network modes were just mixed into one large paragraph instead of a list. Signed-off-by: Alexander Larsson <[email protected]>
This just gets translated to --network=... Signed-off-by: Alexander Larsson <[email protected]>
This is supported by podman run with --network, so makes sense. Signed-off-by: Alexander Larsson <[email protected]>
Signed-off-by: Alexander Larsson <[email protected]>
It was a bit unclear what setting it to empty means. Also, add to the tests verification that this works. Signed-off-by: Alexander Larsson <[email protected]>
alexlarsson
force-pushed
the
quadlet-updates
branch
from
9e19633 to
bac907a
Compare
|
Rebased without the shortname commit |
|
@containers/podman-maintainers PTAL |
|
Haunted by flakes. Restarted the jobs. |
|
@giuseppe PTAL |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alexlarsson, giuseppe The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
github-actions
bot
added
the
locked - please file new issue/PR
This adds various new features and fixes to quadlet compared to the C version:
Any changes in behaviour is fine as no previous version was released.