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

[Feature]: Quadlet support for relative paths for Volume source #17418

Closed
ygalblum opened this issue Feb 8, 2023 · 9 comments · Fixed by #17857
Closed

[Feature]: Quadlet support for relative paths for Volume source #17418

ygalblum opened this issue Feb 8, 2023 · 9 comments · Fixed by #17857
Labels
kind/feature Categorizes issue or PR as related to a new feature. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. quadlet

Comments

@ygalblum
Copy link
Contributor

ygalblum commented Feb 8, 2023

Feature request description

Currently, the Volume key in the Quadlet Container group is equivalent to the -v parameter of podman run with one exception of depending on Quadlet based volumes (via .volume files).
As a result, the source (if provided) is interpreted the following way:

  • If it starts with a /, it is an absolute path to a directory on the Host
  • Otherwise, it is a named volume
    The request is to support relative paths. Quadlet should then translate them based on the location of the location of the .container file, similar to the way it handles the EnvironmentFile key.

Suggest potential solution

Unlike other path cases, for Volume there is already a meaning for a source value that does not start with /. So, to support it, Quadlet will have to require that the relative path starts with a .

Have you considered any alternatives?

No response

Additional context

I'm opening this ticket to continue the discussion in comment: #17177 (comment)

/label quadlet

@ygalblum ygalblum added the kind/feature Categorizes issue or PR as related to a new feature. label Feb 8, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Feb 8, 2023

@ygalblum: The label(s) /label quadlet cannot be applied. These labels are supported: platform/aws, platform/azure, platform/baremetal, platform/google, platform/libvirt, platform/openstack, ga, tide/merge-method-merge, tide/merge-method-rebase, tide/merge-method-squash, px-approved, docs-approved, qe-approved, downstream-change-needed, approved, backport-risk-assessed, bugzilla/valid-bug, cherry-pick-approved, jira/valid-bug, staff-eng-approved. Is this label configured under labels -> additional_labels or labels -> restricted_labels in plugin.yaml?

In response to this:

Feature request description

Currently, the Volume key in the Quadlet Container group is equivalent to the -v parameter of podman run with one exception of depending on Quadlet based volumes (via .volume files).
As a result, the source (if provided) is interpreted the following way:

  • If it starts with a /, it is an absolute path to a directory on the Host
  • Otherwise, it is a named volume
    The request is to support relative paths. Quadlet should then translate them based on the location of the location of the .container file, similar to the way it handles the EnvironmentFile key.

Suggest potential solution

Unlike other path cases, for Volume there is already a meaning for a source value that does not start with /. So, to support it, Quadlet will have to require that the relative path starts with a .

Have you considered any alternatives?

No response

Additional context

I'm opening this ticket to continue the discussion in comment: #17177 (comment)

/label quadlet

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@rhatdan
Copy link
Member

rhatdan commented Feb 8, 2023

SGTM

@github-actions
Copy link

A friendly reminder that this issue had no activity for 30 days.

@rhatdan
Copy link
Member

rhatdan commented Mar 13, 2023

@ygalblum Did you try relative paths which begin with ./ or ../?

@ygalblum
Copy link
Contributor Author

Following your comment I just did.
.container file:

[Container]
Image=registry.fedoraproject.org/fedora:36
Volume=./hello:/hello

which produces the .service file with ExecStart set to:

ExecStart=/usr/bin/podman run --name=systemd-%N --cidfile=%t/%N.cid --replace --rm --log-driver passthrough --runtime /usr/bin/crun --cgroups=split --sdnotify=conmon -d -v ./hello:/hello registry.fedoraproject.org/fedora:36

Starting the service fails:

Error: lstat hello: no such file or directory

This is not a surprise since the base directory is not what the user intended it to be. For example, for user services the base directory if $HOME.

In addition, since Quadlet looks for / to mark a path, the service file does not include the RequiresMountsFor key for the path to be mounted.

Having said that, once I created the directory $HOME/hello, the container started successfully (but still without setting the RequiresMountsFor key)

@rhatdan
Copy link
Member

rhatdan commented Mar 15, 2023

Podman does not support creating source volumes, we differ from Docker here.
Not sure what RequiresMountsFor is or why it would be required?

@rhatdan
Copy link
Member

rhatdan commented Mar 15, 2023

We could make quadlet smarter and add $PWD to the front of all relative paths and then resolve it to a real path.

@ygalblum
Copy link
Contributor Author

RequireForMount is a systemd unit key word: https://www.freedesktop.org/software/systemd/man/systemd.unit.html#RequiresMountsFor=

Adding PWD will not solve the issue because it is implicitly inferred anyhow by Podman to the execution directory (for user its $HOME). Instead what Quadlet can do is resolve the relative path to an absolute. The base directory that should be used is where the .container file resides.

As I pointed in my initial comment, relative path will have to start with . to differentiate it from a Podman volume. This actually aligns with how the -v arguments works.

@rhatdan
Copy link
Member

rhatdan commented Mar 17, 2023

Agreed

@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Aug 29, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 29, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
kind/feature Categorizes issue or PR as related to a new feature. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. quadlet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants