Merge pull request #2054 from jlebon/pr/static-delta-fetch-no-scan

lib/pull: Don't scan commit objects we fetch via deltas

.cci.jenkinsfile

@@ -47,24 +47,23 @@ cosaPod(runAsUser: 0, memory: "9Gi", cpu: "4") {
     checkout scm
     unstash 'build'
     shwrap("""
+      chown -R -h builder: .
       # Move the bits into the cosa pod (but only if major versions match)
       buildroot_id=\$(cat installed/buildroot-id)
       osver=\$(. /usr/lib/os-release && echo \$VERSION_ID)
       if test \$osver = \$buildroot_id; then
         rsync -rlv installed/rootfs/ /
       fi
       rsync -rlv installed/tests/ /
-      coreos-assembler init --force https://github.com/coreos/fedora-coreos-config
-      mkdir -p overrides/rootfs
+      runuser -u builder -- coreos-assembler init --force https://github.com/coreos/fedora-coreos-config
       # And override the on-host bits
       mv installed/rootfs/* overrides/rootfs/
       rm installed -rf
-      coreos-assembler fetch
-      coreos-assembler build
-      coreos-assembler buildextend-metal
-      coreos-assembler buildextend-metal4k
-      coreos-assembler buildextend-live --fast
-
+      runuser -u builder -- coreos-assembler fetch
+      runuser -u builder -- coreos-assembler build
+      runuser -u builder -- coreos-assembler buildextend-metal
+      runuser -u builder -- coreos-assembler buildextend-metal4k
+      runuser -u builder -- coreos-assembler buildextend-live --fast
     """)
   }
   kola(cosaDir: "${env.WORKSPACE}")

src/libostree/ostree-repo-pull-private.h

@@ -88,7 +88,9 @@ typedef struct
   GHashTable *signapi_verified_commits; /* Map<checksum,verification> of commits that have been
                                            signapi verified */
   GHashTable *ref_keyring_map;          /* Maps OstreeCollectionRef to keyring remote name */
-  GPtrArray *static_delta_superblocks;
+
+  GHashTable *static_delta_targets; /* Set<checksum> of commits fetched via static delta */
+
   GHashTable *expected_commit_sizes;           /* Maps commit checksum to known size */
   GHashTable *commit_to_depth;                 /* Maps parent commit checksum maximum depth */
   GHashTable *scanned_metadata;                /* Maps object name to itself */

src/libostree/ostree-repo-pull.c

@@ -210,7 +210,7 @@ update_progress (gpointer user_data)
       pull_data->fetched_deltapart_size, "total-delta-part-size", "t",
       pull_data->total_deltapart_size, "total-delta-part-usize", "t",
       pull_data->total_deltapart_usize, "total-delta-superblocks", "u",
-      pull_data->static_delta_superblocks->len,
+      g_hash_table_size (pull_data->static_delta_targets),
       /* We fetch metadata before content.  These allow us to report metadata fetch progress
          specifically. */
       "outstanding-metadata-fetches", "u", pull_data->n_outstanding_metadata_fetches,
@@ -1069,6 +1069,7 @@ meta_fetch_on_complete (GObject *object, GAsyncResult *result, gpointer user_dat
   g_autoptr (GError) local_error = NULL;
   GError **error = &local_error;
   gboolean free_fetch_data = TRUE;
+  gboolean was_enoent = FALSE;
 
   ostree_object_name_deserialize (fetch_data->object, &checksum, &objtype);
   checksum_obj = ostree_object_to_string (checksum, objtype);
@@ -1079,6 +1080,7 @@ meta_fetch_on_complete (GObject *object, GAsyncResult *result, gpointer user_dat
     {
       if (g_error_matches (local_error, G_IO_ERROR, G_IO_ERROR_NOT_FOUND))
         {
+          was_enoent = TRUE;
           if (fetch_data->is_detached_meta)
             {
               /* There isn't any detached metadata, just fetch the commit */
@@ -1195,7 +1197,7 @@ meta_fetch_on_complete (GObject *object, GAsyncResult *result, gpointer user_dat
   g_assert (pull_data->n_outstanding_metadata_fetches > 0);
   pull_data->n_outstanding_metadata_fetches--;
 
-  if (local_error == NULL)
+  if (local_error == NULL && !was_enoent)
     pull_data->n_fetched_metadata++;
 
   if (_ostree_fetcher_should_retry_request (local_error, fetch_data->n_retries_remaining--))
@@ -1609,9 +1611,10 @@ scan_commit_object (OtPullData *pull_data, const char *checksum, guint recursion
 
   /* We only recurse to looking whether we need dirtree/dirmeta
    * objects if the commit is partial, and we're not doing a
-   * commit-only fetch.
+   * commit-only fetch nor is it the target of a static delta.
    */
-  if (is_partial && !pull_data->is_commit_only)
+  if (is_partial && !pull_data->is_commit_only
+      && !g_hash_table_contains (pull_data->static_delta_targets, checksum))
     {
       g_autoptr (GVariant) tree_contents_csum = NULL;
       g_autoptr (GVariant) tree_meta_csum = NULL;
@@ -2466,7 +2469,7 @@ on_superblock_fetched (GObject *src, GAsyncResult *res, gpointer data)
       delta_superblock = g_variant_ref_sink (g_variant_new_from_bytes (
           (GVariantType *)OSTREE_STATIC_DELTA_SUPERBLOCK_FORMAT, delta_superblock_data, FALSE));
 
-      g_ptr_array_add (pull_data->static_delta_superblocks, g_variant_ref (delta_superblock));
+      g_hash_table_add (pull_data->static_delta_targets, g_strdup (to_revision));
       if (!process_one_static_delta (pull_data, from_revision, to_revision, delta_superblock,
                                      fetch_data->requested_ref, pull_data->cancellable, error))
         goto out;
@@ -4048,8 +4051,8 @@ ostree_repo_pull_with_options (OstreeRepo *self, const char *remote_name_or_base
         need_summary = TRUE;
     }
 
-  pull_data->static_delta_superblocks
-      = g_ptr_array_new_with_free_func ((GDestroyNotify)g_variant_unref);
+  pull_data->static_delta_targets
+      = g_hash_table_new_full (g_str_hash, g_str_equal, (GDestroyNotify)g_free, NULL);
 
   if (need_summary)
     {
@@ -4904,7 +4907,7 @@ ostree_repo_pull_with_options (OstreeRepo *self, const char *remote_name_or_base
   g_clear_pointer (&pull_data->summary_data_sig, g_bytes_unref);
   g_clear_pointer (&pull_data->summary_sig_etag, g_free);
   g_clear_pointer (&pull_data->summary, g_variant_unref);
-  g_clear_pointer (&pull_data->static_delta_superblocks, g_ptr_array_unref);
+  g_clear_pointer (&pull_data->static_delta_targets, g_hash_table_unref);
   g_clear_pointer (&pull_data->commit_to_depth, g_hash_table_unref);
   g_clear_pointer (&pull_data->expected_commit_sizes, g_hash_table_unref);
   g_clear_pointer (&pull_data->scanned_metadata, g_hash_table_unref);

src/ostree/ot-builtin-pull-local.c

@@ -36,6 +36,7 @@ static gboolean opt_per_object_fsync;
 static gboolean opt_untrusted;
 static gboolean opt_bareuseronly_files;
 static gboolean opt_require_static_deltas;
+static gboolean opt_disable_static_deltas;
 static gboolean opt_gpg_verify;
 static gboolean opt_gpg_verify_summary;
 static gboolean opt_disable_verify_bindings;
@@ -60,6 +61,8 @@ static GOptionEntry options[]
           "Reject regular files with mode outside of 0775 (world writable, suid, etc.)", NULL },
         { "require-static-deltas", 0, 0, G_OPTION_ARG_NONE, &opt_require_static_deltas,
           "Require static deltas", NULL },
+        { "disable-static-deltas", 0, 0, G_OPTION_ARG_NONE, &opt_disable_static_deltas,
+          "Do not use static deltas", NULL },
         { "gpg-verify", 0, 0, G_OPTION_ARG_NONE, &opt_gpg_verify,
           "GPG verify commits (must specify --remote)", NULL },
         { "gpg-verify-summary", 0, 0, G_OPTION_ARG_NONE, &opt_gpg_verify_summary,
@@ -186,6 +189,9 @@ ostree_builtin_pull_local (int argc, char **argv, OstreeCommandInvocation *invoc
     if (opt_remote)
       g_variant_builder_add (&builder, "{s@v}", "override-remote-name",
                              g_variant_new_variant (g_variant_new_string (opt_remote)));
+    g_variant_builder_add (
+        &builder, "{s@v}", "disable-static-deltas",
+        g_variant_new_variant (g_variant_new_boolean (opt_disable_static_deltas)));
     g_variant_builder_add (
         &builder, "{s@v}", "require-static-deltas",
         g_variant_new_variant (g_variant_new_boolean (opt_require_static_deltas)));

tests/test-delta.sh

@@ -26,7 +26,7 @@ skip_without_user_xattrs
 bindatafiles="bash true ostree"
 morebindatafiles="false ls"
 
-echo '1..13'
+echo '1..14'
 
 mkdir repo
 ostree_repo_init repo --mode=archive
@@ -183,13 +183,27 @@ echo 'ok heuristic endian detection'
 ${CMD_PREFIX} ostree --repo=repo summary -u
 
 mkdir repo2 && ostree_repo_init repo2 --mode=bare-user
-${CMD_PREFIX} ostree --repo=repo2 pull-local --require-static-deltas repo ${origrev}
+${CMD_PREFIX} ostree --repo=repo2 pull-local --require-static-deltas repo ${origrev} | tee pullstats.txt
+# we should've only fetched the superblock, the index, and the delta part
+assert_file_has_content pullstats.txt '1 delta parts, 2 loose fetched; .* transferred in .* seconds; 0 bytes content written'
 ${CMD_PREFIX} ostree --repo=repo2 fsck
 ${CMD_PREFIX} ostree --repo=repo2 ls ${origrev} >/dev/null
 
 echo 'ok pull delta'
 
-rm repo2 -rf
+# verify that having the commit partially doesn't degrade static delta fetching
+rm repo2 pullstats.txt -rf
+mkdir repo2 && ostree_repo_init repo2 --mode=bare-user
+${CMD_PREFIX} ostree --repo=repo2 pull-local --disable-static-deltas repo ${origrev} --commit-metadata-only
+${CMD_PREFIX} ostree --repo=repo2 pull-local --require-static-deltas repo ${origrev} | tee pullstats.txt
+${CMD_PREFIX} ostree --repo=repo2 fsck
+# we should've only fetched the superblock, the index, and the delta part
+assert_file_has_content pullstats.txt '1 delta parts, 2 loose fetched; .* transferred in .* seconds; 0 bytes content written'
+${CMD_PREFIX} ostree --repo=repo2 ls ${origrev} >/dev/null
+
+echo 'ok pull delta with commitpartial'
+
+rm repo2 pullstats.txt -rf
 mkdir repo2 && ostree_repo_init repo2 --mode=bare-user
 mkdir deltadir
 

tests/test-pull-localcache.sh

@@ -48,7 +48,7 @@ commit=$(${CMD_PREFIX} ostree --repo=ostree-srv/gnomerepo commit -b main --tree=
 rm -rf repo
 init_repo
 ${CMD_PREFIX} ostree --repo=repo pull --localcache-repo repo-local origin main >out.txt
-assert_file_has_content out.txt '3 metadata, 1 content objects fetched (4 meta, 5 content local)'
+assert_file_has_content out.txt '2 metadata, 1 content objects fetched (4 meta, 5 content local)'
 echo "ok pull --localcache-repo"
 
 # Check that pulling the same commit works as well