feat(registry): Allow advancing standard engine replica version after a roll back.

rs/registry/canister/src/mutations/do_update_standard_engine_replica_version.rs

@@ -24,10 +24,11 @@ use serde::Serialize;
 /// 1. Change deployment_progress (usually increase, but decrease is allowed).
 /// 2. Start a new deployment.
 ///
-/// The latter is only allowed if the previous/current deployment is complete
-/// (deployment_progress == 1.0, or there is no previous deployment). The new
-/// deployment must start where the previous one left off, i.e. the new
-/// old_replica_version_id must match the current/old new_replica_version_id.
+/// The latter is only allowed when the fleet is not previously split. That is,
+/// the previous deployment_progress must be either 1.0 or 0.0. Typically, it
+/// would be 1.0, but 0.0 is also allowed in order to support rolling back.
+/// Either way, we say that there is a "unique" standard engine replica version.
+/// Furthermore, the next old_replica_version_id must be this unique version.
 impl Registry {
     pub fn do_update_standard_engine_replica_version(
         &mut self,
@@ -68,21 +69,28 @@ impl Registry {
             return;
         }
 
-        if old_record.deployment_progress < 1.0 {
-            panic!(
-                "Invalid StandardEngineReplicaVersionRecord transition: the current deployment \
-                 is still in progress, so a new deployment cannot be started yet. Current \
-                 record: {old_record:?}. Proposed new record: {new_record:?}.",
-            );
-        }
+        // Starting a new deployment is only allowed once the current one has
+        // fully settled onto one of its two versions.
+        let currently_running_replica_version_id = match old_record.deployment_progress {
+            1.0 => &old_record.new_replica_version_id,
+            0.0 => &old_record.old_replica_version_id,
+            _ => panic!(
+                "Invalid StandardEngineReplicaVersionRecord transition: the fleet is \
+                 currently split (that is, deployment_progress {} is strictly between \
+                 0.0 and 1.0). While the fleet is split, only deployment_progress \
+                 changes are allowed. Current value: {old_record:?}. Requested new \
+                 value: {new_record:?}.",
+                old_record.deployment_progress,
+            ),
+        };
 
         // Based on deployment_progress, looks like we are starting a new deployment.
         // Here, we check the new old_replica_version_id.
         assert_eq!(
-            new_record.old_replica_version_id, old_record.new_replica_version_id,
+            &new_record.old_replica_version_id, currently_running_replica_version_id,
             "Invalid StandardEngineReplicaVersionRecord transition: cannot skip a version. The \
-             next deployment's old_replica_version_id must equal the current deployment's \
-             new_replica_version_id.",
+             next deployment's old_replica_version_id must equal the previous (unique) standard \
+             engine replica version.",
         );
     }
 
@@ -283,7 +291,7 @@ mod tests {
         // Step 1: Prepare the world.
         let mut registry = REGISTRY.clone();
 
-        // Step 1.1: v2 is fully deployed (previously, we were on Replica v1).
+        // Step 1.1: v2 is fully deployed (previously, we were on replica v1).
         registry.do_update_standard_engine_replica_version(
             UpdateStandardEngineReplicaVersionPayload {
                 new_replica_version_id: "v2".into(),
@@ -321,7 +329,77 @@ mod tests {
     }
 
     #[test]
-    #[should_panic(expected = "the current deployment is still in progress")]
+    fn should_allow_starting_a_different_deployment_once_fully_rolled_back() {
+        // Step 1: Prepare the world.
+        let mut registry = REGISTRY.clone();
+
+        // Step 1.1: Fully roll back v1 -> v2 (deployment_progress goes to 0.0,
+        // i.e. all engines are back on v1).
+        registry.do_update_standard_engine_replica_version(
+            UpdateStandardEngineReplicaVersionPayload {
+                new_replica_version_id: "v2".into(),
+                old_replica_version_id: "v1".into(),
+                deployment_progress: 0.0,
+            },
+        );
+
+        // Step 2: Run the code under test. Start a different deployment:
+        // v1 -> v3 (not v2, since v2 was abandoned).
+        registry.do_update_standard_engine_replica_version(
+            UpdateStandardEngineReplicaVersionPayload {
+                new_replica_version_id: "v3".into(),
+                old_replica_version_id: "v1".into(),
+                deployment_progress: 0.1,
+            },
+        );
+
+        // Step 3: Verify result(s).
+
+        // Step 3.1: The previous statement did not panic, even though the
+        // versions changed. This is allowed, because previously,
+        // deployment_progress was 0.0, i.e. all engines had settled back
+        // onto v1.
+
+        // Step 3.2: The contents of registry reflects the change we tried to make.
+        assert_standard_engine_replica_version_record_eq(
+            &registry,
+            StandardEngineReplicaVersionRecord {
+                new_replica_version_id: "v3".to_string(),
+                old_replica_version_id: "v1".to_string(),
+                deployment_progress: 0.1,
+            },
+        );
+    }
+
+    #[test]
+    #[should_panic(expected = "fleet is currently split")]
+    fn should_panic_when_changing_versions_while_partially_rolled_back() {
+        // Step 1: Prepare the world. Partially roll back v1 -> v2 (progress
+        // is neither 0.0 nor 1.0).
+        let mut registry = REGISTRY.clone();
+        registry.do_update_standard_engine_replica_version(
+            UpdateStandardEngineReplicaVersionPayload {
+                new_replica_version_id: "v2".into(),
+                old_replica_version_id: "v1".into(),
+                deployment_progress: 0.5,
+            },
+        );
+
+        // Step 2: Run the code under test. Try to switch to v1 -> v3 instead.
+        registry.do_update_standard_engine_replica_version(
+            UpdateStandardEngineReplicaVersionPayload {
+                new_replica_version_id: "v3".into(),
+                old_replica_version_id: "v1".into(),
+                deployment_progress: 0.1,
+            },
+        );
+
+        // Step 3: Verify result(s).
+        // The assertion is at the top: #[should_panic(...)].
+    }
+
+    #[test]
+    #[should_panic(expected = "fleet is currently split")]
     fn should_panic_when_changing_versions_before_fully_deployed() {
         // Step 1: Prepare the world. Partially deploy v1 -> v2
         // (deployment_progress isn't 1.0 yet).
@@ -375,6 +453,39 @@ mod tests {
         // The assertion is at the top: #[should_panic(...)].
     }
 
+    /// The counterpart of the previous test, but for the other way that the
+    /// fleet can end up on a unique replica version: rolling all the way back
+    /// instead of all the way forward.
+    #[test]
+    #[should_panic(expected = "cannot skip a version")]
+    fn should_panic_when_skipping_a_version_after_rolling_back() {
+        // Step 1: Prepare the world. Fully roll back v1 -> v2. That is, all
+        // engines end up (back) on v1, and v2 gets abandoned.
+        let mut registry = REGISTRY.clone();
+        registry.do_update_standard_engine_replica_version(
+            UpdateStandardEngineReplicaVersionPayload {
+                new_replica_version_id: "v2".into(),
+                old_replica_version_id: "v1".into(),
+                deployment_progress: 0.0,
+            },
+        );
+
+        // Step 2: Run the code under test. The new old_replica_version_id
+        // ("v2") is the version that was just rolled back away from. The
+        // version that engines are actually running is v1, so this deployment
+        // would skip over it.
+        registry.do_update_standard_engine_replica_version(
+            UpdateStandardEngineReplicaVersionPayload {
+                new_replica_version_id: "v3".into(),
+                old_replica_version_id: "v2".into(),
+                deployment_progress: 0.1,
+            },
+        );
+
+        // Step 3: Verify result(s).
+        // The assertion is at the top: #[should_panic(...)].
+    }
+
     #[test]
     #[should_panic(expected = "Using a version that isn't elected.")]
     fn should_panic_if_new_version_not_elected() {

rs/registry/canister/unreleased_changelog.md

@@ -23,6 +23,10 @@ on the process that this file is part of, see
 
 ## Changed
 
+* `UpdateStandardEngineReplicaVersion` can now start a new deployment after the previous one has been
+  fully rolled back (`deployment_progress == 0.0`), not just after it has been fully rolled forward
+  (`deployment_progress == 1.0`).
+
 ## Deprecated
 
 ## Removed