Generic instance update func

[abays]

Intended to be used with our deferred-instance-patch-at-end-of-reconcile pattern

[gibizer]

The instance is a pretty generic name for the package. I would call this reconciler instead. That also helps showing the intention that PatchInstance should be use in a Reconciler.

[stuggi]

@abays @gibizer wondering if it would make sense to have that as part of the helper, so that we call helper.Patch() to patch the object the helper is for ?

[stuggi]

@abays @gibizer wondering if it would make sense to have that as part of the helper, so that we call helper.Patch() to patch the object the helper is for ?

ok, @abays just told me that there is some import cycle then because of the util.log... we could update util/log.go to pass in the logger from the caller ... but that requires changes all over the place where we use the logger funcs. use basic logging in patching could be an alternative.

seems we need to put some more thinking in our logging. I wanted to get back to that and make that better to have e.g. some log level ... but other prios ...

if we agree that this patching func is better located in the helper as it is tightly coupled, we might want to go with basic logging and update that when we got back to logging in general, instead of relocate the patchInstance later ...

[gibizer]

@abays @gibizer wondering if it would make sense to have that as part of the helper, so that we call helper.Patch() to patch the object the helper is for ?

ok, @abays just told me that there is some import cycle then because of the util.log... we could update util/log.go to pass in the logger from the caller ... but that requires changes all over the place where we use the logger funcs. use basic logging in patching could be an alternative.

seems we need to put some more thinking in our logging. I wanted to get back to that and make that better to have e.g. some log level ... but other prios ...

if we agree that this patching func is better located in the helper as it is tightly coupled, we might want to go with basic logging and update that when we got back to logging in general, instead of relocate the patchInstance later ...

What if we introduce a new LogForObject variant that takes a logger instead of the Helper obj. And first use it in Patch when it moved to Helper. But also deprecate the current LogForObject variant and gradually change the operators to use the new variant.

I also see a generic pattern here. Helper is a collection of loosely couple stuff: logger, clients, finalizers, an old copy of the instance, etc. We are passing a Helper instance to everywhere. So we basically couple everything to the Helper even if only part of the Helper is needed in a certain place. This creates unnecessary coupling. If I could I would split up the Helper to smaller pieces:

1. The logic that stores the old instance and calculates the diff for patching. This would only be needed for the actual PatchInstance() call as we are moving all our instance update to that single place. So this part of the helper does not need to be passed to functions called from Reconcile.
2. The rest of the stored fields and Getters can be split out to independent functions or even removed and let a Reconcile classes hold the fields as it already hold some of them like clients. E.g. GetFinalizer() can be rewritten to GetFinalizer(obj client.Object, client client.Client) as it does not depend on any other part of the Helper.

[stuggi]

@abays @gibizer wondering if it would make sense to have that as part of the helper, so that we call helper.Patch() to patch the object the helper is for ?

ok, @abays just told me that there is some import cycle then because of the util.log... we could update util/log.go to pass in the logger from the caller ... but that requires changes all over the place where we use the logger funcs. use basic logging in patching could be an alternative.
seems we need to put some more thinking in our logging. I wanted to get back to that and make that better to have e.g. some log level ... but other prios ...
if we agree that this patching func is better located in the helper as it is tightly coupled, we might want to go with basic logging and update that when we got back to logging in general, instead of relocate the patchInstance later ...

What if we introduce a new LogForObject variant that takes a logger instead of the Helper obj. And first use it in Patch when it moved to Helper. But also deprecate the current LogForObject variant and gradually change the operators to use the new variant.

I also see a generic pattern here. Helper is a collection of loosely couple stuff: logger, clients, finalizers, an old copy of the instance, etc. We are passing a Helper instance to everywhere. So we basically couple everything to the Helper even if only part of the Helper is needed in a certain place. This creates unnecessary coupling. If I could I would split up the Helper to smaller pieces:

1. The logic that stores the old instance and calculates the diff for patching. This would only be needed for the actual PatchInstance() call as we are moving all our instance update to that single place. So this part of the helper does not need to be passed to functions called from Reconcile.
2. The rest of the stored fields and Getters can be split out to independent functions or even removed and let a Reconcile classes hold the fields as it already hold some of them like clients. E.g. GetFinalizer() can be rewritten to GetFinalizer(obj client.Object, client client.Client) as it does not depend on any other part of the Helper.

Its a good idea to add a new variant for LogForObject by doing a log package and have all the future logging stuff there.
Thats what we should do as part or as a prio step for this PR.

Re the other split, probably best to discuss that again after the break. GetFinalizer() is not meant to get the finalizers of
the object. It is to have the finalizer string for the object created during initialization and when add/remove/check if it
is on object can be used. But lets have that conversation later.

[gibizer]

Re the other split, probably best to discuss that again after the break. GetFinalizer() is not meant to get the finalizers of the object. It is to have the finalizer string for the object created during initialization and when add/remove/check if it is on object can be used. But lets have that conversation later.

Then it even better to rename it as well when we separate it out. The implementation is actually simple based on NewHelper:

GetFinalizerName(obj client.Object, client client.Client) (string, error) {
    gvk, err := apiutil.GVKForObject(obj, client.Scheme())
    return gvk.Kind()
}

Looking more at the implementation I think this can be simplified further to:

func GetFinalizerName(obj client.Object) string {
	return obj.GetObjectKind().GroupVersionKind().Kind
}

But I agree we need to table this for next year as this is my last day before my PTO. (I will be back around 5th of Jan)

[abays]

Its a good idea to add a new variant for LogForObject by doing a log package and have all the future logging stuff there.
Thats what we should do as part or as a prio step for this PR

Let's do that as a separate PR first. Who wants to do it? :)

[gibizer]

Its a good idea to add a new variant for LogForObject by doing a log package and have all the future logging stuff there.
Thats what we should do as part or as a prio step for this PR

Let's do that as a separate PR first. Who wants to do it? :)

I will make a quick stab of it.

[gibizer]

Its a good idea to add a new variant for LogForObject by doing a log package and have all the future logging stuff there.
Thats what we should do as part or as a prio step for this PR

Let's do that as a separate PR first. Who wants to do it? :)

I will make a quick stab of it.

#125 I try to take an approach which is more aligned with the existing logr and context interfaces. But I provided some compatibility methods so we can discuss our options. I will push a nova change to show the real usage of it.

[gibizer]

And here is the nova example openstack-k8s-operators/nova-operator#206

[gibizer]

Testing of #125 (#125 (comment)) shows that we don't need any fancy logging interface massaging as the logger from the ctx already has all the information we want in the log.

[gibizer]

Lets change this to

l := log.FromContext(ctx)
l.Info(...) 

The instance related information is already stored in and emitted by that logger

[gibizer]

l.Error()

[gibizer]

l.Info()

[gibizer]

l.Error()

[abays]

The instance is a pretty generic name for the package. I would call this reconciler instead. That also helps showing the intention that PatchInstance should be use in a Reconciler.

@gibizer With the new logging approach, which would remove the import cycle loop and thus allow this code to exist in the helper package, would you still recommend moving this code to a separate reconciler package?

[gibizer]

The instance is a pretty generic name for the package. I would call this reconciler instead. That also helps showing the intention that PatchInstance should be use in a Reconciler.

@gibizer With the new logging approach, which would remove the import cycle loop and thus allow this code to exist in the helper package, would you still recommend moving this code to a separate reconciler package?

Lets move it to the helper package with a Helper receiver. Even if we blow up that Helper struct later the PatchInstance will be close to the struct that holds the before copy of the instance (currently stored in the Helper).

[gibizer]

Looks good to me. Thanks @abays!

[stuggi]

/lgtm

[bogdando]

/lgtm

nvm