chore: Use Ops.CollectStatus Event #53
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -30,9 +30,24 @@ | |
| from lightkube.models.core_v1 import ServicePort, ServiceSpec | ||
| from lightkube.models.meta_v1 import ObjectMeta | ||
| from lightkube.resources.core_v1 import Service | ||
| from ops import ( | ||
| ActiveStatus, | ||
| BlockedStatus, | ||
| CollectStatusEvent, | ||
| MaintenanceStatus, | ||
| ModelError, | ||
| StatusBase, | ||
| WaitingStatus, | ||
| ) | ||
| from ops.charm import ( | ||
| CharmBase, | ||
| EventBase, | ||
| InstallEvent, | ||
| RelationBrokenEvent, | ||
| RelationJoinedEvent, | ||
| RemoveEvent, | ||
| ) | ||
| from ops.main import main | ||
| from ops.pebble import Layer | ||
|
|
||
| logger = logging.getLogger(__name__) | ||
|
|
@@ -65,14 +80,6 @@ class AMFOperatorCharm(CharmBase): | |
|
|
||
| def __init__(self, *args): | ||
| super().__init__(*args) | ||
| self._amf_container_name = self._amf_service_name = "amf" | ||
| self._amf_container = self.unit.get_container(self._amf_container_name) | ||
| self._nrf_requires = NRFRequires(charm=self, relation_name="fiveg-nrf") | ||
|
|
@@ -97,6 +104,12 @@ def __init__(self, *args): | |
| self._database = DatabaseRequires( | ||
| self, relation_name="database", database_name=DATABASE_NAME | ||
| ) | ||
| # Setting attributes to detect broken relations until | ||
| # https://github.com/canonical/operator/issues/940 is fixed | ||
| self._database_relation_breaking = False | ||
|
There was a problem hiding this comment. Hi @benhoyt, According to Juju documentation https://juju.is/docs/sdk/constructs#heading--framework, There was a problem hiding this comment. @gatici I am not experiencing this behaviour. I just brutally added some logs in this revision.
First line prints the default value of There was a problem hiding this comment. @gatici First, there's a discrepancy here between Harness tests (with the same Harness instance) and reality. The charm instance isn't reinitialized between hook executions when the Harness triggers events. This also applies to deferred, which may be what you're seeing here? Normally the charm is a completely new instance (with attributes reset by These discrepancies are a problem that we hope to address that this cycle. See canonical/operator#736 So I wonder if the difference @dariofaccin is seeing is because there were no deferred events in that case (for whatever reason). CC: @tonyandrewmeyer for visibility There was a problem hiding this comment. @benhoyt Many thanks for your help and quick response! |
||
| self._nrf_relation_breaking = False | ||
| self._tls_relation_breaking = False | ||
| self.framework.observe(self.on.collect_unit_status, self._on_collect_unit_status) | ||
| self.framework.observe(self.on.install, self._on_install) | ||
| self.framework.observe(self.on.remove, self._on_remove) | ||
| self.framework.observe(self.on.config_changed, self._configure_amf) | ||
|
|
@@ -124,6 +137,73 @@ def __init__(self, *args): | |
| self._certificates.on.certificate_expiring, self._on_certificate_expiring | ||
| ) | ||
|
|
||
| def _is_unit_in_non_active_status(self) -> Optional[StatusBase]: # noqa: C901 | ||
| """Evaluate and return the unit's current status, or None if it should be active. | ||
|
|
||
| Returns: | ||
| StatusBase: MaintenanceStatus/BlockedStatus/WaitingStatus | ||
| None: If none of the conditionals match | ||
|
|
||
| """ | ||
| if not self.unit.is_leader(): | ||
| # NOTE: In cases where leader status is lost before the charm is | ||
| # finished processing all teardown events, this prevents teardown | ||
| # event code from running. Luckily, for this charm, none of the | ||
| # teardown code is necessary to preform if we're removing the | ||
| # charm. | ||
| return BlockedStatus("Scaling is not implemented for this charm") | ||
|
|
||
| if not self._amf_container.can_connect(): | ||
gatici marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| return MaintenanceStatus("Waiting for service to start") | ||
|
|
||
| if invalid_configs := self._get_invalid_configs(): | ||
| return BlockedStatus(f"The following configurations are not valid: {invalid_configs}") | ||
|
|
||
| if not self.model.get_relation("fiveg-nrf") or self._nrf_relation_breaking: | ||
| return BlockedStatus("Waiting for fiveg-nrf relation") | ||
|
|
||
| if not self.model.get_relation("database") or self._database_relation_breaking: | ||
| return BlockedStatus("Waiting for database relation") | ||
|
Comment on lines
+165
to
+166
There was a problem hiding this comment. Are we sure that
At the moment the modification in integration tests works because the relation is properly removed and There was a problem hiding this comment. In fact self.model.get_relation("database") is generally returning True because of a known issue (https://bugs.launchpad.net/juju/+bug/1979811). Although the integration is removed, it still appears in the model. As self._database_relation_breaking returns True, then unit status is set to Blocked. Juju debug-log: |
||
|
|
||
| if not self.model.get_relation("certificates") or self._tls_relation_breaking: | ||
| return BlockedStatus("Waiting for certificates relation") | ||
|
|
||
| if not self._database_is_available(): | ||
| return WaitingStatus("Waiting for the amf database to be available") | ||
|
|
||
| if not self._get_database_info(): | ||
| return WaitingStatus("Waiting for AMF database info to be available") | ||
|
|
||
| if not self._nrf_requires.nrf_url: | ||
| return WaitingStatus("Waiting for NRF data to be available") | ||
|
|
||
| if not self._amf_container.exists(path=CONFIG_DIR_PATH): | ||
| return WaitingStatus("Waiting for storage to be attached") | ||
|
|
||
| if not _get_pod_ip(): | ||
| return WaitingStatus("Waiting for pod IP address to be available") | ||
|
|
||
| if not self._certificate_is_stored(): | ||
| return WaitingStatus("Waiting for certificates to be stored") | ||
|
|
||
| try: | ||
| self._set_n2_information() | ||
| except ValueError: | ||
| return BlockedStatus("Waiting for MetalLB to be enabled") | ||
|
|
||
| return None | ||
|
|
||
| def _on_collect_unit_status(self, event: CollectStatusEvent): | ||
| """Check the unit status and set to Unit when CollectStatusEvent is fired. | ||
|
|
||
| Args: | ||
| event: CollectStatusEvent | ||
| """ | ||
| if status := self._is_unit_in_non_active_status(): | ||
| event.add_status(status) | ||
| else: | ||
| event.add_status(ActiveStatus()) | ||
|
|
||
| def _on_install(self, event: InstallEvent) -> None: | ||
| client = Client() | ||
| client.apply( | ||
|
|
@@ -176,51 +256,13 @@ def _configure_amf(self, event: EventBase) -> None: # noqa C901 | |
| Args: | ||
| event (PebbleReadyEvent, DatabaseCreatedEvent, NRFAvailableEvent): Juju event | ||
| """ | ||
| if self._is_unit_in_non_active_status(): | ||
| # Unit Status is in Maintanence or Blocked or Waiting status | ||
| event.defer() | ||
| return | ||
|
|
||
| self._generate_config_file() | ||
| self._configure_amf_workload() | ||
|
|
||
| def _on_certificates_relation_created(self, event: EventBase) -> None: | ||
| """Generates Private key.""" | ||
|
|
@@ -229,15 +271,15 @@ def _on_certificates_relation_created(self, event: EventBase) -> None: | |
| return | ||
| self._generate_private_key() | ||
|
|
||
| def _on_certificates_relation_broken(self, event: RelationBrokenEvent) -> None: | ||
| """Deletes TLS related artifacts and reconfigures AMF.""" | ||
| if not self._amf_container.can_connect(): | ||
| event.defer() | ||
| return | ||
| self._delete_private_key() | ||
| self._delete_csr() | ||
| self._delete_certificate() | ||
| self._tls_relation_breaking = True | ||
|
|
||
| def _on_certificates_relation_joined(self, event: EventBase) -> None: | ||
| """Generates CSR and requests new certificate.""" | ||
|
|
@@ -276,21 +318,21 @@ def _on_certificate_expiring(self, event: CertificateExpiringEvent): | |
| return | ||
| self._request_new_certificate() | ||
|
|
||
| def _on_nrf_broken(self, event: RelationBrokenEvent) -> None: | ||
| """Event handler for NRF relation broken. | ||
|
|
||
| Args: | ||
| event (NRFBrokenEvent): Juju event | ||
| """ | ||
| self._nrf_relation_breaking = True | ||
|
|
||
| def _on_database_relation_broken(self, event: RelationBrokenEvent) -> None: | ||
| """Event handler for database relation broken. | ||
|
|
||
| Args: | ||
| event: Juju event | ||
| """ | ||
| self._database_relation_breaking = True | ||
|
|
||
| def _generate_private_key(self) -> None: | ||
| """Generates and stores private key.""" | ||
|
|
@@ -402,7 +444,7 @@ def _on_n2_relation_joined(self, event: RelationJoinedEvent) -> None: | |
| try: | ||
| self._set_n2_information() | ||
| except ValueError: | ||
| logger.info("Waiting for MetalLB to be enabled") | ||
| return | ||
|
|
||
| def _get_n2_amf_ip(self) -> Optional[str]: | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are in no rush to use collect status and I feel like I'd rather wait for the ops fix instead of forcing this change now and reverting some of it when the fix is available. This is quite a large change already right before we want to promote to beta and going ahead with it makes me slightly nervous. I'd at least wait until we have our charms in beta.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. I will freeze the PR's till the ops issue is fixed. Thank you!