From 8072e0891b3b23f5334a3840908b9da1e2acf550 Mon Sep 17 00:00:00 2001 From: Kelvin Fan Date: Tue, 13 Jul 2021 15:45:21 -0400 Subject: [PATCH] upgrade: Check imgref digest before pulling image Previously, `rpm-ostree upgrade` was re-pulling a container image each time, and thorwing it away if it realizes that the commit is the same as the currently booted deployment, much later on. Be smarter about this by checking the image reference digest, and don't bother pulling the container image if the digest is the same. --- rust/src/lib.rs | 11 ++++++- rust/src/sysroot_upgrade.rs | 35 +++++++++++++++++++++-- src/daemon/rpmostree-sysroot-upgrader.cxx | 19 ++++++++++-- src/libpriv/rpmostree-origin.cxx | 31 ++++++++++++++++++++ src/libpriv/rpmostree-origin.h | 7 +++++ 5 files changed, 97 insertions(+), 6 deletions(-) diff --git a/rust/src/lib.rs b/rust/src/lib.rs index 8f412e46bb..9376d4b793 100644 --- a/rust/src/lib.rs +++ b/rust/src/lib.rs @@ -130,7 +130,16 @@ pub mod ffi { // sysroot_upgrade.rs extern "Rust" { - fn import_container(sysroot: Pin<&mut OstreeSysroot>, imgref: String) -> Result; + type ContainerImport; + + fn import_container( + sysroot: Pin<&mut OstreeSysroot>, + imgref: String, + ) -> Result>; + fn get_ostree_commit(&mut self) -> String; + fn get_image_digest(&mut self) -> String; + + pub fn fetch_digest(imgref: String) -> Result; } // core.rs diff --git a/rust/src/sysroot_upgrade.rs b/rust/src/sysroot_upgrade.rs index 1cc4889232..4573a0303b 100644 --- a/rust/src/sysroot_upgrade.rs +++ b/rust/src/sysroot_upgrade.rs @@ -7,18 +7,47 @@ use anyhow::{Context, Result}; use std::convert::TryInto; use std::pin::Pin; +/// `ContainerImport`'s fields are currently identical to ostree-rs-ext''s `Import`'s fields, +/// but cxx.rs currently requires types used as extern Rust types to be defined by the same crate +/// that contains the bridge using them, so we redefine an `ContainerImport` struct here. +pub struct ContainerImport { + pub ostree_commit: String, + pub image_digest: String, +} + +impl ContainerImport { + pub(crate) fn get_ostree_commit(&mut self) -> String { + self.ostree_commit.to_string() + } + + pub(crate) fn get_image_digest(&mut self) -> String { + self.image_digest.to_string() + } +} + /// Import ostree commit in container image using ostree-rs-ext's API. pub fn import_container( mut sysroot: Pin<&mut crate::FFIOstreeSysroot>, imgref: String, -) -> CxxResult { +) -> CxxResult> { // TODO: take a GCancellable and monitor it, and drop the import task (which is how async cancellation works in Rust). let sysroot = &sysroot.gobj_wrap(); let repo = &sysroot.get_repo(gio::NONE_CANCELLABLE)?; let imgref = imgref.as_str().try_into()?; - let commit = build_runtime()? + let imported = build_runtime()? .block_on(async { ostree_ext::container::import(&repo, &imgref, None).await })?; - Ok(commit.ostree_commit) + Ok(Box::new(ContainerImport { + ostree_commit: imported.ostree_commit, + image_digest: imported.image_digest, + })) +} + +/// Fetch the image digest for `imgref` using ostree-rs-ext's API. +pub fn fetch_digest(imgref: String) -> CxxResult { + let imgref = imgref.as_str().try_into()?; + let digest = build_runtime()? + .block_on(async { ostree_ext::container::fetch_manifest_info(&imgref).await })?; + Ok(digest.manifest_digest) } fn build_runtime() -> Result { diff --git a/src/daemon/rpmostree-sysroot-upgrader.cxx b/src/daemon/rpmostree-sysroot-upgrader.cxx index d59176ee4f..3766ab0d6e 100644 --- a/src/daemon/rpmostree-sysroot-upgrader.cxx +++ b/src/daemon/rpmostree-sysroot-upgrader.cxx @@ -426,9 +426,24 @@ rpmostree_sysroot_upgrader_pull_base (RpmOstreeSysrootUpgrader *self, if (override_commit) return glnx_throw (error, "Specifying commit overrides for container-image-reference type refspecs is not supported"); - auto commit = rpmostreecxx::import_container(*self->sysroot, std::string(refspec)); + /* Check to see if the digest that `refspec` points to is the same before pulling a new container image. */ + const char *cur_digest = rpmostree_origin_get_container_image_reference_digest (self->origin); + if (cur_digest) + { + auto new_digest = rpmostreecxx::fetch_digest(std::string(refspec)); + if (strcmp (new_digest.c_str(), cur_digest) == 0) + { + /* No new digest. */ + *out_changed = FALSE; + return TRUE; + } + } + + auto import = rpmostreecxx::import_container(*self->sysroot, std::string(refspec)); - new_base_rev = strdup (commit.c_str()); + g_autofree char *image_digest = strdup (import->get_image_digest().c_str()); + rpmostree_origin_set_container_image_reference_digest (self->origin, image_digest); + new_base_rev = strdup (import->get_ostree_commit().c_str()); break; } case RPMOSTREE_REFSPEC_TYPE_CHECKSUM: diff --git a/src/libpriv/rpmostree-origin.cxx b/src/libpriv/rpmostree-origin.cxx index 9c35b2fd59..2fbc5e9ac6 100644 --- a/src/libpriv/rpmostree-origin.cxx +++ b/src/libpriv/rpmostree-origin.cxx @@ -39,6 +39,9 @@ struct RpmOstreeOrigin { RpmOstreeRefspecType refspec_type; char *cached_refspec; + /* Container image digest, if tracking a container image reference */ + char *cached_digest; + /* Version data that goes along with the refspec */ char *cached_override_commit; @@ -149,6 +152,10 @@ rpmostree_origin_parse_keyfile (GKeyFile *origin, { ret->refspec_type = RPMOSTREE_REFSPEC_TYPE_CONTAINER; ret->cached_refspec = util::move_nullify (imgref); + + g_autofree char *digest = g_key_file_get_string (ret->kf, "origin", "container-image-reference-digest", NULL); + if (digest) + ret->cached_digest = util::move_nullify (digest); } else { @@ -208,6 +215,12 @@ rpmostree_origin_remove_transient_state (RpmOstreeOrigin *origin) rpmostree_origin_set_override_commit (origin, NULL, NULL); } +const char * +rpmostree_origin_get_container_image_reference_digest (RpmOstreeOrigin *origin) +{ + return origin->cached_digest; +} + const char * rpmostree_origin_get_refspec (RpmOstreeOrigin *origin) { @@ -455,6 +468,23 @@ rpmostree_origin_set_regenerate_initramfs (RpmOstreeOrigin *origin, g_key_file_get_string_list (origin->kf, "rpmostree", "initramfs-args", NULL, NULL); } +void +rpmostree_origin_set_container_image_reference_digest (RpmOstreeOrigin *origin, + const char *digest) +{ + if (digest != NULL) + { + g_key_file_set_string (origin->kf, "origin", "container-image-reference-digest", digest); + } + else + { + g_key_file_remove_key (origin->kf, "origin", "container-image-reference-digest", NULL); + } + + g_free (origin->cached_digest); + origin->cached_digest = g_strdup (digest); +} + void rpmostree_origin_set_override_commit (RpmOstreeOrigin *origin, const char *checksum, @@ -533,6 +563,7 @@ rpmostree_origin_set_rebase_custom (RpmOstreeOrigin *origin, { /* Remove `TYPE_CONTAINER`-related keys */ g_key_file_remove_key (origin->kf, "origin", RPMOSTREE_REFSPEC_CONTAINER_ORIGIN_KEY, NULL); + g_key_file_remove_key (origin->kf, "origin", "container-image-reference-digest", NULL); const char *refspec_key = g_key_file_has_key (origin->kf, "origin", RPMOSTREE_REFSPEC_OSTREE_BASE_ORIGIN_KEY, NULL) ? diff --git a/src/libpriv/rpmostree-origin.h b/src/libpriv/rpmostree-origin.h index fd4b516eed..bdbdda0df8 100644 --- a/src/libpriv/rpmostree-origin.h +++ b/src/libpriv/rpmostree-origin.h @@ -57,6 +57,9 @@ rpmostree_origin_dup (RpmOstreeOrigin *origin); void rpmostree_origin_remove_transient_state (RpmOstreeOrigin *origin); +const char * +rpmostree_origin_get_container_image_reference_digest (RpmOstreeOrigin *origin); + const char * rpmostree_origin_get_refspec (RpmOstreeOrigin *origin); @@ -131,6 +134,10 @@ rpmostree_origin_set_regenerate_initramfs (RpmOstreeOrigin *origin, gboolean regenerate, char **args); +void +rpmostree_origin_set_container_image_reference_digest (RpmOstreeOrigin *origin, + const char *digest); + void rpmostree_origin_set_override_commit (RpmOstreeOrigin *origin, const char *checksum,