Use collect-status #202

sed-i · 2023-10-13T20:00:45Z

Issue

Status updates are communicated via exceptions.

Solution

Use collect-status.

In tandem with:

[resource-limits] Add last_error public attribute, to enable charm to use collect-status observability-libs#63

dstathis · 2023-10-17T11:10:15Z

src/charm.py

+        # Check open ports
+        if self.unit.opened_ports() != set(PORTS.values()):
+            event.add_status(WaitingStatus(f'Opening {", ".join(PORTS.keys())} ports'))


This should be maintenance status. Waiting is for waiting on related charms.

True. Although I wonder whether this particular check should probably be removed entirely. open-port goes through right away. Juju itself may take time to perform it, but the charm doesn't, and this status won't reflect when the K8s operation actually goes through anyway.

Agreed on both comments :)

dstathis · 2023-10-17T11:12:39Z

src/charm.py

+        # Make sure the external url is valid
+        if not _is_external_url_valid(self._external_url):
+            # This shouldn't happen
+            event.add_status(
+                BlockedStatus(
+                    f"Invalid external url: '{self._external_url}'; must include scheme and hostname."
+                )
+            )


I believe this should be an error status. The external url is not set manually right? So there is no admin intervention that can fix it. The fact that you say "This shouldn't happen" is likely a hint that it should be error as well.

If it can't happen, yeah, you're right. However, I believe charms can't set an error status, that's from Juju itself (if the hook can't be run, for example). So maybe if this "can't happen" you should check it further back, when self._external_url is being set, and just raise an exception so the charm goes into error state.

This is a leftover from when external url was a config option. Should be removed.

dstathis · 2023-10-17T11:14:37Z

src/charm.py

+        if self.last_error:
+            event.add_status(BlockedStatus(self.last_error))


I think this should also be error status I think because these, as far as I can tell, are miscellaneous un-handled errors.

dstathis · 2023-10-17T11:15:48Z

src/charm.py

+        if not _is_external_url_valid(self._external_url):
+            # This shouldn't happen
+            logger.error(
+                "Invalid external url: '%s'; must include scheme and hostname.",
+                self._external_url,
+            )
+            return


Set self.last_error here?

benhoyt

Looks like a pretty good use of collect-status overall! Added a few comments to think about.

benhoyt · 2023-10-23T23:14:23Z

tests/unit/test_charm.py

        self.assertIsInstance(self.harness.charm.unit.status, BlockedStatus)

+        # Manual cleanup, due to https://github.com/canonical/operator/issues/736
+        self.harness.charm.unit._collected_statuses.clear()


Presumably this part of it (and similar lines below) will be fixed by canonical/operator#1048?

benhoyt · 2023-10-23T23:18:50Z

src/charm.py

    Relation,
    WaitingStatus,
 )
 from ops.pebble import PathError, ProtocolError  # type: ignore

 logger = logging.getLogger(__name__)

+PORTS = {"api": Port("tcp", 9093), "ha": Port("tcp", 9094)}


Per other discussion, I think you'd be better off with:

API_PORT = 9093 HA_PORT = 9094 PORTS = [API_PORT, HA_PORT] ... self.unit.set_ports(*PORTS)

benhoyt · 2023-10-23T23:28:13Z

src/charm.py

+        # Resource limit
+        if not self.resources_patch.is_ready():
+            event.add_status(
+                MaintenanceStatus("[resource patch] Waiting for resource limit patch to apply")


When designing collect-status, we thought about including a prefix/component name automatically, but decided against it as status messages should be nicely-designed UI for the user. This kind of shows why: the fact that it's resource patch-related is already clear from the message part, so the [resource patch] prefix just adds noise to the UI. Something to consider...

Agreed, but in the general case we could have multiple instances of the same lib.

Prometheus: prometheus_scrape is a requirer and provider (two different classes though)

Kratos uses two ingress requirers

benhoyt · 2023-10-23T23:28:28Z

src/charm.py

+                MaintenanceStatus("[resource patch] Waiting for resource limit patch to apply")
+            )
+        if self.resources_patch.last_error:
+            event.add_status(BlockedStatus(f"[resource patch] {self.last_error}"))


Should this be:

Suggested change

event.add_status(BlockedStatus(f"[resource patch] {self.last_error}"))

event.add_status(BlockedStatus(f"[resource patch] {self.resources_patch.last_error}"))

Also, I can see why you used the prefix here, however, will the error messages from the resource patch object already include enough context to avoid the need for it, and give a nicer status message?

Perhaps, but this would also mean we'd need to chase potential changes of the 3rd party package.

benhoyt · 2023-10-23T23:32:10Z

src/charm.py

+        # Check open ports
+        if self.unit.opened_ports() != set(PORTS.values()):
+            event.add_status(WaitingStatus(f'Opening {", ".join(PORTS.keys())} ports'))


True. Although I wonder whether this particular check should probably be removed entirely. open-port goes through right away. Juju itself may take time to perform it, but the charm doesn't, and this status won't reflect when the K8s operation actually goes through anyway.

benhoyt · 2023-10-23T23:33:16Z

src/charm.py

+        # "pebble ready"
+        if not self.container.can_connect():
+            event.add_status(
+                MaintenanceStatus(f"Waiting for '{self.container.name}' container to become ready")


Consider the following. "to become ready" is kind of redundant.

Suggested change

MaintenanceStatus(f"Waiting for '{self.container.name}' container to become ready")

MaintenanceStatus(f"Waiting for container {self.container.name!r}")

benhoyt · 2023-10-23T23:35:36Z

src/charm.py

+        # Make sure the external url is valid
+        if not _is_external_url_valid(self._external_url):
+            # This shouldn't happen
+            event.add_status(
+                BlockedStatus(
+                    f"Invalid external url: '{self._external_url}'; must include scheme and hostname."
+                )
+            )


If it can't happen, yeah, you're right. However, I believe charms can't set an error status, that's from Juju itself (if the hook can't be run, for example). So maybe if this "can't happen" you should check it further back, when self._external_url is being set, and just raise an exception so the charm goes into error state.

benhoyt · 2023-10-23T23:43:23Z

src/charm.py

@@ -428,7 +447,7 @@ def _common_exit_hook(self, update_ca_certs: bool = False) -> None:
        try:
            self.alertmanager_workload.update_config(self._render_manifest())
        except (ConfigUpdateFailure, ConfigError) as e:
-            self.unit.status = BlockedStatus(str(e))
+            self.last_error = str(e)


This is interesting. I can see why you're storing state in this attribute here (and below) in the common exit hook. However, normally you'd query in collect-status the current state of things, rather than look at previously-set state attributes. Might be tricky for this though?

sed-i

Thanks for reviewing @benhoyt and @dstathis!

I don't think this is ready to merge in light of the comments in the lib PR since:

we have custom events
multiple entry points into a lib
we shouldn't use collect_status outside of charm.py

My impression is that those constraints prevent us from a solid collect-status pattern.
I'm going to close this for now.

sed-i · 2023-10-24T21:53:35Z

src/charm.py

    Relation,
    WaitingStatus,
 )
 from ops.pebble import PathError, ProtocolError  # type: ignore

 logger = logging.getLogger(__name__)

+PORTS = {"api": Port("tcp", 9093), "ha": Port("tcp", 9094)}


sed-i · 2023-10-24T21:58:04Z

src/charm.py

+        # Resource limit
+        if not self.resources_patch.is_ready():
+            event.add_status(
+                MaintenanceStatus("[resource patch] Waiting for resource limit patch to apply")


Agreed, but in the general case we could have multiple instances of the same lib.

Prometheus: prometheus_scrape is a requirer and provider (two different classes though)

Kratos uses two ingress requirers

sed-i · 2023-10-24T21:59:16Z

src/charm.py

+                MaintenanceStatus("[resource patch] Waiting for resource limit patch to apply")
+            )
+        if self.resources_patch.last_error:
+            event.add_status(BlockedStatus(f"[resource patch] {self.last_error}"))


Perhaps, but this would also mean we'd need to chase potential changes of the 3rd party package.

sed-i · 2023-10-24T21:59:36Z

src/charm.py

+        # Check open ports
+        if self.unit.opened_ports() != set(PORTS.values()):
+            event.add_status(WaitingStatus(f'Opening {", ".join(PORTS.keys())} ports'))


Agreed on both comments :)

sed-i · 2023-10-24T22:00:07Z

src/charm.py

+        # "pebble ready"
+        if not self.container.can_connect():
+            event.add_status(
+                MaintenanceStatus(f"Waiting for '{self.container.name}' container to become ready")


sed-i · 2023-10-24T22:00:38Z

src/charm.py

+        # Make sure the external url is valid
+        if not _is_external_url_valid(self._external_url):
+            # This shouldn't happen
+            event.add_status(
+                BlockedStatus(
+                    f"Invalid external url: '{self._external_url}'; must include scheme and hostname."
+                )
+            )


This is a leftover from when external url was a config option. Should be removed.

sed-i · 2023-10-24T22:01:02Z

tests/unit/test_charm.py

        self.assertIsInstance(self.harness.charm.unit.status, BlockedStatus)

+        # Manual cleanup, due to https://github.com/canonical/operator/issues/736
+        self.harness.charm.unit._collected_statuses.clear()


sed-i added 3 commits October 12, 2023 23:23

Use open_ports

193c2a1

WIP collect-status

5e454da

WIP collect-status

666eb73

sed-i mentioned this pull request Oct 13, 2023

[resource-limits] Add last_error public attribute, to enable charm to use collect-status canonical/observability-libs#63

Closed

github-actions bot added the Libraries: Out of sync label Oct 13, 2023

Set status only in the charm

da6f7bc

sed-i marked this pull request as ready for review October 17, 2023 00:52

sed-i requested review from Abuelodelanada, dstathis, simskij, lucabello and PietroPasotti as code owners October 17, 2023 00:52

dstathis reviewed Oct 17, 2023

View reviewed changes

benhoyt mentioned this pull request Oct 23, 2023

Charm should be reinitialized at every hook execution in Harness canonical/operator#736

Closed

benhoyt reviewed Oct 23, 2023

View reviewed changes

sed-i commented Oct 24, 2023

View reviewed changes

sed-i closed this Oct 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use collect-status #202

Use collect-status #202

sed-i commented Oct 13, 2023

dstathis Oct 17, 2023

benhoyt Oct 23, 2023

sed-i Oct 24, 2023

dstathis Oct 17, 2023

benhoyt Oct 23, 2023

sed-i Oct 24, 2023

dstathis Oct 17, 2023

dstathis Oct 17, 2023

benhoyt left a comment

benhoyt Oct 23, 2023

sed-i Oct 24, 2023

benhoyt Oct 23, 2023

sed-i Oct 24, 2023

benhoyt Oct 23, 2023

sed-i Oct 24, 2023

benhoyt Oct 23, 2023

benhoyt Oct 23, 2023

sed-i Oct 24, 2023

benhoyt Oct 23, 2023

benhoyt Oct 23, 2023

sed-i Oct 24, 2023

benhoyt Oct 23, 2023

benhoyt Oct 23, 2023

sed-i left a comment

sed-i Oct 24, 2023

sed-i Oct 24, 2023

sed-i Oct 24, 2023

sed-i Oct 24, 2023

sed-i Oct 24, 2023

sed-i Oct 24, 2023

sed-i Oct 24, 2023

		if self.last_error:
		event.add_status(BlockedStatus(self.last_error))

	event.add_status(BlockedStatus(f"[resource patch] {self.last_error}"))
	event.add_status(BlockedStatus(f"[resource patch] {self.resources_patch.last_error}"))

	MaintenanceStatus(f"Waiting for '{self.container.name}' container to become ready")
	MaintenanceStatus(f"Waiting for container {self.container.name!r}")

Use collect-status #202

Use collect-status #202

Conversation

sed-i commented Oct 13, 2023

Issue

Solution

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benhoyt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sed-i left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment