batch node deregistration #5784

langmartin · 2019-06-05T19:33:26Z

Change node deregistration to operate on a batch of node is, partitioned into maxIdsPerReap slices. The user-facing API submits a batch of 1.

Changelog

notnoop

the compatibility issue needs addressing first and will follow up with more feedback there.

nomad/structs/structs.go

nomad/state/state_store_test.go

nomad/util.go

endocrimes

This should be another raft message type, otherwise great start 👍

command/agent/node_endpoint.go

nomad/util.go

nomad/core_sched.go

endocrimes

A few questions about backwards compatibility and regression testing

nomad/node_endpoint.go

endocrimes · 2019-06-07T14:55:14Z

nomad/state/state_store.go

+// DeleteNode deregisters a batch of nodes
+func (s *StateStore) DeleteNode(index uint64, nodes []string) error {
+	if len(nodes) == 0 {
+		return nil


Should this potentially return an error? - It probably signifies a programmer error, as an empty node-id should previously have returned a node lookup failed error.

nomad/node_endpoint_test.go

nomad/fsm.go

command/agent/node_endpoint.go

nomad/core_sched.go

nomad/fsm.go

notnoop · 2019-06-26T13:53:52Z

nomad/fsm.go

@@ -191,6 +191,8 @@ func (n *nomadFSM) Apply(log *raft.Log) interface{} {
 		return n.applyUpsertNode(buf[1:], log.Index)
 	case structs.NodeDeregisterRequestType:
 		return n.applyDeregisterNode(buf[1:], log.Index)
+	case structs.NodeDeregisterBatchRequestType:


Are these sorted by their value? If so, probably belong to the bottom?

nomad/fsm.go

notnoop · 2019-06-26T14:02:06Z

nomad/structs/structs.go

@@ -54,6 +54,7 @@ type MessageType uint8
 const (
 	NodeRegisterRequestType MessageType = iota
 	NodeDeregisterRequestType
+	NodeDeregisterBatchRequestType // QUESTION does iota make it important to append this list?


Yes - this needs to be appended at the end. golang compiler would assign 0 to iota and then increments for subsequent lines. See https://play.golang.org/p/7ZqlecuVJHW and https://golang.org/ref/spec#Iota about iota and constants.

In raft, we store the message type uint8 in messages, so these order here must remain unaltered; otherwise, we will not interpret old raft messages properly on upgrade. Probably belongs to raft notes too.

notnoop · 2019-06-26T14:04:01Z

nomad/structs/structs.go

 type NodeDeregisterRequest struct {
 	NodeID string
 	WriteRequest
 }

+// NodeDeregisterBatchRequest is used for Node.DeregisterBatch endpoint
+// to deregister a batch of nodes from being schedulable entities.
+type NodeDeregisterBatchRequest struct {


I find our naming conventions here odd, Batch seems to appear inconsistently throughout the file: BatchNodeUpdateDrainRequest, JobBatchDeregisterRequest, and NodeDeregisterBatchRequest :(. The same goes for the fsm method handler name.

agreed. I'll rename it to avoid introducing a third pattern at least

command/agent/node_endpoint.go

notnoop · 2019-06-26T14:08:06Z

nomad/structs/structs.go

@@ -316,12 +317,20 @@ type NodeRegisterRequest struct {
 }

 // NodeDeregisterRequest is used for Node.Deregister endpoint
-// to deregister a node as being a schedulable entity.
+// to deregister a batch of nodes from being schedulable entities.
+// Deprecated in 0.9.3


Given that we still use it in http agent nodePurge, we might want to qualify its deprecation and scope it to internal scheduler work.

notnoop

sorry for the many back and forth - noticed one more compatibility issue :(.

nomad/core_sched.go

notnoop · 2019-06-27T03:15:32Z

nomad/structs/structs.go

@@ -316,12 +317,19 @@ type NodeRegisterRequest struct {
 }

 // NodeDeregisterRequest is used for Node.Deregister endpoint
-// to deregister a node as being a schedulable entity.
+// to deregister a batch of nodes from being schedulable entities.


the comment here doesn't need to be updated

notnoop · 2019-06-27T03:19:27Z

nomad/node_endpoint.go

 func (n *Node) Deregister(args *structs.NodeDeregisterRequest, reply *structs.NodeUpdateResponse) error {
-	if done, err := n.srv.forward("Node.Deregister", args, args, reply); done {
+	return n.BatchDeregister(&structs.NodeBatchDeregisterRequest{


I don't think we can safely forward to Node.BatchDeregister here when servers are running mixed fleet. Consider a 0.9.4 nomad server follower workin against 0.9.3 leader: the leader wouldn't be able to handle the call.

I think it's better to leave the old endpoints untouched and only add new ones.

thanks, that makes a lot of sense

notnoop · 2019-06-28T05:22:53Z

nomad/node_endpoint.go

 	if done, err := n.srv.forward("Node.Deregister", args, args, reply); done {
 		return err
 	}
+
+	// If we're handling the request locally on the leader, operate on a batch of size 1
+	return n.batchDeregister(&structs.NodeBatchDeregisterRequest{


:( I don't think this is right yet. Even if we are the leader, it's not safe to use the new raft message type until we confirm that all servers are at least 0.9.4. I would suggest keeping old handling untouched without it depending on batchDeregister (module some function extraction as preferred).

notnoop

lgtm with minor stylistic suggestion. Merge away. Thank you so much for following this through.

notnoop · 2019-07-02T03:41:38Z

nomad/node_endpoint.go

+// deregister takes a raftMessage closure, to support both Deregister and BatchDeregister
+func (n *Node) deregister(raftMessage func() (interface{}, uint64, error),
+	args *structs.NodeBatchDeregisterRequest,
+	reply *structs.NodeUpdateResponse) error {


I would suggest ordering them in the following args, reply, raftApplyFn. It's a bit more consistent with rest of functions, and it's a bit nicer visually for the lambda function to appear last. Also, calling raftApplyFn or applyRaftMsgFn probably captures meaning better.

Co-Authored-By: Mahmood Ali <mahmood@notnoop.com>

github-actions · 2023-02-06T02:15:15Z

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

langmartin requested a review from dadgar June 5, 2019 19:33

notnoop suggested changes Jun 6, 2019

View reviewed changes

nomad/structs/structs.go Show resolved Hide resolved

nomad/state/state_store_test.go Show resolved Hide resolved

nomad/util.go Outdated Show resolved Hide resolved

nomad/util.go Outdated Show resolved Hide resolved

endocrimes suggested changes Jun 6, 2019

View reviewed changes

command/agent/node_endpoint.go Outdated Show resolved Hide resolved

nomad/util.go Outdated Show resolved Hide resolved

schmichael reviewed Jun 6, 2019

View reviewed changes

nomad/core_sched.go Outdated Show resolved Hide resolved

endocrimes suggested changes Jun 7, 2019

View reviewed changes

langmartin force-pushed the b-batch-node-dereg branch from 880ac36 to df405e0 Compare June 7, 2019 15:28

notnoop reviewed Jun 7, 2019

View reviewed changes

command/agent/node_endpoint.go Outdated Show resolved Hide resolved

nomad/core_sched.go Outdated Show resolved Hide resolved

nomad/fsm.go Outdated Show resolved Hide resolved

nomad/fsm.go Outdated Show resolved Hide resolved

langmartin force-pushed the b-batch-node-dereg branch from cfd3fa6 to eda57a7 Compare June 13, 2019 19:12

langmartin requested review from endocrimes and notnoop June 13, 2019 20:55

langmartin force-pushed the b-batch-node-dereg branch from 943bcd9 to 3cc4c64 Compare June 26, 2019 14:05

notnoop reviewed Jun 26, 2019

View reviewed changes

notnoop reviewed Jun 27, 2019

View reviewed changes

langmartin requested a review from notnoop June 27, 2019 19:55

notnoop reviewed Jun 28, 2019

View reviewed changes

notnoop approved these changes Jul 2, 2019

View reviewed changes

langmartin force-pushed the b-batch-node-dereg branch 4 times, most recently from 78736ae to 4ced779 Compare July 8, 2019 16:31

langmartin added 10 commits July 10, 2019 13:56

struct NodeDeregisterRequest has a batch of NodeIDs

4771de6

state_store DeleteNode operates on a batch of ids

4720306

util partitionAll for paging

8516911

core_sched batch node deregistration requests

fbc7fb8

fsm NodeDeregisterRequest is now a batch

f685b00

node_endpoint deregister the batch of nodes

64fd8b3

drainer watch_nodes_test batch of 1

94cd86f

state_store improve error messages

0dd5335

node_endpoint improve error messages

fbd4c25

state_store just one index update, test deletion

5810a09

langmartin and others added 24 commits July 10, 2019 13:56

core_sched check ServersMeetMinimumVersion, send old node deregister

3c97bdc

structs add back NodeDeregisterRequest.NodeID, compatibility

7258fab

fsm honor new and old style NodeDeregisterRequests

97a76af

core_sched check ServersMeetMinimumVersion

aa5193b

util simplify partitionAll

48c1859

node_endpoint raft store then shutdown, test deprecation

d55e79e

fsm variable names for consistency

5921e6f

fsm return an error on both NodeDeregisterRequest fields set

3e097c4

node_endpoint argument setup

bc5e5cf

structs add NodeDeregisterBatchRequest

f32dceb

node endpoint support new NodeDeregisterBatchRequest

23fdadf

fsm support new NodeDeregisterBatchRequest

9c42d03

core_sched use the new rpc names

4d11c32

new file: contributing/checklist-rpc-endpoint.md

c915bf0

fsm label batch_deregister_node metrics explicitly

c20c232

Co-Authored-By: Mahmood Ali <mahmood@notnoop.com>

structs NodeDeregisterBatchRequestType must go at the end

f5632ba

checklist NodeDeregisterBatchRequestType must go at the end

0deb209

fsm new NodeDeregisterBatchRequestType sorted at the end of the case

838460e

state_store error if called without node_ids

3ad8a24

NodeDeregisterBatch -> NodeBatchDeregister match JobBatch pattern

9bdf35a

structs drop deprecation warning, revert unnecessary comment change

675f750

core_sched add compat comment for later removal

5eac719

node_endpoint preserve both messages as rpcs and in raft

e0edc7d

Changelog

17f4951

langmartin force-pushed the b-batch-node-dereg branch from 4ced779 to 17f4951 Compare July 10, 2019 17:57

langmartin merged commit 8da0b91 into master Jul 10, 2019

langmartin deleted the b-batch-node-dereg branch July 10, 2019 18:25

github-actions bot locked as resolved and limited conversation to collaborators Feb 6, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

batch node deregistration #5784

batch node deregistration #5784

langmartin commented Jun 5, 2019 •

edited

Loading

notnoop left a comment

endocrimes left a comment

endocrimes left a comment

endocrimes Jun 7, 2019

notnoop Jun 26, 2019

notnoop Jun 26, 2019

notnoop Jun 26, 2019

langmartin Jun 26, 2019

notnoop Jun 26, 2019

notnoop left a comment

notnoop Jun 27, 2019

notnoop Jun 27, 2019

langmartin Jun 27, 2019

notnoop Jun 28, 2019

notnoop left a comment

notnoop Jul 2, 2019

github-actions bot commented Feb 6, 2023

batch node deregistration #5784

batch node deregistration #5784

Conversation

langmartin commented Jun 5, 2019 • edited Loading

notnoop left a comment

Choose a reason for hiding this comment

endocrimes left a comment

Choose a reason for hiding this comment

endocrimes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

notnoop left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

notnoop left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Feb 6, 2023

langmartin commented Jun 5, 2019 •

edited

Loading