Skip to content

Commit

Permalink
🐛 controllerutil.CreateOrPatch doesn't update not empty Status fields…
Browse files Browse the repository at this point in the history
… if Spec fields are specified
  • Loading branch information
dmvolod committed Mar 4, 2021
1 parent 836b363 commit f0abe78
Show file tree
Hide file tree
Showing 2 changed files with 67 additions and 0 deletions.
14 changes: 14 additions & 0 deletions pkg/controller/controllerutil/controllerutil.go
Original file line number Diff line number Diff line change
Expand Up @@ -309,6 +309,20 @@ func CreateOrPatch(ctx context.Context, c client.Client, obj client.Object, f Mu
if (hasBeforeStatus || hasAfterStatus) && !reflect.DeepEqual(beforeStatus, afterStatus) {
// Only issue a Status Patch if the resource has a status and the beforeStatus
// and afterStatus copies differ
if result == OperationResultUpdated {
// If Status was replaced by Patch before, set it to afterStatus
objectAfterPatch, err := runtime.DefaultUnstructuredConverter.ToUnstructured(obj)
if err != nil {
return result, err
}
if err = unstructured.SetNestedField(objectAfterPatch, afterStatus, "status"); err != nil {
return result, err
}
// If Status was replaced by Patch before, restore patched structure to the obj
if err = runtime.DefaultUnstructuredConverter.FromUnstructured(objectAfterPatch, obj); err != nil {
return result, err
}
}
if err := c.Status().Patch(ctx, obj, statusPatch); err != nil {
return result, err
}
Expand Down
53 changes: 53 additions & 0 deletions pkg/controller/controllerutil/controllerutil_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import (
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/types"
"k8s.io/client-go/kubernetes/scheme"
"k8s.io/utils/pointer"
"sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
)
Expand Down Expand Up @@ -460,6 +461,18 @@ var _ = Describe("Controllerutil", func() {
ExpectWithOffset(1, fetched.Status).To(BeEquivalentTo(deploy.Status))
}

assertLocalDeployStatusWasUpdated := func(fetched *appsv1.Deployment) {
By("local deploy object was updated during patch & has same spec, status, resource version as fetched")
if fetched == nil {
fetched = &appsv1.Deployment{}
ExpectWithOffset(1, c.Get(context.TODO(), deplKey, fetched)).To(Succeed())
}
ExpectWithOffset(1, fetched.ResourceVersion).To(Equal(deploy.ResourceVersion))
ExpectWithOffset(1, *fetched.Spec.Replicas).To(BeEquivalentTo(int32(5)))
ExpectWithOffset(1, fetched.Status).To(BeEquivalentTo(deploy.Status))
ExpectWithOffset(1, len(fetched.Status.Conditions)).To(BeEquivalentTo(1))
}

It("creates a new object if one doesn't exists", func() {
op, err := controllerutil.CreateOrPatch(context.TODO(), c, deploy, specr)

Expand Down Expand Up @@ -559,6 +572,46 @@ var _ = Describe("Controllerutil", func() {
assertLocalDeployWasUpdated(nil)
})

It("patches resource and not empty status", func() {
op, err := controllerutil.CreateOrPatch(context.TODO(), c, deploy, specr)

Expect(op).To(BeEquivalentTo(controllerutil.OperationResultCreated))
Expect(err).NotTo(HaveOccurred())

replicas := int32(3)
deployStatus := appsv1.DeploymentStatus{
ReadyReplicas: 1,
Replicas: replicas,
}
op, err = controllerutil.CreateOrPatch(context.TODO(), c, deploy, func() error {
Expect(deploymentScaler(deploy, replicas)()).To(Succeed())
return deploymentStatusr(deploy, deployStatus)()
})
By("returning no error")
Expect(err).NotTo(HaveOccurred())

By("returning OperationResultUpdatedStatus")
Expect(op).To(BeEquivalentTo(controllerutil.OperationResultUpdatedStatus))

assertLocalDeployWasUpdated(nil)

op, err = controllerutil.CreateOrPatch(context.TODO(), c, deploy, func() error {
deploy.Spec.Replicas = pointer.Int32Ptr(5)
deploy.Status.Conditions = []appsv1.DeploymentCondition{{
Type: appsv1.DeploymentProgressing,
Status: corev1.ConditionTrue,
}}
return nil
})
By("returning no error")
Expect(err).NotTo(HaveOccurred())

By("returning OperationResultUpdatedStatus")
Expect(op).To(BeEquivalentTo(controllerutil.OperationResultUpdatedStatus))

assertLocalDeployStatusWasUpdated(nil)
})

It("errors when MutateFn changes object name on creation", func() {
op, err := controllerutil.CreateOrPatch(context.TODO(), c, deploy, func() error {
Expect(specr()).To(Succeed())
Expand Down

0 comments on commit f0abe78

Please sign in to comment.