engineering: Implement select_servicing_type() (#374)

* broad strokes

* progress

* progress

* fix comment

* impl select_servicing_stage

* nit

* new

* move back to context

* make skip logs trace

* enable runtime test

* remove confusing comment

* test runtime updates

* remove newline

* remove newline

* remove outdated comment

Author: ayaegashi

.pipelines/templates/stages/testing_rollback/testing-template.yml

@@ -32,7 +32,7 @@ parameters:
     displayName: Agent pool
     type: string
     default: "trident-ubuntu-1es-pool-eastus2"
-  
+
   - name: skipExtensionTesting
     displayName: "Skip extension testing"
     type: string
@@ -46,7 +46,7 @@ parameters:
   - name: skipRuntimeUpdateTesting
     displayName: "Skip runtime update testing"
     type: string
-    default: "true"
+    default: "false"
 
 jobs:
   - job: RollbackTesting_${{ replace(parameters.flavor, '-', '_') }}
@@ -115,7 +115,7 @@ jobs:
 
       - bash: |
           set -eux
-          
+
           STORM_DYNAMIC_FLAGS=""
           if [ "${{ parameters.verboseLogging }}" == "True" ]; then
             STORM_DYNAMIC_FLAGS="$STORM_DYNAMIC_FLAGS --verbose"

.pipelines/templates/stages/testing_rollback/vm-testing.yml

@@ -119,6 +119,7 @@ stages:
               platform: qemu
               flavor: qemu-grub
               skipExtensionTesting: false
+              skipRuntimeUpdateTesting: false
       - ${{ if eq(parameters.includeQemu, true) }}:
           - template: testing-template.yml
             parameters:
@@ -129,6 +130,7 @@ stages:
               # qemu image is grub + root verity. in this setup, Image Customizer cannot add
               # an extension to the servicing qcow2
               skipExtensionTesting: true
+              skipRuntimeUpdateTesting: false
       - ${{ if eq(parameters.includeUKI, true) }}:
           - template: testing-template.yml
             parameters:
@@ -137,3 +139,4 @@ stages:
               platform: qemu
               flavor: uki
               skipExtensionTesting: false
+              skipRuntimeUpdateTesting: false

crates/trident/src/subsystems/storage/image.rs renamed to crates/trident/src/engine/context/image.rs

@@ -11,28 +11,31 @@ use crate::engine::EngineContext;
 /// requests an OS image. If yes, update is needed, unless the old Host
 /// Configuration also requested an OS image and the URLs and SHA256 checksums
 /// are the same.
-pub(super) fn ab_update_required(ctx: &EngineContext) -> Result<bool, TridentError> {
-    debug!("Checking OS image to determine if an A/B update is required");
-    // Otherwise, continue checking OS images
-    match (&ctx.spec_old.image, &ctx.spec.image) {
-        // If OS image is not requested in the new spec, no update is needed.
-        (None, None) => Ok(false),
-
-        (None, Some(_)) => {
-            // This is most likely an offline-init's first update.
-            Ok(true)
-        }
-
-        // Update if the sha384 has changed (including if one is 'ignored'), or both are ignored but
-        // the URL has changed.
-        (Some(old_os_image), Some(new_os_image)) => Ok(old_os_image.sha384 != new_os_image.sha384
-            || old_os_image.sha384 == ImageSha384::Ignored && old_os_image.url != new_os_image.url),
-
-        (Some(_), None) => {
-            // Return an error if the old spec requests an OS image but the new spec does not.
-            Err(TridentError::new(InvalidInputError::from(
-                HostConfigurationDynamicValidationError::DeployPartitionImagesAfterOsImage,
-            )))
+impl EngineContext {
+    pub(crate) fn ab_update_required(&self) -> Result<bool, TridentError> {
+        debug!("Checking OS image to determine if an A/B update is required");
+        match (&self.spec_old.image, &self.spec.image) {
+            // If OS image is not requested in the new spec, no update is needed.
+            (None, None) => Ok(false),
+
+            (None, Some(_)) => {
+                // This is most likely an offline-init's first update.
+                Ok(true)
+            }
+
+            // Update if the sha384 has changed (including if one is 'ignored'), or both are ignored but
+            // the URL has changed.
+            (Some(old_os_image), Some(new_os_image)) => Ok(old_os_image.sha384
+                != new_os_image.sha384
+                || old_os_image.sha384 == ImageSha384::Ignored
+                    && old_os_image.url != new_os_image.url),
+
+            (Some(_), None) => {
+                // Return an error if the old spec requests an OS image but the new spec does not.
+                Err(TridentError::new(InvalidInputError::from(
+                    HostConfigurationDynamicValidationError::DeployPartitionImagesAfterOsImage,
+                )))
+            }
         }
     }
 }
@@ -182,7 +185,7 @@ mod tests {
             image: Some(os_image),
             ..Default::default()
         };
-        assert!(!ab_update_required(&ctx).unwrap());
+        assert!(!ctx.ab_update_required().unwrap());
 
         // Test case #2: If OS image has changed, return true.
         let mut hc_os_image_updated = hc_os_image.clone();
@@ -192,6 +195,6 @@ mod tests {
             sha384: ImageSha384::Ignored,
         });
         ctx.spec = hc_os_image_updated;
-        assert!(ab_update_required(&ctx).unwrap());
+        assert!(ctx.ab_update_required().unwrap());
     }
 }

crates/trident/src/engine/context/mod.rs

@@ -20,7 +20,7 @@ use crate::osimage::OsImage;
 
 #[allow(dead_code)]
 pub mod filesystem;
-
+pub(crate) mod image;
 #[cfg(test)]
 mod test_utils;
 

crates/trident/src/engine/mod.rs

@@ -7,7 +7,7 @@ use std::{
 };
 
 use chrono::Utc;
-use log::{debug, error, info, warn};
+use log::{debug, error, info, trace, warn};
 
 use osutils::{dependencies::Dependency, path::join_relative};
 use trident_api::{
@@ -81,12 +81,10 @@ pub(crate) trait Subsystem: Send {
     // TODO: Implement dependencies
     // fn dependencies(&self) -> &'static [&'static str];
 
-    /// Select the servicing type based on the Host Status and Host Configuration.
-    fn select_servicing_type(&self, ctx: &EngineContext) -> Result<ServicingType, TridentError> {
-        if is_default(&ctx.spec_old) {
-            return Ok(ServicingType::CleanInstall);
-        }
-        Ok(ServicingType::AbUpdate)
+    /// Select the servicing type based on information in the Host Status
+    /// relevant to a subsystem.
+    fn select_servicing_type(&self, _ctx: &EngineContext) -> Result<ServicingType, TridentError> {
+        Ok(ServicingType::NoActiveServicing)
     }
 
     /// Servicing types on which a subsystem may run. By default, all subsystems
@@ -256,6 +254,28 @@ fn persist_background_log_and_metrics(
     }
 }
 
+fn select_servicing_type(
+    subsystems: &[Box<dyn Subsystem>],
+    ctx: &EngineContext,
+) -> Result<ServicingType, TridentError> {
+    if is_default(&ctx.spec_old) {
+        return Ok(ServicingType::CleanInstall);
+    } else if ctx
+        .ab_update_required()
+        .message("Failed to determine if A/B update is required")?
+    {
+        return Ok(ServicingType::AbUpdate);
+    }
+
+    Ok(subsystems
+        .iter()
+        .map(|m| m.select_servicing_type(ctx))
+        .collect::<Result<Vec<_>, TridentError>>()?
+        .into_iter()
+        .max()
+        .unwrap_or(ServicingType::NoActiveServicing))
+}
+
 #[tracing::instrument(skip_all)]
 fn validate_host_config(
     subsystems: &[Box<dyn Subsystem>],
@@ -273,7 +293,7 @@ fn validate_host_config(
                 subsystem.name()
             ))?;
         } else {
-            debug!(
+            trace!(
                 "Skipping step 'Validate' for subsystem '{}'",
                 subsystem.name()
             );
@@ -296,7 +316,7 @@ fn prepare(subsystems: &mut [Box<dyn Subsystem>], ctx: &EngineContext) -> Result
                 subsystem.name()
             ))?;
         } else {
-            debug!(
+            trace!(
                 "Skipping step 'Prepare' for subsystem '{}'",
                 subsystem.name()
             );
@@ -335,7 +355,7 @@ fn provision(
                 subsystem.name()
             ))?;
         } else {
-            debug!(
+            trace!(
                 "Skipping step 'Provision' for subsystem '{}'",
                 subsystem.name()
             );
@@ -377,7 +397,7 @@ fn configure(
                 subsystem.name()
             ))?;
         } else {
-            debug!(
+            trace!(
                 "Skipping step 'Configure' for subsystem '{}'",
                 subsystem.name()
             );
@@ -404,7 +424,7 @@ fn update_host_configuration(
                 subsystem.name()
             ))?;
         } else {
-            debug!(
+            trace!(
                 "Skipping step 'Update Host Configuration' for subsystem '{}'",
                 subsystem.name()
             );
@@ -427,7 +447,7 @@ fn clean_up(subsystems: &[Box<dyn Subsystem>], ctx: &EngineContext) -> Result<()
                 subsystem.name()
             ))?;
         } else {
-            debug!(
+            trace!(
                 "Skipping step 'Clean Up' for subsystem '{}'",
                 subsystem.name()
             );

crates/trident/src/engine/runtime_update.rs

@@ -25,7 +25,7 @@ use super::Subsystem;
 /// - ctx: EngineContext.
 /// - state: A mutable reference to the DataStore.
 /// - sender: Optional mutable reference to the gRPC sender.
-#[tracing::instrument(skip_all, fields(servicing_type = format!("{:?}", ctx.servicing_type)))]
+#[tracing::instrument(skip_all, fields(servicing_type = format!("{:?}", ServicingType::RuntimeUpdate)))]
 pub(crate) fn stage_update(
     subsystems: &mut [Box<dyn Subsystem>],
     ctx: EngineContext,
@@ -62,7 +62,6 @@ pub(crate) fn stage_update(
         // Update spec inside the Host Status with the new Host Configuration
         // (stored in ctx.spec).
         hs.spec = ctx.spec;
-        // Update spec_old to the previous spec (ctx.spec_old == hs.spec)
         hs.spec_old = ctx.spec_old;
     })?;
     #[cfg(feature = "grpc-dangerous")]

crates/trident/src/engine/update.rs

@@ -72,13 +72,7 @@ pub(crate) fn update(
     }
 
     debug!("Determining servicing type of required update");
-    let servicing_type = subsystems
-        .iter()
-        .map(|m| m.select_servicing_type(&ctx))
-        .collect::<Result<Vec<_>, TridentError>>()?
-        .into_iter()
-        .max()
-        .unwrap_or(ServicingType::NoActiveServicing);
+    let servicing_type = engine::select_servicing_type(&subsystems, &ctx)?;
     match servicing_type {
         ServicingType::NoActiveServicing => {
             info!("No update servicing required");

crates/trident/src/subsystems/extensions/mod.rs

@@ -104,6 +104,15 @@ impl Subsystem for ExtensionsSubsystem {
         RUNS_ON_ALL
     }
 
+    fn select_servicing_type(&self, ctx: &EngineContext) -> Result<ServicingType, TridentError> {
+        if ctx.spec.os.sysexts != ctx.spec_old.os.sysexts
+            || ctx.spec.os.confexts != ctx.spec_old.os.confexts
+        {
+            return Ok(ServicingType::RuntimeUpdate);
+        }
+        Ok(ServicingType::NoActiveServicing)
+    }
+
     // prepare() is only called during runtime updates, so as to download the
     // extension files during Stage.
     fn prepare(&mut self, ctx: &EngineContext) -> Result<(), TridentError> {

crates/trident/src/subsystems/hooks.rs

@@ -20,6 +20,7 @@ use trident_api::{
         ROOT_MOUNT_POINT_PATH,
     },
     error::{InvalidInputError, ReportError, ServicingError, TridentError, TridentResultExt},
+    is_default,
     status::ServicingType,
 };
 
@@ -46,6 +47,13 @@ impl Subsystem for HooksSubsystem {
         self.writable_etc_overlay
     }
 
+    fn select_servicing_type(&self, ctx: &EngineContext) -> Result<ServicingType, TridentError> {
+        if !is_default(&ctx.spec.scripts) {
+            return Ok(ServicingType::AbUpdate);
+        }
+        Ok(ServicingType::NoActiveServicing)
+    }
+
     fn validate_host_config(&self, ctx: &EngineContext) -> Result<(), TridentError> {
         // Ensure that all scripts that should be run and have a path actually exist
         for script in ctx

crates/trident/src/subsystems/network.rs

@@ -26,6 +26,13 @@ impl Subsystem for NetworkSubsystem {
         RUNS_ON_ALL
     }
 
+    fn select_servicing_type(&self, ctx: &EngineContext) -> Result<ServicingType, TridentError> {
+        if ctx.spec.os.netplan != ctx.spec_old.os.netplan {
+            return Ok(ServicingType::RuntimeUpdate);
+        }
+        Ok(ServicingType::NoActiveServicing)
+    }
+
     fn prepare(&mut self, ctx: &EngineContext) -> Result<(), TridentError> {
         if ctx.servicing_type == ServicingType::RuntimeUpdate
             && ctx.spec.os.netplan != ctx.spec_old.os.netplan