-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
VReplication: Validate min set of user permissions in traffic switch prechecks #16762
Conversation
Review ChecklistHello reviewers! 👋 Please follow this checklist when reviewing this Pull Request. General
Tests
Documentation
New flags
If a workflow is added or modified:
Backward compatibility
|
15cf5c1
to
372b807
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #16762 +/- ##
==========================================
- Coverage 69.51% 69.49% -0.02%
==========================================
Files 1569 1569
Lines 202517 202633 +116
==========================================
+ Hits 140780 140827 +47
- Misses 61737 61806 +69 ☔ View full report in Codecov by Sentry. |
372b807
to
ec8366b
Compare
… switch dry run Signed-off-by: Matt Lord <[email protected]>
ec8366b
to
3963c78
Compare
Signed-off-by: Matt Lord <[email protected]>
Signed-off-by: Matt Lord <[email protected]>
Signed-off-by: Matt Lord <[email protected]>
5d6dc26
to
a4fba70
Compare
Signed-off-by: Matt Lord <[email protected]>
a4fba70
to
815493f
Compare
Signed-off-by: Matt Lord <[email protected]>
Signed-off-by: Matt Lord <[email protected]>
Signed-off-by: Matt Lord <[email protected]>
Signed-off-by: Matt Lord <[email protected]>
Signed-off-by: Matt Lord <[email protected]>
Signed-off-by: Matt Lord <[email protected]>
Signed-off-by: Matt Lord <[email protected]>
Signed-off-by: Matt Lord <[email protected]>
Signed-off-by: Matt Lord <[email protected]>
Signed-off-by: Matt Lord <[email protected]>
Signed-off-by: Matt Lord <[email protected]>
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.
We are adding a new vttablet RPC and calling it from vtctld. This means that you cannot run SwitchTraffic on a cluster that is in a partially upgraded state, or at least you need to upgrade vtctld before vttablet. What is our current stance on being able to run SwitchTraffic mid-upgrade?
Good question! I'll have to test that and think on it... |
Signed-off-by: Matt Lord <[email protected]>
|
Signed-off-by: Matt Lord <[email protected]>
Signed-off-by: Matt Lord <[email protected]>
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.
Some minor changes requested, feel free to merge if/when addressed.
) | ||
) | ||
limit 1 | ||
` |
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.
This looks correct. I wonder if we should not instead parse the result of SHOW GRANTS
instead, as those guarantee correct aggregation of the above mysql.*
tables.
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.
I considered that as well, but I thought that it would be much more complicated since I didn't see any existing parsing support for it. And since that output is formulated from querying these tables anyway I went this direction.
if err != nil { // Should never happen | ||
return nil, vterrors.Errorf(vtrpcpb.Code_INTERNAL, "unexpected result for query %s: expected boolean-like value, got: %q", | ||
query, qr.Rows[0][0].ToString()) | ||
} |
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.
Consider using row := qr.Named().Row()
.
Signed-off-by: Matt Lord <[email protected]>
Signed-off-by: Matt Lord <[email protected]>
Signed-off-by: Matt Lord <[email protected]>
Description
This PR adds another check to the VReplication workflow traffic switching pre-checks that we do in order to avoid any potential downtime or client visible errors from attempting the traffic switch — heading off any user/customer impacting issues. This new check ensures that if we want to setup reverse replication — which is the default in order to support switching traffic back and forth with the
SwitchTraffic
andReverseTraffic
sub-commands — we ensure that the_vt.vreplication
table has the MySQL level privileges needed (select,insert,update,delete) in order to manage the reverse workflow.This is particularly helpful when working with "external" or "unmanaged" tablets that you would often use when first migrating to Vitess. In this case you have to manually setup the necessary privileges for the
--db_filtered_user
and other MySQL users that Vitess uses in themysqld
instances that these unmanaged tablets work with. This check ensures that we have the minimum privileges we need on those unmanaged tablets'mysqld
instances for when we execute theSwitchTraffic
sub-command.The test case in the issue then fails as expected when attempting to switch traffic:
Manual test
Results:
Related Issue(s)
Checklist