Add dbcluster peer relation #56
Conversation
src/charm.py
Outdated
| @@ -39,7 +44,7 @@ def _on_start(self, _): | |||
| self.unit.status = ActiveStatus() | |||
There was a problem hiding this comment.
you shouldn't do this unconditionally in __init__.
Suppose you receive website_relation_joined and you go in Blocked, next event you see (whichever event!) you'll go back to active and the user will never know that there's something wrong with the charm.
There was a problem hiding this comment.
Yes, I understand. Is there a good example of this appropriate to a (currently) simple charm like this one?
There was a problem hiding this comment.
you can look at our most recent charm: https://github.com/canonical/mimir-coordinator-k8s-operator/blob/main/src/charm.py#L103
in a nutshell: observe the collect_unit_status event, and add several 'is this healthy?' endpoints that each adds a piece of status to the event. Either blocked or active.
Ops will take the worst status and set it to the unit, on each event.
| return | ||
|
|
||
| self._db_bind_address = ip | ||
| event.relation.data[self.unit].update({'db-bind-address': ip}) |
There was a problem hiding this comment.
the remote unit can always access a remote's IP via `event.relation.data[event.relation.unit]["ingress-address"], although I'm not sure if it's the same 'ingress address' we're talking about here.
There was a problem hiding this comment.
try jhack show-relation --show-juju-keys db controller to see for yourself what's in there
There was a problem hiding this comment.
ingres-address as set automatically during scope entry is actually just pulled from the zero index of the sorted addresses determined by retrieving network info (the same backing as the call to the network-get tool.
We actually want to give the assurance that the list is constituted by a single unique entry, hence the logic here. Going into the blocked state if we don't have a unique address is a message to the user that they need to set the endpoint binding to an appropriate space.
| @patch("ops.model.Model.get_binding") | ||
| def test_dbcluster_relation_changed_multi_addr_error(self, binding): | ||
| harness = self.harness | ||
| binding.return_value = mockBinding(["192.168.1.17", "192.168.1.18"]) |
There was a problem hiding this comment.
personally I'd use a MagicMock instead of creating these custom types but that's a preference.
binding = MagicMock()
binding.network.ingress_addresses = [...]
binding.network.ingress_address = ...
manadart
force-pushed
the
add-db-cluster
branch
from
adf37a0
to
3465817
Compare
| def _on_start(self, _): | ||
| self.unit.status = ActiveStatus() | ||
| def _on_collect_status(self, event: CollectStatusEvent): | ||
| if len(self._stored.last_bind_addresses) > 1: |
There was a problem hiding this comment.
unless you're doing it on purpose for readability, you can equivalently:
| if len(self._stored.last_bind_addresses) > 1: | |
| if self._stored.last_bind_addresses: |
There was a problem hiding this comment.
len > 1 is having more than 1, not having more than 0. We're explicitly testing for the case that you don't have a singular unique value.
There was a problem hiding this comment.
That would be > 0 though, right?
There was a problem hiding this comment.
Ah my bad, misread that. You're totally right.
| def _on_start(self, _): | ||
| self.unit.status = ActiveStatus() | ||
| def _on_collect_status(self, event: CollectStatusEvent): | ||
| if len(self._stored.last_bind_addresses) > 1: |
There was a problem hiding this comment.
len > 1 is having more than 1, not having more than 0. We're explicitly testing for the case that you don't have a singular unique value.
|
@manadart your commits to this branch aren't signed. Or go back and sign the chain of them with: |
manadart
force-pushed
the
add-db-cluster
branch
2 times, most recently
from
8d2c5e3
to
9cf3fcd
Compare
relation data if we have a unique ingress address, or become blocked if there is not a unique address.
Instead we handle the collect_unit_status event, using our known state to supply candidate status values.
manadart
force-pushed
the
add-db-cluster
branch
from
9cf3fcd
to
980e584
Compare
This adds a new peer relation to the controller charm:
dbcluster.The binding for this relation will ultimately replace the Juju controller configuration article,
juju-ha-space. Accordingly, the charm ensures that the current bound space yields a unique ingress address that gets set in relation data. If the address is not unique, the charm enters the blocked state.Further work will follow that will write a charm-sourced configuration file to disk that will be an analogue for agent configuration.