Skip to content
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-3879 update endpoint on upgrade #426

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

paulomach
Copy link
Contributor

@paulomach paulomach commented Mar 26, 2024

Issue

Slow, no preemptive primary switchover on upgrade.

Solution

Add preemptive switchover where possible.

Copy link

codecov bot commented Mar 26, 2024

Codecov Report

Attention: Patch coverage is 28.57143% with 30 lines in your changes are missing coverage. Please review.

Project coverage is 65.98%. Comparing base (e7c1809) to head (144ce84).
Report is 4 commits behind head on main.

❗ Current head 144ce84 differs from pull request most recent head b60ee97. Consider uploading reports for the commit b60ee97 to get more accurate results

Files Patch % Lines
src/relations/mysql_provider.py 30.43% 15 Missing and 1 partial ⚠️
src/upgrade.py 9.09% 9 Missing and 1 partial ⚠️
src/mysql_vm_helpers.py 50.00% 3 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #426      +/-   ##
==========================================
- Coverage   66.25%   65.98%   -0.27%     
==========================================
  Files          17       17              
  Lines        3180     3199      +19     
  Branches      424      428       +4     
==========================================
+ Hits         2107     2111       +4     
- Misses        935      951      +16     
+ Partials      138      137       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@paulomach paulomach marked this pull request as ready for review April 15, 2024 18:25
Copy link
Contributor

@taurus-forever taurus-forever left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

Comment on lines 332 to 341
# rw endpoints
endpoints = (
self.database.fetch_my_relation_field(relation.id, "endpoints", DB_RELATION_NAME)
or ""
)
if unit_address in endpoints:
self.database.set_endpoints(
relation.id,
",".join([e for e in endpoints.split(",") if unit_address not in e]),
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can this set read-write endpoints to ""?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, and it's intentional - a client app should be able to handle (and wait for the endpoint to be set)

Comment on lines +177 to +179
# we assume the leader is primary, since the switchover is done on pre-upgrade-check
self._primary_switchover()

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: if leader is not primary (e.g. switchover after pre-upgrade-check), will things break?

@@ -309,3 +311,45 @@ def _on_database_provides_relation_departed(self, event: RelationDepartedEvent)
logger.info(f"Removed router from metadata {user.router_id}")
except MySQLRemoveRouterFromMetadataError:
logger.error(f"Failed to remove router from metadata with ID {user.router_id}")

def remove_unit_from_endpoints(self, unit: Unit) -> None:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

where is this method being used?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

b60ee97

nowhere, removed. it would be useful if upgrade happened in distinct hook calls, which is not the case

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants