[RSDK-8344] Expose machine internet connectivity #5253
Conversation
viambot
added
the
safe to test
danielbotros
changed the title
grpc/app_conn.go
Outdated
| // IsConnected checks if the underlying connection is established. | ||
| func (ac *AppConn) IsConnected() bool { |
There was a problem hiding this comment.
Additionally we could add or replace this function with one that returns connectivity.State given that we might want to handle transient failure different from offline
There was a problem hiding this comment.
let's return connectivity.State here. if ac.conn == nil we can return Connecting
viambot
added
safe to test
grpc/app_conn.go
Outdated
| return grpcConn.GetState() | ||
| } | ||
|
|
||
| return connectivity.Shutdown |
There was a problem hiding this comment.
We shouldn't really ever hit this since GetState() is a default method for ClientConn but I wasn't too sure how to handle it. I assume the worst case that the connection is shutdown and unusable, but someone may try to reconnect based off that. I could change it to an optional pattern
There was a problem hiding this comment.
maybe add a const Invalid connectivity.State = 5?
viambot
added
safe to test
grpc/app_conn.go
Outdated
| return grpcConn.GetState() | ||
| } | ||
|
|
||
| return connectivity.Shutdown |
There was a problem hiding this comment.
maybe add a const Invalid connectivity.State = 5?
viambot
added
safe to test
viambot
added
safe to test
viambot
added
safe to test
grpc/conn.go
Outdated
| if c.conn == nil { | ||
| return connectivity.Connecting |
There was a problem hiding this comment.
I handle this case in ReconfigurableClientConn since app conn and shared conn are wrappers around it and I assume we will continue to build ontop of this type
There was a problem hiding this comment.
for ReconfigurableClientConn, I think this would be incorrect because we don't actually know what other structs are doing with ReconfigurableClientConn if conn is nil (they could be purposefully leaving it nil for a while for some other event that we don't know of).
We should return Unknown here but Connecting in AppConn if conn is nil
go.mod
Outdated
| @@ -83,7 +83,7 @@ require ( | |||
| go.uber.org/zap v1.27.0 | |||
| go.viam.com/api v0.1.473 | |||
| go.viam.com/test v1.2.4 | |||
| go.viam.com/utils v0.1.164 | |||
| go.viam.com/utils v0.1.166 | |||
There was a problem hiding this comment.
WIll bump to correct version once goutils changes merged
grpc/conn.go
Outdated
| if c.conn == nil { | ||
| return connectivity.Connecting |
There was a problem hiding this comment.
for ReconfigurableClientConn, I think this would be incorrect because we don't actually know what other structs are doing with ReconfigurableClientConn if conn is nil (they could be purposefully leaving it nil for a while for some other event that we don't know of).
We should return Unknown here but Connecting in AppConn if conn is nil
grpc/shared_conn.go
Outdated
| @@ -359,6 +360,10 @@ func (sc *SharedConn) ProcessEncodedAnswer(encodedAnswer string) error { | |||
| return nil | |||
| } | |||
|
|
|||
| func (sc *SharedConn) GetState() connectivity.State { | |||
| return sc.grpcConn.GetState() | |||
There was a problem hiding this comment.
SharedConn is also a bit awkward because it serves both grpcConn and the peerConn, and there isn't a good way to surface the state of both at the same time. I'd also make a separate ticket and leave this as Unknown for now
There was a problem hiding this comment.
Ah ok thanks for clarifying I didn't think of those considerations for the other two connections
viambot
added
safe to test
viambot
added
safe to test
viambot
added
safe to test
viambot
added
safe to test
viambot
added
safe to test
Description
RSDK-8344 Expose machine internet connectivity
ClientConninterface extended to includeGetStatein this goutils PRGetStatemethod toAppConn,SharedConn, andReconfigurableClientConnas well some mock/test types.Readywhen connected to app without issue)Testing
Main use case is for the data manager. I tested these changes alongside my goutils changes by calling my
GetStatewhere the data manager calls its version ofGetState(inrunSchedulerinsync.go) receiving this log line:2025-09-05T15:52:58.260Z INFO rdk.resource_manager.rdk:service:data_manager/dm.sync sync/sync.go:573 cloud conn state is {"state":"READY"}