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

Grab weak dependencies during reconfigure #2302

Merged
merged 2 commits into from
Apr 28, 2023
Merged

Grab weak dependencies during reconfigure #2302

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
1390ed9
Grab weak dependencies during reconfigure
edaniels Apr 28, 2023
53fa0b6
add test
edaniels Apr 28, 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
98 changes: 69 additions & 29 deletions robot/impl/local_robot.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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(
Expand Down Expand Up @@ -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
Expand All @@ -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)
}
}
Expand Down
147 changes: 134 additions & 13 deletions robot/impl/robot_reconfigure_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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)

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