From 6614ddfb96c3189baf6b11957a7dfd7cefbc486b Mon Sep 17 00:00:00 2001 From: Kschappacher <56745262+Kschappacher@users.noreply.github.com> Date: Wed, 25 Sep 2024 15:45:12 -0400 Subject: [PATCH 01/21] first pass --- config/config.go | 33 ++++--- go.mod | 2 +- go.sum | 4 +- robot/impl/local_robot.go | 50 +++++++++- robot/impl/local_robot_test.go | 168 +++++++++++++++++++++++++++++++++ 5 files changed, 242 insertions(+), 15 deletions(-) diff --git a/config/config.go b/config/config.go index c2ba258ae51..978c803fb59 100644 --- a/config/config.go +++ b/config/config.go @@ -29,17 +29,18 @@ import ( // A Config describes the configuration of a robot. type Config struct { - Cloud *Cloud - Modules []Module - Remotes []Remote - Components []resource.Config - Processes []pexec.ProcessConfig - Services []resource.Config - Packages []PackageConfig - Network NetworkConfig - Auth AuthConfig - Debug bool - LogConfig []logging.LoggerPatternConfig + Cloud *Cloud + Modules []Module + Remotes []Remote + Components []resource.Config + Processes []pexec.ProcessConfig + Services []resource.Config + Packages []PackageConfig + Network NetworkConfig + Auth AuthConfig + Debug bool + LogConfig []logging.LoggerPatternConfig + MaintenanceConfig *MaintenanceConfig ConfigFilePath string @@ -77,6 +78,13 @@ type Config struct { toCache []byte } +// MaintenanceConfig specifies a reconfigure sensor +// When the sensor is read that value will be used to determine if it is safe to reconfigure. +type MaintenanceConfig struct { + SensorName string `json:"sensor_name"` + MaintenanceAllowedKey string `json:"maintenance_allowed_key"` +} + // NOTE: This data must be maintained with what is in Config. type configData struct { Cloud *Cloud `json:"cloud,omitempty"` @@ -93,6 +101,7 @@ type configData struct { EnableWebProfile bool `json:"enable_web_profile"` LogConfig []logging.LoggerPatternConfig `json:"log,omitempty"` Revision string `json:"revision,omitempty"` + MaintenanceConfig *MaintenanceConfig `json:"maintenance,omitempty"` } // AppValidationStatus refers to the. @@ -299,6 +308,7 @@ func (c *Config) UnmarshalJSON(data []byte) error { c.EnableWebProfile = conf.EnableWebProfile c.LogConfig = conf.LogConfig c.Revision = conf.Revision + c.MaintenanceConfig = conf.MaintenanceConfig return nil } @@ -330,6 +340,7 @@ func (c Config) MarshalJSON() ([]byte, error) { EnableWebProfile: c.EnableWebProfile, LogConfig: c.LogConfig, Revision: c.Revision, + MaintenanceConfig: c.MaintenanceConfig, }) } diff --git a/go.mod b/go.mod index 2dcdd77d7ac..cea23abac66 100644 --- a/go.mod +++ b/go.mod @@ -83,7 +83,7 @@ require ( go.uber.org/atomic v1.11.0 go.uber.org/multierr v1.11.0 go.uber.org/zap v1.24.0 - go.viam.com/api v0.1.340 + go.viam.com/api v0.1.341 go.viam.com/test v1.1.1-0.20220913152726-5da9916c08a2 go.viam.com/utils v0.1.100 goji.io v2.0.2+incompatible diff --git a/go.sum b/go.sum index 201f2404d8a..c16d5844144 100644 --- a/go.sum +++ b/go.sum @@ -1583,8 +1583,8 @@ go.uber.org/zap v1.18.1/go.mod h1:xg/QME4nWcxGxrpdeYfq7UvYrLh66cuVKdrbD1XF/NI= go.uber.org/zap v1.23.0/go.mod h1:D+nX8jyLsMHMYrln8A0rJjFt/T/9/bGgIhAqxv5URuY= go.uber.org/zap v1.24.0 h1:FiJd5l1UOLj0wCgbSE0rwwXHzEdAZS6hiiSnxJN/D60= go.uber.org/zap v1.24.0/go.mod h1:2kMP+WWQ8aoFoedH3T2sq6iJ2yDWpHbP0f6MQbS9Gkg= -go.viam.com/api v0.1.340 h1:0fdyOLxkI5vLc6CF0atKpY+2Lh9UWrDrOCx0bJtr2wE= -go.viam.com/api v0.1.340/go.mod h1:5lpVRxMsKFCaahqsnJfPGwJ9baoQ6PIKQu3lxvy6Wtw= +go.viam.com/api v0.1.341 h1:wJ0Pn+0PAyX6prk2PJ0key5TyMFqhUZMRMmrrRZbnB8= +go.viam.com/api v0.1.341/go.mod h1:5lpVRxMsKFCaahqsnJfPGwJ9baoQ6PIKQu3lxvy6Wtw= go.viam.com/test v1.1.1-0.20220913152726-5da9916c08a2 h1:oBiK580EnEIzgFLU4lHOXmGAE3MxnVbeR7s1wp/F3Ps= go.viam.com/test v1.1.1-0.20220913152726-5da9916c08a2/go.mod h1:XM0tej6riszsiNLT16uoyq1YjuYPWlRBweTPRDanIts= go.viam.com/utils v0.1.100 h1:/9gPxFtQuTwzJRdHYEM/qMdMfXm+xBaM+YVbVHB16mk= diff --git a/robot/impl/local_robot.go b/robot/impl/local_robot.go index 6530b824e32..7c6487f4e78 100644 --- a/robot/impl/local_robot.go +++ b/robot/impl/local_robot.go @@ -23,6 +23,7 @@ import ( "google.golang.org/grpc/status" "go.viam.com/rdk/cloud" + "go.viam.com/rdk/components/sensor" "go.viam.com/rdk/config" icloud "go.viam.com/rdk/internal/cloud" "go.viam.com/rdk/logging" @@ -1183,12 +1184,19 @@ func (r *localRobot) reconfigure(ctx context.Context, newConfig *config.Config, var allErrs error + canReconfigure, err := r.checkMaintenanceSensor(newConfig) + if err != nil { + r.logger.Info(err.Error() + ". using default reconfiguration behavior") + } + if !canReconfigure { + return + } // Sync Packages before reconfiguring rest of robot and resolving references to any packages // in the config. // TODO(RSDK-1849): Make this non-blocking so other resources that do not require packages can run before package sync finishes. // TODO(RSDK-2710) this should really use Reconfigure for the package and should allow itself to check // if anything has changed. - err := r.packageManager.Sync(ctx, newConfig.Packages, newConfig.Modules) + err = r.packageManager.Sync(ctx, newConfig.Packages, newConfig.Modules) if err != nil { r.Logger().CErrorw(ctx, "reconfiguration aborted because cloud modules or packages download failed", "error", err) return @@ -1435,3 +1443,43 @@ func (r *localRobot) MachineStatus(ctx context.Context) (robot.MachineStatus, er func (r *localRobot) Version(ctx context.Context) (robot.VersionResponse, error) { return robot.Version() } + +func (r *localRobot) checkMaintenanceSensor(newConfig *config.Config) (bool, error) { + if newConfig.MaintenanceConfig == nil { + return true, errors.New("maintenanceConfig undefined") + } + sensorFound := false + // Need to double check if this is needed + for _, component := range newConfig.Components { + if component.Name == newConfig.MaintenanceConfig.SensorName { + if sensorFound { + return true, errors.New("conflicting maintenance sensors found") + } + sensorFound = true + } + } + if !sensorFound { + return true, errors.Errorf("maintenance sensor %s not found", newConfig.MaintenanceConfig.SensorName) + } + sensorComponent, err := sensor.FromRobot(r, newConfig.MaintenanceConfig.SensorName) + if err != nil { + return true, errors.Errorf("maintenance sensor not found on local robot. %s", err.Error()) + } + return r.checkMaintenanceSensorReadings(newConfig.MaintenanceConfig.MaintenanceAllowedKey, sensorComponent) +} + +func (r *localRobot) checkMaintenanceSensorReadings(maintenanceAllowedKey string, sensor resource.Sensor) (bool, error) { + readings, err := sensor.Readings(context.Background(), map[string]interface{}{}) + if err != nil { + return true, errors.Errorf("error reading maintenance sensor readings. %s", err.Error()) + } + readingVal, ok := readings[maintenanceAllowedKey] + if !ok { + return true, errors.New("error getting MaintenanceAllowedKey from sensor reading") + } + canReconfigure, ok := readingVal.(bool) + if !ok { + return true, errors.New("maintenanceAllowedKey is not a bool value") + } + return canReconfigure, nil +} diff --git a/robot/impl/local_robot_test.go b/robot/impl/local_robot_test.go index 54f87b7c346..6088fb7ef62 100644 --- a/robot/impl/local_robot_test.go +++ b/robot/impl/local_robot_test.go @@ -20,6 +20,7 @@ import ( "github.com/golang/geo/r3" "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" @@ -48,6 +49,7 @@ import ( fakemotor "go.viam.com/rdk/components/motor/fake" "go.viam.com/rdk/components/movementsensor" _ "go.viam.com/rdk/components/register" + "go.viam.com/rdk/components/sensor" "go.viam.com/rdk/config" "go.viam.com/rdk/examples/customresources/apis/gizmoapi" "go.viam.com/rdk/examples/customresources/apis/summationapi" @@ -3955,3 +3957,169 @@ func TestLogPropagation(t *testing.T) { }) } } +func TestCheckMaintenanceSensor(t *testing.T) { + t.Parallel() + logger := logging.NewTestLogger(t) + validMaintenanceConfig := config.MaintenanceConfig{ + SensorName: "Patrick", + MaintenanceAllowedKey: "Star", + } + tests := []struct { + canReconfigure bool + robotConfig *config.Config + newConfig *config.Config + errorMessage string + }{ + { + canReconfigure: true, + robotConfig: &config.Config{}, + newConfig: &config.Config{}, + errorMessage: "maintenanceConfig undefined", + }, + { + canReconfigure: true, + robotConfig: &config.Config{}, + newConfig: &config.Config{ + MaintenanceConfig: &validMaintenanceConfig, + Components: []resource.Config{ + { + Name: "Patrick", + }, + { + Name: "Patrick", + }, + }, + }, + errorMessage: "conflicting maintenance sensors found", + }, + { + canReconfigure: true, + robotConfig: &config.Config{}, + newConfig: &config.Config{ + MaintenanceConfig: &validMaintenanceConfig, + Components: []resource.Config{}, + }, + errorMessage: "maintenance sensor Patrick not found", + }, + { + canReconfigure: true, + robotConfig: &config.Config{}, + newConfig: &config.Config{ + MaintenanceConfig: &validMaintenanceConfig, + Components: []resource.Config{ + { + Name: "Patrick", + }, + }, + }, + errorMessage: "maintenance sensor not found on local robot. resource \"rdk:component:sensor/Patrick\" not found", + }, + { + canReconfigure: true, + robotConfig: &config.Config{Components: []resource.Config{ + { + API: sensor.API, + Name: "Patrick", + Model: resource.DefaultModelFamily.WithModel("fake"), + }, + }}, + newConfig: &config.Config{ + MaintenanceConfig: &validMaintenanceConfig, + Components: []resource.Config{ + { + Name: "Patrick", + }, + }, + }, + errorMessage: "error getting MaintenanceAllowedKey from sensor reading", + }, + } + for _, tc := range tests { + t.Run("", func(t *testing.T) { + r := setupLocalRobot(t, context.Background(), tc.robotConfig, logger) + localRobot := r.(*localRobot) + canReconfigure, err := localRobot.checkMaintenanceSensor(tc.newConfig) + + test.That(t, canReconfigure, test.ShouldEqual, tc.canReconfigure) + test.That(t, err.Error(), test.ShouldEqual, tc.errorMessage) + }) + } +} + +func TestCheckMaintenanceSensorReadings(t *testing.T) { + t.Parallel() + logger := logging.NewTestLogger(t) + tests := []struct { + canReconfigure bool + maintenanceAllowedKey string + sensor resource.Sensor + errorMessage string + }{ + { + canReconfigure: true, + maintenanceAllowedKey: "", + sensor: newErrorSensor(), + errorMessage: "error reading maintenance sensor readings. Wallet not found", + }, + { + canReconfigure: true, + maintenanceAllowedKey: "UnknownKey", + sensor: newSensor(), + errorMessage: "error getting MaintenanceAllowedKey from sensor reading", + }, + } + for _, tc := range tests { + t.Run("", func(t *testing.T) { + r := setupLocalRobot(t, context.Background(), &config.Config{}, logger) + localRobot := r.(*localRobot) + canReconfigure, err := localRobot.checkMaintenanceSensorReadings(tc.maintenanceAllowedKey, tc.sensor) + + test.That(t, canReconfigure, test.ShouldEqual, tc.canReconfigure) + test.That(t, err.Error(), test.ShouldEqual, tc.errorMessage) + }) + } + testsValid := []struct { + canReconfigure bool + maintenanceAllowedKey string + sensor resource.Sensor + }{ + { + canReconfigure: false, + maintenanceAllowedKey: "ThatsMyWallet", + sensor: newSensor(), + }, + { + canReconfigure: true, + maintenanceAllowedKey: "ThatsNotMyWallet", + sensor: newSensor(), + }, + } + for _, tc := range testsValid { + t.Run("", func(t *testing.T) { + r := setupLocalRobot(t, context.Background(), &config.Config{}, logger) + localRobot := r.(*localRobot) + canReconfigure, err := localRobot.checkMaintenanceSensorReadings(tc.maintenanceAllowedKey, tc.sensor) + + test.That(t, canReconfigure, test.ShouldEqual, tc.canReconfigure) + test.That(t, err, test.ShouldBeNil) + }) + } +} + +var readingMap = map[string]any{"ThatsMyWallet": false, "ThatsNotMyWallet": true} + +func newSensor() sensor.Sensor { + s := &inject.Sensor{} + s.ReadingsFunc = func(ctx context.Context, extra map[string]interface{}) (map[string]interface{}, error) { + return readingMap, nil + } + return s +} + +func newErrorSensor() sensor.Sensor { + s := &inject.Sensor{} + s.ReadingsFunc = func(ctx context.Context, extra map[string]interface{}) (map[string]interface{}, error) { + return nil, errors.New("Wallet not found") + } + return s +} From 984b7657850686220f5d4b7178a25e8608bcdca7 Mon Sep 17 00:00:00 2001 From: Kschappacher <56745262+Kschappacher@users.noreply.github.com> Date: Wed, 25 Sep 2024 16:42:02 -0400 Subject: [PATCH 02/21] clean up errors --- robot/impl/local_robot.go | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/robot/impl/local_robot.go b/robot/impl/local_robot.go index 7c6487f4e78..f82ba50f7b3 100644 --- a/robot/impl/local_robot.go +++ b/robot/impl/local_robot.go @@ -1189,6 +1189,7 @@ func (r *localRobot) reconfigure(ctx context.Context, newConfig *config.Config, r.logger.Info(err.Error() + ". using default reconfiguration behavior") } if !canReconfigure { + r.logger.Info("maintenance sensor determined it is not safe to reconfigure, disabling reconfigure") return } // Sync Packages before reconfiguring rest of robot and resolving references to any packages @@ -1475,11 +1476,13 @@ func (r *localRobot) checkMaintenanceSensorReadings(maintenanceAllowedKey string } readingVal, ok := readings[maintenanceAllowedKey] if !ok { - return true, errors.New("error getting MaintenanceAllowedKey from sensor reading") + return true, errors.Errorf("error getting MaintenanceAllowedKey %s from sensor reading", maintenanceAllowedKey) } canReconfigure, ok := readingVal.(bool) if !ok { - return true, errors.New("maintenanceAllowedKey is not a bool value") + return true, errors.Errorf("maintenanceAllowedKey %s is not a bool value", maintenanceAllowedKey) } + + r.logger.Info("maintenanceAllowedKey found canReconfigure set to %v", canReconfigure) return canReconfigure, nil } From 43e8cd9aa3fbb930a7283a85ac0922d0ef7c0a1d Mon Sep 17 00:00:00 2001 From: Kschappacher <56745262+Kschappacher@users.noreply.github.com> Date: Wed, 25 Sep 2024 16:52:28 -0400 Subject: [PATCH 03/21] lint --- robot/impl/local_robot.go | 2 +- robot/impl/local_robot_test.go | 4 +--- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/robot/impl/local_robot.go b/robot/impl/local_robot.go index f82ba50f7b3..1c16b17e426 100644 --- a/robot/impl/local_robot.go +++ b/robot/impl/local_robot.go @@ -1483,6 +1483,6 @@ func (r *localRobot) checkMaintenanceSensorReadings(maintenanceAllowedKey string return true, errors.Errorf("maintenanceAllowedKey %s is not a bool value", maintenanceAllowedKey) } - r.logger.Info("maintenanceAllowedKey found canReconfigure set to %v", canReconfigure) + r.logger.Info("maintenanceAllowedKey found canReconfigure set to %t", canReconfigure) return canReconfigure, nil } diff --git a/robot/impl/local_robot_test.go b/robot/impl/local_robot_test.go index 6088fb7ef62..903bd4a4e01 100644 --- a/robot/impl/local_robot_test.go +++ b/robot/impl/local_robot_test.go @@ -20,7 +20,6 @@ import ( "github.com/golang/geo/r3" "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" @@ -3957,8 +3956,8 @@ func TestLogPropagation(t *testing.T) { }) } } + func TestCheckMaintenanceSensor(t *testing.T) { - t.Parallel() logger := logging.NewTestLogger(t) validMaintenanceConfig := config.MaintenanceConfig{ SensorName: "Patrick", @@ -4047,7 +4046,6 @@ func TestCheckMaintenanceSensor(t *testing.T) { } func TestCheckMaintenanceSensorReadings(t *testing.T) { - t.Parallel() logger := logging.NewTestLogger(t) tests := []struct { canReconfigure bool From a2b0b48500c82c4cae01bfedf3052bc0d9c4e137 Mon Sep 17 00:00:00 2001 From: Kschappacher <56745262+Kschappacher@users.noreply.github.com> Date: Fri, 27 Sep 2024 10:09:26 -0400 Subject: [PATCH 04/21] fix tests and lint --- robot/impl/local_robot.go | 5 ++--- robot/impl/local_robot_test.go | 7 ++++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/robot/impl/local_robot.go b/robot/impl/local_robot.go index 1c16b17e426..1193e4d3d81 100644 --- a/robot/impl/local_robot.go +++ b/robot/impl/local_robot.go @@ -1186,7 +1186,7 @@ func (r *localRobot) reconfigure(ctx context.Context, newConfig *config.Config, canReconfigure, err := r.checkMaintenanceSensor(newConfig) if err != nil { - r.logger.Info(err.Error() + ". using default reconfiguration behavior") + r.logger.Info(err.Error() + ". using default reconfigure behavior") } if !canReconfigure { r.logger.Info("maintenance sensor determined it is not safe to reconfigure, disabling reconfigure") @@ -1447,10 +1447,9 @@ func (r *localRobot) Version(ctx context.Context) (robot.VersionResponse, error) func (r *localRobot) checkMaintenanceSensor(newConfig *config.Config) (bool, error) { if newConfig.MaintenanceConfig == nil { - return true, errors.New("maintenanceConfig undefined") + return true, errors.New("maintenanceConfig undefined. Using default reconfigure") } sensorFound := false - // Need to double check if this is needed for _, component := range newConfig.Components { if component.Name == newConfig.MaintenanceConfig.SensorName { if sensorFound { diff --git a/robot/impl/local_robot_test.go b/robot/impl/local_robot_test.go index 903bd4a4e01..3120599fa50 100644 --- a/robot/impl/local_robot_test.go +++ b/robot/impl/local_robot_test.go @@ -20,6 +20,7 @@ import ( "github.com/golang/geo/r3" "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" @@ -3973,7 +3974,7 @@ func TestCheckMaintenanceSensor(t *testing.T) { canReconfigure: true, robotConfig: &config.Config{}, newConfig: &config.Config{}, - errorMessage: "maintenanceConfig undefined", + errorMessage: "maintenanceConfig undefined. Using default reconfigure", }, { canReconfigure: true, @@ -4030,7 +4031,7 @@ func TestCheckMaintenanceSensor(t *testing.T) { }, }, }, - errorMessage: "error getting MaintenanceAllowedKey from sensor reading", + errorMessage: "error getting MaintenanceAllowedKey Star from sensor reading", }, } for _, tc := range tests { @@ -4063,7 +4064,7 @@ func TestCheckMaintenanceSensorReadings(t *testing.T) { canReconfigure: true, maintenanceAllowedKey: "UnknownKey", sensor: newSensor(), - errorMessage: "error getting MaintenanceAllowedKey from sensor reading", + errorMessage: "error getting MaintenanceAllowedKey UnknownKey from sensor reading", }, } for _, tc := range tests { From 8964051b59ccdb2b550b1aa29a051fa003eeadd8 Mon Sep 17 00:00:00 2001 From: Kschappacher <56745262+Kschappacher@users.noreply.github.com> Date: Fri, 27 Sep 2024 10:25:20 -0400 Subject: [PATCH 05/21] lint --- robot/impl/local_robot_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/robot/impl/local_robot_test.go b/robot/impl/local_robot_test.go index 3120599fa50..0062f6ea28a 100644 --- a/robot/impl/local_robot_test.go +++ b/robot/impl/local_robot_test.go @@ -20,7 +20,6 @@ import ( "github.com/golang/geo/r3" "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" From ad3eff862b0b6398ea941bb603e7d04e135c7ffc Mon Sep 17 00:00:00 2001 From: Kschappacher <56745262+Kschappacher@users.noreply.github.com> Date: Wed, 9 Oct 2024 11:21:33 -0400 Subject: [PATCH 06/21] add timeout and proto conversions --- config/proto_conversions.go | 29 +++ config/proto_conversions_test.go | 43 ++++ robot/impl/local_robot.go | 84 ++++---- robot/impl/local_robot_test.go | 337 ++++++++++++++++++++----------- 4 files changed, 339 insertions(+), 154 deletions(-) diff --git a/config/proto_conversions.go b/config/proto_conversions.go index 64d2141a69f..916237a6939 100644 --- a/config/proto_conversions.go +++ b/config/proto_conversions.go @@ -9,6 +9,7 @@ import ( "github.com/pkg/errors" packagespb "go.viam.com/api/app/packages/v1" pb "go.viam.com/api/app/v1" + protoRdkUtils "go.viam.com/rdk/protoutils" "go.viam.com/utils/pexec" "go.viam.com/utils/protoutils" "go.viam.com/utils/rpc" @@ -94,6 +95,14 @@ func FromProto(proto *pb.RobotConfig, logger logging.Logger) (*Config, error) { cfg.Revision = proto.Revision + if proto.Maintenance != nil { + maintenanceConfig, err := MaintenanceConfigFromProto(proto.Maintenance) + if err != nil { + return nil, errors.Wrap(err, "error converting maintenance config from proto") + } + cfg.MaintenanceConfig = maintenanceConfig + } + logAnyFragmentOverwriteErrors(logger, proto.OverwriteFragmentStatus) return &cfg, nil @@ -633,6 +642,18 @@ func NetworkConfigToProto(network *NetworkConfig) (*pb.NetworkConfig, error) { return &proto, nil } +// MaintenanceConfigToProto converts MaintenanceConfig from the proto equivalent. +func MaintenanceConfigToProto(maintenanceConfig *MaintenanceConfig) (*pb.MaintenanceConfig, error) { + name := resource.NewName(resource.APINamespaceRDK.WithComponentType("sensor"), maintenanceConfig.SensorName) + + proto := pb.MaintenanceConfig{ + SensorName: protoRdkUtils.ResourceNameToProto(name), + MaintenanceAllowedKey: maintenanceConfig.MaintenanceAllowedKey, + } + + return &proto, nil +} + // NetworkConfigFromProto creates NetworkConfig from the proto equivalent. func NetworkConfigFromProto(proto *pb.NetworkConfig) (*NetworkConfig, error) { network := NetworkConfig{ @@ -647,6 +668,14 @@ func NetworkConfigFromProto(proto *pb.NetworkConfig) (*NetworkConfig, error) { return &network, nil } +// MaintenanceConfigFromProto creates a MaintenanceConfig from the proto equivalent +func MaintenanceConfigFromProto(proto *pb.MaintenanceConfig) (*MaintenanceConfig, error) { + MaintenanceConfig := MaintenanceConfig{ + SensorName:protoRdkUtils.ResourceNameFromProto(proto.SensorName).ShortName(), + MaintenanceAllowedKey: proto.GetMaintenanceAllowedKey(), + } + return &MaintenanceConfig, nil +} // AuthConfigToProto converts AuthConfig to the proto equivalent. func AuthConfigToProto(auth *AuthConfig) (*pb.AuthConfig, error) { diff --git a/config/proto_conversions_test.go b/config/proto_conversions_test.go index ab65a9bb051..88e4da7259a 100644 --- a/config/proto_conversions_test.go +++ b/config/proto_conversions_test.go @@ -1005,3 +1005,46 @@ func TestPackageTypeConversion(t *testing.T) { test.That(t, fmt.Sprint(err), test.ShouldContainSubstring, "invalid-package-type") test.That(t, converted, test.ShouldResemble, packagespb.PackageType_PACKAGE_TYPE_UNSPECIFIED.Enum()) } + +func TestMaintenanceConfigToProtoSuccess(t *testing.T) { + testMaintenanceConfig:= MaintenanceConfig{ + SensorName: "car", + MaintenanceAllowedKey: "honk", + } + + proto, err := MaintenanceConfigToProto(&testMaintenanceConfig) + test.That(t, err, test.ShouldBeNil) + out, err := MaintenanceConfigFromProto(proto) + test.That(t, err, test.ShouldBeNil) + + test.That(t, *out, test.ShouldResemble, testMaintenanceConfig) +} + +func TestMaintenanceConfigToProtoRemoteSuccess(t *testing.T) { + testMaintenanceConfig:= MaintenanceConfig{ + SensorName: "car:go:store", + MaintenanceAllowedKey: "fast", + } + + proto, err := MaintenanceConfigToProto(&testMaintenanceConfig) + test.That(t, err, test.ShouldBeNil) + out, err := MaintenanceConfigFromProto(proto) + test.That(t, err, test.ShouldBeNil) + + test.That(t, *out, test.ShouldResemble, testMaintenanceConfig) +} + +func TestMaintenanceConfigToProtoFailure(t *testing.T) { + testMaintenanceConfig:= MaintenanceConfig{ + SensorName: "car:go", + MaintenanceAllowedKey: "slow", + } + + proto, err := MaintenanceConfigToProto(&testMaintenanceConfig) + test.That(t, err, test.ShouldBeNil) + out, err := MaintenanceConfigFromProto(proto) + test.That(t, err, test.ShouldBeNil) + + test.That(t, *out, test.ShouldResemble, testMaintenanceConfig) +} + diff --git a/robot/impl/local_robot.go b/robot/impl/local_robot.go index 1193e4d3d81..221da60452e 100644 --- a/robot/impl/local_robot.go +++ b/robot/impl/local_robot.go @@ -1184,20 +1184,32 @@ func (r *localRobot) reconfigure(ctx context.Context, newConfig *config.Config, var allErrs error - canReconfigure, err := r.checkMaintenanceSensor(newConfig) - if err != nil { - r.logger.Info(err.Error() + ". using default reconfigure behavior") - } - if !canReconfigure { - r.logger.Info("maintenance sensor determined it is not safe to reconfigure, disabling reconfigure") - return + if newConfig.MaintenanceConfig != nil { + sensorComponent, err := sensor.FromRobot(r, newConfig.MaintenanceConfig.SensorName) + if err != nil { + r.logger.Infof("%s, Starting reconfiguration", err.Error()) + } else { + canReconfigure, err := r.checkMaintenanceSensorReadings(newConfig.MaintenanceConfig.MaintenanceAllowedKey, sensorComponent) + if err != nil { + r.logger.Info(err.Error() + ". Starting reconfiguration") + } else { + if !canReconfigure { + r.logger.Info("maintenanceAllowedKey found from readings on maintenance sensor. Reconfigure disabled") + return + } else { + r.logger.Info("maintenanceAllowedKey found from readings on maintenance sensor. Starting reconfiguration") + + } + } + } } + // Sync Packages before reconfiguring rest of robot and resolving references to any packages // in the config. // TODO(RSDK-1849): Make this non-blocking so other resources that do not require packages can run before package sync finishes. // TODO(RSDK-2710) this should really use Reconfigure for the package and should allow itself to check // if anything has changed. - err = r.packageManager.Sync(ctx, newConfig.Packages, newConfig.Modules) + err := r.packageManager.Sync(ctx, newConfig.Packages, newConfig.Modules) if err != nil { r.Logger().CErrorw(ctx, "reconfiguration aborted because cloud modules or packages download failed", "error", err) return @@ -1445,32 +1457,17 @@ func (r *localRobot) Version(ctx context.Context) (robot.VersionResponse, error) return robot.Version() } -func (r *localRobot) checkMaintenanceSensor(newConfig *config.Config) (bool, error) { - if newConfig.MaintenanceConfig == nil { - return true, errors.New("maintenanceConfig undefined. Using default reconfigure") - } - sensorFound := false - for _, component := range newConfig.Components { - if component.Name == newConfig.MaintenanceConfig.SensorName { - if sensorFound { - return true, errors.New("conflicting maintenance sensors found") - } - sensorFound = true - } - } - if !sensorFound { - return true, errors.Errorf("maintenance sensor %s not found", newConfig.MaintenanceConfig.SensorName) - } - sensorComponent, err := sensor.FromRobot(r, newConfig.MaintenanceConfig.SensorName) - if err != nil { - return true, errors.Errorf("maintenance sensor not found on local robot. %s", err.Error()) - } - return r.checkMaintenanceSensorReadings(newConfig.MaintenanceConfig.MaintenanceAllowedKey, sensorComponent) -} - +// checkMaintenanceSensorReadings ensures that errors from reading a sensor are handled properly func (r *localRobot) checkMaintenanceSensorReadings(maintenanceAllowedKey string, sensor resource.Sensor) (bool, error) { - readings, err := sensor.Readings(context.Background(), map[string]interface{}{}) + timeout := 5 * time.Second + ctx, cancel := context.WithTimeout(context.Background(), timeout) + defer cancel() + + readings, timeoutOccurred, err := checkSensorReadingTimeout(ctx, sensor, timeout) if err != nil { + if timeoutOccurred { + return false, err + } return true, errors.Errorf("error reading maintenance sensor readings. %s", err.Error()) } readingVal, ok := readings[maintenanceAllowedKey] @@ -1481,7 +1478,26 @@ func (r *localRobot) checkMaintenanceSensorReadings(maintenanceAllowedKey string if !ok { return true, errors.Errorf("maintenanceAllowedKey %s is not a bool value", maintenanceAllowedKey) } - - r.logger.Info("maintenanceAllowedKey found canReconfigure set to %t", canReconfigure) return canReconfigure, nil } + +type Readings struct { + readings map[string]interface{} + err error +} + +// checkSensorReadingTimeout adds a timeout to Readings to ensure that the call returns after timeout seconds +func checkSensorReadingTimeout(ctx context.Context, sensor resource.Sensor, timeout time.Duration) (map[string]interface{}, bool, error) { + c := make(chan Readings) + + go func(ctx context.Context, c chan Readings) { + readings, err := sensor.Readings(ctx, map[string]interface{}{}) + c <- Readings{readings: readings, err: err} + }(ctx, c) + select { + case readings := <-c: + return readings.readings, false, readings.err + case <-time.After(timeout): + return nil, true, errors.New("maintenance sensor timed out on reading") + } +} diff --git a/robot/impl/local_robot_test.go b/robot/impl/local_robot_test.go index 0062f6ea28a..4bc95b26779 100644 --- a/robot/impl/local_robot_test.go +++ b/robot/impl/local_robot_test.go @@ -20,6 +20,7 @@ import ( "github.com/golang/geo/r3" "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" @@ -3957,139 +3958,70 @@ func TestLogPropagation(t *testing.T) { } } -func TestCheckMaintenanceSensor(t *testing.T) { +func TestCheckMaintenanceSensorReadings(t *testing.T) { logger := logging.NewTestLogger(t) - validMaintenanceConfig := config.MaintenanceConfig{ - SensorName: "Patrick", - MaintenanceAllowedKey: "Star", - } - tests := []struct { - canReconfigure bool - robotConfig *config.Config - newConfig *config.Config - errorMessage string - }{ - { - canReconfigure: true, - robotConfig: &config.Config{}, - newConfig: &config.Config{}, - errorMessage: "maintenanceConfig undefined. Using default reconfigure", - }, - { - canReconfigure: true, - robotConfig: &config.Config{}, - newConfig: &config.Config{ - MaintenanceConfig: &validMaintenanceConfig, - Components: []resource.Config{ - { - Name: "Patrick", - }, - { - Name: "Patrick", - }, - }, - }, - errorMessage: "conflicting maintenance sensors found", - }, - { - canReconfigure: true, - robotConfig: &config.Config{}, - newConfig: &config.Config{ - MaintenanceConfig: &validMaintenanceConfig, - Components: []resource.Config{}, - }, - errorMessage: "maintenance sensor Patrick not found", - }, - { - canReconfigure: true, - robotConfig: &config.Config{}, - newConfig: &config.Config{ - MaintenanceConfig: &validMaintenanceConfig, - Components: []resource.Config{ - { - Name: "Patrick", - }, - }, - }, - errorMessage: "maintenance sensor not found on local robot. resource \"rdk:component:sensor/Patrick\" not found", - }, - { - canReconfigure: true, - robotConfig: &config.Config{Components: []resource.Config{ - { - API: sensor.API, - Name: "Patrick", - Model: resource.DefaultModelFamily.WithModel("fake"), - }, - }}, - newConfig: &config.Config{ - MaintenanceConfig: &validMaintenanceConfig, - Components: []resource.Config{ - { - Name: "Patrick", - }, - }, - }, - errorMessage: "error getting MaintenanceAllowedKey Star from sensor reading", - }, - } - for _, tc := range tests { - t.Run("", func(t *testing.T) { - r := setupLocalRobot(t, context.Background(), tc.robotConfig, logger) - localRobot := r.(*localRobot) - canReconfigure, err := localRobot.checkMaintenanceSensor(tc.newConfig) + t.Run("Sensor reading errors out", func(t *testing.T) { + r := setupLocalRobot(t, context.Background(), &config.Config{}, logger) + localRobot := r.(*localRobot) + canReconfigure, err := localRobot.checkMaintenanceSensorReadings("", newErrorSensor()) - test.That(t, canReconfigure, test.ShouldEqual, tc.canReconfigure) - test.That(t, err.Error(), test.ShouldEqual, tc.errorMessage) - }) - } + test.That(t, canReconfigure, test.ShouldEqual, true) + test.That(t, err.Error(), test.ShouldEqual, "error reading maintenance sensor readings. Wallet not found") + }) + t.Run("maintenanceAllowedKey does not exist", func(t *testing.T) { + r := setupLocalRobot(t, context.Background(), &config.Config{}, logger) + localRobot := r.(*localRobot) + canReconfigure, err := localRobot.checkMaintenanceSensorReadings("keyDoesNotExist", newValidSensor()) + + test.That(t, canReconfigure, test.ShouldEqual, true) + test.That(t, err.Error(), test.ShouldEqual, "error getting MaintenanceAllowedKey keyDoesNotExist from sensor reading") + }) + t.Run("maintenanceAllowedKey is not a boolean", func(t *testing.T) { + r := setupLocalRobot(t, context.Background(), &config.Config{}, logger) + localRobot := r.(*localRobot) + canReconfigure, err := localRobot.checkMaintenanceSensorReadings("ThatIsNotAWallet", newValidSensor()) + + test.That(t, canReconfigure, test.ShouldEqual, true) + test.That(t, err.Error(), test.ShouldEqual, "maintenanceAllowedKey ThatIsNotAWallet is not a bool value") + }) } -func TestCheckMaintenanceSensorReadings(t *testing.T) { - logger := logging.NewTestLogger(t) - tests := []struct { - canReconfigure bool - maintenanceAllowedKey string - sensor resource.Sensor - errorMessage string - }{ - { - canReconfigure: true, - maintenanceAllowedKey: "", - sensor: newErrorSensor(), - errorMessage: "error reading maintenance sensor readings. Wallet not found", - }, - { - canReconfigure: true, - maintenanceAllowedKey: "UnknownKey", - sensor: newSensor(), - errorMessage: "error getting MaintenanceAllowedKey UnknownKey from sensor reading", - }, - } - for _, tc := range tests { - t.Run("", func(t *testing.T) { - r := setupLocalRobot(t, context.Background(), &config.Config{}, logger) - localRobot := r.(*localRobot) - canReconfigure, err := localRobot.checkMaintenanceSensorReadings(tc.maintenanceAllowedKey, tc.sensor) +func TestCheckSensorReadingTimeout(t *testing.T) { + t.Run("sensor reading errors", func(t *testing.T) { + retMap, timeoutOccurred, err := checkSensorReadingTimeout(context.Background(), newErrorSensor(), 2*time.Second) - test.That(t, canReconfigure, test.ShouldEqual, tc.canReconfigure) - test.That(t, err.Error(), test.ShouldEqual, tc.errorMessage) - }) - } + test.That(t, retMap, test.ShouldBeNil) + test.That(t, timeoutOccurred, test.ShouldBeFalse) + test.That(t, err.Error(), test.ShouldEqual, "Wallet not found") + }) + t.Run("sensor reading times out", func(t *testing.T) { + retMap, timeoutOccurred, err := checkSensorReadingTimeout(context.Background(), newTimeoutSensor(), 1*time.Millisecond) + + test.That(t, retMap, test.ShouldBeNil) + test.That(t, timeoutOccurred, test.ShouldBeTrue) + test.That(t, err.Error(), test.ShouldEqual, "maintenance sensor timed out on reading") + }) +} + +func TestCheckMaintenanceSensorReadingsSuccess(t *testing.T) { + logger := logging.NewTestLogger(t) testsValid := []struct { + testName string canReconfigure bool maintenanceAllowedKey string sensor resource.Sensor }{ { + testName: "Sensor returns reading false", canReconfigure: false, maintenanceAllowedKey: "ThatsMyWallet", - sensor: newSensor(), + sensor: newValidSensor(), }, { + testName: "Sensor returns reading true", canReconfigure: true, maintenanceAllowedKey: "ThatsNotMyWallet", - sensor: newSensor(), + sensor: newValidSensor(), }, } for _, tc := range testsValid { @@ -4104,12 +4036,10 @@ func TestCheckMaintenanceSensorReadings(t *testing.T) { } } -var readingMap = map[string]any{"ThatsMyWallet": false, "ThatsNotMyWallet": true} - -func newSensor() sensor.Sensor { +func newValidSensor() sensor.Sensor { s := &inject.Sensor{} s.ReadingsFunc = func(ctx context.Context, extra map[string]interface{}) (map[string]interface{}, error) { - return readingMap, nil + return map[string]any{"ThatsMyWallet": false, "ThatsNotMyWallet": true, "ThatIsNotAWallet": 5}, nil } return s } @@ -4121,3 +4051,170 @@ func newErrorSensor() sensor.Sensor { } return s } + +func newTimeoutSensor() sensor.Sensor { + s := &inject.Sensor{} + s.ReadingsFunc = func(ctx context.Context, extra map[string]interface{}) (map[string]interface{}, error) { + time.Sleep(2 * time.Millisecond) + return nil, errors.New("Wallet not found") + } + return s +} + + +func TestMaintenanceConfigWithRemotes(t *testing.T) { + logger := logging.NewTestLogger(t) + + r := setupLocalRobot(t, context.Background(), &config.Config{}, logger) +} + +func TestConfigMethod(t *testing.T) { + ctx := context.Background() + logger := logging.NewTestLogger(t) + + r := setupLocalRobot(t, context.Background(), &config.Config{}, logger) + + // Use a remote with components and services to ensure none of its resources + // will be returned by Config. + remoteCfg, err := config.Read(context.Background(), "data/remote_fake.json", logger) + test.That(t, err, test.ShouldBeNil) + remoteRobot := setupLocalRobot(t, ctx, remoteCfg, logger) + + options, _, addr := robottestutils.CreateBaseOptionsAndListener(t) + err = remoteRobot.StartWeb(ctx, options) + test.That(t, err, test.ShouldBeNil) + + // Manually define mybase model, as importing it can cause double registration. + myBaseModel := resource.NewModel("acme", "demo", "mybase") + + cfg := &config.Config{ + Cloud: &config.Cloud{}, + Modules: []config.Module{ + { + Name: "mod", + ExePath: complexPath, + LogLevel: "info", + }, + }, + Remotes: []config.Remote{ + { + Name: "foo", + Address: addr, + }, + }, + Components: []resource.Config{ + { + Name: "myBase", + API: base.API, + Model: myBaseModel, + Attributes: rutils.AttributeMap{ + "motorL": "motor1", + "motorR": "motor2", + }, + }, + { + Name: "motor1", + API: motor.API, + Model: fakeModel, + ConvertedAttributes: &fakemotor.Config{}, + ImplicitDependsOn: []string{"builtin:sensors"}, + }, + { + Name: "motor2", + API: motor.API, + Model: fakeModel, + ConvertedAttributes: &fakemotor.Config{}, + }, + }, + Processes: []pexec.ProcessConfig{ + { + ID: "1", + Name: "bash", + Args: []string{"-c", "echo heythere"}, + Log: true, + OneShot: true, + }, + }, + Services: []resource.Config{ + { + Name: "fake1", + API: datamanager.API, + Model: resource.DefaultServiceModel, + ConvertedAttributes: &builtin.Config{}, + ImplicitDependsOn: []string{"foo:builtin:data_manager"}, + }, + { + Name: "builtin", + API: navigation.API, + Model: resource.DefaultServiceModel, + }, + }, + Packages: []config.PackageConfig{ + { + Name: "some-name-1", + Package: "package-1", + Version: "v1", + }, + }, + Network: config.NetworkConfig{}, + Auth: config.AuthConfig{}, + Debug: true, + DisablePartialStart: true, + } + + // Create copy of expectedCfg since Reconfigure modifies cfg. + expectedCfg := *cfg + r.Reconfigure(ctx, cfg) + + // Assert that Config method returns expected value. + actualCfg = r.Config() + + // Assert that default motion and sensor services are still present, but data + // manager default service has been replaced by the "fake1" data manager service. + defaultSvcs = removeDefaultServices(actualCfg) + test.That(t, len(defaultSvcs), test.ShouldEqual, 2) + for _, svc := range defaultSvcs { + test.That(t, svc.API.SubtypeName, test.ShouldBeIn, motion.API.SubtypeName, + sensors.API.SubtypeName) + } + + // Manually inspect remaining service resources as ordering of config is + // non-deterministic within slices. + test.That(t, len(actualCfg.Services), test.ShouldEqual, 2) + for _, svc := range actualCfg.Services { + isFake1DM := svc.Equals(expectedCfg.Services[0]) + isBuiltinNav := svc.Equals(expectedCfg.Services[1]) + test.That(t, isFake1DM || isBuiltinNav, test.ShouldBeTrue) + } + actualCfg.Services = nil + expectedCfg.Services = nil + + // Manually inspect component resources as ordering of config is + // non-deterministic within slices + test.That(t, len(actualCfg.Components), test.ShouldEqual, 3) + for _, comp := range actualCfg.Components { + isMyBase := comp.Equals(expectedCfg.Components[0]) + isMotor1 := comp.Equals(expectedCfg.Components[1]) + isMotor2 := comp.Equals(expectedCfg.Components[2]) + test.That(t, isMyBase || isMotor1 || isMotor2, test.ShouldBeTrue) + } + actualCfg.Components = nil + expectedCfg.Components = nil + + // Manually inspect remote resources, modules, and processes as Equals should be used + // (alreadyValidated will have been set to true). + test.That(t, len(actualCfg.Remotes), test.ShouldEqual, 1) + test.That(t, actualCfg.Remotes[0].Equals(expectedCfg.Remotes[0]), test.ShouldBeTrue) + actualCfg.Remotes = nil + expectedCfg.Remotes = nil + test.That(t, len(actualCfg.Processes), test.ShouldEqual, 1) + test.That(t, actualCfg.Processes[0].Equals(expectedCfg.Processes[0]), test.ShouldBeTrue) + actualCfg.Processes = nil + expectedCfg.Processes = nil + test.That(t, len(actualCfg.Modules), test.ShouldEqual, 1) + test.That(t, actualCfg.Modules[0].Equals(expectedCfg.Modules[0]), test.ShouldBeTrue) + actualCfg.Modules = nil + expectedCfg.Modules = nil + + test.That(t, actualCfg, test.ShouldResemble, &expectedCfg) +} \ No newline at end of file From 3ac4051f6313cf01bbebd878bb4f57c4f6b6442a Mon Sep 17 00:00:00 2001 From: Kschappacher <56745262+Kschappacher@users.noreply.github.com> Date: Wed, 9 Oct 2024 17:28:13 -0400 Subject: [PATCH 07/21] fix timeout --- robot/impl/local_robot.go | 41 ++---- robot/impl/local_robot_test.go | 255 +++++++++++---------------------- testutils/inject/sensor.go | 12 ++ 3 files changed, 112 insertions(+), 196 deletions(-) diff --git a/robot/impl/local_robot.go b/robot/impl/local_robot.go index 221da60452e..a7a0cb634f2 100644 --- a/robot/impl/local_robot.go +++ b/robot/impl/local_robot.go @@ -1184,8 +1184,19 @@ func (r *localRobot) reconfigure(ctx context.Context, newConfig *config.Config, var allErrs error + // Maintenance config can be configured to block reconfigure based off of a sensor reading + // These sensors can be configured on the main robot, a fragment, or a remote + // In situations where there are conflicting sensor names the following behavior happens + // Main robot and remote share sensor name -> main robot sensor is chosen + // Only remote has the sensor name -> remote sensor is read + // Multiple remotes share a senor name -> conflict error is returned and reconfigure happens + // To specify a specific remote sensor use the name format remoteName:sensorName to specify a remote sensor if newConfig.MaintenanceConfig != nil { - sensorComponent, err := sensor.FromRobot(r, newConfig.MaintenanceConfig.SensorName) + name,err:=resource.NewFromString(newConfig.MaintenanceConfig.SensorName) + if err!=nil { + r.logger.Infof("Sensor Name %s is not in a supported format",newConfig.MaintenanceConfig.SensorName ) + } else { + sensorComponent, err := robot.ResourceFromRobot[sensor.Sensor](r, name) if err != nil { r.logger.Infof("%s, Starting reconfiguration", err.Error()) } else { @@ -1203,6 +1214,7 @@ func (r *localRobot) reconfigure(ctx context.Context, newConfig *config.Config, } } } + } // Sync Packages before reconfiguring rest of robot and resolving references to any packages // in the config. @@ -1463,11 +1475,9 @@ func (r *localRobot) checkMaintenanceSensorReadings(maintenanceAllowedKey string ctx, cancel := context.WithTimeout(context.Background(), timeout) defer cancel() - readings, timeoutOccurred, err := checkSensorReadingTimeout(ctx, sensor, timeout) + // Timeouts on this call should be handled grpc and return + readings, err := sensor.Readings(ctx, map[string]interface{}{}) if err != nil { - if timeoutOccurred { - return false, err - } return true, errors.Errorf("error reading maintenance sensor readings. %s", err.Error()) } readingVal, ok := readings[maintenanceAllowedKey] @@ -1480,24 +1490,3 @@ func (r *localRobot) checkMaintenanceSensorReadings(maintenanceAllowedKey string } return canReconfigure, nil } - -type Readings struct { - readings map[string]interface{} - err error -} - -// checkSensorReadingTimeout adds a timeout to Readings to ensure that the call returns after timeout seconds -func checkSensorReadingTimeout(ctx context.Context, sensor resource.Sensor, timeout time.Duration) (map[string]interface{}, bool, error) { - c := make(chan Readings) - - go func(ctx context.Context, c chan Readings) { - readings, err := sensor.Readings(ctx, map[string]interface{}{}) - c <- Readings{readings: readings, err: err} - }(ctx, c) - select { - case readings := <-c: - return readings.readings, false, readings.err - case <-time.After(timeout): - return nil, true, errors.New("maintenance sensor timed out on reading") - } -} diff --git a/robot/impl/local_robot_test.go b/robot/impl/local_robot_test.go index 4bc95b26779..3770d0d6f24 100644 --- a/robot/impl/local_robot_test.go +++ b/robot/impl/local_robot_test.go @@ -3986,23 +3986,6 @@ func TestCheckMaintenanceSensorReadings(t *testing.T) { }) } -func TestCheckSensorReadingTimeout(t *testing.T) { - t.Run("sensor reading errors", func(t *testing.T) { - retMap, timeoutOccurred, err := checkSensorReadingTimeout(context.Background(), newErrorSensor(), 2*time.Second) - - test.That(t, retMap, test.ShouldBeNil) - test.That(t, timeoutOccurred, test.ShouldBeFalse) - test.That(t, err.Error(), test.ShouldEqual, "Wallet not found") - }) - t.Run("sensor reading times out", func(t *testing.T) { - retMap, timeoutOccurred, err := checkSensorReadingTimeout(context.Background(), newTimeoutSensor(), 1*time.Millisecond) - - test.That(t, retMap, test.ShouldBeNil) - test.That(t, timeoutOccurred, test.ShouldBeTrue) - test.That(t, err.Error(), test.ShouldEqual, "maintenance sensor timed out on reading") - }) -} - func TestCheckMaintenanceSensorReadingsSuccess(t *testing.T) { logger := logging.NewTestLogger(t) testsValid := []struct { @@ -4041,6 +4024,7 @@ func newValidSensor() sensor.Sensor { s.ReadingsFunc = func(ctx context.Context, extra map[string]interface{}) (map[string]interface{}, error) { return map[string]any{"ThatsMyWallet": false, "ThatsNotMyWallet": true, "ThatIsNotAWallet": 5}, nil } + s.CloseFunc = func(ctx context.Context) error {return nil} return s } @@ -4052,169 +4036,100 @@ func newErrorSensor() sensor.Sensor { return s } -func newTimeoutSensor() sensor.Sensor { - s := &inject.Sensor{} - s.ReadingsFunc = func(ctx context.Context, extra map[string]interface{}) (map[string]interface{}, error) { - time.Sleep(2 * time.Millisecond) - return nil, errors.New("Wallet not found") - } - return s -} - - func TestMaintenanceConfigWithRemotes(t *testing.T) { - logger := logging.NewTestLogger(t) + model := resource.DefaultModelFamily.WithModel(utils.RandomAlphaString(8)) + resource.RegisterComponent( + sensor.API, + model, + resource.Registration[sensor.Sensor, resource.NoNativeConfig]{Constructor: func( + ctx context.Context, + deps resource.Dependencies, + conf resource.Config, + logger logging.Logger, + ) (sensor.Sensor, error) { + return newValidSensor(), nil + }}) + defer func() { + resource.Deregister(sensor.API, model) + }() - r := setupLocalRobot(t, context.Background(), &config.Config{}, logger) -} + t.Run("robot reconfigures with maintenanceConfig sensor and blocks reconfigure, reconfigure reenabled when maintenanceConfig removed", func(t *testing.T) { + ctx := context.Background() + logger := logging.NewTestLogger(t) + r := setupLocalRobot(t, context.Background(), &config.Config{}, logger) + cfg := &config.Config{ + MaintenanceConfig: &config.MaintenanceConfig{SensorName: "rdk:component:sensor/sensor",MaintenanceAllowedKey:"ThatsMyWallet"}, + Components: []resource.Config{ + { + Name: "sensor", + Model: model, + API: sensor.API, + }, + }, + } + r.Reconfigure(ctx, cfg) + sensorResource, err:= r.ResourceByName(sensor.Named("sensor")) + test.That(t, err, test.ShouldBeNil) + test.That(t, sensorResource, test.ShouldNotBeNil) + + // cfgBlocked := &config.Config{ + // MaintenanceConfig: &config.MaintenanceConfig{SensorName:"sensor",MaintenanceAllowedKey:"ThatsMyWallet"}, + // Components: []resource.Config{ + // { + // Name: "sensor2", + // API: sensor.API, + // Model: model, + // }, + // }, + // } + // r.Reconfigure(ctx, cfgBlocked) + // sensorBlocked, err:= r.ResourceByName(sensor.Named("sensor2")) + // test.That(t, sensorBlocked, test.ShouldBeNil) + // test.That(t, err.Error(),test.ShouldEqual, "resource \"rdk:component:sensor/sensor2\" not found") + // cfgUnblock := &config.Config{ + // Components: []resource.Config{ + // { + // Name: "sensor2", + // API: sensor.API, + // Model: model, + // }, + // }, + // } + // r.Reconfigure(ctx, cfgUnblock) + // sensorBlocked, err = r.ResourceByName(sensor.Named("sensor2")) + // test.That(t, err,test.ShouldBeNil) + // test.That(t, sensorBlocked, test.ShouldNotBeNil) + }) -func TestConfigMethod(t *testing.T) { - ctx := context.Background() - logger := logging.NewTestLogger(t) - r := setupLocalRobot(t, context.Background(), &config.Config{}, logger) + // cfg := &config.Config{ + // MaintenanceConfig: &config.MaintenanceConfig{SensorName:"",MaintenanceAllowedKey:""}, + // Remotes: []config.Remote{ + // { + // Name: "foo", + // Address: addr, + // }, + // }, + // Components: []resource.Config{ + // { + // Name: "sensor", + // API: sensor.API, + // Model: fakeModel, + // }, + // }, + // } - // Use a remote with components and services to ensure none of its resources - // will be returned by Config. - remoteCfg, err := config.Read(context.Background(), "data/remote_fake.json", logger) - test.That(t, err, test.ShouldBeNil) - remoteRobot := setupLocalRobot(t, ctx, remoteCfg, logger) - - options, _, addr := robottestutils.CreateBaseOptionsAndListener(t) - err = remoteRobot.StartWeb(ctx, options) - test.That(t, err, test.ShouldBeNil) - // Manually define mybase model, as importing it can cause double registration. - myBaseModel := resource.NewModel("acme", "demo", "mybase") - - cfg := &config.Config{ - Cloud: &config.Cloud{}, - Modules: []config.Module{ - { - Name: "mod", - ExePath: complexPath, - LogLevel: "info", - }, - }, - Remotes: []config.Remote{ - { - Name: "foo", - Address: addr, - }, - }, - Components: []resource.Config{ - { - Name: "myBase", - API: base.API, - Model: myBaseModel, - Attributes: rutils.AttributeMap{ - "motorL": "motor1", - "motorR": "motor2", - }, - }, - { - Name: "motor1", - API: motor.API, - Model: fakeModel, - ConvertedAttributes: &fakemotor.Config{}, - ImplicitDependsOn: []string{"builtin:sensors"}, - }, - { - Name: "motor2", - API: motor.API, - Model: fakeModel, - ConvertedAttributes: &fakemotor.Config{}, - }, - }, - Processes: []pexec.ProcessConfig{ - { - ID: "1", - Name: "bash", - Args: []string{"-c", "echo heythere"}, - Log: true, - OneShot: true, - }, - }, - Services: []resource.Config{ - { - Name: "fake1", - API: datamanager.API, - Model: resource.DefaultServiceModel, - ConvertedAttributes: &builtin.Config{}, - ImplicitDependsOn: []string{"foo:builtin:data_manager"}, - }, - { - Name: "builtin", - API: navigation.API, - Model: resource.DefaultServiceModel, - }, - }, - Packages: []config.PackageConfig{ - { - Name: "some-name-1", - Package: "package-1", - Version: "v1", - }, - }, - Network: config.NetworkConfig{}, - Auth: config.AuthConfig{}, - Debug: true, - DisablePartialStart: true, - } +} - // Create copy of expectedCfg since Reconfigure modifies cfg. - expectedCfg := *cfg - r.Reconfigure(ctx, cfg) - // Assert that Config method returns expected value. - actualCfg = r.Config() - // Assert that default motion and sensor services are still present, but data - // manager default service has been replaced by the "fake1" data manager service. - defaultSvcs = removeDefaultServices(actualCfg) - test.That(t, len(defaultSvcs), test.ShouldEqual, 2) - for _, svc := range defaultSvcs { - test.That(t, svc.API.SubtypeName, test.ShouldBeIn, motion.API.SubtypeName, - sensors.API.SubtypeName) - } +// Test valid sensor honoring reconfig - // Manually inspect remaining service resources as ordering of config is - // non-deterministic within slices. - test.That(t, len(actualCfg.Services), test.ShouldEqual, 2) - for _, svc := range actualCfg.Services { - isFake1DM := svc.Equals(expectedCfg.Services[0]) - isBuiltinNav := svc.Equals(expectedCfg.Services[1]) - test.That(t, isFake1DM || isBuiltinNav, test.ShouldBeTrue) - } - actualCfg.Services = nil - expectedCfg.Services = nil +// Test remote sensor working - // Manually inspect component resources as ordering of config is - // non-deterministic within slices - test.That(t, len(actualCfg.Components), test.ShouldEqual, 3) - for _, comp := range actualCfg.Components { - isMyBase := comp.Equals(expectedCfg.Components[0]) - isMotor1 := comp.Equals(expectedCfg.Components[1]) - isMotor2 := comp.Equals(expectedCfg.Components[2]) - test.That(t, isMyBase || isMotor1 || isMotor2, test.ShouldBeTrue) - } - actualCfg.Components = nil - expectedCfg.Components = nil +// Test main and remote have conflicting names - // Manually inspect remote resources, modules, and processes as Equals should be used - // (alreadyValidated will have been set to true). - test.That(t, len(actualCfg.Remotes), test.ShouldEqual, 1) - test.That(t, actualCfg.Remotes[0].Equals(expectedCfg.Remotes[0]), test.ShouldBeTrue) - actualCfg.Remotes = nil - expectedCfg.Remotes = nil - test.That(t, len(actualCfg.Processes), test.ShouldEqual, 1) - test.That(t, actualCfg.Processes[0].Equals(expectedCfg.Processes[0]), test.ShouldBeTrue) - actualCfg.Processes = nil - expectedCfg.Processes = nil - test.That(t, len(actualCfg.Modules), test.ShouldEqual, 1) - test.That(t, actualCfg.Modules[0].Equals(expectedCfg.Modules[0]), test.ShouldBeTrue) - actualCfg.Modules = nil - expectedCfg.Modules = nil +// Test conflicting names but specify remote name - test.That(t, actualCfg, test.ShouldResemble, &expectedCfg) -} \ No newline at end of file +// Test conflicting multiple remotes with conflicting names diff --git a/testutils/inject/sensor.go b/testutils/inject/sensor.go index e69c3f0106f..ae049773a00 100644 --- a/testutils/inject/sensor.go +++ b/testutils/inject/sensor.go @@ -12,6 +12,7 @@ type Sensor struct { sensor.Sensor name resource.Name DoFunc func(ctx context.Context, cmd map[string]interface{}) (map[string]interface{}, error) + CloseFunc func(ctx context.Context) error ReadingsFunc func(ctx context.Context, extra map[string]interface{}) (map[string]interface{}, error) } @@ -25,6 +26,17 @@ func (s *Sensor) Name() resource.Name { return s.name } +// Close calls the injected Close or the real version. +func (s *Sensor) Close(ctx context.Context) error { + if s.CloseFunc == nil { + if s.Sensor == nil { + return nil + } + return s.Sensor.Close(ctx) + } + return s.CloseFunc(ctx) +} + // Readings calls the injected Readings or the real version. func (s *Sensor) Readings(ctx context.Context, extra map[string]interface{}) (map[string]interface{}, error) { if s.ReadingsFunc == nil { From 7660bbaac1bda0725a04c5b81cd8cee8e569ad92 Mon Sep 17 00:00:00 2001 From: Kschappacher <56745262+Kschappacher@users.noreply.github.com> Date: Thu, 10 Oct 2024 15:40:38 -0400 Subject: [PATCH 08/21] add remote testings --- config/proto_conversions.go | 12 +- config/proto_conversions_test.go | 21 +-- robot/impl/local_robot_test.go | 250 +++++++++++++++++++++++-------- 3 files changed, 204 insertions(+), 79 deletions(-) diff --git a/config/proto_conversions.go b/config/proto_conversions.go index 916237a6939..01b2e65b273 100644 --- a/config/proto_conversions.go +++ b/config/proto_conversions.go @@ -644,10 +644,14 @@ func NetworkConfigToProto(network *NetworkConfig) (*pb.NetworkConfig, error) { // MaintenanceConfigToProto converts MaintenanceConfig from the proto equivalent. func MaintenanceConfigToProto(maintenanceConfig *MaintenanceConfig) (*pb.MaintenanceConfig, error) { - name := resource.NewName(resource.APINamespaceRDK.WithComponentType("sensor"), maintenanceConfig.SensorName) + // convert from string to resource name + name, err := resource.NewFromString(maintenanceConfig.SensorName) + if err != nil { + return nil, err + } proto := pb.MaintenanceConfig{ - SensorName: protoRdkUtils.ResourceNameToProto(name), + SensorName: protoRdkUtils.ResourceNameToProto(name), MaintenanceAllowedKey: maintenanceConfig.MaintenanceAllowedKey, } @@ -668,10 +672,10 @@ func NetworkConfigFromProto(proto *pb.NetworkConfig) (*NetworkConfig, error) { return &network, nil } -// MaintenanceConfigFromProto creates a MaintenanceConfig from the proto equivalent +// MaintenanceConfigFromProto creates a MaintenanceConfig from the proto equivalent. func MaintenanceConfigFromProto(proto *pb.MaintenanceConfig) (*MaintenanceConfig, error) { MaintenanceConfig := MaintenanceConfig{ - SensorName:protoRdkUtils.ResourceNameFromProto(proto.SensorName).ShortName(), + SensorName: protoRdkUtils.ResourceNameFromProto(proto.SensorName).String(), MaintenanceAllowedKey: proto.GetMaintenanceAllowedKey(), } return &MaintenanceConfig, nil diff --git a/config/proto_conversions_test.go b/config/proto_conversions_test.go index 88e4da7259a..3f137b99ef7 100644 --- a/config/proto_conversions_test.go +++ b/config/proto_conversions_test.go @@ -1007,8 +1007,8 @@ func TestPackageTypeConversion(t *testing.T) { } func TestMaintenanceConfigToProtoSuccess(t *testing.T) { - testMaintenanceConfig:= MaintenanceConfig{ - SensorName: "car", + testMaintenanceConfig := MaintenanceConfig{ + SensorName: "rdk:component:sensor/car", MaintenanceAllowedKey: "honk", } @@ -1021,8 +1021,8 @@ func TestMaintenanceConfigToProtoSuccess(t *testing.T) { } func TestMaintenanceConfigToProtoRemoteSuccess(t *testing.T) { - testMaintenanceConfig:= MaintenanceConfig{ - SensorName: "car:go:store", + testMaintenanceConfig := MaintenanceConfig{ + SensorName: "rdk:component:sensor/go:store", MaintenanceAllowedKey: "fast", } @@ -1035,16 +1035,11 @@ func TestMaintenanceConfigToProtoRemoteSuccess(t *testing.T) { } func TestMaintenanceConfigToProtoFailure(t *testing.T) { - testMaintenanceConfig:= MaintenanceConfig{ - SensorName: "car:go", + testMaintenanceConfig := MaintenanceConfig{ + SensorName: "car:go", MaintenanceAllowedKey: "slow", } - proto, err := MaintenanceConfigToProto(&testMaintenanceConfig) - test.That(t, err, test.ShouldBeNil) - out, err := MaintenanceConfigFromProto(proto) - test.That(t, err, test.ShouldBeNil) - - test.That(t, *out, test.ShouldResemble, testMaintenanceConfig) + _, err := MaintenanceConfigToProto(&testMaintenanceConfig) + test.That(t, err.Error(), test.ShouldEqual, "string \"car:go\" is not a fully qualified resource name") } - diff --git a/robot/impl/local_robot_test.go b/robot/impl/local_robot_test.go index 3770d0d6f24..736c5a0a486 100644 --- a/robot/impl/local_robot_test.go +++ b/robot/impl/local_robot_test.go @@ -4036,8 +4036,11 @@ func newErrorSensor() sensor.Sensor { return s } -func TestMaintenanceConfigWithRemotes(t *testing.T) { +func TestMaintenanceConfigWithRemotesAndFragments(t *testing.T) { + ctx := context.Background() + logger := logging.NewTestLogger(t) model := resource.DefaultModelFamily.WithModel(utils.RandomAlphaString(8)) + modelErrorSensor := resource.DefaultModelFamily.WithModel(utils.RandomAlphaString(8)) resource.RegisterComponent( sensor.API, model, @@ -4049,16 +4052,21 @@ func TestMaintenanceConfigWithRemotes(t *testing.T) { ) (sensor.Sensor, error) { return newValidSensor(), nil }}) + resource.RegisterComponent( + sensor.API, + modelErrorSensor, + resource.Registration[sensor.Sensor, resource.NoNativeConfig]{Constructor: func( + ctx context.Context, + deps resource.Dependencies, + conf resource.Config, + logger logging.Logger, + ) (sensor.Sensor, error) { + return newErrorSensor(), nil + }}) defer func() { resource.Deregister(sensor.API, model) }() - - t.Run("robot reconfigures with maintenanceConfig sensor and blocks reconfigure, reconfigure reenabled when maintenanceConfig removed", func(t *testing.T) { - ctx := context.Background() - logger := logging.NewTestLogger(t) - r := setupLocalRobot(t, context.Background(), &config.Config{}, logger) - cfg := &config.Config{ - MaintenanceConfig: &config.MaintenanceConfig{SensorName: "rdk:component:sensor/sensor",MaintenanceAllowedKey:"ThatsMyWallet"}, + remoteCfg := &config.Config{ Components: []resource.Config{ { Name: "sensor", @@ -4067,69 +4075,187 @@ func TestMaintenanceConfigWithRemotes(t *testing.T) { }, }, } - r.Reconfigure(ctx, cfg) + sensor1 := []resource.Config{ + { + Name: "sensor", + Model: model, + API: sensor.API, + }, + } + sensor2 := []resource.Config{ + { + Name: "sensor2", + API: sensor.API, + Model: model, + }, + } + // This needs to share a name with sensor so name colisions can be tested + errorSensor := []resource.Config { + { + Name: "sensor", + API: sensor.API, + Model: modelErrorSensor, + }, + } + cfgBlocked := &config.Config{ + MaintenanceConfig: &config.MaintenanceConfig{SensorName:"rdk:component:sensor/sensor",MaintenanceAllowedKey:"ThatsMyWallet"}, + Components: sensor2, + } + + + t.Run("robot reconfigures with maintenanceConfig sensor and blocks reconfigure, reconfigure reenabled when maintenanceConfig removed", func(t *testing.T) { + cfg := &config.Config{ + MaintenanceConfig: &config.MaintenanceConfig{SensorName: "rdk:component:sensor/sensor",MaintenanceAllowedKey:"ThatsMyWallet"}, + Components: sensor1, + } + cfgUnblock := &config.Config{ + Components: sensor2, + } + + r := setupLocalRobot(t, context.Background(), cfg, logger) sensorResource, err:= r.ResourceByName(sensor.Named("sensor")) test.That(t, err, test.ShouldBeNil) test.That(t, sensorResource, test.ShouldNotBeNil) - // cfgBlocked := &config.Config{ - // MaintenanceConfig: &config.MaintenanceConfig{SensorName:"sensor",MaintenanceAllowedKey:"ThatsMyWallet"}, - // Components: []resource.Config{ - // { - // Name: "sensor2", - // API: sensor.API, - // Model: model, - // }, - // }, - // } - // r.Reconfigure(ctx, cfgBlocked) - // sensorBlocked, err:= r.ResourceByName(sensor.Named("sensor2")) - // test.That(t, sensorBlocked, test.ShouldBeNil) - // test.That(t, err.Error(),test.ShouldEqual, "resource \"rdk:component:sensor/sensor2\" not found") - // cfgUnblock := &config.Config{ - // Components: []resource.Config{ - // { - // Name: "sensor2", - // API: sensor.API, - // Model: model, - // }, - // }, - // } - // r.Reconfigure(ctx, cfgUnblock) - // sensorBlocked, err = r.ResourceByName(sensor.Named("sensor2")) - // test.That(t, err,test.ShouldBeNil) - // test.That(t, sensorBlocked, test.ShouldNotBeNil) + // Maintenance sensor will block reconfig so sensor2 should not be added + r.Reconfigure(ctx, cfgBlocked) + sensorBlocked, err:= r.ResourceByName(sensor.Named("sensor2")) + test.That(t, sensorBlocked, test.ShouldBeNil) + test.That(t, err.Error(),test.ShouldEqual, "resource \"rdk:component:sensor/sensor2\" not found") + + // removing maintenance config unblocks reconfig and allows sensor to be added + r.Reconfigure(ctx, cfgUnblock) + sensorBlocked, err = r.ResourceByName(sensor.Named("sensor2")) + test.That(t, err,test.ShouldBeNil) + test.That(t, sensorBlocked, test.ShouldNotBeNil) }) + t.Run("Remote sensor works when remote name is specified and not specified", func(t *testing.T) { + ctx := context.Background() + // Setup remote with maintenance sensor + remote := setupLocalRobot(t, context.Background(), remoteCfg, logger) + options, _, addr := robottestutils.CreateBaseOptionsAndListener(t) + err:= remote.StartWeb(ctx, options) + test.That(t,err,test.ShouldBeNil) + cfg := &config.Config{ + MaintenanceConfig: &config.MaintenanceConfig{SensorName: "rdk:component:sensor/sensor",MaintenanceAllowedKey:"ThatsMyWallet"}, + Remotes: []config.Remote{ + { + Name: "remote", + Insecure: true, + Address: addr, + }, + }, + } + cfgBlocked := &config.Config{ + MaintenanceConfig: &config.MaintenanceConfig{SensorName:"rdk:component:sensor/sensor",MaintenanceAllowedKey:"ThatsMyWallet"}, + Components: sensor2, + } + cfgBlockedWithRemoteSpecified := &config.Config{ + MaintenanceConfig: &config.MaintenanceConfig{SensorName:"rdk:component:sensor/remote:sensor",MaintenanceAllowedKey:"ThatsMyWallet"}, + Components: sensor2, + } - // cfg := &config.Config{ - // MaintenanceConfig: &config.MaintenanceConfig{SensorName:"",MaintenanceAllowedKey:""}, - // Remotes: []config.Remote{ - // { - // Name: "foo", - // Address: addr, - // }, - // }, - // Components: []resource.Config{ - // { - // Name: "sensor", - // API: sensor.API, - // Model: fakeModel, - // }, - // }, - // } - - -} - - + // Setup robot pointing maintenanceConfig at the remote sensor + r := setupLocalRobot(t, context.Background(), cfg, logger) + + // reconfig should be blocked ensure new resource is not added + r.Reconfigure(ctx,cfgBlocked) + sensorBlocked, err:= r.ResourceByName(sensor.Named("sensor2")) + test.That(t, sensorBlocked, test.ShouldBeNil) + test.That(t, err.Error(),test.ShouldEqual, "resource \"rdk:component:sensor/sensor2\" not found") + + // Attempt to reconfig again using remote:sensor name + // Reconfig should still be blocked + r.Reconfigure(ctx,cfgBlockedWithRemoteSpecified) + sensorBlocked, err = r.ResourceByName(sensor.Named("sensor2")) + test.That(t, sensorBlocked, test.ShouldBeNil) + test.That(t, err.Error(),test.ShouldEqual, "resource \"rdk:component:sensor/sensor2\" not found") + }) -// Test valid sensor honoring reconfig + t.Run("conflicting remote and main sensors default to main", func(t *testing.T) { + ctx := context.Background() + // Setup remote with error maintenance sensor, if sensor is ever called it will error and reconfigure normally + remoteErrConfig := &config.Config{ + Components: errorSensor, + } + remote := setupLocalRobot(t, context.Background(), remoteErrConfig, logger) + options, _, addr := robottestutils.CreateBaseOptionsAndListener(t) + err:= remote.StartWeb(ctx, options) + test.That(t,err,test.ShouldBeNil) + cfg := &config.Config{ + MaintenanceConfig: &config.MaintenanceConfig{SensorName: "rdk:component:sensor/sensor",MaintenanceAllowedKey:"ThatsMyWallet"}, + Remotes: []config.Remote{ + { + Name: "remote", + Insecure: true, + Address: addr, + }, + }, + Components:sensor1, + } + cfgRemoteUnblocked := &config.Config{ + MaintenanceConfig: &config.MaintenanceConfig{SensorName:"rdk:component:sensor/remote:sensor",MaintenanceAllowedKey:"ThatsMyWallet"}, + Components: sensor2, + } + + // Setup robot pointing maintenanceConfig with conflicting sensors + r := setupLocalRobot(t, context.Background(), cfg, logger) + + // reconfig should be blocked since the sensor on main if chosen instead of the remote + r.Reconfigure(ctx,cfgBlocked) + sensorBlocked, err:= r.ResourceByName(sensor.Named("sensor2")) + test.That(t, sensorBlocked, test.ShouldBeNil) + test.That(t, err.Error(),test.ShouldEqual, "resource \"rdk:component:sensor/sensor2\" not found") + + + // robot should reconfigure since remote will return an error + r.Reconfigure(ctx, cfgRemoteUnblocked) + sensorBlocked, err = r.ResourceByName(sensor.Named("sensor2")) + test.That(t, err,test.ShouldBeNil) + test.That(t, sensorBlocked, test.ShouldNotBeNil) + }) -// Test remote sensor working -// Test main and remote have conflicting names + t.Run("multiple remotes with conflicting names errors out", func(t *testing.T) { + ctx := context.Background() + // setup two identical remotes + remote := setupLocalRobot(t, context.Background(), remoteCfg, logger) + options, _, addr := robottestutils.CreateBaseOptionsAndListener(t) + err:= remote.StartWeb(ctx, options) + remote2 := setupLocalRobot(t, context.Background(), remoteCfg, logger) + options2, _, addr2 := robottestutils.CreateBaseOptionsAndListener(t) + err = remote2.StartWeb(ctx, options2) + test.That(t,err,test.ShouldBeNil) + cfg := &config.Config{ + MaintenanceConfig: &config.MaintenanceConfig{SensorName: "rdk:component:sensor/sensor",MaintenanceAllowedKey:"ThatsMyWallet"}, + Remotes: []config.Remote{ + { + Name: "remote", + Insecure: true, + Address: addr, + }, + { + Name: "remote2", + Insecure: true, + Address: addr2, + }, + }, + } -// Test conflicting names but specify remote name + cfgUnblocked := &config.Config{ + MaintenanceConfig: &config.MaintenanceConfig{SensorName:"rdk:component:sensor/sensor",MaintenanceAllowedKey:"ThatsMyWallet"}, + Components: sensor2, + } + + // Setup robot pointing maintenanceConfig with conflicting remote sensors + r := setupLocalRobot(t, context.Background(), cfg, logger) + + // reconfig should not be blocked because the two remotes will have conflicting resources resulting in reconfiguring + r.Reconfigure(ctx, cfgUnblocked) + sensorUnBlocked, err := r.ResourceByName(sensor.Named("sensor2")) + test.That(t, err,test.ShouldBeNil) + test.That(t, sensorUnBlocked, test.ShouldNotBeNil) -// Test conflicting multiple remotes with conflicting names + }) +} From b20cfdaa49e2ed7d26417ac809f8074c51bba854 Mon Sep 17 00:00:00 2001 From: Kschappacher <56745262+Kschappacher@users.noreply.github.com> Date: Fri, 11 Oct 2024 10:58:56 -0400 Subject: [PATCH 09/21] clean up code --- robot/impl/local_robot.go | 28 ++++++++++++++-------------- robot/impl/local_robot_test.go | 7 +++---- 2 files changed, 17 insertions(+), 18 deletions(-) diff --git a/robot/impl/local_robot.go b/robot/impl/local_robot.go index a7a0cb634f2..b57ad751ff4 100644 --- a/robot/impl/local_robot.go +++ b/robot/impl/local_robot.go @@ -1188,6 +1188,7 @@ func (r *localRobot) reconfigure(ctx context.Context, newConfig *config.Config, // These sensors can be configured on the main robot, a fragment, or a remote // In situations where there are conflicting sensor names the following behavior happens // Main robot and remote share sensor name -> main robot sensor is chosen + // Main robot and fragment share sensor name -> main robot sensor is chosen // Only remote has the sensor name -> remote sensor is read // Multiple remotes share a senor name -> conflict error is returned and reconfigure happens // To specify a specific remote sensor use the name format remoteName:sensorName to specify a remote sensor @@ -1196,21 +1197,20 @@ func (r *localRobot) reconfigure(ctx context.Context, newConfig *config.Config, if err!=nil { r.logger.Infof("Sensor Name %s is not in a supported format",newConfig.MaintenanceConfig.SensorName ) } else { - sensorComponent, err := robot.ResourceFromRobot[sensor.Sensor](r, name) - if err != nil { - r.logger.Infof("%s, Starting reconfiguration", err.Error()) - } else { - canReconfigure, err := r.checkMaintenanceSensorReadings(newConfig.MaintenanceConfig.MaintenanceAllowedKey, sensorComponent) + sensorComponent, err := robot.ResourceFromRobot[sensor.Sensor](r, name) if err != nil { - r.logger.Info(err.Error() + ". Starting reconfiguration") - } else { - if !canReconfigure { - r.logger.Info("maintenanceAllowedKey found from readings on maintenance sensor. Reconfigure disabled") - return + r.logger.Infof("%s, Starting reconfiguration", err.Error()) } else { - r.logger.Info("maintenanceAllowedKey found from readings on maintenance sensor. Starting reconfiguration") - - } + canReconfigure, err := r.checkMaintenanceSensorReadings(newConfig.MaintenanceConfig.MaintenanceAllowedKey, sensorComponent) + if err != nil { + r.logger.Info(err.Error() + ". Starting reconfiguration") + } else { + if !canReconfigure { + r.logger.Info("maintenanceAllowedKey found from readings on maintenance sensor. Reconfigure disabled") + return + } else { + r.logger.Info("maintenanceAllowedKey found from readings on maintenance sensor. Starting reconfiguration") + } } } } @@ -1475,7 +1475,7 @@ func (r *localRobot) checkMaintenanceSensorReadings(maintenanceAllowedKey string ctx, cancel := context.WithTimeout(context.Background(), timeout) defer cancel() - // Timeouts on this call should be handled grpc and return + // Context timeouts on this call should be handled by grpc readings, err := sensor.Readings(ctx, map[string]interface{}{}) if err != nil { return true, errors.Errorf("error reading maintenance sensor readings. %s", err.Error()) diff --git a/robot/impl/local_robot_test.go b/robot/impl/local_robot_test.go index 736c5a0a486..7b651b86033 100644 --- a/robot/impl/local_robot_test.go +++ b/robot/impl/local_robot_test.go @@ -4036,7 +4036,7 @@ func newErrorSensor() sensor.Sensor { return s } -func TestMaintenanceConfigWithRemotesAndFragments(t *testing.T) { +func TestMaintenanceConfig(t *testing.T) { ctx := context.Background() logger := logging.NewTestLogger(t) model := resource.DefaultModelFamily.WithModel(utils.RandomAlphaString(8)) @@ -4102,8 +4102,7 @@ func TestMaintenanceConfigWithRemotesAndFragments(t *testing.T) { Components: sensor2, } - - t.Run("robot reconfigures with maintenanceConfig sensor and blocks reconfigure, reconfigure reenabled when maintenanceConfig removed", func(t *testing.T) { + t.Run("maintenanceConfig sensor blocks reconfigure, reconfigure reenabled when maintenanceConfig removed", func(t *testing.T) { cfg := &config.Config{ MaintenanceConfig: &config.MaintenanceConfig{SensorName: "rdk:component:sensor/sensor",MaintenanceAllowedKey:"ThatsMyWallet"}, Components: sensor1, @@ -4130,7 +4129,7 @@ func TestMaintenanceConfigWithRemotesAndFragments(t *testing.T) { test.That(t, sensorBlocked, test.ShouldNotBeNil) }) - t.Run("Remote sensor works when remote name is specified and not specified", func(t *testing.T) { + t.Run("remote sensor successfully blocks reconfigure when remote name is specified and not specified", func(t *testing.T) { ctx := context.Background() // Setup remote with maintenance sensor remote := setupLocalRobot(t, context.Background(), remoteCfg, logger) From b11af36a7c712aa1337b489a23a4c87b5edf5ca0 Mon Sep 17 00:00:00 2001 From: Kschappacher <56745262+Kschappacher@users.noreply.github.com> Date: Fri, 11 Oct 2024 11:10:03 -0400 Subject: [PATCH 10/21] lint and clean up code --- config/proto_conversions.go | 3 +- robot/impl/local_robot.go | 23 ++-- robot/impl/local_robot_test.go | 216 ++++++++++++++++----------------- testutils/inject/sensor.go | 2 +- 4 files changed, 120 insertions(+), 124 deletions(-) diff --git a/config/proto_conversions.go b/config/proto_conversions.go index 01b2e65b273..fb252a6286b 100644 --- a/config/proto_conversions.go +++ b/config/proto_conversions.go @@ -9,13 +9,13 @@ import ( "github.com/pkg/errors" packagespb "go.viam.com/api/app/packages/v1" pb "go.viam.com/api/app/v1" - protoRdkUtils "go.viam.com/rdk/protoutils" "go.viam.com/utils/pexec" "go.viam.com/utils/protoutils" "go.viam.com/utils/rpc" "google.golang.org/protobuf/types/known/durationpb" "go.viam.com/rdk/logging" + protoRdkUtils "go.viam.com/rdk/protoutils" "go.viam.com/rdk/referenceframe" "go.viam.com/rdk/resource" spatial "go.viam.com/rdk/spatialmath" @@ -672,6 +672,7 @@ func NetworkConfigFromProto(proto *pb.NetworkConfig) (*NetworkConfig, error) { return &network, nil } + // MaintenanceConfigFromProto creates a MaintenanceConfig from the proto equivalent. func MaintenanceConfigFromProto(proto *pb.MaintenanceConfig) (*MaintenanceConfig, error) { MaintenanceConfig := MaintenanceConfig{ diff --git a/robot/impl/local_robot.go b/robot/impl/local_robot.go index b57ad751ff4..e77765ef720 100644 --- a/robot/impl/local_robot.go +++ b/robot/impl/local_robot.go @@ -1193,28 +1193,27 @@ func (r *localRobot) reconfigure(ctx context.Context, newConfig *config.Config, // Multiple remotes share a senor name -> conflict error is returned and reconfigure happens // To specify a specific remote sensor use the name format remoteName:sensorName to specify a remote sensor if newConfig.MaintenanceConfig != nil { - name,err:=resource.NewFromString(newConfig.MaintenanceConfig.SensorName) - if err!=nil { - r.logger.Infof("Sensor Name %s is not in a supported format",newConfig.MaintenanceConfig.SensorName ) + name, err := resource.NewFromString(newConfig.MaintenanceConfig.SensorName) + if err != nil { + r.logger.Infof("Sensor Name %s is not in a supported format", newConfig.MaintenanceConfig.SensorName) } else { sensorComponent, err := robot.ResourceFromRobot[sensor.Sensor](r, name) if err != nil { r.logger.Infof("%s, Starting reconfiguration", err.Error()) } else { canReconfigure, err := r.checkMaintenanceSensorReadings(newConfig.MaintenanceConfig.MaintenanceAllowedKey, sensorComponent) + if !canReconfigure { + r.logger.Info("maintenanceAllowedKey found from readings on maintenance sensor. Reconfigure disabled") + return + } if err != nil { r.logger.Info(err.Error() + ". Starting reconfiguration") } else { - if !canReconfigure { - r.logger.Info("maintenanceAllowedKey found from readings on maintenance sensor. Reconfigure disabled") - return - } else { - r.logger.Info("maintenanceAllowedKey found from readings on maintenance sensor. Starting reconfiguration") - } - } + r.logger.Info("maintenanceAllowedKey found from readings on maintenance sensor. Starting reconfiguration") + } + } } } - } // Sync Packages before reconfiguring rest of robot and resolving references to any packages // in the config. @@ -1469,7 +1468,7 @@ func (r *localRobot) Version(ctx context.Context) (robot.VersionResponse, error) return robot.Version() } -// checkMaintenanceSensorReadings ensures that errors from reading a sensor are handled properly +// checkMaintenanceSensorReadings ensures that errors from reading a sensor are handled properly. func (r *localRobot) checkMaintenanceSensorReadings(maintenanceAllowedKey string, sensor resource.Sensor) (bool, error) { timeout := 5 * time.Second ctx, cancel := context.WithTimeout(context.Background(), timeout) diff --git a/robot/impl/local_robot_test.go b/robot/impl/local_robot_test.go index ce964b8e33d..8c67c043d26 100644 --- a/robot/impl/local_robot_test.go +++ b/robot/impl/local_robot_test.go @@ -20,7 +20,6 @@ import ( "github.com/golang/geo/r3" "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" @@ -3976,7 +3975,7 @@ func newValidSensor() sensor.Sensor { s.ReadingsFunc = func(ctx context.Context, extra map[string]interface{}) (map[string]interface{}, error) { return map[string]any{"ThatsMyWallet": false, "ThatsNotMyWallet": true, "ThatIsNotAWallet": 5}, nil } - s.CloseFunc = func(ctx context.Context) error {return nil} + s.CloseFunc = func(ctx context.Context) error { return nil } return s } @@ -4004,80 +4003,80 @@ func TestMaintenanceConfig(t *testing.T) { ) (sensor.Sensor, error) { return newValidSensor(), nil }}) - resource.RegisterComponent( - sensor.API, - modelErrorSensor, - resource.Registration[sensor.Sensor, resource.NoNativeConfig]{Constructor: func( - ctx context.Context, - deps resource.Dependencies, - conf resource.Config, - logger logging.Logger, - ) (sensor.Sensor, error) { - return newErrorSensor(), nil - }}) - defer func() { - resource.Deregister(sensor.API, model) - }() - remoteCfg := &config.Config{ - Components: []resource.Config{ - { - Name: "sensor", - Model: model, - API: sensor.API, - }, - }, - } - sensor1 := []resource.Config{ - { - Name: "sensor", - Model: model, - API: sensor.API, - }, - } - sensor2 := []resource.Config{ - { - Name: "sensor2", - API: sensor.API, - Model: model, - }, - } - // This needs to share a name with sensor so name colisions can be tested - errorSensor := []resource.Config { + resource.RegisterComponent( + sensor.API, + modelErrorSensor, + resource.Registration[sensor.Sensor, resource.NoNativeConfig]{Constructor: func( + ctx context.Context, + deps resource.Dependencies, + conf resource.Config, + logger logging.Logger, + ) (sensor.Sensor, error) { + return newErrorSensor(), nil + }}) + defer func() { + resource.Deregister(sensor.API, model) + }() + remoteCfg := &config.Config{ + Components: []resource.Config{ { - Name: "sensor", - API: sensor.API, - Model: modelErrorSensor, + Name: "sensor", + Model: model, + API: sensor.API, }, - } - cfgBlocked := &config.Config{ - MaintenanceConfig: &config.MaintenanceConfig{SensorName:"rdk:component:sensor/sensor",MaintenanceAllowedKey:"ThatsMyWallet"}, - Components: sensor2, - } + }, + } + sensor1 := []resource.Config{ + { + Name: "sensor", + Model: model, + API: sensor.API, + }, + } + sensor2 := []resource.Config{ + { + Name: "sensor2", + API: sensor.API, + Model: model, + }, + } + // This needs to share a name with sensor so name colisions can be tested + errorSensor := []resource.Config{ + { + Name: "sensor", + API: sensor.API, + Model: modelErrorSensor, + }, + } + cfgBlocked := &config.Config{ + MaintenanceConfig: &config.MaintenanceConfig{SensorName: "rdk:component:sensor/sensor", MaintenanceAllowedKey: "ThatsMyWallet"}, + Components: sensor2, + } t.Run("maintenanceConfig sensor blocks reconfigure, reconfigure reenabled when maintenanceConfig removed", func(t *testing.T) { cfg := &config.Config{ - MaintenanceConfig: &config.MaintenanceConfig{SensorName: "rdk:component:sensor/sensor",MaintenanceAllowedKey:"ThatsMyWallet"}, - Components: sensor1, + MaintenanceConfig: &config.MaintenanceConfig{SensorName: "rdk:component:sensor/sensor", MaintenanceAllowedKey: "ThatsMyWallet"}, + Components: sensor1, } cfgUnblock := &config.Config{ Components: sensor2, } r := setupLocalRobot(t, context.Background(), cfg, logger) - sensorResource, err:= r.ResourceByName(sensor.Named("sensor")) + sensorResource, err := r.ResourceByName(sensor.Named("sensor")) test.That(t, err, test.ShouldBeNil) test.That(t, sensorResource, test.ShouldNotBeNil) // Maintenance sensor will block reconfig so sensor2 should not be added r.Reconfigure(ctx, cfgBlocked) - sensorBlocked, err:= r.ResourceByName(sensor.Named("sensor2")) + sensorBlocked, err := r.ResourceByName(sensor.Named("sensor2")) test.That(t, sensorBlocked, test.ShouldBeNil) - test.That(t, err.Error(),test.ShouldEqual, "resource \"rdk:component:sensor/sensor2\" not found") + test.That(t, err.Error(), test.ShouldEqual, "resource \"rdk:component:sensor/sensor2\" not found") // removing maintenance config unblocks reconfig and allows sensor to be added r.Reconfigure(ctx, cfgUnblock) sensorBlocked, err = r.ResourceByName(sensor.Named("sensor2")) - test.That(t, err,test.ShouldBeNil) + test.That(t, err, test.ShouldBeNil) test.That(t, sensorBlocked, test.ShouldNotBeNil) }) @@ -4086,127 +4085,124 @@ func TestMaintenanceConfig(t *testing.T) { // Setup remote with maintenance sensor remote := setupLocalRobot(t, context.Background(), remoteCfg, logger) options, _, addr := robottestutils.CreateBaseOptionsAndListener(t) - err:= remote.StartWeb(ctx, options) - test.That(t,err,test.ShouldBeNil) + err := remote.StartWeb(ctx, options) + test.That(t, err, test.ShouldBeNil) cfg := &config.Config{ - MaintenanceConfig: &config.MaintenanceConfig{SensorName: "rdk:component:sensor/sensor",MaintenanceAllowedKey:"ThatsMyWallet"}, + MaintenanceConfig: &config.MaintenanceConfig{SensorName: "rdk:component:sensor/sensor", MaintenanceAllowedKey: "ThatsMyWallet"}, Remotes: []config.Remote{ { - Name: "remote", - Insecure: true, - Address: addr, + Name: "remote", + Insecure: true, + Address: addr, + }, }, - }, } cfgBlocked := &config.Config{ - MaintenanceConfig: &config.MaintenanceConfig{SensorName:"rdk:component:sensor/sensor",MaintenanceAllowedKey:"ThatsMyWallet"}, - Components: sensor2, + MaintenanceConfig: &config.MaintenanceConfig{SensorName: "rdk:component:sensor/sensor", MaintenanceAllowedKey: "ThatsMyWallet"}, + Components: sensor2, } cfgBlockedWithRemoteSpecified := &config.Config{ - MaintenanceConfig: &config.MaintenanceConfig{SensorName:"rdk:component:sensor/remote:sensor",MaintenanceAllowedKey:"ThatsMyWallet"}, - Components: sensor2, + MaintenanceConfig: &config.MaintenanceConfig{SensorName: "rdk:component:sensor/remote:sensor", MaintenanceAllowedKey: "ThatsMyWallet"}, + Components: sensor2, } // Setup robot pointing maintenanceConfig at the remote sensor r := setupLocalRobot(t, context.Background(), cfg, logger) - + // reconfig should be blocked ensure new resource is not added - r.Reconfigure(ctx,cfgBlocked) - sensorBlocked, err:= r.ResourceByName(sensor.Named("sensor2")) + r.Reconfigure(ctx, cfgBlocked) + sensorBlocked, err := r.ResourceByName(sensor.Named("sensor2")) test.That(t, sensorBlocked, test.ShouldBeNil) - test.That(t, err.Error(),test.ShouldEqual, "resource \"rdk:component:sensor/sensor2\" not found") + test.That(t, err.Error(), test.ShouldEqual, "resource \"rdk:component:sensor/sensor2\" not found") // Attempt to reconfig again using remote:sensor name // Reconfig should still be blocked - r.Reconfigure(ctx,cfgBlockedWithRemoteSpecified) + r.Reconfigure(ctx, cfgBlockedWithRemoteSpecified) sensorBlocked, err = r.ResourceByName(sensor.Named("sensor2")) test.That(t, sensorBlocked, test.ShouldBeNil) - test.That(t, err.Error(),test.ShouldEqual, "resource \"rdk:component:sensor/sensor2\" not found") + test.That(t, err.Error(), test.ShouldEqual, "resource \"rdk:component:sensor/sensor2\" not found") }) t.Run("conflicting remote and main sensors default to main", func(t *testing.T) { ctx := context.Background() - // Setup remote with error maintenance sensor, if sensor is ever called it will error and reconfigure normally + // Setup remote with error maintenance sensor, if sensor is ever called it will error and reconfigure normally remoteErrConfig := &config.Config{ Components: errorSensor, } remote := setupLocalRobot(t, context.Background(), remoteErrConfig, logger) options, _, addr := robottestutils.CreateBaseOptionsAndListener(t) - err:= remote.StartWeb(ctx, options) - test.That(t,err,test.ShouldBeNil) + err := remote.StartWeb(ctx, options) + test.That(t, err, test.ShouldBeNil) cfg := &config.Config{ - MaintenanceConfig: &config.MaintenanceConfig{SensorName: "rdk:component:sensor/sensor",MaintenanceAllowedKey:"ThatsMyWallet"}, + MaintenanceConfig: &config.MaintenanceConfig{SensorName: "rdk:component:sensor/sensor", MaintenanceAllowedKey: "ThatsMyWallet"}, Remotes: []config.Remote{ { - Name: "remote", - Insecure: true, - Address: addr, + Name: "remote", + Insecure: true, + Address: addr, + }, }, - }, - Components:sensor1, + Components: sensor1, } cfgRemoteUnblocked := &config.Config{ - MaintenanceConfig: &config.MaintenanceConfig{SensorName:"rdk:component:sensor/remote:sensor",MaintenanceAllowedKey:"ThatsMyWallet"}, - Components: sensor2, + MaintenanceConfig: &config.MaintenanceConfig{SensorName: "rdk:component:sensor/remote:sensor", MaintenanceAllowedKey: "ThatsMyWallet"}, + Components: sensor2, } - + // Setup robot pointing maintenanceConfig with conflicting sensors r := setupLocalRobot(t, context.Background(), cfg, logger) - + // reconfig should be blocked since the sensor on main if chosen instead of the remote - r.Reconfigure(ctx,cfgBlocked) - sensorBlocked, err:= r.ResourceByName(sensor.Named("sensor2")) + r.Reconfigure(ctx, cfgBlocked) + sensorBlocked, err := r.ResourceByName(sensor.Named("sensor2")) test.That(t, sensorBlocked, test.ShouldBeNil) - test.That(t, err.Error(),test.ShouldEqual, "resource \"rdk:component:sensor/sensor2\" not found") + test.That(t, err.Error(), test.ShouldEqual, "resource \"rdk:component:sensor/sensor2\" not found") - // robot should reconfigure since remote will return an error r.Reconfigure(ctx, cfgRemoteUnblocked) sensorBlocked, err = r.ResourceByName(sensor.Named("sensor2")) - test.That(t, err,test.ShouldBeNil) + test.That(t, err, test.ShouldBeNil) test.That(t, sensorBlocked, test.ShouldNotBeNil) }) - - t.Run("multiple remotes with conflicting names errors out", func(t *testing.T) { ctx := context.Background() // setup two identical remotes remote := setupLocalRobot(t, context.Background(), remoteCfg, logger) options, _, addr := robottestutils.CreateBaseOptionsAndListener(t) - err:= remote.StartWeb(ctx, options) + err := remote.StartWeb(ctx, options) + test.That(t, err, test.ShouldBeNil) remote2 := setupLocalRobot(t, context.Background(), remoteCfg, logger) options2, _, addr2 := robottestutils.CreateBaseOptionsAndListener(t) err = remote2.StartWeb(ctx, options2) - test.That(t,err,test.ShouldBeNil) + test.That(t, err, test.ShouldBeNil) cfg := &config.Config{ - MaintenanceConfig: &config.MaintenanceConfig{SensorName: "rdk:component:sensor/sensor",MaintenanceAllowedKey:"ThatsMyWallet"}, + MaintenanceConfig: &config.MaintenanceConfig{SensorName: "rdk:component:sensor/sensor", MaintenanceAllowedKey: "ThatsMyWallet"}, Remotes: []config.Remote{ { - Name: "remote", - Insecure: true, - Address: addr, - }, - { - Name: "remote2", - Insecure: true, - Address: addr2, + Name: "remote", + Insecure: true, + Address: addr, + }, + { + Name: "remote2", + Insecure: true, + Address: addr2, + }, }, - }, } cfgUnblocked := &config.Config{ - MaintenanceConfig: &config.MaintenanceConfig{SensorName:"rdk:component:sensor/sensor",MaintenanceAllowedKey:"ThatsMyWallet"}, - Components: sensor2, + MaintenanceConfig: &config.MaintenanceConfig{SensorName: "rdk:component:sensor/sensor", MaintenanceAllowedKey: "ThatsMyWallet"}, + Components: sensor2, } - + // Setup robot pointing maintenanceConfig with conflicting remote sensors r := setupLocalRobot(t, context.Background(), cfg, logger) - + // reconfig should not be blocked because the two remotes will have conflicting resources resulting in reconfiguring r.Reconfigure(ctx, cfgUnblocked) sensorUnBlocked, err := r.ResourceByName(sensor.Named("sensor2")) - test.That(t, err,test.ShouldBeNil) + test.That(t, err, test.ShouldBeNil) test.That(t, sensorUnBlocked, test.ShouldNotBeNil) - }) } diff --git a/testutils/inject/sensor.go b/testutils/inject/sensor.go index ae049773a00..6b0ac0e4c5d 100644 --- a/testutils/inject/sensor.go +++ b/testutils/inject/sensor.go @@ -12,7 +12,7 @@ type Sensor struct { sensor.Sensor name resource.Name DoFunc func(ctx context.Context, cmd map[string]interface{}) (map[string]interface{}, error) - CloseFunc func(ctx context.Context) error + CloseFunc func(ctx context.Context) error ReadingsFunc func(ctx context.Context, extra map[string]interface{}) (map[string]interface{}, error) } From 10ebeeb0c65fb6f4efa7c9fc97952b866a402631 Mon Sep 17 00:00:00 2001 From: Kschappacher <56745262+Kschappacher@users.noreply.github.com> Date: Fri, 11 Oct 2024 15:58:58 -0400 Subject: [PATCH 11/21] address pr comments --- robot/impl/local_robot.go | 13 +++++++------ robot/impl/local_robot_test.go | 9 +++++---- 2 files changed, 12 insertions(+), 10 deletions(-) diff --git a/robot/impl/local_robot.go b/robot/impl/local_robot.go index e77765ef720..ef3512bb5a1 100644 --- a/robot/impl/local_robot.go +++ b/robot/impl/local_robot.go @@ -1195,19 +1195,19 @@ func (r *localRobot) reconfigure(ctx context.Context, newConfig *config.Config, if newConfig.MaintenanceConfig != nil { name, err := resource.NewFromString(newConfig.MaintenanceConfig.SensorName) if err != nil { - r.logger.Infof("Sensor Name %s is not in a supported format", newConfig.MaintenanceConfig.SensorName) + r.logger.Warnf("Sensor Name %s is not in a supported format", newConfig.MaintenanceConfig.SensorName) } else { sensorComponent, err := robot.ResourceFromRobot[sensor.Sensor](r, name) if err != nil { - r.logger.Infof("%s, Starting reconfiguration", err.Error()) + r.logger.Warnf("%s, Starting reconfiguration", err.Error()) } else { - canReconfigure, err := r.checkMaintenanceSensorReadings(newConfig.MaintenanceConfig.MaintenanceAllowedKey, sensorComponent) + canReconfigure, err := r.checkMaintenanceSensorReadings(ctx, newConfig.MaintenanceConfig.MaintenanceAllowedKey, sensorComponent) if !canReconfigure { r.logger.Info("maintenanceAllowedKey found from readings on maintenance sensor. Reconfigure disabled") return } if err != nil { - r.logger.Info(err.Error() + ". Starting reconfiguration") + r.logger.Warn(err.Error() + ". Starting reconfiguration") } else { r.logger.Info("maintenanceAllowedKey found from readings on maintenance sensor. Starting reconfiguration") } @@ -1469,9 +1469,10 @@ func (r *localRobot) Version(ctx context.Context) (robot.VersionResponse, error) } // checkMaintenanceSensorReadings ensures that errors from reading a sensor are handled properly. -func (r *localRobot) checkMaintenanceSensorReadings(maintenanceAllowedKey string, sensor resource.Sensor) (bool, error) { +func (r *localRobot) checkMaintenanceSensorReadings(ctx context.Context, + maintenanceAllowedKey string, sensor resource.Sensor) (bool, error) { timeout := 5 * time.Second - ctx, cancel := context.WithTimeout(context.Background(), timeout) + ctx, cancel := context.WithTimeout(ctx, timeout) defer cancel() // Context timeouts on this call should be handled by grpc diff --git a/robot/impl/local_robot_test.go b/robot/impl/local_robot_test.go index 8c67c043d26..f015b16b896 100644 --- a/robot/impl/local_robot_test.go +++ b/robot/impl/local_robot_test.go @@ -3914,7 +3914,7 @@ func TestCheckMaintenanceSensorReadings(t *testing.T) { t.Run("Sensor reading errors out", func(t *testing.T) { r := setupLocalRobot(t, context.Background(), &config.Config{}, logger) localRobot := r.(*localRobot) - canReconfigure, err := localRobot.checkMaintenanceSensorReadings("", newErrorSensor()) + canReconfigure, err := localRobot.checkMaintenanceSensorReadings(context.Background(), "", newErrorSensor()) test.That(t, canReconfigure, test.ShouldEqual, true) test.That(t, err.Error(), test.ShouldEqual, "error reading maintenance sensor readings. Wallet not found") @@ -3922,7 +3922,7 @@ func TestCheckMaintenanceSensorReadings(t *testing.T) { t.Run("maintenanceAllowedKey does not exist", func(t *testing.T) { r := setupLocalRobot(t, context.Background(), &config.Config{}, logger) localRobot := r.(*localRobot) - canReconfigure, err := localRobot.checkMaintenanceSensorReadings("keyDoesNotExist", newValidSensor()) + canReconfigure, err := localRobot.checkMaintenanceSensorReadings(context.Background(), "keyDoesNotExist", newValidSensor()) test.That(t, canReconfigure, test.ShouldEqual, true) test.That(t, err.Error(), test.ShouldEqual, "error getting MaintenanceAllowedKey keyDoesNotExist from sensor reading") @@ -3930,7 +3930,7 @@ func TestCheckMaintenanceSensorReadings(t *testing.T) { t.Run("maintenanceAllowedKey is not a boolean", func(t *testing.T) { r := setupLocalRobot(t, context.Background(), &config.Config{}, logger) localRobot := r.(*localRobot) - canReconfigure, err := localRobot.checkMaintenanceSensorReadings("ThatIsNotAWallet", newValidSensor()) + canReconfigure, err := localRobot.checkMaintenanceSensorReadings(context.Background(), "ThatIsNotAWallet", newValidSensor()) test.That(t, canReconfigure, test.ShouldEqual, true) test.That(t, err.Error(), test.ShouldEqual, "maintenanceAllowedKey ThatIsNotAWallet is not a bool value") @@ -3962,7 +3962,7 @@ func TestCheckMaintenanceSensorReadingsSuccess(t *testing.T) { t.Run("", func(t *testing.T) { r := setupLocalRobot(t, context.Background(), &config.Config{}, logger) localRobot := r.(*localRobot) - canReconfigure, err := localRobot.checkMaintenanceSensorReadings(tc.maintenanceAllowedKey, tc.sensor) + canReconfigure, err := localRobot.checkMaintenanceSensorReadings(context.Background(), tc.maintenanceAllowedKey, tc.sensor) test.That(t, canReconfigure, test.ShouldEqual, tc.canReconfigure) test.That(t, err, test.ShouldBeNil) @@ -4016,6 +4016,7 @@ func TestMaintenanceConfig(t *testing.T) { }}) defer func() { resource.Deregister(sensor.API, model) + resource.Deregister(sensor.API, modelErrorSensor) }() remoteCfg := &config.Config{ Components: []resource.Config{ From acd77e03c21182ec25f0c844e9d7414ff45771fd Mon Sep 17 00:00:00 2001 From: Kschappacher <56745262+Kschappacher@users.noreply.github.com> Date: Mon, 14 Oct 2024 14:43:14 -0400 Subject: [PATCH 12/21] address pr comments, move config revision to after reconfig blocker --- robot/impl/local_robot.go | 32 ++++++++++++++++++++------------ robot/impl/local_robot_test.go | 4 ++-- 2 files changed, 22 insertions(+), 14 deletions(-) diff --git a/robot/impl/local_robot.go b/robot/impl/local_robot.go index ef3512bb5a1..e0f87ac658c 100644 --- a/robot/impl/local_robot.go +++ b/robot/impl/local_robot.go @@ -1175,20 +1175,12 @@ func (r *localRobot) applyLocalModuleVersions(cfg *config.Config) { } func (r *localRobot) reconfigure(ctx context.Context, newConfig *config.Config, forceSync bool) { - r.configRevisionMu.Lock() - r.configRevision = config.Revision{ - Revision: newConfig.Revision, - LastUpdated: time.Now(), - } - r.configRevisionMu.Unlock() - var allErrs error // Maintenance config can be configured to block reconfigure based off of a sensor reading - // These sensors can be configured on the main robot, a fragment, or a remote + // These sensors can be configured on the main robot, or a remote // In situations where there are conflicting sensor names the following behavior happens // Main robot and remote share sensor name -> main robot sensor is chosen - // Main robot and fragment share sensor name -> main robot sensor is chosen // Only remote has the sensor name -> remote sensor is read // Multiple remotes share a senor name -> conflict error is returned and reconfigure happens // To specify a specific remote sensor use the name format remoteName:sensorName to specify a remote sensor @@ -1204,6 +1196,14 @@ func (r *localRobot) reconfigure(ctx context.Context, newConfig *config.Config, canReconfigure, err := r.checkMaintenanceSensorReadings(ctx, newConfig.MaintenanceConfig.MaintenanceAllowedKey, sensorComponent) if !canReconfigure { r.logger.Info("maintenanceAllowedKey found from readings on maintenance sensor. Reconfigure disabled") + diff, err := config.DiffConfigs(*r.Config(), *newConfig, false) + if err != nil { + r.logger.CErrorw(ctx, "error diffing the configs", "error", err) + } + // NetworkEqual checks if Cloud/Auth/Network are equal between configs + if !diff.NetworkEqual { + r.logger.Info("Reconfigure disabled but Cloud/Auth/Network config section has been updated") + } return } if err != nil { @@ -1215,6 +1215,13 @@ func (r *localRobot) reconfigure(ctx context.Context, newConfig *config.Config, } } + r.configRevisionMu.Lock() + r.configRevision = config.Revision{ + Revision: newConfig.Revision, + LastUpdated: time.Now(), + } + r.configRevisionMu.Unlock() + // Sync Packages before reconfiguring rest of robot and resolving references to any packages // in the config. // TODO(RSDK-1849): Make this non-blocking so other resources that do not require packages can run before package sync finishes. @@ -1470,7 +1477,8 @@ func (r *localRobot) Version(ctx context.Context) (robot.VersionResponse, error) // checkMaintenanceSensorReadings ensures that errors from reading a sensor are handled properly. func (r *localRobot) checkMaintenanceSensorReadings(ctx context.Context, - maintenanceAllowedKey string, sensor resource.Sensor) (bool, error) { + maintenanceAllowedKey string, sensor resource.Sensor, +) (bool, error) { timeout := 5 * time.Second ctx, cancel := context.WithTimeout(ctx, timeout) defer cancel() @@ -1478,11 +1486,11 @@ func (r *localRobot) checkMaintenanceSensorReadings(ctx context.Context, // Context timeouts on this call should be handled by grpc readings, err := sensor.Readings(ctx, map[string]interface{}{}) if err != nil { - return true, errors.Errorf("error reading maintenance sensor readings. %s", err.Error()) + return false, errors.Errorf("error reading maintenance sensor readings. %s", err.Error()) } readingVal, ok := readings[maintenanceAllowedKey] if !ok { - return true, errors.Errorf("error getting MaintenanceAllowedKey %s from sensor reading", maintenanceAllowedKey) + return true, errors.Errorf("error getting maintenance_allowed_key %s from sensor reading", maintenanceAllowedKey) } canReconfigure, ok := readingVal.(bool) if !ok { diff --git a/robot/impl/local_robot_test.go b/robot/impl/local_robot_test.go index f015b16b896..d231e123e8d 100644 --- a/robot/impl/local_robot_test.go +++ b/robot/impl/local_robot_test.go @@ -3916,7 +3916,7 @@ func TestCheckMaintenanceSensorReadings(t *testing.T) { localRobot := r.(*localRobot) canReconfigure, err := localRobot.checkMaintenanceSensorReadings(context.Background(), "", newErrorSensor()) - test.That(t, canReconfigure, test.ShouldEqual, true) + test.That(t, canReconfigure, test.ShouldEqual, false) test.That(t, err.Error(), test.ShouldEqual, "error reading maintenance sensor readings. Wallet not found") }) t.Run("maintenanceAllowedKey does not exist", func(t *testing.T) { @@ -3925,7 +3925,7 @@ func TestCheckMaintenanceSensorReadings(t *testing.T) { canReconfigure, err := localRobot.checkMaintenanceSensorReadings(context.Background(), "keyDoesNotExist", newValidSensor()) test.That(t, canReconfigure, test.ShouldEqual, true) - test.That(t, err.Error(), test.ShouldEqual, "error getting MaintenanceAllowedKey keyDoesNotExist from sensor reading") + test.That(t, err.Error(), test.ShouldEqual, "error getting maintenance_allowed_key keyDoesNotExist from sensor reading") }) t.Run("maintenanceAllowedKey is not a boolean", func(t *testing.T) { r := setupLocalRobot(t, context.Background(), &config.Config{}, logger) From f4effb4037be23f64a15f257567de367ddeeb3f9 Mon Sep 17 00:00:00 2001 From: Kschappacher <56745262+Kschappacher@users.noreply.github.com> Date: Mon, 14 Oct 2024 15:54:36 -0400 Subject: [PATCH 13/21] update tests --- robot/impl/local_robot.go | 1 + robot/impl/local_robot_test.go | 12 ++++++++++-- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/robot/impl/local_robot.go b/robot/impl/local_robot.go index e0f87ac658c..e4152dc451f 100644 --- a/robot/impl/local_robot.go +++ b/robot/impl/local_robot.go @@ -1486,6 +1486,7 @@ func (r *localRobot) checkMaintenanceSensorReadings(ctx context.Context, // Context timeouts on this call should be handled by grpc readings, err := sensor.Readings(ctx, map[string]interface{}{}) if err != nil { + // if the sensor errors or timeouts we return false to block reconfigure return false, errors.Errorf("error reading maintenance sensor readings. %s", err.Error()) } readingVal, ok := readings[maintenanceAllowedKey] diff --git a/robot/impl/local_robot_test.go b/robot/impl/local_robot_test.go index d231e123e8d..6e0ce46c3b4 100644 --- a/robot/impl/local_robot_test.go +++ b/robot/impl/local_robot_test.go @@ -3987,6 +3987,14 @@ func newErrorSensor() sensor.Sensor { return s } +func newInvalidSensor() sensor.Sensor { + s := &inject.Sensor{} + s.ReadingsFunc = func(ctx context.Context, extra map[string]interface{}) (map[string]interface{}, error) { + return map[string]any{"ThatsMyWallet": 1, "ThatsNotMyWallet": 2}, nil + } + return s +} + func TestMaintenanceConfig(t *testing.T) { ctx := context.Background() logger := logging.NewTestLogger(t) @@ -4012,7 +4020,7 @@ func TestMaintenanceConfig(t *testing.T) { conf resource.Config, logger logging.Logger, ) (sensor.Sensor, error) { - return newErrorSensor(), nil + return newInvalidSensor(), nil }}) defer func() { resource.Deregister(sensor.API, model) @@ -4124,7 +4132,7 @@ func TestMaintenanceConfig(t *testing.T) { test.That(t, err.Error(), test.ShouldEqual, "resource \"rdk:component:sensor/sensor2\" not found") }) - t.Run("conflicting remote and main sensors default to main", func(t *testing.T) { + t.Run("conflicting remote and main sensor names default to main", func(t *testing.T) { ctx := context.Background() // Setup remote with error maintenance sensor, if sensor is ever called it will error and reconfigure normally remoteErrConfig := &config.Config{ From 7ae8e32287534382be106c3623614ffb7efa626c Mon Sep 17 00:00:00 2001 From: Kschappacher <56745262+Kschappacher@users.noreply.github.com> Date: Mon, 14 Oct 2024 16:08:09 -0400 Subject: [PATCH 14/21] add nil check --- robot/impl/local_robot.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/robot/impl/local_robot.go b/robot/impl/local_robot.go index e4152dc451f..9bbd9d13cd0 100644 --- a/robot/impl/local_robot.go +++ b/robot/impl/local_robot.go @@ -1201,7 +1201,7 @@ func (r *localRobot) reconfigure(ctx context.Context, newConfig *config.Config, r.logger.CErrorw(ctx, "error diffing the configs", "error", err) } // NetworkEqual checks if Cloud/Auth/Network are equal between configs - if !diff.NetworkEqual { + if diff != nil && !diff.NetworkEqual { r.logger.Info("Reconfigure disabled but Cloud/Auth/Network config section has been updated") } return From 98f69d31037a561a1f3fce2f71617e6004606561 Mon Sep 17 00:00:00 2001 From: Kurt S <56745262+Kschappacher@users.noreply.github.com> Date: Mon, 14 Oct 2024 16:36:52 -0400 Subject: [PATCH 15/21] Update robot/impl/local_robot.go Co-authored-by: Cheuk <90270663+cheukt@users.noreply.github.com> --- robot/impl/local_robot.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/robot/impl/local_robot.go b/robot/impl/local_robot.go index 9bbd9d13cd0..9e3f073bf5f 100644 --- a/robot/impl/local_robot.go +++ b/robot/impl/local_robot.go @@ -1187,7 +1187,7 @@ func (r *localRobot) reconfigure(ctx context.Context, newConfig *config.Config, if newConfig.MaintenanceConfig != nil { name, err := resource.NewFromString(newConfig.MaintenanceConfig.SensorName) if err != nil { - r.logger.Warnf("Sensor Name %s is not in a supported format", newConfig.MaintenanceConfig.SensorName) + r.logger.Warnf("sensor_name %s in maintenance config is not in a supported format", newConfig.MaintenanceConfig.SensorName) } else { sensorComponent, err := robot.ResourceFromRobot[sensor.Sensor](r, name) if err != nil { From c7f5cfc6f7bbc0ee2d1d24c5bd3b8c33fb38fd55 Mon Sep 17 00:00:00 2001 From: Kurt S <56745262+Kschappacher@users.noreply.github.com> Date: Mon, 14 Oct 2024 16:37:17 -0400 Subject: [PATCH 16/21] Update robot/impl/local_robot.go Co-authored-by: Cheuk <90270663+cheukt@users.noreply.github.com> --- robot/impl/local_robot.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/robot/impl/local_robot.go b/robot/impl/local_robot.go index 9e3f073bf5f..b5f3fbd2cb2 100644 --- a/robot/impl/local_robot.go +++ b/robot/impl/local_robot.go @@ -1195,7 +1195,7 @@ func (r *localRobot) reconfigure(ctx context.Context, newConfig *config.Config, } else { canReconfigure, err := r.checkMaintenanceSensorReadings(ctx, newConfig.MaintenanceConfig.MaintenanceAllowedKey, sensorComponent) if !canReconfigure { - r.logger.Info("maintenanceAllowedKey found from readings on maintenance sensor. Reconfigure disabled") + r.logger.Info("maintenanceAllowedKey found from readings on maintenance sensor. Skipping reconfiguration.") diff, err := config.DiffConfigs(*r.Config(), *newConfig, false) if err != nil { r.logger.CErrorw(ctx, "error diffing the configs", "error", err) From 443ca6d97694f970ea2478e89795c707a95de8d7 Mon Sep 17 00:00:00 2001 From: Kurt S <56745262+Kschappacher@users.noreply.github.com> Date: Mon, 14 Oct 2024 16:37:24 -0400 Subject: [PATCH 17/21] Update robot/impl/local_robot.go Co-authored-by: Cheuk <90270663+cheukt@users.noreply.github.com> --- robot/impl/local_robot.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/robot/impl/local_robot.go b/robot/impl/local_robot.go index b5f3fbd2cb2..2e4f1456403 100644 --- a/robot/impl/local_robot.go +++ b/robot/impl/local_robot.go @@ -1202,7 +1202,7 @@ func (r *localRobot) reconfigure(ctx context.Context, newConfig *config.Config, } // NetworkEqual checks if Cloud/Auth/Network are equal between configs if diff != nil && !diff.NetworkEqual { - r.logger.Info("Reconfigure disabled but Cloud/Auth/Network config section has been updated") + r.logger.Info("Machine reconfiguration skipped but Cloud/Auth/Network config section contain changes and will be applied.") } return } From 62e51aa21c01a31ee052cb22d6ed44227c7029e5 Mon Sep 17 00:00:00 2001 From: Kschappacher <56745262+Kschappacher@users.noreply.github.com> Date: Mon, 14 Oct 2024 16:38:15 -0400 Subject: [PATCH 18/21] remove dup --- config/proto_conversions.go | 8 -------- 1 file changed, 8 deletions(-) diff --git a/config/proto_conversions.go b/config/proto_conversions.go index 7a323d1223e..fb252a6286b 100644 --- a/config/proto_conversions.go +++ b/config/proto_conversions.go @@ -105,14 +105,6 @@ func FromProto(proto *pb.RobotConfig, logger logging.Logger) (*Config, error) { logAnyFragmentOverwriteErrors(logger, proto.OverwriteFragmentStatus) - if proto.Maintenance != nil { - maintenanceConfig, err := MaintenanceConfigFromProto(proto.Maintenance) - if err != nil { - return nil, errors.Wrap(err, "error converting maintenance config from proto") - } - cfg.MaintenanceConfig = maintenanceConfig - } - return &cfg, nil } From f8a58d95ceeaec491a004cbfbacebe8fa9ce731f Mon Sep 17 00:00:00 2001 From: Kschappacher <56745262+Kschappacher@users.noreply.github.com> Date: Mon, 14 Oct 2024 16:49:45 -0400 Subject: [PATCH 19/21] add test cases and ensure readings work after going to proto and back --- robot/impl/local_robot_test.go | 26 ++++++++++++++++++++++++-- 1 file changed, 24 insertions(+), 2 deletions(-) diff --git a/robot/impl/local_robot_test.go b/robot/impl/local_robot_test.go index 6e0ce46c3b4..6430ac5ebfb 100644 --- a/robot/impl/local_robot_test.go +++ b/robot/impl/local_robot_test.go @@ -55,6 +55,7 @@ import ( rgrpc "go.viam.com/rdk/grpc" internalcloud "go.viam.com/rdk/internal/cloud" "go.viam.com/rdk/logging" + "go.viam.com/rdk/protoutils" "go.viam.com/rdk/referenceframe" "go.viam.com/rdk/resource" "go.viam.com/rdk/robot" @@ -3927,7 +3928,7 @@ func TestCheckMaintenanceSensorReadings(t *testing.T) { test.That(t, canReconfigure, test.ShouldEqual, true) test.That(t, err.Error(), test.ShouldEqual, "error getting maintenance_allowed_key keyDoesNotExist from sensor reading") }) - t.Run("maintenanceAllowedKey is not a boolean", func(t *testing.T) { + t.Run("maintenanceAllowedKey is a number not a boolean", func(t *testing.T) { r := setupLocalRobot(t, context.Background(), &config.Config{}, logger) localRobot := r.(*localRobot) canReconfigure, err := localRobot.checkMaintenanceSensorReadings(context.Background(), "ThatIsNotAWallet", newValidSensor()) @@ -3935,6 +3936,22 @@ func TestCheckMaintenanceSensorReadings(t *testing.T) { test.That(t, canReconfigure, test.ShouldEqual, true) test.That(t, err.Error(), test.ShouldEqual, "maintenanceAllowedKey ThatIsNotAWallet is not a bool value") }) + t.Run("maintenanceAllowedKey is one not a boolean", func(t *testing.T) { + r := setupLocalRobot(t, context.Background(), &config.Config{}, logger) + localRobot := r.(*localRobot) + canReconfigure, err := localRobot.checkMaintenanceSensorReadings(context.Background(), "OneIsNotTrue", newValidSensor()) + + test.That(t, canReconfigure, test.ShouldEqual, true) + test.That(t, err.Error(), test.ShouldEqual, "maintenanceAllowedKey OneIsNotTrue is not a bool value") + }) + t.Run("maintenanceAllowedKey is string true not a boolean", func(t *testing.T) { + r := setupLocalRobot(t, context.Background(), &config.Config{}, logger) + localRobot := r.(*localRobot) + canReconfigure, err := localRobot.checkMaintenanceSensorReadings(context.Background(), "TrueIsNotTrue", newValidSensor()) + + test.That(t, canReconfigure, test.ShouldEqual, true) + test.That(t, err.Error(), test.ShouldEqual, "maintenanceAllowedKey TrueIsNotTrue is not a bool value") + }) } func TestCheckMaintenanceSensorReadingsSuccess(t *testing.T) { @@ -3973,7 +3990,12 @@ func TestCheckMaintenanceSensorReadingsSuccess(t *testing.T) { func newValidSensor() sensor.Sensor { s := &inject.Sensor{} s.ReadingsFunc = func(ctx context.Context, extra map[string]interface{}) (map[string]interface{}, error) { - return map[string]any{"ThatsMyWallet": false, "ThatsNotMyWallet": true, "ThatIsNotAWallet": 5}, nil + // We want to ensure that we get the same readings after convering to proto and back to go + readings := map[string]any{"ThatsMyWallet": false, "ThatsNotMyWallet": true, + "ThatIsNotAWallet": 5, "TrueIsNotTrue": "true", "OneIsNotTrue": 1} + readingsProto, _ := protoutils.ReadingGoToProto(readings) + retReadings, _ := protoutils.ReadingProtoToGo(readingsProto) + return retReadings, nil } s.CloseFunc = func(ctx context.Context) error { return nil } return s From 36a788bf2fd4fe86f3aff8069871e8c423d77b7f Mon Sep 17 00:00:00 2001 From: Kschappacher <56745262+Kschappacher@users.noreply.github.com> Date: Mon, 14 Oct 2024 16:52:55 -0400 Subject: [PATCH 20/21] clean up dif --- config/proto_conversions.go | 4 ++-- robot/impl/local_robot.go | 4 ++-- robot/impl/local_robot_test.go | 6 ++++-- 3 files changed, 8 insertions(+), 6 deletions(-) diff --git a/config/proto_conversions.go b/config/proto_conversions.go index fb252a6286b..9df0cae664e 100644 --- a/config/proto_conversions.go +++ b/config/proto_conversions.go @@ -95,6 +95,8 @@ func FromProto(proto *pb.RobotConfig, logger logging.Logger) (*Config, error) { cfg.Revision = proto.Revision + logAnyFragmentOverwriteErrors(logger, proto.OverwriteFragmentStatus) + if proto.Maintenance != nil { maintenanceConfig, err := MaintenanceConfigFromProto(proto.Maintenance) if err != nil { @@ -103,8 +105,6 @@ func FromProto(proto *pb.RobotConfig, logger logging.Logger) (*Config, error) { cfg.MaintenanceConfig = maintenanceConfig } - logAnyFragmentOverwriteErrors(logger, proto.OverwriteFragmentStatus) - return &cfg, nil } diff --git a/robot/impl/local_robot.go b/robot/impl/local_robot.go index 2e4f1456403..c998807e5bc 100644 --- a/robot/impl/local_robot.go +++ b/robot/impl/local_robot.go @@ -1175,8 +1175,6 @@ func (r *localRobot) applyLocalModuleVersions(cfg *config.Config) { } func (r *localRobot) reconfigure(ctx context.Context, newConfig *config.Config, forceSync bool) { - var allErrs error - // Maintenance config can be configured to block reconfigure based off of a sensor reading // These sensors can be configured on the main robot, or a remote // In situations where there are conflicting sensor names the following behavior happens @@ -1222,6 +1220,8 @@ func (r *localRobot) reconfigure(ctx context.Context, newConfig *config.Config, } r.configRevisionMu.Unlock() + var allErrs error + // Sync Packages before reconfiguring rest of robot and resolving references to any packages // in the config. // TODO(RSDK-1849): Make this non-blocking so other resources that do not require packages can run before package sync finishes. diff --git a/robot/impl/local_robot_test.go b/robot/impl/local_robot_test.go index 6430ac5ebfb..4d3e9fb1287 100644 --- a/robot/impl/local_robot_test.go +++ b/robot/impl/local_robot_test.go @@ -3991,8 +3991,10 @@ func newValidSensor() sensor.Sensor { s := &inject.Sensor{} s.ReadingsFunc = func(ctx context.Context, extra map[string]interface{}) (map[string]interface{}, error) { // We want to ensure that we get the same readings after convering to proto and back to go - readings := map[string]any{"ThatsMyWallet": false, "ThatsNotMyWallet": true, - "ThatIsNotAWallet": 5, "TrueIsNotTrue": "true", "OneIsNotTrue": 1} + readings := map[string]any{ + "ThatsMyWallet": false, "ThatsNotMyWallet": true, + "ThatIsNotAWallet": 5, "TrueIsNotTrue": "true", "OneIsNotTrue": 1, + } readingsProto, _ := protoutils.ReadingGoToProto(readings) retReadings, _ := protoutils.ReadingProtoToGo(readingsProto) return retReadings, nil From b70a0a7207ca8dccf72e856e959369ee5474bbf7 Mon Sep 17 00:00:00 2001 From: Kschappacher <56745262+Kschappacher@users.noreply.github.com> Date: Tue, 15 Oct 2024 11:34:57 -0400 Subject: [PATCH 21/21] fix log names --- robot/impl/local_robot.go | 6 +++--- robot/impl/local_robot_test.go | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/robot/impl/local_robot.go b/robot/impl/local_robot.go index c998807e5bc..11617b0cb57 100644 --- a/robot/impl/local_robot.go +++ b/robot/impl/local_robot.go @@ -1193,7 +1193,7 @@ func (r *localRobot) reconfigure(ctx context.Context, newConfig *config.Config, } else { canReconfigure, err := r.checkMaintenanceSensorReadings(ctx, newConfig.MaintenanceConfig.MaintenanceAllowedKey, sensorComponent) if !canReconfigure { - r.logger.Info("maintenanceAllowedKey found from readings on maintenance sensor. Skipping reconfiguration.") + r.logger.Info("maintenance_allowed_key found from readings on maintenance sensor. Skipping reconfiguration.") diff, err := config.DiffConfigs(*r.Config(), *newConfig, false) if err != nil { r.logger.CErrorw(ctx, "error diffing the configs", "error", err) @@ -1207,7 +1207,7 @@ func (r *localRobot) reconfigure(ctx context.Context, newConfig *config.Config, if err != nil { r.logger.Warn(err.Error() + ". Starting reconfiguration") } else { - r.logger.Info("maintenanceAllowedKey found from readings on maintenance sensor. Starting reconfiguration") + r.logger.Info("maintenance_allowed_key found from readings on maintenance sensor. Starting reconfiguration") } } } @@ -1495,7 +1495,7 @@ func (r *localRobot) checkMaintenanceSensorReadings(ctx context.Context, } canReconfigure, ok := readingVal.(bool) if !ok { - return true, errors.Errorf("maintenanceAllowedKey %s is not a bool value", maintenanceAllowedKey) + return true, errors.Errorf("maintenance_allowed_key %s is not a bool value", maintenanceAllowedKey) } return canReconfigure, nil } diff --git a/robot/impl/local_robot_test.go b/robot/impl/local_robot_test.go index 4d3e9fb1287..5383b0230be 100644 --- a/robot/impl/local_robot_test.go +++ b/robot/impl/local_robot_test.go @@ -3934,7 +3934,7 @@ func TestCheckMaintenanceSensorReadings(t *testing.T) { canReconfigure, err := localRobot.checkMaintenanceSensorReadings(context.Background(), "ThatIsNotAWallet", newValidSensor()) test.That(t, canReconfigure, test.ShouldEqual, true) - test.That(t, err.Error(), test.ShouldEqual, "maintenanceAllowedKey ThatIsNotAWallet is not a bool value") + test.That(t, err.Error(), test.ShouldEqual, "maintenance_allowed_key ThatIsNotAWallet is not a bool value") }) t.Run("maintenanceAllowedKey is one not a boolean", func(t *testing.T) { r := setupLocalRobot(t, context.Background(), &config.Config{}, logger) @@ -3942,7 +3942,7 @@ func TestCheckMaintenanceSensorReadings(t *testing.T) { canReconfigure, err := localRobot.checkMaintenanceSensorReadings(context.Background(), "OneIsNotTrue", newValidSensor()) test.That(t, canReconfigure, test.ShouldEqual, true) - test.That(t, err.Error(), test.ShouldEqual, "maintenanceAllowedKey OneIsNotTrue is not a bool value") + test.That(t, err.Error(), test.ShouldEqual, "maintenance_allowed_key OneIsNotTrue is not a bool value") }) t.Run("maintenanceAllowedKey is string true not a boolean", func(t *testing.T) { r := setupLocalRobot(t, context.Background(), &config.Config{}, logger) @@ -3950,7 +3950,7 @@ func TestCheckMaintenanceSensorReadings(t *testing.T) { canReconfigure, err := localRobot.checkMaintenanceSensorReadings(context.Background(), "TrueIsNotTrue", newValidSensor()) test.That(t, canReconfigure, test.ShouldEqual, true) - test.That(t, err.Error(), test.ShouldEqual, "maintenanceAllowedKey TrueIsNotTrue is not a bool value") + test.That(t, err.Error(), test.ShouldEqual, "maintenance_allowed_key TrueIsNotTrue is not a bool value") }) }