From 6bd2c25f64fc72cdc3843251bbb66396283df002 Mon Sep 17 00:00:00 2001 From: Kelvin Fan Date: Tue, 6 Jul 2021 12:07:48 -0400 Subject: [PATCH 1/2] Allow rebase to OSTree commits in container images This adds support for providing a container image as the argument for `rpm-ostree rebase` by teaching rpm-ostree to use ostree-rs-ext's new import OSTree commits from container images feature. `deploy` while based on a container image is not yet implemented. Currently, a tag for the target container image must be provided; i.e. omitting the tag will not be a shortcut for `latest`. --- rust/rpmostree-client/src/lib.rs | 3 +- rust/src/core.rs | 40 ++++++++ rust/src/lib.rs | 9 ++ rust/src/origin.rs | 9 +- rust/src/sysroot_upgrade.rs | 29 ++++++ src/app/rpmostree-builtin-rebase.cxx | 10 ++ src/app/rpmostree-builtin-status.cxx | 10 +- src/daemon/rpmostree-sysroot-upgrader.cxx | 10 ++ src/daemon/rpmostreed-deployment-utils.cxx | 9 +- src/daemon/rpmostreed-transaction-types.cxx | 39 +++++++- src/libpriv/rpmostree-core.cxx | 6 ++ src/libpriv/rpmostree-core.h | 5 + src/libpriv/rpmostree-origin.cxx | 100 +++++++++++++++----- tests/kolainst/destructive/container-image | 78 +++++++++++++++ 14 files changed, 321 insertions(+), 36 deletions(-) create mode 100644 rust/src/sysroot_upgrade.rs create mode 100755 tests/kolainst/destructive/container-image diff --git a/rust/rpmostree-client/src/lib.rs b/rust/rpmostree-client/src/lib.rs index 3005431310..53ad0fecf3 100644 --- a/rust/rpmostree-client/src/lib.rs +++ b/rust/rpmostree-client/src/lib.rs @@ -53,7 +53,8 @@ pub struct Deployment { pub staged: Option, pub booted: bool, pub serial: u32, - pub origin: String, + pub origin: Option, + pub container_image_reference: Option, pub version: Option, } diff --git a/rust/src/core.rs b/rust/src/core.rs index 01550783e9..320e9146b5 100644 --- a/rust/src/core.rs +++ b/rust/src/core.rs @@ -77,6 +77,16 @@ pub(crate) fn run_depmod(rootfs_dfd: i32, kver: &str, unified_core: bool) -> Cxx Ok(()) } +/// Infer whether refspec is a ctonainer image reference. +pub(crate) fn is_container_image_reference(refspec: &str) -> bool { + // Currently, we are simply relying on the fact that there cannot be multiple colons + // or the `@` symbol in TYPE_OSTREE or TYPE_COMMIT refspecs. We may want a more robust + // and reliable way of determining the refspec type in the future, as some container + // transports may possibly not contain colons. + // https://github.com/coreos/rpm-ostree/issues/2909#issuecomment-868151689 + refspec.split(':').nth(2).is_some() || refspec.contains('@') +} + /// Perform reversible filesystem transformations necessary before we execute scripts. pub(crate) struct FilesystemScriptPrep { rootfs: openat::Dir, @@ -174,4 +184,34 @@ mod test { } Ok(()) } + + #[test] + fn test_is_container_image_reference() -> Result<()> { + use super::is_container_image_reference; + + let refspec_type_checksum = + "ee10f8e7ef638d78ba9a9596665067f58021624118875cc4079568da6c63efb0"; + assert!(!is_container_image_reference(refspec_type_checksum)); + + let refspec_type_ostree_with_remote = "fedora:fedora/x86_64/coreos/testing-devel"; + assert!(!is_container_image_reference( + refspec_type_ostree_with_remote + )); + let refspec_type_ostree = "fedora/x86_64/coreos/foo-branch"; + assert!(!is_container_image_reference(refspec_type_ostree)); + + const REFSPEC_TYPE_CONTAINER: &[&str] = &[ + "containers-storage:localhost/fcos:latest", + "docker://quay.io/test-repository/os:version1", + "registry:docker.io/test-repository/os:latest", + "registry:customhostname.com:8080/test-repository/os:latest", + "docker://quay.io/test-repository/os@sha256:6006dca86c2dc549c123ff4f1dcbe60105fb05886531c93a3351ebe81dbe772f", + ]; + + for refspec in REFSPEC_TYPE_CONTAINER { + assert!(is_container_image_reference(refspec)); + } + + Ok(()) + } } diff --git a/rust/src/lib.rs b/rust/src/lib.rs index 3bf323a3fa..d4562db761 100644 --- a/rust/src/lib.rs +++ b/rust/src/lib.rs @@ -128,6 +128,11 @@ pub mod ffi { fn cliwrap_destdir() -> String; } + // sysroot_upgrade.rs + extern "Rust" { + fn import_container(sysroot: Pin<&mut OstreeSysroot>, imgref: String) -> Result; + } + // core.rs extern "Rust" { type TempEtcGuard; @@ -140,6 +145,8 @@ pub mod ffi { fn undo(self: &FilesystemScriptPrep) -> Result<()>; fn run_depmod(rootfs_dfd: i32, kver: &str, unified_core: bool) -> Result<()>; + + fn is_container_image_reference(refspec: &str) -> bool; } // composepost.rs @@ -600,6 +607,8 @@ pub(crate) use self::console_progress::*; mod progress; mod scripts; pub(crate) use self::scripts::*; +mod sysroot_upgrade; +pub(crate) use crate::sysroot_upgrade::*; mod rpmutils; pub(crate) use self::rpmutils::*; mod testutils; diff --git a/rust/src/origin.rs b/rust/src/origin.rs index ae30dea047..f6a1ca4bc3 100644 --- a/rust/src/origin.rs +++ b/rust/src/origin.rs @@ -32,11 +32,14 @@ fn origin_to_treefile_inner(kf: &KeyFile) -> Result> { let mut cfg: crate::treefile::TreeComposeConfig = Default::default(); let refspec_str = if let Some(r) = keyfile_get_optional_string(&kf, ORIGIN, "refspec")? { Some(r) + } else if let Some(r) = keyfile_get_optional_string(&kf, ORIGIN, "baserefspec")? { + Some(r) } else { - keyfile_get_optional_string(&kf, ORIGIN, "baserefspec")? + keyfile_get_optional_string(&kf, ORIGIN, "container-image-reference")? }; - let refspec_str = refspec_str - .ok_or_else(|| anyhow::anyhow!("Failed to find refspec/baserefspec in origin"))?; + let refspec_str = refspec_str.ok_or_else(|| { + anyhow::anyhow!("Failed to find refspec/baserefspec/container-image-reference in origin") + })?; cfg.derive.base_refspec = Some(refspec_str); cfg.packages = parse_stringlist(&kf, PACKAGES, "requested")?; cfg.derive.packages_local = parse_localpkglist(&kf, PACKAGES, "requested-local")?; diff --git a/rust/src/sysroot_upgrade.rs b/rust/src/sysroot_upgrade.rs new file mode 100644 index 0000000000..1cc4889232 --- /dev/null +++ b/rust/src/sysroot_upgrade.rs @@ -0,0 +1,29 @@ +//! Rust portion of `rpmostree-sysroot-upgrader.cxx`. + +// SPDX-License-Identifier: Apache-2.0 OR MIT + +use crate::cxxrsutil::*; +use anyhow::{Context, Result}; +use std::convert::TryInto; +use std::pin::Pin; + +/// 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 { + // 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()? + .block_on(async { ostree_ext::container::import(&repo, &imgref, None).await })?; + Ok(commit.ostree_commit) +} + +fn build_runtime() -> Result { + tokio::runtime::Builder::new_multi_thread() + .enable_all() + .build() + .context("Failed to build tokio runtime") +} diff --git a/src/app/rpmostree-builtin-rebase.cxx b/src/app/rpmostree-builtin-rebase.cxx index 6b07b9ec66..f943507b33 100644 --- a/src/app/rpmostree-builtin-rebase.cxx +++ b/src/app/rpmostree-builtin-rebase.cxx @@ -140,6 +140,16 @@ rpmostree_builtin_rebase (int argc, if (strlen (new_provided_refspec) == 0) return glnx_throw (error, "Refspec is empty"); + if (refspectype == RPMOSTREE_REFSPEC_TYPE_CONTAINER) + { + if (!opt_experimental) + return glnx_throw (error, "Rebasing to a container image reference requires --experimental"); + /* When using the container refspec type, if rebasing to a specific commit, we expect a + * specific digest tag in the refspec, not in a separate argument */ + if (revision) + return glnx_throw (error, "Unexpected ostree revision alongside container refspec type"); + } + /* Check if remote refers to a local repo */ g_autofree char *local_repo_remote = NULL; if (G_IN_SET (refspectype, RPMOSTREE_REFSPEC_TYPE_OSTREE, diff --git a/src/app/rpmostree-builtin-status.cxx b/src/app/rpmostree-builtin-status.cxx index e5e5a9f621..56aafa2ec0 100644 --- a/src/app/rpmostree-builtin-status.cxx +++ b/src/app/rpmostree-builtin-status.cxx @@ -559,6 +559,7 @@ print_one_deployment (RPMOSTreeSysroot *sysroot_proxy, return TRUE; const gchar *origin_refspec; + RpmOstreeRefspecType refspectype = RPMOSTREE_REFSPEC_TYPE_OSTREE; g_autofree const gchar **origin_packages = NULL; g_autofree const gchar **origin_requested_packages = NULL; g_autofree const gchar **origin_requested_local_packages = NULL; @@ -566,7 +567,8 @@ print_one_deployment (RPMOSTreeSysroot *sysroot_proxy, g_autofree const gchar **origin_requested_base_removals = NULL; g_autoptr(GVariant) origin_base_local_replacements = NULL; g_autofree const gchar **origin_requested_base_local_replacements = NULL; - if (g_variant_dict_lookup (dict, "origin", "&s", &origin_refspec)) + if (g_variant_dict_lookup (dict, "origin", "&s", &origin_refspec) || + g_variant_dict_lookup (dict, "container-image-reference", "&s", &origin_refspec)) { origin_packages = lookup_array_and_canonicalize (dict, "packages"); @@ -606,7 +608,6 @@ print_one_deployment (RPMOSTreeSysroot *sysroot_proxy, g_print ("%s ", is_booted ? libsd_special_glyph (BLACK_CIRCLE) : " "); - RpmOstreeRefspecType refspectype = RPMOSTREE_REFSPEC_TYPE_OSTREE; const char *custom_origin_url = NULL; const char *custom_origin_description = NULL; if (origin_refspec) @@ -637,6 +638,11 @@ print_one_deployment (RPMOSTreeSysroot *sysroot_proxy, g_print ("%s", origin_refspec); } break; + case RPMOSTREE_REFSPEC_TYPE_CONTAINER: + { + g_print ("%s", origin_refspec); + } + break; } } else diff --git a/src/daemon/rpmostree-sysroot-upgrader.cxx b/src/daemon/rpmostree-sysroot-upgrader.cxx index 1aa8b2cc79..398f7a5617 100644 --- a/src/daemon/rpmostree-sysroot-upgrader.cxx +++ b/src/daemon/rpmostree-sysroot-upgrader.cxx @@ -443,6 +443,16 @@ rpmostree_sysroot_upgrader_pull_base (RpmOstreeSysrootUpgrader *self, switch (refspec_type) { + case RPMOSTREE_REFSPEC_TYPE_CONTAINER: + { + 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)); + + new_base_rev = strdup (commit.c_str()); + break; + } case RPMOSTREE_REFSPEC_TYPE_CHECKSUM: case RPMOSTREE_REFSPEC_TYPE_OSTREE: { diff --git a/src/daemon/rpmostreed-deployment-utils.cxx b/src/daemon/rpmostreed-deployment-utils.cxx index de3eb47f52..bec7563ba1 100644 --- a/src/daemon/rpmostreed-deployment-utils.cxx +++ b/src/daemon/rpmostreed-deployment-utils.cxx @@ -253,6 +253,7 @@ rpmostreed_deployment_generate_variant (OstreeSysroot *sysroot, RpmOstreeRefspecType refspec_type; g_autofree char *refspec = rpmostree_origin_get_full_refspec (origin, &refspec_type); + g_assert (refspec); gboolean is_layered = FALSE; g_autofree char *base_checksum = NULL; @@ -301,8 +302,12 @@ rpmostreed_deployment_generate_variant (OstreeSysroot *sysroot, switch (refspec_type) { + case RPMOSTREE_REFSPEC_TYPE_CONTAINER: + g_variant_dict_insert (dict, "container-image-reference", "s", refspec); + break; case RPMOSTREE_REFSPEC_TYPE_CHECKSUM: { + g_variant_dict_insert (dict, "origin", "s", refspec); g_autofree char *custom_origin_url = NULL; g_autofree char *custom_origin_description = NULL; rpmostree_origin_get_custom_description (origin, &custom_origin_url, @@ -315,6 +320,7 @@ rpmostreed_deployment_generate_variant (OstreeSysroot *sysroot, break; case RPMOSTREE_REFSPEC_TYPE_OSTREE: { + g_variant_dict_insert (dict, "origin", "s", refspec); if (!variant_add_remote_status (repo, refspec, base_checksum, dict, error)) return NULL; @@ -339,9 +345,6 @@ rpmostreed_deployment_generate_variant (OstreeSysroot *sysroot, break; } - if (refspec) - g_variant_dict_insert (dict, "origin", "s", refspec); - variant_add_from_hash_table (dict, "requested-packages", rpmostree_origin_get_packages (origin)); variant_add_from_hash_table (dict, "requested-local-packages", diff --git a/src/daemon/rpmostreed-transaction-types.cxx b/src/daemon/rpmostreed-transaction-types.cxx index 06b3d0a751..cca2e4dc63 100644 --- a/src/daemon/rpmostreed-transaction-types.cxx +++ b/src/daemon/rpmostreed-transaction-types.cxx @@ -59,9 +59,37 @@ change_origin_refspec (GVariantDict *options, gchar **out_new_refspec, GError **error) { + RpmOstreeRefspecType refspectype; + if (!rpmostree_refspec_classify (refspec, &refspectype, error)) + return FALSE; + RpmOstreeRefspecType current_refspectype; g_autofree gchar *current_refspec = rpmostree_origin_get_full_refspec (origin, ¤t_refspectype); - + + switch (refspectype) + { + case RPMOSTREE_REFSPEC_TYPE_CONTAINER: + { + if (!rpmostree_origin_set_rebase (origin, refspec, error)) + return FALSE; + + if (current_refspectype == RPMOSTREE_REFSPEC_TYPE_CONTAINER + && strcmp (current_refspec, refspec) == 0) + return glnx_throw (error, "Old and new refs are equal: %s", refspec); + + if (out_old_refspec != NULL) + *out_old_refspec = util::move_nullify (current_refspec); + if (out_new_refspec != NULL) + *out_new_refspec = g_strdup (refspec); + return TRUE; + } + case RPMOSTREE_REFSPEC_TYPE_OSTREE: + break; + case RPMOSTREE_REFSPEC_TYPE_CHECKSUM: + break; + } + + /* The rest of the code assumes TYPE_OSTREE refspec */ g_autofree gchar *new_refspec = NULL; if (!rpmostreed_refspec_parse_partial (refspec, current_refspec, @@ -151,6 +179,11 @@ apply_revision_override (RpmostreedTransaction *transaction, if (refspectype == RPMOSTREE_REFSPEC_TYPE_CHECKSUM) return glnx_throw (error, "Cannot look up version while pinned to commit"); + if (refspectype == RPMOSTREE_REFSPEC_TYPE_CONTAINER) + /* NB: Not supported for now, but We can perhaps support this if we allow `revision` to + * possibly be a tag or digest */ + return glnx_throw (error, "Cannot look up version while tracking a container image reference"); + g_autofree char *checksum = NULL; g_autofree char *version = NULL; if (!rpmostreed_parse_revision (revision, &checksum, &version, error)) @@ -160,6 +193,8 @@ apply_revision_override (RpmostreedTransaction *transaction, { switch (refspectype) { + case RPMOSTREE_REFSPEC_TYPE_CONTAINER: + g_assert_not_reached (); /* Handled above */ case RPMOSTREE_REFSPEC_TYPE_OSTREE: { /* Perhaps down the line we'll drive history traversal into libostree */ @@ -183,6 +218,8 @@ apply_revision_override (RpmostreedTransaction *transaction, switch (refspectype) { + case RPMOSTREE_REFSPEC_TYPE_CONTAINER: + g_assert_not_reached (); /* Handled above */ case RPMOSTREE_REFSPEC_TYPE_OSTREE: if (!skip_branch_check) { diff --git a/src/libpriv/rpmostree-core.cxx b/src/libpriv/rpmostree-core.cxx index 743f0194f8..864800a0dd 100644 --- a/src/libpriv/rpmostree-core.cxx +++ b/src/libpriv/rpmostree-core.cxx @@ -59,6 +59,12 @@ rpmostree_refspec_classify (const char *refspec, RpmOstreeRefspecType *out_type, GError **error) { + if (rpmostreecxx::is_container_image_reference(refspec)) + { + *out_type = RPMOSTREE_REFSPEC_TYPE_CONTAINER; + return TRUE; + } + /* Fall back to TYPE_OSTREE if we cannot infer type */ *out_type = RPMOSTREE_REFSPEC_TYPE_OSTREE; if (ostree_validate_checksum_string (refspec, NULL)) diff --git a/src/libpriv/rpmostree-core.h b/src/libpriv/rpmostree-core.h index c59c88c45b..e6b3632530 100644 --- a/src/libpriv/rpmostree-core.h +++ b/src/libpriv/rpmostree-core.h @@ -59,8 +59,13 @@ G_DECLARE_FINAL_TYPE (RpmOstreeContext, rpmostree_context, RPMOSTREE, CONTEXT, G typedef enum { RPMOSTREE_REFSPEC_TYPE_OSTREE, RPMOSTREE_REFSPEC_TYPE_CHECKSUM, + RPMOSTREE_REFSPEC_TYPE_CONTAINER, } RpmOstreeRefspecType; +#define RPMOSTREE_REFSPEC_OSTREE_ORIGIN_KEY "refspec" +#define RPMOSTREE_REFSPEC_OSTREE_BASE_ORIGIN_KEY "baserefspec" +#define RPMOSTREE_REFSPEC_CONTAINER_ORIGIN_KEY "container-image-reference" + gboolean rpmostree_refspec_classify (const char *refspec, RpmOstreeRefspecType *out_type, GError **error); diff --git a/src/libpriv/rpmostree-origin.cxx b/src/libpriv/rpmostree-origin.cxx index 1e3550c53a..3f61420fe8 100644 --- a/src/libpriv/rpmostree-origin.cxx +++ b/src/libpriv/rpmostree-origin.cxx @@ -115,23 +115,47 @@ rpmostree_origin_parse_keyfile (GKeyFile *origin, ret->cached_unconfigured_state = g_key_file_get_string (ret->kf, "origin", "unconfigured-state", NULL); - g_autofree char *refspec = g_key_file_get_string (ret->kf, "origin", "refspec", NULL); - if (!refspec) + /* Note that the refspec type can be inferred from the key in the orgin file, where + * the `RPMOSTREE_REFSPEC_OSTREE_ORIGIN_KEY` and `RPMOSTREE_REFSPEC_OSTREE_BASE_ORIGIN_KEY` keys + * may correspond to either TYPE_OSTREE or TYPE_CHECKSUM (further classification is required), and + * `RPMOSTREE_REFSPEC_CONTAINER_ORIGIN_KEY` always only corresponds to TYPE_CONTAINER. */ + g_autofree char *ost_refspec = g_key_file_get_string (ret->kf, "origin", RPMOSTREE_REFSPEC_OSTREE_ORIGIN_KEY, NULL); + g_autofree char *imgref = g_key_file_get_string (ret->kf, "origin", RPMOSTREE_REFSPEC_CONTAINER_ORIGIN_KEY, NULL); + if (!ost_refspec) { - refspec = g_key_file_get_string (ret->kf, "origin", "baserefspec", NULL); - if (!refspec) - return (RpmOstreeOrigin *)glnx_null_throw (error, "No origin/refspec, or origin/baserefspec in current deployment origin; cannot handle via rpm-ostree"); + /* See if ostree refspec is baserefspec. */ + ost_refspec = g_key_file_get_string (ret->kf, "origin", RPMOSTREE_REFSPEC_OSTREE_BASE_ORIGIN_KEY, NULL); + if (!ost_refspec && !imgref) + return (RpmOstreeOrigin *)glnx_null_throw (error, + "No origin/%s, origin/%s, or origin/%s " + "in current deployment origin; cannot handle via rpm-ostree", + RPMOSTREE_REFSPEC_OSTREE_ORIGIN_KEY, + RPMOSTREE_REFSPEC_OSTREE_BASE_ORIGIN_KEY, + RPMOSTREE_REFSPEC_CONTAINER_ORIGIN_KEY); + } + if (ost_refspec && imgref) + { + return (RpmOstreeOrigin *)glnx_null_throw (error, + "Refspec expressed by multiple keys in deployment origin"); + } + else if (ost_refspec) + { + /* Classify to distinguish between TYPE_CHECKSUM and TYPE_OSTREE */ + if (!rpmostree_refspec_classify (ost_refspec, &ret->refspec_type, error)) + return FALSE; + ret->cached_refspec = util::move_nullify (ost_refspec); + ret->cached_override_commit = + g_key_file_get_string (ret->kf, "origin", "override-commit", NULL); + } + else if (imgref) + { + ret->refspec_type = RPMOSTREE_REFSPEC_TYPE_CONTAINER; + ret->cached_refspec = util::move_nullify (imgref); + } + else + { + g_assert_not_reached (); } - - if (!rpmostree_refspec_classify (refspec, &ret->refspec_type, error)) - return FALSE; - /* Note the lack of a prefix here so that code that just calls - * rpmostree_origin_get_refspec() in the ostree:// case - * sees it without the prefix for compatibility. - */ - ret->cached_refspec = util::move_nullify (refspec); - ret->cached_override_commit = - g_key_file_get_string (ret->kf, "origin", "override-commit", NULL); if (!parse_packages_strv (ret->kf, "packages", "requested", FALSE, ret->cached_packages, error)) @@ -499,8 +523,8 @@ rpmostree_origin_set_rebase_custom (RpmOstreeOrigin *origin, } /* We don't want to carry any commit overrides or version pinning during a - * rebase by default. - */ + * rebase by default. + */ rpmostree_origin_set_override_commit (origin, NULL, NULL); /* See related code in rpmostree_origin_parse_keyfile() */ @@ -508,14 +532,21 @@ rpmostree_origin_set_rebase_custom (RpmOstreeOrigin *origin, return FALSE; g_free (origin->cached_refspec); origin->cached_refspec = g_strdup (new_refspec); + /* Note the following sets different keys depending on the type of refspec; + * TYPE_OSTREE and TYPE_CHECKSUM may set either `RPMOSTREE_REFSPEC_OSTREE_BASE_ORIGIN_KEY` + * or `RPMOSTREE_REFSPEC_OSTREE_ORIGIN_KEY`, while TYPE_CONTAINER will set the + * `RPMOSTREE_REFSPEC_CONTAINER_ORIGIN_KEY` key. */ switch (origin->refspec_type) { case RPMOSTREE_REFSPEC_TYPE_CHECKSUM: case RPMOSTREE_REFSPEC_TYPE_OSTREE: { + /* Remove `TYPE_CONTAINER`-related keys */ + g_key_file_remove_key (origin->kf, "origin", RPMOSTREE_REFSPEC_CONTAINER_ORIGIN_KEY, NULL); + const char *refspec_key = - g_key_file_has_key (origin->kf, "origin", "baserefspec", NULL) ? - "baserefspec" : "refspec"; + g_key_file_has_key (origin->kf, "origin", RPMOSTREE_REFSPEC_OSTREE_BASE_ORIGIN_KEY, NULL) ? + RPMOSTREE_REFSPEC_OSTREE_BASE_ORIGIN_KEY : RPMOSTREE_REFSPEC_OSTREE_ORIGIN_KEY; g_key_file_set_string (origin->kf, "origin", refspec_key, origin->cached_refspec); if (!custom_origin_url) { @@ -532,6 +563,18 @@ rpmostree_origin_set_rebase_custom (RpmOstreeOrigin *origin, } } break; + case RPMOSTREE_REFSPEC_TYPE_CONTAINER: + { + /* Remove `TYPE_OSTREE` and `TYPE_CHECKSUM`-related keys */ + g_assert (!custom_origin_url); + g_key_file_remove_key (origin->kf, "origin", RPMOSTREE_REFSPEC_OSTREE_ORIGIN_KEY, NULL); + g_key_file_remove_key (origin->kf, "origin", RPMOSTREE_REFSPEC_OSTREE_BASE_ORIGIN_KEY, NULL); + g_key_file_remove_key (origin->kf, "origin", "custom-url", NULL); + g_key_file_remove_key (origin->kf, "origin", "custom-description", NULL); + + g_key_file_set_string (origin->kf, "origin", RPMOSTREE_REFSPEC_CONTAINER_ORIGIN_KEY, origin->cached_refspec); + } + break; } return TRUE; @@ -545,23 +588,28 @@ rpmostree_origin_set_rebase (RpmOstreeOrigin *origin, return rpmostree_origin_set_rebase_custom (origin, new_refspec, NULL, NULL, error); } -// Switch to `baserefspec` when changing the origin -// to something core ostree doesnt't understand, i.e. -// when `ostree admin upgrade` would no longer do the right thing. +/* If necessary, switch to `baserefspec` when changing the origin + * to something core ostree doesnt't understand, i.e. + * when `ostree admin upgrade` would no longer do the right thing. */ static void sync_baserefspec (RpmOstreeOrigin *origin) { + /* If we're based on a container image reference, this is not necessary since + * `ostree admin upgrade` won't work regardless of whether or not packages are + * layered. Though this may change in the future. */ + if (origin->refspec_type == RPMOSTREE_REFSPEC_TYPE_CONTAINER) + return; if (rpmostree_origin_may_require_local_assembly (origin)) { - g_key_file_set_value (origin->kf, "origin", "baserefspec", + g_key_file_set_value (origin->kf, "origin", RPMOSTREE_REFSPEC_OSTREE_BASE_ORIGIN_KEY, origin->cached_refspec); - g_key_file_remove_key (origin->kf, "origin", "refspec", NULL); + g_key_file_remove_key (origin->kf, "origin", RPMOSTREE_REFSPEC_OSTREE_ORIGIN_KEY, NULL); } else { - g_key_file_set_value (origin->kf, "origin", "refspec", + g_key_file_set_value (origin->kf, "origin", RPMOSTREE_REFSPEC_OSTREE_ORIGIN_KEY, origin->cached_refspec); - g_key_file_remove_key (origin->kf, "origin", "baserefspec", NULL); + g_key_file_remove_key (origin->kf, "origin", RPMOSTREE_REFSPEC_OSTREE_BASE_ORIGIN_KEY, NULL); } } diff --git a/tests/kolainst/destructive/container-image b/tests/kolainst/destructive/container-image new file mode 100755 index 0000000000..d6657f5df6 --- /dev/null +++ b/tests/kolainst/destructive/container-image @@ -0,0 +1,78 @@ +#!/bin/bash +# +# Copyright (C) 2021 Red Hat, Inc. +# +# This library is free software; you can redistribute it and/or +# modify it under the terms of the GNU Lesser General Public +# License as published by the Free Software Foundation; either +# version 2 of the License, or (at your option) any later version. +# +# This library is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU +# Lesser General Public License for more details. +# +# You should have received a copy of the GNU Lesser General Public +# License along with this library; if not, write to the +# Free Software Foundation, Inc., 59 Temple Place - Suite 330, +# Boston, MA 02111-1307, USA. + +set -euo pipefail + +. ${KOLA_EXT_DATA}/libtest.sh + +set -x + +libtest_prepare_offline +cd $(mktemp -d) + +case "${AUTOPKGTEST_REBOOT_MARK:-}" in + "") + # Test rebase + checksum=$(rpm-ostree status --json | jq -r '.deployments[0].checksum') + rpm-ostree ex-container export --repo=/ostree/repo ${checksum} containers-storage:localhost/fcos + rpm-ostree rebase containers-storage:localhost/fcos:latest --experimental + rpmostree_assert_status ".deployments[0][\"container-image-reference\"] == \"containers-storage:localhost/fcos:latest\"" + rpmostree_assert_status ".deployments[0][\"checksum\"] == \"${checksum}\"" + echo "ok rebase to container image reference" + + /tmp/autopkgtest-reboot 1 + ;; + 1) + if rpm-ostree deploy 42 2>err.txt; then + fatal "unexpectedly deployed version from container image reference" + fi + assert_file_has_content err.txt 'Cannot look up version while tracking a container image reference' + echo "ok cannot deploy when tracking container image" + + # Test layering + if rpm -q foo 2>/dev/null; then + fatal "found foo" + fi + rpm-ostree install ${KOLA_EXT_DATA}/rpm-repos/0/packages/x86_64/foo-1.2-3.x86_64.rpm + echo "ok layering package" + + # Test upgrade + rpm-ostree upgrade > out.txt; + assert_file_has_content out.txt "No upgrade available." + echo "ok no upgrade available" + + # Create a new ostree commit containing foo and export the commit as a container image + with_foo_commit=$(rpm-ostree status --json | jq -r '.deployments[0].checksum') + ostree refs ${with_foo_commit} --create vmcheck_tmp/new_update + new_commit=$(ostree commit -b vmcheck --tree=ref=vmcheck_tmp/new_update) + rpm-ostree ex-container export --repo=/ostree/repo ${new_commit} containers-storage:localhost/fcos + + rpm-ostree uninstall foo + rpm-ostree upgrade + rpmostree_assert_status ".deployments[0][\"checksum\"] == \"${new_commit}\"" + + /tmp/autopkgtest-reboot 2 + ;; + 2) + rpmostree_assert_status ".deployments[0][\"container-image-reference\"] == \"containers-storage:localhost/fcos:latest\"" + assert_streq $(rpm -q foo) foo-1.2-3.x86_64 + echo "ok upgrade" + ;; + *) echo "unexpected mark: ${AUTOPKGTEST_REBOOT_MARK}"; exit 1;; +esac From ff130aac2ae8f40feb43ddc9c1320fb54d4b77b8 Mon Sep 17 00:00:00 2001 From: Kelvin Fan Date: Tue, 13 Jul 2021 15:45:21 -0400 Subject: [PATCH 2/2] upgrade: Check imgref digest before pulling image Previously, `rpm-ostree upgrade` was re-pulling a container image each time, and throwing 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 | 15 ++++++++++- rust/src/sysroot_upgrade.rs | 20 ++++++++++++--- src/daemon/rpmostree-sysroot-upgrader.cxx | 21 +++++++++++++-- src/libpriv/rpmostree-origin.cxx | 31 ++++++++++++++++++++++- src/libpriv/rpmostree-origin.h | 7 +++++ 5 files changed, 86 insertions(+), 8 deletions(-) diff --git a/rust/src/lib.rs b/rust/src/lib.rs index d4562db761..7c264f1576 100644 --- a/rust/src/lib.rs +++ b/rust/src/lib.rs @@ -128,9 +128,22 @@ pub mod ffi { fn cliwrap_destdir() -> String; } + /// `ContainerImport` is currently identical to ostree-rs-ext's `Import` struct, because + /// 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(crate) struct ContainerImport { + pub ostree_commit: String, + pub image_digest: String, + } + // sysroot_upgrade.rs extern "Rust" { - fn import_container(sysroot: Pin<&mut OstreeSysroot>, imgref: String) -> Result; + fn import_container( + sysroot: Pin<&mut OstreeSysroot>, + imgref: String, + ) -> Result>; + + fn fetch_digest(imgref: String) -> Result; } // core.rs diff --git a/rust/src/sysroot_upgrade.rs b/rust/src/sysroot_upgrade.rs index 1cc4889232..c61756cb86 100644 --- a/rust/src/sysroot_upgrade.rs +++ b/rust/src/sysroot_upgrade.rs @@ -3,22 +3,34 @@ // SPDX-License-Identifier: Apache-2.0 OR MIT use crate::cxxrsutil::*; +use crate::ffi::ContainerImport; use anyhow::{Context, Result}; use std::convert::TryInto; use std::pin::Pin; /// Import ostree commit in container image using ostree-rs-ext's API. -pub fn import_container( +pub(crate) 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(crate) 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 398f7a5617..57d4162109 100644 --- a/src/daemon/rpmostree-sysroot-upgrader.cxx +++ b/src/daemon/rpmostree-sysroot-upgrader.cxx @@ -448,9 +448,26 @@ 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)); + if (strstr (refspec, "@")) + return glnx_throw (error, "Pinned to specific container image digest, cannot upgrade"); - new_base_rev = strdup (commit.c_str()); + /* 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->original_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)); + + rpmostree_origin_set_container_image_reference_digest (self->original_origin, import->image_digest.c_str()); + new_base_rev = strdup (import->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 3f61420fe8..0531cce692 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; @@ -115,7 +118,7 @@ rpmostree_origin_parse_keyfile (GKeyFile *origin, ret->cached_unconfigured_state = g_key_file_get_string (ret->kf, "origin", "unconfigured-state", NULL); - /* Note that the refspec type can be inferred from the key in the orgin file, where + /* Note that the refspec type can be inferred from the key in the origin file, where * the `RPMOSTREE_REFSPEC_OSTREE_ORIGIN_KEY` and `RPMOSTREE_REFSPEC_OSTREE_BASE_ORIGIN_KEY` keys * may correspond to either TYPE_OSTREE or TYPE_CHECKSUM (further classification is required), and * `RPMOSTREE_REFSPEC_CONTAINER_ORIGIN_KEY` always only corresponds to TYPE_CONTAINER. */ @@ -151,6 +154,8 @@ rpmostree_origin_parse_keyfile (GKeyFile *origin, { ret->refspec_type = RPMOSTREE_REFSPEC_TYPE_CONTAINER; ret->cached_refspec = util::move_nullify (imgref); + + ret->cached_digest = g_key_file_get_string (ret->kf, "origin", "container-image-reference-digest", NULL); } else { @@ -210,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) { @@ -465,6 +476,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, @@ -543,6 +571,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 2702431798..56d81cf382 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); @@ -134,6 +137,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,