RSDK-2886 Reduce config ticker and simplify completeConfig goroutine (v…

…iamrobotics#2291)

robot/impl/local_robot.go

@@ -56,10 +56,9 @@ type localRobot struct {
 	logger                     golog.Logger
 	activeBackgroundWorkers    sync.WaitGroup
 	cancelBackgroundWorkers    func()
-	remotesChanged             chan string
 	closeContext               context.Context
-	triggerConfig              chan bool
-	configTimer                *time.Ticker
+	triggerConfig              chan struct{}
+	configTicker               *time.Ticker
 	revealSensitiveConfigDiffs bool
 
 	lastWeakDependentsRound int64
@@ -134,11 +133,10 @@ func (r *localRobot) Close(ctx context.Context) error {
 		r.webSvc.Stop()
 	}
 	if r.cancelBackgroundWorkers != nil {
-		close(r.remotesChanged)
 		r.cancelBackgroundWorkers()
 		r.cancelBackgroundWorkers = nil
-		if r.configTimer != nil {
-			r.configTimer.Stop()
+		if r.configTicker != nil {
+			r.configTicker.Stop()
 		}
 		close(r.triggerConfig)
 	}
@@ -362,11 +360,10 @@ func newWithResources(
 		),
 		operations:                 operation.NewManager(logger),
 		logger:                     logger,
-		remotesChanged:             make(chan string),
 		closeContext:               closeCtx,
 		cancelBackgroundWorkers:    cancel,
-		triggerConfig:              make(chan bool),
-		configTimer:                nil,
+		triggerConfig:              make(chan struct{}),
+		configTicker:               nil,
 		revealSensitiveConfigDiffs: rOpts.revealSensitiveConfigDiffs,
 		cloudConnSvc:               cloud.NewCloudConnectionService(cfg.Cloud, logger),
 	}
@@ -453,41 +450,18 @@ func newWithResources(
 	}
 
 	r.activeBackgroundWorkers.Add(1)
-	// this goroutine listen for changes in connection status of a remote
+	r.configTicker = time.NewTicker(5 * time.Second)
+	// This goroutine tries to complete the config and update weak dependencies
+	// if any resources are not configured. It executes every 5 seconds or when
+	// manually triggered. Manual triggers are sent when changes in remotes are
+	// detected and in testing.
 	goutils.ManagedGo(func() {
 		for {
-			if closeCtx.Err() != nil {
-				return
-			}
-			select {
-			case <-closeCtx.Done():
-				return
-			case n, ok := <-r.remotesChanged:
-				if !ok {
-					return
-				}
-				if rr, ok := r.manager.RemoteByName(n); ok {
-					rn := fromRemoteNameToRemoteNodeName(n)
-					r.manager.updateRemoteResourceNames(closeCtx, rn, rr)
-					r.updateWeakDependents(ctx)
-				}
-			}
-		}
-	}, r.activeBackgroundWorkers.Done)
-
-	r.activeBackgroundWorkers.Add(1)
-	r.configTimer = time.NewTicker(25 * time.Second)
-	// this goroutine tries to complete the config if any resources are still unconfigured, it execute on a timer or via a channel
-	goutils.ManagedGo(func() {
-		for {
-			if closeCtx.Err() != nil {
-				return
-			}
 			select {
 			case <-closeCtx.Done():
 				return
+			case <-r.configTicker.C:
 			case <-r.triggerConfig:
-			case <-r.configTimer.C:
 			}
 			anyChanges := r.manager.updateRemotesResourceNames(closeCtx)
 			if r.manager.anyResourcesNotConfigured() {

robot/impl/resource_manager.go

@@ -480,14 +480,12 @@ func (manager *resourceManager) completeConfig(
 			}
 			manager.addRemote(ctx, rr, gNode, *remConf)
 			rr.SetParentNotifier(func() {
-				rName := remConf.Name
-				if robot.closeContext.Err() != nil {
-					return
-				}
+				// Trigger completeConfig goroutine execution when a change in remote
+				// is detected.
 				select {
 				case <-robot.closeContext.Done():
 					return
-				case robot.remotesChanged <- rName:
+				case robot.triggerConfig <- struct{}{}:
 				}
 			})
 		default:

robot/impl/robot_reconfigure_test.go

@@ -317,7 +317,7 @@ func TestRobotReconfigure(t *testing.T) {
 		rr, ok := robot.(*localRobot)
 		test.That(t, ok, test.ShouldBeTrue)
 
-		rr.triggerConfig <- true
+		rr.triggerConfig <- struct{}{}
 
 		utils.SelectContextOrWait(context.Background(), 200*time.Millisecond)
 
@@ -1978,7 +1978,7 @@ func TestRobotReconfigure(t *testing.T) {
 		rr, ok := robot.(*localRobot)
 		test.That(t, ok, test.ShouldBeTrue)
 
-		rr.triggerConfig <- true
+		rr.triggerConfig <- struct{}{}
 
 		utils.SelectContextOrWait(context.Background(), 200*time.Millisecond)
 		mock6, err = robot.ResourceByName(mockNamed("mock6"))
@@ -2672,7 +2672,7 @@ func TestRemoteRobotsGold(t *testing.T) {
 	rr, ok := r.(*localRobot)
 	test.That(t, ok, test.ShouldBeTrue)
 
-	rr.triggerConfig <- true
+	rr.triggerConfig <- struct{}{}
 
 	utils.SelectContextOrWait(ctx, 2*time.Second)
 
@@ -2745,12 +2745,12 @@ func TestRemoteRobotsGold(t *testing.T) {
 	err = remote3.StartWeb(ctx, options)
 	test.That(t, err, test.ShouldBeNil)
 
-	utils.SelectContextOrWait(ctx, 26*time.Second)
+	utils.SelectContextOrWait(ctx, 5*time.Second)
 
 	rr, ok = r.(*localRobot)
 	test.That(t, ok, test.ShouldBeTrue)
 
-	rr.triggerConfig <- true
+	rr.triggerConfig <- struct{}{}
 
 	utils.SelectContextOrWait(ctx, 2*time.Second)
 
@@ -2825,7 +2825,7 @@ func TestInferRemoteRobotDependencyConnectAtStartup(t *testing.T) {
 	rr, ok := r.(*localRobot)
 	test.That(t, ok, test.ShouldBeTrue)
 
-	rr.triggerConfig <- true
+	rr.triggerConfig <- struct{}{}
 	utils.SelectContextOrWait(ctx, 2*time.Second)
 
 	expectedSet := rdktestutils.NewResourceNameSet(
@@ -2844,7 +2844,7 @@ func TestInferRemoteRobotDependencyConnectAtStartup(t *testing.T) {
 	test.That(t, foo.Close(context.Background()), test.ShouldBeNil)
 	// wait for local_robot to detect that the remote is now offline
 	utils.SelectContextOrWait(ctx, 15*time.Second)
-	rr.triggerConfig <- true
+	rr.triggerConfig <- struct{}{}
 	utils.SelectContextOrWait(ctx, 2*time.Second)
 
 	test.That(
@@ -2873,12 +2873,12 @@ func TestInferRemoteRobotDependencyConnectAtStartup(t *testing.T) {
 	err = foo2.StartWeb(ctx, options)
 	test.That(t, err, test.ShouldBeNil)
 
-	utils.SelectContextOrWait(ctx, 26*time.Second)
+	utils.SelectContextOrWait(ctx, 5*time.Second)
 
 	rr, ok = r.(*localRobot)
 	test.That(t, ok, test.ShouldBeTrue)
 
-	rr.triggerConfig <- true
+	rr.triggerConfig <- struct{}{}
 
 	utils.SelectContextOrWait(ctx, 2*time.Second)
 
@@ -2948,8 +2948,8 @@ func TestInferRemoteRobotDependencyConnectAfterStartup(t *testing.T) {
 	rr, ok := r.(*localRobot)
 	test.That(t, ok, test.ShouldBeTrue)
 
-	utils.SelectContextOrWait(ctx, 15*time.Second)
-	rr.triggerConfig <- true
+	utils.SelectContextOrWait(ctx, 5*time.Second)
+	rr.triggerConfig <- struct{}{}
 	utils.SelectContextOrWait(ctx, 2*time.Second)
 
 	expectedSet := rdktestutils.NewResourceNameSet(
@@ -2967,8 +2967,8 @@ func TestInferRemoteRobotDependencyConnectAfterStartup(t *testing.T) {
 
 	test.That(t, foo.Close(context.Background()), test.ShouldBeNil)
 	// wait for local_robot to detect that the remote is now offline
-	utils.SelectContextOrWait(ctx, 15*time.Second)
-	rr.triggerConfig <- true
+	utils.SelectContextOrWait(ctx, 5*time.Second)
+	rr.triggerConfig <- struct{}{}
 	utils.SelectContextOrWait(ctx, 2*time.Second)
 
 	test.That(
@@ -3069,7 +3069,7 @@ func TestInferRemoteRobotDependencyAmbiguous(t *testing.T) {
 	rr, ok := r.(*localRobot)
 	test.That(t, ok, test.ShouldBeTrue)
 
-	rr.triggerConfig <- true
+	rr.triggerConfig <- struct{}{}
 	utils.SelectContextOrWait(ctx, 2*time.Second)
 
 	// we expect the robot to correctly detect the ambiguous dependency and not build the resource
@@ -3100,8 +3100,8 @@ func TestInferRemoteRobotDependencyAmbiguous(t *testing.T) {
 		},
 	}
 	r.Reconfigure(ctx, reConfig)
-	utils.SelectContextOrWait(ctx, 15*time.Second)
-	rr.triggerConfig <- true
+	utils.SelectContextOrWait(ctx, 5*time.Second)
+	rr.triggerConfig <- struct{}{}
 	utils.SelectContextOrWait(ctx, 2*time.Second)
 
 	finalSet := rdktestutils.NewResourceNameSet(