diff --git a/cli/module_registry.go b/cli/module_registry.go index 8153f777824..e456183142c 100644 --- a/cli/module_registry.go +++ b/cli/module_registry.go @@ -625,7 +625,7 @@ func loadManifestOrNil(path string) (*moduleManifest, error) { if err == nil { return &manifest, nil } - if os.IsNotExist(err) { + if errors.Is(err, fs.ErrNotExist) { //nolint:nilnil return nil, nil } diff --git a/config/module.go b/config/module.go index 03b7a7cf2ab..c40315c56f7 100644 --- a/config/module.go +++ b/config/module.go @@ -43,6 +43,9 @@ type Module struct { Status *AppValidationStatus `json:"status"` alreadyValidated bool cachedErr error + + // LocalVersion is an in-process fake version used for local module change management. + LocalVersion string } // JSONManifest contains meta.json fields that are used by both RDK and CLI. @@ -126,7 +129,7 @@ func (m Module) SyntheticPackage() (PackageConfig, error) { } else { name = m.Name } - return PackageConfig{Name: name, Package: name, Type: PackageTypeModule}, nil + return PackageConfig{Name: name, Package: name, Type: PackageTypeModule, Version: m.LocalVersion}, nil } // exeDir returns the parent directory for the unpacked module. diff --git a/config/reader.go b/config/reader.go index 0a5246aab6c..7a6a4a9b1f1 100644 --- a/config/reader.go +++ b/config/reader.go @@ -291,6 +291,9 @@ func readFromCloud( locationID := cfg.Cloud.LocationID machineID := cfg.Cloud.MachineID + // This resets the new config's Cloud section to the original we loaded from file, + // but allows several fields to be updated, and merges the TLS certs which come + // from a different endpoint. mergeCloudConfig := func(to *Config) { *to.Cloud = *cloudCfg to.Cloud.FQDN = fqdn diff --git a/robot/impl/local_robot.go b/robot/impl/local_robot.go index a83e90e9a74..6470423bba1 100644 --- a/robot/impl/local_robot.go +++ b/robot/impl/local_robot.go @@ -12,6 +12,7 @@ import ( "sync/atomic" "time" + "github.com/Masterminds/semver/v3" "github.com/pkg/errors" "go.uber.org/multierr" packagespb "go.viam.com/api/app/packages/v1" @@ -74,6 +75,10 @@ type localRobot struct { // internal services that are in the graph but we also hold onto webSvc web.Service frameSvc framesystem.Service + + // map keyed by Module.Name. This is necessary to get the package manager to use a new folder + // when a local tarball is updated. + localModuleVersions map[string]semver.Version } // ExportResourcesAsDot exports the resource graph as a DOT representation for @@ -414,6 +419,7 @@ func newWithResources( revealSensitiveConfigDiffs: rOpts.revealSensitiveConfigDiffs, cloudConnSvc: icloud.NewCloudConnectionService(cfg.Cloud, logger), shutdownCallback: rOpts.shutdownCallback, + localModuleVersions: make(map[string]semver.Version), } r.mostRecentCfg.Store(config.Config{}) var heartbeatWindow time.Duration @@ -1133,6 +1139,20 @@ func (r *localRobot) Reconfigure(ctx context.Context, newConfig *config.Config) r.reconfigure(ctx, newConfig, false) } +// set Module.LocalVersion on Type=local modules. Call this before localPackages.Sync and in RestartModule. +func (r *localRobot) applyLocalModuleVersions(cfg *config.Config) { + for i := range cfg.Modules { + mod := &cfg.Modules[i] + if mod.Type == config.ModuleTypeLocal { + if ver, ok := r.localModuleVersions[mod.Name]; ok { + mod.LocalVersion = ver.String() + } else { + mod.LocalVersion = semver.Version{}.String() + } + } + } +} + func (r *localRobot) reconfigure(ctx context.Context, newConfig *config.Config, forceSync bool) { var allErrs error @@ -1145,6 +1165,11 @@ func (r *localRobot) reconfigure(ctx context.Context, newConfig *config.Config, if err != nil { allErrs = multierr.Combine(allErrs, err) } + // For local tarball modules, we create synthetic versions for package management. The `localRobot` keeps track of these because + // config reader would overwrite if we just stored it in config. Here, we copy the synthetic version from the `localRobot` into the + // appropriate `config.Module` object inside the `cfg.Modules` slice. Thus, when a local tarball module is reloaded, the viam-server + // can unpack it into a fresh directory rather than reusing the previous one. + r.applyLocalModuleVersions(newConfig) allErrs = multierr.Combine(allErrs, r.localPackages.Sync(ctx, newConfig.Packages, newConfig.Modules)) // Add default services and process their dependencies. Dependencies may @@ -1286,27 +1311,27 @@ func (r *localRobot) CloudMetadata(ctx context.Context) (cloud.Metadata, error) } // restartSingleModule constructs a single-module diff and calls updateResources with it. -func (r *localRobot) restartSingleModule(ctx context.Context, mod config.Module) error { +func (r *localRobot) restartSingleModule(ctx context.Context, mod *config.Module) error { + if mod.Type == config.ModuleTypeLocal { + // note: this version incrementing matters for local tarballs because we want the system to + // unpack into a new directory rather than reusing the old one. Local tarball path matters + // here because it is how artifacts are unpacked for remote reloading. + // We technically only need to do this when mod.NeedsSyntheticPackage(), but we do it + // for all local packages for test suite reasons. + nextVer := r.localModuleVersions[mod.Name].IncPatch() + r.localModuleVersions[mod.Name] = nextVer + mod.LocalVersion = nextVer.String() + r.logger.CInfof(ctx, "incremented local version of %s to %s", mod.Name, mod.LocalVersion) + err := r.localPackages.SyncOne(ctx, *mod) + if err != nil { + return err + } + } diff := config.Diff{ Left: r.Config(), Right: r.Config(), Added: &config.Config{}, - Modified: &config.ModifiedConfigDiff{}, - Removed: &config.Config{Modules: []config.Module{mod}}, - } - err := r.manager.updateResources(ctx, &diff) - if err != nil { - return err - } - err = r.localPackages.SyncOne(ctx, mod) - if err != nil { - return err - } - diff = config.Diff{ - Left: r.Config(), - Right: r.Config(), - Added: &config.Config{Modules: []config.Module{mod}}, - Modified: &config.ModifiedConfigDiff{}, + Modified: &config.ModifiedConfigDiff{Modules: []config.Module{*mod}}, Removed: &config.Config{}, } return r.manager.updateResources(ctx, &diff) @@ -1319,7 +1344,7 @@ func (r *localRobot) RestartModule(ctx context.Context, req robot.RestartModuleR "module not found with id=%s, name=%s. make sure it is configured and running on your machine", req.ModuleID, req.ModuleName) } - err := r.restartSingleModule(ctx, *mod) + err := r.restartSingleModule(ctx, mod) if err != nil { return errors.Wrapf(err, "while restarting module id=%s, name=%s", req.ModuleID, req.ModuleName) } diff --git a/robot/impl/local_robot_test.go b/robot/impl/local_robot_test.go index 10c4b7df8d1..7bf205cf1ec 100644 --- a/robot/impl/local_robot_test.go +++ b/robot/impl/local_robot_test.go @@ -3517,3 +3517,18 @@ func TestSendTriggerConfig(t *testing.T) { } test.That(t, len(actualR.triggerConfig), test.ShouldEqual, 1) } + +func TestRestartModule(t *testing.T) { + ctx := context.Background() + logger := logging.NewTestLogger(t) + simplePath := rtestutils.BuildTempModule(t, "examples/customresources/demos/simplemodule") + mod := &config.Module{Name: "restartSingleModule-test", ExePath: simplePath, Type: config.ModuleTypeLocal} + r := setupLocalRobot(t, ctx, &config.Config{Modules: []config.Module{*mod}}, logger) + test.That(t, mod.LocalVersion, test.ShouldBeEmpty) + + // test restart. note: we're not testing that the PID rolls over because we don't have access to + // that state. 'no error' + 'version incremented' is a cheap proxy for that. + err := r.(*localRobot).restartSingleModule(ctx, mod) + test.That(t, err, test.ShouldBeNil) + test.That(t, mod.LocalVersion, test.ShouldResemble, "0.0.1") +}