Add prune-ref flatpak-installer action #296

dbnicholson · 2021-12-15T01:21:22Z

The purpose of this PR is to add an action for the flatpak installer to cleanup dangling OSTree refs for uninstalled Flatpaks. This is primarily for cleaning up from bugs here or in other software (the image builder).

The PR could be split up some if preferred. The first 4 commits are entirely independent things I came across when making tests. The last commit could be broken up by component. It also might need a couple more tests.

https://phabricator.endlessm.com/T32791

To try to avoid downloading the summary file when it already has the current version, the ostree client requests the summary and signature with an If-Modified-Since HTTP header (when it can't use the preferable If-None-Match header). The HTTP header only has second precision, so it may not receive an updated summary if the request is sent within the same second the summary is updated. To ensure the client will always receive the updated summary, sleep until the next second if necessary. Note that only the signature file is checked since it's always created after the summary. https://phabricator.endlessm.com/T32791

When trying to debug a failing test, it can be useful to see the HTTP requests and responses. Send the `ostree-trivial-httpd` logs to a file in the test temporary directory. https://phabricator.endlessm.com/T32791

Only a subdirectory of the temporary directory was being passed back to the caller, so nothing was cleaning it up. Instead, have the caller allocate the temporary directory and provide the path to perform the checkout in. Without this, `/tmp/ostree-checkout-*` directories were being left behind every time `eos-updater` entered the fetch state. https://phabricator.endlessm.com/T32791

The intention of the test is that 2 app install entries with different branches should not be compressed. The specified entries were installing 2 different apps instead of the same app with different branches. https://phabricator.endlessm.com/T32791

There have been some bugs where a Flatpak is uninstalled but the underlying OSTree ref is not removed. That means the commit objects won't be pruned and the host won't recover any disk space. This adds the `prune-ref` autoinstall action to remove those refs. This action only takes effect when the corresponding Flatpak is no longer installed. https://phabricator.endlessm.com/T32791

wjt · 2021-12-15T09:25:23Z

eos-updater-flatpak-installer/eos-updater-autoinstall.schema.json

+        {
+            "properties": {
+                "action": {
+                    "const": "prune-ref",


@pwithnall would know better, but I am not sure that you can cleanly add new actions like this in a backwards-compatible way without an OS checkpoint. The actions from the OS update being deployed are parsed and prepared by the old eos-updater version.

I copied the example from the manpage to my local system, then did a dry run:

wjt@camille:~$ cat /var/lib/eos-application-tools/flatpak-autoinstall.d/99-prune-ref.json [ { "action": "prune-ref", "branch": "stable", "serial": 2017100300, "ref-kind": "app", "name": "org.example.UninstalledApp", "collection-id": "org.example.Apps", "remote": "example-apps" } ] wjt@camille:~$ /usr/libexec/eos-updater-flatpak-installer --dry-run --mode check eos-updater-flatpak-installer-Message: 09:21:29.098: Running in mode ‘check’ (eos-updater-flatpak-installer:4989): libeos-updater-util-WARNING **: 09:21:29.099: Skipping the following actions while parsing ‘99-prune-ref.json’, due to not supporting their contents. They will not be reapplied later; the system may be in an inconsistent state from this point forward. {"action":"prune-ref","branch":"stable","serial":2017100300,"ref-kind":"app","name":"org.example.UninstalledApp","collection-id":"org.example.Apps","remote":"example-apps"} [...]

I then tried installing an OS update with this file still in place. To my surprise the OS update was deployed despite this warning:

Dec 15 09:23:12 camille eos-updater[1915]: Skipping the following actions while parsing ‘99-prune-ref.json’, due to not supporting their contents. They will not be reapplied later; the system may be in an inconsistent state from this point forward. {"action":"prune-ref","branch":"stable","serial":2017100300,"ref-kind":"app","name":"org.example.UninstalledApp","collection-id":"org.example.Apps","remote":"example-apps"}

So it looks like I am wrong and in fact the action would be applied after booting into the new OS version which supports this feature.

You might still be right (there's a big comment in the code about it), but this action isn't used in the pre-upgrade part by eos-updater since there's no fetching involved.

I think we would need to thoroughly test whether an unrecognised action at the prepare phase would prevent:

subsequent actions in the same file being prepared (I think yes)

actions in subsequently-named files being prepared (?)

I can’t remember what the code was meant to do (sorry), so defer to the big comment and (more importantly) testing of whether this actually works.

dbnicholson · 2021-12-15T22:01:49Z

I broke out the unrelated fixes to #296 and will rebase if that's merged.

pwithnall · 2022-01-05T11:01:24Z

libeos-updater-flatpak-installer/perform-flatpak-actions.c

-      g_message ("Remote name for %s is %s", collection_id, remote_name);
-    }
+  if (collection_id != NULL &&
+      !validate_collection_id_remote (installation,


Nitpick: The branch would be a bit clearer if the validate_collection_id_remote() function was split out in a preparatory commit. No point doing that now, though, since the review is underway :)

pwithnall · 2022-01-05T11:08:56Z

libeos-updater-flatpak-installer/perform-flatpak-actions.c

+
+  formatted_ref = flatpak_ref_format_ref (action->ref->ref);
+  refspec = g_strdup_printf ("%s:%s", action->ref->remote, formatted_ref);
+  if (!ostree_repo_resolve_rev_ext (repo,


Should this be taking the collection ID into account too? Perhaps with ostree_repo_resolve_collection_ref()?

pwithnall · 2022-01-05T11:19:28Z

libeos-updater-util/flatpak-util.c

-          existing_action_for_ref->type == EUU_FLATPAK_REMOTE_REF_ACTION_UPDATE)
+      if (existing_action_for_ref == NULL ||
+          (ref_action_type_priority (action->type) >=
+           ref_action_type_priority (existing_action_for_ref->type)))


aiui this logic will squash [uninstall org.gnome.Foobar, prune-ref org.gnome.Foobar] into just [uninstall org.gnome.Foobar], dropping the prune-ref. Is that what we want? Are we sure that uninstalls will now always prune?

pwithnall · 2022-01-05T11:20:09Z

libeos-updater-util/flatpak-util.c

@@ -1844,6 +1906,8 @@ find_related_refs_for_action (FlatpakInstallation       *installation,
        if (!flatpak_transaction_add_update (transaction, ref_action_ref, NULL, NULL, error))
          return FALSE;
        break;
+      case EUU_FLATPAK_REMOTE_REF_ACTION_PRUNE_REF:
+        break;


Suggest adding a short comment in here to make it clear this case isn’t accidentally unimplemented.

pwithnall · 2022-01-05T11:21:03Z

libeos-updater-util/flatpak-util.c

@@ -2671,3 +2744,64 @@ euu_flatpak_transaction_uninstall (FlatpakInstallation *installation,

  return transaction_run_single_op (transaction, cancellable, error);
 }
+
+


Nitpick: Double empty line should just be a single empty line

pwithnall · 2022-01-05T11:21:50Z

libeos-updater-util/flatpak-util.c

+euu_flatpak_transaction_prune_ref (FlatpakInstallation  *installation,
+                                   const gchar          *remote,
+                                   const gchar          *formatted_ref,
+                                   gboolean              no_prune,


Could do with a brief comment (maybe formatted as a gtk-doc comment for the function) which explains what no_prune means in the context of a function whose name suggests that the only thing it does is to prune.

pwithnall · 2022-01-05T11:24:55Z

libeos-updater-util/tests/flatpak-util.c

+                   EUU_FLATPAK_REMOTE_REF_ACTION_INSTALL);
+}
+
+/* Test that no compresson occurs if 'install' and 'prune-ref' are on


Typo: ‘compression’

pwithnall · 2022-01-05T11:28:17Z

eos-updater-flatpak-installer/eos-updater-autoinstall.schema.json

+        {
+            "properties": {
+                "action": {
+                    "const": "prune-ref",


I can’t remember what the code was meant to do (sorry), so defer to the big comment and (more importantly) testing of whether this actually works.

dbnicholson added 5 commits December 14, 2021 16:12

test-common: Log httpd requests

978a809

When trying to debug a failing test, it can be useful to see the HTTP requests and responses. Send the `ostree-trivial-httpd` logs to a file in the test temporary directory. https://phabricator.endlessm.com/T32791

dbnicholson requested review from pwithnall and wjt December 15, 2021 01:21

wjt reviewed Dec 15, 2021

View reviewed changes

dbnicholson mentioned this pull request Dec 15, 2021

Assorted test fixes #297

Merged

pwithnall reviewed Jan 5, 2022

View reviewed changes

dsd marked this pull request as draft October 18, 2023 07:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add prune-ref flatpak-installer action #296

Add prune-ref flatpak-installer action #296

dbnicholson commented Dec 15, 2021

wjt Dec 15, 2021

dbnicholson Dec 15, 2021

wjt Dec 15, 2021

pwithnall Jan 5, 2022

dbnicholson commented Dec 15, 2021

pwithnall Jan 5, 2022

pwithnall Jan 5, 2022

pwithnall Jan 5, 2022

pwithnall Jan 5, 2022

pwithnall Jan 5, 2022

pwithnall Jan 5, 2022

pwithnall Jan 5, 2022

pwithnall Jan 5, 2022

		@@ -2671,3 +2744,64 @@ euu_flatpak_transaction_uninstall (FlatpakInstallation *installation,

		return transaction_run_single_op (transaction, cancellable, error);
		}

Add prune-ref flatpak-installer action #296

Are you sure you want to change the base?

Add prune-ref flatpak-installer action #296

Conversation

dbnicholson commented Dec 15, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbnicholson commented Dec 15, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment