Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RSDK-8719: - use maintenance config in RDK #4396

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 22 additions & 11 deletions config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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"`
Expand All @@ -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.
Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -330,6 +340,7 @@ func (c Config) MarshalJSON() ([]byte, error) {
EnableWebProfile: c.EnableWebProfile,
LogConfig: c.LogConfig,
Revision: c.Revision,
MaintenanceConfig: c.MaintenanceConfig,
})
}

Expand Down
2 changes: 1 addition & 1 deletion go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -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=
Expand Down
52 changes: 51 additions & 1 deletion robot/impl/local_robot.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -1183,12 +1184,20 @@ 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")
Kschappacher marked this conversation as resolved.
Show resolved Hide resolved
}
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
// 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
Expand Down Expand Up @@ -1435,3 +1444,44 @@ 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. Using default reconfigure")
}
Comment on lines +1449 to +1451
Copy link
Member Author

Choose a reason for hiding this comment

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

move this up to where this is called and don't alert users if this does not exist

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)
}
Kschappacher marked this conversation as resolved.
Show resolved Hide resolved
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())
Kschappacher marked this conversation as resolved.
Show resolved Hide resolved
}
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{}{})
Copy link
Member Author

Choose a reason for hiding this comment

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

add context timeout here and add test for the timeout

Copy link
Member Author

Choose a reason for hiding this comment

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

5 seconds

if err != nil {
return true, 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)
}
canReconfigure, ok := readingVal.(bool)
if !ok {
return true, errors.Errorf("maintenanceAllowedKey %s is not a bool value", maintenanceAllowedKey)
}

r.logger.Info("maintenanceAllowedKey found canReconfigure set to %t", canReconfigure)
Copy link
Member Author

Choose a reason for hiding this comment

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

maintenanceAllowedKey found on readings from maintenance sensor explicitly say skipping reconfiguration or starting reconfiguration

return canReconfigure, nil
}
166 changes: 166 additions & 0 deletions robot/impl/local_robot_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,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"
Expand Down Expand Up @@ -3955,3 +3956,168 @@ func TestLogPropagation(t *testing.T) {
})
}
}

func TestCheckMaintenanceSensor(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,
Copy link
Member Author

Choose a reason for hiding this comment

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

add testname field

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)

test.That(t, canReconfigure, test.ShouldEqual, tc.canReconfigure)
test.That(t, err.Error(), test.ShouldEqual, tc.errorMessage)
})
}
}

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",
},
}
Copy link
Member Author

Choose a reason for hiding this comment

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

add test for non boolean return

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
}
Loading