Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RSDK-2886 Reduce config ticker and simplify completeConfig goroutine #2291

Merged
merged 2 commits into from
Apr 27, 2023
Merged

RSDK-2886 Reduce config ticker and simplify completeConfig goroutine #2291

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b69d831
RSDK-2886 Reduce config ticker and simplify completeConfig goroutine
benjirewis Apr 27, 2023
042aee0
lower durations of SelectContextOrWait where possible
benjirewis Apr 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 12 additions & 38 deletions robot/impl/local_robot.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
}
Expand Down Expand Up @@ -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),
}
Expand Down Expand Up @@ -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() {
Expand Down
8 changes: 3 additions & 5 deletions robot/impl/resource_manager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
32 changes: 16 additions & 16 deletions robot/impl/robot_reconfigure_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down Expand Up @@ -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"))
Expand Down Expand Up @@ -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)

Expand Down Expand Up @@ -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)

Expand Down Expand Up @@ -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(
Expand All @@ -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(
Expand Down Expand Up @@ -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)

Expand Down Expand Up @@ -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(
Expand All @@ -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(
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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(
Expand Down