pingcap / pingcap/tidb-operator

TiKV cancel-offline should preserve topology balance

Open
#6,991 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
Go
Stars
1.3k
Forks
540
Avg merge
3d 2h
Merged PRs (30d)
18

Description

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().

  • HuaxiClaw

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start in pkg/updater/actor.go at ScaleOut and compare its cancelOneOfflining path with pkg/updater/policy/topology.go and the policy setup in pkg/controllers/tikvgroup/tasks/updater.go. Trace how the in-memory topology state is used, then add deterministic unit coverage for partial cancel-offline across AZ labels. Done means cancellation selects the instance that best preserves topology balance without disrupting existing hooks.

Written by the indexing model from the issue text.

Assessment

Tech stack
go, kubernetes
Domain
devops, distributed-systems
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
66/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.