Node heartbeats #3709

rossjones · 2024-03-26T16:31:06Z

Implements heartbeats for compute nodes, sending heartbeat messages to the requester node over NATS PubSub. The server, upon receiving a heartbeat updates the map of nodes to include the current server-side timestamp.

Compute nodes using the heartbeat client, will continuously send heartbeat messages every n seconds.

The heartbeat server receiving these heartbeats maintains a priority queue, which dequeues oldest items (lowest timestamp) first. Every 5 seconds any item older than a specific timestamp is dequeued, and its state either set to unhealthy (if it is the first missed heartbeat) or unknown if it is the second. The default for timestamps is

30s since heartbeat - unhealthy
60s since heartbeat - unknown (node may be live but disconnected)

The next heartbeat sent by a unhealthy of unknown node will make it healthy again and ready to receive work.

The current state of the node is added to the nodeinfo during a Get/GetByPrefix/List call to the node info store. This means that the liveness is dynamic and not persisted to the kvstore for node info.

coderabbitai · 2024-03-26T16:31:11Z

Important

Auto Review Skipped

Auto reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository.

To trigger a single review, invoke the @coderabbitai review command.

Tips

Chat

There are 3 ways to chat with CodeRabbit:

Review comments: Directly reply to a review comment made by CodeRabbit. Example:
- I pushed a fix in commit <commit_id>.
- Generate unit testing code for this file.
- Open a follow-up GitHub issue for this discussion.
Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
- @coderabbitai generate unit testing code for this file.
- @coderabbitai modularize this function.
PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
- @coderabbitai generate interesting stats about this repository and render them as a table.
- @coderabbitai show all the console.log statements in this repository.
- @coderabbitai read src/utils.ts and generate unit testing code.
- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (invoked as PR comments)

@coderabbitai pause to pause the reviews on a PR.
@coderabbitai resume to resume the paused reviews.
@coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
@coderabbitai resolve resolve all the CodeRabbit review comments.
@coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (`.coderabbit.yaml`)

You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
Please see the configuration documentation for more information.
If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

Visit our Documentation for detailed information on how to use CodeRabbit.
Join our Discord Community to get help, request features, and share feedback.
Follow us on X/Twitter for updates and announcements.

frrist

few drive by comments.

pkg/compute/management_client.go

pkg/node/heartbeat/client.go

pkg/node/heartbeat/server.go

frrist

Good stuff!

Left a few comment's I'd like addressed/discussed before merging.

pkg/compute/management_client.go

pkg/models/node_state.go

pkg/node/heartbeat/client.go

pkg/node/heartbeat/types.go

pkg/node/node.go

Adds pkg/node/heartbeat which contains a client and a server for sending heartbeat messages over NATS PubSub.

Enqueues, or re-enqueues heartbeats from nodes as they arrive into a priority queue.

Also enables the use of ordering and filtering of the status field (nodeinfo.State). This means that the approval field is no longer labelled as Status, but as Approval.

aronchick

LGTM but i think we should go with just a binary classification

aronchick · 2024-04-03T17:00:20Z

cmd/cli/node/list.go

@@ -16,15 +16,17 @@ import (

 var defaultColumnGroups = []string{"labels", "capacity"}
 var orderByFields = []string{"id", "type", "available_cpu", "available_memory", "available_disk", "available_gpu", "status"}
-var filterStatusValues = []string{"approved", "pending", "rejected"}
+var filterApprovalValues = []string{"approved", "pending", "rejected"}
+var filterStatusValues = []string{"healthy", "unhealthy", "unknown"}


i think it should be binary - "connected" or "unconnected" (we could add something more sophisticated like "unhealthy" when we have more health checks

Just for the filters, or for thenode list as well?

i think the node list as well - "healthy" and "unhealthy" imply something i don't think we know. We only know if it's connected or disconnected.

Went with Connected/Disconnected for now, discussed with Walid and will add another duration in future to mark the point beyond which we don't think the node is coming back.

docs/docs/dev/cli-reference/cli/node/list/index.md

frrist

Looks good, once comments are address

frrist · 2024-04-04T18:05:47Z

cmd/cli/node/list.go

 	nodeCmd.Flags().StringVar(&o.FilterByStatus, "filter-status", o.FilterByStatus,
 		fmt.Sprintf("Filter nodes by status. One of: %q", filterStatusValues))

 	return nodeCmd
 }

 // Run executes node command
-func (o *ListOptions) run(cmd *cobra.Command, _ []string) {
+func (o *ListOptions) run(cmd *cobra.Command, _ []string) error {


Non-blocking: if we are going to return an error from this method can we replace all the util.Fatal calls while we are at it? (be wary this might break some tests that (annoyingly) override the util.Fatal field 😞)

Can definitely add a ticket to address this, yeah. There's a mix of approaches atm.

Added #3764

frrist · 2024-04-04T18:06:57Z

cmd/cli/node/list.go

+			return fmt.Errorf("cannot use '%s' as filter approval value, should be one of: %q", o.FilterByApproval, filterApprovalValues)
+		}
+	}
+
 	if o.FilterByStatus != "" {
 		if !slices.Contains(filterStatusValues, o.FilterByStatus) {
-			util.Fatal(cmd, fmt.Errorf("cannot use '%s' as filter status value, should be one of: %q", o.FilterByStatus, filterStatusValues), 1)
+			return fmt.Errorf("cannot use '%s' as filter status value, should be one of: %q", o.FilterByStatus, filterStatusValues)


consider printing the specific flag name in the error message e.g. filter-status and filter-approval

frrist · 2024-04-04T18:09:04Z

docs/docs/dev/cli-reference/cli/node/approve/index.md

+
+# Command: `node approve`
+
+The `bacalhau node approve` command offers administrations the ability to approve the cluster membership for a node using its unique identifier.


command offers ~~administrations~~ administrators?
unique identifier being synonymous with name, right?

frrist · 2024-04-04T18:10:48Z

docs/docs/dev/cli-reference/cli/node/approve/index.md

+
+- `-m message`:
+
+  - A message to be attached to the approval action.


makes me wonder if we should also include the ClientID of the user issuing the approval, but that could always be included later.

Yes, I think at the moment something being audited just means it was logged somewhere, and so the client id should be in the request.

frrist · 2024-04-04T18:11:08Z

docs/docs/dev/cli-reference/cli/node/delete/index.md

+sidebar_label: delete
+---
+
+# Command: `node approve`


node delete

frrist · 2024-04-04T18:37:37Z

pkg/models/node_state.go

+// To add a new state,
+// * add it to the end of the list in the const below
+// * add it to strLivenessArray and typeLivenessMap
+// * add it to the livenessContainer and corresponding NodeStates var.
+// * add it to the All() method in the livenessContainer


frrist · 2024-04-04T18:41:12Z

pkg/compute/management_client.go

@@ -124,36 +127,62 @@ func (m *ManagementClient) updateResources(ctx context.Context) {
 		Resources: resources,
 	})
 	if err != nil {
-		log.Ctx(ctx).Error().Err(err).Msg("failed to send resource update to requester node")


While I am thinking about this. Above on line 103 where we call getNodeInfo we do:

nodeInfo := m.getNodeInfo(ctx) response, err := m.managementProxy.UpdateInfo(ctx, requests.UpdateInfoRequest{ Info: nodeInfo, }) if err != nil { log.Ctx(ctx).Error().Err(err).Msg("failed to send update info to requester node") } // TODO response will be nil when err != nil and this code will panic (has panicked for me) if response.Accepted { log.Ctx(ctx).Debug().Msg("update info accepted") } else { log.Ctx(ctx).Error().Msgf("update info rejected: %s", response.Reason) }

Can we address the panic here?

I think I intended this to return after logging the error. Fixed in a96c1b0

frrist · 2024-04-04T18:43:01Z

pkg/node/heartbeat/server.go

+
+	go func(ctx context.Context) {
+		defer func() {
+			_ = h.subscription.Close(ctx) // We're closing down, not much we can do with an error


we can log it on the line below

frrist · 2024-04-04T18:44:42Z

pkg/node/manager/node_manager.go

 	}
 }

+func (n *NodeManager) Start(ctx context.Context) error {
+	log.Ctx(ctx).Info().Msg("Node manager started")


log this after the thing starts, it technically hasn't started yet, and it may not start if n.heartbeats is nil (why do we have this conditional?)

frrist · 2024-04-04T18:44:59Z

pkg/node/manager/node_manager.go

+func (n *NodeManager) Start(ctx context.Context) error {
+	log.Ctx(ctx).Info().Msg("Node manager started")
+
+	if n.heartbeats != nil {


when can this condition evaluate to false?

When we're running some tests.

Implements heartbeats for compute nodes, sending heartbeat messages to the requester node over NATS PubSub. The server, upon receiving a heartbeat updates the map of nodes to include the current server-side timestamp. Compute nodes using the heartbeat client, will continuously send heartbeat messages every n seconds. The heartbeat server receiving these heartbeats maintains a priority queue, which dequeues oldest items (lowest timestamp) first. Every 5 seconds any item older than a specific timestamp is dequeued, and its state either set to unhealthy (if it is the first missed heartbeat) or unknown if it is the second. The default for timestamps is * 30s since heartbeat - unhealthy * 60s since heartbeat - unknown (node may be live but disconnected) The next heartbeat sent by a unhealthy of unknown node will make it healthy again and ready to receive work. The current state of the node is added to the nodeinfo during a Get/GetByPrefix/List call to the node info store. This means that the liveness is dynamic and not persisted to the kvstore for node info.

rossjones force-pushed the heartbeats branch from f5d9000 to f2a7d5a Compare March 26, 2024 16:31

rossjones changed the title ~~Implements compute node heartbeats to requester node(s)~~ Node heartbeats Mar 26, 2024

frrist reviewed Mar 26, 2024

View reviewed changes

pkg/compute/management_client.go Outdated Show resolved Hide resolved

pkg/node/heartbeat/client.go Show resolved Hide resolved

pkg/node/heartbeat/server.go Outdated Show resolved Hide resolved

frrist reviewed Mar 27, 2024

View reviewed changes

frrist assigned rossjones Mar 27, 2024

rossjones force-pushed the heartbeats branch 4 times, most recently from 0ec0bfc to 6bf5028 Compare April 3, 2024 09:54

rossjones added 20 commits April 3, 2024 15:55

Implements compute node heartbeats to requester node(s)

ebda95f

Adds pkg/node/heartbeat which contains a client and a server for sending heartbeat messages over NATS PubSub.

Enqueue heartbeats in priority queue for checking

dfa33b1

Enqueues, or re-enqueues heartbeats from nodes as they arrive into a priority queue.

Fix typing in test;

ecb53bf

Remove loop var

2191025

Initial heartbeat at startup

98235b0

Warn not error when compute cannot send message

9bfd366

Add liveness info to node list

066e2f2

Configuration for frequencies/intervals etc

ce7e83a

Configure heartbeat deadlines

f09e590

Document how to add a new nodestate

ea11796

Close heartbeat client when done

6365301

Fix use of old errors.Wrap

bda14a5

Defaults for new state timeout fields

a75b740

Add tests for heartbeat states

4cd989e

Test for initial state/unknown nodes

d93c416

Make tests idempotent

80a74b4

New nodeinfo per test

d4f254b

Add node state to CLI node list

8408aa9

Also enables the use of ordering and filtering of the status field (nodeinfo.State). This means that the approval field is no longer labelled as Status, but as Approval.

Adds documentation for node management and the new node subcommands

0cc06b6

Add NATS to cspell dictionary

e247467

rossjones force-pushed the heartbeats branch from 6f297db to e247467 Compare April 3, 2024 14:55

rossjones requested a review from a team April 3, 2024 14:55

rossjones marked this pull request as ready for review April 3, 2024 15:46

aronchick approved these changes Apr 3, 2024

View reviewed changes

Fix duplicate docs line

63336e1

frrist approved these changes Apr 4, 2024

View reviewed changes

rossjones added 7 commits April 8, 2024 10:30

Mention flag in error message

a96c1b0

Log potential error during server shutdown

0f7ef4d

Change where we log the node manager start message

c9f92a8

Merge branch 'main' into heartbeats

8487070

switch to bi-modal status

a50051b

Merge branch 'main' into heartbeats

e841e6e

Fix heartbeat server + tests

8cc5fe4

rossjones merged commit 6572703 into main Apr 8, 2024
12 checks passed

rossjones deleted the heartbeats branch April 8, 2024 15:05

wdbaruni mentioned this pull request Apr 15, 2024

Show liveness in 'node list' command #2935

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Node heartbeats #3709

Node heartbeats #3709

rossjones commented Mar 26, 2024 •

edited

Loading

coderabbitai bot commented Mar 26, 2024 •

edited

Loading

Auto Review Skipped

Chat

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configration File (`.coderabbit.yaml`)

Documentation and Community

frrist left a comment

frrist left a comment

aronchick left a comment

aronchick Apr 3, 2024

rossjones Apr 3, 2024

aronchick Apr 3, 2024

rossjones Apr 8, 2024

frrist left a comment

frrist Apr 4, 2024 •

edited

Loading

rossjones Apr 4, 2024

rossjones Apr 8, 2024

frrist Apr 4, 2024

frrist Apr 4, 2024

frrist Apr 4, 2024

rossjones Apr 8, 2024

frrist Apr 4, 2024

frrist Apr 4, 2024

frrist Apr 4, 2024

rossjones Apr 8, 2024

frrist Apr 4, 2024

frrist Apr 4, 2024 •

edited

Loading

frrist Apr 4, 2024

rossjones Apr 8, 2024


		# Command: `node approve`

		The `bacalhau node approve` command offers administrations the ability to approve the cluster membership for a node using its unique identifier.


		- `-m message`:

		- A message to be attached to the approval action.

Node heartbeats #3709

Node heartbeats #3709

Conversation

rossjones commented Mar 26, 2024 • edited Loading

coderabbitai bot commented Mar 26, 2024 • edited Loading

Auto Review Skipped

Chat

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configration File (.coderabbit.yaml)

Documentation and Community

frrist left a comment

Choose a reason for hiding this comment

frrist left a comment

Choose a reason for hiding this comment

aronchick 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

frrist left a comment

Choose a reason for hiding this comment

frrist Apr 4, 2024 • 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

frrist Apr 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rossjones commented Mar 26, 2024 •

edited

Loading

coderabbitai bot commented Mar 26, 2024 •

edited

Loading

CodeRabbit Configration File (`.coderabbit.yaml`)

frrist Apr 4, 2024 •

edited

Loading

frrist Apr 4, 2024 •

edited

Loading