-
Notifications
You must be signed in to change notification settings - Fork 5
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
DPE-1794 Implement COS integration #93
Changes from 3 commits
84af432
a71cbd1
fb501f7
26c99d4
bb7f341
79a5d90
efea884
4a20fa7
927cffe
05302d9
feaa8ad
426bddf
88da3a9
7a241ac
5cb2d56
743604a
0418ba5
234bf0b
44f00cf
8027248
3235040
b215877
6689a35
b16525e
a7e6c77
908a011
8ef349b
b0cba95
1776402
9d03646
d45df31
635d807
d5b41ce
bdb40a9
ade2bff
917d965
6e76961
50044f0
9665f3e
7e49cd7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,14 +11,13 @@ | |
import ops | ||
import tenacity | ||
|
||
import constants | ||
import container | ||
import lifecycle | ||
import logrotate | ||
import machine_upgrade | ||
import relations.cos | ||
import relations.database_provides | ||
import relations.database_requires | ||
import relations.secrets | ||
import server_exceptions | ||
import upgrade | ||
import workload | ||
|
@@ -29,8 +28,6 @@ | |
class MySQLRouterCharm(ops.CharmBase, abc.ABC): | ||
"""MySQL Router charm""" | ||
|
||
_COS_PEER_RELATION_NAME = "cos" | ||
|
||
def __init__(self, *args) -> None: | ||
super().__init__(*args) | ||
# Instantiate before registering other event observers | ||
|
@@ -62,12 +59,6 @@ def __init__(self, *args) -> None: | |
self._upgrade_relation_created, | ||
) | ||
|
||
self.cos_secrets = relations.secrets.RelationSecrets( | ||
self, | ||
self._COS_PEER_RELATION_NAME, | ||
unit_secret_fields=[constants.MONITORING_PASSWORD_KEY], | ||
) | ||
|
||
@property | ||
@abc.abstractmethod | ||
def _subordinate_relation_endpoint_names(self) -> typing.Optional[typing.Iterable[str]]: | ||
|
@@ -76,12 +67,6 @@ def _subordinate_relation_endpoint_names(self) -> typing.Optional[typing.Iterabl | |
Does NOT include relations where charm is principal | ||
""" | ||
|
||
@property | ||
def _tls_certificate_saved(self) -> bool: | ||
"""Whether a TLS certificate is available to use""" | ||
# TODO VM TLS: Remove property after implementing TLS on machine charm | ||
return False | ||
|
||
@property | ||
@abc.abstractmethod | ||
def _container(self) -> container.Container: | ||
|
@@ -97,6 +82,11 @@ def _upgrade(self) -> typing.Optional[upgrade.Upgrade]: | |
def _logrotate(self) -> logrotate.LogRotate: | ||
"""logrotate""" | ||
|
||
@property | ||
@abc.abstractmethod | ||
def _cos(self) -> relations.cos.COSRelation: | ||
"""COS""" | ||
|
||
@property | ||
@abc.abstractmethod | ||
def _read_write_endpoint(self) -> str: | ||
|
@@ -107,6 +97,31 @@ def _read_write_endpoint(self) -> str: | |
def _read_only_endpoint(self) -> str: | ||
"""MySQL Router read-only endpoint""" | ||
|
||
@property | ||
def tls_certificate_saved(self) -> bool: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can this be private method? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. related to #93 (comment) |
||
"""Whether a TLS certificate is available to use""" | ||
# TODO VM TLS: Remove property after implementing TLS on machine_charm | ||
shayancanonical marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return False | ||
|
||
@property | ||
def tls_key(self) -> str: | ||
"""Custom TLS key""" | ||
# TODO VM TLS: Remove property after implementing TLS on machine_charm | ||
return None | ||
|
||
@property | ||
def tls_certificate(self) -> str: | ||
"""Custom TLS certificate""" | ||
# TODO VM TLS: Remove property after implementing TLS on machine_charm | ||
return None | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not in this pr but when this lands in k8s, I think these should be moved to tls.py |
||
|
||
def cos_exporter_config(self, event) -> relations.cos.ExporterConfig: | ||
shayancanonical marked this conversation as resolved.
Show resolved
Hide resolved
|
||
"""Returns the exporter config for MySQLRouter exporter if cos relation exists""" | ||
cos_relation_exists = self._cos.relation_exists and not self._cos.is_relation_breaking( | ||
event | ||
) | ||
return self._cos.exporter_user_config if cos_relation_exists else None | ||
|
||
def get_workload(self, *, event): | ||
"""MySQL Router workload""" | ||
if connection_info := self._database_requires.get_connection_info(event=event): | ||
|
@@ -118,7 +133,7 @@ def get_workload(self, *, event): | |
charm_=self, | ||
) | ||
return self._workload_type( | ||
container_=self._container, logrotate_=self._logrotate, cos=self._cos | ||
container_=self._container, logrotate_=self._logrotate, cos=self._cos, charm_=self | ||
shayancanonical marked this conversation as resolved.
Show resolved
Hide resolved
|
||
) | ||
|
||
@staticmethod | ||
|
@@ -230,7 +245,7 @@ def reconcile(self, event=None) -> None: # noqa: C901 | |
return | ||
if self._upgrade.unit_state == "outdated": | ||
if self._upgrade.authorized: | ||
self._upgrade.upgrade_unit(workload_=workload_, tls=False) | ||
self._upgrade.upgrade_unit(workload_=workload_, tls=self.tls_certificate_saved) | ||
else: | ||
self.set_status(event=event) | ||
logger.debug("Waiting to upgrade") | ||
|
@@ -265,13 +280,7 @@ def reconcile(self, event=None) -> None: # noqa: C901 | |
shell=workload_.shell, | ||
) | ||
if workload_.container_ready: | ||
cos_relation_exists = ( | ||
self._cos.relation_exists and not self._cos.is_relation_breaking(event) | ||
) | ||
workload_.reconcile( | ||
unit_name=self.unit.name, | ||
exporter_config=self._cos.exporter_user_info if cos_relation_exists else None, | ||
) | ||
workload_.reconcile_services(event) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. please use inputs instead of passing the entire charm instance with indirect inputs (so that the dependencies are explicit & reduced) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. passing inputs is acceptable when calling however, i am not a fan of tls files import cos just so it can pass in the necessary inputs solution proposed in this PR:
as an alternative, i am ok with tls files calling the properties on the charm class (as it will have the reference to the charm object) rather than a wrapper ( There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
yeah, my suggestion was to call the reconcile event handler from tls code (similar to https://github.com/canonical/mysql-router-k8s-operator/blob/d43a43481b7f966db340fa994a93cb18cca592dd/src/relations/tls.py#L136) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. addressed in 50044f0 |
||
# Empty waiting status means we're waiting for database requires relation before | ||
# starting workload | ||
if not workload_.status or workload_.status == ops.WaitingStatus(): | ||
|
This file was deleted.
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.
is relations/cos.py vm specific?
if not, this method shouldn't be abstract
if so, the type here should be an abstract implementation, not a concrete implementation (feel free to ignore for now, but keep in mind for k8s)
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.
related to #93 (comment)