Add CreateOrUpdate utility method #98

calind · 2018-07-31T10:43:36Z

Closes #85.

DirectXMan12

comments inline, also needs tests

DirectXMan12 · 2018-07-31T20:08:26Z

pkg/controller/controllerutil/controllerutil.go

+}
+
+// TransformFn is a function which take in a kubernetes object and returns the
+// desired state of that object


note whether or not it's allowed/expected to mutate the object. It can be helpful when refactoring later.

DirectXMan12 · 2018-07-31T20:08:41Z

pkg/controller/controllerutil/controllerutil.go

+	OperationUpdated = "updated"
+)
+
+// CreateOrUpdate creates or updates a kuberenes resource. It takes in a key and


typo: "kubernetes"

DirectXMan12 · 2018-07-31T20:10:39Z

pkg/controller/controllerutil/controllerutil.go

+
+// CreateOrUpdate creates or updates a kuberenes resource. It takes in a key and
+// a placeholder for the existing object and returns the modified object
+func CreateOrUpdate(ctx context.Context, c client.Client, key client.ObjectKey, existing runtime.Object, t TransformFn) (runtime.Object, OperationType, error) {


our client methods tend not to return objects, and instead operate "in-place" on them. This should do so as well

calind · 2018-08-01T17:56:58Z

Hi @DirectXMan12 I've implemented your feedback and also wrote tests

DirectXMan12

Ok, so, I think in general you can drop the key from this, and extract it from the object in question. The signature should be reminiscent of the Create and Update methods. Sorry for not catching that last review.

DirectXMan12 · 2018-08-02T19:24:22Z

pkg/controller/controllerutil/controllerutil.go

+
+// CreateOrUpdate creates or updates a kubernetes resource. It takes in a key and
+// a placeholder for the existing object and returns the operation executed
+func CreateOrUpdate(ctx context.Context, c client.Client, key client.ObjectKey, existing runtime.Object, t TransformFn) (OperationType, error) {


is the purpose of OperationType just logging?

Yup, but we can also pass it to the transform function, so that you know that is it going to create or update.

DirectXMan12 · 2018-08-02T19:25:52Z

pkg/controller/controllerutil/controllerutil.go

+		// Create a new zero value object so that the in parameter of
+		// TransformFn is always a "clean" object, with only Name and Namespace
+		// set
+		zero := reflect.New(reflect.TypeOf(existing).Elem()).Interface()


this is what existing is for. It should never be nil, so you shouldn't have to construct a new zero object.

The reason for creating a new zero value struct is that you can pass in existing an initialized one, eg. with .spec filled. I'm not sure that client.Get zeroes the object if it doesn't find one.

What I want to enforce here, is that if this function is going to create a new object, your transform callback always gets an empty object with only Name and Namespace filled in.

DirectXMan12 · 2018-08-02T19:27:38Z

pkg/controller/controllerutil/controllerutil_test.go

+			By("returning OperationCreated")
+			Expect(op).Should(BeEquivalentTo(controllerutil.OperationCreated))
+
+			By("returning returning no error")


typo: duplicate word

DirectXMan12 · 2018-08-02T19:28:35Z

pkg/controller/controllerutil/controllerutil_test.go

+
+			var scale int32 = 2
+
+			Expect(c.Create(context.TODO(), depl)).Should(Succeed())


style is general Expect(X).To(Y) (Should is reserved for the omega character, which we don't use)

calind · 2018-08-03T09:41:37Z

@DirectXMan12 what do you think about changing the signature to

CreateOrUpdate (ctx context.Context, c client.Client, obj runtime.Object, t ...TransformFn)

That way you can just pass the desired object and if you need to apply some changes you can define a set of transform functions.

The reason for keeping the transform function is that for example in case of Service another controller fills in the ClusterIP and you cannot reset it to null (you will get a validation error).

The TransformFn would change the signature to:

type TransformFn func(existing, desired runtime.Object, op OperationType) (runtime.Object, error)

Edit: I've added to the PR 164020f which does that, so it can be reverted easily.

calind · 2018-08-03T11:54:18Z

Also, do you thing that I should move this to a client/clientutil package?

DirectXMan12 · 2018-08-03T15:36:56Z

Also, do you thing that I should move this to a client/clientutil package

I think it's probably fine where it is.

what do you think about changing the signature to ...

I'm fine with the multiple transform functions. I'm not entirely certain as to the purpose of passing the operation type to the transform function, though. Same with passing existing and desired pointers -- ideally, the transformation should be idempotent, and should take an object, and turn it into the desired state.

I'd expect a signature of

type TransformFunc func(obj runtime.Object) error
func CreateOrUpdate(ctx context.Context, c client.Client, obj runtime.Object, transforms... TransformFunc) error

but it's possible I'm missing something here.

calind · 2018-08-04T11:48:12Z

@DirectXMan12 I think I've made some logic mistakes in the previous commit so I rewrote it in 2fe301a. Let me try to better illustrate the use cases for transform functions which are actually reconcile functions (I have renamed the type to better illustrate this to ReconcileFn).

Probably 90% of times you would use CreateOrUpdate by just giving it a desired object. The role of reconcile functions is to cover some corner cases.

For example if you pass in a corev1.Service as a desired object, when the object is created in apiserver, another controller will set it's spec.clusterIP. After is set, the field becomes immutable. A reconcile function solves this by copying spec.clusterIP from the existing object into the desired object.

So the ReconcileFn signature would become:

type ReconcileFn func(desired, existing runtime.Object) error

As for the operation I've dropped it from the ReconcileFn and mainly it's purpose is for informing the caller what action have been taken (eg. for logging purposes). If you have any feedback on how this should be passed I'm open for changes :)

DirectXMan12 · 2018-08-06T18:39:02Z

aaaah, I see what you're saying with the reconcile func. That makes sense, we just need to make sure that's blindingly obvious.

calind · 2018-08-08T09:32:33Z

@DirectXMan12 after using it in real code, I'm having second thoughts about the current state of ReconcileFn:

type ReconcileFn func(desired, existing runtime.Object) error

because I allways end up with code like:

existing.DeepCopyInto(desired)
desired.replicas = 2

Ideally you want your reconcile function to just do:

existing.replicas = 2

And that is because you always want to mutate the existing state towards a desired state.

So the proposal is to change the signatures to:

// CreateOrUpdate a resrouce with the same type, name and namespace as obj 
// and recocile it's state using the passed in ReconcileFn.
// It returns the executed operation and an error.
func CreateOrUpdate(ctx context.Context, c client.Client, obj runtime.Object, r ReconcileFn) (OperationType, error) {}

// ReconcileFn is a function which mutates the existing object into it's desired state. 
// If there is no existing object, a zero value object is passed in.
type ReconcileFn func(existing runtime.Object) error

Here is an example which always creates/updates a deployment with the scale of 2:

func scale(existing runtime.Object) error {
    var replicas int32 = 2
    out := existing.(*appsv1.Deployment)
    existing.Sepc.Replicas = &replicas
}
CreateOrUpdate(context.TODO(), c, &appsv1.Deployment{Name: "foo", Namespace: "bar"}, scale)

DirectXMan12 · 2018-08-09T16:17:39Z

So, we just need to be clear on the intended usage in the comments -- the ReconcileFunc should actually be doing the work -- it's not just reconciling the differences any more. In the original proposal,
it seemed like you'd do work on the passed in object before calling CreateOrUpdate, and then reconcile the differences if it already exists.

calind · 2018-08-09T17:23:53Z

Yep, you are correct, the ReconcileFn should do the actual work. I’ll update the PR accordingly and also make that clear in the comments. I’ll also add an example usage.

calind · 2018-08-10T11:07:11Z

@DirectXMan12, I've pushed the changes. I've also added an example. It is ready for review.

calind · 2018-08-16T12:54:21Z

I've rebased the changes and also found an issue when the ReconcileFn changes the name/namespace of the object.

@DirectXMan12 PTAL

AMecea · 2018-08-23T13:20:00Z

@DirectXMan12 what's the status of this?

DirectXMan12 · 2018-08-23T20:14:42Z

hey, sorry, busy week. let me try and take a look today.

DirectXMan12

sorry for taking so long to get back to you. I think the internals are close, but there's a couple of rough spots. comments inline.

DirectXMan12 · 2018-08-24T00:51:33Z

pkg/controller/controllerutil/controllerutil.go

+// It returns the executed operation and an error.
+func CreateOrUpdate(ctx context.Context, c client.Client, obj runtime.Object, r ReconcileFn) (OperationType, error) {
+	// op is the operation we are going to attempt
+	var op OperationType = OperationNoop


do we need to have the operation type here? I thought you were going to remove it...

I think needs to be a way to surface the operation. For example, I'm using it to emit events based of that, eg. Successfully created Deployment default/foo. Is there a better way of doing it?

DirectXMan12 · 2018-08-24T00:52:10Z

pkg/controller/controllerutil/controllerutil.go

+	var op OperationType = OperationNoop
+
+	// get the existing object
+	mo, ok := obj.(v1.Object)


what does mo stand for here? when you can, use more descriptive names.

DirectXMan12 · 2018-08-24T00:53:23Z

pkg/controller/controllerutil/controllerutil.go

+)
+
+// CreateOrUpdate a resource with the same type, name and namespace as obj
+// and reconcile it's state using the passed in ReconcileFn.


this needs to document the use of obj, since it's a bit tricky

DirectXMan12 · 2018-08-24T00:54:52Z

pkg/controller/controllerutil/controllerutil.go

+		Name:      mo.GetName(),
+		Namespace: mo.GetNamespace(),
+	}
+	existing := reflect.New(reflect.TypeOf(obj).Elem()).Interface().(runtime.Object)


shouldn't we be using obj here?

If used, obj might get mutated by c.Get, that's why the new existing object.

If we agree that obj can be mutated (eg. we return the existing object with it), than yes, we can use obj directly.

after more code reading I see that client.Create and client.Update mutate the obj. I'll update the PR accordingly.

DirectXMan12 · 2018-08-24T00:57:40Z

pkg/controller/controllerutil/controllerutil.go

+	}
+
+	if errors.IsNotFound(err) {
+		err = c.Create(ctx, newobj)


generally, the passed-in object should be updated with the changes.

calind · 2018-08-27T15:31:42Z

@DirectXMan12 PTAL.

I've implemented the feedback regarding obj parameter. I've also kept OperationType in return as a way to surface the operation. I'm open to suggestions on better ways to do this.

For example I'm using the OperationType to send events, like Successfully created Deployment default/foo when creating/updating a dependent deployment, logging being the other obvious use case for it.

DirectXMan12 · 2018-08-28T17:42:13Z

I think this looks good. Let's have a quick peek from @droot and then we'll merge.

droot · 2018-08-28T20:28:13Z

@DirectXMan12 @calind looking at it.

droot

Sorry, I am late to the party. Have a few comments.

pkg/controller/controllerutil/controllerutil.go

droot · 2018-08-28T21:04:32Z

pkg/controller/controllerutil/controllerutil.go

+		if reflect.DeepEqual(existing, obj) {
+			return OperationNoop, nil
+		}
+		err = c.Update(ctx, obj)


Trying to analyze the semantics of this wrt status sub-resource. If an API implements status as sub-resource (which is the recommended approach), then Update is updating just the Spec part of API (Then the DeepEqual should be of the spec's field only ?) @DirectXMan12

@droot obj being a runtime.Object, is it possible to get the spec of that? If so what should happen with types not having a Spec field?

Thinking more about it.. Is it something the updateFn should tell us ? because it has more knowledge about the logic whether update is needed or not. This also takes away reflect.DeepEqual which could be costly from runtime performance perspective as well.

possible signature.
updateFn(obj runtime.Object) (needsUpdate bool, error)

The updateFn is applied regardless of Create or Update. Its role is to change the existing state towards the desired state. For creation, it starts with an empty obj and for update with the existing object.

Update need to be called if updateFn changed something about obj, that's why the DeepEqual.

I think the better name is mutateFn to make it clear that it does the heavy lifting regardless of Create or Update.

calind · 2018-09-03T17:33:03Z

@droot PTAL

droot · 2018-09-04T20:58:34Z

@calind looks good to me. Can you pl. squash the commits ?

calind · 2018-09-04T21:12:09Z

@droot done

DirectXMan12 · 2018-09-05T17:06:39Z

/lgtm

DirectXMan12 · 2018-09-05T17:17:47Z

/approve

k8s-ci-robot · 2018-09-05T17:17:50Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: calind, DirectXMan12

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [DirectXMan12]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

blaggacao · 2018-10-04T16:19:59Z

@calind Is it correct to assume that I would pass on the desired state object as runtime.Object to the CreateOrUpdate function?
Like this:

	// Fetch the OdooCluster instance
	instance := &clusterv1beta1.OdooCluster{}
	err := r.Get(context.TODO(), request.NamespacedName, instance)
	if err != nil {
		if errors.IsNotFound(err) {
			// Object not found, return.  Created objects are automatically garbage collected.
			// For additional cleanup logic use finalizers.
			return reconcile.Result{}, nil
		}
		// Error reading the object - requeue the request.
		return reconcile.Result{}, err
	}

	// Define the desired DBNamespace object
	objDBNamespace := &clusterv1beta1.DBNamespace{
		ObjectMeta: metav1.ObjectMeta{
			Name:      instance.Name + "-dbnamespace",
			Namespace: instance.Namespace,
		},
		Spec: instance.Spec.DBSpec,  // Desired state
	}

	if err := controllerutil.SetControllerReference(instance, objDBNamespace, r.scheme); err != nil {
		return reconcile.Result{}, err
	}

	_, err := controllerutil.CreateOrUpdate(context.TODO(), r, objDBNamespace, scale)
       // Still don't know how to properly handle _ but doesn't matter, here.

calind · 2018-10-04T16:45:37Z

The object you pass in is the receiver object, aka the result of client.Create or client.Update. The desired state must be reconciled in the MutateFn (aka. the last parameter) like this:

// Define the object name/namespace
objDBNamespace := &clusterv1beta1.DBNamespace{
	ObjectMeta: metav1.ObjectMeta{
		Name:      instance.Name + "-dbnamespace",
		Namespace: instance.Namespace,
	},
}

result, err := controllerutil.CreateOrUpdate(context.TODO(), r.Client, objDBNamespace, func (existing runtime.Object) error{
	// mutate here the state of existing object to the desired state
	// note that at this point _objDBNamespace_ and _existing_ point to the same struct
	out := existing.(*clusterv1beta1.DBNamespace)

	out.Spec = instance.Spec.DBSpec  // Desired state

	if err := controllerutil.SetControllerReference(instance, out, r.scheme); err != nil {
		return err
	}

	return nil
})

if err != nil {
	return reconcile.Result{}, err
}
// Your objDBNamespace now contains the object from the culster after Create/Update
// see: https://godoc.org/github.com/kubernetes-sigs/controller-runtime/pkg/client#Writer
// Result contains the applied operation: Create/Update/NoOp
log.Info("object reconciled", "operation", result)

return reconcile.Result{}, nil

blaggacao · 2018-10-04T17:07:12Z

Thanks, got it! The inline function idiom is a nice idea to keep logic linear to read!

blaggacao · 2018-10-05T10:46:57Z

@calind This commit enabled a beautiful pattern https://github.com/xoe-labs/odoo-operator/blob/2a0248030b53ad82b99c2c229fdd1db3f3ae38aa/pkg/controller/odoocluster/odoocluster_controller.go#L212-L459
I guess I still have to take care of become-immutable fields manually in the mutate function, right?

calind · 2018-10-05T11:17:29Z

Cool, glad to hear that! You can verify in the in the mutate function that out.ObjectMeta.CreationTimestamp.IsZero() to check that an object is going to be created or updated.

Add CreateOrUpdate utility method

Add travis and reportcard images

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jul 31, 2018

calind force-pushed the create-or-update branch from e7b1d33 to 9028c7a Compare July 31, 2018 12:00

DirectXMan12 suggested changes Jul 31, 2018

View reviewed changes

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 1, 2018

DirectXMan12 suggested changes Aug 2, 2018

View reviewed changes

calind force-pushed the create-or-update branch 3 times, most recently from 5dc6b34 to 1431a57 Compare August 4, 2018 11:42

calind force-pushed the create-or-update branch from 1431a57 to 2fe301a Compare August 4, 2018 12:21

DirectXMan12 mentioned this pull request Aug 9, 2018

admission webhook server #81

Merged

calind force-pushed the create-or-update branch from 2fe301a to 74a702e Compare August 10, 2018 11:02

calind force-pushed the create-or-update branch 2 times, most recently from 5110ac3 to 0b3a3fd Compare August 16, 2018 12:47

calind force-pushed the create-or-update branch from 0b3a3fd to 2f12419 Compare August 16, 2018 13:00

mengqiy assigned DirectXMan12 Aug 16, 2018

mengqiy mentioned this pull request Aug 23, 2018

restructure certwriter and certprovisioner packages #113

Merged

DirectXMan12 suggested changes Aug 24, 2018

View reviewed changes

calind force-pushed the create-or-update branch from 2f12419 to 12b7536 Compare August 27, 2018 15:12

DirectXMan12 approved these changes Aug 28, 2018

View reviewed changes

droot reviewed Aug 28, 2018

View reviewed changes

calind mentioned this pull request Sep 3, 2018

[kubebuilder] Add mysqlcluster controller bitpoke/mysql-operator#109

Merged

Add CreateOrUpdate utility method

d47ce17

calind force-pushed the create-or-update branch from 9b41e73 to d47ce17 Compare September 4, 2018 21:01

droot approved these changes Sep 4, 2018

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 5, 2018

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 5, 2018

k8s-ci-robot merged commit 4604c59 into kubernetes-sigs:master Sep 5, 2018

justinsb pushed a commit to justinsb/controller-runtime that referenced this pull request Dec 7, 2018

Merge pull request kubernetes-sigs#98 from presslabs/create-or-update

4b39913

Add CreateOrUpdate utility method

calind mentioned this pull request Feb 8, 2019

⚠️ Call MutateFn on CreateOrUpdate regardless of the operation #319

Merged

DirectXMan12 pushed a commit that referenced this pull request Jan 31, 2020

Merge pull request #98 from pwittrock/master

5c2f3d8

Add travis and reportcard images


		var scale int32 = 2

		Expect(c.Create(context.TODO(), depl)).Should(Succeed())

Add CreateOrUpdate utility method #98

Add CreateOrUpdate utility method #98

Conversation

calind commented Jul 31, 2018

Uh oh!

DirectXMan12 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

calind commented Aug 1, 2018

Uh oh!

DirectXMan12 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

calind Aug 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

calind commented Aug 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

calind commented Aug 3, 2018

Uh oh!

DirectXMan12 commented Aug 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

calind commented Aug 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DirectXMan12 commented Aug 6, 2018

Uh oh!

calind commented Aug 8, 2018

Uh oh!

DirectXMan12 commented Aug 9, 2018

Uh oh!

calind commented Aug 9, 2018

Uh oh!

calind commented Aug 10, 2018

Uh oh!

calind commented Aug 16, 2018

Uh oh!

AMecea commented Aug 23, 2018

Uh oh!

DirectXMan12 commented Aug 23, 2018

Uh oh!

DirectXMan12 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

calind Aug 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

calind Aug 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

calind Aug 3, 2018 •

edited

Loading

calind commented Aug 3, 2018 •

edited

Loading

DirectXMan12 commented Aug 3, 2018 •

edited

Loading

calind commented Aug 4, 2018 •

edited

Loading

calind Aug 27, 2018 •

edited

Loading

calind Aug 27, 2018 •

edited

Loading

calind commented Aug 27, 2018 •

edited

Loading

droot Aug 28, 2018 •

edited

Loading

calind Aug 28, 2018 •

edited

Loading

blaggacao commented Oct 4, 2018 •

edited

Loading