generic coordinator and worker classes

[lucabello]

Issue

Our work on HA solutions (Mimir, Loki, and Tempo) is rapidly becoming more complex, making it easy for the respective Coordinator/Worker charms to diverge.

Solution

Most of the Coordinator/Worker charms are in fact shared between the different HA solutions; this PR adds abstractions to simplify the creation and management of Coordinator/Worker charms, also preventing the charms from diverging.

Testing Instructions

To test these classes, we should use them in our Mimir HA deployment, and make sure the solution still functions.

Mimir PR: (to be added)

Additional Context

• https://github.com/canonical/observability/blob/main/decision-records/2024-06-14--coordinated-multirole-workers.md

[mmkay]

I really like where this is going. A bit of comments from me but it looks like migration might be pretty straightforward, nice :)

[sed-i]

Sorry, I managed to do only a partial review for now.
Should we split out databag_model to a tandem PR? Perhaps taking it outside of this context would help with abstractions?

[lucabello]

I answered all the comments so far, and closed a bunch I agreed with and solved :)

As per the databag_model.py abstraction, I explained here that it's something we already use, and it's brought in with basically no changes (except the ones I just made from the PR comments)

[PietroPasotti]

I answered all the comments so far, and closed a bunch I agreed with and solved :)

As per the databag_model.py abstraction, I explained here that it's something we already use, and it's brought in with basically no changes (except the ones I just made from the PR comments)

given it brings quite a bunch of controversy and noise to this PR, I agree we should probably keep ti separate and make this branch depend on that one.

[lucabello]

I've moved databag_model.py inside interface.py (i.e., the former cluster.py) to make it clear this databag abstraction is only meant for the coordinated workers.

We should absolutely make a shared databag abstraction to re-use in our charms, but (even though the code is still there) I'm taking the file out of the PR so we don't waste time on that for now :)

[Abuelodelanada]

I posted a few comments in the code.

On a more general note I think we should try to avoid as much as we can the coupling between Coordinator and Worker and its collaborators objects.

For instance, JujuTopology, CertHandler, MetricsEndpointProvider, etc are instantiated in the __init__ of the classes.

What do you think about injecting those dependencies instead of creating the instances inside the objects?
I know for that we may need some factory methods/classes... but by reducing the coupling we increase testability and separation of concerns.

[lucabello]

@Abuelodelanada the “coupling” is 50% of the work here, and it's a feature: the coordinator/workers class are meant to abstract two main things

• the coordinator/worker logic (is the deployment complete, recommended, etc.);
• the self-monitoring integrations.

If we were to pass objects like MetricsEndpointProvider to the Coordinator class, there would be no point in having it inside it at all. At some point, we said we want to have a separate object that wraps our COS integrations, and that will also wrap things similarly.

Of course, there is room for improvement:

• the way we interact with relation data could be made generic and re-used by all of our charms;
• self-monitoring integrations could be wrapped in a SelfMonitoring object;
• more stuff, probably.

However, the points above would require discussions, careful design, and that's something I'd love to do — but I don't think we have time for that right now, and we'll have to postpone those.

[sed-i]

@lucabello do we have a tandem PR for this somewhere?

[lucabello]

I have linked the two Mimir PRs where this abstraction is used.

If we don't have other comments, we need to address a few last things:

• this comment @Abuelodelanada @sed-i what do you think?
• update-ca-certificates in the Nginx image: I believe @mmkay has already contacted the right people
• figure out if we can/should have model_uuid in the worker topology without getting warnings from pydantic
• when the worker is getting data from a coordinator app databag, do we need a leader guard? The answer is no, we don't
• is there a better way to fetch the charm libraries than what we do know?

[sed-i]

Nice!
Thanks for being patient with all the comments :)
Let's see how it goes using it in a charm!