Add new force flag to DemotePrimary to force a demotion even when blocked on waiting for semi-sync acks
#18714
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -819,3 +819,31 @@ func (mysqld *Mysqld) SemiSyncExtensionLoaded(ctx context.Context) (mysql.SemiSy | |
|
|
||
| return conn.Conn.SemiSyncExtensionLoaded() | ||
| } | ||
|
|
||
| func (mysqld *Mysqld) IsSemiSyncBlocked(ctx context.Context) (bool, error) { | ||
| conn, err := getPoolReconnect(ctx, mysqld.dbaPool) | ||
|
Member
There was a problem hiding this comment. I would not expect that we need to use the DBA pool for this as we're just selecting from P_S. The DBA pools tend to be relatively small and should only be used when truly necessary or they can block other things that are critical and do require it.
Contributor
There was a problem hiding this comment. I wondered this too, but I don't know what alternatives we have I have seen the small DBA pool get exhausted before, which prevents reparent RPCs that also use that pool. The root cause was likely overloaded mysqld or semi-sync issues, but filling that pool is a risk
Member
Author
There was a problem hiding this comment. I think I used this pool because that's what all the other operations (like vitess/go/vt/mysqlctl/replication.go Lines 496 to 506 in 429964e Maybe it would be overall better to fetch a connection from the pool and then use that connection for all the operations that happen during commands like
Contributor
There was a problem hiding this comment.
Yeah, I don't think we should include that kind of change in this PR, but it's a risk For some context: Slack has seen overloaded tablets cause this pool to fill with I think we'll just need to keep this in mind, and only 1 x RPC is being added to the pile here 👍/👎
Member
There was a problem hiding this comment. That's fair. I don't think we should use DBA everywhere here, but it does line up with the other uses. And using a single connection for all of the work might be a good idea here too. Not related though so we can investigate this more separately. |
||
| if err != nil { | ||
| return false, err | ||
| } | ||
| defer conn.Recycle() | ||
|
|
||
| // Execute the query to check if the primary is blocked on semi-sync. | ||
| semiSyncWaitSessionsRead := "select variable_value from performance_schema.global_status where regexp_like(variable_name, 'Rpl_semi_sync_(source|master)_wait_sessions')" | ||
| res, err := conn.Conn.ExecuteFetch(semiSyncWaitSessionsRead, 1, false) | ||
| if err != nil { | ||
| return false, err | ||
| } | ||
| // If we have no rows, then the primary doesn't have semi-sync enabled. | ||
| // It then follows, that the primary isn't blocked :) | ||
| if len(res.Rows) == 0 { | ||
| return false, nil | ||
| } | ||
|
|
||
| // Read the status value and check if it is non-zero. | ||
| if len(res.Rows) != 1 || len(res.Rows[0]) != 1 { | ||
| return false, fmt.Errorf("unexpected number of rows received - %v", res.Rows) | ||
| } | ||
| value, err := res.Rows[0][0].ToCastInt64() | ||
| return value != 0, err | ||
|
|
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -255,7 +255,7 @@ func stopReplicationAndBuildStatusMaps( | |
| if isSQLErr && sqlErr != nil && sqlErr.Number() == sqlerror.ERNotReplica { | ||
| var primaryStatus *replicationdatapb.PrimaryStatus | ||
|
|
||
| primaryStatus, err = tmc.DemotePrimary(groupCtx, tabletInfo.Tablet, true /* force */) | ||
|
Member
There was a problem hiding this comment. Cool, this function is only called from ERS today so makes sense. 👍
Contributor
There was a problem hiding this comment. @arthurschreiber I haven't thought of a specific reason or use case, etc but I wanted to point out that VTOrc could have a separate problem for a "stalled demoting primary", perhaps In the same category of "do we ever need
Contributor
There was a problem hiding this comment.
Appending here: to achieve this the
Member
Author
There was a problem hiding this comment. @timvaillancourt I don't necessarily see the value in that. In my eyes, due to the way
I'm still not sure whether enabling this always would be safe. Because writes that happen out of band of Vitess could be lost because we disable semi-sync before marking the server read-only. I guess the same can happen with the force flag though. 🤷
Contributor
There was a problem hiding this comment.
This kind of makes sense, but in my experience this doesn't happen "always" - or at least I don't think we had 100% of ERS result in a stuck-demoted I wonder if @rvrangel / @tanjinx could validate this, or provide some data how often an ERS results in a tablet forever demoting
Member
There was a problem hiding this comment. Today,
It's not required, but might potentially prevent future unintended behavior.
Member
Author
There was a problem hiding this comment. Alternatively, we could move this function from
Member
Author
There was a problem hiding this comment. I'll do that in a follow up PR, just to make sure the diffs are easier to read (and so this doesn't require another review round on this PR). |
||
| if err != nil { | ||
| msg := "replica %v thinks it's primary but we failed to demote it: %v" | ||
| err = vterrors.Wrapf(err, msg, alias, err) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -548,20 +548,20 @@ func (tm *TabletManager) InitReplica(ctx context.Context, parent *topodatapb.Tab | |
| // or on a tablet that already transitioned to REPLICA. | ||
| // | ||
| // If a step fails in the middle, it will try to undo any changes it made. | ||
| func (tm *TabletManager) DemotePrimary(ctx context.Context, force bool) (*replicationdatapb.PrimaryStatus, error) { | ||
| log.Infof("DemotePrimary") | ||
| if err := tm.waitForGrantsToHaveApplied(ctx); err != nil { | ||
| return nil, err | ||
| } | ||
| // The public version always reverts on partial failure. | ||
| return tm.demotePrimary(ctx, true /* revertPartialFailure */, force) | ||
| } | ||
|
|
||
| // demotePrimary implements DemotePrimary with an additional, private option. | ||
| // | ||
| // If revertPartialFailure is true, and a step fails in the middle, it will try | ||
| // to undo any changes it made. | ||
| func (tm *TabletManager) demotePrimary(ctx context.Context, revertPartialFailure bool, force bool) (primaryStatus *replicationdatapb.PrimaryStatus, finalErr error) { | ||
| if err := tm.lock(ctx); err != nil { | ||
| return nil, err | ||
| } | ||
|
|
@@ -623,13 +623,37 @@ func (tm *TabletManager) demotePrimary(ctx context.Context, revertPartialFailure | |
| }() | ||
| } | ||
|
|
||
| isSemiSyncBlocked, err := tm.MysqlDaemon.IsSemiSyncBlocked(ctx) | ||
|
mattlord marked this conversation as resolved.
|
||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| if force && isSemiSyncBlocked { | ||
| if tm.isPrimarySideSemiSyncEnabled(ctx) { | ||
|
timvaillancourt marked this conversation as resolved.
|
||
| // Disable the primary side semi-sync to unblock the writes. | ||
| if err := tm.fixSemiSync(ctx, topodatapb.TabletType_REPLICA, SemiSyncActionSet); err != nil { | ||
| return nil, err | ||
| } | ||
| defer func() { | ||
| if finalErr != nil && revertPartialFailure && wasPrimary { | ||
| // enable primary-side semi-sync again | ||
| if err := tm.fixSemiSync(ctx, topodatapb.TabletType_PRIMARY, SemiSyncActionSet); err != nil { | ||
| log.Warningf("fixSemiSync(PRIMARY) failed during revert: %v", err) | ||
| } | ||
| } | ||
| }() | ||
| } | ||
| } else { | ||
| // TODO: Shouldn't we better just fail here? | ||
|
Contributor
There was a problem hiding this comment. @mattlord I think Arthur's intention with this comment is: in the I've lost the data to prove this, but I'm not sure if the "never successful" part of this is really true, because in my experience on a 100% VTOrc + semi-sync production (all unplanned reparents are ERS), we only saw the "forever demoting
Contributor
There was a problem hiding this comment. I thought about this a bit more, and I think the discrepancy in my mind is: an ERS is used for a plethora of problems in VTOrc (and when exec'd manually), some totally unrelated to the health of the replication chain, for example To further complicate things, some problems are just intermittent, meaning a loss of ackers is temporary and they DO come back to a healthy state a few seconds later, due to overload, network partitions/issues, etc. I'm pretty sure this is something we saw in Slack's production, for example So my fear is: if we always disable semi-sync when we are waiting on acks during
Member
Author
There was a problem hiding this comment. The first step of ERS is to stop all replicas from replicating from the primary, so if that's successful no replica should come back. I can imagine that we might not end up stopping replication on all replicas (for whatever reason that is), but the DemotePrimary call will happen in parallel and at least attempt to demote the primary. That might fail as well, but in that case ERS will just stop and not proceed and the cluster will be in an indeterminate state.
Contributor
There was a problem hiding this comment.
@arthurschreiber that's a good point, hmm I guess I should just take a step back then, and just say: we didn't encounter the stalled-demoting If I forget this experience in the wild (maybe I should 😬?) the code changes here all look fine to me, but I'm worried we're missing something, and I feel ERS/reparents aren't tested extensively enough (in general - not this PR specifically) for us to be sure. Perhaps I'll dig into the changes between v19 and now to try to explain why this was the "exception" and not the "norm" at Slack 😕 |
||
|
|
||
| // Now we know no writes are in-flight and no new writes can occur. | ||
| // We just need to wait for no write being blocked on semi-sync ACKs. | ||
| err = tm.SemiSyncMonitor.WaitUntilSemiSyncUnblocked(ctx) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| } | ||
|
|
||
| // We can now set MySQL to super_read_only mode. If we are already super_read_only because of a | ||
| // previous demotion, or because we are not primary anyway, this should be | ||
| // idempotent. | ||
|
|
@@ -651,8 +675,7 @@ func (tm *TabletManager) demotePrimary(ctx context.Context, revertPartialFailure | |
| } | ||
| }() | ||
|
|
||
| // If we haven't disabled the primary side semi-sync so far, do it now. | ||
| if tm.isPrimarySideSemiSyncEnabled(ctx) { | ||
| // If using semi-sync, we need to disable primary-side. | ||
| if err := tm.fixSemiSync(ctx, topodatapb.TabletType_REPLICA, SemiSyncActionSet); err != nil { | ||
|
|
||
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 don't like that we have another method to check this when the monitor exposes methods for this as well.