diff --git a/pkg/controller/instanceset/reconciler_update.go b/pkg/controller/instanceset/reconciler_update.go index d869d50bd56..f1b28304bc5 100644 --- a/pkg/controller/instanceset/reconciler_update.go +++ b/pkg/controller/instanceset/reconciler_update.go @@ -176,15 +176,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, newInstance.pod) && supportResizeSubResource { + switch { + case !equalResourcesInPlaceFields(pod, newInstance.pod) && supportResizeSubResource: err = tree.Update(newPod, kubebuilderx.WithSubResource("resize")) - } else { - if !safeMetadataOnlyInPlaceUpdate(pod, newInstance.pod) { - if err = r.switchover(tree, its, newPod.(*corev1.Pod)); err != nil { - return kubebuilderx.Continue, err - } + case safeMetadataOnlyInPlaceUpdate(pod, newInstance.pod): + err = tree.Update(newPod, kubebuilderx.WithPatch(true)) + default: + if err = r.switchover(tree, its, newPod.(*corev1.Pod)); err != nil { + return kubebuilderx.Continue, err } err = tree.Update(newPod) } diff --git a/pkg/controller/instanceset/reconciler_update_test.go b/pkg/controller/instanceset/reconciler_update_test.go index abfa77033e8..b0f570fb777 100644 --- a/pkg/controller/instanceset/reconciler_update_test.go +++ b/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") }) diff --git a/pkg/controller/kubebuilderx/plan_builder.go b/pkg/controller/kubebuilderx/plan_builder.go index c2ee059d501..2eb38de8354 100644 --- a/pkg/controller/kubebuilderx/plan_builder.go +++ b/pkg/controller/kubebuilderx/plan_builder.go @@ -180,12 +180,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) } diff --git a/pkg/controller/kubebuilderx/plan_builder_test.go b/pkg/controller/kubebuilderx/plan_builder_test.go index 1d45468abae..53fcb01e4c4 100644 --- a/pkg/controller/kubebuilderx/plan_builder_test.go +++ b/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()) + }) }) }) }) diff --git a/pkg/controller/kubebuilderx/reconciler.go b/pkg/controller/kubebuilderx/reconciler.go index ff57b9d4f98..50148867554 100644 --- a/pkg/controller/kubebuilderx/reconciler.go +++ b/pkg/controller/kubebuilderx/reconciler.go @@ -40,6 +40,9 @@ type ObjectOption interface { 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 } type WithSubResource string @@ -48,6 +51,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 ObjectTree struct { // TODO(free6om): should find a better place to hold these two params? context.Context