search

artemiscloud / activemq-artemis-operator

Apache License 2.0
69 stars 63 forks source link

[#898] Make the operator periodically reconcile #899

Closed gaohoward closed 6 months ago

gtully commented 6 months ago

I am not convinced on this one, maybe we need a different variable for our regular retrys, and keep the this as a real periodic resync option when > 0

We currently retry on failure and make use of this period, it would make sense to split that out, b/c it would also make sense to not do periodic re sync most of the time unless there are external resources, but we already have that logic, just that it uses this variable.

gaohoward commented 6 months ago

I am not convinced on this one, maybe we need a different variable for our regular retrys, and keep the this as a real periodic resync option when > 0

We currently retry on failure and make use of this period, it would make sense to split that out, b/c it would also make sense to not do periodic re sync most of the time unless there are external resources, but we already have that logic, just that it uses this variable.

It makes sense to use a different var from the one we already uses. Doing regular resync is not for external resources specifically if external resources means extra mounts or such that. It is for keeping the resources associated with a CR deployment intact (that includes services/secrets/configmaps/ingresses/routes). So that when they are accidentally destroyed the next re-sync can recreate them.

gtully commented 6 months ago

for anything we create, we should watch with our owner reference. We currently do that only for Pods and SS, we had a difficulty with external resources b/c we don't own them, but I think we should just add routes and services to our watch list and the existing periodic reconcile when we have extra mounts will work for the other cases. see: https://github.com/artemiscloud/activemq-artemis-operator/blob/main/controllers/activemqartemis_controller.go#L744

for extra mounts https://github.com/artemiscloud/activemq-artemis-operator/blob/main/controllers/activemqartemis_controller.go#L184

and that was the result of trying a watch that was not constrained by an owner ref, if I recall correctly, there is more detail in https://github.com/artemiscloud/activemq-artemis-operator/issues/457

gaohoward commented 6 months ago

@gtully I'll close this (also the assocated issue) and raise a new PR. Thanks!