Miscellaneous improvements to the mysql legacy relation #254

shayancanonical · 2023-07-31T12:09:15Z

Issue

The following are discrepancies between the legacy mysql relation code in vms vs. k8s:

We are not correctly handling exception cases when calling get_cluster_primary_address (where the return value is None)
We have no exception handling when calling does_mysql_user_exist
We are accessing the root-password directly from the app peer databag rather than using the get_secret helper (which will also retrieve it from the databag in juju v2.9.43)
We are not deleting legacy mysql relation data from the peer databag upon relation broken

Solution

Check if the return value for get_cluster_primary_address is None and if so, avoid calling .split() on None
Add exception handling around does_mysql_user_exist
Access root-password using the get_secret helper
Delete the relation data from the peer databag upon relation broken

src/relations/mysql.py

codecov · 2023-07-31T12:29:42Z

Codecov Report

Merging #254 (087f036) into main (2ff030b) will decrease coverage by 0.24%.
The diff coverage is 28.57%.

@@            Coverage Diff             @@
##             main     #254      +/-   ##
==========================================
- Coverage   63.80%   63.56%   -0.24%     
==========================================
  Files          15       15              
  Lines        2815     2830      +15     
  Branches      365      367       +2     
==========================================
+ Hits         1796     1799       +3     
- Misses        904      915      +11     
- Partials      115      116       +1

Files Changed	Coverage Δ
src/relations/mysql.py	`52.21% <28.57%> (-4.94%)`	⬇️

taurus-forever

This is great, thank you!
P.S. I would use error state instead of blocked, but we will improve this in a general way for all blocked states we use.

paulomach

lgtm

taurus-forever

Nice findings! Tnx!

carlcsaposs-canonical · 2023-08-21T13:32:59Z

src/relations/mysql.py

-            "host": primary_address,
+            "host": primary_address.split(":")[0],
            "password": password,
            "port": "3306",


should port be retrieved from primary_address instead of hard-coded?

We are hardcoding 3306 as the port in numerous places - I'd say we should address it holistically as a separate effort

@taurus-forever

author Shayan Patel <[email protected]> 1690560167 -0400 committer Shayan Patel <[email protected]> 1692738982 +0000 gpgsig -----BEGIN SSH SIGNATURE----- U1NIU0lHAAAAAQAAADMAAAALc3NoLWVkMjU1MTkAAAAg0GAga0rhzA34UTlbQlTVuF8/Hn wp8A23MiF5YOCeQ6sAAAADZ2l0AAAAAAAAAAZzaGE1MTIAAABTAAAAC3NzaC1lZDI1NTE5 AAAAQP08Hjeph0Squupw7oKnz9SMgghKunN3kIxZZ6vstzWSUEKXQnLg08as4enV5ZV9Wg GKtFP9BKOKS5Yy4luhXQ4= -----END SSH SIGNATURE----- parent 6e90ca7 author Shayan Patel <[email protected]> 1690560167 -0400 committer Shayan Patel <[email protected]> 1692738754 +0000 gpgsig -----BEGIN SSH SIGNATURE----- U1NIU0lHAAAAAQAAADMAAAALc3NoLWVkMjU1MTkAAAAg0GAga0rhzA34UTlbQlTVuF8/Hn wp8A23MiF5YOCeQ6sAAAADZ2l0AAAAAAAAAAZzaGE1MTIAAABTAAAAC3NzaC1lZDI1NTE5 AAAAQOavuZ8DHFs11FQHwH/xIDTDJgebY25stheI3oJjhRLKSFvcD5fDf2wx+3p5r3zXIP 8+8619ip29zfWkemkZKgs= -----END SSH SIGNATURE----- Miscellaneous improvements to the mysql legacy relation Update cos_agent charm lib to v0.5 and add missing import Fix call of get_secret method on the charm instead of the unit Address PR feedback Pin python dependencies with poetry (#192) Use charmcraft 2.3.0 to fix release build (#256) charmcraft 2.3.0 was pinned in the integration test build but not in the release build Update keystone charm to yoga/stable in series jammy for shared_db integration test (#260) Use block_until instead of wait_for_idle in shared_db integration test Update cos_agent charm lib to v0.5 Add `poetry lock` commands to lint and format (#257) Lint: `poetry lock --check` verifies that poetry.lock is valid for `pyproject.toml` Format: `poetry lock --no-update` adds any changes in `pyproject.toml` to `poetry.lock` without updated locked versions trivial fix (#261) port from k8s and fixes for constrained memory (#253) * port from k8s and fixes for constrained memory fix function call typing fixes updates cos-agent lib Pin python dependencies with poetry (#192) * Use charmcraft 2.3.0 to fix release build (#256) charmcraft 2.3.0 was pinned in the integration test build but not in the release build * bump lib * fix for dpe-2274 * streamlined test to avoiding timeout --------- Co-authored-by: Carl Csaposs <[email protected]> Use snap with ppa sources + other misc snap related improvements (#249) Fix lint warnings and failing unit tests Update cos_agent charmlib to v0.5 Only start mysqld-exporter on cos relation created and stop it on relation broken Expect mysqld-exporter to be disabled by default for replication integration tests Add missing await statements in exporter integration test Use retry to determine if mysqld-exporeter service has started Set monitoring username and password for exporter in integration test Fix how the username and password were being set for the exporter in the integration test Run format Sleep to wait until mysqld-exporter started Run format Address PR feedback Address PR feedback Update data-platform-workflows to v4.1.4 (#268) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Use self-hosted runners for integration tests (#255) WIP: Implement support for juju secrets and create MySQLCharmBase for common charm code in vm and k8s (#243) Update existing secrets instead of creating a new one every time DPE-2154 profile configuration support (#240) * initial profile configuration support * going for juju latest * bump lib * key has default value * doing renovate's work * some test coverage * temporary pin * reflect value for testing profile Refactor get_member_state (#242) * retrieves all entries then filter Some cases (pod rescheduling) the member_id is not populated, failing the state query with the where clause * explaining comments Update charm libs + fix failing unit tests Bump mysql charmlib patch to 37 Changes resulting from testing with juju 3.1.6 Fail installation if snap already installed (#235) Avoid overriding snap if mysql charm & mysql-router charm installed on same machine Context: https://chat.canonical.com/canonical/pl/b8e1daeskjrejxtryjuxuwi9ua Disable codecov GitHub annotations (#245) DPE-1979 Use /etc/hosts as a hostname resolution of nodes in the cluster (#237) * WIP: Use /etc/hosts as a hostname resolution of nodes in the cluster * Add IP address observer + address PR feedback * Fix failing unit tests * Address remaining PR feedback * Fix failing unit test * Send SIGTERM instead of SIGINT to terminate the ip address observer process * Update cos_agent lib to v0.4 * Fix failing unit test * Update S3 charmlib to v0.3 * Update data_interfaces charmlib to v0.14 DPE-1511 Autogenerate database and username in legacy mysql if not specified in config (#222) * Auto generate username and database if not specified in config for the mysql legacy relation * Clean up username and database from databag upon relation broken * Rework the handling of usernames and databases * Handle config changes other than username and database * Change error log into an info log * Update data_interfaces charm lib to v0.13 * Refactor legacy mysql relation with feedback in mind * Account for empty strings as config values * Update data_interfaces charm lib to v0.16 * Update s3 charm lib to v0.4 * Address PR feedback * Address minor nit in feedback * Avoid using .get() where unnecessary * Update juju pin to 2.9.43.0 and pin macaroonbakery to 1.3.1 in tox * Do not run event handler to update /etc/hosts if all passwords are not set Add CODEOWNERS (#247) Required for self-hosted runners Fix incorrect tls integration test + some secret related changes in tls charmlib Move relevant event handler observers into the mysql charm base class Use secrets caching and address PR feedback Update cos_agent charm lib to v0.5 Pin python dependencies with poetry (#192) Use charmcraft 2.3.0 to fix release build (#256) charmcraft 2.3.0 was pinned in the integration test build but not in the release build Update keystone charm to yoga/stable in series jammy for shared_db integration test (#260) Use block_until instead of wait_for_idle in shared_db integration test Update cos_agent charm lib to v0.5 Add `poetry lock` commands to lint and format (#257) Lint: `poetry lock --check` verifies that poetry.lock is valid for `pyproject.toml` Format: `poetry lock --no-update` adds any changes in `pyproject.toml` to `poetry.lock` without updated locked versions Address refresh from juju <= 3.1.4 to juju >= 3.1.5 trivial fix (#261) port from k8s and fixes for constrained memory (#253) * port from k8s and fixes for constrained memory fix function call typing fixes updates cos-agent lib Pin python dependencies with poetry (#192) * Use charmcraft 2.3.0 to fix release build (#256) charmcraft 2.3.0 was pinned in the integration test build but not in the release build * bump lib * fix for dpe-2274 * streamlined test to avoiding timeout --------- Co-authored-by: Carl Csaposs <[email protected]> Use snap with ppa sources + other misc snap related improvements (#249) Fix lint warnings and failing unit tests Update cos_agent charmlib to v0.5 Only start mysqld-exporter on cos relation created and stop it on relation broken Expect mysqld-exporter to be disabled by default for replication integration tests Add missing await statements in exporter integration test Use retry to determine if mysqld-exporeter service has started Set monitoring username and password for exporter in integration test Fix how the username and password were being set for the exporter in the integration test Run format Sleep to wait until mysqld-exporter started Run format Address PR feedback Address PR feedback Address stylistic PR feedback Update mysql charm libpatch to v0.38 Temporarily disable self-hosted runners (#280) In order to request more self-hosted runners, we need to collect data about self-hosted runner performance compared to GitHub-hosted runners. (@taurus-forever) Since there are a limited number of runners, temporarily disable self-hosted runners on other PRs so that we can collect data with controlled PRs Use renovate preset (#274) Delete CODEOWNERS (#277) No longer a requirement for self-hosted runners [charm] Update charm dependencies (#264) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> DPE-2352 Restart mysql exporter upon monitoring password change (#285) * Restart mysql exporter upon monitoring password change * Add restart_mysql_exporter as an abstract method on MySQLBase [charm lib] Update charm lib dependencies to v41.0.3 (#263) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> methods used on in-place upgrades (#288) * methods used on in-place upgrades * bump libpatch * pr comments bump libpatch (#290) Fix committed merge conflict Add `poetry lock` commands to lint and format (#257) Lint: `poetry lock --check` verifies that poetry.lock is valid for `pyproject.toml` Format: `poetry lock --no-update` adds any changes in `pyproject.toml` to `poetry.lock` without updated locked versions trivial fix (#261) Temporarily disable self-hosted runners (#280) In order to request more self-hosted runners, we need to collect data about self-hosted runner performance compared to GitHub-hosted runners. (@taurus-forever) Since there are a limited number of runners, temporarily disable self-hosted runners on other PRs so that we can collect data with controlled PRs Use renovate preset (#274) Delete CODEOWNERS (#277) No longer a requirement for self-hosted runners [charm] Update charm dependencies (#264) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> [charm lib] Update charm lib dependencies to v41.0.3 (#263) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> methods used on in-place upgrades (#288) * methods used on in-place upgrades * bump libpatch * pr comments bump libpatch (#290) Add `has_cos_relation` to the MySQLCharmBase class (#289) * Restart mysql exporter upon monitoring password change * Add restart_mysql_exporter as an abstract method on MySQLBase * Update libpatch and port has_cos_relations property into MySQLCharmBase * Update mysql patch lib to v0.40 * Add missing import [MISC] Switch maintainers to the DPE mailing list (#271) * Switch maintainers to DPE mailing list * Maintainers strip info line when present (#294) * strip info line when present * fix comment Update data interfaces charm lib to v0.17

shayancanonical requested review from paulomach, taurus-forever and carlcsaposs-canonical July 31, 2023 12:09

carlcsaposs-canonical previously approved these changes Jul 31, 2023

View reviewed changes

src/relations/mysql.py Outdated Show resolved Hide resolved

src/relations/mysql.py Show resolved Hide resolved

src/relations/mysql.py Outdated Show resolved Hide resolved

shayancanonical dismissed carlcsaposs-canonical’s stale review via cdefc8e July 31, 2023 12:28

shayancanonical marked this pull request as ready for review July 31, 2023 13:22

shayancanonical requested a review from a team as a code owner July 31, 2023 13:22

carlcsaposs-canonical previously approved these changes Jul 31, 2023

View reviewed changes

taurus-forever previously approved these changes Aug 7, 2023

View reviewed changes

paulomach previously approved these changes Aug 8, 2023

View reviewed changes

shayancanonical dismissed stale reviews from paulomach, taurus-forever, and carlcsaposs-canonical via 633bc9f August 14, 2023 12:31

paulomach previously approved these changes Aug 14, 2023

View reviewed changes

taurus-forever previously approved these changes Aug 17, 2023

View reviewed changes

shayancanonical force-pushed the fix/mysql_legacy_relation branch from f7eed34 to 704e6fa Compare August 17, 2023 11:57

shayancanonical dismissed stale reviews from taurus-forever and paulomach via 25dce15 August 17, 2023 12:31

paulomach previously approved these changes Aug 18, 2023

View reviewed changes

carlcsaposs-canonical previously approved these changes Aug 21, 2023

View reviewed changes

shayancanonical added 2 commits August 22, 2023 21:17

Merge branch 'main' into fix/mysql_legacy_relation

2b1d79d

shayancanonical force-pushed the fix/mysql_legacy_relation branch from 25dce15 to 2b1d79d Compare August 22, 2023 21:22

Update upgrade charm lib to v0.10

087f036

shayancanonical dismissed stale reviews from carlcsaposs-canonical and paulomach via 087f036 August 23, 2023 11:41

paulomach approved these changes Aug 23, 2023

View reviewed changes

carlcsaposs-canonical approved these changes Aug 23, 2023

View reviewed changes

shayancanonical merged commit 7d94c8d into main Aug 23, 2023

shayancanonical deleted the fix/mysql_legacy_relation branch August 23, 2023 13:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Miscellaneous improvements to the mysql legacy relation #254

Miscellaneous improvements to the mysql legacy relation #254

shayancanonical commented Jul 31, 2023

codecov bot commented Jul 31, 2023 •

edited

Loading

taurus-forever left a comment

paulomach left a comment

taurus-forever left a comment

carlcsaposs-canonical Aug 21, 2023

shayancanonical Aug 22, 2023

Miscellaneous improvements to the mysql legacy relation #254

Miscellaneous improvements to the mysql legacy relation #254

Conversation

shayancanonical commented Jul 31, 2023

Issue

Solution

codecov bot commented Jul 31, 2023 • edited Loading

Codecov Report

taurus-forever left a comment

Choose a reason for hiding this comment

paulomach left a comment

Choose a reason for hiding this comment

taurus-forever left a comment

Choose a reason for hiding this comment

carlcsaposs-canonical Aug 21, 2023

Choose a reason for hiding this comment

shayancanonical Aug 22, 2023

Choose a reason for hiding this comment

codecov bot commented Jul 31, 2023 •

edited

Loading