[DPE-2977][DPE-2978][DPE-2979][DPE-3189] - refactor: restructure entire codebase into managers, events + core

[marcoppenheimer]

Goals

1. Make it easier and quicker to share code and features between VM + K8s ZooKeeper charms, with minimal 'little fixes' each time.

2. Break up code across multiple modules for more straightforward separation of concerns and readability.

Overview

• core/
  • cluster.py
    • Copy-able, abstracted state for Unit, App + client relation data
    • Exposes these primitives for consumption by CharmBase
    • Any 'calculated/dependent' state that isn't explicitly in a relation-data field. Things that look at multiple of the above (e.g all_units_related)
  • models.py
    • StateBase with an overridden update() method, acting as a relation-data object
    • Unit --> ZKServer
    • App --> ZKCluster
    • Client --> ZKClient - contains both provider + requirer data
    • These fixed types provides known signatures for all the esoteric relation-data fields bandied about, as well as crystallizing return types + docstrings
  • workload.py
    • Base parent directory and filepaths that assume a PATHS global in literals.py, which will be different for both K8s + VM, but allows using the same interface
    • A strict, fixed interface for operating on the underlying workload. Implementation of which is in src/workload.py for substrate specific logic, again, using the same common interface between K8s + VM
• events/
  • Various event handlers, and only event handlers, taking CharmBase as a constructor. Can be thought of as 'extensions' of CharmBase, similar to pattern already applied to the majority of charms
  • Goal of splitting this up is to make it more immediately obvious what events are handled where, without having to sift through a bunch of non-event logic
  • Uses self.charm.state and self.charm.manager references to do things, keeping a single state and a single manager to avoid object proliferation
  • In theory, these can also be copy-pasted verbatim between different charm substrate repos
• managers/
  • Contains the 'business' logic for the charm - i.e non-charm operations like API calls, constructing + writing specific files etc that is specific to the workload
  • Benefit of splitting these up is the same as for splitting out events/, it helps with separation of concerns
  • Takes state and workload as constructors, isolating them from CharmBase
• charm.py
  • Creates single-source-of-truth attrs for managers+events+state
  • Contains event-handlers for standard 'lifecycle' events like 'install', 'relation-changed' etc. Motivation to keep them in charm.py and not in events/ is because when surveying any charm for the first time, the first place you look is in charm.py. Centralizes the most important logic
• literals.py
  • Globals with fixed variable names, general internal configuration as before
  • Advantage of having them here is that it allows for substrate specific differences between K8s + VM charms without necessitating code changes
  • For example, SUBSTRATE=vm|k8s allows for branching logic in a single file while building hostnames in a single managers/config file
• workload.py
  • Implements the base workload from core/workload for substrate specific, mostly serving as a replacement for the ZooKeeperSnap class from before this PR

[deusebio]

First pass! Great job! I believe the code has really improved with respect to the structure and the organization. I really like it really a lot more!

In particular, the use of the State really provides a great entry point to the snapshot of the data available to the charm. Moreover, we now very well separate between the data, the business logic and the charm logic.

I have a number of comments and suggestions that I don't think they would take too much time, but I believe they would provide sensible value.

The only major general comment I have is to really create a clean dependency tree among modules, e.g.:

• core can import from common
• managers can import from core (and eventually common)
• events can import from managers (and eventually common and core)
• charm-specific can import from events (and eventually common, core and managers)

With increasing specialization, going from very general (e.g. common that applies to ANY charm) to charm-specific modules.

Right now, this is somewhat broken by the literals since literals is charm specific, but it is imported by multiple level in the hierarchy, and this creates a bit of circular, more complex dependency tree in the scope

[deusebio]

Overall, I'm rather happy with these modifications and I believe the code structuring of the charm has improved significantly.

I would still try to separate the constants of literals.py in the different modules (such that we could eventually externalize/factor out some of these modules), but this can be done later when needed.

For a lower and deeper level review, I'll leave it up to @juditnovak and @zmraul

[juditnovak]

Excellent 1st Prototype!!!

[juditnovak]

Great 1st prototype!
However I foresee furher iterations to reach the "golden standard", for reasons such as :

• [DPE-2977][DPE-2978][DPE-2979][DPE-3189] - refactor: restructure entire codebase into managers, events + core #103 (comment)
• futher consensus is needed on the spec, that may have (foreseeably non-essential) impact implementation here

[zmraul]

Left a couple of comments, as this is an initial pass after doing the changes to Kafka.

A big problem I have (even though I did it the same way) is that the naming convention of Unit --> ZKServer, App --> ZKCluster, Client --> ZKClient both provider + requirer data feels extremely unintuitive to someone that is not familiarized with the charm. Whereas keeping names like peer_relation or client_relation are way more obvious from the get go if you know charming standards.