Add node events #3945

chelseakomlo · 2018-03-06T15:55:02Z

This PR adds an the concept of node events. Only up to 10 node events for a single node are kept at one time. Node events can be created for different purposes, this PR adds the creation of a node event when a node is registered for the first time.

A few considerations:

Should we create constant values for expected node event messages, such as registration?

jippi · 2018-03-06T16:16:28Z

node event wish list:

node connect
node disconnect
node lost connection (~same as a disconnect, which is "planned")

dadgar · 2018-03-08T18:37:47Z

nomad/structs/structs.go

@@ -1153,6 +1154,9 @@ type Node struct {
 	// updated
 	StatusUpdatedAt int64

+	// NodeEvents is a list of the last 10 or lest events for this node


NodeEvents is the most recent set of events generated for the node.
Spelling error and generally don't include the const values in comments. Keep it a bit more generic

dadgar · 2018-03-08T18:38:45Z

nomad/structs/events.go

+	WriteRequest
+}
+
+// EmitNodeEventResponse is a server response to the client about the status of


The client isn't the only one who can call this, so make it more generic. Here and on the request.

dadgar · 2018-03-08T18:39:02Z

nomad/structs/events.go

+// EmitNodeEventRequest is a client request to update the node events source
+// with a new client-side event
+type EmitNodeEventRequest struct {
+	NodeID    string


map[string][]*NodeEvent please

dadgar · 2018-03-08T18:39:48Z

nomad/state/state_store.go

+		// also create a node registration event
+		nodeEvent := &structs.NodeEvent{
+			Message:   "Node Registered",
+			Subsystem: "Server",


dadgar · 2018-03-08T18:40:28Z

nomad/state/events_state_store.go

@@ -0,0 +1,47 @@
+package state


Keep this in the same file. We should break it up at some point into a bit higher level groups: [node, eval, alloc, etc]

dadgar · 2018-03-08T18:51:07Z

nomad/node_endpoint_test.go

+	var resp structs.GenericResponse
+	err = msgpackrpc.CallWithCodec(codec, "Node.EmitEvent", &req, &resp)
+	require.Nil(err)
+	require.NotEqual(0, resp.Index)


Check the node has the event

dadgar · 2018-03-08T18:53:47Z

nomad/state/events_state_store.go

+// events once this bound has been reached.
+func (s *StateStore) upsertNodeEvent(index uint64, nodeID string, event *structs.NodeEvent, txn *memdb.Txn) error {
+
+	ws := memdb.NewWatchSet()


Look how UpdateNodeStatus is done. This method needs to:

access the node directly, not via NodeByID

Copy the Node and reinsert it. (treat objects as immutable). This data structure allows multiple readers to be accessing it so you are modifying a consistent view if you don't copy and re-insert.

Update the Node's Modify index otherwise blocking queries will not work.

dadgar · 2018-03-08T18:55:16Z

nomad/state/events_state_store.go

+	nodeEvents = append(nodeEvents, event)
+	node.NodeEvents = nodeEvents
+
+	txn.Commit()


Shouldn't commit in the implementation method but rather the caller. Otherwise you can't use this method as follows:

handleSomething() { txn := s.db.Txn(true) defer txn.Abort() handleOne() upsertNodeEvent() handleTwo() txn.Commit() }

dadgar · 2018-03-08T18:56:35Z

nomad/state/events_state_store_test.go

+	"github.com/stretchr/testify/require"
+)
+
+func TestStateStore_AddSingleNodeEvent(t *testing.T) {


Look how the other tests access the field that is being modified first, then modify and then assert the watch has been fired. This verifies that the update to the object will unblock blocked queries.

dadgar · 2018-03-08T18:56:58Z

nomad/structs/structs.go

@@ -1153,6 +1154,9 @@ type Node struct {
 	// updated
 	StatusUpdatedAt int64

+	// NodeEvents is a list of the last 10 or lest events for this node
+	NodeEvents []*NodeEvent


Add to the api pkg

preetapan · 2018-03-09T16:33:50Z

nomad/state/events_state_store.go

+		e.ModifyIndex = index
+
+		// keep node events pruned to below 10 simultaneously
+		if len(nodeEvents) >= 10 {


make this 10 a constant named something like max_retained_node_events and use it here and in the tests.

preetapan · 2018-03-09T16:39:56Z

client/client.go

+	for {
+		select {
+		case event := <-c.triggerEmitNodeEvent:
+			batchEventsLock.Lock()


This is more of a Go question - why does this need a lock if only one of these goroutines run per client, and this goroutine is the only one that owns appending to batchevents?

+1 remove the lock

preetapan · 2018-03-09T16:40:31Z

client/client.go

+				continue
+			}
+
+			c.submitNodeEvents(batchEvents)


This should check for error state, and if there is no error set batchEvents to nil so that the next batch can start over.

If there's an error we could accommodate more events in the buffer but will need to have an upper bound so that it doesn't keep growing unbounded.

+1 to this and you should truncate this list to < 10 as well since if the client is disconnected for a while it may want to keep emitting events but they will fail. So limit the number of events. Please use the const value

preetapan · 2018-03-09T16:46:00Z

client/client.go

+		// emit node event goroutine was already running
+	}
+}
+


If possible, consider adding a test to client_test that verifies that the node events correctly propagate from the client via this goroutine to the server

dadgar · 2018-03-10T00:18:45Z

command/node_status.go

@@ -386,6 +388,34 @@ func (c *NodeStatusCommand) formatNode(client *api.Client, node *api.Node) int {

 }

+func (c *NodeStatusCommand) outputNodeStatusEvents(node *api.Node) {
+	c.Ui.Output(c.Colorize().Color("\n[bold]Node Events "))
+	c.outputNodeEvent(node.NodeEvents)


Can you take in whether it is in verbose mode and only emit details in verbose

dadgar · 2018-03-10T00:19:38Z

command/node_status.go

+func (c *NodeStatusCommand) outputNodeEvent(events []*api.NodeEvent) {
+	size := len(events)
+	nodeEvents := make([]string, size+1)
+	nodeEvents[0] = "Timestamp|Subsystem|Message|Details"


Time and not timestamp

dadgar · 2018-03-10T00:25:32Z

client/client.go

+// these kinds of events include when a driver moves from healthy to unhealthy
+// (and vice versa)
+func (c *Client) submitNodeEvents(e []*structs.NodeEvent) error {
+	node := c.Node()


use c.NodeID()

dadgar · 2018-03-10T00:27:03Z

client/client.go

+		c.logger.Printf("[ERR] client: emitting node events failed %v", err)
+		return err
+	}
+	c.logger.Printf("[INFO] client: emit node events complete")


Remove this INFO log

Should this be replaced with any log level, as this is logging?

dadgar · 2018-03-10T00:27:33Z

client/client.go

+	}
+	var resp structs.EmitNodeEventResponse
+	if err := c.RPC("Node.EmitEvent", &req, &resp); err != nil {
+		c.logger.Printf("[ERR] client: emitting node events failed %v", err)


Just return the error and have the logging in the watcher

dadgar · 2018-03-10T00:36:40Z

command/node_status.go

+}
+
+func formatEventDetails(details map[string]string) string {
+	var output string


Last detail pair will have trailing comma. Do:

output := make([]string, 0, len(details)) for k, v := range details { output = append(output, fmt.Sprintf("%s: %s, ", k, v)) } return strings.Join(output, ", ")

dadgar · 2018-03-10T00:37:22Z

nomad/fsm.go

@@ -236,6 +236,8 @@ func (n *nomadFSM) Apply(log *raft.Log) interface{} {
 		return n.applyACLTokenBootstrap(buf[1:], log.Index)
 	case structs.AutopilotRequestType:
 		return n.applyAutopilotUpdate(buf[1:], log.Index)
+	case structs.AddNodeEventType:


Change to AddNodeEventsType and the methods to capture the plurality

dadgar · 2018-03-10T00:40:29Z

nomad/fsm.go

+		return err
+	}
+
+	for nodeID, nodeEvents := range req.NodeEvents {


This all should be handled in the state store. Look at something like UpsertEvals.

It should be

if err := n.state.UpsertNodeEvents(index, req.NodeEvents); err != nil { n.logger.Printf("[ERR] nomad.fsm: failed to upsert node events: %v", err) return err }

dadgar · 2018-03-10T00:40:47Z

nomad/fsm.go

+func (n *nomadFSM) applyAddNodeEventType(buf []byte, index uint64) interface{} {
+	var req structs.EmitNodeEventRequest
+	if err := structs.Decode(buf, &req); err != nil {
+		n.logger.Printf("[ERR] nomad.fsm: failed to decode EmitNodeEventREquest: %v", err)


EmitNodeEventREquest

dadgar · 2018-03-10T00:44:24Z

nomad/state/state_store.go

+			Timestamp: node.StatusUpdatedAt,
+		}
+
+		node.NodeEvents = make([]*structs.NodeEvent, 0)


Make it at least size 1 so it doesn't resize immediately: make([]*structs.NodeEvent, 0, 1)

qkate · 2018-03-10T01:28:42Z

client/client.go

+	return nil
+}
+
+// emitEvent is a handler which receives node events and on a interval and


typo? "node events ~~and~~ on a interval"

or? "node events and on a interval ~~and~~ submits them"

Also I think s/emitEvent/watchEmitEvents in this comment?

qkate · 2018-03-10T01:32:02Z

client/client.go

+	}
+}
+
+// emitEvent triggers a emit node event


s/emitEvent/triggerNodeEvent?

qkate · 2018-03-10T01:36:06Z

command/agent/node_endpoint_test.go

@@ -396,5 +396,11 @@ func TestHTTP_NodeQuery(t *testing.T) {
 		if n.ID != node.ID {
 			t.Fatalf("bad: %#v", n)
 		}
+		if len(n.NodeEvents) != 1 {


Should this be <1 instead of !=1?

qkate · 2018-03-10T01:45:02Z

nomad/structs/structs.go

@@ -1153,11 +1154,52 @@ type Node struct {
 	// updated
 	StatusUpdatedAt int64

+	// NodeEvents is the most recent set of events generated for the node


When you add a constant per @preetapan 's feedback here: https://github.com/hashicorp/nomad/pull/3945/files#r173500252

then mention that constant in this comment. (Otherwise when reading this comment I'm curious "most recent events, like how many though?")

dadgar · 2018-03-13T03:23:20Z

client/client.go

@@ -1052,6 +1061,9 @@ func (c *Client) registerAndHeartbeat() {
 	// Start watching changes for node changes
 	go c.watchNodeUpdates()

+	// Start watching for emitting node events
+	go c.watchEmitEvents()


watchNodeEvents?

dadgar · 2018-03-13T03:23:48Z

client/client.go

+// these kinds of events include when a driver moves from healthy to unhealthy
+// (and vice versa)
+func (c *Client) submitNodeEvents(events []*structs.NodeEvent) error {
+	nodeID := c.Node().ID


c.NodeID()

dadgar · 2018-03-13T03:24:46Z

client/client.go

+	if err := c.RPC("Node.EmitEvents", &req, &resp); err != nil {
+		return fmt.Errorf("Emitting node event failed: %v", err)
+	}
+	c.logger.Printf("[INFO] client: emit node events complete")


Not a useful info level log. These should be useful for operators. Can you remove this

dadgar · 2018-03-13T03:42:02Z

client/client.go

+
+// watchEmitEvents is a handler which receives node events and on a interval and
+// submits them in batch format to the server
+func (c *Client) watchEmitEvents() {


https://gist.github.com/dadgar/64cbe0dd70b4703c06f12b0dcf6e16d8

dadgar · 2018-03-13T03:45:32Z

command/node_status.go

@@ -386,6 +388,38 @@ func (c *NodeStatusCommand) formatNode(client *api.Client, node *api.Node) int {

 }

+func (c *NodeStatusCommand) outputNodeStatusEvents(node *api.Node) {
+	if !c.verbose {


Sorry should have been clearer. The "details" column should only be outputted in verbose mode.

dadgar · 2018-03-13T03:45:50Z

command/node_status.go

+}
+
+func formatEventDetails(details map[string]string) string {
+	var output string


dadgar · 2018-03-13T03:48:05Z

nomad/node_endpoint.go

+		return err
+	}
+	defer metrics.MeasureSince([]string{"nomad", "client", "emit_event"}, time.Now())
+


Return an error if there are no events

dadgar · 2018-03-13T03:51:28Z

nomad/state/state_store.go

+		}
+
+		// Copy the existing node
+		copyNode := new(structs.Node)


copyNode = node.Copy()

dadgar · 2018-03-13T03:51:44Z

nomad/state/state_store.go

+
+			// keep node events pruned to below 10 simultaneously
+			if len(nodeEvents) >= structs.MaxRetainedNodeEvents {
+				delta := len(nodeEvents) - 10


Use the constant

dadgar · 2018-03-13T03:54:02Z

nomad/structs/structs.go

 	return nn
 }

+func copyNodeEvents(first *Node) []*NodeEvent {


This isn't actually copying. This is just appending them to a new list.

add node event when registering a node for the first time

Add node events to CLI

Changelog

github-actions · 2023-03-11T02:13:33Z

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.

chelseakomlo force-pushed the f-add-node-events branch from b012473 to c35d189 Compare March 8, 2018 11:02

dadgar requested changes Mar 8, 2018

View reviewed changes

chelseakomlo force-pushed the f-add-node-events branch from b7e2c37 to f93ea2a Compare March 9, 2018 14:22

preetapan reviewed Mar 9, 2018

View reviewed changes

dadgar requested changes Mar 10, 2018

View reviewed changes

qkate reviewed Mar 10, 2018

View reviewed changes

client/client.go Outdated

}

}

// emitEvent triggers a emit node event

Copy link

Contributor

qkate Mar 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/emitEvent/triggerNodeEvent?

qkate reviewed Mar 10, 2018

View reviewed changes

chelseakomlo force-pushed the f-add-node-events branch from 9cad283 to afe6637 Compare March 12, 2018 18:10

dadgar requested changes Mar 13, 2018

View reviewed changes

chelseakomlo force-pushed the f-add-node-events branch 2 times, most recently from 2295446 to 42c17f7 Compare March 13, 2018 23:36

dadgar approved these changes Mar 14, 2018

View reviewed changes

chelseakomlo added 12 commits March 13, 2018 18:05

NodeEvents struct

365d47c

RPC, FSM, state store for Node.EmitEvent

f051d43

add node event when registering a node for the first time

fix up error logging

55e0c7d

move adding node registration event to the state store

de6199a

code review feedback

551e96f

Ensure node updates don't strip node events

fc81975

Add node events to CLI

add node id to tests; check for node events in state store

56f5e66

move all structs to structs file

cbbb60a

code review feedback

705ff98

batch submitting node events

55670c8

add client side emitting of node events

a72dcbf

Changelog

fix up go check warnings

9ef6ad0

chelseakomlo and others added 7 commits March 13, 2018 18:08

keep state store functions in one file

3f561c3

code review feedback

6eb9911

make check fixes

3a1043d

code review feedback

319f809

fixes

57ddd51

nodeevents -> events

eceabb6

small cleanup

8ba5ba3

dadgar force-pushed the f-add-node-events branch from f1641b5 to 8ba5ba3 Compare March 14, 2018 01:08

chelseakomlo merged commit 9cb3a02 into master Mar 14, 2018

chelseakomlo deleted the f-add-node-events branch March 14, 2018 12:43

github-actions bot locked as resolved and limited conversation to collaborators Mar 11, 2023

Add node events #3945

Add node events #3945

Conversation

chelseakomlo commented Mar 6, 2018 • edited Loading

jippi commented Mar 6, 2018

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

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

preetapan Mar 9, 2018 • edited Loading

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

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Mar 11, 2023

chelseakomlo commented Mar 6, 2018 •

edited

Loading

preetapan Mar 9, 2018 •

edited

Loading