diff --git a/release-notes/unreleased/174-no-reconcile-panic.md b/release-notes/unreleased/174-no-reconcile-panic.md new file mode 100644 index 0000000..56786f1 --- /dev/null +++ b/release-notes/unreleased/174-no-reconcile-panic.md @@ -0,0 +1,43 @@ +# Release Notes for #174 follow-up: no panic on the Knative reconcile path + +## Bug Fix + +### What Changed + +`RegistrationAction::AlreadyLatest` now carries the deployment id it was decided from, so the +Knative reconciler reads it out of the verdict instead of unwrapping the `Option` it had just +passed in. + +The planner reaches `AlreadyLatest` only after matching a recorded id against Restate's usage +map, so the id is necessarily present — but that reasoning lived in an `.expect()` at the call +site rather than in the type: + +```rust +let recorded_id = recorded_id.cloned().expect("AlreadyLatest implies an id"); +``` + +The invariant held, so this was not reachable in practice. It was still a panic sited on the +reconcile path, where the cost of being wrong is not an error condition but the controller +task unwinding, and it was only reachable there because the id had to be recovered separately +from the decision that depended on it. Moving the id into the variant makes the bad state +unrepresentable and removes the assertion entirely. + +The ReplicaSet path compared against the variant rather than destructuring it, so it changes +to `matches!` and is otherwise untouched. + +### Why This Matters + +A `RestateDeployment` in the steady state — already latest, nothing to tell Restate — is the +most frequently taken branch in the Knative reconciler. An assertion there is the one least +likely to be exercised by anything but production. + +### Impact on Users + +None. No behaviour change, no CRD change, and no new status field or annotation: the panic was +unreachable, and this removes the possibility rather than a symptom. + +### Related Issues + +- Issue #174: Support rolling RestateDeployments back to a previously registered revision +- Review feedback on #178 (`reconcilers/knative.rs`): "i think we shouldn't panic here, it's on + the reconcile path" diff --git a/src/controllers/restatedeployment/controller.rs b/src/controllers/restatedeployment/controller.rs index 62c413c..0992349 100644 --- a/src/controllers/restatedeployment/controller.rs +++ b/src/controllers/restatedeployment/controller.rs @@ -496,7 +496,7 @@ impl RestateDeployment { }); } - if action != RegistrationAction::AlreadyLatest { + if !matches!(action, RegistrationAction::AlreadyLatest { .. }) { let valid = async { if let Some(cluster_name) = &self.spec.restate.register.cluster { // wait for the cluster to be ready before registering to it diff --git a/src/controllers/restatedeployment/reconcilers/knative.rs b/src/controllers/restatedeployment/reconcilers/knative.rs index 67840e9..b361052 100644 --- a/src/controllers/restatedeployment/reconcilers/knative.rs +++ b/src/controllers/restatedeployment/reconcilers/knative.rs @@ -671,14 +671,13 @@ async fn register_or_promote_deployment( match &action { // Restate already sends new invocations here; nothing to say to it. - registration::RegistrationAction::AlreadyLatest => { - let recorded_id = recorded_id.cloned().expect("AlreadyLatest implies an id"); + registration::RegistrationAction::AlreadyLatest { deployment_id } => { trace!( - deployment_id = %recorded_id, + deployment_id = %deployment_id, "Configuration's deployment is already latest" ); - annotate_configuration(ctx, namespace, config, &recorded_id).await?; - return Ok(recorded_id); + annotate_configuration(ctx, namespace, config, deployment_id).await?; + return Ok(deployment_id.clone()); } registration::RegistrationAction::Conflict => { return Err(Error::DeploymentNotLatest { diff --git a/src/controllers/restatedeployment/registration.rs b/src/controllers/restatedeployment/registration.rs index ef75bfd..6798634 100644 --- a/src/controllers/restatedeployment/registration.rs +++ b/src/controllers/restatedeployment/registration.rs @@ -55,8 +55,10 @@ pub(super) enum RegistrationAction { /// it, intact. Carries the id that currently holds latest, for the event the operator /// emits afterwards. Promote { superseded_by: String }, - /// Already serving new invocations. Leave it alone. - AlreadyLatest, + /// Already serving new invocations. Leave it alone. Carries the recorded id, which this + /// variant can only be reached with — holding it here is what saves the caller from + /// re-deriving it from an `Option` it has already proven to be `Some`. + AlreadyLatest { deployment_id: String }, /// Our deployment is registered but superseded, and no version of this RestateDeployment /// is serving these services either — so something outside it is. Forcing here would /// start a promotion war between two controllers, each bumping revisions to take the @@ -103,7 +105,9 @@ pub(super) fn plan_registration( }; if usage.latest_for_service { - return RegistrationAction::AlreadyLatest; + return RegistrationAction::AlreadyLatest { + deployment_id: recorded_id.to_owned(), + }; } // Our deployment exists but is superseded. It can only have been superseded by whoever @@ -341,6 +345,12 @@ mod tests { } } + fn already_latest(deployment_id: &str) -> RegistrationAction { + RegistrationAction::AlreadyLatest { + deployment_id: deployment_id.into(), + } + } + #[test] fn nothing_recorded_registers() { assert_eq!( @@ -364,10 +374,24 @@ mod tests { let deployments = DeploymentUsageMap::from([("dp_v1".into(), usage(true, 0))]); assert_eq!( plan_registration(Some("dp_v1"), &deployments, || owned(&["dp_v1"])), - RegistrationAction::AlreadyLatest + already_latest("dp_v1") ); } + /// The id travels with the verdict so the caller never has to assert an invariant the + /// planner already established. Previously the Knative path recovered it by unwrapping + /// the same `Option` it had passed in, which put a panic on the reconcile path. + #[test] + fn already_latest_carries_the_recorded_id() { + let deployments = DeploymentUsageMap::from([("dp_v1".into(), usage(true, 3))]); + let RegistrationAction::AlreadyLatest { deployment_id } = + plan_registration(Some("dp_v1"), &deployments, || owned(&["dp_v1"])) + else { + panic!("expected AlreadyLatest"); + }; + assert_eq!(deployment_id, "dp_v1"); + } + /// The steady state must not pay for the cluster-wide reflector walk that only a /// rollback needs. #[test] @@ -473,7 +497,7 @@ mod tests { fn only_promotion_overwrites() { assert_eq!(RegistrationAction::Register.overwrite(), Overwrite::No); assert_eq!(promote("dp_v2").overwrite(), Overwrite::Yes); - assert_eq!(RegistrationAction::AlreadyLatest.overwrite(), Overwrite::No); + assert_eq!(already_latest("dp_v1").overwrite(), Overwrite::No); assert_eq!(RegistrationAction::Conflict.overwrite(), Overwrite::No); assert!(!Overwrite::No.force()); assert!(Overwrite::Yes.force());