Bug Report
Summary
TiKV cancel-offline currently cancels the first beingOffline instance when scaling back out, without considering the topology policy used by scale-in. If multiple TiKV instances are being offlined across different availability zones, partially canceling the scale-in can keep the wrong instance and leave the TiKVGroup AZ distribution imbalanced.
Code path
At 7bcbb0ef23ba72c9aae0bffd67389915ffbfdb32:
pkg/controllers/tikvgroup/tasks/updater.go builds a topology policy from spec.schedulePolicies and applies it to normal add/delete/update paths:
WithAddHooks(..., topoPolicy)
WithDelHooks(topoPolicy)
WithScaleInPreferPolicy(topoPolicy.PolicyScaleIn())
pkg/updater/policy/topology.go implements PolicyScaleIn() using p.all.NextDel(), so normal scale-in chooses an instance whose deletion keeps the topology spread balanced.
pkg/updater/actor.go handles scale-out while there are offlining instances by canceling offline instead of creating a new instance:
func (act *actor[T, O, R]) ScaleOut(ctx context.Context) error {
if act.beingOffline.Len() > 0 {
// TODO: could implement more sophisticated selection logic
if err := act.cancelOneOfflining(ctx, act.beingOffline.List()[0]); err != nil {
return err
}
return nil
}
...
}
This bypasses the topology policy entirely. The selected instance depends on beingOffline.List()[0], not on which cancellation would restore or preserve the best AZ spread.
Reproduction scenario
Assume a TiKVGroup uses an EvenlySpread schedule policy across AZs and starts from a balanced layout:
az-a: tikv-a1, tikv-a2
az-b: tikv-b1, tikv-b2
az-c: tikv-c1, tikv-c2
Scale from 6 to 3. The topology-aware scale-in may mark one instance in each AZ offline, leaving one active instance per AZ.
Before the offline process completes, cancel the scale-in partially by increasing replicas from 3 to 4. The expected result is to keep one of the offlining instances in an AZ that produces the best 4-replica spread, for example 2/1/1.
Currently, ScaleOut() cancels act.beingOffline.List()[0]. If this order does not match the topology-aware choice, it can keep an arbitrary AZ and produce a less desirable spread after the remaining offlining instances are deleted. The same issue is more visible when the offlining set is not symmetric, for example after a previous topology change or after one offlining instance has already reached StoreOfflined and moved to the deleted set.
Expected behavior
Cancel-offline should use a topology-aware selection policy, similar to normal scale-in/scale-out:
- When scaling out by canceling an offlining instance, choose the
beingOffline instance whose return best balances the topology spread.
- Existing add/update/delete hooks should remain consistent with the in-memory topology state used during one updater execution.
- Unit tests should cover partial cancel-offline with topology labels/AZs.
Actual behavior
Cancel-offline ignores the topology policy and always picks the first item from beingOffline.List().
Suggested fix direction
Introduce a cancel-offline prefer policy or reuse topology scheduling in ScaleOut() when beingOffline.Len() > 0. The policy probably needs to evaluate the topology after adding a beingOffline instance back to the active set, instead of simply following list order.
Also consider making the test deterministic for this scenario, because the current behavior depends on the ordering of State.List().
Bug Report
Summary
TiKV cancel-offline currently cancels the first
beingOfflineinstance when scaling back out, without considering the topology policy used by scale-in. If multiple TiKV instances are being offlined across different availability zones, partially canceling the scale-in can keep the wrong instance and leave the TiKVGroup AZ distribution imbalanced.Code path
At
7bcbb0ef23ba72c9aae0bffd67389915ffbfdb32:pkg/controllers/tikvgroup/tasks/updater.gobuilds a topology policy fromspec.schedulePoliciesand applies it to normal add/delete/update paths:WithAddHooks(..., topoPolicy)WithDelHooks(topoPolicy)WithScaleInPreferPolicy(topoPolicy.PolicyScaleIn())pkg/updater/policy/topology.goimplementsPolicyScaleIn()usingp.all.NextDel(), so normal scale-in chooses an instance whose deletion keeps the topology spread balanced.pkg/updater/actor.gohandles scale-out while there are offlining instances by canceling offline instead of creating a new instance:This bypasses the topology policy entirely. The selected instance depends on
beingOffline.List()[0], not on which cancellation would restore or preserve the best AZ spread.Reproduction scenario
Assume a TiKVGroup uses an
EvenlySpreadschedule policy across AZs and starts from a balanced layout:az-a:tikv-a1,tikv-a2az-b:tikv-b1,tikv-b2az-c:tikv-c1,tikv-c2Scale from 6 to 3. The topology-aware scale-in may mark one instance in each AZ offline, leaving one active instance per AZ.
Before the offline process completes, cancel the scale-in partially by increasing replicas from 3 to 4. The expected result is to keep one of the offlining instances in an AZ that produces the best 4-replica spread, for example
2/1/1.Currently,
ScaleOut()cancelsact.beingOffline.List()[0]. If this order does not match the topology-aware choice, it can keep an arbitrary AZ and produce a less desirable spread after the remaining offlining instances are deleted. The same issue is more visible when the offlining set is not symmetric, for example after a previous topology change or after one offlining instance has already reachedStoreOfflinedand moved to the deleted set.Expected behavior
Cancel-offline should use a topology-aware selection policy, similar to normal scale-in/scale-out:
beingOfflineinstance whose return best balances the topology spread.Actual behavior
Cancel-offline ignores the topology policy and always picks the first item from
beingOffline.List().Suggested fix direction
Introduce a cancel-offline prefer policy or reuse topology scheduling in
ScaleOut()whenbeingOffline.Len() > 0. The policy probably needs to evaluate the topology after adding abeingOfflineinstance back to the active set, instead of simply following list order.Also consider making the test deterministic for this scenario, because the current behavior depends on the ordering of
State.List().