Checkpoint adding observability feature #1143
Conversation
Signed-off-by: David Martin <[email protected]>
There was a problem hiding this comment.
Code looks like going in a correct direction.
Few notes to think about:
- Have you considered using a regular dynamic client to do the monitoring resources reconciliation (create, update, delete)?? I do not see any real need to use the monitoring client. I might be missing somehting, though
- When implementing the reconciliation, try to answer the following questions: What happens when the user changes the switch between
enabled: truetoenabled: falseand vice versa? What if somebody externally updates the monitoring resources manually (or some external controller)?? Should kuadrant controller watch the reconciled resources and revert the changes to make sure it is what kuadrant wants to be? For instance, somebody (let's hope accidentally) removes the montoring resources
I will keep watching this PR and providing review, but I will defer the approval to the current experts @Boomatang and @KevFan as I am no longer as familiar as them with the current codebase.
| @@ -54,6 +54,11 @@ func (p *Kuadrant) GetLocator() string { | |||
|
|
|||
| // KuadrantSpec defines the desired state of Kuadrant | |||
| type KuadrantSpec struct { | |||
| Observability Observability `json:"observability,omitempty"` | |||
There was a problem hiding this comment.
I would make this pointer, so you can discriminate between
a) not set
b) set to false
c) set to true
There was a problem hiding this comment.
Understood.
From the reconcile function point of view, I'm wondering if there would be a difference between a and b.
I think the behaviour would be the same for both i.e. remove the SerivceMonitors/PodMonitors if they exist.
There was a problem hiding this comment.
Hay Dav, I have left a few comments around the use of the policy machinery and the structure the code should be in. If there is any questions, please feel free to ask. The policy machinery works differently to the controller runtime and I have a lot of assumed knowledge, so some of my comments might be vague.
| // Check if observability is enabled | ||
| if !kObj.Spec.Observability.Enable { | ||
| logger.Info("observability enable is false") | ||
| // TODO: Remove monitors if they exist |
There was a problem hiding this comment.
I assume this TODO would not remove service monitors that the user may have created.
As a note, the linking function will add the ServiceMonitors to the topology linked to the parent, which I assume would be the Kuadrant CR. So the questions becomes what do you do if the ServiceMonitor has no parent in the topology, as in kuadrant CR was removed.
There was a problem hiding this comment.
I assume this TODO would not remove service monitors that the user may have created.
That would be the idea, yes.
Targeting specific labels should be sufficient.
what do you do if the ServiceMonitor has no parent in the topology, as in kuadrant CR was removed
I'm not sure yet, but a couple of thoughts:
- Would it matter if we can remove the ServiceMonitor based on a label when the Kuadrant CR is removed?
- Is this a problem already when the Kuadrant CR is removed, and it was linked to other resources like authorino and limitador? (I haven't dived into that code yet)
There was a problem hiding this comment.
One why the topology can change the clean up code is based around the parents of a resource. If the monitors are children of resources, let it be the kuadrant CR or autorino, limitador, what you can do is scan the root of the topology for any monitors. The clean up question becomes has this resource got any parents? If no, then it can be removed from the cluster.
| MonClient monclient.Interface | ||
| } | ||
|
|
||
| func NewObservabilityReconciler(client *dynamic.DynamicClient, rm meta.RESTMapper, mc monclient.Interface) *ObservabilityReconciler { |
There was a problem hiding this comment.
nit: when it comes to naming, are we using this initialism as a standard in this code base?
There was a problem hiding this comment.
do you mean the param names like rm for the RESTMapper param?
Signed-off-by: David Martin <[email protected]>
Signed-off-by: David Martin <[email protected]>
Signed-off-by: David Martin <[email protected]>
Signed-off-by: David Martin <[email protected]>
Signed-off-by: David Martin <[email protected]>
Closes #1065
TODO: