⚠️ RSDK-7903 Cache machine status in robot client #4399

maximpertsov · 2024-09-27T13:25:45Z

⚠️ hold off on reviewing for now - the following issue needs to be addressed first

https://github.com/viamrobotics/rdk/pull/4399/files#r1783017466

Update robot client to source and cache resources from the MachineStatus API instead of the ResourceNames API.

This change is mostly a no-op for public APIs:

ResourceNames does not change - it will return the exact same resources.
MachineStatus changes slightly - it will no longer include RDK internal resources and RemoteAPIs (e.g. the resource that represents the remote itself). This mirrors the behavior of ResourceNames.

This change is a prerequisite for keeping remote resources in ResourceNames when they get disconnected - after making that change, we will want MachineStatus to show that those disconnected resources are in a non-Ready state, which is why the robot client needs to cache more information than just ResourceNames.

See this thread for details on our strategy break this change up.

maximpertsov · 2024-09-27T16:12:45Z

robot/client/client_session.go

@@ -28,6 +28,7 @@ var exemptFromSession = map[string]bool{
 	"/proto.rpc.webrtc.v1.SignalingService/OptionalWebRTCConfig":     true,
 	"/proto.rpc.v1.AuthService/Authenticate":                         true,
 	"/proto.rpc.v1.ExternalAuthService/AuthenticateTo":               true,
+	"/viam.robot.v1.RobotService/GetMachineStatus":                   true,


since we are now using MachineStatus for inter-robot communication alongside ResourceNames, we need to exempt it from session safety monitoring like we do for ResourceNames

maximpertsov · 2024-09-27T16:15:56Z

robot/client/client_session_test.go

+							ResourceNamesFunc: func() []resource.Name { return injectResources },
+							MachineStatusFunc: func(ctx context.Context) (robot.MachineStatus, error) {
+								return rdktestutils.ResourcesToMachineStatus(injectResources), nil


the bulk of test updates in this PR involve injecting MachineStatus function to test robot clients, since this API replaces ResourceNames as the means of sourcing resources

maximpertsov · 2024-09-27T16:17:28Z

robot/impl/resource_manager.go

+		if name.API == client.RemoteAPI {
+			continue
+		}
+		if name.API.Type.Namespace == resource.APINamespaceRDKInternal {
+			continue
+		}


this is where I make the slight change to the MachineStatus API that I note in the PR description:

MachineStatus changes slightly - it will no longer include RDK internal resources and RemoteAPIs (e.g. the resource that represents the remote itself). This mirrors the behavior of ResourceNames.

maximpertsov · 2024-09-27T16:17:58Z

robot/impl/local_robot_test.go

-		{
-			Name: resource.Name{
-				API:  resource.APINamespaceRDKInternal.WithServiceType("framesystem"),
-				Name: "builtin",
-			},
-			State: resource.NodeStateReady,
-		},
-		{
-			Name: resource.Name{
-				API:  resource.APINamespaceRDKInternal.WithServiceType("cloud_connection"),
-				Name: "builtin",
-			},
-			State: resource.NodeStateReady,
-		},
-		{
-			Name: resource.Name{
-				API:  resource.APINamespaceRDKInternal.WithServiceType("packagemanager"),
-				Name: "builtin",
-			},
-			State: resource.NodeStateReady,
-		},
-		{
-			Name: resource.Name{
-				API:  resource.APINamespaceRDKInternal.WithServiceType("web"),
-				Name: "builtin",
-			},
-			State: resource.NodeStateReady,
-		},


corresponds to https://github.com/viamrobotics/rdk/pull/4399/files#r1778868684

cheukt · 2024-09-27T17:46:28Z

removing myself since I probably won't have time to review, but generally seems good

benjirewis

Pretty much LGTM! Just some questions.

benjirewis · 2024-09-30T18:45:53Z

robot/client/client.go

@@ -71,7 +71,7 @@ type RobotClient struct {
 	dialOptions []rpc.DialOption

 	mu                       sync.RWMutex
-	resourceNames            []resource.Name
+	cachedMachineStatus      robot.MachineStatus


Should this be *robot.MachineStatus to indicate a possible state where we haven't cached anything? Would mean you didn't have to return robot.MachineStatus{} in some of your error cases and could just return nil.

Should this be *robot.MachineStatus to indicate a possible state where we haven't cached anything?

agreed - it would be helpful to at least make the internal cachedMachineStatus field a pointer to better express the case where nothing was cached yet.

however...

Would mean you didn't have to return robot.MachineStatus{} in some of your error cases and could just return nil.

the MachineStatus interface method specifies a struct return value instead of a struct pointer, so that method would still have to return MachineStatus{} values. we can revisit this interface typing decision if folks are strongly against it. I wanted to avoid having to do worry about excessive nil checking on MachineStatus, but depending on how large that structure grows it might be useful to make it a pointer for performance reasons.

make it a pointer for performance reasons

discussed offline with @dgottlieb - the resource.Status objects, whose fields are most likely to expand in the short-term, are in slice and therefore stored on the heap and don't create extra work when copying.

there's no immediate plans to add any fields to the top-level struct - perhaps we can revisit this interface type if/when that changes

Yeah fair; thanks for the response here! Leaving as-is seems fine to me given your points.

cool, as-is for both the internal cachedMachineStatus field as well as the public method?

I don't feel too strongly. I guess I noticed everything was robot.MachineStatus and not *robot.MachineStatus, and felt it to be odd. Given your reasoning for keeping the public method as robot.MachineStatus, I think either value or pointer is fine for the cached value.

robot/impl/resource_manager.go

benjirewis · 2024-09-30T18:49:44Z

robot/client/client_test.go

+		injectRobot.MachineStatusFunc = func(ctx context.Context) (robot.MachineStatus, error) {
+			mu.Lock()
+			defer mu.Unlock()
+			callCountNames++


Curious if you could use a different variable here. Is it right to assume that the ResourceNamesFunc above is not going to get called at all anymore?

ugh - sorry missed this due to large-file folding. yes good call, will ~~make separate counters for each and test accordingly~~ just count machine status and remove the resource names stub altogether

ended up dropping a lot of useless code - thanks for catching!

maximpertsov · 2024-10-01T14:55:26Z

robot/client/client.go

@@ -620,22 +620,29 @@ func (rc *RobotClient) resources(ctx context.Context) ([]resource.Name, []resour
 		defer cancel()
 	}

-	resp, err := rc.client.ResourceNames(ctx, &pb.ResourceNamesRequest{})
+	resp, err := rc.machineStatus(ctx)


heads up @benjirewis @cheukt

spoke IRL with @dgottlieb - we can't move forward with this change, since it's backwards breaking. specifically, this will break comms between two machines if one is running a version has this this endpoint but the other is running a version that does not.

making this transition will require a different strategy to account for this risk, if we still decide to make it.

maximpertsov · 2024-10-01T16:36:00Z

revert this PR to a draft until this issue is resolved: https://github.com/viamrobotics/rdk/pull/4399/files#r1783017466

maximpertsov · 2024-10-07T19:26:33Z

superseded by #4421

cache machine status instead of resource names in robot client

c73a7c5

viambot added the safe to test This pull request is marked safe to test from a trusted zone label Sep 27, 2024

filter out RDKInternal and RemoteAPIs from resource statuses

48ba860

viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Sep 27, 2024

maximpertsov commented Sep 27, 2024

View reviewed changes

maximpertsov mentioned this pull request Sep 27, 2024

⚠️ RSDK-7903 Include unhealthy remotes in resource list with status #4273

Closed

6 tasks

maximpertsov requested review from dgottlieb, benjirewis and cheukt and removed request for dgottlieb, benjirewis and cheukt September 27, 2024 16:22

maximpertsov marked this pull request as ready for review September 27, 2024 16:22

maximpertsov requested review from dgottlieb, benjirewis and cheukt September 27, 2024 16:22

cheukt removed their request for review September 27, 2024 17:46

benjirewis reviewed Sep 30, 2024

View reviewed changes

CR@benjirewis: document resource types excluded from ResourceStatuses

2dadf77

viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Sep 30, 2024

CR@benjirewis: remove all injected ResourceNames funcs

e23e3a0

viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Sep 30, 2024

...

45b7ce6

viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Sep 30, 2024

maximpertsov commented Oct 1, 2024

View reviewed changes

maximpertsov marked this pull request as draft October 1, 2024 16:36

maximpertsov changed the title ~~RSDK-7903 Cache machine status in robot client~~ ⚠️ RSDK-7903 Cache machine status in robot client Oct 1, 2024

maximpertsov closed this Oct 7, 2024

maximpertsov deleted the RSDK-7903-client-uses-machine-status-noop branch October 7, 2024 19:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

⚠️ RSDK-7903 Cache machine status in robot client #4399

⚠️ RSDK-7903 Cache machine status in robot client #4399

maximpertsov commented Sep 27, 2024 •

edited

Loading

maximpertsov Sep 27, 2024

maximpertsov Sep 27, 2024

maximpertsov Sep 27, 2024

maximpertsov Sep 27, 2024

cheukt commented Sep 27, 2024

benjirewis left a comment

benjirewis Sep 30, 2024

maximpertsov Sep 30, 2024

maximpertsov Sep 30, 2024

benjirewis Sep 30, 2024

maximpertsov Sep 30, 2024

benjirewis Sep 30, 2024

benjirewis Sep 30, 2024

maximpertsov Sep 30, 2024 •

edited

Loading

maximpertsov Sep 30, 2024

maximpertsov Oct 1, 2024

maximpertsov commented Oct 1, 2024

maximpertsov commented Oct 7, 2024

⚠️ RSDK-7903 Cache machine status in robot client #4399

⚠️ RSDK-7903 Cache machine status in robot client #4399

Conversation

maximpertsov commented Sep 27, 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

cheukt commented Sep 27, 2024

benjirewis 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maximpertsov Sep 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maximpertsov commented Oct 1, 2024

maximpertsov commented Oct 7, 2024

maximpertsov commented Sep 27, 2024 •

edited

Loading

maximpertsov Sep 30, 2024 •

edited

Loading