Crash due to missing relation about alertmanager-k8s-operator HOT 6 OPEN

Mitigation fix 1

@@ -279,7 +280,15 @@ class AlertmanagerProvider(RelationManagerBase):
             # a single consumer charm's unit may be related to multiple providers
             if self.name in self.charm.model.relations:
                 for relation in self.charm.model.relations[self.name]:
-                    relation.data[self.charm.unit].update(self._generate_relation_data(relation))
+                    # Sometimes there is a dangling relation, for which we get the following error:
+                    # ops.model.ModelError: b'ERROR relation 17 not found (not found)\n'
+                    # when trying to `network-get alerting`. Suppressing the ModelError in this
+                    # case, with the expectation that Juju would resolve the dangling relation
+                    # eventually.
+                    with contextlib.suppress(ModelError):
+                        relation.data[self.charm.unit].update(
+                            self._generate_relation_data(relation)
+                        )

Mitigation fix 2

The following Provider code (alertmanager side) updates relation data with prometheus, so prometheus knows the (public) IP address of alertmanager:

Iirc, bind_address is known to be broken..? @rbarry82 @simskij @mmanciop

In that case I could use the (private) socket.getfqdn().

HOWEVER, the relation.data[self.charm.unit].update(...) bit is probably going to raise anyway, because the relation is gone.

Root cause

Could this be a refcount issue due to the --force used? @rbarry82

from alertmanager-k8s-operator.

Mitigation:

Don't use --force.

bind_address is not known to be broken. It asks Juju directly. See here. Arguably OF should treat subprocess.CalledProcessError in that case as None like it does if it cannot find one, but see the commit message here again. Force "takes out all the stops" and we should expect bad behavior like this.

If anything, this could be a regression in that Juju commit.

Either way, using contextlib.suppress is not the right way to cover it. What happens to the relation data then? try/except, and if there's a ModelError, treat it the same as getting None from get binding and return an empty string as a guardrail. This line will do the right thing anyway, but do we want to rely on that as the only safeguard unless we write a test for it?

from alertmanager-k8s-operator.

(Thanks @rbarry82, will follow up shortly after the following)

After suppressing ModelError in alertmanager, I get something similar in prom:

controller-0: 17:29:59 ERROR juju.worker.caasapplicationprovisioner.runner exited "prom": Operation cannot be fulfilled on pods "prom-0": the object has been modified; please apply your changes to the latest version and try again
controller-0: 17:30:36 ERROR juju.worker.caasapplicationprovisioner.runner exited "prom": application "prom" not found
controller-0: 17:30:39 ERROR juju.worker.caasapplicationprovisioner.runner exited "prom": failed to watch for changes to application "prom": application "prom" not found
# ...
controller-0: 17:32:46 ERROR juju.worker.storageprovisioner failed to set status: cannot set status: filesystem not found
unit-prom-0: 17:32:46 ERROR unit.prom/0.juju-log alertmanager:4: Uncaught exception while in charm code:
Traceback (most recent call last):
  File "/var/lib/juju/agents/unit-prom-0/charm/venv/ops/model.py", line 1598, in _run
    result = run(args, **kwargs)
  File "/usr/lib/python3.8/subprocess.py", line 516, in run
    raise CalledProcessError(retcode, process.args,
subprocess.CalledProcessError: Command '('/var/lib/juju/tools/unit-prom-0/network-get', 'ingress', '--format=json')' returned non-zero exit status 1.

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "./src/charm.py", line 374, in <module>
    main(PrometheusCharm)
  File "/var/lib/juju/agents/unit-prom-0/charm/venv/ops/main.py", line 419, in main
    charm = charm_class(framework)
  File "./src/charm.py", line 58, in __init__
    self.ingress = IngressPerUnitRequirer(self, endpoint="ingress", port=self._port)
  File "/var/lib/juju/agents/unit-prom-0/charm/lib/charms/traefik_k8s/v0/ingress_per_unit.py", line 353, in __init__
    self.auto_data = self._complete_request(host or "", port)
  File "/var/lib/juju/agents/unit-prom-0/charm/lib/charms/traefik_k8s/v0/ingress_per_unit.py", line 376, in _complete_request
    host = str(binding.network.bind_address)
  File "/var/lib/juju/agents/unit-prom-0/charm/venv/ops/model.py", line 558, in network
    self._network = Network(self._backend.network_get(self.name, self._relation_id))
  File "/var/lib/juju/agents/unit-prom-0/charm/venv/ops/model.py", line 1862, in network_get
    return self._run(*cmd, return_output=True, use_json=True)
  File "/var/lib/juju/agents/unit-prom-0/charm/venv/ops/model.py", line 1600, in _run
    raise ModelError(e.stderr)
ops.model.ModelError: b'ERROR relation 4 not found (not found)\n'
unit-prom-0: 17:32:46 ERROR juju.worker.uniter.operation hook "alertmanager-relation-departed" (via hook dispatching script: dispatch) failed: exit status 1

from alertmanager-k8s-operator.

Mitigation:

Don't use --force. Or open a bug against OF to handle this gracefully. This is frankly not for individual charms to handle.

from alertmanager-k8s-operator.

try/except, and if there's a ModelError, treat it the same as getting None from get binding and return an empty string as a guardrail.

Not much sense in updating relation data if the relation is gone.
But I gave it a try anyway:

try:
    relation.data[self.charm.unit].update(
        self._generate_relation_data(relation)
    )
except ModelError:
    relation.data[self.charm.unit].pop("public_address")

which gives the good old ops.model.ModelError: b'ERROR permission denied\n'.

from alertmanager-k8s-operator.

I guess I should clarify:

contextlib.suppress may as well be catch {} in Java or On Error Return in VB6.

It has a limited number of "real" use cases. For what we've seen, the libjuju websocket thing where an unexpected/unsynchronized shutdown may remove the socket out from under you as part of shutdown (or the same with Pebble). Or a truly stateless application which is unaware of whether or not it is holding leadership and other instances may attempt to write.

A ModelError is an actual exception, in that you would be hard pressed to find a case where this should happen during regular operation, and should not be suppressed. It should be explicitly handled with log.warning|error in the except block, and the other side (which assumes an empty .get() is something to ignore rather than another "we should never encounter a relation databag which does not have this set* scenario) should instead return an empty string or something obvious.

Same question:

            address = relation.data[unit].get("public_address")
            if address:
                alertmanagers.append(address)

If you inverted it to if not address:, how many times should that happen? Probably never. There should always be a public_address key if my cursory read of this is correct.

from alertmanager-k8s-operator.