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 - Allow setting Service WorkingDirectory for Kube units #19256

Merged
merged 1 commit into from
Jul 19, 2023

Conversation

ygalblum
Copy link
Contributor

Add key for Quadlet to set WorkingDirectory to the directory of the YAML or Unit file Add Doc
Add E2E tests
Add System test

Does this PR introduce a user-facing change?

Yes

Quadlet - Allow the user to instruct Quadlet to set the service working directory relative to the YAML or Unit files

Resolves #17177

@openshift-ci openshift-ci bot added release-note approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jul 17, 2023
Copy link
Member

@Luap99 Luap99 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand the use case, why do we need a new field for that?

A user can already set:

[Service]
WorkingDirectory=/mypath

What am I missing?

@ygalblum
Copy link
Contributor Author

What am I missing?

The issue is how transferable the unit file is. For example, let's say that we have a service that different users want to run. They can each place it under /etc/containers/systemd/users/$UID. Now, assuming they want the path to point into this directory, only the generator can handle it automatically. Your suggestion will require setting this field differently for each user.

@Luap99
Copy link
Member

Luap99 commented Jul 17, 2023

Right but for that you can use the systemd specifiers (%t,%h,etc...)
I just wonder if this is really needed? But if you think that makes in easier to use for users I have no objections.

code LGTM

@ygalblum
Copy link
Contributor Author

Maybe the specifiers can solve the use case I added. But, from the discussion in #17177 it seems that having Quadlet resolve the paths is desired

@rhatdan
Copy link
Member

rhatdan commented Jul 17, 2023

I think the issue right now is that this is random, and not something the user can easily expect.

@rhatdan
Copy link
Member

rhatdan commented Jul 17, 2023

What is the default for this now? I am more interested in the default WorkingDir be set to the location of the quadlet.

@Luap99
Copy link
Member

Luap99 commented Jul 17, 2023

Systemd defaults to the users $HOME when rootless and to / when run as root AFAICT.

test/e2e/quadlet/workingdir-unit.kube Outdated Show resolved Hide resolved
@ygalblum ygalblum force-pushed the quadlet-working-dir branch 3 times, most recently from 4b2708b to 12e310b Compare July 18, 2023 08:50
@ygalblum ygalblum requested a review from edsantiago July 18, 2023 08:50
Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, nice work!

Comment on lines 943 to 944
run journalctl "--since=$STARTED_TIME" --unit="$QUADLET_SERVICE_NAME"
is "$output" '.*Started.*\.service.*'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a blocker (right now), but please join the discussion here: #19236 (review)

@ygalblum ygalblum force-pushed the quadlet-working-dir branch from 12e310b to 9e66d42 Compare July 18, 2023 13:19
@TomSweeneyRedHat
Copy link
Member

@ygalblum some of the tests are still a bit red.

@edsantiago
Copy link
Member

Test failures were podman search, one of our most popular flakes. Passed on rerun.

@ygalblum if you could kindly update your system test to the non-journalctl form I'm using in #19276 I would be most grateful, but I will also understand if you've had quite enough of the back-and-forth on this.

Add key for Quadlet to set WorkingDirectory to the directory of the YAML or Unit file
Add Doc
Add E2E tests
Add System test

Signed-off-by: Ygal Blum <[email protected]>
@ygalblum ygalblum force-pushed the quadlet-working-dir branch from 9e66d42 to 8d19070 Compare July 19, 2023 08:56
@ygalblum
Copy link
Contributor Author

you could kindly update your system test

@edsantiago I've updated my test to not use journalctl the same way as in #19284

@edsantiago
Copy link
Member

Thank you! LGTM but I will let others make the final approval.

Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 19, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 19, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: edsantiago, vrothberg, ygalblum

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:
  • OWNERS [edsantiago,vrothberg,ygalblum]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-robot openshift-merge-robot merged commit d98978a into containers:main Jul 19, 2023
@ygalblum ygalblum deleted the quadlet-working-dir branch July 19, 2023 11:49
@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 Oct 18, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 18, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. release-note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants