DPE-1794 Implement COS integration #93

shayancanonical · 2023-11-21T23:35:06Z

Issue

We need to introduce COS support for MySQLRouter

Solution

Introduce COS support for MySQLRouter
Introduce the scaffolding for secrets to be able to store the monitoring password in the juju secrets backend

Prerequisites

We need to merge the snap PR that adds the COS machinery to the snap. After, we need to update the snap revision in the snap.py file
We need to import the secrets lib after the following secrets PR in data-platform-libs is merged

TODO

~~Determine why the All Connections Information and Route byte to/from server sections on the dashboard are not populated when more than 1 routers are deployed~~ (see DPE-1794 Implement COS integration #93 (comment))
~~Test router log collection (via Loki) when the following issue is resolved~~ (see DPE-1794 Implement COS integration #93 (comment))

Demo

carlcsaposs-canonical

what's the purpose of using Juju secrets/are they necessary?

shayancanonical · 2023-11-27T13:50:16Z

I believe that there are small things which result in using juju secrets to store the monitoring password a good idea:

What if the admin wants to retrieve the monitoring password in order to use the password manually? We would have no way of implementing the get-password user=monitoring action
We would need to chain the ephemeral password through a bunch of functions in a stateless approach (which is a minor maintainability nit)

…e config file

carlcsaposs-canonical

(still reviewing, haven't looked at most of it yet)

pyproject.toml

src/snap.py

src/utils.py

lib/charms/data_platform_libs/v0/data_secrets.py

carlcsaposs-canonical · 2023-11-30T12:04:49Z

metadata.yaml

+  mysql-router-peers:
+    interface: mysql_router_peers


nit: could you name this based on what the relation is used for?

my thinking is that it's a better practice to use separate peer relations for separate things, so that you get different events for different things (e.g. upgrades uses its own peer relation)

our convention has been <charm_name>_peers to store all internal charm related data in the peerbag as well as secrets. please let me know if you can think of a better name for this interface/endpoint

don't think this comment went through on last review: suggest cos-rest-api-password or something similar

src/abstract_charm.py

src/relations/cos.py

src/snap.py

src/workload.py

src/relations/cos.py

tests/unit/conftest.py

shayancanonical · 2023-12-01T02:55:09Z

@carlcsaposs-canonical @taurus-forever refactored and polished quite a bit addressing this last iteration of PR feedback

the main functional change: we will always use the rest_api_credentials file for the HTTP server. this enables the disabling/enabling of the router everytime there is a relation formed/broken with COS. instead, we will only change the credentials in this rest_api_credentials file everytime we form/break a COS relation

shayancanonical · 2023-12-01T02:55:18Z

@carlcsaposs-canonical @taurus-forever refactored and polished quite a bit addressing this last iteration of PR feedback

the main functional change: we will always use the rest_api_credentials file for the HTTP server. this enables the disabling/enabling of the router everytime there is a relation formed/broken with COS. instead, we will only change the credentials in this rest_api_credentials file everytime we form/break a COS relation

src/workload.py

paulomach

Overall looks good
For integration testing, wouldn't deploying grafana-agent-k8s and relating, be enough to start the exporter and test the endpoints?

src/container.py

src/relations/cos.py

src/snap.py

shayancanonical · 2023-12-04T14:33:24Z

Added an integration test for the exporter endpoint (when relation formed with COS) in 05302d9

…rs on CI

…t condition

taurus-forever

LGTM, one minor change (can be done later or skipped).
Please fire all TODOs from PR descriptions as Jira tickets.

tests/integration/test_exporter.py

shayancanonical · 2024-03-11T13:11:14Z

Confirmed that all the TODOs in the PR description are no longer relevant.

Confirmed that 2 MySQLRouter applications integrated with the same COS stack results in functioning dashboards (fully populated) for both applications
Confirmed that logs flowing through to loki and accessible through COS grafana for both MySQLRouter applications

src/container.py

src/abstract_charm.py

src/workload.py

tests/unit/conftest.py

tests/integration/test_exporter.py

…ets in data_interfaces

src/snap.py

src/container.py

carlcsaposs-canonical · 2024-03-14T15:48:24Z

src/abstract_charm.py

                workload_.reconcile(
-                    tls=self._tls_certificate_saved,


this is used in k8s charm & abstract_charm.py is shared with k8s charm

looking at k8s charm, looks like there might be a bug with self._tls_certificate_saved not beginning with underscore in kubernetes_charm.py

carlcsaposs-canonical · 2024-03-14T15:49:13Z

src/workload.py

+        *,
+        container_: container.Container,
+        logrotate_: logrotate.LogRotate,
+        cos_: "relations.cos.COSRelation",


cos shouldn't conflict with relations.cos (would only conflict with import cos)

am I mistaken?

carlcsaposs-canonical · 2024-03-14T15:56:19Z

src/workload.py

+        if tls:
+            self.enable_tls() and self._container.mysql_router_service_enabled and self._restart(
+                tls=tls
+            )


imo, it would be more robust to have tls input on reconcile and call reconcile from TLS file

so that everything uses the reconcile approach instead of an implicit dependency in the reconcile loop on the event-based tls enable/disable

src/relations/cos.py

carlcsaposs-canonical · 2024-03-14T16:14:44Z

src/relations/cos.py

 import ops
 from charms.grafana_agent.v0.cos_agent import COSAgentProvider

+import abstract_charm


does this cause circular import?

outdated. it indeed caused a circular import

carlcsaposs-canonical · 2024-03-14T16:15:08Z

src/machine_charm.py

+        self.peer_relation_app = data_interfaces.DataPeer(
+            self,
+            relation_name=self._PEER_RELATION_NAME,
+            secret_field_name=self._SECRET_INTERNAL_LABEL,
+            deleted_label=self._SECRET_DELETED_LABEL,
+        )
+        self.peer_relation_unit = data_interfaces.DataPeerUnit(
+            self,
+            relation_name=self._PEER_RELATION_NAME,
+            additional_secret_fields=[
+                relations.cos.MONITORING_PASSWORD_KEY,
+            ],
+            secret_field_name=self._SECRET_INTERNAL_LABEL,
+            deleted_label=self._SECRET_DELETED_LABEL,
+        )


any reason this is in machine_charm.py instead of abstract_charm.py?

outdated: moved to cos file

carlcsaposs-canonical · 2024-03-14T16:15:14Z

src/machine_charm.py

        # DEPRECATED shared-db: Enable legacy "mysql-shared" interface
-        del self._database_provides
        self._database_provides = relations.database_providers_wrapper.RelationEndpoint(self)


why was this removed?

Removed due to the following error: https://github.com/canonical/mysql-router-operator/actions/runs/8233518223/job/22513139520

As far as I can tell, we are instantiating _database_provides.RelationEndpoint twice with the same unique identifier. I remove the variable declaration in the super class, and we will ensure that both the machine_charm and k8s_charm files define it exactly once

did the test failure recently appear? I don't think I've seen it before

it appeared when i upgraded the data_interfaces charm lib

that seems like it might be a bug in the lib; suggest reporting

if that bug won't get fixed in the lib, would recommend using self._database_provides as an argument to __init__ for database_providers_wrapper.RelationEndpoint so that abstract_charm.py can remain unchanged (and typed correctly)

carlcsaposs-canonical · 2024-03-14T16:17:29Z

metadata.yaml

+  mysql-router-peers:
+    interface: mysql_router_peers


don't think this comment went through on last review: suggest cos-rest-api-password or something similar

carlcsaposs-canonical · 2024-03-14T16:19:06Z

src/machine_charm.py

+        self.peer_relation_app = data_interfaces.DataPeer(
+            self,
+            relation_name=self._PEER_RELATION_NAME,
+            secret_field_name=self._SECRET_INTERNAL_LABEL,
+            deleted_label=self._SECRET_DELETED_LABEL,
+        )


are app secrets used?

app secrets is not used at the moment, but it is needed for the proven get_secret() and set_secret() methods. the code will soon be replaced when new charm structure is adopted, so not worth tinkering much with it. this does not create an app scoped secrets. just allows storing app related secrets if we want to in the future

…e file

src/abstract_charm.py

src/constants.py

src/snap.py

src/abstract_charm.py

carlcsaposs-canonical · 2024-03-15T15:47:09Z

src/abstract_charm.py

@@ -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:


can this be private method?

related to #93 (comment)

carlcsaposs-canonical · 2024-03-15T15:55:02Z

src/abstract_charm.py

+    @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


not in this pr but when this lands in k8s, I think these should be moved to tls.py

similar to https://github.com/canonical/mysql-router-k8s-operator/blob/d43a43481b7f966db340fa994a93cb18cca592dd/src/kubernetes_charm.py#L53

src/abstract_charm.py

carlcsaposs-canonical · 2024-03-15T15:58:39Z

src/abstract_charm.py

+    @property
+    @abc.abstractmethod
+    def _cos(self) -> relations.cos.COSRelation:
+        """COS"""
+


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)

related to #93 (comment)

src/workload.py

…ntil

… externally

shayancanonical · 2024-03-19T08:49:48Z

Thanks for the reviews! Merging, and will iterate on further improvements with follow up PRs

## Issue We need to introduce support for COS Fixes #218 ## Solution 1. Introduce support for COS (ported from [PR in MySQLRouter VM](canonical/mysql-router-operator#93)) 2. Fix all TLS databag usage to instead use secrets (using data_interfaces charm lib) ## TODO File bug regarding intermittent issue where router exporter does not start up cleanly (due to bind-address in use) which auto-resolves due to pebble retries. ## Demo Grafana dashboard: ![image](https://github.com/canonical/mysql-router-k8s-operator/assets/99665202/13ca1ebb-5c34-45dd-9c59-3a80e5f166ca) Juju model tested along with TLS certificates operator: ![image](https://github.com/canonical/mysql-router-k8s-operator/assets/99665202/3af37174-899a-40a9-8d2d-7c9a0170ce55)

shayancanonical added 2 commits November 20, 2023 12:47

WIP: Initial implementation of COS with a lot of rough edges

84af432

WIP: rough working prototype of cos integration

a71cbd1

shayancanonical requested review from carlcsaposs-canonical, taurus-forever and paulomach and removed request for carlcsaposs-canonical November 21, 2023 23:35

carlcsaposs-canonical reviewed Nov 24, 2023

View reviewed changes

shayancanonical added 4 commits November 27, 2023 20:12

Replace secrets charm lib with published one

fb501f7

Merge branch 'main' into feature/cos_integration

26c99d4

WIP

bb7f341

Fix bug where credentials were passed to router options instead of th…

79a5d90

…e config file

carlcsaposs-canonical reviewed Nov 28, 2023

View reviewed changes

carlcsaposs-canonical reviewed Nov 30, 2023

View reviewed changes

taurus-forever reviewed Nov 30, 2023

View reviewed changes

tests/unit/conftest.py Outdated Show resolved Hide resolved

Address PR feedback

efea884

carlcsaposs-canonical reviewed Dec 1, 2023

View reviewed changes

src/workload.py Outdated Show resolved Hide resolved

paulomach reviewed Dec 1, 2023

View reviewed changes

src/container.py Outdated Show resolved Hide resolved

src/relations/cos.py Show resolved Hide resolved

src/snap.py Outdated Show resolved Hide resolved

shayancanonical added 3 commits December 1, 2023 18:58

Address PR feedback + update data_interfaces and cos_agent charm libs

4a20fa7

Merge branch 'main' into feature/cos_integration

927cffe

Add integration test for the exporter endpoint

05302d9

shayancanonical added 5 commits December 4, 2023 19:53

Increase timeout for exporter tests to avoid unnecessary timeout erro…

feaa8ad

…rs on CI

Skip exporter tests on focal

426bddf

Delete extra ) from CI workflow file

88da3a9

Try removing curly braces from integration test condition in ci.yaml

7a241ac

Try using single quotes for matrix variable values in integration tes…

5cb2d56

…t condition

shayancanonical requested review from paulomach, taurus-forever and carlcsaposs-canonical March 7, 2024 13:50

taurus-forever approved these changes Mar 11, 2024

View reviewed changes

tests/integration/test_exporter.py Outdated Show resolved Hide resolved

shayancanonical added 2 commits March 11, 2024 13:13

Address minor PR feedback

8ef349b

Update outdated charm libs

b0cba95

carlcsaposs-canonical reviewed Mar 11, 2024

View reviewed changes

shayancanonical added 4 commits March 13, 2024 17:40

Address PR feedback

1776402

Minor leftover improvements + fix bugs

9d03646

Fix typo in method call

d45df31

Fix bugs + remove usage of data_secrets and use dynamic usage of secr…

635d807

…ets in data_interfaces

carlcsaposs-canonical reviewed Mar 14, 2024

View reviewed changes

Another round of feedback + move abstracted secrets code to a separat…

d5b41ce

…e file

carlcsaposs-canonical reviewed Mar 15, 2024

View reviewed changes

src/abstract_charm.py Outdated Show resolved Hide resolved

src/abstract_charm.py Outdated Show resolved Hide resolved

src/constants.py Outdated Show resolved Hide resolved

src/snap.py Outdated Show resolved Hide resolved

shayancanonical added 3 commits March 15, 2024 12:43

Address PR feedback

bdb40a9

Leftovers cleanup

ade2bff

Fix bug introduced during refactor

917d965

carlcsaposs-canonical reviewed Mar 15, 2024

View reviewed changes

shayancanonical added 4 commits March 15, 2024 20:27

Address feedback + make database test more resilient by using block_u…

6e76961

…ntil

Avoid using reconcile_services wrapper in workload

50044f0

Make properties in abstract charm private as they will not be invoked…

9665f3e

… externally

Instantiate database_provides in abstract_charm for type hinting

7e49cd7

shayancanonical merged commit a7c99af into main Mar 19, 2024
20 checks passed

shayancanonical deleted the feature/cos_integration branch March 19, 2024 08:50

shayancanonical mentioned this pull request Mar 22, 2024

[DPE-1795] Implement support for COS canonical/mysql-router-k8s-operator#210

Merged

carlcsaposs-canonical mentioned this pull request Apr 22, 2024

Upgrades broken #130

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DPE-1794 Implement COS integration #93

DPE-1794 Implement COS integration #93

shayancanonical commented Nov 21, 2023 •

edited

Loading

carlcsaposs-canonical left a comment

shayancanonical commented Nov 27, 2023

carlcsaposs-canonical left a comment

carlcsaposs-canonical Nov 30, 2023

shayancanonical Dec 1, 2023

carlcsaposs-canonical Mar 14, 2024

shayancanonical commented Dec 1, 2023

shayancanonical commented Dec 1, 2023

paulomach left a comment •

edited

Loading

shayancanonical commented Dec 4, 2023

taurus-forever left a comment

shayancanonical commented Mar 11, 2024

carlcsaposs-canonical Mar 14, 2024

carlcsaposs-canonical Mar 14, 2024

carlcsaposs-canonical Mar 14, 2024

carlcsaposs-canonical Mar 14, 2024

shayancanonical Mar 15, 2024

carlcsaposs-canonical Mar 14, 2024

shayancanonical Mar 15, 2024

carlcsaposs-canonical Mar 14, 2024

shayancanonical Mar 15, 2024

carlcsaposs-canonical Mar 15, 2024

shayancanonical Mar 15, 2024

carlcsaposs-canonical Mar 18, 2024

carlcsaposs-canonical Mar 14, 2024

carlcsaposs-canonical Mar 14, 2024

shayancanonical Mar 15, 2024

carlcsaposs-canonical Mar 15, 2024

shayancanonical Mar 15, 2024

carlcsaposs-canonical Mar 15, 2024

carlcsaposs-canonical Mar 15, 2024

shayancanonical Mar 15, 2024

shayancanonical commented Mar 19, 2024

DPE-1794 Implement COS integration #93

DPE-1794 Implement COS integration #93

Conversation

shayancanonical commented Nov 21, 2023 • edited Loading

Issue

Solution

Prerequisites

TODO

Demo

carlcsaposs-canonical left a comment

Choose a reason for hiding this comment

shayancanonical commented Nov 27, 2023

carlcsaposs-canonical 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

shayancanonical commented Dec 1, 2023

shayancanonical commented Dec 1, 2023

paulomach left a comment • edited Loading

Choose a reason for hiding this comment

shayancanonical commented Dec 4, 2023

taurus-forever left a comment

Choose a reason for hiding this comment

shayancanonical commented Mar 11, 2024

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

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

shayancanonical commented Mar 19, 2024

shayancanonical commented Nov 21, 2023 •

edited

Loading

paulomach left a comment •

edited

Loading