chore: skip switchover for safe metadata-only in-place updates (#10312)

Author: weicao

pkg/controller/instanceset/in_place_update_util.go

@@ -258,6 +258,79 @@ func equalBasicInPlaceFields(old, new *corev1.Pod) bool {
 	return true
 }
 
+// safeMetadataOnlyInPlaceUpdate returns true when the only difference between
+// old and new is a Pod metadata patch (annotations and/or labels) that has no
+// effect on the running database process or availability. In that case the
+// caller can skip the lifecycle switchover that is normally invoked before an
+// in-place pod update.
+//
+// Specifically, it returns true if and only if:
+//   - old and new are otherwise equal when annotations and labels are ignored
+//     (basic spec fields + container resources match)
+//   - metadata has actually changed (do not vacuously skip switchover when
+//     there is no real diff)
+//   - the annotation diff does not include constant.RestartAnnotationKey, the
+//     explicit restart trigger which always implies a process restart
+//   - the annotation diff does not touch any key prefixed with
+//     constant.UpgradeRestartAnnotationKey ("config.kubeblocks.io/restart"),
+//     which the parameters controller writes via GenerateUniqKeyWithConfig to
+//     signal a restart-required config change. filterInPlaceFields keeps these
+//     keys alongside RestartAnnotationKey, so a diff on any of them must fall
+//     through to switchover to honor the restart contract.
+//
+// Label changes are allowed because pure label patches do not restart the
+// container, do not change the spec, and do not touch the database process.
+// Service-selector routing impact is a network-layer concern and is not
+// resolved by invoking lifecycle switchover. Role-label patches are likewise
+// state synchronization, not a trigger for additional switchover.
+func safeMetadataOnlyInPlaceUpdate(old, new *corev1.Pod) bool {
+	oldCopy := old.DeepCopy()
+	newCopy := new.DeepCopy()
+	oldCopy.Annotations = nil
+	newCopy.Annotations = nil
+	oldCopy.Labels = nil
+	newCopy.Labels = nil
+	if !equalBasicInPlaceFields(oldCopy, newCopy) || !equalResourcesInPlaceFields(oldCopy, newCopy) {
+		return false
+	}
+	if equalField(old.Annotations, new.Annotations) && equalField(old.Labels, new.Labels) {
+		return false
+	}
+	if !equalField(old.Annotations[constant.RestartAnnotationKey], new.Annotations[constant.RestartAnnotationKey]) {
+		return false
+	}
+	if !equalUpgradeRestartAnnotations(old.Annotations, new.Annotations) {
+		return false
+	}
+	return true
+}
+
+// equalUpgradeRestartAnnotations returns true when old and new agree on every
+// annotation whose key is prefixed with constant.UpgradeRestartAnnotationKey
+// ("config.kubeblocks.io/restart"). The parameters controller writes these via
+// GenerateUniqKeyWithConfig to signal that a config change requires a pod
+// restart; filterInPlaceFields keeps them in the in-place template diff, so
+// any add/change/remove on these keys must invoke switchover.
+func equalUpgradeRestartAnnotations(old, new map[string]string) bool {
+	keys := make(map[string]struct{})
+	for k := range old {
+		if strings.HasPrefix(k, constant.UpgradeRestartAnnotationKey) {
+			keys[k] = struct{}{}
+		}
+	}
+	for k := range new {
+		if strings.HasPrefix(k, constant.UpgradeRestartAnnotationKey) {
+			keys[k] = struct{}{}
+		}
+	}
+	for k := range keys {
+		if old[k] != new[k] {
+			return false
+		}
+	}
+	return true
+}
+
 // equalResourcesInPlaceFields checks if the desired values of pod resources are equal to their current actual values.
 // If they are equal, it returns true. Containers in 'old' that are not recognized (they may have been injected by external mutating admission webhooks)
 // will not participate in the comparison.

pkg/controller/instanceset/instance_util_test.go

@@ -99,6 +99,134 @@ var _ = Describe("instance util test", func() {
 				return item
 			})).ShouldNot(Succeed())
 		})
+
+		It("identifies safe metadata-only in-place updates per process-impact semantic", func() {
+			basePod := builder.NewPodBuilder(namespace, name+"-0").
+				AddAnnotations("kept", "value", constant.CMInsConfigurationHashLabelKey, "old-hash").
+				AddLabels("app", "valkey").
+				SetContainers(template.Spec.Containers).
+				GetObject()
+
+			positiveCases := []struct {
+				name   string
+				mutate func(*corev1.Pod)
+			}{{
+				name: "config-hash annotation patch",
+				mutate: func(pod *corev1.Pod) {
+					pod.Annotations[constant.CMInsConfigurationHashLabelKey] = "new-hash"
+				},
+			}, {
+				name: "non-restart annotation added",
+				mutate: func(pod *corev1.Pod) {
+					pod.Annotations["custom"] = "value"
+				},
+			}, {
+				name: "non-restart annotation value changed",
+				mutate: func(pod *corev1.Pod) {
+					pod.Annotations["kept"] = "changed"
+				},
+			}, {
+				name: "label added",
+				mutate: func(pod *corev1.Pod) {
+					pod.Labels["extra"] = "value"
+				},
+			}, {
+				name: "label value changed",
+				mutate: func(pod *corev1.Pod) {
+					pod.Labels["app"] = "valkey-renamed"
+				},
+			}, {
+				name: "role label state synchronization",
+				mutate: func(pod *corev1.Pod) {
+					pod.Labels[constant.RoleLabelKey] = "primary"
+				},
+			}}
+			for _, tc := range positiveCases {
+				By("skipping switchover when " + tc.name)
+				newPod := basePod.DeepCopy()
+				tc.mutate(newPod)
+				Expect(safeMetadataOnlyInPlaceUpdate(basePod, newPod)).Should(BeTrue())
+			}
+
+			negativeCases := []struct {
+				name   string
+				mutate func(*corev1.Pod)
+			}{{
+				name:   "no diff",
+				mutate: func(pod *corev1.Pod) {},
+			}, {
+				name: "restart annotation added",
+				mutate: func(pod *corev1.Pod) {
+					pod.Annotations[constant.RestartAnnotationKey] = "2026-05-19T14:00:00Z"
+				},
+			}, {
+				name: "restart annotation value changed",
+				mutate: func(pod *corev1.Pod) {
+					if pod.Annotations == nil {
+						pod.Annotations = map[string]string{}
+					}
+					pod.Annotations[constant.RestartAnnotationKey] = "next"
+				},
+			}, {
+				name: "upgrade-restart prefixed annotation added (config.kubeblocks.io/restart-mysql-config)",
+				mutate: func(pod *corev1.Pod) {
+					if pod.Annotations == nil {
+						pod.Annotations = map[string]string{}
+					}
+					pod.Annotations[constant.UpgradeRestartAnnotationKey+"-mysql-config"] = "hash-1"
+				},
+			}, {
+				name: "exact UpgradeRestartAnnotationKey annotation added (no suffix)",
+				mutate: func(pod *corev1.Pod) {
+					if pod.Annotations == nil {
+						pod.Annotations = map[string]string{}
+					}
+					pod.Annotations[constant.UpgradeRestartAnnotationKey] = "trigger"
+				},
+			}, {
+				name: "container image changed",
+				mutate: func(pod *corev1.Pod) {
+					pod.Spec.Containers[0].Image = "valkey:10"
+				},
+			}, {
+				name: "container resources changed",
+				mutate: func(pod *corev1.Pod) {
+					pod.Spec.Containers[0].Resources.Requests = corev1.ResourceList{
+						corev1.ResourceCPU: resource.MustParse("1"),
+					}
+				},
+			}, {
+				name: "container env added",
+				mutate: func(pod *corev1.Pod) {
+					pod.Spec.Containers[0].Env = append(pod.Spec.Containers[0].Env, corev1.EnvVar{Name: "EXTRA", Value: "v"})
+				},
+			}}
+			for _, tc := range negativeCases {
+				By("invoking switchover when " + tc.name)
+				newPod := basePod.DeepCopy()
+				tc.mutate(newPod)
+				Expect(safeMetadataOnlyInPlaceUpdate(basePod, newPod)).Should(BeFalse())
+			}
+
+			By("invoking switchover when an existing upgrade-restart prefixed annotation value changes")
+			podWithUpgradeRestart := basePod.DeepCopy()
+			podWithUpgradeRestart.Annotations[constant.UpgradeRestartAnnotationKey+"-mysql-config"] = "hash-1"
+			mutatedUpgradeRestart := podWithUpgradeRestart.DeepCopy()
+			mutatedUpgradeRestart.Annotations[constant.UpgradeRestartAnnotationKey+"-mysql-config"] = "hash-2"
+			Expect(safeMetadataOnlyInPlaceUpdate(podWithUpgradeRestart, mutatedUpgradeRestart)).Should(BeFalse())
+
+			By("invoking switchover when an existing upgrade-restart prefixed annotation is removed")
+			removedUpgradeRestart := podWithUpgradeRestart.DeepCopy()
+			delete(removedUpgradeRestart.Annotations, constant.UpgradeRestartAnnotationKey+"-mysql-config")
+			Expect(safeMetadataOnlyInPlaceUpdate(podWithUpgradeRestart, removedUpgradeRestart)).Should(BeFalse())
+
+			By("skipping switchover when a non-restart config.kubeblocks.io/* annotation changes (prefix does not match)")
+			nonRestartConfig := basePod.DeepCopy()
+			nonRestartConfig.Annotations["config.kubeblocks.io/non-restart-key"] = "value-1"
+			mutatedNonRestart := nonRestartConfig.DeepCopy()
+			mutatedNonRestart.Annotations["config.kubeblocks.io/non-restart-key"] = "value-2"
+			Expect(safeMetadataOnlyInPlaceUpdate(nonRestartConfig, mutatedNonRestart)).Should(BeTrue())
+		})
 	})
 
 	Context("buildInstanceName2TemplateMap", func() {

pkg/controller/instanceset/reconciler_update.go

@@ -181,8 +181,10 @@ func (r *updateReconciler) Reconcile(tree *kubebuilderx.ObjectTree) (kubebuilder
 			if !equalResourcesInPlaceFields(pod, newInstance.pod) && supportResizeSubResource {
 				err = tree.Update(newPod, kubebuilderx.WithSubResource("resize"))
 			} else {
-				if err = r.switchover(tree, its, newPod.(*corev1.Pod)); err != nil {
-					return kubebuilderx.Continue, err
+				if !safeMetadataOnlyInPlaceUpdate(pod, newInstance.pod) {
+					if err = r.switchover(tree, its, newPod.(*corev1.Pod)); err != nil {
+						return kubebuilderx.Continue, err
+					}
 				}
 				err = tree.Update(newPod)
 			}
@@ -274,6 +276,8 @@ func (r *updateReconciler) isPodCanBeUpdated(tree *kubebuilderx.ObjectTree, its
 	return true, false
 }
 
+var newLifecycleAction = lifecycle.New
+
 func (r *updateReconciler) switchover(tree *kubebuilderx.ObjectTree, its *workloads.InstanceSet, pod *corev1.Pod) error {
 	if its.Spec.MembershipReconfiguration == nil || its.Spec.MembershipReconfiguration.Switchover == nil {
 		return nil
@@ -296,7 +300,7 @@ func (r *updateReconciler) switchover(tree *kubebuilderx.ObjectTree, its *worklo
 		}
 		return m
 	}()
-	lfa, err := lifecycle.New(its.Namespace, clusterName, its.Labels[constant.KBAppComponentLabelKey], lifecycleActions, templateVars, pod)
+	lfa, err := newLifecycleAction(its.Namespace, clusterName, its.Labels[constant.KBAppComponentLabelKey], lifecycleActions, templateVars, pod)
 	if err != nil {
 		return err
 	}

pkg/controller/instanceset/reconciler_update_test.go

@@ -20,6 +20,7 @@ along with this program.  If not, see <http://www.gnu.org/licenses/>.
 package instanceset
 
 import (
+	"context"
 	"slices"
 	"time"
 
@@ -39,6 +40,7 @@ import (
 	"github.com/apecloud/kubeblocks/pkg/constant"
 	"github.com/apecloud/kubeblocks/pkg/controller/builder"
 	"github.com/apecloud/kubeblocks/pkg/controller/kubebuilderx"
+	"github.com/apecloud/kubeblocks/pkg/controller/lifecycle"
 	intctrlutil "github.com/apecloud/kubeblocks/pkg/controllerutil"
 	viper "github.com/apecloud/kubeblocks/pkg/viperx"
 )
@@ -410,5 +412,124 @@ var _ = Describe("update reconciler test", func() {
 		It("inplace updates pod resource using resize subresource", func() {
 			testInplacePodVerticalScaling(true)
 		})
+
+		It("patches pod without calling switchover for metadata-only in-place updates", func() {
+			// This test exercises the Reconcile call site (not just the
+			// safeMetadataOnlyInPlaceUpdate helper) to assert the contract
+			// from PR #10252: when the only difference between the existing
+			// pod and the rebuilt pod is non-restart metadata, the pod is
+			// patched but the switchover lifecycle action must not be
+			// invoked.
+			origSupportResize := intctrlutil.SupportResizeSubResource
+			intctrlutil.SupportResizeSubResource = func() (bool, error) { return false, nil }
+			defer func() { intctrlutil.SupportResizeSubResource = origSupportResize }()
+
+			spy := &lifecycleCallSpy{}
+			origNewLifecycleAction := newLifecycleAction
+			newLifecycleAction = func(_ string, _ string, _ string, _ *kbappsv1.ComponentLifecycleActions, _ map[string]any, _ *corev1.Pod, _ ...*corev1.Pod) (lifecycle.Lifecycle, error) {
+				return spy, nil
+			}
+			defer func() { newLifecycleAction = origNewLifecycleAction }()
+
+			tree := kubebuilderx.NewObjectTree()
+			its.Spec.PodManagementPolicy = appsv1.ParallelPodManagement
+			its.Spec.Replicas = ptr.To[int32](1)
+			its.Spec.PodUpdatePolicy = kbappsv1.PreferInPlacePodUpdatePolicyType
+			// Configure a switchover action so r.switchover would invoke
+			// newLifecycleAction (and thus the spy) if the gate did not
+			// suppress it.
+			its.Spec.MembershipReconfiguration = &workloads.MembershipReconfiguration{
+				Switchover: &kbappsv1.Action{
+					Exec: &kbappsv1.ExecAction{Command: []string{"true"}},
+				},
+			}
+			tree.SetRoot(its)
+
+			// Run the revision pipeline against the original template so the
+			// generated pod's controller-revision label matches
+			// its.Status.UpdateRevisions. This avoids the revision-mismatch
+			// branch in getPodUpdatePolicy that would force recreatePolicy.
+			prepareForUpdate(tree)
+
+			pods := tree.List(&corev1.Pod{})
+			Expect(pods).Should(HaveLen(1))
+			pod := pods[0].(*corev1.Pod)
+			pod.Status.Phase = corev1.PodRunning
+			pod.Status.Conditions = append(pod.Status.Conditions, corev1.PodCondition{
+				Type:               corev1.PodReady,
+				Status:             corev1.ConditionTrue,
+				LastTransitionTime: metav1.NewTime(time.Now().Add(-1 * minReadySeconds * time.Second)),
+			})
+
+			// Mutate the template AFTER prepareForUpdate so the rebuilt pod
+			// differs from the existing pod only in the config-hash
+			// annotation. its.Status.UpdateRevisions is no longer
+			// recomputed, so the pod's revision still matches and the
+			// reconciler enters the in-place branch via basicUpdate=true.
+			if its.Spec.Template.ObjectMeta.Annotations == nil {
+				its.Spec.Template.ObjectMeta.Annotations = map[string]string{}
+			}
+			its.Spec.Template.ObjectMeta.Annotations[constant.CMInsConfigurationHashLabelKey] = "new-hash"
+
+			reconciler = NewUpdateReconciler()
+			res, err := reconciler.Reconcile(tree)
+			Expect(err).Should(BeNil())
+			Expect(res).Should(Equal(kubebuilderx.Continue))
+
+			postPods := tree.List(&corev1.Pod{})
+			Expect(postPods).Should(HaveLen(1),
+				"pod should be in-place updated, not deleted/recreated")
+			updatedPod := postPods[0].(*corev1.Pod)
+			Expect(updatedPod.Annotations).Should(HaveKeyWithValue(constant.CMInsConfigurationHashLabelKey, "new-hash"),
+				"pod should be patched with the new config-hash annotation even though switchover is skipped")
+			Expect(spy.switchoverCalls).Should(Equal(0),
+				"switchover must not be invoked when only the config-hash annotation differs")
+		})
 	})
 })
+
+// lifecycleCallSpy is a test double for lifecycle.Lifecycle used to assert
+// that switchover is or is not invoked during reconciliation. Methods that
+// the call-site tests do not exercise return nil to satisfy the interface.
+type lifecycleCallSpy struct {
+	switchoverCalls  int
+	reconfigureCalls int
+}
+
+func (s *lifecycleCallSpy) PostProvision(_ context.Context, _ client.Reader, _ *lifecycle.Options) error {
+	return nil
+}
+
+func (s *lifecycleCallSpy) PreTerminate(_ context.Context, _ client.Reader, _ *lifecycle.Options) error {
+	return nil
+}
+
+func (s *lifecycleCallSpy) RoleProbe(_ context.Context, _ client.Reader, _ *lifecycle.Options) ([]byte, error) {
+	return nil, nil
+}
+
+func (s *lifecycleCallSpy) Switchover(_ context.Context, _ client.Reader, _ *lifecycle.Options, _ string) error {
+	s.switchoverCalls++
+	return nil
+}
+
+func (s *lifecycleCallSpy) MemberJoin(_ context.Context, _ client.Reader, _ *lifecycle.Options) error {
+	return nil
+}
+
+func (s *lifecycleCallSpy) MemberLeave(_ context.Context, _ client.Reader, _ *lifecycle.Options) error {
+	return nil
+}
+
+func (s *lifecycleCallSpy) Reconfigure(_ context.Context, _ client.Reader, _ *lifecycle.Options, _ map[string]string) error {
+	s.reconfigureCalls++
+	return nil
+}
+
+func (s *lifecycleCallSpy) AccountProvision(_ context.Context, _ client.Reader, _ *lifecycle.Options, _, _, _ string) error {
+	return nil
+}
+
+func (s *lifecycleCallSpy) UserDefined(_ context.Context, _ client.Reader, _ *lifecycle.Options, _ string, _ *kbappsv1.Action, _ map[string]string) error {
+	return nil
+}