-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
feedback on new podman module capability #20000
Comments
Thanks for the feedback, @lastephey ! I just returned from PTO but will go through the items before the meeting on Thursday. |
Specifiying Env Variables
It can be specified only once as an array. A |
Inject Host Env Variables
Currently, there is only the AFAIUI, you are looking for a means to selectively inject a host env variales. |
NOTE: we may need to break this one big issue into multiple smaller ones to be able to track them correctly. Let's continue the conversation for now. |
Preserve Symlinks in Mounts
If the destination of a symlink is copied along with the mount, then symlinks are being preserved, see:
@lastephey, can you elaborate more on how the nature of the link? |
--privileged field in containers.conf
That does not exist at the moment, but we could add it. However, |
--env-merge field in containers.conf
No, that is not supported, so I think it's a good candidate to break it out. Can you elaborate on appending to |
That is exactly how we want it to be used. One thing I just noticed is that
|
One more issue I found while playing: |
The --module can only be parsed on the root level. It cannot work on the command level, because it must be "manually" parsed on init() to make sure the specified configuration files/modules are loaded prior to parsing the flags via Cobra. Hence move --module from the "persistent" to the "local" flags which will yield an error instead of doing nothing when being specified on the command level: ``` $ ./bin/podman run --module=foo.conf --rm alpine Error: unknown flag: --module See 'podman run --help' ``` Reported in containers#20000. Signed-off-by: Valentin Rothberg <[email protected]>
Opened #20014 to fix that. |
As found while working on containers#20000, the `--env-host` flag should use the default from containers.conf. Add a new "supported fields" test to the system tests to make sure we have a goto test for catching such regressions. I suspect more flags to not use the defaults from containers.conf. Signed-off-by: Valentin Rothberg <[email protected]>
Thanks for all your comments @vrothberg. Sorry for the info dump- I'm happy to file separate feature requests to keep this more organized. I'll try to reply to your questions today or tomorrow. |
Inject Host Env Variables
Yes, it would be nice to choose a specific host variable to inject (or a few variables, like you have demonstrated). Is that appropriate for a feature request? |
--env-merge field in containers.conf
We would like to use it to prepend the path to an injected MPI library, for example. A user might already have a path
|
--privileged field in containers.conf
We can figure out which settings we really need. |
Preserve Symlinks in MountsHere's an example. I'm using this line in my module file to do the
Here are our target libraries on the host system:
Here are the mounted libraries inside the container running with my gpu module:
We'd like the existing symlinks to be preserved. |
@scanon has some additional feedback about stacking modules and making sure they don't overwrite each other (i.e. environment variable settings). He can give you more details tomorrow. Thanks again for looking into this so far @vrothberg! 🙏 |
How about something like:
|
I think having a |
I had a look at it and I don't see a way to achieve it, the kernel always follows the symlinks for the mount source so there is no way to bind mount a symlink. What we could do is to "copy" these symlinks and create them manually in the container rootfs |
I created a separate issue for the symlink issue: #20098 |
As discussed in containers/podman#20000, we need an opt-in mechanism to _append_ string arrays during loading sequence of containers.conf files. At the moment, existing fields/data will be overriden with each loaded config that sets the specified field/option. The TOML (toml.io) config format does not allow for attributing fields and structs are implicitly represented as "tables". I wanted to extend a string array with a simple boolean field, for instance: ```TOML env=["FOO=bar"] env.append=true ``` TOML doesn't suppor tthe upper idea as it's not a properly formatted table. So I looked for alternatives and found that TOML supports so-called "mixed-type arrays". As the same suggests, such arrays allow for including more than one type and that seemed like a reasonable candidate as it allows for _extending_ the existing syntax without introducing new fields or even yet-another way of loading conf files. The new format can be seen in the tests. Please note that this is just a _tested_ POC. Integrating the POC in containers.conf may turn into a bigger journey as Podman is directly (ab)using many of the fields. Since they have to be changed to the new type (see POC), Podman will not compile without changes. Signed-off-by: Valentin Rothberg <[email protected]>
As discussed in containers/podman#20000, we need an opt-in mechanism to _append_ string arrays during loading sequence of containers.conf files. At the moment, existing fields/data will be overriden with each loaded config that sets the specified field/option. The TOML (toml.io) config format does not allow for attributing fields and structs are implicitly represented as "tables". I wanted to extend a string array with a simple boolean field, for instance: ```TOML env=["FOO=bar"] env.append=true ``` TOML doesn't suppor tthe upper idea as it's not a properly formatted table. So I looked for alternatives and found that TOML supports so-called "mixed-type arrays". As the same suggests, such arrays allow for including more than one type and that seemed like a reasonable candidate as it allows for _extending_ the existing syntax without introducing new fields or even yet-another way of loading conf files. The new format can be seen in the tests. Please note that this is just a _tested_ POC. Integrating the POC in containers.conf may turn into a bigger journey as Podman is directly (ab)using many of the fields. Since they have to be changed to the new type (see POC), Podman will not compile without changes. Signed-off-by: Valentin Rothberg <[email protected]>
@lastephey @rhatdan @Luap99 I created a POC allowing for appending arrays in containers.conf: containers/common#1675 |
As discussed in containers/podman#20000, we need an opt-in mechanism to _append_ string arrays during loading sequence of containers.conf files. At the moment, existing fields/data will be overriden with each loaded config that sets the specified field/option. The TOML (toml.io) config format does not allow for attributing fields and structs are implicitly represented as "tables". I wanted to extend a string array with a simple boolean field, for instance: ```TOML env=["FOO=bar"] env.append=true ``` TOML doesn't suppor tthe upper idea as it's not a properly formatted table. So I looked for alternatives and found that TOML supports so-called "mixed-type arrays". As the same suggests, such arrays allow for including more than one type and that seemed like a reasonable candidate as it allows for _extending_ the existing syntax without introducing new fields or even yet-another way of loading conf files. The new format can be seen in the tests. Please note that this is just a _tested_ POC. Integrating the POC in containers.conf may turn into a bigger journey as Podman is directly (ab)using many of the fields. Since they have to be changed to the new type (see POC), Podman will not compile without changes. Signed-off-by: Valentin Rothberg <[email protected]>
As discussed in containers/podman#20000, we need an opt-in mechanism to _append_ string arrays during loading sequence of containers.conf files. At the moment, existing fields/data will be overriden with each loaded config that sets the specified field/option. The TOML (toml.io) config format does not allow for attributing fields and structs are implicitly represented as "tables". I wanted to extend a string array with a simple boolean field, for instance: ```TOML env=["FOO=bar"] env.append=true ``` TOML doesn't suppor tthe upper idea as it's not a properly formatted table. So I looked for alternatives and found that TOML supports so-called "mixed-type arrays". As the same suggests, such arrays allow for including more than one type and that seemed like a reasonable candidate as it allows for _extending_ the existing syntax without introducing new fields or even yet-another way of loading conf files. The new format can be seen in the tests. Please note that this is just a _tested_ POC. Integrating the POC in containers.conf may turn into a bigger journey as Podman is directly (ab)using many of the fields. Since they have to be changed to the new type (see POC), Podman will not compile without changes. Signed-off-by: Valentin Rothberg <[email protected]>
As discussed in containers/podman#20000, we need an opt-in mechanism to _append_ string arrays during loading sequence of containers.conf files. At the moment, existing fields/data will be overriden with each loaded config that sets the specified field/option. The TOML (toml.io) config format does not allow for attributing fields and structs are implicitly represented as "tables". I wanted to extend a string array with a simple boolean field, for instance: ```TOML env=["FOO=bar"] env.append=true ``` TOML doesn't suppor tthe upper idea as it's not a properly formatted table. So I looked for alternatives and found that TOML supports so-called "mixed-type arrays". As the same suggests, such arrays allow for including more than one type and that seemed like a reasonable candidate as it allows for _extending_ the existing syntax without introducing new fields or even yet-another way of loading conf files. The new format can be seen in the tests. Please note that this is just a _tested_ POC. Integrating the POC in containers.conf may turn into a bigger journey as Podman is directly (ab)using many of the fields. Since they have to be changed to the new type (see POC), Podman will not compile without changes. Signed-off-by: Valentin Rothberg <[email protected]>
As requested in containers/podman/issues/20000, add a `privileged` field to the containers table in containers.conf. I was hesitant to add such a field at first (for security reasons) but I understand that such a field can come in handy when using modules - certain workloads require a privileged container. Signed-off-by: Valentin Rothberg <[email protected]>
As requested in containers/podman/issues/20000, add a `privileged` field to the containers table in containers.conf. I was hesitant to add such a field at first (for security reasons) but I understand that such a field can come in handy when using modules - certain workloads require a privileged container. Signed-off-by: Valentin Rothberg <[email protected]>
As discussed in containers/podman#20000, we need an opt-in mechanism to _append_ string arrays during loading sequence of containers.conf files. At the moment, existing fields/data will be overriden with each loaded config that sets the specified field/option. The TOML (toml.io) config format does not allow for attributing fields and structs are implicitly represented as "tables". I wanted to extend a string array with a simple boolean field, for instance: ```TOML env=["FOO=bar"] env.append=true ``` TOML doesn't suppor tthe upper idea as it's not a properly formatted table. So I looked for alternatives and found that TOML supports so-called "mixed-type arrays". As the same suggests, such arrays allow for including more than one type and that seemed like a reasonable candidate as it allows for _extending_ the existing syntax without introducing new fields or even yet-another way of loading conf files. The new format can be seen in the tests. Please note that this is just a _tested_ POC. Integrating the POC in containers.conf may turn into a bigger journey as Podman is directly (ab)using many of the fields. Since they have to be changed to the new type (see POC), Podman will not compile without changes. Signed-off-by: Valentin Rothberg <[email protected]>
As requested in containers/issues/20000, add a `privileged` field to the containers table in containers.conf. I was hesitant to add such a field at first (for security reasons) but I understand that such a field can come in handy when using modules - certain workloads require a privileged container. Signed-off-by: Valentin Rothberg <[email protected]>
As discussed yesterday, injecting host variables works already (I had no idea):
|
#20252 added support for the |
#20463 enabled the append logic for env, volumes and mounts. |
#20483 complets the append logic for all string arrays in containers.conf |
Feature request description
Thanks podman devs, especially @vrothberg and @rhatdan, for your work on implementing the new
module
capability.We did a little initial testing and just wanted to share some feedback and ask a few questions. cc @scanon @danfulton
Our goal would be to use this instead of the custom module functionality we developed for podman-hpc. I'm working with our
gpu
andmpich
modules as examples.gpu testing
First, working from our gpu module, I did the following test.
I've had trouble setting more than one environment variable. What is the right way to do this? I've tried
and
but it seems like it only keeps the most recently set variable.
Feature request: make it possible to set more than one environment variable
Next, it's not clear to me how to copy in an environment variable that is set on the host into the container. For example, we'll want to copy in
NVIDIA_VISIBLE_DEVICES
from the host.Feature request: make it possible to copy specific environment variables set on the host
Next, it's really great that
mount
has a glob option and I was pleased with how well it works, but to make sure the cuda driver works correctly, we also need to preserve the symlinks between the drivers. I don't know if that's possible with mount.Feature request: add ability for mount to preserve symlinks.
Very basic gpu functionality seems to work while using the module:
I think the reason the CUDA Version is N/A is because we don't have the drivers properly symlinked. But it's not bad!
MPI thoughts
Next, working from our mpich module, I didn't test anything, but @scanon and I looked through containers.conf to see if it supports the capabilities we need.
Does
containers.conf
support setting the privileged flag? That is one of the capabilities we currently set in our mpich module. Apologies if it does and we missed it.Feature request: support setting privileged flag
Is env-merge supported in
containers.conf
We'd need that to be able to append toLD_LIBRARY_PATH
.Feature request: support env-merge
Finally, another difficulty is that we initialize our
shared-run
mode for MPI in our current mpich module. I'm not sure if this would be possible in the podman module configuration.General feedback
It would be really nice to have a way to easily toggle the individual modules on and off. For example, users might want both the
gpu
andmpich
modules, but they may not want thecvmfs
module that we also offer. Right now it seems like they could stackcontainers.module.conf
files, but it would be really nice if they could do it via the CLI or in some other more user-friendly way. For example,podman run --module=foo --module=bar
.Hopefully this initial feedback is helpful. Thanks for your work on this.
Suggest potential solution
A clear and concise description of what you want to happen.
Have you considered any alternatives?
A clear and concise description of any alternative solutions or features you've considered.
Additional context
Add any other context or screenshots about the feature request here.
The text was updated successfully, but these errors were encountered: