kvserver: skip non-live nodes when considering candidates for transfers by knz · Pull Request #55808 · cockroachdb/cockroach

knz · 2020-10-21T11:16:29Z

(This PR is forked off #55460 to simplify the discussion. I believe there's no discussion left here? Maybe I can merge it directly?)

Fixes #55440.

Prior to this patch, 3 components could attempt to transfer a replica
to a node currently being drained:

the store rebalancer, which rebalances replicas based on disk
usage and QPS.
the allocator, to place new replicas.
the allocator, to rebalance replicas depending on load.

This commit introduces a consideration for node liveness when building
the list of candidates, to detect whether a target node is
acceptable. Any node that is not LIVE according to its liveness status
is not considered for a transfer.

Release note (bug fix): In some cases CockroachDB would attempt to
transfer ranges to nodes in the process of being decommissioned or
being shut down; this could cause disruption the moment the node
did actually terminate. This bug has been fixed. It had been
introduced some time before v2.0.

cockroach-teamcity · 2020-10-21T11:16:36Z

This change is

tbg

if there weren't any unresolved comments on the diff in the original PR.

Reviewed 7 of 7 files at r1.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @andreimatei and @knz)

pkg/kv/kvserver/store_rebalancer.go, line 675 at r1 (raw file):

	}

	// If the target store is on a separate node, we will also care

This probably does fix replicate/wide, though not the general problem underlying it. I think the roachtest always ends up wanting to move replicas to a dead node, which will be prevented by your new check. Though it could still end up wanting to move data to another live node in theory. I shall validate.

tbg · 2020-10-21T12:24:19Z

pkg/kv/kvserver/allocator_test.go

+// TestAllocateCandidatesExcludeDrainingNodes checks that draining nodes,
+// as per a store pool's isNodeValidForRoutineReplicaTransfer(),
+// are excluded from the list of candidates for an allocation.
+func TestAllocateCandidatesExcludeDrainingNodes(t *testing.T) {


nit: there's nothing particular about draining here. You're testing the isNodeReadyFor.... method. Maybe TestAllocateCandidatesExcludeNonReadyNodes.

tbg · 2020-10-21T12:25:42Z

What has me confused is that I think you made this PR primarily for draining nodes, but are you really changing anything for draining nodes? I think a draining node will show up as LIVE, won't it?

cockroach/pkg/kv/kvserver/store_pool.go

Lines 142 to 161 in 785aea7

    
           func LivenessStatus( 
        
           	l kvserverpb.Liveness, now time.Time, deadThreshold time.Duration, 
        
           ) kvserverpb.NodeLivenessStatus { 
        
           	if l.IsDead(now, deadThreshold) { 
        
           		if !l.Membership.Active() { 
        
           			return kvserverpb.NodeLivenessStatus_DECOMMISSIONED 
        
           		} 
        
           		return kvserverpb.NodeLivenessStatus_DEAD 
        
           	} 
        
           	if !l.Membership.Active() { 
        
           		return kvserverpb.NodeLivenessStatus_DECOMMISSIONING 
        
           	} 
        
           	if l.Draining { 
        
           		return kvserverpb.NodeLivenessStatus_UNAVAILABLE 
        
           	} 
        
           	if l.IsLive(now) { 
        
           		return kvserverpb.NodeLivenessStatus_LIVE 
        
           	} 
        
           	return kvserverpb.NodeLivenessStatus_UNAVAILABLE 
        
           }

The concept of draining seems wholly unconnected to the NodeLivenessStatus.

knz

are you really changing anything for draining nodes? I think a draining node will show up as LIVE, won't it?

no it shows as unavailable: see, if l.Draining comes before l.IsLive.

Is this ready to merge?

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @andreimatei and @tbg)

pkg/kv/kvserver/allocator_test.go, line 2460 at r1 (raw file):

Previously, tbg (Tobias Grieger) wrote…

nit: there's nothing particular about draining here. You're testing the isNodeReadyFor.... method. Maybe TestAllocateCandidatesExcludeNonReadyNodes.

Renamed.

pkg/kv/kvserver/store_rebalancer.go, line 675 at r1 (raw file):

Previously, tbg (Tobias Grieger) wrote…

This probably does fix replicate/wide, though not the general problem underlying it. I think the roachtest always ends up wanting to move replicas to a dead node, which will be prevented by your new check. Though it could still end up wanting to move data to another live node in theory. I shall validate.

Yes agreed.

tbg

no it shows as unavailable: see, if l.Draining comes before l.IsLive.

Oh, missed that, thanks.

LGTM, though can you update the comment on NODE_STATUS_UNAVAILABLE to indicate that a draining node falls into this category?

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @andreimatei and @tbg)

Prior to this patch, 3 components could attempt to transfer a replica to a node currently being drained: - the store rebalancer, which rebalances replicas based on disk usage and QPS. - the allocator, to place new replicas. - the allocator, to rebalance replicas depending on load. This commit introduces a consideration for node liveness when building the list of candidates, to detect whether a target node is acceptable. Any node that is not LIVE according to its liveness status is not considered for a transfer. Release note (bug fix): In some cases CockroachDB would attempt to transfer ranges to nodes in the process of being decommissioned or being shut down; this could cause disruption the moment the node did actually terminate. This bug has been fixed. It had been introduced some time before v2.0.

knz

LGTM, though can you update the comment on NODE_STATUS_UNAVAILABLE to indicate that a draining node falls into this category?

Done.

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @andreimatei and @tbg)

knz · 2020-11-12T17:40:32Z

bors r=tbg

craig · 2020-11-12T18:55:33Z

This PR was included in a batch that was canceled, it will be automatically retried

craig · 2020-11-12T19:58:13Z

Build succeeded:

GitHub CI (Cockroach)

knz requested review from andreimatei and tbg October 21, 2020 11:16

knz mentioned this pull request Oct 21, 2020

server,kvserver: wait for liveness record refresh on other nodes at the end of drain #55460

Draft

tbg approved these changes Oct 21, 2020

View reviewed changes

tbg reviewed Oct 21, 2020

View reviewed changes

knz commented Oct 23, 2020

View reviewed changes

knz force-pushed the 20201021-rebalance branch from ff76faf to b398d4d Compare October 23, 2020 16:34

tbg reviewed Oct 30, 2020

View reviewed changes

tbg self-requested a review October 30, 2020 11:40

knz force-pushed the 20201021-rebalance branch from b398d4d to ffa0ce3 Compare November 12, 2020 17:40

knz commented Nov 12, 2020

View reviewed changes

craig bot merged commit a5e67b6 into cockroachdb:master Nov 12, 2020

knz deleted the 20201021-rebalance branch November 13, 2020 10:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kvserver: skip non-live nodes when considering candidates for transfers#55808

kvserver: skip non-live nodes when considering candidates for transfers#55808
craig[bot] merged 1 commit intocockroachdb:masterfrom
knz:20201021-rebalance

knz commented Oct 21, 2020 •

edited

Loading

Uh oh!

cockroach-teamcity commented Oct 21, 2020

Uh oh!

tbg left a comment

Uh oh!

tbg Oct 21, 2020

Uh oh!

tbg commented Oct 21, 2020

Uh oh!

knz left a comment

Uh oh!

tbg left a comment

Uh oh!

knz left a comment

Uh oh!

knz commented Nov 12, 2020

Uh oh!

craig bot commented Nov 12, 2020

Uh oh!

craig bot commented Nov 12, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

knz commented Oct 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cockroach-teamcity commented Oct 21, 2020

Uh oh!

tbg left a comment

Choose a reason for hiding this comment

Uh oh!

tbg Oct 21, 2020

Choose a reason for hiding this comment

Uh oh!

tbg commented Oct 21, 2020

Uh oh!

knz left a comment

Choose a reason for hiding this comment

Uh oh!

tbg left a comment

Choose a reason for hiding this comment

Uh oh!

knz left a comment

Choose a reason for hiding this comment

Uh oh!

knz commented Nov 12, 2020

Uh oh!

craig bot commented Nov 12, 2020

Uh oh!

craig bot commented Nov 12, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

knz commented Oct 21, 2020 •

edited

Loading