[charmed_mysql] refactor & support k8s env

[Deezzir]

This PR introduces a proper K8S variant for the charmed_mysql plugin. The collection will use a kubectl if it detects a K8S environment and mysql pods or the snap to collect data.

---

Please place an 'X' inside each '[]' to confirm you adhere to our Contributor Guidelines

• Is the commit message split over multiple lines and hard-wrapped at 72 characters?
• Is the subject and message clear and concise?
• Does the subject start with [plugin_name] if submitting a plugin patch or a [section_name] if part of the core sosreport code?
• Does the commit contain a Signed-off-by: First Lastname [email protected]?
• Are any related Issues or existing PRs properly referenced via a Closes (Issue) or Resolved (PR) line?
• Are all passwords or private data gathered by this PR obfuscated?

[packit-as-a-service]

Congratulations! One of the builds has completed. 🍾

You can install the built RPMs by following these steps:

• sudo yum install -y dnf-plugins-core on RHEL 8
• sudo dnf install -y dnf-plugins-core on Fedora
• dnf copr enable packit/sosreport-sos-4134
• And now you can install the packages.

Please note that the RPMs should be used only in a testing environment.

[bmr-cymru]

The shell commands added in this PR will not work: please refactor them to use Python to drive the iteration rather than shell loops.

The fix up commits can be squashed into the parent since they aren't really separate tasks - note that the preferred commit/PR title formatting in sos is to use a "[tag]" at the start of the subject, e.g.:

[charmed_mysql_k8s] add new plugin

Since your first patch is also refactoring charmed_mysql it could also be:

[charmed_mysql_k8s,charmed_mysql] refactor charmed_mysql and add k8s version

Or split the first patch in two - one to refactor charmed_mysql and a second to add the new charmed_mysql_k8s plugin. This may make the changes easier to review and navigate in the git log in future.

[Deezzir]

@bmr-cymru I've updated the PR description. I think it is better to keep the changes for both plugins in this PR, as I removed some dead K8S code from the charmed_mysql plugin. WDYT?

[bmr-cymru]

@Deezzir - yes - keep them in the same PR, but perhaps consider splitting the first patch into two commits (the refactoring of the existing plugin, then the addition of the new plugin). It's just a suggestion, but again, it's about making it easy to review.

I would definitely consider rebasing and squashing/rewriting commits at this point. Like I said, sos uses [tag] description of change for commit messages, not refactor:/chore:/fix:. Especially the fixup patches should be squashed into their parents to avoid reviewers having to dig through separate patches to see the overall change.

[Deezzir]

@bmr-cymru the commits are now squashed into two separate commits - one for charmed_mysql and the second for charmed_mysql_k8s

[bmr-cymru]

@bmr-cymru the commits are now squashed into two separate commits - one for charmed_mysql and the second for charmed_mysql_k8s

Great, thanks! It's way easier to comprehend the changes in this structure.

[Deezzir]

@bmr-cymru I've removed the echo $MYSQL_PWD... and switched to contextually managed tempdir

[arif-ali]

Apologies for the late review here, as I have been head down on other items.

Why not have one charmed_mysql plugin, and have a check in there to distinguish between the k8s version and the non-k8s version. There are a lot of items that are similar, and it can be nice to have them in the same plugin. I am ok, if you feel that would require too much work.

One example where this is done is the kubernetes plugin, where we have checks for Charmed k8s, Canonical k8s, microk8s.

[pmoravec]

The code itself looks good to me up to the 2 points mentioned by Arif:

• charmed_mysql_k8s needs better enablement check(s)
• it really makes sense to merge the two plugins into one. Sorry I havent realized this earlier that so much code is common. Maybe the best approach is to have one file with two classes where CharmedMySQLK8s is a child one of CharmedMySQL with different check_enabled method and setup, but lots of variables (mysql_queries, conf_paths,..) and some code common?

I can live with the current plugins design, the preference among options can be subjective and we also have e.g. aap_containerised plugin (though the difference from aap_* ones is bigger). But I bit defer to Arif's opinion here as I know nothing about juju.

[Deezzir]

@pmoravec @arif-ali I've tried to make the charmed_mysql_k8s inherit from the charmed_mysql plugin, but it does not seem to work. The child plugin is not being initialized/checked for triggers. I can still put them in one file and make some variables like queries and conf_paths shared. WDYT?

I've also addressed the generic trigger, now it should be better

[arif-ali]

The child plugin is not being initialized/checked for triggers. I can still put them in one file and make some variables like queries and conf_paths shared. WDYT?

We should be able to do this in one file, like we have done in the past with other plugins. If it helps, we can work on this offline, and help you to get to the end result.

[Deezzir]

@bmr-cymru @pmoravec

We synced with @arif-ali (kudos) and decided to merge the two plugin files into one, with dispatching depending on the environment and shared logic. PTAL

[pmoravec]

Just a theoretical thought: could there be a deployment with username set but empty password? If so, we must return db_user, None here.

[pmoravec]

Ditto if a user can have empty password. (but again, rather theoretical than practical comment, you can ignore it)

[pmoravec]

Rather a nitpick (or not, @TurboTurtle ?): while self.conf_paths entries start with leading /, isn't it safer to have os.path.join here (or at least {self.snap_path_current}/{self.conf_paths[..]})? Or am I too paranoically overdoing it?

[bmr-cymru]

If the self.conf_paths entries begin with os.path.sep (/) then you need to trim that off before passing to join(), otherwise the joined path is treated as absolute and returned unmodified:

>>> join("/myroot", "/some/other/path")
'/some/other/path'

[arif-ali]

@Deezzir can you also have a look at this too

[Deezzir]

What can we do here? As pointed by @bmr-cymru we cannot use os.path.join without escaping the leading /. I guess we can do a dedicated function for that. Wdyt? Or we leave it as is

[bmr-cymru]

I guess there are three ways to go here:

• Keep it as-is - maybe add a comment above the blocks 214-215 and 220-224 noting that this expects the paths to be in the form /path/with/leading/slash. I don't mind this too much tbh.
• Change the conf_paths table to use relative paths, and switch to join($ROOT, self.conf_paths[...]) everywhere (so line 151 would just be join("/", self.conf_path[...]), this list would be join(self.snap_path_*, self.conf_paths[...])). This is more churn but consistent and obedient to preferred path construction style.
• Add a little _join_conf_path() helper to trim the leading / and glue the paths together in the list literals. This is less preferable imho.

What do others thing?

[Deezzir]

I like the third option, proactively added a _join_conf_path to safely join the paths, PTAL!

[pmoravec]

Generally ACK. I would appreciate the direct return in check_enabled method, other comments are just ideas left for a consideration.

Thanks for the patience with our reviews.

[arif-ali]

just a nitpick based on @pmoravec comment, but otherwise looks ok to me

[bmr-cymru]

Functionally this is fine - as a minor style nitpick _join_conf_paths() could be a local function defined inside CharmedMySQL._process_snap() (above the add_forbidden_paths() call) - it doesn't reference self. and is only called from that one method, but it's not a hard requirement.

[arif-ali]

Thanks for all your work on this, and the patience on all the reviews