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-448] Add Partial Start option to robot config #1617

Merged
Merged
Show file tree
Hide file tree
Changes from 2 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
35 changes: 27 additions & 8 deletions config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
"sync"
"time"

"github.com/edaniels/golog"
"github.com/pkg/errors"
"go.viam.com/utils"
"go.viam.com/utils/pexec"
Expand All @@ -27,8 +28,7 @@ type Config struct {
Services []Service `json:"services,omitempty"`
Network NetworkConfig `json:"network"`
Auth AuthConfig `json:"auth"`

Debug bool `json:"debug,omitempty"`
Debug bool `json:"debug,omitempty"`

ConfigFilePath string `json:"-"`

Expand All @@ -46,6 +46,10 @@ type Config struct {
// If false, it's for creating a robot via the RDK library. This is helpful for
// error messages that can indicate flags/config fields to use.
FromCommand bool `json:"-"`

// DisablepartialStart ensures that a robot will only start when all the components,
Copy link
Member

Choose a reason for hiding this comment

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

nit: DisablepartialStart -> DisablePartialStart

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed the nit, added the two test cases in the AC of the story and added the proto story https://viam.atlassian.net/browse/RSDK-918

// services, and remotes pass config validation. This value is false by default
DisablePartialStart bool `json:"disablepartialstart"`
cheukt marked this conversation as resolved.
Show resolved Hide resolved
}

// Ensure ensures all parts of the config are valid.
Expand All @@ -58,30 +62,45 @@ func (c *Config) Ensure(fromCloud bool) error {

for idx := 0; idx < len(c.Remotes); idx++ {
if err := c.Remotes[idx].Validate(fmt.Sprintf("%s.%d", "remotes", idx)); err != nil {
return err
if c.DisablePartialStart {
return err
}
golog.Global().Error(err)
cheukt marked this conversation as resolved.
Show resolved Hide resolved
}
}

for idx := 0; idx < len(c.Components); idx++ {
dependsOn, err := c.Components[idx].Validate(fmt.Sprintf("%s.%d", "components", idx))
if err != nil {
return errors.Errorf("error validating component %s: %s", c.Components[idx].Name, err)
fullErr := errors.Errorf("error validating component %s: %s", c.Components[idx].Name, err)
if c.DisablePartialStart {
return fullErr
}
golog.Global().Error(fullErr)
} else {
c.Components[idx].ImplicitDependsOn = dependsOn
}
c.Components[idx].ImplicitDependsOn = dependsOn
}

for idx := 0; idx < len(c.Processes); idx++ {
if err := c.Processes[idx].Validate(fmt.Sprintf("%s.%d", "processes", idx)); err != nil {
return err
if c.DisablePartialStart {
return err
}
golog.Global().Error(err)
}
}

for idx := 0; idx < len(c.Services); idx++ {
dependsOn, err := c.Services[idx].Validate(fmt.Sprintf("%s.%d", "services", idx))
if err != nil {
return err
if c.DisablePartialStart {
return err
}
golog.Global().Error(err)
} else {
c.Services[idx].ImplicitDependsOn = dependsOn
}
c.Services[idx].ImplicitDependsOn = dependsOn
}

if err := c.Network.Validate("network"); err != nil {
Expand Down
187 changes: 183 additions & 4 deletions config/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,8 @@ func TestConfigEnsure(t *testing.T) {
test.That(t, invalidCloud.Ensure(true), test.ShouldBeNil)

invalidRemotes := config.Config{
Remotes: []config.Remote{{}},
DisablePartialStart: true,
Remotes: []config.Remote{{}},
}
err = invalidRemotes.Ensure(false)
test.That(t, err, test.ShouldNotBeNil)
Expand All @@ -176,7 +177,8 @@ func TestConfigEnsure(t *testing.T) {
test.That(t, invalidRemotes.Ensure(false), test.ShouldBeNil)

invalidComponents := config.Config{
Components: []config.Component{{}},
DisablePartialStart: true,
Components: []config.Component{{}},
}
err = invalidComponents.Ensure(false)
test.That(t, err, test.ShouldNotBeNil)
Expand All @@ -193,13 +195,15 @@ func TestConfigEnsure(t *testing.T) {
c6 := config.Component{Namespace: resource.ResourceNamespaceRDK, Name: "c6"}
c7 := config.Component{Namespace: resource.ResourceNamespaceRDK, Name: "c7", DependsOn: []string{"c6", "c4"}}
components := config.Config{
Components: []config.Component{c7, c6, c5, c3, c4, c1, c2},
DisablePartialStart: true,
Components: []config.Component{c7, c6, c5, c3, c4, c1, c2},
}
err = components.Ensure(false)
test.That(t, err, test.ShouldBeNil)

invalidProcesses := config.Config{
Processes: []pexec.ProcessConfig{{}},
DisablePartialStart: true,
Processes: []pexec.ProcessConfig{{}},
}
err = invalidProcesses.Ensure(false)
test.That(t, err, test.ShouldNotBeNil)
Expand Down Expand Up @@ -317,6 +321,181 @@ func TestConfigEnsure(t *testing.T) {
test.That(t, invalidAuthConfig.Ensure(false), test.ShouldBeNil)
}

func TestConfigEnsurePartialStart(t *testing.T) {
var emptyConfig config.Config
test.That(t, emptyConfig.Ensure(false), test.ShouldBeNil)

invalidCloud := config.Config{
Cloud: &config.Cloud{},
}
err := invalidCloud.Ensure(false)
test.That(t, err, test.ShouldNotBeNil)
test.That(t, err.Error(), test.ShouldContainSubstring, `cloud`)
test.That(t, err.Error(), test.ShouldContainSubstring, `"id" is required`)
invalidCloud.Cloud.ID = "some_id"
err = invalidCloud.Ensure(false)
test.That(t, err, test.ShouldNotBeNil)
test.That(t, err.Error(), test.ShouldContainSubstring, `"secret" is required`)
err = invalidCloud.Ensure(true)
test.That(t, err, test.ShouldNotBeNil)
test.That(t, err.Error(), test.ShouldContainSubstring, `"fqdn" is required`)
invalidCloud.Cloud.Secret = "my_secret"
test.That(t, invalidCloud.Ensure(false), test.ShouldBeNil)
test.That(t, invalidCloud.Ensure(true), test.ShouldNotBeNil)
invalidCloud.Cloud.Secret = ""
invalidCloud.Cloud.FQDN = "wooself"
err = invalidCloud.Ensure(true)
test.That(t, err, test.ShouldNotBeNil)
test.That(t, err.Error(), test.ShouldContainSubstring, `"local_fqdn" is required`)
invalidCloud.Cloud.LocalFQDN = "yeeself"
test.That(t, invalidCloud.Ensure(true), test.ShouldBeNil)

invalidRemotes := config.Config{
Remotes: []config.Remote{{}},
}
err = invalidRemotes.Ensure(false)
test.That(t, err, test.ShouldBeNil)
invalidRemotes.Remotes[0].Name = "foo"
err = invalidRemotes.Ensure(false)
test.That(t, err, test.ShouldBeNil)
invalidRemotes.Remotes[0].Address = "bar"
test.That(t, invalidRemotes.Ensure(false), test.ShouldBeNil)

invalidComponents := config.Config{
Components: []config.Component{{}},
}
err = invalidComponents.Ensure(false)
test.That(t, err, test.ShouldBeNil)
invalidComponents.Components[0].Name = "foo"
test.That(t, invalidComponents.Ensure(false), test.ShouldBeNil)

c1 := config.Component{Namespace: resource.ResourceNamespaceRDK, Name: "c1"}
c2 := config.Component{Namespace: resource.ResourceNamespaceRDK, Name: "c2", DependsOn: []string{"c1"}}
c3 := config.Component{Namespace: resource.ResourceNamespaceRDK, Name: "c3", DependsOn: []string{"c1", "c2"}}
c4 := config.Component{Namespace: resource.ResourceNamespaceRDK, Name: "c4", DependsOn: []string{"c1", "c3"}}
c5 := config.Component{Namespace: resource.ResourceNamespaceRDK, Name: "c5", DependsOn: []string{"c2", "c4"}}
c6 := config.Component{Namespace: resource.ResourceNamespaceRDK, Name: "c6"}
c7 := config.Component{Namespace: resource.ResourceNamespaceRDK, Name: "c7", DependsOn: []string{"c6", "c4"}}
components := config.Config{
Components: []config.Component{c7, c6, c5, c3, c4, c1, c2},
}
err = components.Ensure(false)
test.That(t, err, test.ShouldBeNil)

invalidProcesses := config.Config{
Processes: []pexec.ProcessConfig{{}},
}
err = invalidProcesses.Ensure(false)
test.That(t, err, test.ShouldBeNil)
invalidProcesses.Processes[0].Name = "foo"
test.That(t, invalidProcesses.Ensure(false), test.ShouldBeNil)

invalidNetwork := config.Config{
Network: config.NetworkConfig{
NetworkConfigData: config.NetworkConfigData{
TLSCertFile: "hey",
},
},
}
err = invalidNetwork.Ensure(false)
test.That(t, err, test.ShouldNotBeNil)
test.That(t, err.Error(), test.ShouldContainSubstring, `network`)
test.That(t, err.Error(), test.ShouldContainSubstring, `both tls`)

invalidNetwork.Network.TLSCertFile = ""
invalidNetwork.Network.TLSKeyFile = "hey"
err = invalidNetwork.Ensure(false)
test.That(t, err, test.ShouldNotBeNil)
test.That(t, err.Error(), test.ShouldContainSubstring, `network`)
test.That(t, err.Error(), test.ShouldContainSubstring, `both tls`)

invalidNetwork.Network.TLSCertFile = "dude"
test.That(t, invalidNetwork.Ensure(false), test.ShouldBeNil)

invalidNetwork.Network.TLSCertFile = ""
invalidNetwork.Network.TLSKeyFile = ""
test.That(t, invalidNetwork.Ensure(false), test.ShouldBeNil)

invalidNetwork.Network.BindAddress = "woop"
err = invalidNetwork.Ensure(false)
test.That(t, err, test.ShouldNotBeNil)
test.That(t, err.Error(), test.ShouldContainSubstring, `bind_address`)
test.That(t, err.Error(), test.ShouldContainSubstring, `missing port`)

invalidNetwork.Network.BindAddress = "woop"
invalidNetwork.Network.Listener = &net.TCPListener{}
err = invalidNetwork.Ensure(false)
test.That(t, err, test.ShouldNotBeNil)
test.That(t, err.Error(), test.ShouldContainSubstring, `only set one of`)

invalidAuthConfig := config.Config{
Auth: config.AuthConfig{},
}
test.That(t, invalidAuthConfig.Ensure(false), test.ShouldBeNil)

invalidAuthConfig.Auth.Handlers = []config.AuthHandlerConfig{
{Type: rpc.CredentialsTypeAPIKey},
}
err = invalidAuthConfig.Ensure(false)
test.That(t, err, test.ShouldNotBeNil)
test.That(t, err.Error(), test.ShouldContainSubstring, `auth.handlers.0`)
test.That(t, err.Error(), test.ShouldContainSubstring, `required`)
test.That(t, err.Error(), test.ShouldContainSubstring, `key`)

validAPIKeyHandler := config.AuthHandlerConfig{
Type: rpc.CredentialsTypeAPIKey,
Config: config.AttributeMap{
"key": "foo",
},
}

invalidAuthConfig.Auth.Handlers = []config.AuthHandlerConfig{
validAPIKeyHandler,
validAPIKeyHandler,
}
err = invalidAuthConfig.Ensure(false)
test.That(t, err, test.ShouldNotBeNil)
test.That(t, err.Error(), test.ShouldContainSubstring, `auth.handlers.1`)
test.That(t, err.Error(), test.ShouldContainSubstring, `duplicate`)
test.That(t, err.Error(), test.ShouldContainSubstring, `api-key`)

invalidAuthConfig.Auth.Handlers = []config.AuthHandlerConfig{
validAPIKeyHandler,
{Type: "unknown"},
}
err = invalidAuthConfig.Ensure(false)
test.That(t, err, test.ShouldNotBeNil)
test.That(t, err.Error(), test.ShouldContainSubstring, `auth.handlers.1`)
test.That(t, err.Error(), test.ShouldContainSubstring, `do not know how`)
test.That(t, err.Error(), test.ShouldContainSubstring, `unknown`)

invalidAuthConfig.Auth.Handlers = []config.AuthHandlerConfig{
validAPIKeyHandler,
}
test.That(t, invalidAuthConfig.Ensure(false), test.ShouldBeNil)

validAPIKeyHandler.Config = config.AttributeMap{
"keys": []string{},
}
invalidAuthConfig.Auth.Handlers = []config.AuthHandlerConfig{
validAPIKeyHandler,
}
err = invalidAuthConfig.Ensure(false)
test.That(t, err, test.ShouldNotBeNil)
test.That(t, err.Error(), test.ShouldContainSubstring, `auth.handlers.0`)
test.That(t, err.Error(), test.ShouldContainSubstring, `required`)
test.That(t, err.Error(), test.ShouldContainSubstring, `key`)

validAPIKeyHandler.Config = config.AttributeMap{
"keys": []string{"one", "two"},
}
invalidAuthConfig.Auth.Handlers = []config.AuthHandlerConfig{
validAPIKeyHandler,
}

test.That(t, invalidAuthConfig.Ensure(false), test.ShouldBeNil)
}

func TestCopyOnlyPublicFields(t *testing.T) {
t.Run("copy sample config", func(t *testing.T) {
content, err := os.ReadFile("data/robot.json")
Expand Down
2 changes: 1 addition & 1 deletion config/reader_ext_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ func TestFromReaderValidate(t *testing.T) {
test.That(t, err, test.ShouldNotBeNil)
test.That(t, err.Error(), test.ShouldContainSubstring, `"id" is required`)

_, err = config.FromReader(context.Background(), "somepath", strings.NewReader(`{"components": [{}]}`), logger)
_, err = config.FromReader(context.Background(), "somepath", strings.NewReader(`{"disablepartialstart":true,"components": [{}]}`), logger)
test.That(t, err, test.ShouldNotBeNil)
test.That(t, err.Error(), test.ShouldContainSubstring, `components.0`)
test.That(t, err.Error(), test.ShouldContainSubstring, `"name" is required`)
Expand Down
33 changes: 33 additions & 0 deletions robot/impl/local_robot_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1393,6 +1393,39 @@ func TestGetRemoteResourceAndGrandFather(t *testing.T) {
test.That(t, err, test.ShouldBeError, "more that one remote resources with name \"pieceArm\" exists")
}

func TestValidationErrorOnReconfigure(t *testing.T) {
logger := golog.NewTestLogger(t)
ctx := context.Background()

badConfig := &config.Config{
Components: []config.Component{
{
Namespace: resource.ResourceNamespaceRDK,
Name: "",
Type: base.SubtypeName,
Model: "random",
},
},
Cloud: &config.Cloud{},
}
r, err := robotimpl.New(ctx, badConfig, logger)
defer func() {
test.That(t, r.Close(context.Background()), test.ShouldBeNil)
}()
test.That(t, err, test.ShouldBeNil)
test.That(t, r, test.ShouldNotBeNil)

name := base.Named("")
noBase, err := r.ResourceByName(name)
test.That(
t,
err,
test.ShouldBeError,
errors.Wrapf(utils.NewConfigValidationFieldRequiredError("", "name"), "resource %q not available", name),
)
test.That(t, noBase, test.ShouldBeNil)
}

func TestResourceStartsOnReconfigure(t *testing.T) {
logger := golog.NewTestLogger(t)
ctx := context.Background()
Expand Down
15 changes: 15 additions & 0 deletions robot/impl/resource_manager.go
Original file line number Diff line number Diff line change
Expand Up @@ -377,6 +377,11 @@ func (manager *resourceManager) completeConfig(
}
manager.logger.Debugw("we are now handling the resource", "resource", r)
if c, ok := wrap.config.(config.Component); ok {
_, err := c.Validate("")
if err != nil {
cheukt marked this conversation as resolved.
Show resolved Hide resolved
wrap.err = err
continue
}
iface, err := manager.processComponent(ctx, r, c, wrap.real, robot)
if err != nil {
manager.logger.Errorw("error building component", "resource", c.ResourceName(), "model", c.Model, "error", err)
Expand All @@ -385,6 +390,11 @@ func (manager *resourceManager) completeConfig(
}
manager.resources.AddNode(r, iface)
} else if s, ok := wrap.config.(config.Service); ok {
_, err := s.Validate("")
if err != nil {
wrap.err = err
continue
}
iface, err := manager.processService(ctx, s, wrap.real, robot)
if err != nil {
manager.logger.Errorw("error building service", "resource", s.ResourceName(), "model", s.Model, "error", err)
Expand All @@ -393,6 +403,11 @@ func (manager *resourceManager) completeConfig(
}
manager.resources.AddNode(r, iface)
} else if rc, ok := wrap.config.(config.Remote); ok {
err := rc.Validate("")
if err != nil {
wrap.err = err
continue
}
rr, err := manager.processRemote(ctx, rc)
if err != nil {
manager.logger.Errorw("error connecting to remote", "remote", rc.Name, "error", err)
Expand Down