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

Add a change-update-summary config option #1681

Closed
wants to merge 5 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@mwleeds
Copy link
Member

mwleeds commented Jul 16, 2018

No description provided.

@mwleeds mwleeds force-pushed the mwleeds:1664-update-summary branch from 866bc1b to 40258d1 Jul 16, 2018

lib/repo: Take exclusive lock while generating summary
This ensures that commits aren't deleted and refs aren't added, removed,
or updated while the summary is being generated. This is in preparation
for adding a repo config option that will automatically regenerate the
summary on every ref change.

@mwleeds mwleeds force-pushed the mwleeds:1664-update-summary branch from 40258d1 to 83baafa Jul 16, 2018

<term><varname>change-update-summary</varname></term>
<listitem><para>Boolean value controlling whether or not to
automatically update the summary file after any ref is added,
removed, or updated. Defaults to <literal>false</literal>.

This comment has been minimized.

@cgwalters

cgwalters Jul 17, 2018

Member

It's worth mentioning/comparing with commit-update-summary from 9e6ac6d in #83. This is an extension of that.

I understand the argument for doing this; but do also keep in mind my original arguments for not doing it by default - systems may want to ensure that multiple refs are updated atomically. I suppose though that is (mostly) supported here if one does the ref updates in a single ostree txn via transaction_set_ref().

I am not sure if there was an explicit reason the commit-update-summary option was implemented in the ostree commit CLI - it seems to me it should be in the library in ostree_repo_commit_transaction() right?

This comment has been minimized.

@mwleeds

mwleeds Jul 17, 2018

Author Member

It's worth mentioning/comparing with commit-update-summary from 9e6ac6d in #83. This is an extension of that.

Sure, I added another sentence.

I understand the argument for doing this; but do also keep in mind my original arguments for not doing it by default - systems may want to ensure that multiple refs are updated atomically. I suppose though that is (mostly) supported here if one does the ref updates in a single ostree txn via transaction_set_ref().

Even with transactions the summary update won't be atomic with more than one ref. It will be updated for each ref update in ostree_repo_commit_transaction(). I suppose the alternative would be to teach eos-updater and flatpak to update the summary after an operation but I'm not sure it's worth the complexity that would entail. You'd probably want it to be a configurable option, but why would clients of ostree implement its own option? And perhaps the atomicity won't matter much in practice, since flatpak pulls most dependencies before the app ref, and at least for the USB case GNOME Software shouldn't show an app as able to be put on a USB until it's been fully installed. In the LAN case I think GNOME Software would show the app despite missing dependencies so you could run into errors if you try to install at exactly the wrong time.

I am not sure if there was an explicit reason the commit-update-summary option was implemented in the ostree commit CLI - it seems to me it should be in the library in ostree_repo_commit_transaction() right?

I had the same question but didn't feel qualified to make that change.

This comment has been minimized.

@jlebon

jlebon Jul 24, 2018

Member

it seems to me it should be in the library in ostree_repo_commit_transaction() right?

That struck me as odd too. We should open an issue for that.

This comment has been minimized.

@jlebon
*/
gboolean
ostree_repo_regenerate_summary (OstreeRepo *self,
GVariant *additional_metadata,
GCancellable *cancellable,
GError **error)
{
/* Take an exclusive lock. This makes sure the commits and deltas don't get
* deleted while generating the summary. It also means we can be sure refs

This comment has been minimized.

@cgwalters

cgwalters Jul 17, 2018

Member

Yes, but it doesn't protect against something deleting them immediately afterwards. I can see this helping in some cases of course, but we'd need a lot more work (probably some sort of client-side retry loop) to ensure consistency from the client's view.

This comment has been minimized.

@mwleeds

mwleeds Jul 17, 2018

Author Member

In the case of a ref deletion the summary would only briefly be out of date because the deletion would trigger an update. I'm not sure how you could get around that because I don't know how you could update the two files atomically.

@mwleeds

This comment has been minimized.

Copy link
Member Author

mwleeds commented Jul 19, 2018

I think the unit test failures are not related to these changes

@mwleeds

This comment has been minimized.

Copy link
Member Author

mwleeds commented Jul 19, 2018

bot, retest this please

@jlebon

This comment has been minimized.

Copy link
Member

jlebon commented Jul 24, 2018

Thanks! This looks sane to me. I'll let @cgwalters do the final review in case he had other concerns.

mwleeds added some commits Jul 13, 2018

config: Add a core/change-update-summary option
This commits adds and implements a boolean repo config option called
"change-update-summary" which updates the summary file every time a ref
changes (additions, updates, and deletions).

The main impetus for this feature is that the `ostree create-usb` and
`flatpak create-usb` commands depend on the repo summary being up to
date. On the command line you can work around this by asking the user to
run `ostree summary --update` but in the case of GNOME Software calling
out to `flatpak create-usb` this wouldn't work because it's running as a
user and the repo is owned by root. That strategy also means flatpak
can't update the repo metadata refs for fear of invalidating the
summary.

Another use case for this relates to LAN updates. Specifically, the
component of eos-updater that generates DNS-SD records advertising ostree
refs depends on the repo summary being up to date.

Since ostree_repo_regenerate_summary() now takes an exclusive lock, this
should be safe to enable. However it's not enabled by default because of
the performance cost, and because it's more useful on clients than
servers (which likely have another mechanism for updating the summary).

Fixes #1664

@mwleeds mwleeds force-pushed the mwleeds:1664-update-summary branch from 2bcf713 to 4d7215d Jul 26, 2018

@mwleeds

This comment has been minimized.

Copy link
Member Author

mwleeds commented Jul 26, 2018

I pushed a fixup that changes the implementation to delay summary updates until the end of a transaction, which should be more performant.

NULL,
cancellable,
error))
return FALSE;

This comment has been minimized.

@jlebon

jlebon Jul 26, 2018

Member

At this point, it might be worth adding a update_summary field in OstreeRepo, or at least factor this out to a helper function?

This comment has been minimized.

@mwleeds

mwleeds Jul 26, 2018

Author Member

If it were a field initialized when OstreeRepo is initialized, there's a possibility its on-disk value is different by the time this function is run. Sure, we could make it a helper function.

@mwleeds

This comment has been minimized.

Copy link
Member Author

mwleeds commented Jul 26, 2018

I pushed a fixup to break out a helper function, and another fixup to help if anyone wants to compare the repo mtime with the summary mtime.

error))
goto out;
/* No need to update it again if we did for each ref change */
if (opt_orphan || !change_update_summary)

This comment has been minimized.

@jlebon

jlebon Jul 27, 2018

Member

Shouldn't this be if (!opt_orphan && !change_update_summary)? Or are we trying to retain backwards compatibility with the previous behaviour of updating the summary even for orphan commits? You don't have to make the change in this PR if that's the reasoning. I can't imagine anyone actually relying on that somehow, though I'm cool with making this a separate patch for clarity.

This comment has been minimized.

@mwleeds

mwleeds Jul 29, 2018

Author Member

Yes, I was trying to continue doing what the code did before

@jlebon

This comment has been minimized.

Copy link
Member

jlebon commented Jul 30, 2018

Thanks!
@rh-atomic-bot r+ 0b3b563

@rh-atomic-bot

This comment has been minimized.

Copy link
Collaborator

rh-atomic-bot commented Jul 30, 2018

⌛️ Testing commit 0b3b563 with merge b989d94...

rh-atomic-bot added a commit that referenced this pull request Jul 30, 2018

lib/repo: Take exclusive lock while generating summary
This ensures that commits aren't deleted and refs aren't added, removed,
or updated while the summary is being generated. This is in preparation
for adding a repo config option that will automatically regenerate the
summary on every ref change.

Closes: #1681
Approved by: jlebon

rh-atomic-bot added a commit that referenced this pull request Jul 30, 2018

config: Add a core/change-update-summary option
This commits adds and implements a boolean repo config option called
"change-update-summary" which updates the summary file every time a ref
changes (additions, updates, and deletions).

The main impetus for this feature is that the `ostree create-usb` and
`flatpak create-usb` commands depend on the repo summary being up to
date. On the command line you can work around this by asking the user to
run `ostree summary --update` but in the case of GNOME Software calling
out to `flatpak create-usb` this wouldn't work because it's running as a
user and the repo is owned by root. That strategy also means flatpak
can't update the repo metadata refs for fear of invalidating the
summary.

Another use case for this relates to LAN updates. Specifically, the
component of eos-updater that generates DNS-SD records advertising ostree
refs depends on the repo summary being up to date.

Since ostree_repo_regenerate_summary() now takes an exclusive lock, this
should be safe to enable. However it's not enabled by default because of
the performance cost, and because it's more useful on clients than
servers (which likely have another mechanism for updating the summary).

Fixes #1664

Closes: #1681
Approved by: jlebon
@jlebon

This comment has been minimized.

Copy link
Member

jlebon commented Jul 30, 2018

@rh-atomic-bot

This comment has been minimized.

Copy link
Collaborator

rh-atomic-bot commented Jul 30, 2018

⌛️ Testing commit 0b3b563 with merge 0d3d1fa...

rh-atomic-bot added a commit that referenced this pull request Jul 30, 2018

lib/repo: Take exclusive lock while generating summary
This ensures that commits aren't deleted and refs aren't added, removed,
or updated while the summary is being generated. This is in preparation
for adding a repo config option that will automatically regenerate the
summary on every ref change.

Closes: #1681
Approved by: jlebon

rh-atomic-bot added a commit that referenced this pull request Jul 30, 2018

config: Add a core/change-update-summary option
This commits adds and implements a boolean repo config option called
"change-update-summary" which updates the summary file every time a ref
changes (additions, updates, and deletions).

The main impetus for this feature is that the `ostree create-usb` and
`flatpak create-usb` commands depend on the repo summary being up to
date. On the command line you can work around this by asking the user to
run `ostree summary --update` but in the case of GNOME Software calling
out to `flatpak create-usb` this wouldn't work because it's running as a
user and the repo is owned by root. That strategy also means flatpak
can't update the repo metadata refs for fear of invalidating the
summary.

Another use case for this relates to LAN updates. Specifically, the
component of eos-updater that generates DNS-SD records advertising ostree
refs depends on the repo summary being up to date.

Since ostree_repo_regenerate_summary() now takes an exclusive lock, this
should be safe to enable. However it's not enabled by default because of
the performance cost, and because it's more useful on clients than
servers (which likely have another mechanism for updating the summary).

Fixes #1664

Closes: #1681
Approved by: jlebon
@jlebon

This comment has been minimized.

Copy link
Member

jlebon commented Jul 30, 2018

@rh-atomic-bot

This comment has been minimized.

Copy link
Collaborator

rh-atomic-bot commented Jul 30, 2018

⌛️ Testing commit 0b3b563 with merge 6869bad...

rh-atomic-bot added a commit that referenced this pull request Jul 30, 2018

config: Add a core/change-update-summary option
This commits adds and implements a boolean repo config option called
"change-update-summary" which updates the summary file every time a ref
changes (additions, updates, and deletions).

The main impetus for this feature is that the `ostree create-usb` and
`flatpak create-usb` commands depend on the repo summary being up to
date. On the command line you can work around this by asking the user to
run `ostree summary --update` but in the case of GNOME Software calling
out to `flatpak create-usb` this wouldn't work because it's running as a
user and the repo is owned by root. That strategy also means flatpak
can't update the repo metadata refs for fear of invalidating the
summary.

Another use case for this relates to LAN updates. Specifically, the
component of eos-updater that generates DNS-SD records advertising ostree
refs depends on the repo summary being up to date.

Since ostree_repo_regenerate_summary() now takes an exclusive lock, this
should be safe to enable. However it's not enabled by default because of
the performance cost, and because it's more useful on clients than
servers (which likely have another mechanism for updating the summary).

Fixes #1664

Closes: #1681
Approved by: jlebon
@rh-atomic-bot

This comment has been minimized.

Copy link
Collaborator

rh-atomic-bot commented Jul 30, 2018

☀️ Test successful - status-atomicjenkins
Approved by: jlebon
Pushing 6869bad to master...

@mwleeds mwleeds deleted the mwleeds:1664-update-summary branch Jul 30, 2018

mwleeds added a commit to mwleeds/ostree that referenced this pull request Sep 24, 2018

lib/repo: Take exclusive lock while generating summary
This ensures that commits aren't deleted and refs aren't added, removed,
or updated while the summary is being generated. This is in preparation
for adding a repo config option that will automatically regenerate the
summary on every ref change.

Closes: ostreedev#1681
Approved by: jlebon

mwleeds added a commit to mwleeds/ostree that referenced this pull request Sep 24, 2018

config: Add a core/change-update-summary option
This commits adds and implements a boolean repo config option called
"change-update-summary" which updates the summary file every time a ref
changes (additions, updates, and deletions).

The main impetus for this feature is that the `ostree create-usb` and
`flatpak create-usb` commands depend on the repo summary being up to
date. On the command line you can work around this by asking the user to
run `ostree summary --update` but in the case of GNOME Software calling
out to `flatpak create-usb` this wouldn't work because it's running as a
user and the repo is owned by root. That strategy also means flatpak
can't update the repo metadata refs for fear of invalidating the
summary.

Another use case for this relates to LAN updates. Specifically, the
component of eos-updater that generates DNS-SD records advertising ostree
refs depends on the repo summary being up to date.

Since ostree_repo_regenerate_summary() now takes an exclusive lock, this
should be safe to enable. However it's not enabled by default because of
the performance cost, and because it's more useful on clients than
servers (which likely have another mechanism for updating the summary).

Fixes ostreedev#1664

Closes: ostreedev#1681
Approved by: jlebon
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.