canonical / charmed-mysql-snap

SNAP for Charmed MySQL charm.
https://snapcraft.io/charmed-mysql
Apache License 2.0
0 stars 4 forks source link

[DPE-4173] Add ability to customize router exporter listen port #50

Closed shayancanonical closed 2 months ago

shayancanonical commented 3 months ago

Issue

The default router exporter listen port in mysqlrotuer-exporter versions < 6.0.0 is 49152 (which is an ephemeral port that is occasionally used by MySQLRouter to connect to MySQL) leading to port conflicts with MySQLRouter.

Solution

Add ability to specify a custom listen port in the snap/rock

carlcsaposs-canonical commented 3 months ago

couple questions:

49152 (which is an ephemeral port that is occasionally used by MySQLRouter to connect to MySQL)

is this documented anywhere?

should we submit PR to upstream to use different default port?

shayancanonical commented 3 months ago

Not documented anywhere - router is notorious for not having comprehensive detailed documentation. However, I did see mysqlrouter <-> mysql connections whose ports were around 49152. After customizing listen-port to 9152 (https://github.com/canonical/mysql-router-operator/actions/runs/9809259651/job/27093449442?pr=154), I was able to re-run exporter tests 10 times without an error we saw frequently

Furthermore, upstream released v6.0.0 where the default listen port is changed. We can request a rebuild of our PPA and use this version in the near future

carlcsaposs-canonical commented 3 months ago

Furthermore, upstream released v6.0.0 where the default listen port is changed. We can request a rebuild of our PPA and use this version in the near future

any reason to not do this now instead of adding ability to customize port?

shayancanonical commented 3 months ago

I think being able to customize port is still a good addition regardless. Furthermore, I would like to allocate time for proper upgrade to v6.0.0 of router exporter (which may contain other changes). I also would like to take incremental steps to resolve the issue - the first being customizing listen port and the second being requesting a rebuilding of the PPA and upgrading to this PPA version

carlcsaposs-canonical commented 3 months ago

I think being able to customize port is still a good addition regardless

from a developer perspective, IMO this adds complexity which will have a cost to maintain & support (for example of cost to maintain: https://github.com/canonical/charmed-mysql-rock/pull/42#discussion_r1658288677 https://chat.canonical.com/canonical/pl/n3yqm37hefntfdimduh7hihy5w not exactly the same—but I still think adding things we don't need to our snap has a cost) IMO, we shouldn't accept that cost without a benefit

from a product perspective, if we intend to ship this snap as a product separate from the charm, I think this config value might need PM approval. At least for charms, it seems like our philosophy is to expose less config values & reserve config options for things that are impactful for the user (to reduce UX complexity, reduce # of configurations we need to test & support, etc.)

paulomach commented 3 months ago

I think being able to customize port is still a good addition regardless

from a developer perspective, IMO this adds complexity which will have a cost to maintain & support (for example of cost to maintain: canonical/charmed-mysql-rock#42 (comment) https://chat.canonical.com/canonical/pl/n3yqm37hefntfdimduh7hihy5w not exactly the same—but I still think adding things we don't need to our snap has a cost) IMO, we shouldn't accept that cost without a benefit

from a product perspective, if we intend to ship this snap as a product separate from the charm, I think this config value might need PM approval. At least for charms, it seems like our philosophy is to expose less config values & reserve config options for things that are impactful for the user (to reduce UX complexity, reduce # of configurations we need to test & support, etc.)

@carlcsaposs-canonical , in principle you are right. But the change is minimal, meaning that the cost of maintenance is a fair compromise given we can collect benefits on the tests stabilization. Furthermore, there's nothing in the way of dropping the support once we upgrade to v6 and/or produce a standalone snap in the future.

carlcsaposs-canonical commented 3 months ago

But the change is minimal, meaning that the cost of maintenance is a fair compromise given we can collect benefits on the tests stabilization. Furthermore, there's nothing in the way of dropping the support once we upgrade to v6 and/or produce a standalone snap in the future.

I get that it's fairly simple, but I think there's hidden complexity

for example, if we drop this option from the snap, the complexity around charm upgrade/rollback/downgrade when upgrading from snap with config option to snap without config option

how expensive is upgrading to exporter v6 in the snap? my intuition is that the upgrade should be relatively cheap—and this temporary complexity doesn't seem worth it, but I might be missing something

shayancanonical commented 3 months ago

I am not sure that we would have a valid reason to drop the --listen-port customization on our side. What would we do if we find out that there is another issue with the new default port 9152? Or if we get a requirement that we should use a port other than the default port in upstream? It is already a configurable option in mysql-router-exporter, so I think that it being configurable in the snap is not too much of a leap

With Alex out this week, we can reach out to the server team to rebuild the exporter PPA if the consensus is adamantly against adding a customization of the listen-port. However, I stand firmly by the opinion that adding customization of listen-port is logical

carlcsaposs-canonical commented 3 months ago

What would we do if we find out that there is another issue with the new default port 9152? Or if we get a requirement that we should use a port other than the default port in upstream?

IMO, solving for issues that don't exist yet (and that we don't have indication will exist) is dangerous—it has a real future cost

with that said, if there's a real user need to configure the exporter port, then I think it's worth adding