feat: enqueue instances on rgd update #474
Conversation
|
With regards to the update annotation - currently, I feel like the update probably should not be conditional at all. Primarily because even if an instance is annotated with |
There was a problem hiding this comment.
Awesome, thank you @fabianburth !! left a few thoughts/questions below
| dc.handlers.Store(gvr, handler) | ||
| // trigger reconciliation of the corresponding gvr's | ||
| objs, err := dc.kubeClient.Resource(gvr).Namespace("").List(ctx, metav1.ListOptions{}) |
There was a problem hiding this comment.
Does this list objects in all namespaces or just in the default namespace?
There was a problem hiding this comment.
Apparently, it does list objects in all namespaces. But since I actually had to go check after you asked, I included an object with a non-default namespace in the test.
| @@ -62,8 +62,10 @@ import ( | |||
| "time" | |||
|
|
|||
| "github.com/go-logr/logr" | |||
| "github.com/kro-run/kro/api/v1alpha1" | |||
There was a problem hiding this comment.
can you please move this import to the L80 import group, checkout this commit message golang/tools@ed69e84
There was a problem hiding this comment.
Resolved itself (due to the removal of the annotation constants)
api/v1alpha1/types.go
Outdated
|
|
||
| // InstanceUpdatePolicy is the annotation key for the instance update policy | ||
| InstanceUpdatePolicy = "kro.run/instance-update-policy" | ||
|
|
||
| // InstanceUpdatePolicyOnRGDUpdate represents the update policy that leads to | ||
| // a reconciliation of the instance after the corresponding resource graph is updated | ||
| InstanceUpdatePolicyOnRGDUpdate = "on-rgd-update" | ||
|
|
||
| // InstanceUpdatePolicyIgnoreRGDUpdate represents the update policy that explicitly | ||
| // avoids reconciliation of the instance after the corresponding resource graph is updated | ||
| InstanceUpdatePolicyIgnoreRGDUpdate = "ignore-rgd-update" |
There was a problem hiding this comment.
I'm thinking that we should first implement the core functionality of updating instances on RGD updates as the default behavior. I'm concerned about introducing annotations at this stage since they represent a potential breaking change and we probablyy should discuss the naming conventions and semantics carefully. maybe we could address the annotation mechanism/names in a separate PR?
There was a problem hiding this comment.
Sure!
As mentioned in my comment, I feel very unsure about the annotations and their expected / intended behaviour anyway.
Enhancement
Currently, when applying a modified
ResourceGraphDefinition, the corresponding instance objects will not be enqueued for reconciliation. This means, the changes in the RGD do not take effect until the next reconciliation of the instance. If the instances are not modified, the next scheduled reconciliation occurs after the resync period of the dynamic controller.This PR proposes a change to allow for a rather immediate reconciliation of corresponding instance objects.
This is also mentioned in this issue and work on that has already been started in PR.
TODO: @a-hilaly I'd like to revisit the idea of the update annotation.