Adds NodeInfo endpoint to retriever and dispersal services for observability #630

pschork · 2024-07-09T20:07:10Z

After deploying this change, we can have dataapi (via reachability check) hit this endpoint and log semver per operator.

grpcurl --plaintext localhost:32005 node.Dispersal.VersionInfo
{
  "semver": "0.7.5"
}

grpcurl --plaintext localhost:32005 node.Dispersal.VersionInfo
{
  "semver": "0.8.0-rc.1"
}

Why are these changes needed?

Checks

I've made sure the lint is passing in this PR.
I've made sure the tests are passing. Note that there might be a few flaky tests, in that case, please comment that they are not relevant.
Testing Strategy
- Unit tests
- Integration tests
- This PR is not tested :(

…ervability into node versions ``` grpcurl --plaintext localhost:32005 node.Dispersal.VersionInfo { "semver": "0.7.5" } ``` ``` grpcurl --plaintext localhost:32005 node.Dispersal.VersionInfo { "semver": "0.8.0-rc.1" } ```

ian-shim

lgtm
Does this work in parallel with the existing http endpoint? xxx.xxx.xxx.xxx/eigen/node
What's the advantage over the previous version check?

ian-shim · 2024-07-09T22:09:54Z

api/proto/node/node.proto

@@ -14,13 +14,17 @@ service Dispersal {
 	// for the protocol-defined length of custody. It will return a signature at the
 	// end to attest to the data in this request it has processed.
 	rpc StoreChunks(StoreChunksRequest) returns (StoreChunksReply) {}
+    // Retrieve version into metadata


nit: indent

ian-shim · 2024-07-09T22:10:28Z

node/config.go

@@ -33,7 +33,7 @@ var (
 		0: "eth_quorum",
 		1: "permissioned_quorum",
 	}
-	SemVer    = "v0.0.0"


How is this used today and what difference does this make?

This is a fallback. Only gets triggered if you do a go build without passing -ldflags="-X 'github.com/Layr-Labs/eigenda/node.SemVer=${SEMVER}' so pretty much just our unit tests will see 0.0.0

I'm removed the v to be semver spec compliant. None of our released docker builds use the v prefix.

ian-shim · 2024-07-09T22:11:10Z

node/config.go

@@ -33,7 +33,7 @@ var (
 		0: "eth_quorum",
 		1: "permissioned_quorum",


Not sure how this is used. Do we need to update this to eigen_quorum?

jianoaix · 2024-07-09T23:03:12Z

node/config.go

@@ -33,7 +33,7 @@ var (
 		0: "eth_quorum",
 		1: "permissioned_quorum",


jianoaix · 2024-07-09T23:05:10Z

api/proto/node/node.proto

 }

 service Retrieval {
 	// RetrieveChunks retrieves the chunks for a blob custodied at the Node.
 	rpc RetrieveChunks(RetrieveChunksRequest) returns (RetrieveChunksReply) {}
 	// GetBlobHeader is similar to RetrieveChunks, this just returns the header of the blob.
 	rpc GetBlobHeader(GetBlobHeaderRequest) returns (GetBlobHeaderReply) {}
+    // Retrieve version into metadata


into -> info

api/proto/node/node.proto

pschork · 2024-07-10T00:32:17Z

lgtm Does this work in parallel with the existing http endpoint? xxx.xxx.xxx.xxx/eigen/node What's the advantage over the previous version check?

The advantage is that we have a guarantee that it is reachable because the IP:PORT is registered. The node api runs on port 9093 by default, but operator is free to change that port, and there is no guarantee that port is publicly exposed.

pschork · 2024-07-10T01:44:09Z

Adds node info metadata

grpcurl --plaintext localhost:32005 node.Dispersal.NodeInfo
{
  "semver": "0.8.0-pschork-version-info.1",
  "arch": "arm64",
  "os": "linux",
  "numCpu": 8,
  "memBytes": "8222203904"
}

api/proto/node/node.proto

jianoaix · 2024-07-10T16:59:21Z

lgtm Does this work in parallel with the existing http endpoint? xxx.xxx.xxx.xxx/eigen/node What's the advantage over the previous version check?

The advantage is that we have a guarantee that it is reachable because the IP:PORT is registered. The node api runs on port 9093 by default, but operator is free to change that port, and there is no guarantee that port is publicly exposed.

If we want to rely on this API operationally (not just informationally), it has to have a granrantee.
However I wonder if there is any concern of forcing this API on each operator

node/grpc/server.go

pschork · 2024-07-10T20:30:13Z

However I wonder if there is any concern of forcing this API on each operator

Exposing semver seems reasonable and defensible for purpose of managing breaking migration rollout. Semver is already exposed on node api, though there is no guarantee node api is exposed externally.

System level info is less defensible and operators could raise security/privacy concerns to allowing the enumeration of internal specs.

cc @anupsv

pschork · 2024-07-10T21:43:56Z

Added a flag to disable resource information (os, arch, cpus, mem) from the NodeInfo API.
This will allow operator to OPT-OUT of exposing resource info.
Semver is always exposed.

# NODE_DISABLE_NODE_INFO_RESOURCES=true
grpcurl --plaintext localhost:32005 node.Dispersal.NodeInfo
{
  "semver": "0.8.0-pschork-version-info.1"
}

…mem) on the NodeInfo endpoint

pschork · 2024-07-10T21:58:08Z

Added a flag to disable resource information (os, arch, cpus, mem) from the NodeInfo API. This will allow operator to OPT-OUT of exposing resource info. Semver is always exposed.
# NODE_DISABLE_NODE_INFO_RESOURCES=true
grpcurl --plaintext localhost:32005 node.Dispersal.NodeInfo
{
  "semver": "0.8.0-pschork-version-info.1"
}

If we want to be less aggressive, perhaps this should be OPT-IN. @jianoaix @ian-shim @anupsv @nivethapu

ian-shim

I think opt out is totally reasonable

pschork requested review from jianoaix and ian-shim July 9, 2024 20:07

pschork force-pushed the pschork/version_info branch from 6e4b456 to ad343e7 Compare July 9, 2024 20:13

pschork marked this pull request as ready for review July 9, 2024 20:19

ian-shim reviewed Jul 9, 2024

View reviewed changes

jianoaix reviewed Jul 9, 2024

View reviewed changes

Lint

c08bbf8

Add node system info

fb9314f

pschork force-pushed the pschork/version_info branch from ba014f7 to fb9314f Compare July 10, 2024 01:43

jianoaix approved these changes Jul 10, 2024

View reviewed changes

api/proto/node/node.proto Show resolved Hide resolved

ian-shim reviewed Jul 10, 2024

View reviewed changes

node/grpc/server.go Outdated Show resolved Hide resolved

pschork added 2 commits July 10, 2024 13:36

Lint

6d73a4a

Add automaxprocs to correctly determine CPU resources in container

b23166c

Allow operator to disable host resource information (os, arch, cpus, …

44695ea

…mem) on the NodeInfo endpoint

pschork force-pushed the pschork/version_info branch from 6e6f421 to 44695ea Compare July 10, 2024 21:47

jianoaix approved these changes Jul 10, 2024

View reviewed changes

ian-shim approved these changes Jul 10, 2024

View reviewed changes

Lint

afd5cd2

pschork changed the title ~~Adds VersionInfo endpoint to retriever and dispersal services for observability~~ Adds NodeInfo endpoint to retriever and dispersal services for observability Jul 10, 2024

Lint

7360628

pschork merged commit c2a0a33 into master Jul 10, 2024
9 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds NodeInfo endpoint to retriever and dispersal services for observability #630

Adds NodeInfo endpoint to retriever and dispersal services for observability #630

pschork commented Jul 9, 2024

ian-shim left a comment

ian-shim Jul 9, 2024

ian-shim Jul 9, 2024

pschork Jul 10, 2024 •

edited

Loading

ian-shim Jul 9, 2024

jianoaix Jul 9, 2024

jianoaix Jul 9, 2024

jianoaix Jul 9, 2024

pschork commented Jul 10, 2024

pschork commented Jul 10, 2024

jianoaix commented Jul 10, 2024 •

edited

Loading

pschork commented Jul 10, 2024 •

edited

Loading

pschork commented Jul 10, 2024 •

edited

Loading

pschork commented Jul 10, 2024

ian-shim left a comment

		@@ -33,7 +33,7 @@ var (
		0: "eth_quorum",
		1: "permissioned_quorum",

Adds NodeInfo endpoint to retriever and dispersal services for observability #630

Adds NodeInfo endpoint to retriever and dispersal services for observability #630

Conversation

pschork commented Jul 9, 2024

Why are these changes needed?

Checks

ian-shim left a comment

Choose a reason for hiding this comment

ian-shim Jul 9, 2024

Choose a reason for hiding this comment

ian-shim Jul 9, 2024

Choose a reason for hiding this comment

pschork Jul 10, 2024 • edited Loading

Choose a reason for hiding this comment

ian-shim Jul 9, 2024

Choose a reason for hiding this comment

jianoaix Jul 9, 2024

Choose a reason for hiding this comment

jianoaix Jul 9, 2024

Choose a reason for hiding this comment

jianoaix Jul 9, 2024

Choose a reason for hiding this comment

pschork commented Jul 10, 2024

pschork commented Jul 10, 2024

jianoaix commented Jul 10, 2024 • edited Loading

pschork commented Jul 10, 2024 • edited Loading

pschork commented Jul 10, 2024 • edited Loading

pschork commented Jul 10, 2024

ian-shim left a comment

Choose a reason for hiding this comment

pschork Jul 10, 2024 •

edited

Loading

jianoaix commented Jul 10, 2024 •

edited

Loading

pschork commented Jul 10, 2024 •

edited

Loading

pschork commented Jul 10, 2024 •

edited

Loading