chore: patch metadata-only pod updates (#10401)

(cherry picked from commit e296fcd)

Author: weicao

pkg/controller/instance/reconciler_update.go

@@ -149,15 +149,14 @@ func (r *updateReconciler) Reconcile(tree *kubebuilderx.ObjectTree) (kubebuilder
 				return kubebuilderx.Continue, err
 			}
 
-			// if already updating using subresource, don't update it again, because without subresource, those fields are considered immutable.
-			// Another reconciliation will be triggered since pod status will be updated.
-			if !equalResourcesInPlaceFields(pod, newPod) && supportResizeSubResource {
+			switch {
+			case !equalResourcesInPlaceFields(pod, newPod) && supportResizeSubResource:
 				err = tree.Update(newMergedPod, kubebuilderx.WithSubResource("resize"))
-			} else {
-				if !safeMetadataOnlyInPlaceUpdate(pod, newPod) {
-					if err = r.switchover(tree, inst, newMergedPod.(*corev1.Pod)); err != nil {
-						return kubebuilderx.Continue, err
-					}
+			case safeMetadataOnlyInPlaceUpdate(pod, newPod):
+				err = tree.Update(newMergedPod, kubebuilderx.WithPatch(true))
+			default:
+				if err = r.switchover(tree, inst, newMergedPod.(*corev1.Pod)); err != nil {
+					return kubebuilderx.Continue, err
 				}
 				err = tree.Update(newMergedPod)
 			}

pkg/controller/instanceset/reconciler_update.go

@@ -177,15 +177,14 @@ func (r *updateReconciler) Reconcile(tree *kubebuilderx.ObjectTree) (kubebuilder
 				return kubebuilderx.Continue, err
 			}
 
-			// if already updating using subresource, don't update it again, because without subresource, those fields are considered immutable.
-			// Another reconciliation will be triggered since pod status will be updated.
-			if !equalResourcesInPlaceFields(pod, newPod) && supportResizeSubResource {
+			switch {
+			case !equalResourcesInPlaceFields(pod, newPod) && supportResizeSubResource:
 				err = tree.Update(newMergedPod, kubebuilderx.WithSubResource("resize"))
-			} else {
-				if !safeMetadataOnlyInPlaceUpdate(pod, newPod) {
-					if err = r.switchover(tree, its, newMergedPod.(*corev1.Pod)); err != nil {
-						return kubebuilderx.Continue, err
-					}
+			case safeMetadataOnlyInPlaceUpdate(pod, newPod):
+				err = tree.Update(newMergedPod, kubebuilderx.WithPatch(true))
+			default:
+				if err = r.switchover(tree, its, newMergedPod.(*corev1.Pod)); err != nil {
+					return kubebuilderx.Continue, err
 				}
 				err = tree.Update(newMergedPod)
 			}

pkg/controller/instanceset/reconciler_update_test.go

@@ -482,6 +482,10 @@ var _ = Describe("update reconciler test", func() {
 			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")
+			_, option, err := tree.GetWithOption(updatedPod)
+			Expect(err).NotTo(HaveOccurred())
+			Expect(option.Patch).Should(BeTrue(),
+				"metadata-only pod updates should use a patch to avoid stale full-update conflicts")
 			Expect(spy.switchoverCalls).Should(Equal(0),
 				"switchover must not be invoked when only the config-hash annotation differs")
 		})

pkg/controller/kubebuilderx/plan_builder.go

@@ -187,12 +187,18 @@ func buildOrderedVertices(transCtx *transformContext, currentTree *ObjectTree, d
 					transCtx.logger.Error(err, "can't get GVKName from object", "object", newObj.GetName())
 					return
 				}
-				var v *model.ObjectVertex
-				subResource := desiredTree.childrenOptions[*name].SubResource
+				var (
+					v           *model.ObjectVertex
+					subResource = desiredTree.childrenOptions[*name].SubResource
+					action      = model.ActionUpdatePtr()
+				)
+				if desiredTree.childrenOptions[*name].Patch {
+					action = model.ActionPatchPtr()
+				}
 				if subResource != "" {
-					v = model.NewObjectVertex(oldObj, newObj, model.ActionUpdatePtr(), inDataContext4G(), model.WithSubResource(subResource))
+					v = model.NewObjectVertex(oldObj, newObj, action, inDataContext4G(), model.WithSubResource(subResource))
 				} else {
-					v = model.NewObjectVertex(oldObj, newObj, model.ActionUpdatePtr(), inDataContext4G())
+					v = model.NewObjectVertex(oldObj, newObj, action, inDataContext4G())
 				}
 				findAndAppend(v)
 			}

pkg/controller/kubebuilderx/plan_builder_test.go

@@ -98,6 +98,27 @@ var _ = Describe("plan builder test", func() {
 			Expect(planBuilder.defaultWalkFunc(v)).Should(Succeed())
 		})
 
+		It("should patch object", func() {
+			svcOrig := builder.NewServiceBuilder(namespace, name).GetObject()
+			svc := svcOrig.DeepCopy()
+			svc.Labels = map[string]string{"patched": "true"}
+			v := &model.ObjectVertex{
+				OriObj: svcOrig,
+				Obj:    svc,
+				Action: model.ActionPatchPtr(),
+			}
+			k8sMock.EXPECT().
+				Patch(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).
+				DoAndReturn(func(_ context.Context, obj *corev1.Service, _ client.Patch, _ ...client.PatchOption) error {
+					Expect(obj).ShouldNot(BeNil())
+					Expect(obj.Namespace).Should(Equal(svc.Namespace))
+					Expect(obj.Name).Should(Equal(svc.Name))
+					Expect(obj.Labels).Should(Equal(svc.Labels))
+					return nil
+				}).Times(1)
+			Expect(planBuilder.defaultWalkFunc(v)).Should(Succeed())
+		})
+
 		It("should update pvc object", func() {
 			pvcOrig := builder.NewPVCBuilder(namespace, name).GetObject()
 			pvc := pvcOrig.DeepCopy()
@@ -286,6 +307,30 @@ var _ = Describe("plan builder test", func() {
 				}
 				Expect(found).To(BeTrue())
 			})
+
+			It("should append a patch vertex when requested by object options", func() {
+				oldPod := builder.NewPodBuilder("ns", "pod").GetObject()
+				newPod := oldPod.DeepCopy()
+				newPod.Labels = map[string]string{"patched": "true"}
+
+				currentTree := NewObjectTree()
+				desiredTree := NewObjectTree()
+				currentTree.SetRoot(builder.NewInstanceSetBuilder("ns", "root").GetObject())
+				desiredTree.SetRoot(currentTree.GetRoot())
+				Expect(currentTree.Add(oldPod)).Should(Succeed())
+				Expect(desiredTree.Update(newPod, WithPatch(true))).Should(Succeed())
+
+				vertices := buildOrderedVertices(transCtx, currentTree, desiredTree)
+
+				found := false
+				for _, v := range vertices {
+					if pod, ok := v.Obj.(*corev1.Pod); ok && pod.Name == "pod" {
+						found = true
+						Expect(*v.Action).To(Equal(model.PATCH))
+					}
+				}
+				Expect(found).To(BeTrue())
+			})
 		})
 	})
 })

pkg/controller/kubebuilderx/reconciler.go

@@ -41,6 +41,9 @@ type ObjectOptions struct {
 	// if not empty, action should be done on the specified subresource
 	SubResource string
 
+	// if true, the object should be patched instead of fully updated.
+	Patch bool
+
 	// if true, the object should not be reconciled
 	SkipToReconcile bool
 }
@@ -51,6 +54,12 @@ func (w WithSubResource) ApplyToObject(opts *ObjectOptions) {
 	opts.SubResource = string(w)
 }
 
+type WithPatch bool
+
+func (w WithPatch) ApplyToObject(opts *ObjectOptions) {
+	opts.Patch = bool(w)
+}
+
 type SkipToReconcile bool
 
 func (o SkipToReconcile) ApplyToObject(opts *ObjectOptions) {