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

Quadlet support symlinked subdirectory #23755

Closed
pypingou opened this issue Aug 26, 2024 · 12 comments · Fixed by #24018
Closed

Quadlet support symlinked subdirectory #23755

pypingou opened this issue Aug 26, 2024 · 12 comments · Fixed by #24018
Labels
kind/feature Categorizes issue or PR as related to a new feature. quadlet

Comments

@pypingou
Copy link
Member

Feature request description

Currently quadlet supports having, for example: /etc/containers/systemd be a symlink.
It supports having files in that directory be symlinks (regardless of whether the directory itself is a symlink).
It supports having subfolder in that directory (regardless of whether it is a symlink).

But quadlet does not support having a subfolder be a symlink.

For my use-case, I want to be able to atomically switch a from a set of quadlet files to another. The simplest way to do that is using symlinks.
I can get it to work using the top level directory, but that means no one else but me can use that directory as otherwise I will override everything that's in it.

So in order to play nice with other apps/folks, I'd like to be able to group all my quadlets in say: /etc/containers/systemd/apps and have that be a symlink to another folder which I can update when needed to atomically move from a set of quadlet files to another.

Suggest potential solution

After discussing with @ygalblum it looks like the issue is that filePath.WalkDir doesn't follow symlink. This got fixed for the top-level folder.
Could we use the same approach for the first level of subfolders?

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.

@pypingou pypingou added the kind/feature Categorizes issue or PR as related to a new feature. label Aug 26, 2024
pypingou added a commit to pypingou/autosd_container_demo that referenced this issue Aug 26, 2024
This is a ugly hack until we can have either:
- quadlet support for symlinked subfoldeer:
  containers/podman#23755
or
- a way for osbuild to delete files/folders:
  osbuild/osbuild#1869

In this mean time, this is a ugly hack that enables us to remove this
empty folder so we can replace it with a symlink via validator.

Signed-off-by: Pierre-Yves Chibon <[email protected]>
@Luap99 Luap99 added the quadlet label Aug 26, 2024
@ygalblum
Copy link
Contributor

As @pypingou wrote, we discussed this earlier. I understand the requirement and see the benefit in supporting this request. Having said that, I know that this one of those slippery slops things. So, I wanted to clear it with others before implementing it.
@Luap99 @rhatdan @alexlarsson @vrothberg @umohnani8 anyone else WDYT?

@Luap99
Copy link
Member

Luap99 commented Aug 26, 2024

I see no problem with following symlinks, however at least for the root generator the following rule must be followed:

Non-essential file systems like /var/ and /home/ are mounted after generators have run

https://www.freedesktop.org/software/systemd/man/latest/systemd.generator.html

which means the symlink cannot point to volumes that are mounted later. Although I guess this is not an issue for rootless users.

@ygalblum
Copy link
Contributor

Yes, this is true for all symlinks (and we've seen this before).

But, my question is this. Currently, the code uses filepath.WalkDir which does not follow symlinks in order to avoid infinite loops. From this discussion: golang/go#49580 I see that this is quite the pitfall. So, I would like to limit the support for symlinks only one lever further, but then still use filepath.WalkDir. This is where my slippery slop starts (I can see people asking why we stopped at this level). WDYT?

@Luap99
Copy link
Member

Luap99 commented Aug 26, 2024

Yeah restricting one level seems a bit odd for users. I would prefer to do it for all.

avoid infinite loops

That is certainly a good point, given the generator blocks the systemd boot process AFAIK that could be painful to deal with. I think we can avoid loops by storing each path in a map as key and check if we have seen the path before then skip it.

@alexlarsson
Copy link
Contributor

Another possibility is to read a config file somewhere and use that to add additional quadlet sources. Then that could be used to point to a toplevel symlink to solve @pypingou s problem.

@ygalblum
Copy link
Contributor

Thanks @alexlarsson I like this idea. Will it be OK to add a section for Quadlet in containers.conf?

@pypingou
Copy link
Member Author

pypingou commented Aug 27, 2024 via email

@ygalblum
Copy link
Contributor

@pypingou I would go for a list of paths that are in addition to the default ones. Then each path can actually be a symlink

@pypingou
Copy link
Member Author

pypingou commented Aug 28, 2024 via email

@Luap99
Copy link
Member

Luap99 commented Aug 28, 2024

Will it be OK to add a section for Quadlet in containers.conf?

Containers.conf is not a good place because it will bloat the quadlet binary size massively due to of all the transitive imports there. My understanding is that this is not wanted for quadlet.

But do we really want a config file just to specific extra paths? If we allow symlinks this seems easier to use than having to configure another config file. But I also realize a config file could be used for more options in the future but so far I haven't seen a need for it.

@alexlarsson
Copy link
Contributor

alexlarsson commented Aug 28, 2024

Yeah, I don't think we want to use the full podman config parser in the generator.
What if we add a /etc/containers/systemd.d directory that can contain symlinks to directories.

Then we don't even need to parse any config file.

@ygalblum
Copy link
Contributor

@alexlarsson do you mean that we'll have just one directory for which Quadlet will support symlinks on the next level? I don't mind that, just trying to limit the complaints and questions we'll get in the future. So, we need to be specific and document it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Categorizes issue or PR as related to a new feature. quadlet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants