diff --git a/pkg/controller/instance/in_place_update_utils.go b/pkg/controller/instance/in_place_update_utils.go index e3dab69e444..94c2ccdc71f 100644 --- a/pkg/controller/instance/in_place_update_utils.go +++ b/pkg/controller/instance/in_place_update_utils.go @@ -297,13 +297,16 @@ func equalResourcesInPlaceFields(old, new *corev1.Pod) bool { return false } oc := old.Spec.Containers[index] - realRequests := nc.Resources.Requests - // 'requests' defaults to Limits if that is explicitly specified, see: https://kubernetes.io/docs/reference/kubernetes-api/workload-resources/pod-v1/#resources - if realRequests == nil { - realRequests = nc.Resources.Limits - } - if !equalField(oc.Resources.Requests, realRequests) { - return false + for _, resourceName := range []corev1.ResourceName{corev1.ResourceCPU, corev1.ResourceMemory} { + if request, ok := nc.Resources.Requests[resourceName]; ok { + oldRequest := corev1.ResourceList{} + if existing, ok := oc.Resources.Requests[resourceName]; ok { + oldRequest[resourceName] = existing + } + if !equalField(oldRequest, corev1.ResourceList{resourceName: request}) { + return false + } + } } if !equalField(oc.Resources.Limits, nc.Resources.Limits) { return false diff --git a/pkg/controller/instance/reconciler_update.go b/pkg/controller/instance/reconciler_update.go index 90a49f335a0..4e0353ebc94 100644 --- a/pkg/controller/instance/reconciler_update.go +++ b/pkg/controller/instance/reconciler_update.go @@ -159,15 +159,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) } diff --git a/pkg/controller/instance/utils_test.go b/pkg/controller/instance/utils_test.go index 1768932ca6a..3c5099bb7a1 100644 --- a/pkg/controller/instance/utils_test.go +++ b/pkg/controller/instance/utils_test.go @@ -182,6 +182,49 @@ func TestSafeMetadataOnlyInPlaceUpdate(t *testing.T) { } } +func TestEqualResourcesInPlaceFieldsSkipsOmittedRequests(t *testing.T) { + oldPod := builder.NewPodBuilder("default", "mysql-0"). + SetContainers([]corev1.Container{{ + Name: "mysql", + Resources: corev1.ResourceRequirements{ + Limits: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("1"), + corev1.ResourceMemory: resource.MustParse("2Gi"), + }, + Requests: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("500m"), + corev1.ResourceMemory: resource.MustParse("1Gi"), + }, + }, + }}). + GetObject() + + omittedRequests := oldPod.DeepCopy() + omittedRequests.Spec.Containers[0].Resources = corev1.ResourceRequirements{ + Limits: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("1"), + corev1.ResourceMemory: resource.MustParse("2Gi"), + }, + } + if !equalResourcesInPlaceFields(oldPod, omittedRequests) { + t.Fatal("omitted desired CPU/memory requests should not force resource inequality") + } + + explicitRequestChanged := omittedRequests.DeepCopy() + explicitRequestChanged.Spec.Containers[0].Resources.Requests = corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("1"), + } + if equalResourcesInPlaceFields(oldPod, explicitRequestChanged) { + t.Fatal("explicit desired CPU request must still be compared") + } + + limitChanged := omittedRequests.DeepCopy() + limitChanged.Spec.Containers[0].Resources.Limits[corev1.ResourceMemory] = resource.MustParse("3Gi") + if equalResourcesInPlaceFields(oldPod, limitChanged) { + t.Fatal("limits must still be compared") + } +} + func TestConfigsToUpdateStillReportsRealConfigHashMismatch(t *testing.T) { inst := builder.NewInstanceBuilder("default", "valkey-0"). SetConfigs([]workloads.ConfigTemplate{{ diff --git a/pkg/controller/instanceset/in_place_update_util.go b/pkg/controller/instanceset/in_place_update_util.go index 1383959fdc6..04fae42d279 100644 --- a/pkg/controller/instanceset/in_place_update_util.go +++ b/pkg/controller/instanceset/in_place_update_util.go @@ -306,13 +306,16 @@ func equalResourcesInPlaceFields(old, new *corev1.Pod) bool { return false } oc := old.Spec.Containers[index] - realRequests := nc.Resources.Requests - // 'requests' defaults to Limits if that is explicitly specified, see: https://kubernetes.io/docs/reference/kubernetes-api/workload-resources/pod-v1/#resources - if realRequests == nil { - realRequests = nc.Resources.Limits - } - if !equalField(oc.Resources.Requests, realRequests) { - return false + for _, resourceName := range []corev1.ResourceName{corev1.ResourceCPU, corev1.ResourceMemory} { + if request, ok := nc.Resources.Requests[resourceName]; ok { + oldRequest := corev1.ResourceList{} + if existing, ok := oc.Resources.Requests[resourceName]; ok { + oldRequest[resourceName] = existing + } + if !equalField(oldRequest, corev1.ResourceList{resourceName: request}) { + return false + } + } } if !equalField(oc.Resources.Limits, nc.Resources.Limits) { return false diff --git a/pkg/controller/instanceset/in_place_update_util_test.go b/pkg/controller/instanceset/in_place_update_util_test.go index 9601b186d86..fbdd523ae8c 100644 --- a/pkg/controller/instanceset/in_place_update_util_test.go +++ b/pkg/controller/instanceset/in_place_update_util_test.go @@ -88,6 +88,43 @@ var _ = Describe("instance util test", func() { mergeInPlaceFields(newPod, oldPod) Expect(equalBasicInPlaceFields(oldPod, newPod)).Should(BeTrue()) }) + + It("skips omitted CPU and memory requests in resource comparison", func() { + oldPod := builder.NewPodBuilder(namespace, "foo-0"). + SetContainers([]corev1.Container{{ + Name: "foo", + Resources: corev1.ResourceRequirements{ + Limits: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("1"), + corev1.ResourceMemory: resource.MustParse("2Gi"), + }, + Requests: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("500m"), + corev1.ResourceMemory: resource.MustParse("1Gi"), + }, + }, + }}). + GetObject() + + omittedRequests := oldPod.DeepCopy() + omittedRequests.Spec.Containers[0].Resources = corev1.ResourceRequirements{ + Limits: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("1"), + corev1.ResourceMemory: resource.MustParse("2Gi"), + }, + } + Expect(equalResourcesInPlaceFields(oldPod, omittedRequests)).Should(BeTrue()) + + explicitRequestChanged := omittedRequests.DeepCopy() + explicitRequestChanged.Spec.Containers[0].Resources.Requests = corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("1"), + } + Expect(equalResourcesInPlaceFields(oldPod, explicitRequestChanged)).Should(BeFalse()) + + limitChanged := omittedRequests.DeepCopy() + limitChanged.Spec.Containers[0].Resources.Limits[corev1.ResourceMemory] = resource.MustParse("3Gi") + Expect(equalResourcesInPlaceFields(oldPod, limitChanged)).Should(BeFalse()) + }) }) Context("container image comparison", func() { diff --git a/pkg/controller/instanceset/reconciler_update.go b/pkg/controller/instanceset/reconciler_update.go index 0506ea598aa..1c3984e476e 100644 --- a/pkg/controller/instanceset/reconciler_update.go +++ b/pkg/controller/instanceset/reconciler_update.go @@ -188,15 +188,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) } diff --git a/pkg/controller/instanceset/reconciler_update_test.go b/pkg/controller/instanceset/reconciler_update_test.go index 719919a6a62..94f1d174f00 100644 --- a/pkg/controller/instanceset/reconciler_update_test.go +++ b/pkg/controller/instanceset/reconciler_update_test.go @@ -495,6 +495,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 53c7371b8e6..3efd0821e34 100644 --- a/pkg/controller/kubebuilderx/plan_builder.go +++ b/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) } 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 3d6c6d53f04..63f7b45c12d 100644 --- a/pkg/controller/kubebuilderx/reconciler.go +++ b/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) {