Always return VIP as host if set #179
Conversation
shayancanonical
requested review from
paulomach,
taurus-forever and
carlcsaposs-canonical
There was a problem hiding this comment.
question: could you provide more information about what the deployment looks like (in terms of juju apps, relations, and config) if vip is externally configured?
also, is it possible to configure vip outside of hacluster and with hacluster simultaneously? do we need to guard UX if someone attempts this?
src/machine_charm.py
Outdated
| if ( | ||
| self._ha_cluster.relation | ||
| and self._ha_cluster.is_clustered() | ||
| and self.config.get("vip") | ||
| ): | ||
| if self.is_externally_accessible(event=None) and self.config.get("vip"): | ||
| return self.config["vip"] |
There was a problem hiding this comment.
if ha_cluster.is_clustered() is False, will router be providing a non-accessible endpoint in the databag? is that desired?
There was a problem hiding this comment.
+1 on that. Can this just be transient condition though?
There was a problem hiding this comment.
this change was made to support the use case where the VIP is somehow configured outside of hacluster. however, it is possible to confirm that hacluster is clustered if a relation with hacluster exists
There was a problem hiding this comment.
Checking _ha_cluster.is_clustered() only if relation with _ha_cluster exists in 03c5777
|
@carlcsaposs-canonical raises some very valid points. however, the stance in pgbouncer is that we do not know exactly how field will be using the virtual IP feature and to deliberately avoid being too restrictive. if the user decides to use the @dragomirp please correct me if my understanding deviates from what we disucssed |
shayancanonical
requested review from
paulomach,
dragomirp and
carlcsaposs-canonical
…oviding VIP as endpoint
As far as I am aware, the hacluster charm is the only Juju way to handle virtual IPs, but I don't think we can assume it will be the only vIP setup that will be used. Any manual vIP setup will be invisible to the charm, hooks wise, and IMHO difficult to detect, by inspecting the network. I'd rather leave the vIP setup open ended, at least initially, rather than breaking potential use cases that we may have to fix urgently. Setting an unreachable vIP should be detected during initial deployment and in the worst case would just require redeploying the router. |
src/relations/hacluster.py
Outdated
| if ( | ||
| isinstance(self.charm.get_workload(event=None), workload.AuthenticatedWorkload) | ||
| and self.charm.unit.is_leader() | ||
| and vip | ||
| ): | ||
| if self.charm.unit.is_leader() and vip: | ||
| return ops.ActiveStatus(f"VIP: {vip}") |
There was a problem hiding this comment.
if workload is not running we shouldn't set active status
also follow-up to #177 (comment)
also this could also be an issue for refresh, where active status with vip will block refresh status (https://github.com/canonical/charm-refresh/blob/main/docs/requirements_and_user_experience.md#lower-priority-statuses)
There was a problem hiding this comment.
removed ActiveStatus setting on leader unit in 49ce228
| self._ha_cluster.relation | ||
| and self._ha_cluster.is_clustered() | ||
| and self.config.get("vip") | ||
| not self.is_externally_accessible(event=None) | ||
| or not self.config.get("vip") | ||
| or (self._ha_cluster and not self._ha_cluster.is_clustered()) |
There was a problem hiding this comment.
do we need to check for ha_cluster.relation?
There was a problem hiding this comment.
added a check in is_clustered() to return false if no relation formed yet in 49ce228
|
@shayancanonical please clarify open questions with Carl directly. Feel free to inclide Dragomir in discussion to sync mysql-router with pgbouncer UX. |
Issue
We would like to always return the configured VIP as the host when externally connected (to support use cases where the VIP may be configured outside of the charms e.g. openstack octavia)
Solution
Update the conditions that
Reference
PGBouncer charm has the above described functionality: https://github.com/canonical/pgbouncer-operator/pull/337/files#diff-b9ed39bbc9c0387bd3e07da31d13373745534a1cd723d3e292c73496b12e307cR551