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

boot: Add ostree-finalize-staged.path #1740

Closed
wants to merge 1 commit into from

Conversation

jlebon
Copy link
Member

@jlebon jlebon commented Sep 28, 2018

Rather than manually starting the ostree-finalize-staged.service unit,
we can leverage systemd's path units for this. It fits quite nicely too,
given that we already have a path we drop iif we have a staged
deployment.


I started on this thinking we somehow didn't have to change presets. But of course, we do still need a preset for the path unit itself. So anyway, just throwing this up here, since I do think overall that using a path unit is nicer, but this is now predicated on getting a preset added in distros (at least for rpm-ostree-based downstreams, we can just do like in #1482, for others... we'd need to mention it clearly in the release notes).

Documentation=man:ostree(1)

[Path]
PathExists=/run/ostree/staged-deployment
Copy link
Member

Choose a reason for hiding this comment

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

When is this condition evaluated though? Is it only once at startup?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's monitored using inotify: https://www.freedesktop.org/software/systemd/man/systemd.path.html

It'd be nice if this also deactivated the service if we did e.g. rpm-ostree cleanup -p, but alas: systemd/systemd#3642

Though at least we'd gain that for free once/if that gets implemented.

@cgwalters
Copy link
Member

but this is now predicated on getting a preset added in distros

Right. So we can do that now and then land this after the next release?

jlebon added a commit to jlebon/rpm-ostree that referenced this pull request Oct 15, 2018
In preparations for ostreedev/ostree#1740, just
hard enable this path unit for now since centrally-maintained distro
presets still need to be updated.
@jlebon
Copy link
Member Author

jlebon commented Oct 15, 2018

Rather than manually starting the `ostree-finalize-staged.service` unit,
we can leverage systemd's path units for this. It fits quite nicely too,
given that we already have a path we drop iif we have a staged
deployment.

To give some time for the preset to make it to systems, we don't yet
drop the explicit call to `systemctl start`. Though we do make it
conditional based on a DEBUG env var so that we can actually test it in
CI for now. Once we're sure this has propagated, we can drop the
`systemctl start` path and the env var together.
@jlebon
Copy link
Member Author

jlebon commented Oct 16, 2018

OK, I've tested this in combination with coreos/rpm-ostree#1617. As the commit message mentions, I think we can get this in now for the upcoming release and then in the next release or the one after, drop that systemctl start path for good.

@jlebon
Copy link
Member Author

jlebon commented Oct 16, 2018

@jlebon
Copy link
Member Author

jlebon commented Oct 16, 2018

error: Installed (but unpackaged) file(s) found:
/usr/lib/systemd/system/ostree-finalize-staged.path

Right, this'll be fixed by the dist-git PR, though FAH28-insttests is not currently marked as required.

rh-atomic-bot pushed a commit to coreos/rpm-ostree that referenced this pull request Oct 16, 2018
In preparations for ostreedev/ostree#1740, just
hard enable this path unit for now since centrally-maintained distro
presets still need to be updated.

Closes: #1617
Approved by: cgwalters
@jlebon
Copy link
Member Author

jlebon commented Oct 18, 2018

bot, retest this please

@jlebon jlebon mentioned this pull request Oct 22, 2018
@cgwalters
Copy link
Member

@rh-atomic-bot r+ cdda3a0

@rh-atomic-bot
Copy link

🙀 cdda3a0 is not a valid commit SHA. Please try again with e092139.

@cgwalters
Copy link
Member

@rh-atomic-bot r+

@rh-atomic-bot
Copy link

📌 Commit e092139 has been approved by cgwalters

@rh-atomic-bot
Copy link

⌛ Testing commit e092139 with merge 40341a4...

rh-atomic-bot pushed a commit that referenced this pull request Oct 23, 2018
Rather than manually starting the `ostree-finalize-staged.service` unit,
we can leverage systemd's path units for this. It fits quite nicely too,
given that we already have a path we drop iif we have a staged
deployment.

To give some time for the preset to make it to systems, we don't yet
drop the explicit call to `systemctl start`. Though we do make it
conditional based on a DEBUG env var so that we can actually test it in
CI for now. Once we're sure this has propagated, we can drop the
`systemctl start` path and the env var together.

Closes: #1740
Approved by: cgwalters
@jlebon
Copy link
Member Author

jlebon commented Oct 23, 2018

@rh-atomic-bot retry

@rh-atomic-bot
Copy link

⌛ Testing commit e092139 with merge ac1a919...

@rh-atomic-bot
Copy link

☀️ Test successful - status-atomicjenkins
Approved by: cgwalters
Pushing ac1a919 to master...

LorbusChris pushed a commit to LorbusChris/ostree-spec that referenced this pull request Oct 23, 2018
Doing this before the upstream PR[1] gets merged because upstream CI
needs to be able to build RPMs, which uses this. Anyway, the glob is
kept general so it's equally valid even without path.

[1] ostreedev/ostree#1740
@jlebon jlebon deleted the pr/use-path branch April 24, 2023 02:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants