-
Notifications
You must be signed in to change notification settings - Fork 10
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
DPE-5178 Adopt admin-address throught out #516
Conversation
) | ||
if conn.is_connected(): | ||
connections.append(conn) | ||
sleep(0.5) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sleep added to slow down tests so I could validate connections in database. Left here for the same reason, since the test is fairly quick
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It smells to me, it is a workaround for https://warthogs.atlassian.net/browse/DPE-5340 (we should sent peer details when we ready to accept traffic only).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a workaround for that issue. Just to be able to observe the test while developing.
The error on the nightly tests seems a different issue to me, since the router is connecting to a standby cluster (and we have yet to change/discuss if the router should connect to database through router to pick leadership changes on async cases)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, left a comment about asserting that the admin connection is possible even when max connections are saturated
create_db_connections(1, **credentials) | ||
|
||
logger.info("Get cluster status while connections are saturated") | ||
_ = await run_action(mysql_unit, "get-cluster-status") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we also add a check that the admin port is unaffected? or at least admin connections are active?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Getting the cluster status rely on the admin connection. So when client connections are saturated, the action passing does just that. If the action fails, run_action
will raise an assertion error
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! LGTM.
Q: do we still have usecases for clusteradmin OR we need to clean it one day in the future from Juju Secrets and Documentation?
) | ||
if conn.is_connected(): | ||
connections.append(conn) | ||
sleep(0.5) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It smells to me, it is a workaround for https://warthogs.atlassian.net/browse/DPE-5340 (we should sent peer details when we ready to accept traffic only).
|
* on ip change, restart mysqld * ip change custom charm event needs to be defined on charm object
Issue
As part of cycle, we ought to utilize admin-address for charm usage.
The idea is to ensure that the charm can connect to mysqld
instances (local or remote unit) even when
max_connections
is saturatedby related application(s), since connections to admin-address are do not count against
max_connections
.Solution
admin_address
in configuration file.admin_address:admin_port
for users withADMIN_CONNECTION
privilege.s/clusteradmin/serverconfig
due cluster admin not having (and not needing to have, since replication channels don't count againstmax_connection
)ADMIN_CONNECTION
privilege.