daemon: Add new Reload D-Bus method #1311
Conversation
In this PR: coreos#1309 I was hitting race conditions running `ostree admin pin` then `rpm-ostree cleanup` as it was possible that the daemon hadn't handled the inotify on the sysroot and reloaded the deployment state before the txn request came in. Close this race by doing an implicit `reload` before starting a txn. This is a pretty efficient operation because for the sysroot we're just doing a `stat()` and comparing mtime. Implementation wise, change the external API to drop the "did change" boolean as nothing outside of the `sysroot.c` file used it. A followup to this would be changing the `status` CLI to call a (new) DBus API like `RequestReload` that at least did the sysroot reload if the daemon was otherwise idle or so? And it'd be available to unprivileged users.
src/daemon/rpmostreed-sysroot.c
Outdated
| if (!sysroot_reload (self, &sysroot_changed, error)) | ||
| return FALSE; | ||
|
|
||
| /* always send an UPDATED signal to also force OS interfaces to reload */ |
There was a problem hiding this comment.
If we're doing this then shouldn't the method be called
reload_sysroot_force_os_reload() or something?
| { | ||
| /* GetOS() is always allowed */ | ||
| /* GetOS() and Reload() are always allowed */ |
There was a problem hiding this comment.
Things are weird now because this is identical to ReloadConfig right? The way I was thinking about this is we wouldn't reread the remotes in /etc, but we would do the "stat /sysroot for mtime" check.
The rationale I have for this is an admin might be in the middle of editing a config file in /etc, and having some other unprivileged software come in and read the half-written config file is exactly the problem we're avoiding by having an explicit reload command.
So internally we'd split reload_sysroot() from reload_sysroot_and_config()?
There was a problem hiding this comment.
Reload only does sysroot_reload + signal right? ReloadConfig does that and rereads config files.
There was a problem hiding this comment.
OK right, I was slightly confused, it is doing ostree_repo_reload_config but...eh that's all right.
There was a problem hiding this comment.
No actually that one will reload the remotes too, right?
There was a problem hiding this comment.
Ahhh, right. It's confusing because we have three configs: repo config, remotes, and rpm-ostreed.conf. :) I renamed sysroot_reload to be more obvious about this and added some comments! ⬇️
Follow-up to previous commit. Since we have so many concepts called "sysroot" and "configs", let's be explicit with our naming here to be sure we know what we're calling.
Add a new `Reload` method as a softer alternative to `ReloadConfig`. We'll also make use of this on the client side to sync with the daemon.
Specifically in this case, this allows us to close a race condition during `upgrade --check` where the `CachedUpdate` property might not have been updated yet when we read it after finishing the transaction.
|
⚡ Test exempted: pull fully rebased and already tested. |
Follow-up to previous commit. Since we have so many concepts called "sysroot" and "configs", let's be explicit with our naming here to be sure we know what we're calling. Closes: #1311 Approved by: cgwalters
Add a new `Reload` method as a softer alternative to `ReloadConfig`. We'll also make use of this on the client side to sync with the daemon. Closes: #1311 Approved by: cgwalters
Specifically in this case, this allows us to close a race condition during `upgrade --check` where the `CachedUpdate` property might not have been updated yet when we read it after finishing the transaction. Closes: #1311 Approved by: cgwalters
Follow up to #1310. Adds a new
Reload()D-Bus method to make it easier to sync state. See commit messages for details.