Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

✨CreateOrPatch #850

Merged
merged 2 commits into from
Sep 25, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
108 changes: 108 additions & 0 deletions pkg/controller/controllerutil/controllerutil.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,13 @@ package controllerutil
import (
"context"
"fmt"
"reflect"

"k8s.io/apimachinery/pkg/api/equality"
"k8s.io/apimachinery/pkg/api/errors"
"k8s.io/apimachinery/pkg/api/meta"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/runtime/schema"
"k8s.io/utils/pointer"
Expand Down Expand Up @@ -180,6 +182,10 @@ const ( // They should complete the sentence "Deployment default/foo has been ..
OperationResultCreated OperationResult = "created"
// OperationResultUpdated means that an existing resource is updated
OperationResultUpdated OperationResult = "updated"
// OperationResultUpdatedStatus means that an existing resource and its status is updated
OperationResultUpdatedStatus OperationResult = "updatedStatus"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are currently only used for patch operations. The naming may be confusing since an update sets the whole object, while a patch does not. Consider OperationResultPatched + OperationResultPatchedStatus?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it doesn't really matter since we're diffing the object after mutation and deriving the patch based on that, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @alexeldeib,

These are currently only used for patch operations. The naming may be confusing since an update sets the whole object, while a patch does not. Consider OperationResultPatched + OperationResultPatchedStatus?

The reason for the current approach is to be consistent with the existing constants.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's the difference between OperationResultUpdatedStatus and OperationResultUpdated? does it make sense to expose this distinction in CreateOrUpdate as well? If not, does it make sense to remove one?

Copy link
Contributor Author

@akutz akutz Mar 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's the difference between OperationResultUpdatedStatus and OperationResultUpdated? does it make sense to expose this distinction in CreateOrUpdate as well? If not, does it make sense to remove one?

Hi @alexeldeib,

I suppose if we do not create Patched variants, then there is no real distinction between OperationResultUpdated and OperationResultUpdatedStatus. Good catch.

To me Updated means the result, not the method used to achieve it. This is another reason I did not create the variants. But I have no real strong opinions here.

What is important to me is to have a result that indicates the status sub-resource was updated, but not the primary resource itself.

Beyond that, I do not have any real strong thoughts on how the result codes are indicated.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(edit: weird timing -- you managed to reply before I posted this and saw it after :) )

my initial reaction was because of a race condition like this:

thread a - createOrPatch

  1. get object
  2. mutate and extract patch
  3. send patch to API server

thread b - racer

  1. get obj
  2. mutate
  3. send to apiserver

semantically I would want to tell the user their patch was successfully applied but I cannot guarantee that it was applied against the same object they provided (i.e., b3 occurred before a3). This is a slightly less strong guarantee than OperationResultUpdated (which would imply either a3 or b3 would fail), so I would have matched the HTTP methods and reached for OperationResultPatched.

To me OperationResultUpdatedStatus means what I think OperationResultUpdatedStatusOnly is supposed to mean -- that a patch was applied but only the status was updated. Neither of them actually means an update operation was what was executed, vs the naming of OperationResultCreated, OperationResultUpdated

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uhm what if both the object and the status were updated? Then we basically need to return two results or not?

Copy link
Contributor Author

@akutz akutz Jun 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If both were updated, then OperationResultUpdated would imply both were updated.

I didn't change the names of the results to Patched because I was trying to preserve compatibility with the existing result values. I'm happy to adjust the names.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But the fact that the CreateOrPatch tries to update the Status whereas CreateOrUpdate does not is just an implementation detail and not inherent to doing patch vs update or am I missing something?

May I ask for the reason you need to differentiate if the status resource was updated? I am asking because I believe that whatever we do here should also be added to CreateOrUpdate and I know ppl use the result sometimes to determine how to proceed. If we extend this we might break them which is why maybe returning a []result may be better (but nor completely sure).

// OperationResultUpdatedStatusOnly means that only an existing status is updated
OperationResultUpdatedStatusOnly OperationResult = "updatedStatusOnly"
)

// CreateOrUpdate creates or updates the given object in the Kubernetes
Expand Down Expand Up @@ -223,6 +229,108 @@ func CreateOrUpdate(ctx context.Context, c client.Client, obj runtime.Object, f
return OperationResultUpdated, nil
}

// CreateOrPatch creates or patches the given object in the Kubernetes
// cluster. The object's desired state must be reconciled with the before
// state inside the passed in callback MutateFn.
//
// The MutateFn is called regardless of creating or updating an object.
//
// It returns the executed operation and an error.
func CreateOrPatch(ctx context.Context, c client.Client, obj runtime.Object, f MutateFn) (OperationResult, error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/crossplane/crossplane-runtime/blob/c092201a/pkg/resource/resource.go#L274

We have a similar function that we use in various Crossplane controllers. We've found that passing a function that takes the existing and the desired resource can be useful, for example to ensure that the existing resource has the same controller reference as the desired resource before we mutate it.

key, err := client.ObjectKeyFromObject(obj)
if err != nil {
return OperationResultNone, err
}

if err := c.Get(ctx, key, obj); err != nil {
if !errors.IsNotFound(err) {
return OperationResultNone, err
}
if f != nil {
if err := mutate(f, key, obj); err != nil {
return OperationResultNone, err
}
}
if err := c.Create(ctx, obj); err != nil {
return OperationResultNone, err
}
return OperationResultCreated, nil
}

// Create patches for the object and its possible status.
objPatch := client.MergeFrom(obj.DeepCopyObject())
statusPatch := client.MergeFrom(obj.DeepCopyObject())

// Create a copy of the original object as well as converting that copy to
// unstructured data.
before, err := runtime.DefaultUnstructuredConverter.ToUnstructured(obj.DeepCopyObject())
if err != nil {
return OperationResultNone, err
}

// Attempt to extract the status from the resource for easier comparison later
beforeStatus, hasBeforeStatus, err := unstructured.NestedFieldCopy(before, "status")
if err != nil {
return OperationResultNone, err
}

// If the resource contains a status then remove it from the unstructured
// copy to avoid unnecessary patching later.
if hasBeforeStatus {
unstructured.RemoveNestedField(before, "status")
}

// Mutate the original object.
if f != nil {
if err := mutate(f, key, obj); err != nil {
return OperationResultNone, err
}
}

// Convert the resource to unstructured to compare against our before copy.
after, err := runtime.DefaultUnstructuredConverter.ToUnstructured(obj)
if err != nil {
return OperationResultNone, err
}

// Attempt to extract the status from the resource for easier comparison later
afterStatus, hasAfterStatus, err := unstructured.NestedFieldCopy(after, "status")
if err != nil {
return OperationResultNone, err
}

// If the resource contains a status then remove it from the unstructured
// copy to avoid unnecessary patching later.
if hasAfterStatus {
unstructured.RemoveNestedField(after, "status")
}

result := OperationResultNone

if !reflect.DeepEqual(before, after) {
vincepri marked this conversation as resolved.
Show resolved Hide resolved
// Only issue a Patch if the before and after resources (minus status) differ
if err := c.Patch(ctx, obj, objPatch); err != nil {
return result, err
}
result = OperationResultUpdated
}

if (hasBeforeStatus || hasAfterStatus) && !reflect.DeepEqual(beforeStatus, afterStatus) {
vincepri marked this conversation as resolved.
Show resolved Hide resolved
// Only issue a Status Patch if the resource has a status and the beforeStatus
// and afterStatus copies differ
if err := c.Status().Patch(ctx, obj, statusPatch); err != nil {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will always fail if the status subresource is not enabled or not?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So update to gracefully handle the resources that lack a status sub resource?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So update to gracefully handle the resources that lack a status sub resource?

Sorry I don't understand what you mean by this comment?

We could probably handle this by just optimistically patching status and handling the error we get in case its not enabled (I believe its a 404 but might be wrong)?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you referring to the if condition above?

return result, err
}
if result == OperationResultUpdated {
result = OperationResultUpdatedStatus
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vincepri fyi since this was one we wanted more 👀 on

IMO my comment in https://github.com/kubernetes-sigs/controller-runtime/pull/850/files#r392628297 is still accurate, either OperationResultUpdatedStatus should be removed or there should be real differentiation between OperationResultUpdatedStatus and OperationResultUpdated.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that makes sense, I'd suggest to just keep one for now and revisit in a future version. +1 removing the updated status one

} else {
result = OperationResultUpdatedStatusOnly
}
}

return result, nil
Comment on lines +308 to +331
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for illustrative purposes:

Suggested change
result := OperationResultNone
if !reflect.DeepEqual(before, after) {
// Only issue a Patch if the before and after resources (minus status) differ
if err := c.Patch(ctx, obj, objPatch); err != nil {
return result, err
}
result = OperationResultUpdated
}
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 err := c.Status().Patch(ctx, obj, statusPatch); err != nil {
return result, err
}
if result == OperationResultUpdated {
result = OperationResultUpdatedStatus
} else {
result = OperationResultUpdatedStatusOnly
}
}
return result, nil
result := OperationResultNone
if !reflect.DeepEqual(before, after) {
// Only issue a Patch if the before and after resources (minus status) differ
if err := c.Patch(ctx, obj, objPatch); err != nil {
return result, err
}
result = OperationResultUpdatedSpec
}
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 err := c.Status().Patch(ctx, obj, statusPatch); err != nil {
return result, err
}
if result == OperationResultUpdatedSpec {
result = OperationResultUpdated
} else {
result = OperationResultUpdatedStatus
}
}
return result, nil

}

// mutate wraps a MutateFn and applies validation to its result
func mutate(f MutateFn, key client.ObjectKey, obj runtime.Object) error {
if err := f(); err != nil {
Expand Down
214 changes: 214 additions & 0 deletions pkg/controller/controllerutil/controllerutil_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -406,6 +406,213 @@ var _ = Describe("Controllerutil", func() {
})
})

Describe("CreateOrPatch", func() {
var deploy *appsv1.Deployment
var deplSpec appsv1.DeploymentSpec
var deplKey types.NamespacedName
var specr controllerutil.MutateFn

BeforeEach(func() {
deploy = &appsv1.Deployment{
ObjectMeta: metav1.ObjectMeta{
Name: fmt.Sprintf("deploy-%d", rand.Int31()),
Namespace: "default",
},
}

deplSpec = appsv1.DeploymentSpec{
Selector: &metav1.LabelSelector{
MatchLabels: map[string]string{"foo": "bar"},
},
Template: corev1.PodTemplateSpec{
ObjectMeta: metav1.ObjectMeta{
Labels: map[string]string{
"foo": "bar",
},
},
Spec: corev1.PodSpec{
Containers: []corev1.Container{
{
Name: "busybox",
Image: "busybox",
},
},
},
},
}

deplKey = types.NamespacedName{
Name: deploy.Name,
Namespace: deploy.Namespace,
}

specr = deploymentSpecr(deploy, deplSpec)
})

assertLocalDeployWasUpdated := 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).To(BeEquivalentTo(deploy.Spec))
ExpectWithOffset(1, fetched.Status).To(BeEquivalentTo(deploy.Status))
}

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

By("returning no error")
Expect(err).NotTo(HaveOccurred())

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

By("actually having the deployment created")
fetched := &appsv1.Deployment{}
Expect(c.Get(context.TODO(), deplKey, fetched)).To(Succeed())

By("being mutated by MutateFn")
Expect(fetched.Spec.Template.Spec.Containers).To(HaveLen(1))
Expect(fetched.Spec.Template.Spec.Containers[0].Name).To(Equal(deplSpec.Template.Spec.Containers[0].Name))
Expect(fetched.Spec.Template.Spec.Containers[0].Image).To(Equal(deplSpec.Template.Spec.Containers[0].Image))
})

It("patches existing object", func() {
var scale int32 = 2
op, err := controllerutil.CreateOrPatch(context.TODO(), c, deploy, specr)
Expect(err).NotTo(HaveOccurred())
Expect(op).To(BeEquivalentTo(controllerutil.OperationResultCreated))

op, err = controllerutil.CreateOrPatch(context.TODO(), c, deploy, deploymentScaler(deploy, scale))
By("returning no error")
Expect(err).NotTo(HaveOccurred())

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

By("actually having the deployment scaled")
fetched := &appsv1.Deployment{}
Expect(c.Get(context.TODO(), deplKey, fetched)).To(Succeed())
Expect(*fetched.Spec.Replicas).To(Equal(scale))
assertLocalDeployWasUpdated(fetched)
})

It("patches only changed objects", func() {
op, err := controllerutil.CreateOrPatch(context.TODO(), c, deploy, specr)

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

op, err = controllerutil.CreateOrPatch(context.TODO(), c, deploy, deploymentIdentity)
By("returning no error")
Expect(err).NotTo(HaveOccurred())

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

assertLocalDeployWasUpdated(nil)
})

It("patches only changed status", func() {
op, err := controllerutil.CreateOrPatch(context.TODO(), c, deploy, specr)

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

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

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

assertLocalDeployWasUpdated(nil)
})

It("patches resource and 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)
})

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

By("returning error")
Expect(err).To(HaveOccurred())

By("returning OperationResultNone")
Expect(op).To(BeEquivalentTo(controllerutil.OperationResultNone))
})

It("errors when MutateFn renames an object", func() {
op, err := controllerutil.CreateOrPatch(context.TODO(), c, deploy, specr)

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

op, err = controllerutil.CreateOrPatch(context.TODO(), c, deploy, deploymentRenamer(deploy))

By("returning error")
Expect(err).To(HaveOccurred())

By("returning OperationResultNone")
Expect(op).To(BeEquivalentTo(controllerutil.OperationResultNone))
})

It("errors when object namespace changes", func() {
op, err := controllerutil.CreateOrPatch(context.TODO(), c, deploy, specr)

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

op, err = controllerutil.CreateOrPatch(context.TODO(), c, deploy, deploymentNamespaceChanger(deploy))

By("returning error")
Expect(err).To(HaveOccurred())

By("returning OperationResultNone")
Expect(op).To(BeEquivalentTo(controllerutil.OperationResultNone))
})

It("aborts immediately if there was an error initially retrieving the object", func() {
op, err := controllerutil.CreateOrPatch(context.TODO(), errorReader{c}, deploy, func() error {
Fail("Mutation method should not run")
return nil
})

Expect(op).To(BeEquivalentTo(controllerutil.OperationResultNone))
Expect(err).To(HaveOccurred())
})
})

Describe("Finalizers", func() {
var obj runtime.Object = &errRuntimeObj{}
var deploy *appsv1.Deployment
Expand Down Expand Up @@ -473,6 +680,13 @@ func deploymentSpecr(deploy *appsv1.Deployment, spec appsv1.DeploymentSpec) cont
}
}

func deploymentStatusr(deploy *appsv1.Deployment, status appsv1.DeploymentStatus) controllerutil.MutateFn {
return func() error {
deploy.Status = status
return nil
}
}

var deploymentIdentity controllerutil.MutateFn = func() error {
return nil
}
Expand Down