RSDK-7903 Keep unhealthy remotes in ResourceNames #4421

maximpertsov · 2024-10-04T19:59:54Z

Changes

Keep offline remote resources in ResourceNames
Add an internal unreachable field to graph_nodes, so that existing tests have a way to assert on the subset of resources are still online

maximpertsov · 2024-10-04T20:08:17Z

robot/impl/resource_manager.go

@@ -182,9 +182,31 @@ func (manager *resourceManager) updateRemoteResourceNames(
 	rr internalRemoteRobot,
 	recreateAllClients bool,
 ) bool {
-	manager.logger.CDebugw(ctx, "updating remote resource names", "remote", remoteName, "recreateAllClients", recreateAllClients)
+	logger := manager.logger.WithFields("remote", remoteName)


flyby: we have a lot of logs in this function that all include the remote name as metadata - this local logger formalized this

maximpertsov · 2024-10-04T20:09:29Z

robot/impl/resource_manager.go

@@ -194,15 +216,14 @@ func (manager *resourceManager) updateRemoteResourceNames(

 	for _, resName := range newResources {
 		remoteResName := resName
+		resLogger := logger.WithFields("resource", remoteResName)


flyby: similar to the flyby above - all logs local to this loop should include in the resource name in the current loop variable.

maximpertsov · 2024-10-04T20:10:08Z

robot/impl/resource_manager.go

 	}

 	for resName, isActive := range activeResourceNames {
 		if isActive {
 			continue
 		}
-		manager.logger.CDebugw(ctx, "attempting to remove remote resource", "name", resName)
+		resLogger := logger.WithFields("resource", resName)


flyby: exactly the same as this flyby

maximpertsov · 2024-10-04T20:13:02Z

robot/impl/resource_manager.go

+func (manager *resourceManager) includeResourceName(k resource.Name) bool {
+	if k.API == client.RemoteAPI ||
+		k.API.Type.Namespace == resource.APINamespaceRDKInternal {
+		return false
+	}
+	gNode, ok := manager.resources.Node(k)
+	if !ok || !gNode.HasResource() {
+		return false
+	}
+	return true
+}


factored out to ensure ResourceNames and reachableResourceNames filter for the same resources. another approach instead of having multiple functions is to allow ResourceNames to receive an optional parameter that configures what kinds of resources are filtered (i think @dgottlieb suggested a version of this approach)

I don't think I suggested that. The ResourceNamesRequest proto object can take new optional parameters, but the RobotClient.ResourceNames() API cannot without being compile-time backwards breaking.

Ah sorry I got mixed up, you mentioned this for the proto not the method! My mistake - I meant to give credit not incept :)

maximpertsov · 2024-10-04T20:14:37Z

robot/impl/resource_manager_test.go

+// want to ensure that once calling `ResourceNames` on the main part returns some remote resource --
+// that remote resource will always be returned until it is configured away. When either the remote
+// robot removes it from its config. Or when the main part removes the remote.
+func TestOfflineRemoteResources(t *testing.T) {


full credit to @dgottlieb for the test - I ported it from here

dgottlieb

I'm good with the logic as-is so long as we document what "include resource names" is all about.

dgottlieb · 2024-10-07T12:46:33Z

resource/resource_graph.go

+	return g.subGraphFrom(node)
+}
+
+func (g *Graph) subGraphFrom(node Name) (*Graph, error) {


This makes sense to me, but let's call the function subGraphFromWithMutex. And document the caller must be holding the mu mutex that protects the nodes map.

to confirm, did you mean to suggest renaming the public function to SubGraphFromWithMutex, or renaming this private function to subGraphFromWith*out*Mutex (asterisks for emphasis)

I meant naming the private one to subGraphWithMutex. Which I suppose sounds funny on the function side, but looks proper when calling it:

func (g *Graph) SubGraphFrom(node Name) (*Graph, error) { g.mu.Lock() defer g.mu.Unlock() return g.subGraphFromWithMutex(node) }

hmm, for me that's a bit misleading since I would expect the function name to communicate what it is doing - this name assumes that is being called with a mutex held upstream.

Given functions (by default) always take the locks they require, the confusion isn't registering on my side. I'm not going to hold the PR up over the naming. But there needs to be documentation that the caller must ~~not~~ be holding the graph mutex.

yeah i was going to say i’m flexible on naming also as long as there is a doc-string that states the behavior. will add that before merging

i've warmed up to your version: b380627

dgottlieb · 2024-10-07T13:49:08Z

robot/impl/resource_manager.go

 	}
 	return names
 }

+func (manager *resourceManager) includeResourceName(k resource.Name) bool {


I see why you factored this out, but the name isn't good. This isn't looking at reachability, so it's not the whole story on whether to "include the resource name". We should document this with exactly what it does and why it does it.

Or because the tests are already using reachableResourceNames under the hood, we can remove this and change the test helper:

func verifyReachableResourceNames(tb testing.TB, r robot.LocalRobot, expected []resource.Name) { tb.Helper() lRobot, ok := r.(*localRobot) test.That(tb, ok, test.ShouldBeTrue) reachable := make([]resource.Name, 0) for _, resName := range lRobot.ResourceNames() { if node, exists := lRobot.manager.graph.Node(resName); exists && node.IsReachable() { reachable = append(reachable, resName) } } testutils.VerifySameResourceNames(tb, reachable, expected) }

But that would also require adding the IsReachable API method and fixing mutexes on MarkReachability.

My PR initially included an IsReachable method but the result was kind of awkward and deadlock-y. Probably because I needed to do your second suggestion of fixing mutexes.

I'd prefer to just change the name to some better if that's okay - I agree with you that include is crappy and incomplete. So really this is just a utility function that is meant to apply the same filters that ResourceNames applies. So perhaps a lowercase resourceNames with a doc-string specifying what exactly is included/excluded is the way to go?

done 2d2fbba

dgottlieb · 2024-10-07T13:53:00Z

robot/impl/utils.go

+	test.That(tb, ok, test.ShouldBeTrue)
+
+	reachable := lRobot.manager.reachableResourceNames()
+	testutils.VerifySameResourceNames(tb, reachable, expected)


Nice compromise to keep existing tests working.

dgottlieb · 2024-10-07T14:01:27Z

resource/resource_graph.go

+		return err
+	}
+	for _, node := range subGraph.nodes {
+		node.unreachable = !reachable


We don't need to take the node mutex because the graph is locked? I think that's risky.

yeah, that's true - I'll add a private helper to set node reachability with a mutex

done 324ce54

dgottlieb · 2024-10-07T14:04:12Z

robot/impl/resource_manager.go

+		return false
+	}
+	gNode, ok := manager.resources.Node(k)
+	if !ok || !gNode.HasResource() {


I was always confused why a node wouldn't have a resource. Is this the unconfigured case? Can we document it here (in addition to the function documentation).

Is this the unconfigured case

I don't think !HasResource() and Unconfigured are exactly 1-1, or at least I haven't been able to prove that to myself yet. I would certainly like them to be. Lemme do a quick review of when this happens exactly and see if I can document this a bit better.

Yeah there is some inconsistency. A glaring example is found in the [GraphNode.Close] method - there we nil-ify node resource but do not change the node state. There's a follow-up ticket to clean this and potential other state inconsistencies up, so let me take care of that as a separate change if that's okay.

Yeah, definitely don't do anything about it here. Was just hoping to gain clarity on whether that clause is needed and for what reason.

dgottlieb · 2024-10-07T14:05:41Z

robot/impl/resource_manager.go

+func (manager *resourceManager) includeResourceName(k resource.Name) bool {
+	if k.API == client.RemoteAPI ||
+		k.API.Type.Namespace == resource.APINamespaceRDKInternal {
+		return false
+	}
+	gNode, ok := manager.resources.Node(k)
+	if !ok || !gNode.HasResource() {
+		return false
+	}
+	return true
+}


I don't think I suggested that. The ResourceNamesRequest proto object can take new optional parameters, but the RobotClient.ResourceNames() API cannot without being compile-time backwards breaking.

dgottlieb · 2024-10-07T18:56:47Z

Those docstrings are excellent

maximpertsov added 8 commits October 3, 2024 16:03

keep disconnected resources in ResourceNames

f3d90c6

@dgottlieb's offline test

b11fbb6

ported from PR: viamrobotics#4268

remove flyby MachineStatus change

47caf81

update tests

e602b9f

TODO

8529441

set back to reachable

5471a72

flyby: log with

336277f

flyby: with resource logger

2eb65d3

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

maximpertsov commented Oct 4, 2024

View reviewed changes

lint

7d154ec

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 Oct 4, 2024

maximpertsov marked this pull request as ready for review October 4, 2024 20:58

maximpertsov requested a review from a team October 4, 2024 20:58

dgottlieb approved these changes Oct 7, 2024

View reviewed changes

maximpertsov added 2 commits October 7, 2024 13:23

CR@dgottlieb: includeResources -> resourceNames + docs

2d2fbba

CR@dgottlieb: hold mutex on node when setting reachability

324ce54

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 Oct 7, 2024

CR@dgottlieb: rename to WithMutex + doc

b380627

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 Oct 7, 2024

maximpertsov force-pushed the RSDK-7903-keep-unhealthy-remotes-minimal branch from ddf426f to b380627 Compare October 7, 2024 17:56

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 Oct 7, 2024

maximpertsov requested a review from dgottlieb October 7, 2024 18:03

dgottlieb approved these changes Oct 7, 2024

View reviewed changes

maximpertsov merged commit 216563f into viamrobotics:main Oct 7, 2024
29 of 37 checks passed

maximpertsov deleted the RSDK-7903-keep-unhealthy-remotes-minimal branch October 7, 2024 19:03

This was referenced Oct 7, 2024

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

Closed

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

Closed

maximpertsov mentioned this pull request Oct 25, 2024

RSDK-8966 - Fix bug preventing removal of offline remotes #4454

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RSDK-7903 Keep unhealthy remotes in ResourceNames #4421

RSDK-7903 Keep unhealthy remotes in ResourceNames #4421

maximpertsov commented Oct 4, 2024 •

edited

Loading

maximpertsov Oct 4, 2024

maximpertsov Oct 4, 2024

maximpertsov Oct 4, 2024

maximpertsov Oct 4, 2024

dgottlieb Oct 7, 2024

maximpertsov Oct 7, 2024

maximpertsov Oct 4, 2024

dgottlieb left a comment

dgottlieb Oct 7, 2024

maximpertsov Oct 7, 2024

dgottlieb Oct 7, 2024

maximpertsov Oct 7, 2024

dgottlieb Oct 7, 2024 •

edited

Loading

maximpertsov Oct 7, 2024

maximpertsov Oct 7, 2024

dgottlieb Oct 7, 2024

maximpertsov Oct 7, 2024

dgottlieb Oct 7, 2024

maximpertsov Oct 7, 2024

dgottlieb Oct 7, 2024

dgottlieb Oct 7, 2024

maximpertsov Oct 7, 2024

maximpertsov Oct 7, 2024

dgottlieb Oct 7, 2024

maximpertsov Oct 7, 2024

maximpertsov Oct 7, 2024

dgottlieb Oct 7, 2024

dgottlieb Oct 7, 2024

dgottlieb commented Oct 7, 2024

RSDK-7903 Keep unhealthy remotes in ResourceNames #4421

RSDK-7903 Keep unhealthy remotes in ResourceNames #4421

Conversation

maximpertsov commented Oct 4, 2024 • edited Loading

Changes

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

dgottlieb 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

dgottlieb Oct 7, 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

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

dgottlieb commented Oct 7, 2024

maximpertsov commented Oct 4, 2024 •

edited

Loading

dgottlieb Oct 7, 2024 •

edited

Loading