[SERVER-54460] Resharding may delete the state document before fully completing Created: 11/Feb/21 Updated: 29/Oct/23 Resolved: 04/May/21 |
|
| Status: | Closed |
| Project: | Core Server |
| Component/s: | Sharding |
| Affects Version/s: | None |
| Fix Version/s: | 4.9.0-rc1, 5.0.0-rc0 |
| Type: | Bug | Priority: | Major - P3 |
| Reporter: | Andrew Shuvalov (Inactive) | Assignee: | Cheahuychou Mao |
| Resolution: | Fixed | Votes: | 0 |
| Labels: | PM-234-M3, PM-234-T-lifecycle | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original Estimate: | Not Specified | ||
| Issue Links: |
|
||||||||||||||||||||||||
| Backwards Compatibility: | Fully Compatible | ||||||||||||||||||||||||
| Operating System: | ALL | ||||||||||||||||||||||||
| Backport Requested: |
v4.9
|
||||||||||||||||||||||||
| Steps To Reproduce: | 1. change the src/mongo/db/repl/primary_only_service_op_observer.cpp PrimaryOnlyServiceOpObserver::onDelete() to release the service with error: service->releaseInstance( 2. run the test: buildscripts/resmoke.py run --suite=sharding --repeat 1 --mongodSetParameters=" { featureFlagTenantMigrations: true}" jstests/sharding/api_params_nontransaction_sharded.js it will fail with: is dropped", |
||||||||||||||||||||||||
| Sprint: | Sharding 2021-05-03, Sharding 2021-05-17 | ||||||||||||||||||||||||
| Participants: | |||||||||||||||||||||||||
| Story Points: | 2 | ||||||||||||||||||||||||
| Description |
|
I do not claim that this issue can cause actual production failures, but it was a real problem for me blocking from fully implementing The idea I was trying to implement in However if I make this bridge as discussed in that bug, the resharding fails at the moment the state doc is deleted, before completion. I don't see a simple fix myself. |
| Comments |
| Comment by Githook User [ 04/May/21 ] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Author: {'name': 'Cheahuychou Mao', 'email': 'mao.cheahuychou@gmail.com', 'username': 'cheahuychou'}Message: (cherry picked from commit c02a82e18fe3fc3cf9ed76962fe05c22bf376332) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Githook User [ 04/May/21 ] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Author: {'name': 'Cheahuychou Mao', 'email': 'mao.cheahuychou@gmail.com', 'username': 'cheahuychou'}Message: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Max Hirschhorn [ 20/Feb/21 ] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
I chatted with Andrew and Lingzhi over Zoom about this ticket on Friday. There is a bug in ReshardingCoordinator where it is possible for _completionPromise to never be set. This is because the coordinator state document is removed in an earlier lambda function than one which fulfills the _completionPromise. It is therefore possible for the ScopedTaskExecutor to be shut down and for the onCompletion() callback to never be called.
The following patch is an implementation of my idea to use an onCommit() handler. The *reshard*.js tests pass locally with the exception of a known failure in resharding_clones_duplicate_key.js. For whoever picks this ticket up, I think we should additionally make PrimaryOnlyService::releaseInstance() and PrimaryOnlyService::Instance::interrupt() noexcept to reflect intent. Functions in onCommit() handlers are not allowed to throw anyway.
| ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Max Hirschhorn [ 13/Feb/21 ] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Great points. The _id of the state documents in resharding is already a UUIDv4 value. A new resharding operation will use a fresh UUIDv4 value even when run for the same namespace again. I think what makes this ticket difficult to address is that the user-facing reshardCollection command accepts the namespace string rather than the reshardingUUID. This is to be consistent with how the namespace string is the interface for all other MongoDB commands. The config server primary is responsible for generating the reshardingUUID but must also ensure a resharding operation isn't already in-progress for the collection. ( I feel the reason why the donorStartMigration command can safely expose its UUIDv4 value is because on the sender is still going to be [MongoDB the company]'s code which can be vetted to avoid misuse. Happy to chat more over Zoom on Tuesday. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Andrew Shuvalov (Inactive) [ 13/Feb/21 ] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Got you. I would suggest it might be cleaner to match the state doc by uuid and always create a new one on rerun. If this is impossible than this ticket is impossible, too. I just want you to be aware that this race prevented a change I was trying to make. Specifically, I was trying to always interrupt any service instance when state doc is deleted, from the observer. It was fine for the tenant migration service because there the state doc is garbage collected, but it broke the resharding service because there the stare doc deletion is part of the flow. In my experiment, the interrupt made the service to fail. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Max Hirschhorn [ 11/Feb/21 ] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
andrew.shuvalov, let me provide some details on what resharding is hoping to achieve. The goal is to make it possible to immediately rerun the reshardCollection command again (either on the same namespace or a different one) after it returns successfully. We therefore have the ReshardingCoordinator service set up so that the reshardCollection command doesn't return until the coordinator state document has been deleted. This means that the coordinator state document must be deleted before ReshardingCoordinator::_completionPromise is fulfilled. For comparison, tenant migrations is marking the state document as able to be reaped by the TTL index. The actual deletion of the state document doesn't happen until later in wall-clock time. I don't think I have enough context on |