From 1390ed948deace37faa0b6cdb9d984d480d255d3 Mon Sep 17 00:00:00 2001 From: Eric Daniels Date: Fri, 28 Apr 2023 14:01:18 -0400 Subject: [PATCH 1/2] Grab weak dependencies during reconfigure --- robot/impl/local_robot.go | 98 +++++++++++++++++++++++++++------------ 1 file changed, 69 insertions(+), 29 deletions(-) diff --git a/robot/impl/local_robot.go b/robot/impl/local_robot.go index 91b6661f7a8..a1ef3e363ed 100644 --- a/robot/impl/local_robot.go +++ b/robot/impl/local_robot.go @@ -513,7 +513,7 @@ func (r *localRobot) getDependencies( if deps := gNode.UnresolvedDependencies(); len(deps) != 0 { return nil, errors.Errorf("resource has unresolved dependencies: %v", deps) } - deps := make(resource.Dependencies) + allDeps := make(resource.Dependencies) var needUpdate bool for _, dep := range r.manager.resources.GetAllParentsOf(rName) { if node, ok := r.manager.resources.Node(dep); ok { @@ -528,14 +528,75 @@ func (r *localRobot) getDependencies( if err != nil { return nil, &resource.DependencyNotReadyError{Name: dep.Name, Reason: err} } - deps[dep] = r + allDeps[dep] = r + } + nodeConf := gNode.Config() + for weakDepName, weakDepRes := range r.getWeakDependencies(rName, nodeConf.API, nodeConf.Model) { + if _, ok := allDeps[weakDepName]; ok { + continue + } + allDeps[weakDepName] = weakDepRes } if needUpdate { r.updateWeakDependents(ctx) } - return deps, nil + return allDeps, nil +} + +func (r *localRobot) getWeakDependencyNames(api resource.API, model resource.Model) []internal.ResourceMatcher { + reg, ok := resource.LookupRegistration(api, model) + if !ok { + return nil + } + return reg.WeakDependencies +} + +func (r *localRobot) getWeakDependencies(resName resource.Name, api resource.API, model resource.Model) resource.Dependencies { + weakDepNames := r.getWeakDependencyNames(api, model) + + allResources := map[resource.Name]resource.Resource{} + internalResources := map[resource.Name]resource.Resource{} + components := map[resource.Name]resource.Resource{} + for _, n := range r.manager.resources.Names() { + if !(n.API.IsComponent() || n.API.IsService()) { + continue + } + res, err := r.ResourceByName(n) + if err != nil { + if !resource.IsDependencyNotReadyError(err) && !resource.IsNotAvailableError(err) { + r.Logger().Debugw("error finding resource while getting weak dependencies", "resource", n, "error", err) + } + continue + } + switch { + case n.API.IsComponent(): + allResources[n] = res + components[n] = res + default: + allResources[n] = res + if n.API.Type.Namespace == resource.APINamespaceRDKInternal { + internalResources[n] = res + } + } + } + + deps := make(resource.Dependencies, len(weakDepNames)) + for _, dep := range weakDepNames { + switch dep { + case internal.ComponentDependencyWildcardMatcher: + for k, v := range components { + if k == resName { + continue + } + deps[k] = v + } + default: + // no other matchers supported right now. you could imagine a LiteralMatcher in the future + } + } + return deps } func (r *localRobot) newResource( @@ -631,10 +692,6 @@ func (r *localRobot) updateWeakDependents(ctx context.Context) { updateResourceWeakDependents := func(conf resource.Config) { resName := conf.ResourceName() - reg, ok := resource.LookupRegistration(resName.API, conf.Model) - if !ok || len(reg.WeakDependencies) == 0 { - return - } resNode, ok := r.manager.resources.Node(resName) if !ok { return @@ -643,32 +700,15 @@ func (r *localRobot) updateWeakDependents(ctx context.Context) { if err != nil { return } - allDeps := make(resource.Dependencies, len(reg.WeakDependencies)) - for _, dep := range reg.WeakDependencies { - switch dep { - case internal.ComponentDependencyWildcardMatcher: - for k, v := range components { - if k == resName { - continue - } - allDeps[k] = v - } - default: - // no other matchers supported right now. you could imagine a LiteralMatcher in the future - } + if len(r.getWeakDependencyNames(conf.API, conf.Model)) == 0 { + return } - strongDeps, err := r.getDependencies(ctx, resName, resNode) + deps, err := r.getDependencies(ctx, resName, resNode) if err != nil { - r.Logger().Errorw("failed to get strong dependencies during weak update; skipping", "resource", resName, "error", err) + r.Logger().Errorw("failed to get dependencies during weak update; skipping", "resource", resName, "error", err) return } - for name, dep := range strongDeps { - if _, ok := allDeps[name]; ok { - continue - } - allDeps[name] = dep - } - if err := res.Reconfigure(ctx, allDeps, conf); err != nil { + if err := res.Reconfigure(ctx, deps, conf); err != nil { r.Logger().Errorw("failed to reconfigure resource with weak dependencies", "resource", resName, "error", err) } } From 53fa0b6f901a91f58b66a230fdb72ad3983386f8 Mon Sep 17 00:00:00 2001 From: Eric Daniels Date: Fri, 28 Apr 2023 16:51:31 -0400 Subject: [PATCH 2/2] add test --- robot/impl/robot_reconfigure_test.go | 147 ++++++++++++++++++++++++--- 1 file changed, 134 insertions(+), 13 deletions(-) diff --git a/robot/impl/robot_reconfigure_test.go b/robot/impl/robot_reconfigure_test.go index d1bbc742938..2aad5714857 100644 --- a/robot/impl/robot_reconfigure_test.go +++ b/robot/impl/robot_reconfigure_test.go @@ -27,6 +27,7 @@ import ( "go.viam.com/rdk/components/board" "go.viam.com/rdk/components/camera" "go.viam.com/rdk/components/encoder" + "go.viam.com/rdk/components/generic" "go.viam.com/rdk/components/gripper" "go.viam.com/rdk/components/motor" "go.viam.com/rdk/components/movementsensor" @@ -2317,9 +2318,36 @@ func (s *someTypeWithWeakAndStrongDeps) Reconfigure( conf resource.Config, ) error { s.resources = deps + ourConf, err := resource.NativeConfig[*someTypeWithWeakAndStrongDepsConfig](conf) + if err != nil { + return err + } + for _, dep := range ourConf.deps { + if _, err := deps.Lookup(dep); err != nil { + return err + } + } + for _, dep := range ourConf.weakDeps { + if _, err := deps.Lookup(dep); err != nil { + return err + } + } return nil } +type someTypeWithWeakAndStrongDepsConfig struct { + deps []resource.Name + weakDeps []resource.Name +} + +func (s *someTypeWithWeakAndStrongDepsConfig) Validate(_ string) ([]string, error) { + depNames := make([]string, 0, len(s.deps)) + for _, dep := range s.deps { + depNames = append(depNames, dep.String()) + } + return depNames, nil +} + func TestUpdateWeakDependents(t *testing.T) { logger := golog.NewTestLogger(t) @@ -2340,7 +2368,7 @@ func TestUpdateWeakDependents(t *testing.T) { resource.Register( weakAPI, weakModel, - resource.Registration[*someTypeWithWeakAndStrongDeps, resource.NoNativeConfig]{ + resource.Registration[*someTypeWithWeakAndStrongDeps, *someTypeWithWeakAndStrongDepsConfig]{ Constructor: func( ctx context.Context, deps resource.Dependencies, @@ -2375,10 +2403,9 @@ func TestUpdateWeakDependents(t *testing.T) { weakCfg2 := config.Config{ Components: []resource.Config{ { - Name: weak1Name.Name, - API: weakAPI, - Model: weakModel, - DependsOn: []string{base1Name.Name}, + Name: weak1Name.Name, + API: weakAPI, + Model: weakModel, }, { Name: base1Name.Name, @@ -2401,10 +2428,9 @@ func TestUpdateWeakDependents(t *testing.T) { weakCfg3 := config.Config{ Components: []resource.Config{ { - Name: weak1Name.Name, - API: weakAPI, - Model: weakModel, - DependsOn: []string{base1Name.Name}, + Name: weak1Name.Name, + API: weakAPI, + Model: weakModel, }, { Name: base1Name.Name, @@ -2433,10 +2459,9 @@ func TestUpdateWeakDependents(t *testing.T) { weakCfg4 := config.Config{ Components: []resource.Config{ { - Name: weak1Name.Name, - API: weakAPI, - Model: weakModel, - DependsOn: []string{base1Name.Name}, + Name: weak1Name.Name, + API: weakAPI, + Model: weakModel, }, { Name: base1Name.Name, @@ -2454,6 +2479,102 @@ func TestUpdateWeakDependents(t *testing.T) { test.That(t, err, test.ShouldBeNil) test.That(t, weak1.resources, test.ShouldHaveLength, 1) test.That(t, weak1.resources, test.ShouldContainKey, base1Name) + + base2Name := base.Named("base2") + weakCfg5 := config.Config{ + Components: []resource.Config{ + { + Name: weak1Name.Name, + API: weakAPI, + Model: weakModel, + ConvertedAttributes: &someTypeWithWeakAndStrongDepsConfig{ + deps: []resource.Name{generic.Named("foo")}, + }, + }, + { + Name: base1Name.Name, + API: base.API, + Model: fake.Model, + }, + { + Name: base2Name.Name, + API: base.API, + Model: fake.Model, + }, + }, + } + test.That(t, weakCfg5.Ensure(false, logger), test.ShouldBeNil) + robot.Reconfigure(context.Background(), &weakCfg5) + + _, err = robot.ResourceByName(weak1Name) + test.That(t, err, test.ShouldNotBeNil) + test.That(t, err.Error(), test.ShouldContainSubstring, "not initialized") + + weakCfg6 := config.Config{ + Components: []resource.Config{ + { + Name: weak1Name.Name, + API: weakAPI, + Model: weakModel, + ConvertedAttributes: &someTypeWithWeakAndStrongDepsConfig{ + weakDeps: []resource.Name{base1Name}, + }, + }, + { + Name: base1Name.Name, + API: base.API, + Model: fake.Model, + }, + { + Name: base2Name.Name, + API: base.API, + Model: fake.Model, + }, + }, + } + test.That(t, weakCfg6.Ensure(false, logger), test.ShouldBeNil) + robot.Reconfigure(context.Background(), &weakCfg6) + res, err = robot.ResourceByName(weak1Name) + test.That(t, err, test.ShouldBeNil) + weak1, err = resource.AsType[*someTypeWithWeakAndStrongDeps](res) + test.That(t, err, test.ShouldBeNil) + test.That(t, weak1.resources, test.ShouldHaveLength, 2) + test.That(t, weak1.resources, test.ShouldContainKey, base1Name) + test.That(t, weak1.resources, test.ShouldContainKey, base2Name) + + weakCfg7 := config.Config{ + Components: []resource.Config{ + { + Name: weak1Name.Name, + API: weakAPI, + Model: weakModel, + ConvertedAttributes: &someTypeWithWeakAndStrongDepsConfig{ + deps: []resource.Name{base2Name}, + weakDeps: []resource.Name{base1Name}, + }, + }, + { + Name: base1Name.Name, + API: base.API, + Model: fake.Model, + }, + { + Name: base2Name.Name, + API: base.API, + Model: fake.Model, + }, + }, + } + test.That(t, weakCfg7.Ensure(false, logger), test.ShouldBeNil) + robot.Reconfigure(context.Background(), &weakCfg7) + + res, err = robot.ResourceByName(weak1Name) + test.That(t, err, test.ShouldBeNil) + weak1, err = resource.AsType[*someTypeWithWeakAndStrongDeps](res) + test.That(t, err, test.ShouldBeNil) + test.That(t, weak1.resources, test.ShouldHaveLength, 2) + test.That(t, weak1.resources, test.ShouldContainKey, base1Name) + test.That(t, weak1.resources, test.ShouldContainKey, base2Name) } func TestDefaultServiceReconfigure(t *testing.T) {