loqrecovery,server: apply staged loqrecovery plans on start #95582
Conversation
|
This change is |
aliher1911
changed the title
aliher1911
force-pushed
the
loq_05online_apply_plan
branch
6 times, most recently
from
0ef1ed8 to
b4d9c47
Compare
aliher1911
force-pushed
the
loq_05online_apply_plan
branch
3 times, most recently
from
2d4ed66 to
174daf5
Compare
| (gogoproto.nullable) = false, | ||
| (gogoproto.stdtime) = true]; | ||
| // If most recent recovery plan application failed, Error will contain | ||
| // aggregated error messages containing all encountered errors. |
There was a problem hiding this comment.
Will there ever be more than one error here?
There was a problem hiding this comment.
It is possible to have more than one commit error in case we have multiple stores and batch commit fails. At this point we collect all commit errors since we can't rollback anymore.
pkg/server/loss_of_quorum.go
Outdated
| ) { | ||
| var cleanup loqrecoverypb.DeferredRecoveryActions | ||
| err := stores.VisitStores(func(s *kvserver.Store) error { | ||
| c, found, err := loqrecovery.ConsumeCleanupActionsInfo(ctx, s.Engine()) |
There was a problem hiding this comment.
Should we wait to remove the action until we've successfully decommissioned the nodes?
There was a problem hiding this comment.
Currently it is just trying once and relying on the fact that all nodes would try to do this so one will succeed. Not a strong guarantee. Alternatively, we could keep trying for some time, maybe longer than theoretical liveness range recovery time e.g. replication queue should pick up liveness within 10 min, then it would upreplicate (which could be throttled potentially by indeterminate amount), then we'll pick it up within seconds and update.
I don't like idea of keeping this entry if we can't update liveness. It would stay there till next restart so you'll have this action silently "scheduled". It is idempotent now, so technically fine.
There was a problem hiding this comment.
Discussed offline. We'll need to get these nodes decommissioned one way or another (otherwise they could rejoin the cluster via newly added nodes or prevent later cluster upgrades), so we should keep retrying with exponential backoff and make sure the actions are idempotent. We only remove the action when it's successfully applied. In the worst case, the operator can manually perform e.g. the decommission action to complete the job.
Commit adds a helper to join multiple indentifiers into a string which is helpful in cli commands. Release note: None
aliher1911
force-pushed
the
loq_05online_apply_plan
branch
3 times, most recently
from
80a1cfd to
734f7a9
Compare
aliher1911
force-pushed
the
loq_05online_apply_plan
branch
3 times, most recently
from
efb2efd to
88f9746
Compare
This commit adds loss of quorum recovery plan application functionality to server startup. Plans are staged during application phase and then applied when rolling restart of affected nodes is performed. Plan application peforms same actions that are performed by debug recovery apply-plan on offline storage. It is also updating loss of quorum recovery status stored in a store local key. Release note: None
aliher1911
force-pushed
the
loq_05online_apply_plan
branch
from
88f9746 to
9c6b9e9
Compare
| return err | ||
| } | ||
|
|
||
| if err := planStore.RemovePlan(); err != nil { |
There was a problem hiding this comment.
Should we remove this before it's successfully applied? Is it better to retry on the next restart?
There was a problem hiding this comment.
We already reported it as failed though. If we have a replica that updated after we created plan then we'll end up with plan that would fail forever not being able to apply. I think it is safer to drop it. And run another recovery if needed.
|
bors r=erikgrinaker |
|
Build succeeded: |
This commit adds loss of quorum recovery plan application functionality
to server startup. Plans are staged during application phase and then
applied when rolling restart of affected nodes is performed.
Plan application peforms same actions that are performed by debug
recovery apply-plan on offline storage. It is also updating loss of
quorum recovery status stored in a store local key.
Release note: None
Fixes #93121