RSDK-8765 - Add CloudMetadata to MachineStatus #4637

cheukt · 2024-12-16T23:17:07Z

original PR: #4637

Tested a few scenarios
Local -> remote1, works

if remote1 is running an old version of viam-server, remote1's resources get a blank cloud metadata
if local is not a cloud robot, remote1's resources still get cloud metadata

local -> remote1 -> remote2 also works

tried it with remote1 off (after initial connection), remote1 and remote2's resources exists but get a blank cloud metadata
remote2 with old rdk, remote2's resources get a blank cloud metadata
remote2 off, remote2's resources get a blank cloud metadata

…metadata-to-maichine-status

cheukt · 2024-12-18T17:15:56Z

resource/graph_node.go

-// Status encapsulates a resource name along with state transition metadata.
-type Status struct {
+// NodeStatus encapsulates a resource name along with state transition metadata.
+type NodeStatus struct {
 	Name        Name


could also move Name out of NodeStatus and into resource.Status, but keeping it there for now since I think it would be good to have the name come from the node and not the graph

https://viam.atlassian.net/browse/RSDK-9550 is the ticket, marked it as low since it's low-impact

cheukt · 2024-12-18T17:18:20Z

resource/graph_node.go

 }

-func (w *GraphNode) resourceStatus() Status {
-	var resName Name


the issue here is that test objects are not very flexible and there is not a good way to pass in the correct node name to the resource (especially if going across "remotes")

Could you elaborate on why this won't match the resource name? Is it only in the case of remote resources? Just want an example of a mismatch so I better understand in which cases the original config.Name or current.Name() are incorrect.

mock remote resources in tests - especially how I've opted to not actually spin up real servers, the "remote" resources are real resources and no way to get the names to be accurate.

cheukt · 2024-12-18T17:19:04Z

robot/impl/local_robot.go

-			if closeCtx.Err() != nil {
-				return
-			}
+	if !rOpts.disableBackgroundReconfiguration {


this is not strictly necessary, but added as a convenience to provide better guarantees to our tests

Per my comment below, could we rename this option disableCompleteConfigWorker? The goroutine here isn't fully Reconfigureing, it's only calling completeConfig and updateWeakDependents.

cheukt · 2024-12-18T17:19:17Z

robot/impl/local_robot.go

@@ -1157,7 +1159,7 @@ func (r *localRobot) reconfigure(ctx context.Context, newConfig *config.Config,
 	if newConfig.Cloud != nil {
 		r.Logger().CDebug(ctx, "updating cached config")
 		if err := newConfig.StoreToCache(); err != nil {
-			r.logger.CErrorw(ctx, "error storing the config", "error", err)
+			r.logger.CDebugw(ctx, "error storing the config", "error", err)


Wouldn't this be a real error scenario? Failure to cache the new config to disk?

cheukt · 2024-12-18T17:21:03Z

robot/impl/local_robot.go

+	md, _ := r.CloudMetadata(ctx) //nolint:errcheck
+	for _, resourceStatus := range r.manager.resources.Status() {
+		// if the resource is local, we can use the status as is and attach the cloud metadata of this robot.
+		if !resourceStatus.Name.ContainsRemoteNames() && resourceStatus.Name.API != client.RemoteAPI {


nodes for remote robots also count as remote

cheukt · 2024-12-18T17:25:06Z

robot/impl/resource_manager.go

+
+// defaultRemoteMachineStatusTimeout is the default timeout for getting resource statuses from remotes. This prevents
+// remote cycles from preventing this call from finishing.
+var defaultRemoteMachineStatusTimeout = time.Minute


open to keeping this simpler and not having this timeout as well

This prevents remote cycles

I wouldn't have expected this timeout to be "preventing" remote cycles, but I do think we need timeouts for any remote interactions for when the remote is unexpectedly offline. How does this timeout "prevent" cycles?

this timeout "prevent" cycles by basically adding an upper limit to how long this call can run for - if it is an infinite loop (remote1 points to remote2 and remote2 points to remote1), at least the call will return after a minute

benjirewis

Nice! Some initial comments + qs.

benjirewis · 2024-12-19T21:52:46Z

resource/graph_node.go

 }

-func (w *GraphNode) resourceStatus() Status {
-	var resName Name


Could you elaborate on why this won't match the resource name? Is it only in the case of remote resources? Just want an example of a mismatch so I better understand in which cases the original config.Name or current.Name() are incorrect.

robot/impl/local_robot.go

benjirewis · 2024-12-19T21:58:20Z

robot/impl/local_robot.go

-			if closeCtx.Err() != nil {
-				return
-			}
+	if !rOpts.disableBackgroundReconfiguration {


Per my comment below, could we rename this option disableCompleteConfigWorker? The goroutine here isn't fully Reconfigureing, it's only calling completeConfig and updateWeakDependents.

benjirewis · 2024-12-19T21:58:48Z

robot/impl/local_robot.go

@@ -1157,7 +1159,7 @@ func (r *localRobot) reconfigure(ctx context.Context, newConfig *config.Config,
 	if newConfig.Cloud != nil {
 		r.Logger().CDebug(ctx, "updating cached config")
 		if err := newConfig.StoreToCache(); err != nil {
-			r.logger.CErrorw(ctx, "error storing the config", "error", err)
+			r.logger.CDebugw(ctx, "error storing the config", "error", err)


Wouldn't this be a real error scenario? Failure to cache the new config to disk?

robot/impl/local_robot_test.go

benjirewis · 2024-12-19T22:14:45Z

robot/impl/resource_manager.go

+
+// defaultRemoteMachineStatusTimeout is the default timeout for getting resource statuses from remotes. This prevents
+// remote cycles from preventing this call from finishing.
+var defaultRemoteMachineStatusTimeout = time.Minute


This prevents remote cycles

I wouldn't have expected this timeout to be "preventing" remote cycles, but I do think we need timeouts for any remote interactions for when the remote is unexpectedly offline. How does this timeout "prevent" cycles?

robot/impl/robot_options.go

testutils/resource_utils.go

robot/impl/local_robot_test.go

benjirewis

Generally LGTM, just a couple nits.

benjirewis · 2024-12-23T16:43:26Z

robot/impl/local_robot.go

@@ -287,6 +287,34 @@ func (r *localRobot) sendTriggerConfig(caller string) {
 	}
 }

+func (r *localRobot) completeConfigWorker() {


[nit] I realize you have the comment above the calling site of this method, but a comment above the method describing what it does would also be super helpful for posterity.

benjirewis · 2024-12-23T16:44:12Z

robot/impl/robot_options.go

@@ -20,6 +20,9 @@ type options struct {
 	shutdownCallback func()

 	enableFTDC bool
+
+	// disableCompleteConfigWorker starts the robot without any background processes - should only be used for tests.


Suggested change

// disableCompleteConfigWorker starts the robot without any background processes - should only be used for tests.

// disableCompleteConfigWorker starts the robot without the complete config worker - should only be used for tests.

benjirewis · 2024-12-23T16:44:49Z

robot/impl/robot_options.go

@@ -82,3 +85,10 @@ func WithShutdownCallback(shutdownFunc func()) Option {
 		o.shutdownCallback = shutdownFunc
 	})
 }
+
+// withDisableCompleteConfigWorker returns a Option which disables background reconfiguration.


Suggested change

// withDisableCompleteConfigWorker returns a Option which disables background reconfiguration.

// withDisableCompleteConfigWorker returns an Option which disables the complete config worker.

robot/impl/local_robot_test.go

Kschappacher and others added 24 commits November 19, 2024 16:52

first pass

07a1e6a

Merge remote-tracking branch 'upstream/main' into RSDK-8409-add-cloud…

184adfa

…metadata-to-maichine-status

fix remote name bug

ffa052c

lint and fix tests

379e48a

Merge branch 'main' into cloud-metadata

7047e40

move proto conversion, add test

ca92ca6

fix test

5e3c416

some more updates

2d2663d

Merge branch 'main' into cloud-metadata

0014486

add simple test

757b405

add option

96f3bd4

Merge branch 'main' into cloud-metadata

8847686

some test update

4506964

slight change

234338e

Merge branch 'main' into cloud-metadata

804c587

more

c3bf357

rename node.ResourceStatus to Status

c7dc40a

+1

3034f19

flyby

9d26f97

add helper

5fadd59

add remote tests

db7c921

two remotes test

369a8a8

last test

edfa12e

lint

47ed7b7

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

cheukt mentioned this pull request Dec 16, 2024

Rsdk 8409 add cloudmetadata to maichine status #4569

Closed

cheukt added the appimage-ignore-tests Build AppImage of PR and ignore tests label Dec 17, 2024

fix

8d9a2f8

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 Dec 17, 2024

finish up tests

ac22c89

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 Dec 18, 2024

lint + remove unnecessary helper

ecffe7d

cheukt commented Dec 18, 2024

View reviewed changes

cheukt requested a review from benjirewis December 18, 2024 17:31

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 Dec 18, 2024

cheukt marked this pull request as ready for review December 18, 2024 17:31

Merge branch 'main' into cloud-metadata

c7e0db8

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 Dec 18, 2024

cheukt mentioned this pull request Dec 18, 2024

RSDK-8844 - Expose machine status viamrobotics/viam-typescript-sdk#432

Merged

benjirewis reviewed Dec 19, 2024

View reviewed changes

cheukt added 2 commits December 23, 2024 10:46

Merge branch 'main' into cloud-metadata

1e00b67

pr feedback

ac42a26

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 Dec 23, 2024

cheukt requested a review from benjirewis December 23, 2024 16:28

benjirewis approved these changes Dec 23, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RSDK-8765 - Add CloudMetadata to MachineStatus #4637

RSDK-8765 - Add CloudMetadata to MachineStatus #4637

cheukt commented Dec 16, 2024 •

edited

Loading

cheukt Dec 18, 2024

cheukt Dec 18, 2024

cheukt Dec 18, 2024

benjirewis Dec 19, 2024

cheukt Dec 19, 2024

cheukt Dec 18, 2024

benjirewis Dec 19, 2024

cheukt Dec 18, 2024

benjirewis Dec 19, 2024

cheukt Dec 18, 2024

cheukt Dec 18, 2024

benjirewis Dec 19, 2024

cheukt Dec 19, 2024 •

edited

Loading

benjirewis left a comment

benjirewis Dec 19, 2024

benjirewis Dec 19, 2024

benjirewis Dec 19, 2024

benjirewis Dec 19, 2024

benjirewis left a comment

benjirewis Dec 23, 2024

benjirewis Dec 23, 2024

benjirewis Dec 23, 2024

	// disableCompleteConfigWorker starts the robot without any background processes - should only be used for tests.
	// disableCompleteConfigWorker starts the robot without the complete config worker - should only be used for tests.

	// withDisableCompleteConfigWorker returns a Option which disables background reconfiguration.
	// withDisableCompleteConfigWorker returns an Option which disables the complete config worker.

RSDK-8765 - Add CloudMetadata to MachineStatus #4637

Are you sure you want to change the base?

RSDK-8765 - Add CloudMetadata to MachineStatus #4637

Conversation

cheukt commented Dec 16, 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

cheukt Dec 19, 2024 • edited Loading

Choose a reason for hiding this comment

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

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

cheukt commented Dec 16, 2024 •

edited

Loading

cheukt Dec 19, 2024 •

edited

Loading