From 974351ec66abe13c2567779ccd94e8e576c1fe99 Mon Sep 17 00:00:00 2001 From: Bashar Eid Date: Thu, 2 Jan 2025 18:04:49 -0500 Subject: [PATCH 01/50] pass directories --- config/module.go | 4 ++++ module/modmanager/manager.go | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/config/module.go b/config/module.go index 604e9ea0bd4..e5cc20bd0fe 100644 --- a/config/module.go +++ b/config/module.go @@ -249,7 +249,11 @@ func (m *Module) FirstRun( dataDir string, env map[string]string, logger logging.Logger, + packagesDir, + viamHomeDir string, ) error { + fmt.Println("PACKAGES DIRECTORY: ", packagesDir) + fmt.Println("VIAM HOME DIRECTORY: ", viamHomeDir) logger = logger.Sublogger("first_run").WithFields("module", m.Name) unpackedModDir, err := m.exeDir(localPackagesDir) diff --git a/module/modmanager/manager.go b/module/modmanager/manager.go index 194532c4fad..a16a62e9a51 100644 --- a/module/modmanager/manager.go +++ b/module/modmanager/manager.go @@ -1107,7 +1107,7 @@ func (mgr *Manager) FirstRun(ctx context.Context, conf config.Module) error { } env := getFullEnvironment(conf, dataDir, mgr.viamHomeDir) - return conf.FirstRun(ctx, pkgsDir, dataDir, env, mgr.logger) + return conf.FirstRun(ctx, pkgsDir, dataDir, env, mgr.logger, mgr.packagesDir, mgr.viamHomeDir) } func (m *module) startProcess( From d15c8c15b737f1d52c62759d93a28798308be3ba Mon Sep 17 00:00:00 2001 From: Bashar Eid Date: Fri, 3 Jan 2025 15:09:14 -0500 Subject: [PATCH 02/50] search through top-level directory --- config/module.go | 41 ++++++++++++++++++++++++++++++------ module/modmanager/manager.go | 5 ++++- 2 files changed, 38 insertions(+), 8 deletions(-) diff --git a/config/module.go b/config/module.go index e5cc20bd0fe..0f766102bb2 100644 --- a/config/module.go +++ b/config/module.go @@ -249,11 +249,7 @@ func (m *Module) FirstRun( dataDir string, env map[string]string, logger logging.Logger, - packagesDir, - viamHomeDir string, ) error { - fmt.Println("PACKAGES DIRECTORY: ", packagesDir) - fmt.Println("VIAM HOME DIRECTORY: ", viamHomeDir) logger = logger.Sublogger("first_run").WithFields("module", m.Name) unpackedModDir, err := m.exeDir(localPackagesDir) @@ -271,7 +267,7 @@ func (m *Module) FirstRun( // Load the module's meta.json. If it doesn't exist DEBUG log and exit quietly. // For all other errors WARN log and exit. - meta, err := m.getJSONManifest(unpackedModDir) + meta, err := m.getJSONManifest(unpackedModDir, env) var pathErr *os.PathError switch { case errors.As(err, &pathErr): @@ -385,7 +381,7 @@ func (m *Module) FirstRun( // 1. if there is a meta.json in the exe dir, use that, except in local non-tarball case. // 2. if this is a local tarball and there's a meta.json next to the tarball, use that. // Note: the working directory must be the unpacked tarball directory or local exec directory. -func (m Module) getJSONManifest(unpackedModDir string) (*JSONManifest, error) { +func (m Module) getJSONManifest(unpackedModDir string, env map[string]string) (*JSONManifest, error) { // note: we don't look at internal meta.json in local non-tarball case because user has explicitly requested a binary. localNonTarball := m.Type == ModuleTypeLocal && !m.NeedsSyntheticPackage() if !localNonTarball { @@ -418,5 +414,36 @@ func (m Module) getJSONManifest(unpackedModDir string) (*JSONManifest, error) { } return meta, err } - return nil, errors.New("failed to find meta.json") + + moduleWorkingDirectory, ok := env["VIAM_MODULE_ROOT"] + if ok { + metaPath, err := utils.SafeJoinDir(moduleWorkingDirectory, "meta.json") + if err != nil { + return nil, err + } + _, err = os.Stat(metaPath) + if err == nil { + meta, err := parseJSONFile[JSONManifest](metaPath) + if err != nil { + return nil, err + } + return meta, nil + } + } + metaPath, err := utils.SafeJoinDir(unpackedModDir, "meta.json") + if err != nil { + return nil, err + } + _, err = os.Stat(metaPath) + if err == nil { + meta, err := parseJSONFile[JSONManifest](metaPath) + if err != nil { + return nil, err + } + return meta, nil + } + if !ok { + return nil, errors.Errorf("VIAM_MODULE_ROOT not set. Failed to find meta.json in executable directory %s", metaPath) + } + return nil, errors.Errorf("failed to find meta.json. Searched in path set by VIAM_MODULE_ROOT %s and executable directory %s", moduleWorkingDirectory, metaPath) } diff --git a/module/modmanager/manager.go b/module/modmanager/manager.go index a16a62e9a51..dc7c8822aef 100644 --- a/module/modmanager/manager.go +++ b/module/modmanager/manager.go @@ -1098,6 +1098,7 @@ func (mgr *Manager) FirstRun(ctx context.Context, conf config.Module) error { // This value is normally set on a field on the [module] struct but it seems like we can safely get it on demand. var dataDir string if mgr.moduleDataParentDir != "" { + fmt.Println("MODULE DATA PARENT DIRECTORY: ", mgr.moduleDataParentDir) var err error // TODO: why isn't conf.Name being sanitized like PackageConfig.SanitizedName? dataDir, err = rutils.SafeJoinDir(mgr.moduleDataParentDir, conf.Name) @@ -1106,8 +1107,9 @@ func (mgr *Manager) FirstRun(ctx context.Context, conf config.Module) error { } } env := getFullEnvironment(conf, dataDir, mgr.viamHomeDir) + fmt.Println("ENV: ", env) - return conf.FirstRun(ctx, pkgsDir, dataDir, env, mgr.logger, mgr.packagesDir, mgr.viamHomeDir) + return conf.FirstRun(ctx, pkgsDir, dataDir, env, mgr.logger) } func (m *module) startProcess( @@ -1139,6 +1141,7 @@ func (m *module) startProcess( moduleEnvironment := m.getFullEnvironment(viamHomeDir) // Prefer VIAM_MODULE_ROOT as the current working directory if present but fallback to the directory of the exepath moduleWorkingDirectory, ok := moduleEnvironment["VIAM_MODULE_ROOT"] + fmt.Println("MODULE WORKING DIRECTORY: ", moduleWorkingDirectory) if !ok { moduleWorkingDirectory = filepath.Dir(absoluteExePath) m.logger.CDebugw(ctx, "VIAM_MODULE_ROOT was not passed to module. Defaulting to module's working directory", From 249ff76574b0053c4e9b66bcc592d6768ca090e0 Mon Sep 17 00:00:00 2001 From: Bashar Eid Date: Fri, 3 Jan 2025 15:16:57 -0500 Subject: [PATCH 03/50] restore manager.go file --- module/modmanager/manager.go | 177 ++++++++++++++--------------------- 1 file changed, 71 insertions(+), 106 deletions(-) diff --git a/module/modmanager/manager.go b/module/modmanager/manager.go index dc7c8822aef..df42a710b64 100644 --- a/module/modmanager/manager.go +++ b/module/modmanager/manager.go @@ -30,8 +30,6 @@ import ( "google.golang.org/protobuf/types/known/structpb" "go.viam.com/rdk/config" - "go.viam.com/rdk/ftdc" - "go.viam.com/rdk/ftdc/sys" rdkgrpc "go.viam.com/rdk/grpc" "go.viam.com/rdk/logging" modlib "go.viam.com/rdk/module" @@ -165,14 +163,7 @@ func (rmap *resourceModuleMap) Range(f func(name resource.Name, mod *module) boo // Manager is the root structure for the module system. type Manager struct { - // mu (mostly) coordinates API methods that modify the `modules` map. Specifically, - // `AddResource` can be called concurrently during a reconfigure. But `RemoveResource` or - // resources being restarted after a module crash are given exclusive access. - // - // mu additionally is used for exclusive access when `Add`ing modules (as opposed to resources), - // `Reconfigure`ing modules, `Remove`ing modules and `Close`ing the `Manager`. - mu sync.RWMutex - + mu sync.RWMutex logger logging.Logger modules moduleMap parentAddr string @@ -213,6 +204,9 @@ func (mgr *Manager) Close(ctx context.Context) error { // Handles returns all the models for each module registered. func (mgr *Manager) Handles() map[string]modlib.HandlerMap { + mgr.mu.Lock() + defer mgr.mu.Unlock() + res := map[string]modlib.HandlerMap{} mgr.modules.Range(func(n string, m *module) bool { @@ -285,12 +279,11 @@ func (mgr *Manager) Add(ctx context.Context, confs ...config.Module) error { wg.Add(1) go func(i int, conf config.Module) { defer wg.Done() - moduleLogger := mgr.logger.Sublogger(conf.Name) - moduleLogger.CInfow(ctx, "Now adding module", "module", conf.Name) - err := mgr.add(ctx, conf, moduleLogger) + mgr.logger.CInfow(ctx, "Now adding module", "module", conf.Name) + err := mgr.add(ctx, conf) if err != nil { - moduleLogger.CErrorw(ctx, "Error adding module", "module", conf.Name, "error", err) + mgr.logger.CErrorw(ctx, "Error adding module", "module", conf.Name, "error", err) errs[i] = err return } @@ -309,10 +302,9 @@ func (mgr *Manager) Add(ctx context.Context, confs ...config.Module) error { return combinedErr } -func (mgr *Manager) add(ctx context.Context, conf config.Module, moduleLogger logging.Logger) error { +func (mgr *Manager) add(ctx context.Context, conf config.Module) error { _, exists := mgr.modules.Load(conf.Name) if exists { - // Keeping this as a manager logger since it is dealing with manager behavior mgr.logger.CWarnw(ctx, "Not adding module that already exists", "module", conf.Name) return nil } @@ -332,9 +324,9 @@ func (mgr *Manager) add(ctx context.Context, conf config.Module, moduleLogger lo cfg: conf, dataDir: moduleDataDir, resources: map[resource.Name]*addedResource{}, - logger: moduleLogger, ftdc: mgr.ftdc, - port: int(mgr.nextPort.Add(1)), + port: mgr.nextPort, + logger: mgr.logger.Sublogger(conf.Name), } if err := mgr.startModule(ctx, mod); err != nil { @@ -348,6 +340,7 @@ func (mgr *Manager) startModuleProcess(mod *module) error { mgr.restartCtx, mgr.parentAddr, mgr.newOnUnexpectedExitHandler(mod), + mgr.logger, mgr.viamHomeDir, mgr.packagesDir, ) @@ -364,20 +357,20 @@ func (mgr *Manager) startModule(ctx context.Context, mod *module) error { var success bool defer func() { if !success { - mod.cleanupAfterStartupFailure() + mod.cleanupAfterStartupFailure(mgr.logger) } }() // create the module's data directory if mod.dataDir != "" { - mod.logger.Debugf("Creating data directory %q for module %q", mod.dataDir, mod.cfg.Name) + mgr.logger.Infof("Creating data directory %q for module %q", mod.dataDir, mod.cfg.Name) if err := os.MkdirAll(mod.dataDir, 0o750); err != nil { return errors.WithMessage(err, "error while creating data directory for module "+mod.cfg.Name) } } cleanup := rutils.SlowStartupLogger( - ctx, "Waiting for module to complete startup and registration", "module", mod.cfg.Name, mod.logger) + ctx, "Waiting for module to complete startup and registration", "module", mod.cfg.Name, mgr.logger) defer cleanup() if err := mgr.startModuleProcess(mod); err != nil { @@ -388,13 +381,13 @@ func (mgr *Manager) startModule(ctx context.Context, mod *module) error { return errors.WithMessage(err, "error while dialing module "+mod.cfg.Name) } - if err := mod.checkReady(ctx, mgr.parentAddr); err != nil { + if err := mod.checkReady(ctx, mgr.parentAddr, mgr.logger); err != nil { return errors.WithMessage(err, "error while waiting for module to be ready "+mod.cfg.Name) } - mod.registerResources(mgr) + mod.registerResources(mgr, mgr.logger) mgr.modules.Store(mod.cfg.Name, mod) - mod.logger.Infow("Module successfully added", "module", mod.cfg.Name) + mgr.logger.Infow("Module successfully added", "module", mod.cfg.Name) success = true return nil } @@ -417,7 +410,7 @@ func (mgr *Manager) Reconfigure(ctx context.Context, conf config.Module) ([]reso handledResourceNameStrings = append(handledResourceNameStrings, name.String()) } - mod.logger.CInfow(ctx, "Module configuration changed. Stopping the existing module process", "module", conf.Name) + mgr.logger.CInfow(ctx, "Module configuration changed. Stopping the existing module process", "module", conf.Name) if err := mgr.closeModule(mod, true); err != nil { // If removal fails, assume all handled resources are orphaned. @@ -427,17 +420,17 @@ func (mgr *Manager) Reconfigure(ctx context.Context, conf config.Module) ([]reso mod.cfg = conf mod.resources = map[resource.Name]*addedResource{} - mod.logger.CInfow(ctx, "Existing module process stopped. Starting new module process", "module", conf.Name) + mgr.logger.CInfow(ctx, "Existing module process stopped. Starting new module process", "module", conf.Name) if err := mgr.startModule(ctx, mod); err != nil { // If re-addition fails, assume all handled resources are orphaned. return handledResourceNames, err } - mod.logger.CInfow(ctx, "New module process is running and responding to gRPC requests", "module", + mgr.logger.CInfow(ctx, "New module process is running and responding to gRPC requests", "module", mod.cfg.Name, "module address", mod.addr) - mod.logger.CInfow(ctx, "Resources handled by reconfigured module will be re-added to new module process", + mgr.logger.CInfow(ctx, "Resources handled by reconfigured module will be re-added to new module process", "module", mod.cfg.Name, "resources", handledResourceNameStrings) return handledResourceNames, nil } @@ -480,16 +473,16 @@ func (mgr *Manager) Remove(modName string) ([]resource.Name, error) { func (mgr *Manager) closeModule(mod *module, reconfigure bool) error { // resource manager should've removed these cleanly if this isn't a reconfigure if !reconfigure && len(mod.resources) != 0 { - mod.logger.Warnw("Forcing removal of module with active resources", "module", mod.cfg.Name) + mgr.logger.Warnw("Forcing removal of module with active resources", "module", mod.cfg.Name) } // need to actually close the resources within the module itself before stopping for res := range mod.resources { _, err := mod.client.RemoveResource(context.Background(), &pb.RemoveResourceRequest{Name: res.String()}) if err != nil { - mod.logger.Errorw("Error removing resource", "module", mod.cfg.Name, "resource", res.Name, "error", err) + mgr.logger.Errorw("Error removing resource", "module", mod.cfg.Name, "resource", res.Name, "error", err) } else { - mod.logger.Infow("Successfully removed resource from module", "module", mod.cfg.Name, "resource", res.Name) + mgr.logger.Infow("Successfully removed resource from module", "module", mod.cfg.Name, "resource", res.Name) } } @@ -498,7 +491,7 @@ func (mgr *Manager) closeModule(mod *module, reconfigure bool) error { } if err := mod.sharedConn.Close(); err != nil { - mod.logger.Warnw("Error closing connection to module", "error", err) + mgr.logger.Warnw("Error closing connection to module", "error", err) } mod.deregisterResources() @@ -511,7 +504,7 @@ func (mgr *Manager) closeModule(mod *module, reconfigure bool) error { }) mgr.modules.Delete(mod.cfg.Name) - mod.logger.Infow("Module successfully closed", "module", mod.cfg.Name) + mgr.logger.Infow("Module successfully closed", "module", mod.cfg.Name) return nil } @@ -534,7 +527,7 @@ func (mgr *Manager) addResource(ctx context.Context, conf resource.Config, deps return nil, errors.Errorf("no active module registered to serve resource api %s and model %s", conf.API, conf.Model) } - mod.logger.CInfow(ctx, "Adding resource to module", "resource", conf.Name, "module", mod.cfg.Name) + mgr.logger.CInfow(ctx, "Adding resource to module", "resource", conf.Name, "module", mod.cfg.Name) confProto, err := config.ComponentConfigToProto(&conf) if err != nil { @@ -553,7 +546,7 @@ func (mgr *Manager) addResource(ctx context.Context, conf resource.Config, deps apiInfo, ok := resource.LookupGenericAPIRegistration(conf.API) if !ok || apiInfo.RPCClient == nil { - mod.logger.CWarnw(ctx, "No built-in grpc client for modular resource", "resource", conf.ResourceName()) + mgr.logger.CWarnw(ctx, "No built-in grpc client for modular resource", "resource", conf.ResourceName()) return rdkgrpc.NewForeignResource(conf.ResourceName(), &mod.sharedConn), nil } @@ -562,12 +555,14 @@ func (mgr *Manager) addResource(ctx context.Context, conf resource.Config, deps // ReconfigureResource updates/reconfigures a modular component with a new configuration. func (mgr *Manager) ReconfigureResource(ctx context.Context, conf resource.Config, deps []string) error { + mgr.mu.RLock() + defer mgr.mu.RUnlock() mod, ok := mgr.getModule(conf) if !ok { return errors.Errorf("no module registered to serve resource api %s and model %s", conf.API, conf.Model) } - mod.logger.CInfow(ctx, "Reconfiguring resource for module", "resource", conf.Name, "module", mod.cfg.Name) + mgr.logger.CInfow(ctx, "Reconfiguring resource for module", "resource", conf.Name, "module", mod.cfg.Name) confProto, err := config.ComponentConfigToProto(&conf) if err != nil { @@ -577,7 +572,6 @@ func (mgr *Manager) ReconfigureResource(ctx context.Context, conf resource.Confi if err != nil { return err } - mod.resourcesMu.Lock() defer mod.resourcesMu.Unlock() mod.resources[conf.ResourceName()] = &addedResource{conf, deps} @@ -588,6 +582,8 @@ func (mgr *Manager) ReconfigureResource(ctx context.Context, conf resource.Confi // Configs returns a slice of config.Module representing the currently managed // modules. func (mgr *Manager) Configs() []config.Module { + mgr.mu.RLock() + defer mgr.mu.RUnlock() var configs []config.Module mgr.modules.Range(func(_ string, mod *module) bool { configs = append(configs, mod.cfg) @@ -598,12 +594,16 @@ func (mgr *Manager) Configs() []config.Module { // Provides returns true if a component/service config WOULD be handled by a module. func (mgr *Manager) Provides(conf resource.Config) bool { + mgr.mu.RLock() + defer mgr.mu.RUnlock() _, ok := mgr.getModule(conf) return ok } // IsModularResource returns true if an existing resource IS handled by a module. func (mgr *Manager) IsModularResource(name resource.Name) bool { + mgr.mu.RLock() + defer mgr.mu.RUnlock() _, ok := mgr.rMap.Load(name) return ok } @@ -617,7 +617,7 @@ func (mgr *Manager) RemoveResource(ctx context.Context, name resource.Name) erro return errors.Errorf("resource %+v not found in module", name) } - mod.logger.CInfow(ctx, "Removing resource for module", "resource", name.String(), "module", mod.cfg.Name) + mgr.logger.CInfow(ctx, "Removing resource for module", "resource", name.String(), "module", mod.cfg.Name) mgr.rMap.Delete(name) delete(mod.resources, name) @@ -636,6 +636,8 @@ func (mgr *Manager) RemoveResource(ctx context.Context, name resource.Name) erro // ValidateConfig determines whether the given config is valid and returns its implicit // dependencies. func (mgr *Manager) ValidateConfig(ctx context.Context, conf resource.Config) ([]string, error) { + mgr.mu.RLock() + defer mgr.mu.RUnlock() mod, ok := mgr.getModule(conf) if !ok { return nil, @@ -669,6 +671,8 @@ func (mgr *Manager) ValidateConfig(ctx context.Context, conf resource.Config) ([ // and modified resources. It also puts modular resources whose module has been modified or added in conf.Added if // they are not already there since the resources themselves are not necessarily new. func (mgr *Manager) ResolveImplicitDependenciesInConfig(ctx context.Context, conf *config.Diff) error { + mgr.mu.RLock() + defer mgr.mu.RUnlock() // NOTE(benji): We could simplify some of the following `continue` // conditional clauses to a single clause, but we split them for readability. for _, c := range conf.Right.Components { @@ -858,7 +862,7 @@ func (mgr *Manager) newOnUnexpectedExitHandler(mod *module) func(exitCode int) b defer mod.inStartup.Store(false) // Log error immediately, as this is unexpected behavior. - mod.logger.Errorw( + mgr.logger.Errorw( "Module has unexpectedly exited.", "module", mod.cfg.Name, "exit_code", exitCode, ) @@ -874,7 +878,7 @@ func (mgr *Manager) newOnUnexpectedExitHandler(mod *module) func(exitCode int) b if orphanedResourceNames := mgr.attemptRestart(mgr.restartCtx, mod); orphanedResourceNames != nil { if mgr.removeOrphanedResources != nil { mgr.removeOrphanedResources(mgr.restartCtx, orphanedResourceNames) - mod.logger.Debugw( + mgr.logger.Debugw( "Removed resources after failed module restart", "module", mod.cfg.Name, "resources", resource.NamesToStrings(orphanedResourceNames), @@ -882,7 +886,7 @@ func (mgr *Manager) newOnUnexpectedExitHandler(mod *module) func(exitCode int) b } return false } - mod.logger.Infow("Module successfully restarted, re-adding resources", "module", mod.cfg.Name) + mgr.logger.Infow("Module successfully restarted, re-adding resources", "module", mod.cfg.Name) // Otherwise, add old module process' resources to new module; warn if new // module cannot handle old resource and remove it from mod.resources. @@ -893,7 +897,7 @@ func (mgr *Manager) newOnUnexpectedExitHandler(mod *module) func(exitCode int) b // read lock, so we execute it here with a write lock to make sure it doesn't // run concurrently. if _, err := mgr.addResourceWithWriteLock(mgr.restartCtx, res.conf, res.deps); err != nil { - mod.logger.Warnw("Error while re-adding resource to module", + mgr.logger.Warnw("Error while re-adding resource to module", "resource", name, "module", mod.cfg.Name, "error", err) mgr.rMap.Delete(name) @@ -908,7 +912,7 @@ func (mgr *Manager) newOnUnexpectedExitHandler(mod *module) func(exitCode int) b mgr.removeOrphanedResources(mgr.restartCtx, orphanedResourceNames) } - mod.logger.Infow("Module resources successfully re-added after module restart", "module", mod.cfg.Name) + mgr.logger.Infow("Module resources successfully re-added after module restart", "module", mod.cfg.Name) return false } } @@ -957,7 +961,7 @@ func (mgr *Manager) attemptRestart(ctx context.Context, mod *module) []resource. // executable we were given for initial module addition. cleanup := rutils.SlowStartupLogger( - ctx, "Waiting for module to complete restart and re-registration", "module", mod.cfg.Name, mod.logger) + ctx, "Waiting for module to complete restart and re-registration", "module", mod.cfg.Name, mgr.logger) defer cleanup() // Attempt to restart module process 3 times. @@ -989,13 +993,13 @@ func (mgr *Manager) attemptRestart(ctx context.Context, mod *module) []resource. return orphanedResourceNames } - if err := mod.checkReady(ctx, mgr.parentAddr); err != nil { + if err := mod.checkReady(ctx, mgr.parentAddr, mgr.logger); err != nil { mgr.logger.CErrorw(ctx, "Error while waiting for restarted module to be ready", "module", mod.cfg.Name, "error", err) return orphanedResourceNames } - mod.registerResources(mgr) + mod.registerResources(mgr, mgr.logger) success = true return nil @@ -1046,11 +1050,11 @@ func (m *module) dial() error { // checkReady sends a `ReadyRequest` and waits for either a `ReadyResponse`, or a context // cancelation. -func (m *module) checkReady(ctx context.Context, parentAddr string) error { - ctxTimeout, cancelFunc := context.WithTimeout(ctx, rutils.GetModuleStartupTimeout(m.logger)) +func (m *module) checkReady(ctx context.Context, parentAddr string, logger logging.Logger) error { + ctxTimeout, cancelFunc := context.WithTimeout(ctx, rutils.GetModuleStartupTimeout(logger)) defer cancelFunc() - m.logger.CInfow(ctx, "Waiting for module to respond to ready request", "module", m.cfg.Name) + logger.CInfow(ctx, "Waiting for module to respond to ready request", "module", m.cfg.Name) req := &pb.ReadyRequest{ParentAddress: parentAddr} @@ -1058,7 +1062,7 @@ func (m *module) checkReady(ctx context.Context, parentAddr string) error { var err error req.WebrtcOffer, err = m.sharedConn.GenerateEncodedOffer() if err != nil { - m.logger.CWarnw(ctx, "Unable to generate offer for module PeerConnection. Ignoring.", "err", err) + logger.CWarnw(ctx, "Unable to generate offer for module PeerConnection. Ignoring.", "err", err) } for { @@ -1080,7 +1084,7 @@ func (m *module) checkReady(ctx context.Context, parentAddr string) error { err = m.sharedConn.ProcessEncodedAnswer(resp.WebrtcAnswer) if err != nil { - m.logger.CWarnw(ctx, "Unable to create PeerConnection with module. Ignoring.", "err", err) + logger.CWarnw(ctx, "Unable to create PeerConnection with module. Ignoring.", "err", err) } // The `ReadyRespones` also includes the Viam `API`s and `Model`s the module provides. This @@ -1098,7 +1102,6 @@ func (mgr *Manager) FirstRun(ctx context.Context, conf config.Module) error { // This value is normally set on a field on the [module] struct but it seems like we can safely get it on demand. var dataDir string if mgr.moduleDataParentDir != "" { - fmt.Println("MODULE DATA PARENT DIRECTORY: ", mgr.moduleDataParentDir) var err error // TODO: why isn't conf.Name being sanitized like PackageConfig.SanitizedName? dataDir, err = rutils.SafeJoinDir(mgr.moduleDataParentDir, conf.Name) @@ -1107,7 +1110,6 @@ func (mgr *Manager) FirstRun(ctx context.Context, conf config.Module) error { } } env := getFullEnvironment(conf, dataDir, mgr.viamHomeDir) - fmt.Println("ENV: ", env) return conf.FirstRun(ctx, pkgsDir, dataDir, env, mgr.logger) } @@ -1116,6 +1118,7 @@ func (m *module) startProcess( ctx context.Context, parentAddr string, oue func(int) bool, + logger logging.Logger, viamHomeDir string, packagesDir string, ) error { @@ -1141,13 +1144,12 @@ func (m *module) startProcess( moduleEnvironment := m.getFullEnvironment(viamHomeDir) // Prefer VIAM_MODULE_ROOT as the current working directory if present but fallback to the directory of the exepath moduleWorkingDirectory, ok := moduleEnvironment["VIAM_MODULE_ROOT"] - fmt.Println("MODULE WORKING DIRECTORY: ", moduleWorkingDirectory) if !ok { moduleWorkingDirectory = filepath.Dir(absoluteExePath) - m.logger.CDebugw(ctx, "VIAM_MODULE_ROOT was not passed to module. Defaulting to module's working directory", + logger.CWarnw(ctx, "VIAM_MODULE_ROOT was not passed to module. Defaulting to module's working directory", "module", m.cfg.Name, "dir", moduleWorkingDirectory) } else { - m.logger.CInfow(ctx, "Starting module in working directory", "module", m.cfg.Name, "dir", moduleWorkingDirectory) + logger.CInfow(ctx, "Starting module in working directory", "module", m.cfg.Name, "dir", moduleWorkingDirectory) } pconf := pexec.ProcessConfig{ @@ -1163,25 +1165,21 @@ func (m *module) startProcess( // supplied and module manager has a DebugLevel logger. if m.cfg.LogLevel != "" { pconf.Args = append(pconf.Args, fmt.Sprintf(logLevelArgumentTemplate, m.cfg.LogLevel)) - } else if m.logger.Level().Enabled(zapcore.DebugLevel) { + } else if logger.Level().Enabled(zapcore.DebugLevel) { pconf.Args = append(pconf.Args, fmt.Sprintf(logLevelArgumentTemplate, "debug")) } - m.process = pexec.NewManagedProcess(pconf, m.logger) + m.process = pexec.NewManagedProcess(pconf, logger) if err := m.process.Start(context.Background()); err != nil { return errors.WithMessage(err, "module startup failed") } - // Turn on process cpu/memory diagnostics for the module process. If there's an error, we - // continue normally, just without FTDC. - m.registerProcessWithFTDC() - checkTicker := time.NewTicker(100 * time.Millisecond) defer checkTicker.Stop() - m.logger.CInfow(ctx, "Starting up module", "module", m.cfg.Name) - ctxTimeout, cancel := context.WithTimeout(ctx, rutils.GetModuleStartupTimeout(m.logger)) + logger.CInfow(ctx, "Starting up module", "module", m.cfg.Name) + ctxTimeout, cancel := context.WithTimeout(ctx, rutils.GetModuleStartupTimeout(logger)) defer cancel() for { select { @@ -1217,21 +1215,9 @@ func (m *module) stopProcess() error { if m.process == nil { return nil } - - m.logger.Infof("Stopping module: %s process", m.cfg.Name) - // Attempt to remove module's .sock file if module did not remove it // already. - defer func() { - rutils.RemoveFileNoError(m.addr) - - // The system metrics "statser" is resilient to the process dying under the hood. An empty set - // of metrics will be reported. Therefore it is safe to continue monitoring the module process - // while it's in shutdown. - if m.ftdc != nil { - m.ftdc.Remove(m.process.ID()) - } - }() + defer rutils.RemoveFileNoError(m.addr) // TODO(RSDK-2551): stop ignoring exit status 143 once Python modules handle // SIGTERM correctly. @@ -1242,11 +1228,10 @@ func (m *module) stopProcess() error { } return err } - return nil } -func (m *module) registerResources(mgr modmaninterface.ModuleManager) { +func (m *module) registerResources(mgr modmaninterface.ModuleManager, logger logging.Logger) { for api, models := range m.handles { if _, ok := resource.LookupGenericAPIRegistration(api.API); !ok { resource.RegisterAPI( @@ -1258,7 +1243,7 @@ func (m *module) registerResources(mgr modmaninterface.ModuleManager) { switch { case api.API.IsComponent(): for _, model := range models { - m.logger.Infow("Registering component API and model from module", "module", m.cfg.Name, "API", api.API, "model", model) + logger.Infow("Registering component API and model from module", "module", m.cfg.Name, "API", api.API, "model", model) // We must copy because the Discover closure func relies on api and model, but they are iterators and mutate. // Copying prevents mutation. modelCopy := model @@ -1273,13 +1258,13 @@ func (m *module) registerResources(mgr modmaninterface.ModuleManager) { return mgr.AddResource(ctx, conf, DepsToNames(deps)) }, Discover: func(ctx context.Context, logger logging.Logger, extra map[string]interface{}) (interface{}, error) { - extraStructPb, err := structpb.NewStruct(extra) + extraStruct, err := structpb.NewStruct(extra) if err != nil { return nil, err } req := &robotpb.DiscoverComponentsRequest{ Queries: []*robotpb.DiscoveryQuery{ - {Subtype: apiCopy.API.String(), Model: modelCopy.String(), Extra: extraStructPb}, + {Subtype: apiCopy.API.String(), Model: modelCopy.String(), Extra: extraStruct}, }, } @@ -1301,7 +1286,7 @@ func (m *module) registerResources(mgr modmaninterface.ModuleManager) { } case api.API.IsService(): for _, model := range models { - m.logger.Infow("Registering service API and model from module", "module", m.cfg.Name, "API", api.API, "model", model) + logger.Infow("Registering service API and model from module", "module", m.cfg.Name, "API", api.API, "model", model) resource.RegisterService(api.API, model, resource.Registration[resource.Resource, resource.NoNativeConfig]{ Constructor: func( ctx context.Context, @@ -1314,7 +1299,7 @@ func (m *module) registerResources(mgr modmaninterface.ModuleManager) { }) } default: - m.logger.Errorw("Invalid module type", "API type", api.API.Type) + logger.Errorw("Invalid module type", "API type", api.API.Type) } } } @@ -1328,10 +1313,10 @@ func (m *module) deregisterResources() { m.handles = nil } -func (m *module) cleanupAfterStartupFailure() { +func (m *module) cleanupAfterStartupFailure(logger logging.Logger) { if err := m.stopProcess(); err != nil { msg := "Error while stopping process of module that failed to start" - m.logger.Errorw(msg, "module", m.cfg.Name, "error", err) + logger.Errorw(msg, "module", m.cfg.Name, "error", err) } utils.UncheckedError(m.sharedConn.Close()) } @@ -1351,26 +1336,6 @@ func (m *module) getFullEnvironment(viamHomeDir string) map[string]string { return getFullEnvironment(m.cfg, m.dataDir, viamHomeDir) } -func (m *module) registerProcessWithFTDC() { - if m.ftdc == nil { - return - } - - pid, err := m.process.UnixPid() - if err != nil { - m.logger.Warnw("Module process has no pid. Cannot start ftdc.", "err", err) - return - } - - statser, err := sys.NewPidSysUsageStatser(pid) - if err != nil { - m.logger.Warnw("Cannot find /proc files", "err", err) - return - } - - m.ftdc.Add(fmt.Sprintf("proc.modules.%s", m.process.ID()), statser) -} - func getFullEnvironment( cfg config.Module, dataDir string, From 9fac490bc0072460f65feaeba70e8652672f17a9 Mon Sep 17 00:00:00 2001 From: Bashar Eid Date: Fri, 3 Jan 2025 15:18:12 -0500 Subject: [PATCH 04/50] actually restore manager.go file --- module/modmanager/manager.go | 172 +++++++++++++++++++++-------------- 1 file changed, 102 insertions(+), 70 deletions(-) diff --git a/module/modmanager/manager.go b/module/modmanager/manager.go index df42a710b64..ca36af9f851 100644 --- a/module/modmanager/manager.go +++ b/module/modmanager/manager.go @@ -30,6 +30,8 @@ import ( "google.golang.org/protobuf/types/known/structpb" "go.viam.com/rdk/config" + "go.viam.com/rdk/ftdc" + "go.viam.com/rdk/ftdc/sys" rdkgrpc "go.viam.com/rdk/grpc" "go.viam.com/rdk/logging" modlib "go.viam.com/rdk/module" @@ -163,7 +165,14 @@ func (rmap *resourceModuleMap) Range(f func(name resource.Name, mod *module) boo // Manager is the root structure for the module system. type Manager struct { - mu sync.RWMutex + // mu (mostly) coordinates API methods that modify the `modules` map. Specifically, + // `AddResource` can be called concurrently during a reconfigure. But `RemoveResource` or + // resources being restarted after a module crash are given exclusive access. + // + // mu additionally is used for exclusive access when `Add`ing modules (as opposed to resources), + // `Reconfigure`ing modules, `Remove`ing modules and `Close`ing the `Manager`. + mu sync.RWMutex + logger logging.Logger modules moduleMap parentAddr string @@ -204,9 +213,6 @@ func (mgr *Manager) Close(ctx context.Context) error { // Handles returns all the models for each module registered. func (mgr *Manager) Handles() map[string]modlib.HandlerMap { - mgr.mu.Lock() - defer mgr.mu.Unlock() - res := map[string]modlib.HandlerMap{} mgr.modules.Range(func(n string, m *module) bool { @@ -279,11 +285,12 @@ func (mgr *Manager) Add(ctx context.Context, confs ...config.Module) error { wg.Add(1) go func(i int, conf config.Module) { defer wg.Done() + moduleLogger := mgr.logger.Sublogger(conf.Name) - mgr.logger.CInfow(ctx, "Now adding module", "module", conf.Name) - err := mgr.add(ctx, conf) + moduleLogger.CInfow(ctx, "Now adding module", "module", conf.Name) + err := mgr.add(ctx, conf, moduleLogger) if err != nil { - mgr.logger.CErrorw(ctx, "Error adding module", "module", conf.Name, "error", err) + moduleLogger.CErrorw(ctx, "Error adding module", "module", conf.Name, "error", err) errs[i] = err return } @@ -302,9 +309,10 @@ func (mgr *Manager) Add(ctx context.Context, confs ...config.Module) error { return combinedErr } -func (mgr *Manager) add(ctx context.Context, conf config.Module) error { +func (mgr *Manager) add(ctx context.Context, conf config.Module, moduleLogger logging.Logger) error { _, exists := mgr.modules.Load(conf.Name) if exists { + // Keeping this as a manager logger since it is dealing with manager behavior mgr.logger.CWarnw(ctx, "Not adding module that already exists", "module", conf.Name) return nil } @@ -326,7 +334,7 @@ func (mgr *Manager) add(ctx context.Context, conf config.Module) error { resources: map[resource.Name]*addedResource{}, ftdc: mgr.ftdc, port: mgr.nextPort, - logger: mgr.logger.Sublogger(conf.Name), + logger: moduleLogger, } if err := mgr.startModule(ctx, mod); err != nil { @@ -340,7 +348,6 @@ func (mgr *Manager) startModuleProcess(mod *module) error { mgr.restartCtx, mgr.parentAddr, mgr.newOnUnexpectedExitHandler(mod), - mgr.logger, mgr.viamHomeDir, mgr.packagesDir, ) @@ -357,20 +364,20 @@ func (mgr *Manager) startModule(ctx context.Context, mod *module) error { var success bool defer func() { if !success { - mod.cleanupAfterStartupFailure(mgr.logger) + mod.cleanupAfterStartupFailure() } }() // create the module's data directory if mod.dataDir != "" { - mgr.logger.Infof("Creating data directory %q for module %q", mod.dataDir, mod.cfg.Name) + mod.logger.Debugf("Creating data directory %q for module %q", mod.dataDir, mod.cfg.Name) if err := os.MkdirAll(mod.dataDir, 0o750); err != nil { return errors.WithMessage(err, "error while creating data directory for module "+mod.cfg.Name) } } cleanup := rutils.SlowStartupLogger( - ctx, "Waiting for module to complete startup and registration", "module", mod.cfg.Name, mgr.logger) + ctx, "Waiting for module to complete startup and registration", "module", mod.cfg.Name, mod.logger) defer cleanup() if err := mgr.startModuleProcess(mod); err != nil { @@ -381,13 +388,13 @@ func (mgr *Manager) startModule(ctx context.Context, mod *module) error { return errors.WithMessage(err, "error while dialing module "+mod.cfg.Name) } - if err := mod.checkReady(ctx, mgr.parentAddr, mgr.logger); err != nil { + if err := mod.checkReady(ctx, mgr.parentAddr); err != nil { return errors.WithMessage(err, "error while waiting for module to be ready "+mod.cfg.Name) } - mod.registerResources(mgr, mgr.logger) + mod.registerResources(mgr) mgr.modules.Store(mod.cfg.Name, mod) - mgr.logger.Infow("Module successfully added", "module", mod.cfg.Name) + mod.logger.Infow("Module successfully added", "module", mod.cfg.Name) success = true return nil } @@ -410,7 +417,7 @@ func (mgr *Manager) Reconfigure(ctx context.Context, conf config.Module) ([]reso handledResourceNameStrings = append(handledResourceNameStrings, name.String()) } - mgr.logger.CInfow(ctx, "Module configuration changed. Stopping the existing module process", "module", conf.Name) + mod.logger.CInfow(ctx, "Module configuration changed. Stopping the existing module process", "module", conf.Name) if err := mgr.closeModule(mod, true); err != nil { // If removal fails, assume all handled resources are orphaned. @@ -420,17 +427,17 @@ func (mgr *Manager) Reconfigure(ctx context.Context, conf config.Module) ([]reso mod.cfg = conf mod.resources = map[resource.Name]*addedResource{} - mgr.logger.CInfow(ctx, "Existing module process stopped. Starting new module process", "module", conf.Name) + mod.logger.CInfow(ctx, "Existing module process stopped. Starting new module process", "module", conf.Name) if err := mgr.startModule(ctx, mod); err != nil { // If re-addition fails, assume all handled resources are orphaned. return handledResourceNames, err } - mgr.logger.CInfow(ctx, "New module process is running and responding to gRPC requests", "module", + mod.logger.CInfow(ctx, "New module process is running and responding to gRPC requests", "module", mod.cfg.Name, "module address", mod.addr) - mgr.logger.CInfow(ctx, "Resources handled by reconfigured module will be re-added to new module process", + mod.logger.CInfow(ctx, "Resources handled by reconfigured module will be re-added to new module process", "module", mod.cfg.Name, "resources", handledResourceNameStrings) return handledResourceNames, nil } @@ -473,16 +480,16 @@ func (mgr *Manager) Remove(modName string) ([]resource.Name, error) { func (mgr *Manager) closeModule(mod *module, reconfigure bool) error { // resource manager should've removed these cleanly if this isn't a reconfigure if !reconfigure && len(mod.resources) != 0 { - mgr.logger.Warnw("Forcing removal of module with active resources", "module", mod.cfg.Name) + mod.logger.Warnw("Forcing removal of module with active resources", "module", mod.cfg.Name) } // need to actually close the resources within the module itself before stopping for res := range mod.resources { _, err := mod.client.RemoveResource(context.Background(), &pb.RemoveResourceRequest{Name: res.String()}) if err != nil { - mgr.logger.Errorw("Error removing resource", "module", mod.cfg.Name, "resource", res.Name, "error", err) + mod.logger.Errorw("Error removing resource", "module", mod.cfg.Name, "resource", res.Name, "error", err) } else { - mgr.logger.Infow("Successfully removed resource from module", "module", mod.cfg.Name, "resource", res.Name) + mod.logger.Infow("Successfully removed resource from module", "module", mod.cfg.Name, "resource", res.Name) } } @@ -491,7 +498,7 @@ func (mgr *Manager) closeModule(mod *module, reconfigure bool) error { } if err := mod.sharedConn.Close(); err != nil { - mgr.logger.Warnw("Error closing connection to module", "error", err) + mod.logger.Warnw("Error closing connection to module", "error", err) } mod.deregisterResources() @@ -504,7 +511,7 @@ func (mgr *Manager) closeModule(mod *module, reconfigure bool) error { }) mgr.modules.Delete(mod.cfg.Name) - mgr.logger.Infow("Module successfully closed", "module", mod.cfg.Name) + mod.logger.Infow("Module successfully closed", "module", mod.cfg.Name) return nil } @@ -527,7 +534,7 @@ func (mgr *Manager) addResource(ctx context.Context, conf resource.Config, deps return nil, errors.Errorf("no active module registered to serve resource api %s and model %s", conf.API, conf.Model) } - mgr.logger.CInfow(ctx, "Adding resource to module", "resource", conf.Name, "module", mod.cfg.Name) + mod.logger.CInfow(ctx, "Adding resource to module", "resource", conf.Name, "module", mod.cfg.Name) confProto, err := config.ComponentConfigToProto(&conf) if err != nil { @@ -546,7 +553,7 @@ func (mgr *Manager) addResource(ctx context.Context, conf resource.Config, deps apiInfo, ok := resource.LookupGenericAPIRegistration(conf.API) if !ok || apiInfo.RPCClient == nil { - mgr.logger.CWarnw(ctx, "No built-in grpc client for modular resource", "resource", conf.ResourceName()) + mod.logger.CWarnw(ctx, "No built-in grpc client for modular resource", "resource", conf.ResourceName()) return rdkgrpc.NewForeignResource(conf.ResourceName(), &mod.sharedConn), nil } @@ -555,14 +562,12 @@ func (mgr *Manager) addResource(ctx context.Context, conf resource.Config, deps // ReconfigureResource updates/reconfigures a modular component with a new configuration. func (mgr *Manager) ReconfigureResource(ctx context.Context, conf resource.Config, deps []string) error { - mgr.mu.RLock() - defer mgr.mu.RUnlock() mod, ok := mgr.getModule(conf) if !ok { return errors.Errorf("no module registered to serve resource api %s and model %s", conf.API, conf.Model) } - mgr.logger.CInfow(ctx, "Reconfiguring resource for module", "resource", conf.Name, "module", mod.cfg.Name) + mod.logger.CInfow(ctx, "Reconfiguring resource for module", "resource", conf.Name, "module", mod.cfg.Name) confProto, err := config.ComponentConfigToProto(&conf) if err != nil { @@ -572,6 +577,7 @@ func (mgr *Manager) ReconfigureResource(ctx context.Context, conf resource.Confi if err != nil { return err } + mod.resourcesMu.Lock() defer mod.resourcesMu.Unlock() mod.resources[conf.ResourceName()] = &addedResource{conf, deps} @@ -582,8 +588,6 @@ func (mgr *Manager) ReconfigureResource(ctx context.Context, conf resource.Confi // Configs returns a slice of config.Module representing the currently managed // modules. func (mgr *Manager) Configs() []config.Module { - mgr.mu.RLock() - defer mgr.mu.RUnlock() var configs []config.Module mgr.modules.Range(func(_ string, mod *module) bool { configs = append(configs, mod.cfg) @@ -594,16 +598,12 @@ func (mgr *Manager) Configs() []config.Module { // Provides returns true if a component/service config WOULD be handled by a module. func (mgr *Manager) Provides(conf resource.Config) bool { - mgr.mu.RLock() - defer mgr.mu.RUnlock() _, ok := mgr.getModule(conf) return ok } // IsModularResource returns true if an existing resource IS handled by a module. func (mgr *Manager) IsModularResource(name resource.Name) bool { - mgr.mu.RLock() - defer mgr.mu.RUnlock() _, ok := mgr.rMap.Load(name) return ok } @@ -617,7 +617,7 @@ func (mgr *Manager) RemoveResource(ctx context.Context, name resource.Name) erro return errors.Errorf("resource %+v not found in module", name) } - mgr.logger.CInfow(ctx, "Removing resource for module", "resource", name.String(), "module", mod.cfg.Name) + mod.logger.CInfow(ctx, "Removing resource for module", "resource", name.String(), "module", mod.cfg.Name) mgr.rMap.Delete(name) delete(mod.resources, name) @@ -636,8 +636,6 @@ func (mgr *Manager) RemoveResource(ctx context.Context, name resource.Name) erro // ValidateConfig determines whether the given config is valid and returns its implicit // dependencies. func (mgr *Manager) ValidateConfig(ctx context.Context, conf resource.Config) ([]string, error) { - mgr.mu.RLock() - defer mgr.mu.RUnlock() mod, ok := mgr.getModule(conf) if !ok { return nil, @@ -671,8 +669,6 @@ func (mgr *Manager) ValidateConfig(ctx context.Context, conf resource.Config) ([ // and modified resources. It also puts modular resources whose module has been modified or added in conf.Added if // they are not already there since the resources themselves are not necessarily new. func (mgr *Manager) ResolveImplicitDependenciesInConfig(ctx context.Context, conf *config.Diff) error { - mgr.mu.RLock() - defer mgr.mu.RUnlock() // NOTE(benji): We could simplify some of the following `continue` // conditional clauses to a single clause, but we split them for readability. for _, c := range conf.Right.Components { @@ -862,7 +858,7 @@ func (mgr *Manager) newOnUnexpectedExitHandler(mod *module) func(exitCode int) b defer mod.inStartup.Store(false) // Log error immediately, as this is unexpected behavior. - mgr.logger.Errorw( + mod.logger.Errorw( "Module has unexpectedly exited.", "module", mod.cfg.Name, "exit_code", exitCode, ) @@ -878,7 +874,7 @@ func (mgr *Manager) newOnUnexpectedExitHandler(mod *module) func(exitCode int) b if orphanedResourceNames := mgr.attemptRestart(mgr.restartCtx, mod); orphanedResourceNames != nil { if mgr.removeOrphanedResources != nil { mgr.removeOrphanedResources(mgr.restartCtx, orphanedResourceNames) - mgr.logger.Debugw( + mod.logger.Debugw( "Removed resources after failed module restart", "module", mod.cfg.Name, "resources", resource.NamesToStrings(orphanedResourceNames), @@ -886,7 +882,7 @@ func (mgr *Manager) newOnUnexpectedExitHandler(mod *module) func(exitCode int) b } return false } - mgr.logger.Infow("Module successfully restarted, re-adding resources", "module", mod.cfg.Name) + mod.logger.Infow("Module successfully restarted, re-adding resources", "module", mod.cfg.Name) // Otherwise, add old module process' resources to new module; warn if new // module cannot handle old resource and remove it from mod.resources. @@ -897,7 +893,7 @@ func (mgr *Manager) newOnUnexpectedExitHandler(mod *module) func(exitCode int) b // read lock, so we execute it here with a write lock to make sure it doesn't // run concurrently. if _, err := mgr.addResourceWithWriteLock(mgr.restartCtx, res.conf, res.deps); err != nil { - mgr.logger.Warnw("Error while re-adding resource to module", + mod.logger.Warnw("Error while re-adding resource to module", "resource", name, "module", mod.cfg.Name, "error", err) mgr.rMap.Delete(name) @@ -912,7 +908,7 @@ func (mgr *Manager) newOnUnexpectedExitHandler(mod *module) func(exitCode int) b mgr.removeOrphanedResources(mgr.restartCtx, orphanedResourceNames) } - mgr.logger.Infow("Module resources successfully re-added after module restart", "module", mod.cfg.Name) + mod.logger.Infow("Module resources successfully re-added after module restart", "module", mod.cfg.Name) return false } } @@ -961,7 +957,7 @@ func (mgr *Manager) attemptRestart(ctx context.Context, mod *module) []resource. // executable we were given for initial module addition. cleanup := rutils.SlowStartupLogger( - ctx, "Waiting for module to complete restart and re-registration", "module", mod.cfg.Name, mgr.logger) + ctx, "Waiting for module to complete restart and re-registration", "module", mod.cfg.Name, mod.logger) defer cleanup() // Attempt to restart module process 3 times. @@ -993,13 +989,13 @@ func (mgr *Manager) attemptRestart(ctx context.Context, mod *module) []resource. return orphanedResourceNames } - if err := mod.checkReady(ctx, mgr.parentAddr, mgr.logger); err != nil { + if err := mod.checkReady(ctx, mgr.parentAddr); err != nil { mgr.logger.CErrorw(ctx, "Error while waiting for restarted module to be ready", "module", mod.cfg.Name, "error", err) return orphanedResourceNames } - mod.registerResources(mgr, mgr.logger) + mod.registerResources(mgr) success = true return nil @@ -1050,11 +1046,11 @@ func (m *module) dial() error { // checkReady sends a `ReadyRequest` and waits for either a `ReadyResponse`, or a context // cancelation. -func (m *module) checkReady(ctx context.Context, parentAddr string, logger logging.Logger) error { - ctxTimeout, cancelFunc := context.WithTimeout(ctx, rutils.GetModuleStartupTimeout(logger)) +func (m *module) checkReady(ctx context.Context, parentAddr string) error { + ctxTimeout, cancelFunc := context.WithTimeout(ctx, rutils.GetModuleStartupTimeout(m.logger)) defer cancelFunc() - logger.CInfow(ctx, "Waiting for module to respond to ready request", "module", m.cfg.Name) + m.logger.CInfow(ctx, "Waiting for module to respond to ready request", "module", m.cfg.Name) req := &pb.ReadyRequest{ParentAddress: parentAddr} @@ -1062,7 +1058,7 @@ func (m *module) checkReady(ctx context.Context, parentAddr string, logger loggi var err error req.WebrtcOffer, err = m.sharedConn.GenerateEncodedOffer() if err != nil { - logger.CWarnw(ctx, "Unable to generate offer for module PeerConnection. Ignoring.", "err", err) + m.logger.CWarnw(ctx, "Unable to generate offer for module PeerConnection. Ignoring.", "err", err) } for { @@ -1084,7 +1080,7 @@ func (m *module) checkReady(ctx context.Context, parentAddr string, logger loggi err = m.sharedConn.ProcessEncodedAnswer(resp.WebrtcAnswer) if err != nil { - logger.CWarnw(ctx, "Unable to create PeerConnection with module. Ignoring.", "err", err) + m.logger.CWarnw(ctx, "Unable to create PeerConnection with module. Ignoring.", "err", err) } // The `ReadyRespones` also includes the Viam `API`s and `Model`s the module provides. This @@ -1118,7 +1114,6 @@ func (m *module) startProcess( ctx context.Context, parentAddr string, oue func(int) bool, - logger logging.Logger, viamHomeDir string, packagesDir string, ) error { @@ -1146,10 +1141,10 @@ func (m *module) startProcess( moduleWorkingDirectory, ok := moduleEnvironment["VIAM_MODULE_ROOT"] if !ok { moduleWorkingDirectory = filepath.Dir(absoluteExePath) - logger.CWarnw(ctx, "VIAM_MODULE_ROOT was not passed to module. Defaulting to module's working directory", + m.logger.CDebugw(ctx, "VIAM_MODULE_ROOT was not passed to module. Defaulting to module's working directory", "module", m.cfg.Name, "dir", moduleWorkingDirectory) } else { - logger.CInfow(ctx, "Starting module in working directory", "module", m.cfg.Name, "dir", moduleWorkingDirectory) + m.logger.CInfow(ctx, "Starting module in working directory", "module", m.cfg.Name, "dir", moduleWorkingDirectory) } pconf := pexec.ProcessConfig{ @@ -1165,21 +1160,25 @@ func (m *module) startProcess( // supplied and module manager has a DebugLevel logger. if m.cfg.LogLevel != "" { pconf.Args = append(pconf.Args, fmt.Sprintf(logLevelArgumentTemplate, m.cfg.LogLevel)) - } else if logger.Level().Enabled(zapcore.DebugLevel) { + } else if m.logger.Level().Enabled(zapcore.DebugLevel) { pconf.Args = append(pconf.Args, fmt.Sprintf(logLevelArgumentTemplate, "debug")) } - m.process = pexec.NewManagedProcess(pconf, logger) + m.process = pexec.NewManagedProcess(pconf, m.logger) if err := m.process.Start(context.Background()); err != nil { return errors.WithMessage(err, "module startup failed") } + // Turn on process cpu/memory diagnostics for the module process. If there's an error, we + // continue normally, just without FTDC. + m.registerProcessWithFTDC() + checkTicker := time.NewTicker(100 * time.Millisecond) defer checkTicker.Stop() - logger.CInfow(ctx, "Starting up module", "module", m.cfg.Name) - ctxTimeout, cancel := context.WithTimeout(ctx, rutils.GetModuleStartupTimeout(logger)) + m.logger.CInfow(ctx, "Starting up module", "module", m.cfg.Name) + ctxTimeout, cancel := context.WithTimeout(ctx, rutils.GetModuleStartupTimeout(m.logger)) defer cancel() for { select { @@ -1215,9 +1214,21 @@ func (m *module) stopProcess() error { if m.process == nil { return nil } + + m.logger.Infof("Stopping module: %s process", m.cfg.Name) + // Attempt to remove module's .sock file if module did not remove it // already. - defer rutils.RemoveFileNoError(m.addr) + defer func() { + rutils.RemoveFileNoError(m.addr) + + // The system metrics "statser" is resilient to the process dying under the hood. An empty set + // of metrics will be reported. Therefore it is safe to continue monitoring the module process + // while it's in shutdown. + if m.ftdc != nil { + m.ftdc.Remove(m.process.ID()) + } + }() // TODO(RSDK-2551): stop ignoring exit status 143 once Python modules handle // SIGTERM correctly. @@ -1228,10 +1239,11 @@ func (m *module) stopProcess() error { } return err } + return nil } -func (m *module) registerResources(mgr modmaninterface.ModuleManager, logger logging.Logger) { +func (m *module) registerResources(mgr modmaninterface.ModuleManager) { for api, models := range m.handles { if _, ok := resource.LookupGenericAPIRegistration(api.API); !ok { resource.RegisterAPI( @@ -1243,7 +1255,7 @@ func (m *module) registerResources(mgr modmaninterface.ModuleManager, logger log switch { case api.API.IsComponent(): for _, model := range models { - logger.Infow("Registering component API and model from module", "module", m.cfg.Name, "API", api.API, "model", model) + m.logger.Infow("Registering component API and model from module", "module", m.cfg.Name, "API", api.API, "model", model) // We must copy because the Discover closure func relies on api and model, but they are iterators and mutate. // Copying prevents mutation. modelCopy := model @@ -1258,13 +1270,13 @@ func (m *module) registerResources(mgr modmaninterface.ModuleManager, logger log return mgr.AddResource(ctx, conf, DepsToNames(deps)) }, Discover: func(ctx context.Context, logger logging.Logger, extra map[string]interface{}) (interface{}, error) { - extraStruct, err := structpb.NewStruct(extra) + extraStructPb, err := structpb.NewStruct(extra) if err != nil { return nil, err } req := &robotpb.DiscoverComponentsRequest{ Queries: []*robotpb.DiscoveryQuery{ - {Subtype: apiCopy.API.String(), Model: modelCopy.String(), Extra: extraStruct}, + {Subtype: apiCopy.API.String(), Model: modelCopy.String(), Extra: extraStructPb}, }, } @@ -1286,7 +1298,7 @@ func (m *module) registerResources(mgr modmaninterface.ModuleManager, logger log } case api.API.IsService(): for _, model := range models { - logger.Infow("Registering service API and model from module", "module", m.cfg.Name, "API", api.API, "model", model) + m.logger.Infow("Registering service API and model from module", "module", m.cfg.Name, "API", api.API, "model", model) resource.RegisterService(api.API, model, resource.Registration[resource.Resource, resource.NoNativeConfig]{ Constructor: func( ctx context.Context, @@ -1299,7 +1311,7 @@ func (m *module) registerResources(mgr modmaninterface.ModuleManager, logger log }) } default: - logger.Errorw("Invalid module type", "API type", api.API.Type) + m.logger.Errorw("Invalid module type", "API type", api.API.Type) } } } @@ -1313,10 +1325,10 @@ func (m *module) deregisterResources() { m.handles = nil } -func (m *module) cleanupAfterStartupFailure(logger logging.Logger) { +func (m *module) cleanupAfterStartupFailure() { if err := m.stopProcess(); err != nil { msg := "Error while stopping process of module that failed to start" - logger.Errorw(msg, "module", m.cfg.Name, "error", err) + m.logger.Errorw(msg, "module", m.cfg.Name, "error", err) } utils.UncheckedError(m.sharedConn.Close()) } @@ -1336,6 +1348,26 @@ func (m *module) getFullEnvironment(viamHomeDir string) map[string]string { return getFullEnvironment(m.cfg, m.dataDir, viamHomeDir) } +func (m *module) registerProcessWithFTDC() { + if m.ftdc == nil { + return + } + + pid, err := m.process.UnixPid() + if err != nil { + m.logger.Warnw("Module process has no pid. Cannot start ftdc.", "err", err) + return + } + + statser, err := sys.NewPidSysUsageStatser(pid) + if err != nil { + m.logger.Warnw("Cannot find /proc files", "err", err) + return + } + + m.ftdc.Add(fmt.Sprintf("modules.%s", m.process.ID()), statser) +} + func getFullEnvironment( cfg config.Module, dataDir string, From 19287f32ed2ac9596077457b23e7df6b1fe5aa55 Mon Sep 17 00:00:00 2001 From: Bashar Eid Date: Fri, 3 Jan 2025 16:11:22 -0500 Subject: [PATCH 05/50] create helper function --- config/module.go | 42 ++++++++++++++++++++++-------------------- 1 file changed, 22 insertions(+), 20 deletions(-) diff --git a/config/module.go b/config/module.go index 0f766102bb2..646dc0f6dcc 100644 --- a/config/module.go +++ b/config/module.go @@ -386,17 +386,11 @@ func (m Module) getJSONManifest(unpackedModDir string, env map[string]string) (* localNonTarball := m.Type == ModuleTypeLocal && !m.NeedsSyntheticPackage() if !localNonTarball { // this is case 1, meta.json in exe folder. - metaPath, err := utils.SafeJoinDir(unpackedModDir, "meta.json") + meta, err := findMetaJSONFile(unpackedModDir) if err != nil { return nil, err } - _, err = os.Stat(metaPath) - if err == nil { - // this is case 1, meta.json in exe dir - meta, err := parseJSONFile[JSONManifest](metaPath) - if err != nil { - return nil, err - } + if meta != nil { return meta, nil } } @@ -417,20 +411,31 @@ func (m Module) getJSONManifest(unpackedModDir string, env map[string]string) (* moduleWorkingDirectory, ok := env["VIAM_MODULE_ROOT"] if ok { - metaPath, err := utils.SafeJoinDir(moduleWorkingDirectory, "meta.json") + meta, err := findMetaJSONFile(moduleWorkingDirectory) if err != nil { return nil, err } - _, err = os.Stat(metaPath) - if err == nil { - meta, err := parseJSONFile[JSONManifest](metaPath) - if err != nil { - return nil, err - } + if meta != nil { return meta, nil } } - metaPath, err := utils.SafeJoinDir(unpackedModDir, "meta.json") + + meta, err := findMetaJSONFile(unpackedModDir) + if err != nil { + return nil, err + } + if meta != nil { + return meta, nil + } + + if !ok { + return nil, errors.Errorf("VIAM_MODULE_ROOT not set. Failed to find meta.json in executable directory %s", unpackedModDir) + } + return nil, errors.Errorf("failed to find meta.json. Searched in executable directory %s and path set by VIAM_MODULE_ROOT %s", moduleWorkingDirectory, unpackedModDir) +} + +func findMetaJSONFile(dir string) (*JSONManifest, error) { + metaPath, err := utils.SafeJoinDir(dir, "meta.json") if err != nil { return nil, err } @@ -442,8 +447,5 @@ func (m Module) getJSONManifest(unpackedModDir string, env map[string]string) (* } return meta, nil } - if !ok { - return nil, errors.Errorf("VIAM_MODULE_ROOT not set. Failed to find meta.json in executable directory %s", metaPath) - } - return nil, errors.Errorf("failed to find meta.json. Searched in path set by VIAM_MODULE_ROOT %s and executable directory %s", moduleWorkingDirectory, metaPath) + return nil, nil } From 4b46142034eb45e10a5281ab997a89a4ef751ca1 Mon Sep 17 00:00:00 2001 From: Bashar Eid Date: Fri, 3 Jan 2025 16:43:28 -0500 Subject: [PATCH 06/50] use environment variable if registry module --- config/module.go | 60 ++++++++++++++++++++++++++++-------------------- 1 file changed, 35 insertions(+), 25 deletions(-) diff --git a/config/module.go b/config/module.go index 646dc0f6dcc..ca952838810 100644 --- a/config/module.go +++ b/config/module.go @@ -267,7 +267,7 @@ func (m *Module) FirstRun( // Load the module's meta.json. If it doesn't exist DEBUG log and exit quietly. // For all other errors WARN log and exit. - meta, err := m.getJSONManifest(unpackedModDir, env) + meta, err, moduleWorkingDirectory := m.getJSONManifest(unpackedModDir, env) var pathErr *os.PathError switch { case errors.As(err, &pathErr): @@ -282,7 +282,13 @@ func (m *Module) FirstRun( logger.Debug("no first run script specified, skipping first run") return nil } - relFirstRunPath, err := utils.SafeJoinDir(unpackedModDir, meta.FirstRun) + var firstRunTopLevelDir string + if moduleWorkingDirectory == "" { + firstRunTopLevelDir = unpackedModDir + } else { + firstRunTopLevelDir = moduleWorkingDirectory + } + relFirstRunPath, err := utils.SafeJoinDir(firstRunTopLevelDir, meta.FirstRun) if err != nil { logger.Errorw("failed to build path to first run script, skipping first run", "error", err) return nil @@ -381,17 +387,17 @@ func (m *Module) FirstRun( // 1. if there is a meta.json in the exe dir, use that, except in local non-tarball case. // 2. if this is a local tarball and there's a meta.json next to the tarball, use that. // Note: the working directory must be the unpacked tarball directory or local exec directory. -func (m Module) getJSONManifest(unpackedModDir string, env map[string]string) (*JSONManifest, error) { +func (m Module) getJSONManifest(unpackedModDir string, env map[string]string) (*JSONManifest, error, string) { // note: we don't look at internal meta.json in local non-tarball case because user has explicitly requested a binary. localNonTarball := m.Type == ModuleTypeLocal && !m.NeedsSyntheticPackage() if !localNonTarball { // this is case 1, meta.json in exe folder. meta, err := findMetaJSONFile(unpackedModDir) if err != nil { - return nil, err + return nil, err, "" } if meta != nil { - return meta, nil + return meta, nil, "" } } if m.NeedsSyntheticPackage() { @@ -399,43 +405,47 @@ func (m Module) getJSONManifest(unpackedModDir string, env map[string]string) (* // TODO(RSDK-7848): remove this case once java sdk supports internal meta.json. metaPath, err := utils.SafeJoinDir(filepath.Dir(m.ExePath), "meta.json") if err != nil { - return nil, err + return nil, err, "" } meta, err := parseJSONFile[JSONManifest](metaPath) if err != nil { // note: this error deprecates the side-by-side case because the side-by-side case is deprecated. - return nil, errors.Wrapf(err, "couldn't find meta.json inside tarball %s (or next to it)", m.ExePath) + return nil, errors.Wrapf(err, "couldn't find meta.json inside tarball %s (or next to it)", m.ExePath), "" } - return meta, err + return meta, err, "" } - moduleWorkingDirectory, ok := env["VIAM_MODULE_ROOT"] - if ok { - meta, err := findMetaJSONFile(moduleWorkingDirectory) + if m.Type == ModuleTypeRegistry { + moduleWorkingDirectory, ok := env["VIAM_MODULE_ROOT"] + if ok { + meta, err := findMetaJSONFile(moduleWorkingDirectory) + if err != nil { + return nil, err, "" + } + if meta != nil { + return meta, nil, moduleWorkingDirectory + } + } + + meta, err := findMetaJSONFile(unpackedModDir) if err != nil { - return nil, err + return nil, err, "" } if meta != nil { - return meta, nil + return meta, nil, "" } - } - meta, err := findMetaJSONFile(unpackedModDir) - if err != nil { - return nil, err - } - if meta != nil { - return meta, nil - } - - if !ok { - return nil, errors.Errorf("VIAM_MODULE_ROOT not set. Failed to find meta.json in executable directory %s", unpackedModDir) + if !ok { + return nil, errors.Errorf("VIAM_MODULE_ROOT not set. Failed to find meta.json in executable directory %s", unpackedModDir), "" + } + return nil, errors.Errorf("failed to find meta.json. Searched in executable directory %s and path set by VIAM_MODULE_ROOT %s", moduleWorkingDirectory, unpackedModDir), "" } - return nil, errors.Errorf("failed to find meta.json. Searched in executable directory %s and path set by VIAM_MODULE_ROOT %s", moduleWorkingDirectory, unpackedModDir) + return nil, errors.New("failed to find meta.json"), "" } func findMetaJSONFile(dir string) (*JSONManifest, error) { metaPath, err := utils.SafeJoinDir(dir, "meta.json") + fmt.Println("META PATH: ", metaPath) if err != nil { return nil, err } From 91ece93c509e5d2feea9a3532df3225c1fc40a50 Mon Sep 17 00:00:00 2001 From: Bashar Eid Date: Fri, 3 Jan 2025 16:48:10 -0500 Subject: [PATCH 07/50] remove print statement --- config/module.go | 1 - 1 file changed, 1 deletion(-) diff --git a/config/module.go b/config/module.go index ca952838810..5ee9b5c5436 100644 --- a/config/module.go +++ b/config/module.go @@ -445,7 +445,6 @@ func (m Module) getJSONManifest(unpackedModDir string, env map[string]string) (* func findMetaJSONFile(dir string) (*JSONManifest, error) { metaPath, err := utils.SafeJoinDir(dir, "meta.json") - fmt.Println("META PATH: ", metaPath) if err != nil { return nil, err } From df1742d3ed995859043c077f2fd3520d32d605ce Mon Sep 17 00:00:00 2001 From: Bashar Eid Date: Fri, 3 Jan 2025 17:12:26 -0500 Subject: [PATCH 08/50] fix lint errors --- config/module.go | 29 +++++++++++++++-------------- 1 file changed, 15 insertions(+), 14 deletions(-) diff --git a/config/module.go b/config/module.go index 5ee9b5c5436..17752ed5c31 100644 --- a/config/module.go +++ b/config/module.go @@ -267,7 +267,7 @@ func (m *Module) FirstRun( // Load the module's meta.json. If it doesn't exist DEBUG log and exit quietly. // For all other errors WARN log and exit. - meta, err, moduleWorkingDirectory := m.getJSONManifest(unpackedModDir, env) + meta, moduleWorkingDirectory, err := m.getJSONManifest(unpackedModDir, env) var pathErr *os.PathError switch { case errors.As(err, &pathErr): @@ -387,17 +387,17 @@ func (m *Module) FirstRun( // 1. if there is a meta.json in the exe dir, use that, except in local non-tarball case. // 2. if this is a local tarball and there's a meta.json next to the tarball, use that. // Note: the working directory must be the unpacked tarball directory or local exec directory. -func (m Module) getJSONManifest(unpackedModDir string, env map[string]string) (*JSONManifest, error, string) { +func (m Module) getJSONManifest(unpackedModDir string, env map[string]string) (*JSONManifest, string, error) { // note: we don't look at internal meta.json in local non-tarball case because user has explicitly requested a binary. localNonTarball := m.Type == ModuleTypeLocal && !m.NeedsSyntheticPackage() if !localNonTarball { // this is case 1, meta.json in exe folder. meta, err := findMetaJSONFile(unpackedModDir) if err != nil { - return nil, err, "" + return nil, "", err } if meta != nil { - return meta, nil, "" + return meta, "", nil } } if m.NeedsSyntheticPackage() { @@ -405,14 +405,14 @@ func (m Module) getJSONManifest(unpackedModDir string, env map[string]string) (* // TODO(RSDK-7848): remove this case once java sdk supports internal meta.json. metaPath, err := utils.SafeJoinDir(filepath.Dir(m.ExePath), "meta.json") if err != nil { - return nil, err, "" + return nil, "", err } meta, err := parseJSONFile[JSONManifest](metaPath) if err != nil { // note: this error deprecates the side-by-side case because the side-by-side case is deprecated. - return nil, errors.Wrapf(err, "couldn't find meta.json inside tarball %s (or next to it)", m.ExePath), "" + return nil, "", errors.Wrapf(err, "couldn't find meta.json inside tarball %s (or next to it)", m.ExePath) } - return meta, err, "" + return meta, "", err } if m.Type == ModuleTypeRegistry { @@ -420,27 +420,28 @@ func (m Module) getJSONManifest(unpackedModDir string, env map[string]string) (* if ok { meta, err := findMetaJSONFile(moduleWorkingDirectory) if err != nil { - return nil, err, "" + return nil, "", err } if meta != nil { - return meta, nil, moduleWorkingDirectory + return meta, moduleWorkingDirectory, nil } } meta, err := findMetaJSONFile(unpackedModDir) if err != nil { - return nil, err, "" + return nil, "", err } if meta != nil { - return meta, nil, "" + return meta, "", nil } if !ok { - return nil, errors.Errorf("VIAM_MODULE_ROOT not set. Failed to find meta.json in executable directory %s", unpackedModDir), "" + return nil, "", errors.Errorf("VIAM_MODULE_ROOT not set. Failed to find meta.json in executable directory %s", unpackedModDir) } - return nil, errors.Errorf("failed to find meta.json. Searched in executable directory %s and path set by VIAM_MODULE_ROOT %s", moduleWorkingDirectory, unpackedModDir), "" + return nil, "", errors.Errorf("failed to find meta.json. Searched in executable directory %s and path set by VIAM_MODULE_ROOT %s", + moduleWorkingDirectory, unpackedModDir) } - return nil, errors.New("failed to find meta.json"), "" + return nil, "", errors.New("failed to find meta.json") } func findMetaJSONFile(dir string) (*JSONManifest, error) { From 49efade3ac98868bbd4d848188bffc8d268a556a Mon Sep 17 00:00:00 2001 From: Bashar Eid Date: Mon, 6 Jan 2025 10:52:55 -0500 Subject: [PATCH 09/50] restore manager.go file --- module/modmanager/manager.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/module/modmanager/manager.go b/module/modmanager/manager.go index ca36af9f851..ad05669fdef 100644 --- a/module/modmanager/manager.go +++ b/module/modmanager/manager.go @@ -332,9 +332,9 @@ func (mgr *Manager) add(ctx context.Context, conf config.Module, moduleLogger lo cfg: conf, dataDir: moduleDataDir, resources: map[resource.Name]*addedResource{}, + logger: moduleLogger, ftdc: mgr.ftdc, port: mgr.nextPort, - logger: moduleLogger, } if err := mgr.startModule(ctx, mod); err != nil { From 4a86d330a267207be038fafba4352358fca1de4b Mon Sep 17 00:00:00 2001 From: Bashar Eid Date: Tue, 7 Jan 2025 20:05:48 -0500 Subject: [PATCH 10/50] restore manager.go file --- module/modmanager/manager.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/module/modmanager/manager.go b/module/modmanager/manager.go index ad05669fdef..194532c4fad 100644 --- a/module/modmanager/manager.go +++ b/module/modmanager/manager.go @@ -334,7 +334,7 @@ func (mgr *Manager) add(ctx context.Context, conf config.Module, moduleLogger lo resources: map[resource.Name]*addedResource{}, logger: moduleLogger, ftdc: mgr.ftdc, - port: mgr.nextPort, + port: int(mgr.nextPort.Add(1)), } if err := mgr.startModule(ctx, mod); err != nil { @@ -1365,7 +1365,7 @@ func (m *module) registerProcessWithFTDC() { return } - m.ftdc.Add(fmt.Sprintf("modules.%s", m.process.ID()), statser) + m.ftdc.Add(fmt.Sprintf("proc.modules.%s", m.process.ID()), statser) } func getFullEnvironment( From 08e075728f12b04bbf729c64fad53a8e5b1506ad Mon Sep 17 00:00:00 2001 From: Bashar Eid Date: Wed, 8 Jan 2025 13:51:45 -0500 Subject: [PATCH 11/50] always return top level directory --- config/module.go | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-) diff --git a/config/module.go b/config/module.go index 17752ed5c31..743cea706b2 100644 --- a/config/module.go +++ b/config/module.go @@ -282,13 +282,7 @@ func (m *Module) FirstRun( logger.Debug("no first run script specified, skipping first run") return nil } - var firstRunTopLevelDir string - if moduleWorkingDirectory == "" { - firstRunTopLevelDir = unpackedModDir - } else { - firstRunTopLevelDir = moduleWorkingDirectory - } - relFirstRunPath, err := utils.SafeJoinDir(firstRunTopLevelDir, meta.FirstRun) + relFirstRunPath, err := utils.SafeJoinDir(moduleWorkingDirectory, meta.FirstRun) if err != nil { logger.Errorw("failed to build path to first run script, skipping first run", "error", err) return nil @@ -397,7 +391,7 @@ func (m Module) getJSONManifest(unpackedModDir string, env map[string]string) (* return nil, "", err } if meta != nil { - return meta, "", nil + return meta, unpackedModDir, nil } } if m.NeedsSyntheticPackage() { @@ -405,14 +399,14 @@ func (m Module) getJSONManifest(unpackedModDir string, env map[string]string) (* // TODO(RSDK-7848): remove this case once java sdk supports internal meta.json. metaPath, err := utils.SafeJoinDir(filepath.Dir(m.ExePath), "meta.json") if err != nil { - return nil, "", err + return nil, unpackedModDir, err } meta, err := parseJSONFile[JSONManifest](metaPath) if err != nil { // note: this error deprecates the side-by-side case because the side-by-side case is deprecated. - return nil, "", errors.Wrapf(err, "couldn't find meta.json inside tarball %s (or next to it)", m.ExePath) + return nil, unpackedModDir, errors.Wrapf(err, "couldn't find meta.json inside tarball %s (or next to it)", m.ExePath) } - return meta, "", err + return meta, unpackedModDir, err } if m.Type == ModuleTypeRegistry { @@ -432,7 +426,7 @@ func (m Module) getJSONManifest(unpackedModDir string, env map[string]string) (* return nil, "", err } if meta != nil { - return meta, "", nil + return meta, unpackedModDir, nil } if !ok { From a320e8a3962f1f854b0cfbab0c0c470b14277979 Mon Sep 17 00:00:00 2001 From: Bashar Eid Date: Wed, 8 Jan 2025 13:53:58 -0500 Subject: [PATCH 12/50] always use helper function to find meta.json file --- config/module.go | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/config/module.go b/config/module.go index 743cea706b2..6f3620f91ab 100644 --- a/config/module.go +++ b/config/module.go @@ -397,16 +397,13 @@ func (m Module) getJSONManifest(unpackedModDir string, env map[string]string) (* if m.NeedsSyntheticPackage() { // this is case 2, side-by-side // TODO(RSDK-7848): remove this case once java sdk supports internal meta.json. - metaPath, err := utils.SafeJoinDir(filepath.Dir(m.ExePath), "meta.json") + meta, err := findMetaJSONFile(filepath.Dir(m.ExePath)) if err != nil { - return nil, unpackedModDir, err + return nil, "", err } - meta, err := parseJSONFile[JSONManifest](metaPath) - if err != nil { - // note: this error deprecates the side-by-side case because the side-by-side case is deprecated. - return nil, unpackedModDir, errors.Wrapf(err, "couldn't find meta.json inside tarball %s (or next to it)", m.ExePath) + if meta != nil { + return meta, unpackedModDir, nil } - return meta, unpackedModDir, err } if m.Type == ModuleTypeRegistry { From 56e4222f44ab6095853b25cf55223cdcce384f76 Mon Sep 17 00:00:00 2001 From: Bashar Eid Date: Wed, 8 Jan 2025 14:07:53 -0500 Subject: [PATCH 13/50] reword error returned when no meta.json file found --- config/module.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/config/module.go b/config/module.go index 6f3620f91ab..e20a1596a87 100644 --- a/config/module.go +++ b/config/module.go @@ -377,6 +377,8 @@ func (m *Module) FirstRun( return nil } +// TODO(bashar): update comment with current state of things +// TODO(bashar): write test(s) // getJSONManifest returns a loaded meta.json from one of two sources (in order of precedence): // 1. if there is a meta.json in the exe dir, use that, except in local non-tarball case. // 2. if this is a local tarball and there's a meta.json next to the tarball, use that. @@ -427,9 +429,9 @@ func (m Module) getJSONManifest(unpackedModDir string, env map[string]string) (* } if !ok { - return nil, "", errors.Errorf("VIAM_MODULE_ROOT not set. Failed to find meta.json in executable directory %s", unpackedModDir) + return nil, "", errors.Errorf("VIAM_MODULE_ROOT not set. Searched instead in executable directory %s but failed to find meta.json", unpackedModDir) } - return nil, "", errors.Errorf("failed to find meta.json. Searched in executable directory %s and path set by VIAM_MODULE_ROOT %s", + return nil, "", errors.Errorf("failed to find meta.json. Searched in executable directory %s and path set by VIAM_MODULE_ROOT %s", moduleWorkingDirectory, unpackedModDir) } return nil, "", errors.New("failed to find meta.json") From a757e69d4b8b6ee02d9ea30ab131417069bccabe Mon Sep 17 00:00:00 2001 From: Bashar Eid Date: Wed, 8 Jan 2025 14:09:11 -0500 Subject: [PATCH 14/50] check for non-nil error first --- config/module.go | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/config/module.go b/config/module.go index e20a1596a87..3b1d8cd5e6d 100644 --- a/config/module.go +++ b/config/module.go @@ -442,13 +442,16 @@ func findMetaJSONFile(dir string) (*JSONManifest, error) { if err != nil { return nil, err } + _, err = os.Stat(metaPath) - if err == nil { - meta, err := parseJSONFile[JSONManifest](metaPath) - if err != nil { - return nil, err - } - return meta, nil + if err != nil { + return nil, nil } - return nil, nil + + meta, err := parseJSONFile[JSONManifest](metaPath) + if err != nil { + return nil, err + } + + return meta, nil } From 947fd2f2f79497ddef64aadf2b0c36c9b782e456 Mon Sep 17 00:00:00 2001 From: Bashar Eid Date: Wed, 8 Jan 2025 15:26:42 -0500 Subject: [PATCH 15/50] rework order of conditions --- config/module.go | 71 ++++++++++++++++++++++++++++++------------------ 1 file changed, 45 insertions(+), 26 deletions(-) diff --git a/config/module.go b/config/module.go index 3b1d8cd5e6d..93128b1789c 100644 --- a/config/module.go +++ b/config/module.go @@ -384,57 +384,76 @@ func (m *Module) FirstRun( // 2. if this is a local tarball and there's a meta.json next to the tarball, use that. // Note: the working directory must be the unpacked tarball directory or local exec directory. func (m Module) getJSONManifest(unpackedModDir string, env map[string]string) (*JSONManifest, string, error) { - // note: we don't look at internal meta.json in local non-tarball case because user has explicitly requested a binary. - localNonTarball := m.Type == ModuleTypeLocal && !m.NeedsSyntheticPackage() - if !localNonTarball { - // this is case 1, meta.json in exe folder. - meta, err := findMetaJSONFile(unpackedModDir) - if err != nil { - return nil, "", err - } - if meta != nil { - return meta, unpackedModDir, nil - } - } - if m.NeedsSyntheticPackage() { - // this is case 2, side-by-side - // TODO(RSDK-7848): remove this case once java sdk supports internal meta.json. - meta, err := findMetaJSONFile(filepath.Dir(m.ExePath)) - if err != nil { - return nil, "", err - } - if meta != nil { - return meta, unpackedModDir, nil - } - } + // note: all online modules qualify for cases 1 & 2; local tarballs for cases 2 & 3; and local non-tarballs for none. We don't look at + // internal meta.json in local non-tarball case because user has explicitly requested a binary. + + // note: each case is exited iff no errors occur but the meta.json file is not found + + var ok bool + var moduleWorkingDirectory string + + online := m.Type == ModuleTypeRegistry - if m.Type == ModuleTypeRegistry { - moduleWorkingDirectory, ok := env["VIAM_MODULE_ROOT"] + // case 1: online + if online { + moduleWorkingDirectory, ok = env["VIAM_MODULE_ROOT"] if ok { meta, err := findMetaJSONFile(moduleWorkingDirectory) if err != nil { return nil, "", err } + if meta != nil { return meta, moduleWorkingDirectory, nil } } + } + + localNonTarball := m.Type == ModuleTypeLocal && !m.NeedsSyntheticPackage() + // case 2: online OR non-tarball + if !localNonTarball { meta, err := findMetaJSONFile(unpackedModDir) if err != nil { return nil, "", err } + + if meta != nil { + return meta, unpackedModDir, nil + } + } + + var exeDir string + + // TODO(RSDK-7848): remove this case once java sdk supports internal meta.json. + // case 3: local AND tarball + if m.NeedsSyntheticPackage() { + exeDir = filepath.Dir(m.ExePath) + + meta, err := findMetaJSONFile(exeDir) + if err != nil { + return nil, "", err + } + if meta != nil { return meta, unpackedModDir, nil } + } + if online { if !ok { return nil, "", errors.Errorf("VIAM_MODULE_ROOT not set. Searched instead in executable directory %s but failed to find meta.json", unpackedModDir) } + return nil, "", errors.Errorf("failed to find meta.json. Searched in executable directory %s and path set by VIAM_MODULE_ROOT %s", moduleWorkingDirectory, unpackedModDir) } - return nil, "", errors.New("failed to find meta.json") + + if !localNonTarball { + return nil, "", errors.Errorf("failed to find meta.json. Searched in %s and %s", unpackedModDir, exeDir) + } + + return nil, "", errors.Errorf("failed to find meta.json. Searched only in %s", exeDir) } func findMetaJSONFile(dir string) (*JSONManifest, error) { From a2b6fbe271b17ef4502eb447cb82b4c2e009ae3e Mon Sep 17 00:00:00 2001 From: Bashar Eid Date: Wed, 8 Jan 2025 15:33:09 -0500 Subject: [PATCH 16/50] update comment --- config/module.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/config/module.go b/config/module.go index 93128b1789c..c32dd34b349 100644 --- a/config/module.go +++ b/config/module.go @@ -377,11 +377,11 @@ func (m *Module) FirstRun( return nil } -// TODO(bashar): update comment with current state of things // TODO(bashar): write test(s) -// getJSONManifest returns a loaded meta.json from one of two sources (in order of precedence): -// 1. if there is a meta.json in the exe dir, use that, except in local non-tarball case. -// 2. if this is a local tarball and there's a meta.json next to the tarball, use that. +// getJSONManifest returns a loaded meta.json from one of three sources (in order of precedence): +// 1. if this is an online module and there is a meta.json in its top level directory, use that. +// 3. if there is a meta.json in the exe dir, use that, exept in local non-tarball case. +// 4. if this is a local tarball and there's a meta.json next to the tarball, use that. // Note: the working directory must be the unpacked tarball directory or local exec directory. func (m Module) getJSONManifest(unpackedModDir string, env map[string]string) (*JSONManifest, string, error) { // note: all online modules qualify for cases 1 & 2; local tarballs for cases 2 & 3; and local non-tarballs for none. We don't look at From 79db09dcd913739afc01241e2fc544915712dd98 Mon Sep 17 00:00:00 2001 From: Bashar Eid Date: Wed, 8 Jan 2025 15:35:16 -0500 Subject: [PATCH 17/50] update TODO comment --- config/module.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/config/module.go b/config/module.go index c32dd34b349..36258e5f41d 100644 --- a/config/module.go +++ b/config/module.go @@ -377,7 +377,7 @@ func (m *Module) FirstRun( return nil } -// TODO(bashar): write test(s) +// TODO(RSDK-9498): write test(s) // getJSONManifest returns a loaded meta.json from one of three sources (in order of precedence): // 1. if this is an online module and there is a meta.json in its top level directory, use that. // 3. if there is a meta.json in the exe dir, use that, exept in local non-tarball case. From 0524189b9e702abe1331554b9edf4652d164b931 Mon Sep 17 00:00:00 2001 From: Bashar Eid Date: Wed, 8 Jan 2025 15:37:36 -0500 Subject: [PATCH 18/50] fix numbering typo in comment --- config/module.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/config/module.go b/config/module.go index 36258e5f41d..b6f7438077c 100644 --- a/config/module.go +++ b/config/module.go @@ -380,8 +380,8 @@ func (m *Module) FirstRun( // TODO(RSDK-9498): write test(s) // getJSONManifest returns a loaded meta.json from one of three sources (in order of precedence): // 1. if this is an online module and there is a meta.json in its top level directory, use that. -// 3. if there is a meta.json in the exe dir, use that, exept in local non-tarball case. -// 4. if this is a local tarball and there's a meta.json next to the tarball, use that. +// 2. if there is a meta.json in the exe dir, use that, exept in local non-tarball case. +// 3. if this is a local tarball and there's a meta.json next to the tarball, use that. // Note: the working directory must be the unpacked tarball directory or local exec directory. func (m Module) getJSONManifest(unpackedModDir string, env map[string]string) (*JSONManifest, string, error) { // note: all online modules qualify for cases 1 & 2; local tarballs for cases 2 & 3; and local non-tarballs for none. We don't look at From 159b72316a721aa5bfb838de50b4233a6ade4da8 Mon Sep 17 00:00:00 2001 From: Bashar Eid Date: Wed, 8 Jan 2025 15:59:02 -0500 Subject: [PATCH 19/50] correctly return error --- config/module.go | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/config/module.go b/config/module.go index b6f7438077c..90288bc6534 100644 --- a/config/module.go +++ b/config/module.go @@ -400,7 +400,10 @@ func (m Module) getJSONManifest(unpackedModDir string, env map[string]string) (* if ok { meta, err := findMetaJSONFile(moduleWorkingDirectory) if err != nil { - return nil, "", err + // return from getJSONManifest() if the error returned does NOT indicate that the file wasn't found + if !os.IsNotExist(err) { + return nil, "", err + } } if meta != nil { @@ -415,7 +418,9 @@ func (m Module) getJSONManifest(unpackedModDir string, env map[string]string) (* if !localNonTarball { meta, err := findMetaJSONFile(unpackedModDir) if err != nil { - return nil, "", err + if !os.IsNotExist(err) { + return nil, "", err + } } if meta != nil { @@ -432,7 +437,9 @@ func (m Module) getJSONManifest(unpackedModDir string, env map[string]string) (* meta, err := findMetaJSONFile(exeDir) if err != nil { - return nil, "", err + if !os.IsNotExist(err) { + return nil, "", err + } } if meta != nil { @@ -464,7 +471,7 @@ func findMetaJSONFile(dir string) (*JSONManifest, error) { _, err = os.Stat(metaPath) if err != nil { - return nil, nil + return nil, err } meta, err := parseJSONFile[JSONManifest](metaPath) From 7797a3438cc6b08b6e2214bddffde6103731e8f8 Mon Sep 17 00:00:00 2001 From: Bashar Eid Date: Thu, 9 Jan 2025 15:17:50 -0500 Subject: [PATCH 20/50] lint --- config/module.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/config/module.go b/config/module.go index 90288bc6534..e4245144983 100644 --- a/config/module.go +++ b/config/module.go @@ -449,7 +449,8 @@ func (m Module) getJSONManifest(unpackedModDir string, env map[string]string) (* if online { if !ok { - return nil, "", errors.Errorf("VIAM_MODULE_ROOT not set. Searched instead in executable directory %s but failed to find meta.json", unpackedModDir) + return nil, "", errors.Errorf("VIAM_MODULE_ROOT not set. Searched instead in executable directory %s but failed to find meta.json", + unpackedModDir) } return nil, "", errors.Errorf("failed to find meta.json. Searched in executable directory %s and path set by VIAM_MODULE_ROOT %s", From ce8f7b9e9b553a2fa4a04074baf25fce8565437e Mon Sep 17 00:00:00 2001 From: Bashar Eid Date: Thu, 9 Jan 2025 16:45:46 -0500 Subject: [PATCH 21/50] fix typo --- config/module.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/config/module.go b/config/module.go index e4245144983..590ace75460 100644 --- a/config/module.go +++ b/config/module.go @@ -380,7 +380,7 @@ func (m *Module) FirstRun( // TODO(RSDK-9498): write test(s) // getJSONManifest returns a loaded meta.json from one of three sources (in order of precedence): // 1. if this is an online module and there is a meta.json in its top level directory, use that. -// 2. if there is a meta.json in the exe dir, use that, exept in local non-tarball case. +// 2. if there is a meta.json in the exe dir, use that, except in local non-tarball case. // 3. if this is a local tarball and there's a meta.json next to the tarball, use that. // Note: the working directory must be the unpacked tarball directory or local exec directory. func (m Module) getJSONManifest(unpackedModDir string, env map[string]string) (*JSONManifest, string, error) { From a4a0a9ccf491a28100e717da6a185ee45effe97d Mon Sep 17 00:00:00 2001 From: Bashar Eid Date: Thu, 9 Jan 2025 17:38:28 -0500 Subject: [PATCH 22/50] add unit tests for finding meta.json file --- config/module_test.go | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/config/module_test.go b/config/module_test.go index efe192edf70..938360c2a05 100644 --- a/config/module_test.go +++ b/config/module_test.go @@ -102,6 +102,35 @@ func TestSyntheticModule(t *testing.T) { }) } +func testFindMetaJSONFile(t *testing.T) { + tmp := t.TempDir() + metaJSONFilePath := filepath.Join(tmp, "meta.json") + + t.Run("MissingMetaFile", func(t *testing.T) { + meta, err := findMetaJSONFile(tmp) + test.That(t, meta, test.ShouldBeNil) + test.That(t, err, test.ShouldEqual, os.IsNotExist) + }) + + file, err := os.Create(metaJSONFilePath) + test.That(t, err, test.ShouldBeNil) + defer file.Close() + t.Run("InvalidMetaFile", func(t *testing.T) { + meta, err := findMetaJSONFile(tmp) + test.That(t, meta, test.ShouldBeNil) + test.That(t, err, test.ShouldNotBeNil) + test.That(t, err, test.ShouldNotEqual, os.IsNotExist) + }) + + validMeta := JSONManifest{Entrypoint: "entry"} + testWriteJSON(t, metaJSONFilePath, &validMeta) + t.Run("ValidMetaFileFound", func(t *testing.T) { + meta, err := findMetaJSONFile(tmp) + test.That(t, meta, test.ShouldEqual, validMeta) + test.That(t, err, test.ShouldBeNil) + }) +} + // testWriteJSON is a t.Helper that serializes `value` to `path` as json. func testWriteJSON(t *testing.T, path string, value any) { t.Helper() From c0ed578bbbbf538fee7b4bbd03b54d984fc616f3 Mon Sep 17 00:00:00 2001 From: Bashar Eid Date: Thu, 9 Jan 2025 17:40:10 -0500 Subject: [PATCH 23/50] export previously added unit test --- config/module_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/config/module_test.go b/config/module_test.go index 938360c2a05..910156539b4 100644 --- a/config/module_test.go +++ b/config/module_test.go @@ -102,7 +102,7 @@ func TestSyntheticModule(t *testing.T) { }) } -func testFindMetaJSONFile(t *testing.T) { +func TestFindMetaJSONFile(t *testing.T) { tmp := t.TempDir() metaJSONFilePath := filepath.Join(tmp, "meta.json") From 242c644bfa210a7fb54b2031a7198c6ef02d8032 Mon Sep 17 00:00:00 2001 From: Bashar Eid Date: Thu, 9 Jan 2025 17:54:20 -0500 Subject: [PATCH 24/50] correctly check error type in unit tests --- config/module_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/config/module_test.go b/config/module_test.go index 910156539b4..0e5c0c21a64 100644 --- a/config/module_test.go +++ b/config/module_test.go @@ -109,7 +109,7 @@ func TestFindMetaJSONFile(t *testing.T) { t.Run("MissingMetaFile", func(t *testing.T) { meta, err := findMetaJSONFile(tmp) test.That(t, meta, test.ShouldBeNil) - test.That(t, err, test.ShouldEqual, os.IsNotExist) + test.That(t, os.IsNotExist(err), test.ShouldBeTrue) }) file, err := os.Create(metaJSONFilePath) @@ -126,7 +126,7 @@ func TestFindMetaJSONFile(t *testing.T) { testWriteJSON(t, metaJSONFilePath, &validMeta) t.Run("ValidMetaFileFound", func(t *testing.T) { meta, err := findMetaJSONFile(tmp) - test.That(t, meta, test.ShouldEqual, validMeta) + test.That(t, *meta, test.ShouldResemble, validMeta) test.That(t, err, test.ShouldBeNil) }) } From 06382f42af37eba7e329f333a6fe998f8409eef0 Mon Sep 17 00:00:00 2001 From: Bashar Eid Date: Fri, 10 Jan 2025 10:47:29 -0500 Subject: [PATCH 25/50] reword errors --- config/module.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/config/module.go b/config/module.go index 590ace75460..66423067763 100644 --- a/config/module.go +++ b/config/module.go @@ -458,10 +458,10 @@ func (m Module) getJSONManifest(unpackedModDir string, env map[string]string) (* } if !localNonTarball { - return nil, "", errors.Errorf("failed to find meta.json. Searched in %s and %s", unpackedModDir, exeDir) + return nil, "", errors.Errorf("failed to find meta.json. Searched in %s and executable directory %s", unpackedModDir, exeDir) } - return nil, "", errors.Errorf("failed to find meta.json. Searched only in %s", exeDir) + return nil, "", errors.Errorf("local tarball: failed to find meta.json. Searched only in %s", exeDir) } func findMetaJSONFile(dir string) (*JSONManifest, error) { From 165277838a3b76b17eda0cd30724caf9f4cae407 Mon Sep 17 00:00:00 2001 From: Bashar Eid Date: Fri, 10 Jan 2025 10:55:23 -0500 Subject: [PATCH 26/50] include module type in errors --- config/module.go | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/config/module.go b/config/module.go index 66423067763..3640291d789 100644 --- a/config/module.go +++ b/config/module.go @@ -402,7 +402,7 @@ func (m Module) getJSONManifest(unpackedModDir string, env map[string]string) (* if err != nil { // return from getJSONManifest() if the error returned does NOT indicate that the file wasn't found if !os.IsNotExist(err) { - return nil, "", err + return nil, "", errors.Wrap(err, "registry module") } } @@ -419,7 +419,11 @@ func (m Module) getJSONManifest(unpackedModDir string, env map[string]string) (* meta, err := findMetaJSONFile(unpackedModDir) if err != nil { if !os.IsNotExist(err) { - return nil, "", err + if online { + return nil, "", errors.Wrap(err, "registry module") + } + + return nil, "", errors.Wrap(err, "local non-tarball") } } @@ -438,7 +442,7 @@ func (m Module) getJSONManifest(unpackedModDir string, env map[string]string) (* meta, err := findMetaJSONFile(exeDir) if err != nil { if !os.IsNotExist(err) { - return nil, "", err + return nil, "", errors.Wrap(err, "local tarball") } } @@ -449,16 +453,16 @@ func (m Module) getJSONManifest(unpackedModDir string, env map[string]string) (* if online { if !ok { - return nil, "", errors.Errorf("VIAM_MODULE_ROOT not set. Searched instead in executable directory %s but failed to find meta.json", + return nil, "", errors.Errorf("registry module: VIAM_MODULE_ROOT not set. Searched instead in executable directory %s but failed to find meta.json", unpackedModDir) } - return nil, "", errors.Errorf("failed to find meta.json. Searched in executable directory %s and path set by VIAM_MODULE_ROOT %s", + return nil, "", errors.Errorf("registry module: failed to find meta.json. Searched in executable directory %s and path set by VIAM_MODULE_ROOT %s", moduleWorkingDirectory, unpackedModDir) } if !localNonTarball { - return nil, "", errors.Errorf("failed to find meta.json. Searched in %s and executable directory %s", unpackedModDir, exeDir) + return nil, "", errors.Errorf("local non-tarball: failed to find meta.json. Searched in %s and executable directory %s", unpackedModDir, exeDir) } return nil, "", errors.Errorf("local tarball: failed to find meta.json. Searched only in %s", exeDir) From ac9380a814319a6afe3ec611abd3c56a7c6b5516 Mon Sep 17 00:00:00 2001 From: Bashar Eid Date: Fri, 10 Jan 2025 13:23:55 -0500 Subject: [PATCH 27/50] clarify cases --- config/module.go | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/config/module.go b/config/module.go index 3640291d789..95d3f223ae5 100644 --- a/config/module.go +++ b/config/module.go @@ -402,70 +402,70 @@ func (m Module) getJSONManifest(unpackedModDir string, env map[string]string) (* if err != nil { // return from getJSONManifest() if the error returned does NOT indicate that the file wasn't found if !os.IsNotExist(err) { - return nil, "", errors.Wrap(err, "registry module") + return nil, "", errors.Wrap(err, "registry module") // TODO } } if meta != nil { - return meta, moduleWorkingDirectory, nil + return meta, moduleWorkingDirectory, nil // TODO } } } localNonTarball := m.Type == ModuleTypeLocal && !m.NeedsSyntheticPackage() - // case 2: online OR non-tarball + // case 2: online OR tarball if !localNonTarball { meta, err := findMetaJSONFile(unpackedModDir) if err != nil { if !os.IsNotExist(err) { if online { - return nil, "", errors.Wrap(err, "registry module") + return nil, "", errors.Wrap(err, "registry module") // TODO } - return nil, "", errors.Wrap(err, "local non-tarball") + return nil, "", errors.Wrap(err, "local tarball") // DONE } } if meta != nil { - return meta, unpackedModDir, nil + return meta, unpackedModDir, nil // TODO } } var exeDir string // TODO(RSDK-7848): remove this case once java sdk supports internal meta.json. - // case 3: local AND tarball + // case 3: local AND non-tarball if m.NeedsSyntheticPackage() { exeDir = filepath.Dir(m.ExePath) meta, err := findMetaJSONFile(exeDir) if err != nil { if !os.IsNotExist(err) { - return nil, "", errors.Wrap(err, "local tarball") + return nil, "", errors.Wrap(err, "local non-tarball") // TODO } } if meta != nil { - return meta, unpackedModDir, nil + return meta, unpackedModDir, nil // TODO } } if online { if !ok { return nil, "", errors.Errorf("registry module: VIAM_MODULE_ROOT not set. Searched instead in executable directory %s but failed to find meta.json", - unpackedModDir) + unpackedModDir) // TODO } return nil, "", errors.Errorf("registry module: failed to find meta.json. Searched in executable directory %s and path set by VIAM_MODULE_ROOT %s", - moduleWorkingDirectory, unpackedModDir) + moduleWorkingDirectory, unpackedModDir) // TODO } if !localNonTarball { - return nil, "", errors.Errorf("local non-tarball: failed to find meta.json. Searched in %s and executable directory %s", unpackedModDir, exeDir) + return nil, "", errors.Errorf("local tarball: failed to find meta.json. Searched in %s and executable directory %s", unpackedModDir, exeDir) // TODO } - return nil, "", errors.Errorf("local tarball: failed to find meta.json. Searched only in %s", exeDir) + return nil, "", errors.Errorf("local non-tarball: failed to find meta.json. Searched only in %s", exeDir) // TODO } func findMetaJSONFile(dir string) (*JSONManifest, error) { From 3732b8af9412f222cc9637fae15fe1587aac803b Mon Sep 17 00:00:00 2001 From: Bashar Eid Date: Fri, 10 Jan 2025 13:27:55 -0500 Subject: [PATCH 28/50] remove unnecessary conditions --- config/module.go | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/config/module.go b/config/module.go index 95d3f223ae5..738da87b6bb 100644 --- a/config/module.go +++ b/config/module.go @@ -406,9 +406,7 @@ func (m Module) getJSONManifest(unpackedModDir string, env map[string]string) (* } } - if meta != nil { - return meta, moduleWorkingDirectory, nil // TODO - } + return meta, moduleWorkingDirectory, nil // TODO } } @@ -427,9 +425,7 @@ func (m Module) getJSONManifest(unpackedModDir string, env map[string]string) (* } } - if meta != nil { - return meta, unpackedModDir, nil // TODO - } + return meta, unpackedModDir, nil // TODO } var exeDir string @@ -446,9 +442,7 @@ func (m Module) getJSONManifest(unpackedModDir string, env map[string]string) (* } } - if meta != nil { - return meta, unpackedModDir, nil // TODO - } + return meta, unpackedModDir, nil // TODO } if online { From 581f6b8215dcbc88b007bff0fa412b8ad5613c1a Mon Sep 17 00:00:00 2001 From: Bashar Eid Date: Fri, 10 Jan 2025 18:12:11 -0500 Subject: [PATCH 29/50] begin implementing unit tests --- config/module.go | 66 +++++++++++++++++++++++++------------------ config/module_test.go | 63 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 102 insertions(+), 27 deletions(-) diff --git a/config/module.go b/config/module.go index 738da87b6bb..9a6aa4adcf9 100644 --- a/config/module.go +++ b/config/module.go @@ -4,6 +4,7 @@ import ( "bufio" "context" "encoding/json" + stderrors "errors" "fmt" "os" "os/exec" @@ -379,87 +380,98 @@ func (m *Module) FirstRun( // TODO(RSDK-9498): write test(s) // getJSONManifest returns a loaded meta.json from one of three sources (in order of precedence): -// 1. if this is an online module and there is a meta.json in its top level directory, use that. +// 1. if this is an registry module and there is a meta.json in its top level directory, use that. // 2. if there is a meta.json in the exe dir, use that, except in local non-tarball case. // 3. if this is a local tarball and there's a meta.json next to the tarball, use that. // Note: the working directory must be the unpacked tarball directory or local exec directory. func (m Module) getJSONManifest(unpackedModDir string, env map[string]string) (*JSONManifest, string, error) { - // note: all online modules qualify for cases 1 & 2; local tarballs for cases 2 & 3; and local non-tarballs for none. We don't look at + // note: all registry modules qualify for cases 1 & 2; local tarballs for cases 2 & 3; and local non-tarballs for case 3. We don't look at // internal meta.json in local non-tarball case because user has explicitly requested a binary. // note: each case is exited iff no errors occur but the meta.json file is not found var ok bool var moduleWorkingDirectory string + var registryErr error online := m.Type == ModuleTypeRegistry - // case 1: online + // case 1: registry if online { moduleWorkingDirectory, ok = env["VIAM_MODULE_ROOT"] if ok { - meta, err := findMetaJSONFile(moduleWorkingDirectory) - if err != nil { + var meta *JSONManifest + meta, registryErr = findMetaJSONFile(moduleWorkingDirectory) + if registryErr != nil { // return from getJSONManifest() if the error returned does NOT indicate that the file wasn't found - if !os.IsNotExist(err) { - return nil, "", errors.Wrap(err, "registry module") // TODO + if !os.IsNotExist(registryErr) { + return nil, "", errors.Wrap(registryErr, "registry module") } } - return meta, moduleWorkingDirectory, nil // TODO + if meta != nil { + return meta, moduleWorkingDirectory, nil + } } } + var registryTarballErr error + localNonTarball := m.Type == ModuleTypeLocal && !m.NeedsSyntheticPackage() - // case 2: online OR tarball + // case 2: registry OR tarball if !localNonTarball { - meta, err := findMetaJSONFile(unpackedModDir) - if err != nil { - if !os.IsNotExist(err) { + var meta *JSONManifest + meta, registryTarballErr = findMetaJSONFile(unpackedModDir) + if registryTarballErr != nil { + if !os.IsNotExist(registryTarballErr) { if online { - return nil, "", errors.Wrap(err, "registry module") // TODO + return nil, "", errors.Wrap(registryTarballErr, "registry module") // DONE } - return nil, "", errors.Wrap(err, "local tarball") // DONE + return nil, "", errors.Wrap(registryTarballErr, "local tarball") } } - return meta, unpackedModDir, nil // TODO + if meta != nil { + return meta, unpackedModDir, nil // DONE + } } var exeDir string + var localTarballErr error // TODO(RSDK-7848): remove this case once java sdk supports internal meta.json. - // case 3: local AND non-tarball + // case 3: local AND tarball if m.NeedsSyntheticPackage() { exeDir = filepath.Dir(m.ExePath) - meta, err := findMetaJSONFile(exeDir) - if err != nil { - if !os.IsNotExist(err) { - return nil, "", errors.Wrap(err, "local non-tarball") // TODO + var meta *JSONManifest + meta, localTarballErr = findMetaJSONFile(exeDir) + if localTarballErr != nil { + if !os.IsNotExist(localTarballErr) { + return nil, "", errors.Wrap(localTarballErr, "local non-tarball") } } - return meta, unpackedModDir, nil // TODO + if meta != nil { + return meta, exeDir, nil + } } if online { if !ok { - return nil, "", errors.Errorf("registry module: VIAM_MODULE_ROOT not set. Searched instead in executable directory %s but failed to find meta.json", - unpackedModDir) // TODO + return nil, "", errors.Wrap(registryTarballErr, "registry module: failed to find meta.json. VIAM_MODULE_ROOT not set") // DONE } - return nil, "", errors.Errorf("registry module: failed to find meta.json. Searched in executable directory %s and path set by VIAM_MODULE_ROOT %s", - moduleWorkingDirectory, unpackedModDir) // TODO + return nil, "", errors.Wrap(stderrors.Join(registryErr, registryTarballErr), "registry module: failed to find meta.json") // DONE } if !localNonTarball { - return nil, "", errors.Errorf("local tarball: failed to find meta.json. Searched in %s and executable directory %s", unpackedModDir, exeDir) // TODO + return nil, "", errors.Wrap(stderrors.Join(registryTarballErr, localTarballErr), "local tarball: failed to find meta.json") } - return nil, "", errors.Errorf("local non-tarball: failed to find meta.json. Searched only in %s", exeDir) // TODO + return nil, "", errors.Errorf("local non-tarball: did not search for meta.json") } func findMetaJSONFile(dir string) (*JSONManifest, error) { diff --git a/config/module_test.go b/config/module_test.go index 0e5c0c21a64..d2dadb25572 100644 --- a/config/module_test.go +++ b/config/module_test.go @@ -6,6 +6,7 @@ import ( "path/filepath" "testing" + "github.com/pkg/errors" "go.viam.com/test" ) @@ -102,6 +103,68 @@ func TestSyntheticModule(t *testing.T) { }) } +func TestGetJSONManifest(t *testing.T) { + validJSONManifest := JSONManifest{Entrypoint: "entry"} + + t.Run("RegistryModule", func(t *testing.T) { + tmp := t.TempDir() + + topLevelDir := tmp + topLevelMetaJSONFilepath := filepath.Join(topLevelDir, "meta.json") + unpackedModDir := filepath.Join(tmp, "unpacked-mod-dir") + unpackedModMetaJSONFilepath := filepath.Join(unpackedModDir, "meta.json") + env := make(map[string]string, 1) + modRegistry := Module{Type: ModuleTypeRegistry} + + err := os.Mkdir(unpackedModDir, 0700) + + // meta.json not found; only unpacked module directory searched + meta, moduleWorkingDirectory, err := modRegistry.getJSONManifest(unpackedModDir, env) + test.That(t, meta, test.ShouldBeNil) + test.That(t, moduleWorkingDirectory, test.ShouldBeEmpty) + test.That(t, err, test.ShouldNotBeNil) + test.That(t, err.Error(), test.ShouldContainSubstring, "registry module") + test.That(t, errors.Is(err, os.ErrNotExist), test.ShouldBeTrue) + test.That(t, err.Error(), test.ShouldContainSubstring, unpackedModMetaJSONFilepath) + test.That(t, err.Error(), test.ShouldNotContainSubstring, topLevelMetaJSONFilepath) + + // meta.json not found; top level module directory and unpacked module directories searched + env["VIAM_MODULE_ROOT"] = tmp + + meta, moduleWorkingDirectory, err = modRegistry.getJSONManifest(unpackedModDir, env) + test.That(t, meta, test.ShouldBeNil) + test.That(t, moduleWorkingDirectory, test.ShouldBeEmpty) + test.That(t, err, test.ShouldNotBeNil) + test.That(t, err.Error(), test.ShouldContainSubstring, "registry module") + test.That(t, errors.Is(err, os.ErrNotExist), test.ShouldBeTrue) + test.That(t, err.Error(), test.ShouldContainSubstring, unpackedModMetaJSONFilepath) + test.That(t, err.Error(), test.ShouldContainSubstring, topLevelMetaJSONFilepath) + + // meta.json found in unpacked modular directory; parsing fails + unpackedModMetaJSONFile, err := os.Create(unpackedModMetaJSONFilepath) + test.That(t, err, test.ShouldBeNil) + defer unpackedModMetaJSONFile.Close() + + meta, moduleWorkingDirectory, err = modRegistry.getJSONManifest(unpackedModDir, env) + test.That(t, meta, test.ShouldBeNil) + test.That(t, moduleWorkingDirectory, test.ShouldBeEmpty) + test.That(t, err, test.ShouldNotBeNil) + test.That(t, err.Error(), test.ShouldContainSubstring, "registry module") + test.That(t, errors.Is(err, os.ErrNotExist), test.ShouldBeFalse) + + // meta.json found in unpacked modular directory; parsing succeeds + testWriteJSON(t, unpackedModMetaJSONFilepath, validJSONManifest) + + meta, moduleWorkingDirectory, err = modRegistry.getJSONManifest(unpackedModDir, env) + test.That(t, *meta, test.ShouldResemble, validJSONManifest) + test.That(t, moduleWorkingDirectory, test.ShouldEqual, unpackedModDir) + test.That(t, err, test.ShouldBeNil) + + // meta.json found in top level modular directory; parsing fails + // meta.json found in top level modular directory; parsing succeeds + }) +} + func TestFindMetaJSONFile(t *testing.T) { tmp := t.TempDir() metaJSONFilePath := filepath.Join(tmp, "meta.json") From 33ae953ae7c61962501125d35ae80163f379e382 Mon Sep 17 00:00:00 2001 From: Bashar Eid Date: Mon, 13 Jan 2025 10:37:54 -0500 Subject: [PATCH 30/50] write unit tests for registry module case --- config/module_test.go | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/config/module_test.go b/config/module_test.go index d2dadb25572..05e4933f51f 100644 --- a/config/module_test.go +++ b/config/module_test.go @@ -129,7 +129,7 @@ func TestGetJSONManifest(t *testing.T) { test.That(t, err.Error(), test.ShouldNotContainSubstring, topLevelMetaJSONFilepath) // meta.json not found; top level module directory and unpacked module directories searched - env["VIAM_MODULE_ROOT"] = tmp + env["VIAM_MODULE_ROOT"] = topLevelDir meta, moduleWorkingDirectory, err = modRegistry.getJSONManifest(unpackedModDir, env) test.That(t, meta, test.ShouldBeNil) @@ -161,7 +161,24 @@ func TestGetJSONManifest(t *testing.T) { test.That(t, err, test.ShouldBeNil) // meta.json found in top level modular directory; parsing fails + topLevelMetaJSONFile, err := os.Create(topLevelMetaJSONFilepath) + test.That(t, err, test.ShouldBeNil) + defer topLevelMetaJSONFile.Close() + + meta, moduleWorkingDirectory, err = modRegistry.getJSONManifest(unpackedModDir, env) + test.That(t, meta, test.ShouldBeNil) + test.That(t, moduleWorkingDirectory, test.ShouldBeEmpty) + test.That(t, err, test.ShouldNotBeNil) + test.That(t, err.Error(), test.ShouldContainSubstring, "registry module") + test.That(t, errors.Is(err, os.ErrNotExist), test.ShouldBeFalse) + // meta.json found in top level modular directory; parsing succeeds + testWriteJSON(t, topLevelMetaJSONFilepath, validJSONManifest) + + meta, moduleWorkingDirectory, err = modRegistry.getJSONManifest(unpackedModDir, env) + test.That(t, *meta, test.ShouldResemble, validJSONManifest) + test.That(t, moduleWorkingDirectory, test.ShouldEqual, topLevelDir) + test.That(t, err, test.ShouldBeNil) }) } From e5f6122370be22db1494b312a6d8c4423e24f02b Mon Sep 17 00:00:00 2001 From: Bashar Eid Date: Mon, 13 Jan 2025 10:46:16 -0500 Subject: [PATCH 31/50] test local non-tarball case --- config/module_test.go | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/config/module_test.go b/config/module_test.go index 05e4933f51f..6f1e4e3e1ec 100644 --- a/config/module_test.go +++ b/config/module_test.go @@ -117,6 +117,7 @@ func TestGetJSONManifest(t *testing.T) { modRegistry := Module{Type: ModuleTypeRegistry} err := os.Mkdir(unpackedModDir, 0700) + test.That(t, err, test.ShouldBeNil) // meta.json not found; only unpacked module directory searched meta, moduleWorkingDirectory, err := modRegistry.getJSONManifest(unpackedModDir, env) @@ -180,6 +181,24 @@ func TestGetJSONManifest(t *testing.T) { test.That(t, moduleWorkingDirectory, test.ShouldEqual, topLevelDir) test.That(t, err, test.ShouldBeNil) }) + + t.Run("LocalNontarball", func(t *testing.T) { + tmp := t.TempDir() + + unpackedModDir := filepath.Join(tmp, "unpacked-mod-dir") + env := map[string]string{} + modLocalNontar := Module{Type: ModuleTypeLocal} + + err := os.Mkdir(unpackedModDir, 0700) + test.That(t, err, test.ShouldBeNil) + + meta, moduleWorkingDirectory, err := modLocalNontar.getJSONManifest(unpackedModDir, env) + test.That(t, meta, test.ShouldBeNil) + test.That(t, moduleWorkingDirectory, test.ShouldBeEmpty) + test.That(t, err, test.ShouldNotBeNil) + test.That(t, err.Error(), test.ShouldContainSubstring, "local non-tarball") + test.That(t, errors.Is(err, os.ErrNotExist), test.ShouldBeFalse) + }) } func TestFindMetaJSONFile(t *testing.T) { From 566680866461d47d962d4a75af622aa9f211cb22 Mon Sep 17 00:00:00 2001 From: Bashar Eid Date: Mon, 13 Jan 2025 10:47:07 -0500 Subject: [PATCH 32/50] fix case description --- config/module.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/config/module.go b/config/module.go index 9a6aa4adcf9..0e9634a0b0b 100644 --- a/config/module.go +++ b/config/module.go @@ -405,12 +405,12 @@ func (m Module) getJSONManifest(unpackedModDir string, env map[string]string) (* if registryErr != nil { // return from getJSONManifest() if the error returned does NOT indicate that the file wasn't found if !os.IsNotExist(registryErr) { - return nil, "", errors.Wrap(registryErr, "registry module") + return nil, "", errors.Wrap(registryErr, "registry module") // DONE } } if meta != nil { - return meta, moduleWorkingDirectory, nil + return meta, moduleWorkingDirectory, nil // DONE } } } @@ -450,7 +450,7 @@ func (m Module) getJSONManifest(unpackedModDir string, env map[string]string) (* meta, localTarballErr = findMetaJSONFile(exeDir) if localTarballErr != nil { if !os.IsNotExist(localTarballErr) { - return nil, "", errors.Wrap(localTarballErr, "local non-tarball") + return nil, "", errors.Wrap(localTarballErr, "local tarball") } } From 20fc5226a000cebff952b95295ef7c75d422db4a Mon Sep 17 00:00:00 2001 From: Bashar Eid Date: Mon, 13 Jan 2025 10:51:40 -0500 Subject: [PATCH 33/50] mark test as complete --- config/module.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/config/module.go b/config/module.go index 0e9634a0b0b..5bbab7dd19d 100644 --- a/config/module.go +++ b/config/module.go @@ -471,7 +471,7 @@ func (m Module) getJSONManifest(unpackedModDir string, env map[string]string) (* return nil, "", errors.Wrap(stderrors.Join(registryTarballErr, localTarballErr), "local tarball: failed to find meta.json") } - return nil, "", errors.Errorf("local non-tarball: did not search for meta.json") + return nil, "", errors.Errorf("local non-tarball: did not search for meta.json") // DONE } func findMetaJSONFile(dir string) (*JSONManifest, error) { From a97e64886028b3be7fc6f2c27d0175a83f4cbcf5 Mon Sep 17 00:00:00 2001 From: Bashar Eid Date: Mon, 13 Jan 2025 10:58:36 -0500 Subject: [PATCH 34/50] correctly create error --- config/module.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/config/module.go b/config/module.go index 5bbab7dd19d..428f613c403 100644 --- a/config/module.go +++ b/config/module.go @@ -471,7 +471,7 @@ func (m Module) getJSONManifest(unpackedModDir string, env map[string]string) (* return nil, "", errors.Wrap(stderrors.Join(registryTarballErr, localTarballErr), "local tarball: failed to find meta.json") } - return nil, "", errors.Errorf("local non-tarball: did not search for meta.json") // DONE + return nil, "", errors.New("local non-tarball: did not search for meta.json") // DONE } func findMetaJSONFile(dir string) (*JSONManifest, error) { From a9f1ff922d8a7a494f1c4ee35e2206103216be8c Mon Sep 17 00:00:00 2001 From: Bashar Eid Date: Mon, 13 Jan 2025 11:20:08 -0500 Subject: [PATCH 35/50] lint --- config/module_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/config/module_test.go b/config/module_test.go index 6f1e4e3e1ec..a51adfda9ce 100644 --- a/config/module_test.go +++ b/config/module_test.go @@ -116,7 +116,7 @@ func TestGetJSONManifest(t *testing.T) { env := make(map[string]string, 1) modRegistry := Module{Type: ModuleTypeRegistry} - err := os.Mkdir(unpackedModDir, 0700) + err := os.Mkdir(unpackedModDir, 0o700) test.That(t, err, test.ShouldBeNil) // meta.json not found; only unpacked module directory searched @@ -189,7 +189,7 @@ func TestGetJSONManifest(t *testing.T) { env := map[string]string{} modLocalNontar := Module{Type: ModuleTypeLocal} - err := os.Mkdir(unpackedModDir, 0700) + err := os.Mkdir(unpackedModDir, 0o700) test.That(t, err, test.ShouldBeNil) meta, moduleWorkingDirectory, err := modLocalNontar.getJSONManifest(unpackedModDir, env) From b2318af1bb69351c0a41555117c4f137b2055994 Mon Sep 17 00:00:00 2001 From: Bashar Eid Date: Mon, 13 Jan 2025 13:13:36 -0500 Subject: [PATCH 36/50] finish unit tests for getJSONManifest() --- config/module.go | 16 +++++------ config/module_test.go | 65 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 73 insertions(+), 8 deletions(-) diff --git a/config/module.go b/config/module.go index 428f613c403..6251e38bd98 100644 --- a/config/module.go +++ b/config/module.go @@ -380,7 +380,7 @@ func (m *Module) FirstRun( // TODO(RSDK-9498): write test(s) // getJSONManifest returns a loaded meta.json from one of three sources (in order of precedence): -// 1. if this is an registry module and there is a meta.json in its top level directory, use that. +// 1. if this is a registry module and there is a meta.json in its top level directory, use that. // 2. if there is a meta.json in the exe dir, use that, except in local non-tarball case. // 3. if this is a local tarball and there's a meta.json next to the tarball, use that. // Note: the working directory must be the unpacked tarball directory or local exec directory. @@ -405,12 +405,12 @@ func (m Module) getJSONManifest(unpackedModDir string, env map[string]string) (* if registryErr != nil { // return from getJSONManifest() if the error returned does NOT indicate that the file wasn't found if !os.IsNotExist(registryErr) { - return nil, "", errors.Wrap(registryErr, "registry module") // DONE + return nil, "", errors.Wrap(registryErr, "registry module") } } if meta != nil { - return meta, moduleWorkingDirectory, nil // DONE + return meta, moduleWorkingDirectory, nil } } } @@ -426,7 +426,7 @@ func (m Module) getJSONManifest(unpackedModDir string, env map[string]string) (* if registryTarballErr != nil { if !os.IsNotExist(registryTarballErr) { if online { - return nil, "", errors.Wrap(registryTarballErr, "registry module") // DONE + return nil, "", errors.Wrap(registryTarballErr, "registry module") } return nil, "", errors.Wrap(registryTarballErr, "local tarball") @@ -434,7 +434,7 @@ func (m Module) getJSONManifest(unpackedModDir string, env map[string]string) (* } if meta != nil { - return meta, unpackedModDir, nil // DONE + return meta, unpackedModDir, nil } } @@ -461,17 +461,17 @@ func (m Module) getJSONManifest(unpackedModDir string, env map[string]string) (* if online { if !ok { - return nil, "", errors.Wrap(registryTarballErr, "registry module: failed to find meta.json. VIAM_MODULE_ROOT not set") // DONE + return nil, "", errors.Wrap(registryTarballErr, "registry module: failed to find meta.json. VIAM_MODULE_ROOT not set") } - return nil, "", errors.Wrap(stderrors.Join(registryErr, registryTarballErr), "registry module: failed to find meta.json") // DONE + return nil, "", errors.Wrap(stderrors.Join(registryErr, registryTarballErr), "registry module: failed to find meta.json") } if !localNonTarball { return nil, "", errors.Wrap(stderrors.Join(registryTarballErr, localTarballErr), "local tarball: failed to find meta.json") } - return nil, "", errors.New("local non-tarball: did not search for meta.json") // DONE + return nil, "", errors.New("local non-tarball: did not search for meta.json") } func findMetaJSONFile(dir string) (*JSONManifest, error) { diff --git a/config/module_test.go b/config/module_test.go index a51adfda9ce..8732e08c2b8 100644 --- a/config/module_test.go +++ b/config/module_test.go @@ -182,6 +182,71 @@ func TestGetJSONManifest(t *testing.T) { test.That(t, err, test.ShouldBeNil) }) + t.Run("LocalTarball", func(t *testing.T) { + tmp := t.TempDir() + + exePath := filepath.Join(tmp, "module.tgz") + exeDir := filepath.Dir(exePath) + exeMetaJSONFilepath := filepath.Join(exeDir, "meta.json") + unpackedModDir := filepath.Join(tmp, "unpacked-mod-dir") + unpackedModMetaJSONFilepath := filepath.Join(unpackedModDir, "meta.json") + env := map[string]string{} + modLocalTar := Module{Type: ModuleTypeLocal, ExePath: exePath} + + err := os.Mkdir(unpackedModDir, 0o700) + test.That(t, err, test.ShouldBeNil) + + // meta.json not found; unpacked module and executable directories searched + meta, moduleWorkingDirectory, err := modLocalTar.getJSONManifest(unpackedModDir, env) + test.That(t, meta, test.ShouldBeNil) + test.That(t, moduleWorkingDirectory, test.ShouldBeEmpty) + test.That(t, err, test.ShouldNotBeNil) + test.That(t, err.Error(), test.ShouldContainSubstring, "local tarball") + test.That(t, errors.Is(err, os.ErrNotExist), test.ShouldBeTrue) + test.That(t, err.Error(), test.ShouldContainSubstring, unpackedModDir) + test.That(t, err.Error(), test.ShouldContainSubstring, exeDir) + + // meta.json found in executable directory; parsing fails + exeMetaJSONFile, err := os.Create(exeMetaJSONFilepath) + test.That(t, err, test.ShouldBeNil) + defer exeMetaJSONFile.Close() + + meta, moduleWorkingDirectory, err = modLocalTar.getJSONManifest(unpackedModDir, env) + test.That(t, meta, test.ShouldBeNil) + test.That(t, moduleWorkingDirectory, test.ShouldBeEmpty) + test.That(t, err, test.ShouldNotBeNil) + test.That(t, err.Error(), test.ShouldContainSubstring, "local tarball") + test.That(t, errors.Is(err, os.ErrNotExist), test.ShouldBeFalse) + + // meta.json found in executable directory; parsing succeeds + testWriteJSON(t, exeMetaJSONFilepath, validJSONManifest) + + meta, moduleWorkingDirectory, err = modLocalTar.getJSONManifest(unpackedModDir, env) + test.That(t, *meta, test.ShouldResemble, validJSONManifest) + test.That(t, moduleWorkingDirectory, test.ShouldEqual, exeDir) + test.That(t, err, test.ShouldBeNil) + + // meta.json found in unpacked modular directory; parsing fails + unpackedModMetaJSONFile, err := os.Create(unpackedModMetaJSONFilepath) + test.That(t, err, test.ShouldBeNil) + defer unpackedModMetaJSONFile.Close() + + meta, moduleWorkingDirectory, err = modLocalTar.getJSONManifest(unpackedModDir, env) + test.That(t, meta, test.ShouldBeNil) + test.That(t, moduleWorkingDirectory, test.ShouldBeEmpty) + test.That(t, err, test.ShouldNotBeNil) + test.That(t, err.Error(), test.ShouldContainSubstring, "local tarball") + test.That(t, errors.Is(err, os.ErrNotExist), test.ShouldBeFalse) + + // meta.json found in unpacked module directory; parsing succeeds + testWriteJSON(t, unpackedModMetaJSONFilepath, validJSONManifest) + + meta, moduleWorkingDirectory, err = modLocalTar.getJSONManifest(unpackedModDir, env) + test.That(t, *meta, test.ShouldResemble, validJSONManifest) + test.That(t, moduleWorkingDirectory, test.ShouldEqual, unpackedModDir) + test.That(t, err, test.ShouldBeNil) + }) + t.Run("LocalNontarball", func(t *testing.T) { tmp := t.TempDir() From e700d0f6711a1659f92189c411e41b3f8fc7be98 Mon Sep 17 00:00:00 2001 From: Bashar Eid Date: Mon, 13 Jan 2025 13:17:57 -0500 Subject: [PATCH 37/50] update comments --- config/module.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/config/module.go b/config/module.go index 6251e38bd98..72676031706 100644 --- a/config/module.go +++ b/config/module.go @@ -382,10 +382,10 @@ func (m *Module) FirstRun( // getJSONManifest returns a loaded meta.json from one of three sources (in order of precedence): // 1. if this is a registry module and there is a meta.json in its top level directory, use that. // 2. if there is a meta.json in the exe dir, use that, except in local non-tarball case. -// 3. if this is a local tarball and there's a meta.json next to the tarball, use that. +// 3. if this is a local tarball, use the meta.json in unpackedModDir. // Note: the working directory must be the unpacked tarball directory or local exec directory. func (m Module) getJSONManifest(unpackedModDir string, env map[string]string) (*JSONManifest, string, error) { - // note: all registry modules qualify for cases 1 & 2; local tarballs for cases 2 & 3; and local non-tarballs for case 3. We don't look at + // note: all registry modules qualify for cases 1 & 2; local tarballs for cases 2 & 3; and local non-tarballs for none. We don't look at // internal meta.json in local non-tarball case because user has explicitly requested a binary. // note: each case is exited iff no errors occur but the meta.json file is not found From 550223150a39a98bd0012d51875028a465162930 Mon Sep 17 00:00:00 2001 From: Bashar Eid Date: Mon, 13 Jan 2025 14:50:28 -0500 Subject: [PATCH 38/50] write FirstRun() unit tests --- config/module_test.go | 61 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 61 insertions(+) diff --git a/config/module_test.go b/config/module_test.go index 8732e08c2b8..cedd902f1ec 100644 --- a/config/module_test.go +++ b/config/module_test.go @@ -1,12 +1,15 @@ package config import ( + "bytes" + "context" "encoding/json" "os" "path/filepath" "testing" "github.com/pkg/errors" + "go.viam.com/rdk/logging" "go.viam.com/test" ) @@ -103,6 +106,60 @@ func TestSyntheticModule(t *testing.T) { }) } +func TestFirstRun(t *testing.T) { + m := Module{Type: ModuleTypeRegistry} + + tmp := t.TempDir() + exePath := filepath.Join(tmp, "whatever.sh") + m.ExePath = exePath + metaJSONFilepath := filepath.Join(tmp, "meta.json") + + ctx := context.Background() + localPackagesDir := "" + dataDir := "" + env := map[string]string{"VIAM_MODULE_ROOT": tmp} + logger := logging.NewTestLogger(t) + + t.Run("MetaFileNotFound", func(t *testing.T) { + err := m.FirstRun(ctx, localPackagesDir, dataDir, env, logger) + // TODO: test logger output + // "meta.json not found, skipping first run" + test.That(t, err, test.ShouldBeNil) + }) + + t.Run("MetaFileInvalid", func(t *testing.T) { + metaJSONFile, err := os.Create(metaJSONFilepath) + test.That(t, err, test.ShouldBeNil) + defer metaJSONFile.Close() + + err = m.FirstRun(ctx, localPackagesDir, dataDir, env, logger) + // TODO: test logger output + // "failed to parse meta.json, skipping first run" + test.That(t, err, test.ShouldBeNil) + }) + + t.Run("NoFirstRunScript", func(t *testing.T) { + testWriteJSON(t, metaJSONFilepath, JSONManifest{}) + + err := m.FirstRun(ctx, localPackagesDir, dataDir, env, logger) + t.Log(err) + // TODO: test logger output + // "no first run script specified, skipping first run" + test.That(t, err, test.ShouldBeNil) + }) + + t.Run("InvalidFirstRunPath", func(t *testing.T) { + testWriteJSON(t, metaJSONFilepath, JSONManifest{FirstRun: "../firstrun.sh"}) + + err := m.FirstRun(ctx, localPackagesDir, dataDir, env, logger) + t.Log(err) + // TODO: test logger output + // "failed to build path to first run script, skipping first run" + test.That(t, err, test.ShouldBeNil) + + }) +} + func TestGetJSONManifest(t *testing.T) { validJSONManifest := JSONManifest{Entrypoint: "entry"} @@ -305,3 +362,7 @@ func testWriteJSON(t *testing.T, path string, value any) { err = encoder.Encode(value) test.That(t, err, test.ShouldBeNil) } + +func testCheckLogOutput(t *testing.T, buf bytes.Buffer, expected string) { + t.Helper() +} From 6d503449d1283e2b2ceda95ca739e30c3bfed351 Mon Sep 17 00:00:00 2001 From: Bashar Eid Date: Mon, 13 Jan 2025 15:00:50 -0500 Subject: [PATCH 39/50] remove unused helper function --- config/module_test.go | 4 ---- 1 file changed, 4 deletions(-) diff --git a/config/module_test.go b/config/module_test.go index cedd902f1ec..83d34d90c30 100644 --- a/config/module_test.go +++ b/config/module_test.go @@ -362,7 +362,3 @@ func testWriteJSON(t *testing.T, path string, value any) { err = encoder.Encode(value) test.That(t, err, test.ShouldBeNil) } - -func testCheckLogOutput(t *testing.T, buf bytes.Buffer, expected string) { - t.Helper() -} From a3a79edb2a4fa99820e79844663489e84f3c4957 Mon Sep 17 00:00:00 2001 From: Bashar Eid Date: Mon, 13 Jan 2025 15:02:04 -0500 Subject: [PATCH 40/50] remove unused import --- config/module_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/config/module_test.go b/config/module_test.go index 83d34d90c30..c33408d3fd1 100644 --- a/config/module_test.go +++ b/config/module_test.go @@ -1,7 +1,6 @@ package config import ( - "bytes" "context" "encoding/json" "os" From 995fd410fcd29052ce235601ce4eb9f1ea1822bc Mon Sep 17 00:00:00 2001 From: Bashar Eid Date: Mon, 13 Jan 2025 15:02:44 -0500 Subject: [PATCH 41/50] lint --- config/module_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/config/module_test.go b/config/module_test.go index c33408d3fd1..0b7eb13a2a5 100644 --- a/config/module_test.go +++ b/config/module_test.go @@ -8,8 +8,9 @@ import ( "testing" "github.com/pkg/errors" - "go.viam.com/rdk/logging" "go.viam.com/test" + + "go.viam.com/rdk/logging" ) // testChdir is a helper that cleans up an os.Chdir. @@ -155,7 +156,6 @@ func TestFirstRun(t *testing.T) { // TODO: test logger output // "failed to build path to first run script, skipping first run" test.That(t, err, test.ShouldBeNil) - }) } From 1cec229862a2d39024e7a73025bb5c17b0325a11 Mon Sep 17 00:00:00 2001 From: Bashar Eid Date: Mon, 13 Jan 2025 15:20:24 -0500 Subject: [PATCH 42/50] check logs --- config/module_test.go | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/config/module_test.go b/config/module_test.go index 0b7eb13a2a5..e7e0fcd3175 100644 --- a/config/module_test.go +++ b/config/module_test.go @@ -118,13 +118,12 @@ func TestFirstRun(t *testing.T) { localPackagesDir := "" dataDir := "" env := map[string]string{"VIAM_MODULE_ROOT": tmp} - logger := logging.NewTestLogger(t) + logger, observedLogs := logging.NewObservedTestLogger(t) t.Run("MetaFileNotFound", func(t *testing.T) { err := m.FirstRun(ctx, localPackagesDir, dataDir, env, logger) - // TODO: test logger output - // "meta.json not found, skipping first run" test.That(t, err, test.ShouldBeNil) + test.That(t, observedLogs.FilterMessage("meta.json not found, skipping first run").Len(), test.ShouldEqual, 1) }) t.Run("MetaFileInvalid", func(t *testing.T) { @@ -133,9 +132,8 @@ func TestFirstRun(t *testing.T) { defer metaJSONFile.Close() err = m.FirstRun(ctx, localPackagesDir, dataDir, env, logger) - // TODO: test logger output - // "failed to parse meta.json, skipping first run" test.That(t, err, test.ShouldBeNil) + test.That(t, observedLogs.FilterMessage("failed to parse meta.json, skipping first run").Len(), test.ShouldEqual, 1) }) t.Run("NoFirstRunScript", func(t *testing.T) { @@ -143,9 +141,8 @@ func TestFirstRun(t *testing.T) { err := m.FirstRun(ctx, localPackagesDir, dataDir, env, logger) t.Log(err) - // TODO: test logger output - // "no first run script specified, skipping first run" test.That(t, err, test.ShouldBeNil) + test.That(t, observedLogs.FilterMessage("no first run script specified, skipping first run").Len(), test.ShouldEqual, 1) }) t.Run("InvalidFirstRunPath", func(t *testing.T) { @@ -153,9 +150,8 @@ func TestFirstRun(t *testing.T) { err := m.FirstRun(ctx, localPackagesDir, dataDir, env, logger) t.Log(err) - // TODO: test logger output - // "failed to build path to first run script, skipping first run" test.That(t, err, test.ShouldBeNil) + test.That(t, observedLogs.FilterMessage("failed to build path to first run script, skipping first run").Len(), test.ShouldEqual, 1) }) } From a9360d66090872c369ec83ea58d8cbc498b68097 Mon Sep 17 00:00:00 2001 From: Bashar Eid Date: Mon, 13 Jan 2025 15:58:24 -0500 Subject: [PATCH 43/50] only search unpacked module directory if different from top level module directory --- config/module.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/config/module.go b/config/module.go index 72676031706..33b1a1746bf 100644 --- a/config/module.go +++ b/config/module.go @@ -420,7 +420,7 @@ func (m Module) getJSONManifest(unpackedModDir string, env map[string]string) (* localNonTarball := m.Type == ModuleTypeLocal && !m.NeedsSyntheticPackage() // case 2: registry OR tarball - if !localNonTarball { + if !localNonTarball && unpackedModDir != moduleWorkingDirectory { var meta *JSONManifest meta, registryTarballErr = findMetaJSONFile(unpackedModDir) if registryTarballErr != nil { From 74985147f681569cab38444afe479636a8ebea50 Mon Sep 17 00:00:00 2001 From: Bashar Eid Date: Wed, 15 Jan 2025 14:43:35 -0500 Subject: [PATCH 44/50] add comments explaining environment variables --- config/module_test.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/config/module_test.go b/config/module_test.go index e7e0fcd3175..a1a736b81f4 100644 --- a/config/module_test.go +++ b/config/module_test.go @@ -117,7 +117,10 @@ func TestFirstRun(t *testing.T) { ctx := context.Background() localPackagesDir := "" dataDir := "" + + // getJSONManifest() uses the "VIAM_MODUE_ROOT" environment variable to find the top level directory of a registry module env := map[string]string{"VIAM_MODULE_ROOT": tmp} + logger, observedLogs := logging.NewObservedTestLogger(t) t.Run("MetaFileNotFound", func(t *testing.T) { @@ -182,6 +185,9 @@ func TestGetJSONManifest(t *testing.T) { test.That(t, err.Error(), test.ShouldNotContainSubstring, topLevelMetaJSONFilepath) // meta.json not found; top level module directory and unpacked module directories searched + + // setting the "VIAM_MODULE_ROOT" environment variable allows getJSONManifest() to search in a registry module's top level directory + // for the meta.json file. The variable is accessed through the 'env' function parameter env["VIAM_MODULE_ROOT"] = topLevelDir meta, moduleWorkingDirectory, err = modRegistry.getJSONManifest(unpackedModDir, env) From c6b66b13fc9fdc83592d435077a233ad4ec022e9 Mon Sep 17 00:00:00 2001 From: Bashar Eid Date: Wed, 15 Jan 2025 14:45:01 -0500 Subject: [PATCH 45/50] rename test function to specify that it only tests registry modules --- config/module_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/config/module_test.go b/config/module_test.go index a1a736b81f4..1dc961ab144 100644 --- a/config/module_test.go +++ b/config/module_test.go @@ -106,7 +106,7 @@ func TestSyntheticModule(t *testing.T) { }) } -func TestFirstRun(t *testing.T) { +func TestRegistryModuleFirstRun(t *testing.T) { m := Module{Type: ModuleTypeRegistry} tmp := t.TempDir() From 6c8f702b94a8d23ae64853212a5966e79d9adba7 Mon Sep 17 00:00:00 2001 From: Bashar Eid Date: Wed, 15 Jan 2025 15:32:43 -0500 Subject: [PATCH 46/50] create helper function --- config/module_test.go | 61 ++++++++++++++++++++++++++++++++++--------- 1 file changed, 48 insertions(+), 13 deletions(-) diff --git a/config/module_test.go b/config/module_test.go index 1dc961ab144..9a2ff5a8b70 100644 --- a/config/module_test.go +++ b/config/module_test.go @@ -8,6 +8,7 @@ import ( "testing" "github.com/pkg/errors" + "go.uber.org/zap/zaptest/observer" "go.viam.com/test" "go.viam.com/rdk/logging" @@ -107,29 +108,35 @@ func TestSyntheticModule(t *testing.T) { } func TestRegistryModuleFirstRun(t *testing.T) { - m := Module{Type: ModuleTypeRegistry} - - tmp := t.TempDir() - exePath := filepath.Join(tmp, "whatever.sh") - m.ExePath = exePath - metaJSONFilepath := filepath.Join(tmp, "meta.json") - ctx := context.Background() localPackagesDir := "" dataDir := "" - // getJSONManifest() uses the "VIAM_MODUE_ROOT" environment variable to find the top level directory of a registry module - env := map[string]string{"VIAM_MODULE_ROOT": tmp} + t.Run("MetaFileNotFound", func(t *testing.T) { + m := Module{Type: ModuleTypeRegistry} + tmp := t.TempDir() + exePath := filepath.Join(tmp, "whatever.sh") + m.ExePath = exePath + + // getJSONManifest() uses the "VIAM_MODUE_ROOT" environment variable to find the top level directory of a registry module + env := map[string]string{"VIAM_MODULE_ROOT": tmp} - logger, observedLogs := logging.NewObservedTestLogger(t) + logger, observedLogs := logging.NewObservedTestLogger(t) - t.Run("MetaFileNotFound", func(t *testing.T) { err := m.FirstRun(ctx, localPackagesDir, dataDir, env, logger) test.That(t, err, test.ShouldBeNil) test.That(t, observedLogs.FilterMessage("meta.json not found, skipping first run").Len(), test.ShouldEqual, 1) }) t.Run("MetaFileInvalid", func(t *testing.T) { + m := Module{Type: ModuleTypeRegistry} + tmp := t.TempDir() + exePath := filepath.Join(tmp, "whatever.sh") + m.ExePath = exePath + metaJSONFilepath := filepath.Join(tmp, "meta.json") + env := map[string]string{"VIAM_MODULE_ROOT": tmp} + logger, observedLogs := logging.NewObservedTestLogger(t) + metaJSONFile, err := os.Create(metaJSONFilepath) test.That(t, err, test.ShouldBeNil) defer metaJSONFile.Close() @@ -140,19 +147,33 @@ func TestRegistryModuleFirstRun(t *testing.T) { }) t.Run("NoFirstRunScript", func(t *testing.T) { + m := Module{Type: ModuleTypeRegistry} + tmp := t.TempDir() + exePath := filepath.Join(tmp, "whatever.sh") + m.ExePath = exePath + metaJSONFilepath := filepath.Join(tmp, "meta.json") + env := map[string]string{"VIAM_MODULE_ROOT": tmp} + logger, observedLogs := logging.NewObservedTestLogger(t) + testWriteJSON(t, metaJSONFilepath, JSONManifest{}) err := m.FirstRun(ctx, localPackagesDir, dataDir, env, logger) - t.Log(err) test.That(t, err, test.ShouldBeNil) test.That(t, observedLogs.FilterMessage("no first run script specified, skipping first run").Len(), test.ShouldEqual, 1) }) t.Run("InvalidFirstRunPath", func(t *testing.T) { + m := Module{Type: ModuleTypeRegistry} + tmp := t.TempDir() + exePath := filepath.Join(tmp, "whatever.sh") + m.ExePath = exePath + metaJSONFilepath := filepath.Join(tmp, "meta.json") + env := map[string]string{"VIAM_MODULE_ROOT": tmp} + logger, observedLogs := logging.NewObservedTestLogger(t) + testWriteJSON(t, metaJSONFilepath, JSONManifest{FirstRun: "../firstrun.sh"}) err := m.FirstRun(ctx, localPackagesDir, dataDir, env, logger) - t.Log(err) test.That(t, err, test.ShouldBeNil) test.That(t, observedLogs.FilterMessage("failed to build path to first run script, skipping first run").Len(), test.ShouldEqual, 1) }) @@ -363,3 +384,17 @@ func testWriteJSON(t *testing.T, path string, value any) { err = encoder.Encode(value) test.That(t, err, test.ShouldBeNil) } + +// testSetUpRegistryModule is a t.Helper that creates a registry module with a meta.json file and an executable file in its top level +// directory. It also returns a logger and its observed logs for testing +func testSetUpRegistryModule(t *testing.T) (module Module, metaJSONFilepath string, env map[string]string, logger logging.Logger, observedLogs *observer.ObservedLogs) { + t.Helper() + module = Module{Type: ModuleTypeRegistry} + tmp := t.TempDir() + exePath := filepath.Join(tmp, "whatever.sh") + module.ExePath = exePath + metaJSONFilepath = filepath.Join(tmp, "meta.json") + env["VIAM_MODULE_ROOT"] = tmp + logger, observedLogs = logging.NewObservedTestLogger(t) + return +} From f00fc03c57d718e1a0e9df8b4dd1dead6554ac83 Mon Sep 17 00:00:00 2001 From: Bashar Eid Date: Wed, 15 Jan 2025 15:36:23 -0500 Subject: [PATCH 47/50] lint --- config/module_test.go | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/config/module_test.go b/config/module_test.go index 9a2ff5a8b70..16eed57e59c 100644 --- a/config/module_test.go +++ b/config/module_test.go @@ -386,15 +386,19 @@ func testWriteJSON(t *testing.T, path string, value any) { } // testSetUpRegistryModule is a t.Helper that creates a registry module with a meta.json file and an executable file in its top level -// directory. It also returns a logger and its observed logs for testing -func testSetUpRegistryModule(t *testing.T) (module Module, metaJSONFilepath string, env map[string]string, logger logging.Logger, observedLogs *observer.ObservedLogs) { +// directory. It also returns a logger and its observed logs for testing. +func testSetUpRegistryModule(t *testing.T) (module Module, metaJSONFilepath string, env map[string]string, logger logging.Logger, + observedLogs *observer.ObservedLogs) { t.Helper() module = Module{Type: ModuleTypeRegistry} tmp := t.TempDir() exePath := filepath.Join(tmp, "whatever.sh") module.ExePath = exePath metaJSONFilepath = filepath.Join(tmp, "meta.json") + + env = make(map[string]string, 1) env["VIAM_MODULE_ROOT"] = tmp + logger, observedLogs = logging.NewObservedTestLogger(t) return } From 6698cfceef25240b453eed58e5d742f5cb680ea3 Mon Sep 17 00:00:00 2001 From: Bashar Eid Date: Wed, 15 Jan 2025 16:58:45 -0500 Subject: [PATCH 48/50] add new tests --- config/module_test.go | 88 +++++++++++++++++++++++++++---------------- 1 file changed, 56 insertions(+), 32 deletions(-) diff --git a/config/module_test.go b/config/module_test.go index 16eed57e59c..df71acd2200 100644 --- a/config/module_test.go +++ b/config/module_test.go @@ -113,69 +113,92 @@ func TestRegistryModuleFirstRun(t *testing.T) { dataDir := "" t.Run("MetaFileNotFound", func(t *testing.T) { - m := Module{Type: ModuleTypeRegistry} - tmp := t.TempDir() - exePath := filepath.Join(tmp, "whatever.sh") - m.ExePath = exePath - - // getJSONManifest() uses the "VIAM_MODUE_ROOT" environment variable to find the top level directory of a registry module - env := map[string]string{"VIAM_MODULE_ROOT": tmp} - - logger, observedLogs := logging.NewObservedTestLogger(t) + module, _, env, logger, observedLogs := testSetUpRegistryModule(t) - err := m.FirstRun(ctx, localPackagesDir, dataDir, env, logger) + err := module.FirstRun(ctx, localPackagesDir, dataDir, env, logger) test.That(t, err, test.ShouldBeNil) test.That(t, observedLogs.FilterMessage("meta.json not found, skipping first run").Len(), test.ShouldEqual, 1) }) t.Run("MetaFileInvalid", func(t *testing.T) { - m := Module{Type: ModuleTypeRegistry} - tmp := t.TempDir() - exePath := filepath.Join(tmp, "whatever.sh") - m.ExePath = exePath - metaJSONFilepath := filepath.Join(tmp, "meta.json") - env := map[string]string{"VIAM_MODULE_ROOT": tmp} - logger, observedLogs := logging.NewObservedTestLogger(t) + module, metaJSONFilepath, env, logger, observedLogs := testSetUpRegistryModule(t) metaJSONFile, err := os.Create(metaJSONFilepath) test.That(t, err, test.ShouldBeNil) defer metaJSONFile.Close() - err = m.FirstRun(ctx, localPackagesDir, dataDir, env, logger) + err = module.FirstRun(ctx, localPackagesDir, dataDir, env, logger) test.That(t, err, test.ShouldBeNil) test.That(t, observedLogs.FilterMessage("failed to parse meta.json, skipping first run").Len(), test.ShouldEqual, 1) }) t.Run("NoFirstRunScript", func(t *testing.T) { - m := Module{Type: ModuleTypeRegistry} + module, metaJSONFilepath, env, logger, observedLogs := testSetUpRegistryModule(t) + + testWriteJSON(t, metaJSONFilepath, JSONManifest{}) + + err := module.FirstRun(ctx, localPackagesDir, dataDir, env, logger) + test.That(t, err, test.ShouldBeNil) + test.That(t, observedLogs.FilterMessage("no first run script specified, skipping first run").Len(), test.ShouldEqual, 1) + }) + + t.Run("InvalidFirstRunPath", func(t *testing.T) { + module, metaJSONFilepath, env, logger, observedLogs := testSetUpRegistryModule(t) + + testWriteJSON(t, metaJSONFilepath, JSONManifest{FirstRun: "../firstrun.sh"}) + + err := module.FirstRun(ctx, localPackagesDir, dataDir, env, logger) + test.That(t, err, test.ShouldBeNil) + test.That(t, observedLogs.FilterMessage("failed to build path to first run script, skipping first run").Len(), test.ShouldEqual, 1) + }) + + // the executable is one level deep, and the meta.json file is in the same directory + t.Run("NoFirstRunScriptOneLevelExe", func(t *testing.T) { + module := Module{Type: ModuleTypeRegistry} + tmp := t.TempDir() - exePath := filepath.Join(tmp, "whatever.sh") - m.ExePath = exePath - metaJSONFilepath := filepath.Join(tmp, "meta.json") + exeDir := filepath.Join(tmp, "executable-directory") + exePath := filepath.Join(exeDir, "whatever.sh") + module.ExePath = exePath + exeMetaJSONFilepath := filepath.Join(exeDir, "meta.json") + env := map[string]string{"VIAM_MODULE_ROOT": tmp} + logger, observedLogs := logging.NewObservedTestLogger(t) - testWriteJSON(t, metaJSONFilepath, JSONManifest{}) + err := os.Mkdir(exeDir, 0o700) + test.That(t, err, test.ShouldBeNil) + + testWriteJSON(t, exeMetaJSONFilepath, JSONManifest{}) - err := m.FirstRun(ctx, localPackagesDir, dataDir, env, logger) + err = module.FirstRun(ctx, localPackagesDir, dataDir, env, logger) test.That(t, err, test.ShouldBeNil) test.That(t, observedLogs.FilterMessage("no first run script specified, skipping first run").Len(), test.ShouldEqual, 1) }) - t.Run("InvalidFirstRunPath", func(t *testing.T) { - m := Module{Type: ModuleTypeRegistry} + // the executable is one level deep, and the meta.json file is in the top level directory + t.Run("NoFirstRunScriptOneLevelTopLevel", func(t *testing.T) { + module := Module{Type: ModuleTypeRegistry} + tmp := t.TempDir() - exePath := filepath.Join(tmp, "whatever.sh") - m.ExePath = exePath - metaJSONFilepath := filepath.Join(tmp, "meta.json") + topLevelDir := tmp + exeDir := filepath.Join(tmp, "executable-directory") + exePath := filepath.Join(exeDir, "whatever.sh") + module.ExePath = exePath + exeMetaJSONFilepath := filepath.Join(topLevelDir, "meta.json") + env := map[string]string{"VIAM_MODULE_ROOT": tmp} + logger, observedLogs := logging.NewObservedTestLogger(t) - testWriteJSON(t, metaJSONFilepath, JSONManifest{FirstRun: "../firstrun.sh"}) + err := os.Mkdir(exeDir, 0o700) + test.That(t, err, test.ShouldBeNil) + + testWriteJSON(t, exeMetaJSONFilepath, JSONManifest{}) - err := m.FirstRun(ctx, localPackagesDir, dataDir, env, logger) + err = module.FirstRun(ctx, localPackagesDir, dataDir, env, logger) test.That(t, err, test.ShouldBeNil) - test.That(t, observedLogs.FilterMessage("failed to build path to first run script, skipping first run").Len(), test.ShouldEqual, 1) + test.That(t, observedLogs.FilterMessage("no first run script specified, skipping first run").Len(), test.ShouldEqual, 1) }) } @@ -396,6 +419,7 @@ func testSetUpRegistryModule(t *testing.T) (module Module, metaJSONFilepath stri module.ExePath = exePath metaJSONFilepath = filepath.Join(tmp, "meta.json") + // getJSONManifest() uses the "VIAM_MODUE_ROOT" environment variable to find the top level directory of a registry module env = make(map[string]string, 1) env["VIAM_MODULE_ROOT"] = tmp From 99e3305479aa9255bc7aacfa86bec6cf428c2238 Mon Sep 17 00:00:00 2001 From: Bashar Eid Date: Wed, 15 Jan 2025 16:59:36 -0500 Subject: [PATCH 49/50] lint --- config/module_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/config/module_test.go b/config/module_test.go index df71acd2200..8329dad5c23 100644 --- a/config/module_test.go +++ b/config/module_test.go @@ -411,7 +411,8 @@ func testWriteJSON(t *testing.T, path string, value any) { // testSetUpRegistryModule is a t.Helper that creates a registry module with a meta.json file and an executable file in its top level // directory. It also returns a logger and its observed logs for testing. func testSetUpRegistryModule(t *testing.T) (module Module, metaJSONFilepath string, env map[string]string, logger logging.Logger, - observedLogs *observer.ObservedLogs) { + observedLogs *observer.ObservedLogs, +) { t.Helper() module = Module{Type: ModuleTypeRegistry} tmp := t.TempDir() From 52656ca3e86170361fcda21572da8cd9bb8e7681 Mon Sep 17 00:00:00 2001 From: Bashar Eid Date: Thu, 16 Jan 2025 14:45:12 -0500 Subject: [PATCH 50/50] remove TODO comment --- config/module.go | 1 - 1 file changed, 1 deletion(-) diff --git a/config/module.go b/config/module.go index 33b1a1746bf..7040e8a77ae 100644 --- a/config/module.go +++ b/config/module.go @@ -378,7 +378,6 @@ func (m *Module) FirstRun( return nil } -// TODO(RSDK-9498): write test(s) // getJSONManifest returns a loaded meta.json from one of three sources (in order of precedence): // 1. if this is a registry module and there is a meta.json in its top level directory, use that. // 2. if there is a meta.json in the exe dir, use that, except in local non-tarball case.