initial cockroachdb-operator charm for review #1
Conversation
The operator framework repository itself has recently lowered the allowed length - adjust this source code accordingly.
This PR needs to go in canonical/operator#196 to avoid canonical/operator#202 when running tests.
metadata.yaml
Outdated
| maintainers: | ||
| - github.com/dshcherb | ||
| description: CockroachDB Charm | ||
| min-juju-version: 2.7.5 | ||
| tags: | ||
| - database | ||
| provides: | ||
| db: | ||
| interface: pgsql | ||
| optional: true | ||
| proxy-listen-tcp: | ||
| interface: proxy-listen-tcp |
There was a problem hiding this comment.
Indentation in this file is inconsistent.
src/charm.py
Outdated
| ModelError, | ||
| ) | ||
| from cluster import CockroachDBCluster | ||
| from interface_proxy_listen_tcp import ProxyListenTcpInterfaceProvides |
There was a problem hiding this comment.
Not a nice file or module name to read/write.
There was a problem hiding this comment.
Agreed, I rushed a bit with names. I think names like TCPLoadBalancer and TCPServer (or TCPBackend) would be more comprehensive.
src/charm.py
Outdated
|
|
||
| class CockroachDBCharmEvents(CharmEvents): | ||
| cockroachdb_started = EventSource(CockroachStartedEvent) | ||
| cluster_initialized = EventSource(ClusterInitializedEvent) |
There was a problem hiding this comment.
Everything here is from CochroachDB, so either we consistently prefix them after the database, which doesn't seem so nice since the class itself already gives these names context, or we avoid the term in all of them. My preference is for the latter.
The first one might be database_started, or daemon_started, etc, depending on what it means.
There was a problem hiding this comment.
Agreed, daemon_started or even daemonized would be better.
src/charm.py
Outdated
| COCKROACHDB_SERVICE = 'cockroachdb.service' | ||
| SYSTEMD_SERVICE_FILE = f'/etc/systemd/system/{COCKROACHDB_SERVICE}' | ||
| WORKING_DIRECTORY = '/var/lib/cockroach' | ||
| COCKROACH_INSTALL_DIR = '/usr/local/bin' | ||
| COCKROACH_BINARY_PATH = f'{COCKROACH_INSTALL_DIR}/cockroach' | ||
| COCKROACH_USERNAME = 'cockroach' | ||
| PSQL_PORT = 26257 | ||
| HTTP_PORT = 8080 | ||
|
|
||
| MAX_RETRIES = 10 | ||
| RETRY_TIMEOUT = timedelta(milliseconds=125) |
There was a problem hiding this comment.
A bit unexpected to see all of these under this class. What's the rationale?
They feel more like properties of the overall charm code than this type in particular. If they indeed are supposed to be changeable somehow depending on this type, though, then they likely shouldn't look like constants. But happy to discuss further looking at established conventions.
There was a problem hiding this comment.
Just wanted to associate them with a particular type until I needed them elsewhere to avoid having globals. I aim to split out the code that mutates the machine state into a separate component - it might be that the need to pull those attributes out will arise at that point.
There was a problem hiding this comment.
Let's discuss further if it's a problem.
Otherwise, I moved some "constants" to a separate type:
e6741a9
src/charm.py
Outdated
| self.framework.observe(event, self) | ||
|
|
||
| self.cluster = CockroachDBCluster(self, 'cluster') | ||
| self.tcp_load_balancer = ProxyListenTcpInterfaceProvides(self, 'proxy-listen-tcp') |
There was a problem hiding this comment.
We need a better type name for that one.
There was a problem hiding this comment.
Agreed in #1 (comment)
src/charm.py
Outdated
| resource_path = None | ||
|
|
||
| if resource_path is None: | ||
| ARCHITECTURE = 'amd64' # hard-coded until it becomes important |
There was a problem hiding this comment.
Why all caps when this is a local variable?
What if resource_path is not None? The variable will be unset and the logic below will break down, I think?
There was a problem hiding this comment.
Why all caps when this is a local variable?
Just to indicate that it is meant to be a constant on purpose.
If a resource is not provided at the deployment time or later, and an attempt is made to retrieve it, a ModelError will be raised which will result in None being assigned to resource_path (so we can just try and download a binary instead). Otherwise, it will be a valid file system path.
| self._setup_systemd_service() | ||
|
|
||
| @property | ||
| def is_single_node(self): |
There was a problem hiding this comment.
New changes:
* moved the code that mutates the system state to a separate type
(DbInstanceManager);
* moved events previously emitted by the charm to DbInstanceManager
* this created a need for the charm code to subscribe the
CockroachDbCluster type to the ClusterInitialized event from
CockroachDbCharm and a subsequent change to how this event is unit
tested in test_cluster.py;
* dependency injection of TestDbInstanceManager is done via a class
attribute which is far from ideal but given the charm type is
initialized by the framework there is no direct way to control its
arguments;
* TestDbInstanceManager overrides the methods that modify the system
state leaving the rest of the logic interesting for testing intact;
3 test cases need uncommenting either partially or fully when PR 196
gets merged:
canonical/operator#196
|
Hey folks, I was very happy to see this charm being worked on, but it seems to have stalled. So, I know it's a bit cheeky to ask, but I'm wondering what the status is of this work? Are Canonical still planning on completing this charm and maybe releasing it on the charm store? 🙏 |
This charm provides means to deploy and operate cockroachdb.
An overview of how it can be used can be found here:
https://github.com/dshcherb/bundle-cockroachdb-ha#overview