command: add '-monitor' flag to node drain #4260
Conversation
command/node_drain.go
Outdated
| // If monitoring the drain start the montior and return when done | ||
| if monitor { | ||
| c.Ui.Info(fmt.Sprintf("%s: Monitoring node %q: Ctrl-C to detach monitoring", formatTime(time.Now()), node.ID)) | ||
| c.monitorDrain(client, context.Background(), node, node.ModifyIndex-1, ignoreSystem) |
There was a problem hiding this comment.
@dadgar does this seem reasonable to -1 the index? Otherwise the drain blocks until the node is updated, which causes odd UX if the node has completed draining.
nickethier
force-pushed
the
f-cli-node-drain-monitor
branch
from
0a738be
to
59db0cb
Compare
command/node_drain.go
Outdated
|
|
||
| if err := flags.Parse(args); err != nil { | ||
| return 1 | ||
| } | ||
|
|
||
| // Check that enable or disable is not set with monitor | ||
| if monitor && enable || monitor && disable { |
There was a problem hiding this comment.
monitor && ( enable || disable ) ?
There was a problem hiding this comment.
Without the context, that looks kind of goofy.
CHANGELOG.md
Outdated
| @@ -1,6 +1,8 @@ | |||
| ## 0.8.4 (Unreleased) | |||
|
|
|||
| IMPROVEMENTS: | |||
| * cli: Add node drain monitoring with new `-monitor` flag on node drain | |||
There was a problem hiding this comment.
So this should be below the other since they are both under CLI but the length of this changelog entry is longer
| @@ -113,7 +113,8 @@ func (c *NodeDrainCommand) Name() string { return "node-drain" } | |||
|
|
|||
| func (c *NodeDrainCommand) Run(args []string) int { | |||
| var enable, disable, detach, force, | |||
| noDeadline, ignoreSystem, keepIneligible, self, autoYes bool | |||
| noDeadline, ignoreSystem, keepIneligible, | |||
command/node_drain_test.go
Outdated
| out = outBuf.String() | ||
| t.Logf("Output:\n%s", out) | ||
|
|
||
| require.Contains(out, "marked all allocations for migration") |
There was a problem hiding this comment.
Hmm that is pretty jank. If you run nomad node drain -monitor <node that has never been marked for drain> it will say that all allocations have been marked for migration even if that isn't true.
I think we need to modify this (https://github.com/hashicorp/nomad/blob/51440e24a7886986d74fcb94320707842adcb39c/api/nodes.go) to track whether the DrainStrategy was ever non-nil and then if it becomes nil it can print that message. Otherwise if it is nil and has never been set to a non-nil strategy we can print a more generic message like, "no drain strategy set"
nickethier
force-pushed
the
f-cli-node-drain-monitor
branch
from
06c8cdc
to
0c62b9c
Compare
|
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. |
No description provided.