fix: remove Liveness and Acceptance from NodeInfo #3785

frrist · 2024-04-11T22:05:12Z

fixes Compute Nodes broadcast NodeInfo with unknown approval status which overrides previous approvals/rejections #3783
Introduces NodeState type used to track NodeInfo, Liveness, and Acceptance. Removes the idea of Livenss and Acceptance from data sent by compute nodes to the Requester(s) since compute nodes should not influence their Liveness or Acceptance. Those are values related to the Requesters view of the network.

coderabbitai · 2024-04-11T22:05:17Z

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.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

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 · 2024-04-11T22:08:00Z

@coderabbitai /review

coderabbitai · 2024-04-11T22:11:48Z

Tip

For best results, initiate chat on the files or code changes.

@frrist, I have reviewed the changes made in the PR as per your request. Here's a summary of the key modifications relevant to the PR's objectives:

Changes in `nodeinfo.go`:

NodeInfo Structure: The NodeInfo struct now includes fields like BacalhauVersion, PeerInfo, NodeType, Labels, and ComputeNodeInfo. This struct is designed to provide comprehensive information about a node, distinguishing between requester and compute nodes.
ComputeNodeInfo Structure: This new struct includes details specific to compute nodes such as execution engines, publishers, storage sources, and capacities. It also tracks the number of running and enqueued executions, which is crucial for managing compute resources.

Changes in `types.go`:

HealthInfo and FreeSpace Structures: These structures are used to report system health and disk space availability. This is part of ongoing efforts to improve monitoring and diagnostics.

Observations:

The PR effectively removes the direct influence of compute nodes on their Liveness and Acceptance by segregating node-specific information into NodeInfo and ComputeNodeInfo.
The introduction of NodeType helps in clearly distinguishing the roles within the network, which aligns with the PR's goal to refine how node roles and statuses are handled.

Recommendations:

Ensure that the removal of Liveness and Acceptance from the direct control of compute nodes is accompanied by adequate checks and balances in the parts of the system where these properties are now handled.
Consider adding more detailed tests, especially integration tests, to ensure that these changes interact correctly with the existing functionalities and that there are no regressions.

If you need further detailed analysis or have specific areas of concern, please let me know!

Analysis chain

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Fetch the changes made in the PR
git fetch origin pull/3785/head:pr-3785
git diff pr-3785

Length of output: 162

Script:

#!/bin/bash
# List files changed in the PR and show their contents
git diff --name-only pr-3785 | xargs git show pr-3785:

Length of output: 690

Script:

#!/bin/bash
# Fetch the contents of files in directories likely to contain the relevant changes
git show pr-3785:pkg/
git show pr-3785:cmd/

Length of output: 491

Script:

#!/bin/bash
# Fetch the contents of specific files within directories that are likely to contain the relevant changes
git show pr-3785:pkg/node/
git show pr-3785:pkg/model/
git show pr-3785:pkg/types/

Length of output: 1133

Script:

#!/bin/bash
# Fetch the contents of the nodeinfo.go file and other relevant files
git show pr-3785:pkg/model/nodeinfo.go
git show pr-3785:pkg/types/types.go

Length of output: 2170

rossjones

Looks great. Definitely an improvement to break into different types rather than jamming more fields into the existing one.

Couple of comments about the word liveness, as it feels a bit weird to use it in CLI output, but it might just be me.

Slight concern about the deletions and whether they affect libp2p usage, as we no longer test with the libp2p transport, it might be worth testing manually (if you haven't already) to ensure it continues to work.

rossjones · 2024-04-12T07:33:52Z

cmd/cli/job/describe.go

@@ -102,7 +102,7 @@ func (o *DescribeOptions) printHeaderData(cmd *cobra.Command, job *models.Job) {
 		{Left: "Name", Right: job.Name},
 		{Left: "Namespace", Right: job.Namespace},
 		{Left: "Type", Right: job.Type},
-		{Left: "State", Right: job.State.StateType},
+		{Left: "Liveness", Right: job.State.StateType},


This is the state of the job, rather than the node, is Liveness the correct word here?

whoops, my editor was a little overzealous with the name change, will revert this.

rossjones · 2024-04-12T07:34:24Z

cmd/cli/job/executions.go

@@ -97,7 +97,7 @@ var (
 		Value:        func(e *models.Execution) string { return strconv.FormatUint(e.Revision, 10) },
 	}
 	executionColumnState = output.TableColumn[*models.Execution]{
-		ColumnConfig: table.ColumnConfig{Name: "State", WidthMax: 10, WidthMaxEnforcer: text.WrapText},
+		ColumnConfig: table.ColumnConfig{Name: "Liveness", WidthMax: 10, WidthMaxEnforcer: text.WrapText},


Same question, is Liveness the word we want to use for the execution?

no, this was a mistake - continuing to blame the enthusiasm of my editor

rossjones · 2024-04-12T07:34:46Z

cmd/cli/job/history.go

@@ -108,7 +108,7 @@ var historyColumns = []output.TableColumn[*models.JobHistory]{
 		},
 	},
 	{
-		ColumnConfig: table.ColumnConfig{Name: "New State", WidthMax: 20, WidthMaxEnforcer: text.WrapText},
+		ColumnConfig: table.ColumnConfig{Name: "New Liveness", WidthMax: 20, WidthMaxEnforcer: text.WrapText},


New Liveness sounds a bit weird.

will revert

rossjones · 2024-04-12T07:35:22Z

pkg/compute/management_client.go

@@ -101,7 +102,7 @@ func (m *ManagementClient) RegisterNode(ctx context.Context) error {
 func (m *ManagementClient) deliverInfo(ctx context.Context) {
 	// We _could_ avoid attempting an update if we are not registered, but
 	// by doing so we will get frequent errors that the node is not
-	// registered.
+	// registered.c


Suggested change

// registered.c

// registered.

rossjones · 2024-04-12T07:41:19Z

pkg/node/manager/node_manager.go

+	if err := n.store.Add(ctx, models.NodeState{
+		Info:     request.Info,
+		Approval: n.defaultApprovalState,
+		// NB(forrest): by virtue of a compute node calling this endpoint we can consider it connected


This should be okay, but using other signals than the heartbeat might lead to flapping, so if that happens, this might be a reasonable place to look.

rossjones · 2024-04-12T07:43:16Z

pkg/node/manager/node_manager.go

+		Info: request.Info,
+		// the nodes approval state is assumed to be approved here, but re-use existing state
+		Approval: existing.Approval,
+		// TODO can we assume the node is connected here?


Probably. It might be best to inject a heartbeat as the liveness is held in memory rather than the store and we probably want to avoid situations where the node is not in the liveness map and we accidentally send Connected rather than the default of Disconnected.

rossjones · 2024-04-12T07:45:46Z

pkg/orchestrator/selection/discovery/chained.go

These aren't still needed for libp2p are they?

frrist · 2024-04-15T14:30:57Z

Based on conversation with @wdbaruni and @seanmtracey we need to evaluate if this change breaks backwards compatibility and if it does handle it gracefully by adjusting marshaling or adjusting the topic we exchange these messages on to include a version

- fixes #3783 - Introduces NodeState type used to track NodeInfo, Liveness, and Acceptance. Removes the idea of Livenss and Acceptance from data sent by compute nodes to the Requester(s) since compute nodes should not influence their Liveness or Acceptance. Those are values related to the Requesters view of the network.

- its not needed and necessary obfuscation

frrist · 2024-04-18T02:01:45Z

This change has been validated with a v1.3.0 Requester communicating with a v1.3.1-rc Compute node and v1.3.0 Compute node as well as a v1.3.1-rc Requester node communicating with a v1.3.1-rc Compute node and v1.3.0 Compute node. Meaning it’s backwards and forwards compatible.

wdbaruni

Few minor comments

wdbaruni · 2024-04-18T05:13:29Z

pkg/models/node_approval.go

+// TODO if we ever pass a pointer to this type and use `==` comparison on it we're gonna have a bad time
+// implement an `Equal()` method for this type and default to it.


this sounds bad. can you open an issue to track this TODO?

wdbaruni · 2024-04-18T05:14:38Z

pkg/models/node_connection.go

+type livenessContainer struct {
+	CONNECTED    NodeConnectionState
+	DISCONNECTED NodeConnectionState
+	HEALTHY      NodeConnectionState


What is HEALTHY?

wdbaruni · 2024-04-18T05:17:10Z

pkg/nats/transport/nats.go

@@ -85,7 +86,7 @@ type NATSTransport struct {
 	natsClient        *nats_helper.ClientManager
 	computeProxy      compute.Endpoint
 	callbackProxy     compute.Callback
-	nodeInfoPubSub    pubsub.PubSub[models.NodeInfo]
+	nodeInfoPubSub    pubsub.PubSub[models.NodeState]


shouldn't PubSub be about NodeInfo and not NodeState?

wdbaruni · 2024-04-18T05:18:29Z

pkg/node/manager/node_manager.go

 	resourceMap          *concurrency.StripedMap[models.Resources]
 	heartbeats           *heartbeat.HeartbeatServer
-	defaultApprovalState models.NodeApproval
+	defaultApprovalState models.NodeMembershipState
 }

 type NodeManagerParams struct {
 	NodeInfo             routing.NodeInfoStore


should be Store

wdbaruni · 2024-04-18T05:26:26Z

pkg/orchestrator/selection/discovery/chained.go

-type Chain struct {
-	discoverers  []orchestrator.NodeDiscoverer
-	ignoreErrors bool
-}


I remember you saying at one point that you are removing all chain implementations and replacing them with simple arrays. While the Chain looks like a simple array, it implements NodeDiscoverer interface which allows different components to just accept the interface rather than an array of interfaces. This simplifies testing, error handling (e.g. ignoreErrors), and can allow chain implementations with AND or OR logic.

wdbaruni · 2024-04-18T05:32:28Z

Ahh .. I didn't see auto-merge was enabled. Anyways, there are few comments that can be addressed in a follow-up PR if the comments are valid

- fixes #3783 - Introduces NodeState type used to track NodeInfo, Connection, and Membershio. Removes the idea of Connection and Membership from data sent by compute nodes to the Requester(s) since compute nodes should not influence their Connection state or mmembership. Those are values related to the Requesters view of the network. --------- Co-authored-by: frrist <forrest@expanso.io>

frrist mentioned this pull request Apr 11, 2024

Generate events when nodes are approved/rejected #3772

Open

frrist self-assigned this Apr 11, 2024

rossjones approved these changes Apr 12, 2024

View reviewed changes

rossjones mentioned this pull request Apr 12, 2024

Stop approval states being overwritten #3782

Closed

frrist added 5 commits April 17, 2024 15:29

refactor: remove chain discovery pattern

df91dbe

- its not needed and necessary obfuscation

fix: failing tests and add helpful comments

9aac750

comments: comment the new types and what they are for

a38f894

fix: labels test needs to adjust to new response

afb1118

frrist force-pushed the frrist/fix/node-info-state branch from 4e640d9 to e9641c3 Compare April 18, 2024 01:00

review: address pr review

2c3a92b

frrist force-pushed the frrist/fix/node-info-state branch from e9641c3 to 2c3a92b Compare April 18, 2024 01:16

frrist enabled auto-merge (squash) April 18, 2024 02:00

wdbaruni approved these changes Apr 18, 2024

View reviewed changes

frrist merged commit 3b3d8a8 into main Apr 18, 2024
12 checks passed

frrist deleted the frrist/fix/node-info-state branch April 18, 2024 05:31

frrist mentioned this pull request May 22, 2024

bacalhau node list returns error failed request: invalid node type: nodeTypeUndefined #4024

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: remove Liveness and Acceptance from NodeInfo #3785

fix: remove Liveness and Acceptance from NodeInfo #3785

frrist commented Apr 11, 2024

coderabbitai bot commented Apr 11, 2024 •

edited

Loading

Auto Review Skipped

Chat

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configration File (`.coderabbit.yaml`)

Documentation and Community

frrist commented Apr 11, 2024

coderabbitai bot commented Apr 11, 2024

rossjones left a comment

rossjones Apr 12, 2024

frrist Apr 12, 2024

rossjones Apr 12, 2024

frrist Apr 12, 2024 •

edited

Loading

rossjones Apr 12, 2024

frrist Apr 12, 2024

rossjones Apr 12, 2024

rossjones Apr 12, 2024

rossjones Apr 12, 2024

rossjones Apr 12, 2024

frrist commented Apr 15, 2024

frrist commented Apr 18, 2024

wdbaruni left a comment

wdbaruni Apr 18, 2024

wdbaruni Apr 18, 2024

wdbaruni Apr 18, 2024

wdbaruni Apr 18, 2024

wdbaruni Apr 18, 2024

wdbaruni commented Apr 18, 2024 •

edited

Loading

		// TODO if we ever pass a pointer to this type and use `==` comparison on it we're gonna have a bad time
		// implement an `Equal()` method for this type and default to it.

fix: remove Liveness and Acceptance from NodeInfo #3785

fix: remove Liveness and Acceptance from NodeInfo #3785

Conversation

frrist commented Apr 11, 2024

coderabbitai bot commented Apr 11, 2024 • edited Loading

Auto Review Skipped

Chat

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configration File (.coderabbit.yaml)

Documentation and Community

frrist commented Apr 11, 2024

coderabbitai bot commented Apr 11, 2024

Changes in nodeinfo.go:

Changes in types.go:

Observations:

Recommendations:

rossjones 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

frrist Apr 12, 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

frrist commented Apr 15, 2024

frrist commented Apr 18, 2024

wdbaruni 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

wdbaruni commented Apr 18, 2024 • edited Loading

coderabbitai bot commented Apr 11, 2024 •

edited

Loading

CodeRabbit Configration File (`.coderabbit.yaml`)

Changes in `nodeinfo.go`:

Changes in `types.go`:

frrist Apr 12, 2024 •

edited

Loading

wdbaruni commented Apr 18, 2024 •

edited

Loading