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.

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-474 Save resource error when not started #1514

Merged
merged 7 commits into from
Oct 24, 2022
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
17 changes: 12 additions & 5 deletions components/arm/fake/fake.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,14 +31,15 @@ var fakeModelJSON []byte

// AttrConfig is used for converting config attributes.
type AttrConfig struct {
FailNew bool `json:"fail_new"`
ArmModel string `json:"arm-model"`
FailNew bool `json:"fail_new"`
FailValidate bool `json:"fail_validate"`
ArmModel string `json:"arm-model"`
}

// Validate ensures all parts of the config are valid.
func (config *AttrConfig) Validate(path string) error {
if config.FailNew {
return errors.New("whoops")
if config.FailValidate {
return errors.New("whoops! failed to validate")
}
return nil
}
Expand Down Expand Up @@ -66,7 +67,13 @@ func NewArm(cfg config.Component, logger golog.Logger) (arm.LocalArm, error) {
var model referenceframe.Model
var err error
if cfg.ConvertedAttributes != nil {
switch cfg.ConvertedAttributes.(*AttrConfig).ArmModel {
converted := cfg.ConvertedAttributes.(*AttrConfig)

if converted.FailNew {
return nil, errors.New("whoops! failed to start up")
}

switch converted.ArmModel {
case xarm.ModelName6DOF:
model, err = xarm.Model(cfg.Name, 6)
case xarm.ModelName7DOF:
Expand Down
7 changes: 3 additions & 4 deletions robot/impl/local_robot.go
Original file line number Diff line number Diff line change
Expand Up @@ -494,7 +494,7 @@ func (r *localRobot) newService(ctx context.Context, config config.Service) (int
// If service model/type not found then print list of valid models they can choose from
if f == nil {
validModels := registry.FindValidServiceModels(rName)
return nil, errors.Errorf("unknown service subtype: %s and/or model: %s use one of the following valid models: %s",
return nil, errors.Errorf("unknown service type: %s and/or model: %s use one of the following valid models: %s",
rName.Subtype, config.Model, strings.Join(validModels, ", "))
}

Expand Down Expand Up @@ -537,7 +537,7 @@ func (r *localRobot) newResource(ctx context.Context, config config.Component) (
rName := config.ResourceName()
f := registry.ComponentLookup(rName.Subtype, config.Model)
if f == nil {
return nil, errors.Errorf("unknown component subtype: %s and/or model: %s", rName.Subtype, config.Model)
return nil, errors.Errorf("unknown component type: %s and/or model: %s", rName.Subtype, config.Model)
}

deps, err := r.getDependencies(rName)
Expand All @@ -554,7 +554,7 @@ func (r *localRobot) newResource(ctx context.Context, config config.Component) (
}

if err != nil {
return nil, errors.Errorf("error building resource %s/%s/%s: %s", config.Model, rName.Subtype, config.Name, err)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really want to remove all the identifying descriptors here? I can imagine a case where these errors may be hard to debug especially if you have two components with same subtype and model.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ohh cool, Makes sense!

return nil, err
}

c := registry.ResourceSubtypeLookup(rName.Subtype)
Expand All @@ -571,7 +571,6 @@ func (r *localRobot) newResource(ctx context.Context, config config.Component) (
func (r *localRobot) updateDefaultServices(ctx context.Context) {
resources := map[resource.Name]interface{}{}
for _, n := range r.ResourceNames() {
// TODO(RSDK-333) if not found, could mean a name clash or a remote service
res, err := r.ResourceByName(n)
if err != nil {
r.Logger().Debugw("not found while grabbing all resources during default svc refresh", "resource", res, "error", err)
Expand Down
11 changes: 10 additions & 1 deletion robot/impl/local_robot_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import (
"github.com/pkg/errors"
"go.mongodb.org/mongo-driver/bson/primitive"
"go.uber.org/zap"

// registers all components.
commonpb "go.viam.com/api/common/v1"
armpb "go.viam.com/api/component/arm/v1"
Expand Down Expand Up @@ -1443,7 +1444,15 @@ func TestResourceStartsOnReconfigure(t *testing.T) {
test.That(t, r, test.ShouldNotBeNil)

noBase, err := r.ResourceByName(base.Named("fake0"))
test.That(t, err, test.ShouldBeError, rutils.NewResourceNotFoundError(base.Named("fake0")))
test.That(
t,
err,
test.ShouldBeError,
rutils.NewResourceGetError(
base.Named("fake0"),
errors.New("component build error: unknown component type: rdk:component:base and/or model: random"),
),
)
test.That(t, noBase, test.ShouldBeNil)

noSvc, err := r.ResourceByName(datamanager.Named("fake1"))
Expand Down
30 changes: 22 additions & 8 deletions robot/impl/resource_manager.go
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ type resourceManager struct {
type resourcePlaceholder struct {
real interface{}
config interface{}
err error
}

type resourceManagerOptions struct {
Expand Down Expand Up @@ -364,7 +365,8 @@ func (manager *resourceManager) completeConfig(
if c, ok := wrap.config.(config.Component); ok {
iface, err := manager.processComponent(ctx, r, c, wrap.real, robot)
if err != nil {
manager.logger.Errorw("error building component", "error", err)
manager.logger.Errorw("error building component", "resource", c.ResourceName(), "model", c.Model, "error", err)
wrap.err = errors.Wrap(err, "component build error")
continue
}
manager.resources.AddNode(r, iface)
Expand All @@ -375,7 +377,8 @@ func (manager *resourceManager) completeConfig(
} else if rc, ok := wrap.config.(config.Remote); ok {
rr, err := manager.processRemote(ctx, rc)
if err != nil {
manager.logger.Errorw("error connecting to remote", "error", err)
manager.logger.Errorw("error connecting to remote", "remote", rc.Name, "error", err)
wrap.err = errors.Wrap(err, "remote connection error")
continue
}
manager.addRemote(ctx, rr, rc, robot)
Expand Down Expand Up @@ -406,11 +409,14 @@ func (manager *resourceManager) completeConfig(
}
s, ok := wrap.config.(config.Service)
if !ok {
manager.logger.Errorw("service config is not a service config", "resource", r)
err := errors.New("service config is not a service config")
manager.logger.Errorw(err.Error(), "resource", r)
wrap.err = errors.Wrap(err, "service build error")
}
iface, err := manager.processService(ctx, s, wrap.real, robot)
if err != nil {
manager.logger.Errorw("error building service", "error", err)
manager.logger.Errorw("error building service", "resource", s.ResourceName(), "model", s.Model, "error", err)
wrap.err = errors.Wrap(err, "service build error")
continue
}
manager.resources.AddNode(r, iface)
Expand Down Expand Up @@ -501,7 +507,7 @@ func (manager *resourceManager) processRemote(ctx context.Context,
err = errors.New("must use Config.AllowInsecureCreds to connect to a non-TLS secured robot")
}
}
return nil, errors.Wrapf(err, "couldn't connect to robot remote (%s)", config.Address)
return nil, errors.Errorf("couldn't connect to robot remote (%s): %s", config.Address, err)
}
manager.logger.Debugw("connected now to remote", "remote", config.Name)
return robotClient, nil
Expand All @@ -514,7 +520,11 @@ func (manager *resourceManager) RemoteByName(name string) (robot.Robot, bool) {
if iface, ok := manager.resources.Node(rName); ok {
part, ok := iface.(robot.Robot)
if !ok {
manager.logger.Errorw("tried to access remote but its not a robot interface", "remote_name", name, "type", iface)
if ph, ok := iface.(*resourcePlaceholder); ok {
manager.logger.Errorw("failed to get remote", "remote", name, "err", ph.err)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: remote not available

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

} else {
manager.logger.Errorw("tried to access remote but its not a robot interface", "remote", name, "type", iface)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are we going to get the relevant Type info without using %T?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it doesn't - updated

}
}
return part, ok
}
Expand Down Expand Up @@ -564,6 +574,7 @@ func (manager *resourceManager) markChildrenForUpdate(ctx context.Context, rName
wrapper := &resourcePlaceholder{
real: nil,
config: originalConfig,
err: errors.New("resource marked for update"),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: resource not updated yet

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

}
manager.resources.AddNode(x, wrapper)
}
Expand Down Expand Up @@ -630,6 +641,7 @@ func (manager *resourceManager) wrapResource(name resource.Name, config interfac
wrapper = &resourcePlaceholder{
real: part,
config: config,
err: errors.New("resource being initialized"),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: resource not initialized yet

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

}
}
// the first thing we need to do is seek if the resource name already exists as an unknownType, if so
Expand Down Expand Up @@ -685,7 +697,7 @@ func (manager *resourceManager) wrapResource(name resource.Name, config interfac
}

// updateResourceGraph using the difference between current config
// and next we create resource wrappers to be consumed ny completeConfig later on
// and next we create resource wrappers to be consumed by completeConfig later on
// Ideally at the end of this function we should have a complete graph representation of the configuration.
func (manager *resourceManager) updateResources(
ctx context.Context,
Expand Down Expand Up @@ -752,9 +764,11 @@ func (manager *resourceManager) updateResources(
func (manager *resourceManager) ResourceByName(name resource.Name) (interface{}, error) {
robotPart, ok := manager.resources.Node(name)
if ok && robotPart != nil {
if _, ok = robotPart.(*resourcePlaceholder); !ok {
ph, ok := robotPart.(*resourcePlaceholder)
if !ok {
return robotPart, nil
}
return nil, rutils.NewResourceGetError(name, ph.err)
}
// if we haven't found a resource of this name then we are going to look into remote resources to find it.
if !ok && !name.ContainsRemoteNames() {
Expand Down
5 changes: 5 additions & 0 deletions utils/errors.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,11 @@ func NewResourceNotFoundError(name resource.Name) error {
return errors.Errorf("resource %q not found", name)
}

// NewResourceGetError is used when a resource could not be retrieved because it did not start up properly.
func NewResourceGetError(name resource.Name, err error) error {
return errors.Wrapf(err, "failed to get resource %q", name)
}

// NewRemoteResourceClashError is used when you are more than one resource with the same name exist.
func NewRemoteResourceClashError(name string) error {
return errors.Errorf("more that one remote resources with name %q exists", name)
Expand Down