-
Notifications
You must be signed in to change notification settings - Fork 1.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Emit Node Events for draining #4284
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -27,6 +27,11 @@ const ( | |
// maxParallelRequestsPerDerive is the maximum number of parallel Vault | ||
// create token requests that may be outstanding per derive request | ||
maxParallelRequestsPerDerive = 16 | ||
|
||
// NodeDrainEvents are the various drain messages | ||
NodeDrainEventDrainSet = "Node drain strategy set" | ||
NodeDrainEventDrainDisabled = "Node drain disabled" | ||
NodeDrainEventDrainUpdated = "Node drain stategy updated" | ||
) | ||
|
||
// Node endpoint is used for client interactions | ||
|
@@ -439,6 +444,9 @@ func (n *Node) UpdateDrain(args *structs.NodeUpdateDrainRequest, | |
if args.NodeID == "" { | ||
return fmt.Errorf("missing node ID for drain update") | ||
} | ||
if args.NodeEvent != nil { | ||
return fmt.Errorf("node event must not be set") | ||
} | ||
|
||
// Look for the node | ||
snap, err := n.srv.fsm.State().Snapshot() | ||
|
@@ -468,6 +476,18 @@ func (n *Node) UpdateDrain(args *structs.NodeUpdateDrainRequest, | |
args.DrainStrategy.ForceDeadline = time.Now().Add(args.DrainStrategy.Deadline) | ||
} | ||
|
||
// Construct the node event | ||
args.NodeEvent = structs.NewNodeEvent().SetSubsystem(structs.NodeEventSubsystemDrain) | ||
if node.DrainStrategy == nil && args.DrainStrategy == nil { | ||
return nil // Nothing to do | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't this go to line 492 where you apply the node update raft transaction? If there is no node drain event, seems like it should still apply the node update without the event. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So I made this skip the raft transaction since it is a no-op. It is essentially saying, please update the node to have no drain strategy even when the node already doesn't have one. |
||
} else if node.DrainStrategy == nil && args.DrainStrategy != nil { | ||
args.NodeEvent.SetMessage(NodeDrainEventDrainSet) | ||
} else if node.DrainStrategy != nil && args.DrainStrategy != nil { | ||
args.NodeEvent.SetMessage(NodeDrainEventDrainUpdated) | ||
} else if node.DrainStrategy != nil && args.DrainStrategy == nil { | ||
args.NodeEvent.SetMessage(NodeDrainEventDrainDisabled) | ||
} | ||
|
||
// Commit this update via Raft | ||
_, index, err := n.srv.raftApply(structs.NodeUpdateDrainRequestType, args) | ||
if err != nil { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what are the other two events expected and should they also be asserted here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be Register and Drain Enabled. I am not asserting it here since those aren't set by the node drainer system which this is testing.