Migrate to using the charmed-mysql snap #39
Conversation
Codecov Report
@@ Coverage Diff @@
## main #39 +/- ##
==========================================
+ Coverage 57.66% 57.97% +0.30%
==========================================
Files 7 7
Lines 437 395 -42
Branches 138 113 -25
==========================================
- Hits 252 229 -23
+ Misses 185 166 -19
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
| MYSQL_ROUTER_USER = "mysql" | ||
| MYSQL_HOME_DIRECTORY = "/var/lib/mysql" | ||
| CHARMED_MYSQL_SNAP = "charmed-mysql" | ||
| CHARMED_MYSQL_SNAP_REVISION = 48 |
There was a problem hiding this comment.
TODO: change revision to latest 8.0/edge revision once charmed mysql snap PR 21 is merged
shayancanonical
requested review from
paulomach,
taurus-forever and
carlcsaposs-canonical
| "--directory", | ||
| f"{MYSQL_HOME_DIRECTORY}/{name}", |
There was a problem hiding this comment.
It looks like --directory enables multiple mysqlrouters to run on the same system. https://dev.mysql.com/doc/mysql-router/8.0/en/mysqlrouter.html#option_mysqlrouter_directory
Do we need this? https://chat.canonical.com/canonical/pl/do19o5znoprume7kb8474miuhw
There was a problem hiding this comment.
Since there is a 1:1 relation between mysqlrouter and the database (at least in the VM charm), I envisioned multiple mysqlrouter charms deployed to the same machine achieve parallelism using parallel installs with the charmed-mysql snap itself.
However, may be we need --directory to support relations with multiple applications in the k8s charm.
Thoughts?
There was a problem hiding this comment.
Got it, makes sense!
For the k8s charm I don't think we need multiple instances of mysqlrouter running—if we need to support relations with multiple applications, I think we would still have one instance of mysqlrouter. Does that sound right to you?
There was a problem hiding this comment.
Yeah, I think that that makes sense. I wonder if the --name flag becomes obsolete in that case in this PR?
There was a problem hiding this comment.
I see the use-cases to install 2+ mysql-router as subordinate charms in the same app.
e.g. you have two independent clusters and app need access there.
@shayancanonical I think we discussed this yesterday, did we misunderstand each other?
There was a problem hiding this comment.
based on our discussion during the previous database office hours: we will only ever need the mysqlrouter process to connect to one database cluster at a time. however, the application can relate to mysqlrouter multiple times to request different databases in the same database cluster. since the current operator does not correctly support multiple relations with the application, i suggest we leave this PR as is. however, @carlcsaposs-canonical's refactor of the charm will address this concern
to install 2 different mysqlrouter applications as subordinate charms on the same machine, we need to utilize the experimental feature parallel installs for snaps. however, the details there are still murky and hard to project (the changes to the snap charm library in operator-libs-linux are not immediately clear)
| "--directory", | ||
| f"{MYSQL_HOME_DIRECTORY}/{name}", |
There was a problem hiding this comment.
I see the use-cases to install 2+ mysql-router as subordinate charms in the same app.
e.g. you have two independent clusters and app need access there.
@shayancanonical I think we discussed this yesterday, did we misunderstand each other?
| replace_socket_location_command = [ | ||
| "sudo", | ||
| "sed", | ||
| "-Ei", | ||
| f"s:/tmp/(.+).sock:{CHARMED_MYSQL_COMMON_DIRECTORY}/var/run/mysqlrouter/\\1.sock:g", | ||
| f"{CHARMED_MYSQL_DATA_DIRECTORY}/etc/mysqlrouter/mysqlrouter.conf", | ||
| ] | ||
| subprocess.run(replace_socket_location_command, check=True) |
There was a problem hiding this comment.
@delgod mysqlrouter --bootstrap doesn't allow to specify socket location.
Should we consider to send improvement PR to the vendor or stay with sed forever?
| "sed", | ||
| "-Ei", | ||
| "s:/tmp/(.+).sock:/var/snap/charmed-mysql/common/var/run/mysqlrouter/\\1.sock:g", | ||
| "/var/snap/charmed-mysql/current/etc/mysqlrouter/mysqlrouter.conf", | ||
| ] |
There was a problem hiding this comment.
General comment: having copy&paste duplicates in test and in charm.py for bootstrap_cmd and replace_socket_location_cmd leads to the human mistakes in the future. Can/should we extract it to the common resource?
There was a problem hiding this comment.
I think that would kinda defeat the point of having a test. Maybe a unit test for this isn't helpful?
There was a problem hiding this comment.
I agree, extracting it would defeat the point of having a test. I modified the existing tests so they pass, but the tests will need to be adjusted when carl's refactor is applied to this operator
|
Superseded by #44 |
dpe-1628
Issue
We are currently using the mysqlrouter installed from an apt package.
Solution
We would like to move to using the charmed-mysql snap