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

deploy: Delete .updated file from /etc and /var on new deployments #1631

Closed
wants to merge 1 commit into from

Conversation

dbnicholson
Copy link
Member

Systemd units using ConditionNeedsUpdate run if the mtime of .updated in
the specified directory is newer than /usr. Since /usr has an mtime of
0, there's no way to have an older .updated file. Systemd units
typically specify ConditionNeedsUpdate=/etc or ConditionNeedsUpdate=/var
to support stateless systems like ostree.

Remove the file from the new deployment's /etc and the OS's /var
regardless of where they came from to ensure that these systemd units
run when booting new deployments. This will provide a method to run
services only on upgrade.

Closes: #1628
https://bugzilla.gnome.org/show_bug.cgi?id=752950

Systemd units using ConditionNeedsUpdate run if the mtime of .updated in
the specified directory is newer than /usr. Since /usr has an mtime of
0, there's no way to have an older .updated file. Systemd units
typically specify ConditionNeedsUpdate=/etc or ConditionNeedsUpdate=/var
to support stateless systems like ostree.

Remove the file from the new deployment's /etc and the OS's /var
regardless of where they came from to ensure that these systemd units
run when booting new deployments. This will provide a method to run
services only on upgrade.

Closes: ostreedev#1628
https://bugzilla.gnome.org/show_bug.cgi?id=752950
@papr-bot
Copy link

Can one of the admins verify this patch?

@cgwalters
Copy link
Member

bot, add author to whitelist

return FALSE;

/* Ensure that the new deployment does not have /etc/.updated or
* /var/.updated so that systemd ConditionNeedsUpdate=/etc|/var services run
Copy link
Member

Choose a reason for hiding this comment

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

Not worth a respin, but just a note: Since /var is shared, this applies to all deployments. It means that if an upgrade is interrupted, on reboot we'll run through ConditionNeedsUpdate=/var for the booted deployment too.

In practice, this should be fine; on e.g. Silverblue today that's just systemd-journal-catalog-update.service.

Copy link
Member Author

Choose a reason for hiding this comment

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

By a respin do you just mean for the comment? Because obviously unless the sysroot is changed to have /var per-deployment rather than per-os then nothing can be changed there.

My experience is that all the ConditionNeedsUpdate units are using it for optimization - they're relatively slow tasks that you don't want to run on every boot. But nothing enforces that and you do need to make the units relatively idempotent if they're performing actions that you really only want to do once (e.g. use a stamp file or something). Just for the record, we make heavy use of ConditionNeedsUpdate units at Endless to perform post-boot migration tasks for things that we can't update via the OS update. There's a pile of semi-horrifying things in https://github.com/endlessm/eos-boot-helper using this mechanism.

Copy link
Member

Choose a reason for hiding this comment

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

By a respin do you just mean for the comment?

Yep. I would have phrased it like "Ensure the new deployment does not have /etc/.updated, and also delete the stateroot's /var/.updated, so that..." or something.

@cgwalters
Copy link
Member

@rh-atomic-bot r+ 19d1884

@rh-atomic-bot
Copy link

⚡ Test exempted: merge already tested.

@cgwalters
Copy link
Member

We're going over the CoreOS/Container Linux patches, and this one is related:

coreos/systemd@5afd535

This sounds right but I'd love to have more detail about exactly what unit broke.

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

4 participants