From 98d78c38fdee44edb60d145ff661c6896a2b7baf Mon Sep 17 00:00:00 2001 From: Maxim Pertsov Date: Tue, 10 Sep 2024 14:34:48 -0400 Subject: [PATCH] RSDK-8701 Show configuring state through MachineStatus endpoint (#4359) --- robot/impl/local_robot.go | 2 - robot/impl/local_robot_test.go | 259 ++++++++++++++++++++------------- testutils/resource_utils.go | 14 ++ 3 files changed, 173 insertions(+), 102 deletions(-) diff --git a/robot/impl/local_robot.go b/robot/impl/local_robot.go index 431aab4b6c2..6530b824e32 100644 --- a/robot/impl/local_robot.go +++ b/robot/impl/local_robot.go @@ -1422,9 +1422,7 @@ func (r *localRobot) Shutdown(ctx context.Context) error { func (r *localRobot) MachineStatus(ctx context.Context) (robot.MachineStatus, error) { var result robot.MachineStatus - r.manager.resourceGraphLock.Lock() result.Resources = append(result.Resources, r.manager.resources.Status()...) - r.manager.resourceGraphLock.Unlock() r.configRevisionMu.RLock() result.Config = r.configRevision diff --git a/robot/impl/local_robot_test.go b/robot/impl/local_robot_test.go index de422e29695..12b97fa907f 100644 --- a/robot/impl/local_robot_test.go +++ b/robot/impl/local_robot_test.go @@ -6,11 +6,13 @@ import ( "crypto/x509" "errors" "fmt" + "log" "math" "os" "path" "path/filepath" "strings" + "sync" "testing" "time" @@ -3329,8 +3331,23 @@ type mockResource struct { } type mockConfig struct { - Value int `json:"value"` - Fail bool `json:"fail"` + Value int `json:"value"` + Fail bool `json:"fail"` + Sleep string `json:"sleep"` +} + +//nolint:unparam // the resource name is currently always "m" but this could easily change +func newMockConfig(name string, val int, fail bool, sleep string) resource.Config { + return resource.Config{ + Name: name, + Model: mockModel, + API: mockAPI, + // We need to specify both `Attributes` and `ConvertedAttributes`. + // The former triggers a reconfiguration and the former is actually + // used to reconfigure the component. + Attributes: rutils.AttributeMap{"value": val, "fail": fail, "sleep": sleep}, + ConvertedAttributes: &mockConfig{Value: val, Fail: fail, Sleep: sleep}, + } } var errMockValidation = errors.New("whoops") @@ -3368,10 +3385,67 @@ func (m *mockResource) Reconfigure( if err != nil { return err } + if mConf.Sleep != "" { + if d, err := time.ParseDuration(mConf.Sleep); err == nil { + log.Printf("sleeping for %s\n", d) + time.Sleep(d) + } + } m.value = mConf.Value return nil } +// getExpectedDefaultStatuses returns a slice of default [resource.Status] with a given +// revision set for motion and sensor services. +func getExpectedDefaultStatuses(revision string) []resource.Status { + return []resource.Status{ + { + 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, + }, + { + Name: resource.Name{ + API: resource.APINamespaceRDK.WithServiceType("motion"), + Name: "builtin", + }, + State: resource.NodeStateReady, + Revision: revision, + }, + { + Name: resource.Name{ + API: resource.APINamespaceRDK.WithServiceType("sensors"), + Name: "builtin", + }, + State: resource.NodeStateReady, + Revision: revision, + }, + } +} + func TestMachineStatus(t *testing.T) { logger := logging.NewTestLogger(t) ctx := context.Background() @@ -3383,93 +3457,32 @@ func TestMachineStatus(t *testing.T) { ) defer resource.Deregister(mockAPI, mockModel) - rev1 := "rev1" - builtinRev := rev1 - - getExpectedDefaultStatuses := func() []resource.Status { - return []resource.Status{ - { - 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, - }, - { - Name: resource.Name{ - API: resource.APINamespaceRDK.WithServiceType("motion"), - Name: "builtin", - }, - State: resource.NodeStateReady, - Revision: builtinRev, - }, - { - Name: resource.Name{ - API: resource.APINamespaceRDK.WithServiceType("sensors"), - Name: "builtin", - }, - State: resource.NodeStateReady, - Revision: builtinRev, - }, - } - } - t.Run("default resources", func(t *testing.T) { + rev1 := "rev1" lr := setupLocalRobot(t, ctx, &config.Config{Revision: rev1}, logger) mStatus, err := lr.MachineStatus(ctx) test.That(t, err, test.ShouldBeNil) test.That(t, mStatus.Config.Revision, test.ShouldEqual, rev1) - expectedStatuses := getExpectedDefaultStatuses() + expectedStatuses := getExpectedDefaultStatuses(rev1) rtestutils.VerifySameResourceStatuses(t, mStatus.Resources, expectedStatuses) }) - t.Run("reconfigure", func(t *testing.T) { - lr := setupLocalRobot(t, ctx, &config.Config{Revision: rev1}, logger) - - expectedConfigError := fmt.Errorf("resource config validation error: %w", errMockValidation) + t.Run("poll after working and failing reconfigures", func(t *testing.T) { + lr := setupLocalRobot(t, ctx, &config.Config{Revision: "rev1"}, logger) // Add a fake resource to the robot. rev2 := "rev2" - builtinRev = rev2 lr.Reconfigure(ctx, &config.Config{ - Revision: rev2, - Components: []resource.Config{ - { - Name: "m", - Model: mockModel, - API: mockAPI, - ConvertedAttributes: &mockConfig{}, - }, - }, + Revision: rev2, + Components: []resource.Config{newMockConfig("m", 0, false, "")}, }) mStatus, err := lr.MachineStatus(ctx) test.That(t, err, test.ShouldBeNil) test.That(t, mStatus.Config.Revision, test.ShouldEqual, rev2) expectedStatuses := rtestutils.ConcatResourceStatuses( - getExpectedDefaultStatuses(), + getExpectedDefaultStatuses(rev2), []resource.Status{ { Name: mockNamed("m"), @@ -3482,27 +3495,17 @@ func TestMachineStatus(t *testing.T) { // Update resource config to cause reconfiguration to fail. rev3 := "rev3" - builtinRev = rev3 lr.Reconfigure(ctx, &config.Config{ - Revision: rev3, - Components: []resource.Config{ - { - Name: "m", - Model: mockModel, - API: mockAPI, - // We need to specify both `Attributes` and `ConvertedAttributes`. - // The former triggers a reconfiguration and the former is actually - // used to reconfigure the component. - Attributes: rutils.AttributeMap{"fail": true}, - ConvertedAttributes: &mockConfig{Fail: true}, - }, - }, + Revision: rev3, + Components: []resource.Config{newMockConfig("m", 0, true, "")}, }) mStatus, err = lr.MachineStatus(ctx) test.That(t, err, test.ShouldBeNil) test.That(t, mStatus.Config.Revision, test.ShouldEqual, rev3) + + expectedConfigError := fmt.Errorf("resource config validation error: %w", errMockValidation) expectedStatuses = rtestutils.ConcatResourceStatuses( - getExpectedDefaultStatuses(), + getExpectedDefaultStatuses(rev3), []resource.Status{ { Name: mockNamed("m"), @@ -3516,27 +3519,15 @@ func TestMachineStatus(t *testing.T) { // Update resource with a working config. rev4 := "rev4" - builtinRev = rev4 lr.Reconfigure(ctx, &config.Config{ - Revision: rev4, - Components: []resource.Config{ - { - Name: "m", - Model: mockModel, - API: mockAPI, - // We need to specify both `Attributes` and `ConvertedAttributes`. - // The former triggers a reconfiguration and the former is actually - // used to reconfigure the component. - Attributes: rutils.AttributeMap{"value": 200}, - ConvertedAttributes: &mockConfig{Value: 200}, - }, - }, + Revision: rev4, + Components: []resource.Config{newMockConfig("m", 200, false, "")}, }) mStatus, err = lr.MachineStatus(ctx) test.That(t, err, test.ShouldBeNil) test.That(t, mStatus.Config.Revision, test.ShouldEqual, rev4) expectedStatuses = rtestutils.ConcatResourceStatuses( - getExpectedDefaultStatuses(), + getExpectedDefaultStatuses(rev4), []resource.Status{ { Name: mockNamed("m"), @@ -3547,4 +3538,72 @@ func TestMachineStatus(t *testing.T) { ) rtestutils.VerifySameResourceStatuses(t, mStatus.Resources, expectedStatuses) }) + + t.Run("poll during reconfiguration", func(t *testing.T) { + rev1 := "rev1" + lr := setupLocalRobot(t, ctx, &config.Config{ + Revision: rev1, + Components: []resource.Config{newMockConfig("m", 200, false, "")}, + }, logger) + + // update resource with a working config that is slow to reconfigure. + rev2 := "rev2" + var wg sync.WaitGroup + wg.Add(1) + go func() { + defer wg.Done() + lr.Reconfigure(ctx, &config.Config{ + Revision: rev2, + Components: []resource.Config{newMockConfig("m", 300, false, "1s")}, + }) + }() + // sleep for a short amount of time to allow the machine to receive a new + // revision. this sleep should be shorter than the resource update duration + // defined above so that updated resource is still in a "configuring" state. + time.Sleep(time.Millisecond * 100) + + // get status while reconfiguring + mStatus, err := lr.MachineStatus(ctx) + test.That(t, err, test.ShouldBeNil) + test.That(t, mStatus.Config.Revision, test.ShouldEqual, rev2) + + // the component whose config changed should be the only component in a + // "configuring" state and associated with the original revision. + filterConfiguring := rtestutils.FilterByStatus(t, mStatus.Resources, resource.NodeStateConfiguring) + expectedConfiguring := []resource.Status{ + { + Name: mockNamed("m"), + State: resource.NodeStateConfiguring, + Revision: rev1, + }, + } + rtestutils.VerifySameResourceStatuses(t, filterConfiguring, expectedConfiguring) + + // all other components should be in the "ready" state and associated with the + // new revision. + filterReady := rtestutils.FilterByStatus(t, mStatus.Resources, resource.NodeStateReady) + expectedReady := getExpectedDefaultStatuses(rev2) + rtestutils.VerifySameResourceStatuses(t, filterReady, expectedReady) + + wg.Wait() + + // get status after reconfigure finishes + mStatus, err = lr.MachineStatus(ctx) + test.That(t, err, test.ShouldBeNil) + test.That(t, mStatus.Config.Revision, test.ShouldEqual, rev2) + + // now all components, including the one whose config changed, should all be in + // the "ready" state and associated with the new revision. + expectedStatuses := rtestutils.ConcatResourceStatuses( + getExpectedDefaultStatuses(rev2), + []resource.Status{ + { + Name: mockNamed("m"), + State: resource.NodeStateReady, + Revision: rev2, + }, + }, + ) + rtestutils.VerifySameResourceStatuses(t, mStatus.Resources, expectedStatuses) + }) } diff --git a/testutils/resource_utils.go b/testutils/resource_utils.go index eb8868ce8ed..9c87f9748e5 100644 --- a/testutils/resource_utils.go +++ b/testutils/resource_utils.go @@ -83,6 +83,20 @@ func VerifySameResourceStatuses(tb testing.TB, actual, expected []resource.Statu test.That(tb, sortedActual, test.ShouldResemble, sortedExpected) } +// FilterByStatus takes a slice of [resource.Status] and a [resource.NodeState] and +// returns a slice of [resource.Status] that are in the given [resource.NodeState]. +func FilterByStatus(tb testing.TB, resourceStatuses []resource.Status, state resource.NodeState) []resource.Status { + tb.Helper() + + var result []resource.Status + for _, rs := range resourceStatuses { + if rs.State == state { + result = append(result, rs) + } + } + return result +} + func newSortedResourceStatuses(resourceStatuses []resource.Status) []resource.Status { sorted := make([]resource.Status, len(resourceStatuses)) copy(sorted, resourceStatuses)