From 9bcade1a9e0a17af1335bf041f869c6200fd4dc4 Mon Sep 17 00:00:00 2001 From: Cheuk Tse Date: Wed, 21 Aug 2024 12:26:28 -0400 Subject: [PATCH 01/19] chages --- config/reader.go | 13 ++++++++++++- robot/impl/local_robot.go | 5 +++++ 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/config/reader.go b/config/reader.go index d0cedc7eeae..d121e81c309 100644 --- a/config/reader.go +++ b/config/reader.go @@ -95,6 +95,10 @@ func getCloudCacheFilePath(id string) string { return filepath.Join(ViamDotDir, fmt.Sprintf("cached_cloud_config_%s.json", id)) } +func getNextCloudCacheFilePath(id string) string { + return filepath.Join(ViamDotDir, fmt.Sprintf("cached_cloud_config_%s_next.json", id)) +} + func readFromCache(id string) (*Config, error) { r, err := os.Open(getCloudCacheFilePath(id)) if err != nil { @@ -125,11 +129,18 @@ func storeToCache(id string, cfg *Config) error { } reader := bytes.NewReader(md) - path := getCloudCacheFilePath(id) + path := getNextCloudCacheFilePath(id) return artifact.AtomicStore(path, reader, id) } +func ReplaceCache(id string) error { + nextCachePath := getNextCloudCacheFilePath(id) + path := getCloudCacheFilePath(id) + + return os.Rename(nextCachePath, path) +} + func clearCache(id string) { utils.UncheckedErrorFunc(func() error { return os.Remove(getCloudCacheFilePath(id)) diff --git a/robot/impl/local_robot.go b/robot/impl/local_robot.go index a5a702b792f..a4a3e6ac0a4 100644 --- a/robot/impl/local_robot.go +++ b/robot/impl/local_robot.go @@ -1178,6 +1178,11 @@ func (r *localRobot) reconfigure(ctx context.Context, newConfig *config.Config, // if anything has changed. err := r.packageManager.Sync(ctx, newConfig.Packages, newConfig.Modules) if err != nil { + r.Logger().CErrorw(ctx, "reconfiguration aborted because package sync failed", "error", err) + return + } + r.Logger().CDebug(ctx, "replacing cache") + if err := config.ReplaceCache(newConfig.Cloud.ID); err != nil { allErrs = multierr.Combine(allErrs, err) } // For local tarball modules, we create synthetic versions for package management. The `localRobot` keeps track of these because From c75c474c6fb6cd5ef6f63bfff5e549a145e240db Mon Sep 17 00:00:00 2001 From: Cheuk Tse Date: Fri, 30 Aug 2024 19:32:05 -0400 Subject: [PATCH 02/19] some initial changes --- config/config.go | 30 +++++++++++++ config/reader.go | 33 +------------- config/reader_test.go | 31 +++++++++++-- robot/impl/local_robot.go | 92 ++++++++++++++++++++++++++++++++------- 4 files changed, 135 insertions(+), 51 deletions(-) diff --git a/config/config.go b/config/config.go index 476ba07de64..37838d882a9 100644 --- a/config/config.go +++ b/config/config.go @@ -2,6 +2,7 @@ package config import ( + "bytes" "crypto/tls" "encoding/json" "fmt" @@ -15,6 +16,7 @@ import ( "time" "github.com/pkg/errors" + "go.viam.com/utils/artifact" "go.viam.com/utils/jwks" "go.viam.com/utils/pexec" "go.viam.com/utils/rpc" @@ -68,6 +70,10 @@ type Config struct { // Revision contains the current revision of the config. Revision string + + // unprocessedConfig stores the unprocessed version of the config that will be cached. + // This version is kept because the config is changed as it moves through the system. + unprocessedConfig *Config } // NOTE: This data must be maintained with what is in Config. @@ -238,6 +244,30 @@ func (c Config) FindComponent(name string) *resource.Config { return nil } +// setUnprocessedConfig updates unprocessedConfig. +func (c *Config) setUnprocessedConfig(cfg *Config) { + c.unprocessedConfig = cfg +} + +// UnprocessedConfig returns unprocessedConfig. +func (c Config) UnprocessedConfig() *Config { + return c.unprocessedConfig +} + +// StoreToCache caches the unprocessedConfig. +func (c Config) StoreToCache() error { + if err := os.MkdirAll(ViamDotDir, 0o700); err != nil { + return err + } + md, err := json.MarshalIndent(c.unprocessedConfig, "", " ") + if err != nil { + return err + } + reader := bytes.NewReader(md) + path := getCloudCacheFilePath(c.Cloud.ID) + return artifact.AtomicStore(path, reader, c.Cloud.ID) +} + // UnmarshalJSON unmarshals JSON into the config and adjusts some // names if they are not fully filled in. func (c *Config) UnmarshalJSON(data []byte) error { diff --git a/config/reader.go b/config/reader.go index d121e81c309..c7f24cc4d98 100644 --- a/config/reader.go +++ b/config/reader.go @@ -17,7 +17,6 @@ import ( "github.com/pkg/errors" apppb "go.viam.com/api/app/v1" "go.viam.com/utils" - "go.viam.com/utils/artifact" "go.viam.com/utils/rpc" "golang.org/x/sys/cpu" @@ -95,10 +94,6 @@ func getCloudCacheFilePath(id string) string { return filepath.Join(ViamDotDir, fmt.Sprintf("cached_cloud_config_%s.json", id)) } -func getNextCloudCacheFilePath(id string) string { - return filepath.Join(ViamDotDir, fmt.Sprintf("cached_cloud_config_%s_next.json", id)) -} - func readFromCache(id string) (*Config, error) { r, err := os.Open(getCloudCacheFilePath(id)) if err != nil { @@ -118,29 +113,6 @@ func readFromCache(id string) (*Config, error) { return unprocessedConfig, nil } -func storeToCache(id string, cfg *Config) error { - if err := os.MkdirAll(ViamDotDir, 0o700); err != nil { - return err - } - - md, err := json.MarshalIndent(cfg, "", " ") - if err != nil { - return err - } - reader := bytes.NewReader(md) - - path := getNextCloudCacheFilePath(id) - - return artifact.AtomicStore(path, reader, id) -} - -func ReplaceCache(id string) error { - nextCachePath := getNextCloudCacheFilePath(id) - path := getCloudCacheFilePath(id) - - return os.Rename(nextCachePath, path) -} - func clearCache(id string) { utils.UncheckedErrorFunc(func() error { return os.Remove(getCloudCacheFilePath(id)) @@ -328,10 +300,7 @@ func readFromCloud( mergeCloudConfig(cfg) unprocessedConfig.Cloud.TLSCertificate = tls.certificate unprocessedConfig.Cloud.TLSPrivateKey = tls.privateKey - - if err := storeToCache(cloudCfg.ID, unprocessedConfig); err != nil { - logger.Errorw("failed to cache config", "error", err) - } + cfg.setUnprocessedConfig(unprocessedConfig) return cfg, nil } diff --git a/config/reader_test.go b/config/reader_test.go index 495bda36e17..f04a0fb52c0 100644 --- a/config/reader_test.go +++ b/config/reader_test.go @@ -1,7 +1,9 @@ package config import ( + "bytes" "context" + "encoding/json" "errors" "fmt" "io/fs" @@ -13,6 +15,7 @@ import ( "github.com/google/uuid" pb "go.viam.com/api/app/v1" "go.viam.com/test" + "go.viam.com/utils/artifact" "go.viam.com/rdk/config/testutils" "go.viam.com/rdk/logging" @@ -20,6 +23,22 @@ import ( "go.viam.com/rdk/services/shell" ) +func storeToCache(id string, cfg *Config) error { + if err := os.MkdirAll(ViamDotDir, 0o700); err != nil { + return err + } + + md, err := json.MarshalIndent(cfg, "", " ") + if err != nil { + return err + } + reader := bytes.NewReader(md) + + path := getCloudCacheFilePath(id) + + return artifact.AtomicStore(path, reader, id) +} + func TestFromReader(t *testing.T) { const ( robotPartID = "forCachingTest" @@ -192,12 +211,15 @@ func TestStoreToCache(t *testing.T) { cfg.Cloud = cloud // store our config to the cloud - err = storeToCache(cfg.Cloud.ID, cfg) + cfgToCache := &Config{Cloud: &Cloud{ID: "forCachingTest"}} + cfgToCache.setUnprocessedConfig(cfg) + err = cfgToCache.StoreToCache() test.That(t, err, test.ShouldBeNil) // read config from cloud, confirm consistency cloudCfg, err := readFromCloud(ctx, cfg, nil, true, false, logger) test.That(t, err, test.ShouldBeNil) + cloudCfg.unprocessedConfig = nil test.That(t, cloudCfg, test.ShouldResemble, cfg) // Modify our config @@ -207,10 +229,12 @@ func TestStoreToCache(t *testing.T) { // read config from cloud again, confirm that the cached config differs from cfg cloudCfg2, err := readFromCloud(ctx, cfg, nil, true, false, logger) test.That(t, err, test.ShouldBeNil) - test.That(t, cloudCfg2, test.ShouldNotResemble, cfg) + cloudCfg2.unprocessedConfig = nil + test.That(t, cloudCfg2, test.ShouldNotResemble, cfgToCache) // store the updated config to the cloud - err = storeToCache(cfg.Cloud.ID, cfg) + cfgToCache.setUnprocessedConfig(cfg) + err = cfgToCache.StoreToCache() test.That(t, err, test.ShouldBeNil) test.That(t, cfg.Ensure(true, logger), test.ShouldBeNil) @@ -218,6 +242,7 @@ func TestStoreToCache(t *testing.T) { // read updated cloud config, confirm that it now matches our updated cfg cloudCfg3, err := readFromCloud(ctx, cfg, nil, true, false, logger) test.That(t, err, test.ShouldBeNil) + cloudCfg3.unprocessedConfig = nil test.That(t, cloudCfg3, test.ShouldResemble, cfg) } diff --git a/robot/impl/local_robot.go b/robot/impl/local_robot.go index bc6b12a94a7..d50265d3637 100644 --- a/robot/impl/local_robot.go +++ b/robot/impl/local_robot.go @@ -5,6 +5,7 @@ package robotimpl import ( + "bytes" "context" "strings" "sync" @@ -57,6 +58,11 @@ type localRobot struct { cloudConnSvc icloud.ConnectionService logger logging.Logger activeBackgroundWorkers sync.WaitGroup + + // configCleanedUp tracks whether the robot has cleaned up after a successful configuration + configCleanUpMu sync.Mutex + configCleanedUp bool + // reconfigureWorkers tracks goroutines spawned by reconfiguration functions. we only // wait on this group in tests to prevent goleak-related failures. however, we do not // wait on this group outside of testing, since the related goroutines may be running @@ -548,6 +554,7 @@ func newWithResources( r.logger.CDebugw(ctx, "configuration attempt triggered by remote") } anyChanges := r.manager.updateRemotesResourceNames(closeCtx) + cfg := r.mostRecentCfg.Load().(config.Config) if r.manager.anyResourcesNotConfigured() { anyChanges = true r.manager.completeConfig(closeCtx, r, false) @@ -555,6 +562,10 @@ func newWithResources( if anyChanges { r.updateWeakDependents(ctx) r.logger.CDebugw(ctx, "configuration attempt completed with changes", "trigger", trigger) + + if !r.manager.anyResourcesNotConfigured() { + r.onConfigurationSuccess(ctx, cfg) + } } } }, r.activeBackgroundWorkers.Done) @@ -1174,13 +1185,6 @@ func (r *localRobot) applyLocalModuleVersions(cfg *config.Config) { } func (r *localRobot) reconfigure(ctx context.Context, newConfig *config.Config, forceSync bool) { - r.configRevisionMu.Lock() - r.configRevision = config.Revision{ - Revision: newConfig.Revision, - LastUpdated: time.Now(), - } - r.configRevisionMu.Unlock() - var allErrs error // Sync Packages before reconfiguring rest of robot and resolving references to any packages @@ -1190,19 +1194,27 @@ func (r *localRobot) reconfigure(ctx context.Context, newConfig *config.Config, // if anything has changed. err := r.packageManager.Sync(ctx, newConfig.Packages, newConfig.Modules) if err != nil { - r.Logger().CErrorw(ctx, "reconfiguration aborted because package sync failed", "error", err) + r.Logger().CErrorw(ctx, "reconfiguration aborted because cloud modules or packages download failed", "error", err) return } - r.Logger().CDebug(ctx, "replacing cache") - if err := config.ReplaceCache(newConfig.Cloud.ID); err != nil { - allErrs = multierr.Combine(allErrs, err) - } // For local tarball modules, we create synthetic versions for package management. The `localRobot` keeps track of these because // config reader would overwrite if we just stored it in config. Here, we copy the synthetic version from the `localRobot` into the // appropriate `config.Module` object inside the `cfg.Modules` slice. Thus, when a local tarball module is reloaded, the viam-server // can unpack it into a fresh directory rather than reusing the previous one. r.applyLocalModuleVersions(newConfig) - allErrs = multierr.Combine(allErrs, r.localPackages.Sync(ctx, newConfig.Packages, newConfig.Modules)) + err = r.localPackages.Sync(ctx, newConfig.Packages, newConfig.Modules) + if err != nil { + r.Logger().CErrorw(ctx, "reconfiguration aborted because local modules or packages sync failed", "error", err) + return + } + + // Update configRevision, as the robot is starting to reconfigure itself. + r.configRevisionMu.Lock() + r.configRevision = config.Revision{ + Revision: newConfig.Revision, + LastUpdated: time.Now(), + } + r.configRevisionMu.Unlock() // Add default services and process their dependencies. Dependencies may // already come from config validation so we check that here. @@ -1274,7 +1286,10 @@ func (r *localRobot) reconfigure(ctx context.Context, newConfig *config.Config, } // Set mostRecentConfig if resources were not equal. + r.configCleanUpMu.Lock() r.mostRecentCfg.Store(*newConfig) + r.configCleanedUp = false + r.configCleanUpMu.Unlock() // First we mark diff.Removed resources and their children for removal. processesToClose, resourcesToCloseBeforeComplete, _ := r.manager.markRemoved(ctx, diff.Removed, r.logger) @@ -1302,17 +1317,62 @@ func (r *localRobot) reconfigure(ctx context.Context, newConfig *config.Config, allErrs = multierr.Combine(allErrs, err) } + if !r.manager.anyResourcesNotConfigured() { + r.onConfigurationSuccess(ctx, *newConfig) + } + + if allErrs != nil { + r.logger.CErrorw(ctx, "The following errors were gathered during reconfiguration", "errors", allErrs) + } else { + r.logger.CInfow(ctx, "Robot (re)configured") + } +} + +func (r *localRobot) onConfigurationSuccess(ctx context.Context, cfg config.Config) { + r.configCleanUpMu.Lock() + defer r.configCleanUpMu.Unlock() + + if r.configCleanedUp { + return + } + + r.Logger().CDebug(ctx, "(re)configuration success, starting cleanup...") + + var allErrs error + + // update cache if this is a cloud-connected robot + if cfg.Cloud != nil { + // check that the unprocessed config in cfg and mostRecentConfig is the same, otherwise it is possible that the config used + // for reconfiguration and the latest resource graph may be mismatched. + c1, err := cfg.UnprocessedConfig().MarshalJSON() + if err != nil { + r.Logger().CDebugw(ctx, "error while marshalling JSON after configuration success", "err", err) + } + + c2, err := r.mostRecentCfg.Load().(config.Config).UnprocessedConfig().MarshalJSON() + if err != nil { + r.Logger().CDebugw(ctx, "error while marshalling JSON after configuration success", "err", err) + } + + if !bytes.Equal(c1, c2) { + return + } + + r.Logger().CDebug(ctx, "updating cache") + allErrs = cfg.StoreToCache() + } + // Cleanup unused packages after all old resources have been closed above. This ensures // processes are shutdown before any files are deleted they are using. allErrs = multierr.Combine(allErrs, r.packageManager.Cleanup(ctx)) allErrs = multierr.Combine(allErrs, r.localPackages.Cleanup(ctx)) // Cleanup extra dirs from previous modules or rogue scripts. allErrs = multierr.Combine(allErrs, r.manager.moduleManager.CleanModuleDataDirectory()) - if allErrs != nil { - r.logger.CErrorw(ctx, "The following errors were gathered during reconfiguration", "errors", allErrs) + r.logger.CDebugw(ctx, "The following errors were gathered during (re)configuration success cleanup", "errors", allErrs) } else { - r.logger.CInfow(ctx, "Robot (re)configured") + r.configCleanedUp = true + r.Logger().CDebug(ctx, "(re)configuration success cleanup successful") } } From e9a5d8ebb1821d78da3d62baaf826fd81d48cda4 Mon Sep 17 00:00:00 2001 From: Cheuk Tse Date: Tue, 3 Sep 2024 16:31:34 -0400 Subject: [PATCH 03/19] pr comment --- config/config.go | 10 ++++++---- config/reader.go | 4 +++- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/config/config.go b/config/config.go index 37838d882a9..0fc5e9e0d78 100644 --- a/config/config.go +++ b/config/config.go @@ -244,9 +244,11 @@ func (c Config) FindComponent(name string) *resource.Config { return nil } -// setUnprocessedConfig updates unprocessedConfig. -func (c *Config) setUnprocessedConfig(cfg *Config) { - c.unprocessedConfig = cfg +// setUnprocessedConfig sets unprocessedConfig with a copy of the config passed in. +func (c *Config) setUnprocessedConfig(cfg *Config) error { + copy, err := cfg.CopyOnlyPublicFields() + c.unprocessedConfig = copy + return err } // UnprocessedConfig returns unprocessedConfig. @@ -259,7 +261,7 @@ func (c Config) StoreToCache() error { if err := os.MkdirAll(ViamDotDir, 0o700); err != nil { return err } - md, err := json.MarshalIndent(c.unprocessedConfig, "", " ") + md, err := json.MarshalIndent(c.UnprocessedConfig(), "", " ") if err != nil { return err } diff --git a/config/reader.go b/config/reader.go index c7f24cc4d98..393a527ba3e 100644 --- a/config/reader.go +++ b/config/reader.go @@ -300,8 +300,10 @@ func readFromCloud( mergeCloudConfig(cfg) unprocessedConfig.Cloud.TLSCertificate = tls.certificate unprocessedConfig.Cloud.TLSPrivateKey = tls.privateKey - cfg.setUnprocessedConfig(unprocessedConfig) + if err := cfg.setUnprocessedConfig(unprocessedConfig); err != nil { + logger.Errorw("failed to set unprocessed config", "error", err) + } return cfg, nil } From e984c8938a7da44040b505f3feb0ca8f7ccd0028 Mon Sep 17 00:00:00 2001 From: Cheuk Tse Date: Tue, 3 Sep 2024 16:33:30 -0400 Subject: [PATCH 04/19] comment --- config/config.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/config/config.go b/config/config.go index 0fc5e9e0d78..dfde698f860 100644 --- a/config/config.go +++ b/config/config.go @@ -252,12 +252,12 @@ func (c *Config) setUnprocessedConfig(cfg *Config) error { } // UnprocessedConfig returns unprocessedConfig. -func (c Config) UnprocessedConfig() *Config { +func (c *Config) UnprocessedConfig() *Config { return c.unprocessedConfig } // StoreToCache caches the unprocessedConfig. -func (c Config) StoreToCache() error { +func (c *Config) StoreToCache() error { if err := os.MkdirAll(ViamDotDir, 0o700); err != nil { return err } From 63f812beccb06c3a369ad00dcc045cd6629c4ae0 Mon Sep 17 00:00:00 2001 From: Cheuk Tse Date: Tue, 3 Sep 2024 16:43:59 -0400 Subject: [PATCH 05/19] revert some changes --- robot/impl/local_robot.go | 67 +++++---------------------------------- 1 file changed, 8 insertions(+), 59 deletions(-) diff --git a/robot/impl/local_robot.go b/robot/impl/local_robot.go index d50265d3637..9b0508ced6e 100644 --- a/robot/impl/local_robot.go +++ b/robot/impl/local_robot.go @@ -5,7 +5,6 @@ package robotimpl import ( - "bytes" "context" "strings" "sync" @@ -554,7 +553,6 @@ func newWithResources( r.logger.CDebugw(ctx, "configuration attempt triggered by remote") } anyChanges := r.manager.updateRemotesResourceNames(closeCtx) - cfg := r.mostRecentCfg.Load().(config.Config) if r.manager.anyResourcesNotConfigured() { anyChanges = true r.manager.completeConfig(closeCtx, r, false) @@ -562,10 +560,6 @@ func newWithResources( if anyChanges { r.updateWeakDependents(ctx) r.logger.CDebugw(ctx, "configuration attempt completed with changes", "trigger", trigger) - - if !r.manager.anyResourcesNotConfigured() { - r.onConfigurationSuccess(ctx, cfg) - } } } }, r.activeBackgroundWorkers.Done) @@ -1208,13 +1202,16 @@ func (r *localRobot) reconfigure(ctx context.Context, newConfig *config.Config, return } + r.Logger().CDebug(ctx, "updating cached config") + if err := newConfig.StoreToCache(); err != nil { + r.logger.CErrorw(ctx, "error storing the config", "error", err) + } + // Update configRevision, as the robot is starting to reconfigure itself. - r.configRevisionMu.Lock() r.configRevision = config.Revision{ Revision: newConfig.Revision, LastUpdated: time.Now(), } - r.configRevisionMu.Unlock() // Add default services and process their dependencies. Dependencies may // already come from config validation so we check that here. @@ -1286,10 +1283,7 @@ func (r *localRobot) reconfigure(ctx context.Context, newConfig *config.Config, } // Set mostRecentConfig if resources were not equal. - r.configCleanUpMu.Lock() r.mostRecentCfg.Store(*newConfig) - r.configCleanedUp = false - r.configCleanUpMu.Unlock() // First we mark diff.Removed resources and their children for removal. processesToClose, resourcesToCloseBeforeComplete, _ := r.manager.markRemoved(ctx, diff.Removed, r.logger) @@ -1317,62 +1311,17 @@ func (r *localRobot) reconfigure(ctx context.Context, newConfig *config.Config, allErrs = multierr.Combine(allErrs, err) } - if !r.manager.anyResourcesNotConfigured() { - r.onConfigurationSuccess(ctx, *newConfig) - } - - if allErrs != nil { - r.logger.CErrorw(ctx, "The following errors were gathered during reconfiguration", "errors", allErrs) - } else { - r.logger.CInfow(ctx, "Robot (re)configured") - } -} - -func (r *localRobot) onConfigurationSuccess(ctx context.Context, cfg config.Config) { - r.configCleanUpMu.Lock() - defer r.configCleanUpMu.Unlock() - - if r.configCleanedUp { - return - } - - r.Logger().CDebug(ctx, "(re)configuration success, starting cleanup...") - - var allErrs error - - // update cache if this is a cloud-connected robot - if cfg.Cloud != nil { - // check that the unprocessed config in cfg and mostRecentConfig is the same, otherwise it is possible that the config used - // for reconfiguration and the latest resource graph may be mismatched. - c1, err := cfg.UnprocessedConfig().MarshalJSON() - if err != nil { - r.Logger().CDebugw(ctx, "error while marshalling JSON after configuration success", "err", err) - } - - c2, err := r.mostRecentCfg.Load().(config.Config).UnprocessedConfig().MarshalJSON() - if err != nil { - r.Logger().CDebugw(ctx, "error while marshalling JSON after configuration success", "err", err) - } - - if !bytes.Equal(c1, c2) { - return - } - - r.Logger().CDebug(ctx, "updating cache") - allErrs = cfg.StoreToCache() - } - // Cleanup unused packages after all old resources have been closed above. This ensures // processes are shutdown before any files are deleted they are using. allErrs = multierr.Combine(allErrs, r.packageManager.Cleanup(ctx)) allErrs = multierr.Combine(allErrs, r.localPackages.Cleanup(ctx)) // Cleanup extra dirs from previous modules or rogue scripts. allErrs = multierr.Combine(allErrs, r.manager.moduleManager.CleanModuleDataDirectory()) + if allErrs != nil { - r.logger.CDebugw(ctx, "The following errors were gathered during (re)configuration success cleanup", "errors", allErrs) + r.logger.CErrorw(ctx, "The following errors were gathered during reconfiguration", "errors", allErrs) } else { - r.configCleanedUp = true - r.Logger().CDebug(ctx, "(re)configuration success cleanup successful") + r.logger.CInfow(ctx, "Robot (re)configured") } } From c4ff5de274544c22d99a8691b5248fd07a539cd9 Mon Sep 17 00:00:00 2001 From: Cheuk Tse Date: Tue, 3 Sep 2024 16:45:14 -0400 Subject: [PATCH 06/19] remove unused lock --- robot/impl/local_robot.go | 4 ---- 1 file changed, 4 deletions(-) diff --git a/robot/impl/local_robot.go b/robot/impl/local_robot.go index 9b0508ced6e..6588201f80d 100644 --- a/robot/impl/local_robot.go +++ b/robot/impl/local_robot.go @@ -58,10 +58,6 @@ type localRobot struct { logger logging.Logger activeBackgroundWorkers sync.WaitGroup - // configCleanedUp tracks whether the robot has cleaned up after a successful configuration - configCleanUpMu sync.Mutex - configCleanedUp bool - // reconfigureWorkers tracks goroutines spawned by reconfiguration functions. we only // wait on this group in tests to prevent goleak-related failures. however, we do not // wait on this group outside of testing, since the related goroutines may be running From 3c3786fef3d4acaab33d058180acc27855094c5e Mon Sep 17 00:00:00 2001 From: Cheuk Tse Date: Tue, 3 Sep 2024 16:54:14 -0400 Subject: [PATCH 07/19] lint --- robot/impl/local_robot.go | 1 - 1 file changed, 1 deletion(-) diff --git a/robot/impl/local_robot.go b/robot/impl/local_robot.go index 6588201f80d..198d0c0033a 100644 --- a/robot/impl/local_robot.go +++ b/robot/impl/local_robot.go @@ -57,7 +57,6 @@ type localRobot struct { cloudConnSvc icloud.ConnectionService logger logging.Logger activeBackgroundWorkers sync.WaitGroup - // reconfigureWorkers tracks goroutines spawned by reconfiguration functions. we only // wait on this group in tests to prevent goleak-related failures. however, we do not // wait on this group outside of testing, since the related goroutines may be running From 995c92534e8dfe23bb4d11294c299577f26439de Mon Sep 17 00:00:00 2001 From: Cheuk Tse Date: Tue, 3 Sep 2024 17:03:54 -0400 Subject: [PATCH 08/19] lint --- config/config.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/config/config.go b/config/config.go index dfde698f860..7dd0bc006fe 100644 --- a/config/config.go +++ b/config/config.go @@ -246,8 +246,8 @@ func (c Config) FindComponent(name string) *resource.Config { // setUnprocessedConfig sets unprocessedConfig with a copy of the config passed in. func (c *Config) setUnprocessedConfig(cfg *Config) error { - copy, err := cfg.CopyOnlyPublicFields() - c.unprocessedConfig = copy + cpy, err := cfg.CopyOnlyPublicFields() + c.unprocessedConfig = cpy return err } From 7f87e2923137c35a16a28588fd03bad380afd33f Mon Sep 17 00:00:00 2001 From: Cheuk Tse Date: Tue, 3 Sep 2024 17:21:04 -0400 Subject: [PATCH 09/19] fix tests --- config/reader_test.go | 54 +++++++++++++++++++++---------------------- 1 file changed, 26 insertions(+), 28 deletions(-) diff --git a/config/reader_test.go b/config/reader_test.go index f04a0fb52c0..44234fee939 100644 --- a/config/reader_test.go +++ b/config/reader_test.go @@ -1,10 +1,7 @@ package config import ( - "bytes" "context" - "encoding/json" - "errors" "fmt" "io/fs" "os" @@ -13,9 +10,9 @@ import ( "time" "github.com/google/uuid" + "github.com/pkg/errors" pb "go.viam.com/api/app/v1" "go.viam.com/test" - "go.viam.com/utils/artifact" "go.viam.com/rdk/config/testutils" "go.viam.com/rdk/logging" @@ -23,22 +20,6 @@ import ( "go.viam.com/rdk/services/shell" ) -func storeToCache(id string, cfg *Config) error { - if err := os.MkdirAll(ViamDotDir, 0o700); err != nil { - return err - } - - md, err := json.MarshalIndent(cfg, "", " ") - if err != nil { - return err - } - reader := bytes.NewReader(md) - - path := getCloudCacheFilePath(id) - - return artifact.AtomicStore(path, reader, id) -} - func TestFromReader(t *testing.T) { const ( robotPartID = "forCachingTest" @@ -88,7 +69,6 @@ func TestFromReader(t *testing.T) { appAddress := fmt.Sprintf("http://%s", fakeServer.Addr().String()) cfgText := fmt.Sprintf(`{"cloud":{"id":%q,"app_address":%q,"secret":%q}}`, robotPartID, appAddress, secret) gotCfg, err := FromReader(ctx, "", strings.NewReader(cfgText), logger) - defer clearCache(robotPartID) test.That(t, err, test.ShouldBeNil) expectedCloud := *cloudResponse @@ -98,6 +78,8 @@ func TestFromReader(t *testing.T) { expectedCloud.RefreshInterval = time.Duration(10000000000) test.That(t, gotCfg.Cloud, test.ShouldResemble, &expectedCloud) + test.That(t, gotCfg.StoreToCache(), test.ShouldBeNil) + defer clearCache(robotPartID) cachedCfg, err := readFromCache(robotPartID) test.That(t, err, test.ShouldBeNil) expectedCloud.AppAddress = "" @@ -121,7 +103,10 @@ func TestFromReader(t *testing.T) { MachineID: "the-machine", } cachedConf := &Config{Cloud: cachedCloud} - err := storeToCache(robotPartID, cachedConf) + + cfgToCache := &Config{Cloud: &Cloud{ID: robotPartID}} + cfgToCache.setUnprocessedConfig(cachedConf) + err := cfgToCache.StoreToCache() test.That(t, err, test.ShouldBeNil) defer clearCache(robotPartID) @@ -172,7 +157,6 @@ func TestFromReader(t *testing.T) { appAddress := fmt.Sprintf("http://%s", fakeServer.Addr().String()) cfgText := fmt.Sprintf(`{"cloud":{"id":%q,"app_address":%q,"secret":%q}}`, robotPartID, appAddress, secret) gotCfg, err := FromReader(ctx, "", strings.NewReader(cfgText), logger) - defer clearCache(robotPartID) test.That(t, err, test.ShouldBeNil) expectedCloud := *cloudResponse @@ -180,6 +164,9 @@ func TestFromReader(t *testing.T) { expectedCloud.RefreshInterval = time.Duration(10000000000) test.That(t, gotCfg.Cloud, test.ShouldResemble, &expectedCloud) + err = gotCfg.StoreToCache() + defer clearCache(robotPartID) + test.That(t, err, test.ShouldBeNil) cachedCfg, err := readFromCache(robotPartID) test.That(t, err, test.ShouldBeNil) expectedCloud.AppAddress = "" @@ -210,7 +197,7 @@ func TestStoreToCache(t *testing.T) { } cfg.Cloud = cloud - // store our config to the cloud + // store our config to the cache cfgToCache := &Config{Cloud: &Cloud{ID: "forCachingTest"}} cfgToCache.setUnprocessedConfig(cfg) err = cfgToCache.StoreToCache() @@ -329,7 +316,9 @@ func TestReadTLSFromCache(t *testing.T) { defer clearCache(robotPartID) cfg.Cloud = nil - err = storeToCache(robotPartID, cfg) + cfgToCache := &Config{Cloud: &Cloud{ID: robotPartID}} + cfgToCache.setUnprocessedConfig(cfg) + err = cfgToCache.StoreToCache() test.That(t, err, test.ShouldBeNil) tls := tlsConfig{} @@ -340,11 +329,14 @@ func TestReadTLSFromCache(t *testing.T) { t.Run("invalid cached TLS", func(t *testing.T) { defer clearCache(robotPartID) cloud := &Cloud{ + ID: robotPartID, TLSPrivateKey: "key", } cfg.Cloud = cloud - err = storeToCache(robotPartID, cfg) + cfgToCache := &Config{Cloud: &Cloud{ID: robotPartID}} + cfgToCache.setUnprocessedConfig(cfg) + err = cfgToCache.StoreToCache() test.That(t, err, test.ShouldBeNil) tls := tlsConfig{} @@ -358,12 +350,15 @@ func TestReadTLSFromCache(t *testing.T) { t.Run("invalid cached TLS but insecure signaling", func(t *testing.T) { defer clearCache(robotPartID) cloud := &Cloud{ + ID: robotPartID, TLSPrivateKey: "key", SignalingInsecure: true, } cfg.Cloud = cloud - err = storeToCache(robotPartID, cfg) + cfgToCache := &Config{Cloud: &Cloud{ID: robotPartID}} + cfgToCache.setUnprocessedConfig(cfg) + err = cfgToCache.StoreToCache() test.That(t, err, test.ShouldBeNil) tls := tlsConfig{} @@ -377,12 +372,15 @@ func TestReadTLSFromCache(t *testing.T) { t.Run("valid cached TLS", func(t *testing.T) { defer clearCache(robotPartID) cloud := &Cloud{ + ID: robotPartID, TLSCertificate: "cert", TLSPrivateKey: "key", } cfg.Cloud = cloud - err = storeToCache(robotPartID, cfg) + cfgToCache := &Config{Cloud: &Cloud{ID: robotPartID}} + cfgToCache.setUnprocessedConfig(cfg) + err = cfgToCache.StoreToCache() test.That(t, err, test.ShouldBeNil) // the config is missing several fields required to start the robot, but this From cf5a6d240839f00b750704b85736436b7e0e841c Mon Sep 17 00:00:00 2001 From: Cheuk Tse Date: Wed, 4 Sep 2024 10:53:41 -0400 Subject: [PATCH 10/19] fix --- config/config.go | 10 ++++------ robot/impl/local_robot.go | 8 +++++--- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/config/config.go b/config/config.go index 7dd0bc006fe..a7e04abac39 100644 --- a/config/config.go +++ b/config/config.go @@ -251,17 +251,15 @@ func (c *Config) setUnprocessedConfig(cfg *Config) error { return err } -// UnprocessedConfig returns unprocessedConfig. -func (c *Config) UnprocessedConfig() *Config { - return c.unprocessedConfig -} - // StoreToCache caches the unprocessedConfig. func (c *Config) StoreToCache() error { + if c.unprocessedConfig == nil { + return errors.New("no unprocessed config to cache") + } if err := os.MkdirAll(ViamDotDir, 0o700); err != nil { return err } - md, err := json.MarshalIndent(c.UnprocessedConfig(), "", " ") + md, err := json.MarshalIndent(c.unprocessedConfig, "", " ") if err != nil { return err } diff --git a/robot/impl/local_robot.go b/robot/impl/local_robot.go index 198d0c0033a..243286fe1de 100644 --- a/robot/impl/local_robot.go +++ b/robot/impl/local_robot.go @@ -1197,9 +1197,11 @@ func (r *localRobot) reconfigure(ctx context.Context, newConfig *config.Config, return } - r.Logger().CDebug(ctx, "updating cached config") - if err := newConfig.StoreToCache(); err != nil { - r.logger.CErrorw(ctx, "error storing the config", "error", err) + if newConfig.Cloud != nil { + r.Logger().CDebug(ctx, "updating cached config") + if err := newConfig.StoreToCache(); err != nil { + r.logger.CErrorw(ctx, "error storing the config", "error", err) + } } // Update configRevision, as the robot is starting to reconfigure itself. From c9aad452f37ec9b7209b0a33f1c79b0e2706e9b7 Mon Sep 17 00:00:00 2001 From: Cheuk Tse Date: Wed, 4 Sep 2024 10:56:14 -0400 Subject: [PATCH 11/19] add --- config/reader_test.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/config/reader_test.go b/config/reader_test.go index 44234fee939..2164d3ca486 100644 --- a/config/reader_test.go +++ b/config/reader_test.go @@ -197,8 +197,12 @@ func TestStoreToCache(t *testing.T) { } cfg.Cloud = cloud - // store our config to the cache + // errors if no unprocessed config to cache cfgToCache := &Config{Cloud: &Cloud{ID: "forCachingTest"}} + err = cfgToCache.StoreToCache() + test.That(t, err.Error(), test.ShouldContainSubstring, "no unprocessed config to cache") + + // store our config to the cache cfgToCache.setUnprocessedConfig(cfg) err = cfgToCache.StoreToCache() test.That(t, err, test.ShouldBeNil) From 3b08396d64a784eed25459b5df1aa236827990bb Mon Sep 17 00:00:00 2001 From: Cheuk Tse Date: Wed, 4 Sep 2024 12:29:00 -0400 Subject: [PATCH 12/19] make SetUnprocessedConfig public --- config/config.go | 4 ++-- config/reader.go | 2 +- config/reader_test.go | 14 +++++++------- 3 files changed, 10 insertions(+), 10 deletions(-) diff --git a/config/config.go b/config/config.go index a7e04abac39..e89034493f1 100644 --- a/config/config.go +++ b/config/config.go @@ -244,8 +244,8 @@ func (c Config) FindComponent(name string) *resource.Config { return nil } -// setUnprocessedConfig sets unprocessedConfig with a copy of the config passed in. -func (c *Config) setUnprocessedConfig(cfg *Config) error { +// SetUnprocessedConfig sets unprocessedConfig with a copy of the config passed in. +func (c *Config) SetUnprocessedConfig(cfg *Config) error { cpy, err := cfg.CopyOnlyPublicFields() c.unprocessedConfig = cpy return err diff --git a/config/reader.go b/config/reader.go index 393a527ba3e..c94dce9f53a 100644 --- a/config/reader.go +++ b/config/reader.go @@ -301,7 +301,7 @@ func readFromCloud( unprocessedConfig.Cloud.TLSCertificate = tls.certificate unprocessedConfig.Cloud.TLSPrivateKey = tls.privateKey - if err := cfg.setUnprocessedConfig(unprocessedConfig); err != nil { + if err := cfg.SetUnprocessedConfig(unprocessedConfig); err != nil { logger.Errorw("failed to set unprocessed config", "error", err) } return cfg, nil diff --git a/config/reader_test.go b/config/reader_test.go index 2164d3ca486..1c8cd32c04a 100644 --- a/config/reader_test.go +++ b/config/reader_test.go @@ -105,7 +105,7 @@ func TestFromReader(t *testing.T) { cachedConf := &Config{Cloud: cachedCloud} cfgToCache := &Config{Cloud: &Cloud{ID: robotPartID}} - cfgToCache.setUnprocessedConfig(cachedConf) + cfgToCache.SetUnprocessedConfig(cachedConf) err := cfgToCache.StoreToCache() test.That(t, err, test.ShouldBeNil) defer clearCache(robotPartID) @@ -203,7 +203,7 @@ func TestStoreToCache(t *testing.T) { test.That(t, err.Error(), test.ShouldContainSubstring, "no unprocessed config to cache") // store our config to the cache - cfgToCache.setUnprocessedConfig(cfg) + cfgToCache.SetUnprocessedConfig(cfg) err = cfgToCache.StoreToCache() test.That(t, err, test.ShouldBeNil) @@ -224,7 +224,7 @@ func TestStoreToCache(t *testing.T) { test.That(t, cloudCfg2, test.ShouldNotResemble, cfgToCache) // store the updated config to the cloud - cfgToCache.setUnprocessedConfig(cfg) + cfgToCache.SetUnprocessedConfig(cfg) err = cfgToCache.StoreToCache() test.That(t, err, test.ShouldBeNil) @@ -321,7 +321,7 @@ func TestReadTLSFromCache(t *testing.T) { cfg.Cloud = nil cfgToCache := &Config{Cloud: &Cloud{ID: robotPartID}} - cfgToCache.setUnprocessedConfig(cfg) + cfgToCache.SetUnprocessedConfig(cfg) err = cfgToCache.StoreToCache() test.That(t, err, test.ShouldBeNil) @@ -339,7 +339,7 @@ func TestReadTLSFromCache(t *testing.T) { cfg.Cloud = cloud cfgToCache := &Config{Cloud: &Cloud{ID: robotPartID}} - cfgToCache.setUnprocessedConfig(cfg) + cfgToCache.SetUnprocessedConfig(cfg) err = cfgToCache.StoreToCache() test.That(t, err, test.ShouldBeNil) @@ -361,7 +361,7 @@ func TestReadTLSFromCache(t *testing.T) { cfg.Cloud = cloud cfgToCache := &Config{Cloud: &Cloud{ID: robotPartID}} - cfgToCache.setUnprocessedConfig(cfg) + cfgToCache.SetUnprocessedConfig(cfg) err = cfgToCache.StoreToCache() test.That(t, err, test.ShouldBeNil) @@ -383,7 +383,7 @@ func TestReadTLSFromCache(t *testing.T) { cfg.Cloud = cloud cfgToCache := &Config{Cloud: &Cloud{ID: robotPartID}} - cfgToCache.setUnprocessedConfig(cfg) + cfgToCache.SetUnprocessedConfig(cfg) err = cfgToCache.StoreToCache() test.That(t, err, test.ShouldBeNil) From 5892f145bbd1777879d3c1b099be6782b56a5dbd Mon Sep 17 00:00:00 2001 From: Cheuk Tse Date: Wed, 4 Sep 2024 14:05:41 -0400 Subject: [PATCH 13/19] fix tests --- config/watcher_test.go | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/config/watcher_test.go b/config/watcher_test.go index 3cb7df459a0..43a211f09fc 100644 --- a/config/watcher_test.go +++ b/config/watcher_test.go @@ -259,6 +259,16 @@ func TestNewWatcherCloud(t *testing.T) { }}, } + unprocessedFromCfg := func(cfg config.Config) *config.Config { + // the unprocessed config uses the original config read from the cloud, + // and the cloud config is missing a few fields in the proto, meaning a few fields need to be cleared out. + unprocessed, err := cfg.CopyOnlyPublicFields() + test.That(t, err, test.ShouldBeNil) + unprocessed.Cloud.AppAddress = "" + unprocessed.Cloud.RefreshInterval = 10 * time.Second + return unprocessed + } + storeConfigInServer(confToReturn) watcher, err := config.NewWatcher(context.Background(), &config.Config{Cloud: newCloudConf()}, logger) @@ -268,6 +278,7 @@ func TestNewWatcherCloud(t *testing.T) { confToExpect.Cloud.TLSCertificate = certsToReturn.TLSCertificate confToExpect.Cloud.TLSPrivateKey = certsToReturn.TLSPrivateKey test.That(t, confToExpect.Ensure(true, logger), test.ShouldBeNil) + confToExpect.SetUnprocessedConfig(unprocessedFromCfg(confToExpect)) newConf := <-watcher.Config() test.That(t, newConf, test.ShouldResemble, &confToExpect) @@ -305,6 +316,7 @@ func TestNewWatcherCloud(t *testing.T) { confToExpect.Cloud.TLSCertificate = certsToReturn.TLSCertificate confToExpect.Cloud.TLSPrivateKey = certsToReturn.TLSPrivateKey test.That(t, confToExpect.Ensure(true, logger), test.ShouldBeNil) + confToExpect.SetUnprocessedConfig(unprocessedFromCfg(confToExpect)) newConf = <-watcher.Config() test.That(t, newConf, test.ShouldResemble, &confToExpect) @@ -356,6 +368,7 @@ func TestNewWatcherCloud(t *testing.T) { confToExpect.Cloud.TLSCertificate = certsToReturn.TLSCertificate confToExpect.Cloud.TLSPrivateKey = certsToReturn.TLSPrivateKey test.That(t, confToExpect.Ensure(true, logger), test.ShouldBeNil) + confToExpect.SetUnprocessedConfig(unprocessedFromCfg(confToExpect)) newConf = <-watcher.Config() test.That(t, newConf, test.ShouldResemble, &confToExpect) From 9105fd23c4302f9e46c4fec5e6b19b4688074921 Mon Sep 17 00:00:00 2001 From: Cheuk Tse Date: Wed, 4 Sep 2024 15:25:00 -0400 Subject: [PATCH 14/19] add locks --- robot/impl/local_robot.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/robot/impl/local_robot.go b/robot/impl/local_robot.go index 243286fe1de..dab86cb9ecf 100644 --- a/robot/impl/local_robot.go +++ b/robot/impl/local_robot.go @@ -1205,10 +1205,12 @@ func (r *localRobot) reconfigure(ctx context.Context, newConfig *config.Config, } // Update configRevision, as the robot is starting to reconfigure itself. + r.configRevisionMu.Lock() r.configRevision = config.Revision{ Revision: newConfig.Revision, LastUpdated: time.Now(), } + r.configRevisionMu.Unlock() // Add default services and process their dependencies. Dependencies may // already come from config validation so we check that here. From 43f47567b44f620492ebc5533beb4f996e574e76 Mon Sep 17 00:00:00 2001 From: Cheuk Tse Date: Wed, 4 Sep 2024 15:39:58 -0400 Subject: [PATCH 15/19] marshal earlier --- config/config.go | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/config/config.go b/config/config.go index e89034493f1..a9acfc2f452 100644 --- a/config/config.go +++ b/config/config.go @@ -71,9 +71,9 @@ type Config struct { // Revision contains the current revision of the config. Revision string - // unprocessedConfig stores the unprocessed version of the config that will be cached. + // unprocessedConfig stores the JSON marshalled unprocessed version of the config that will be cached. // This version is kept because the config is changed as it moves through the system. - unprocessedConfig *Config + unprocessedConfig []byte } // NOTE: This data must be maintained with what is in Config. @@ -246,9 +246,12 @@ func (c Config) FindComponent(name string) *resource.Config { // SetUnprocessedConfig sets unprocessedConfig with a copy of the config passed in. func (c *Config) SetUnprocessedConfig(cfg *Config) error { - cpy, err := cfg.CopyOnlyPublicFields() - c.unprocessedConfig = cpy - return err + md, err := json.MarshalIndent(cfg, "", " ") + if err != nil { + return err + } + c.unprocessedConfig = md + return nil } // StoreToCache caches the unprocessedConfig. @@ -259,11 +262,7 @@ func (c *Config) StoreToCache() error { if err := os.MkdirAll(ViamDotDir, 0o700); err != nil { return err } - md, err := json.MarshalIndent(c.unprocessedConfig, "", " ") - if err != nil { - return err - } - reader := bytes.NewReader(md) + reader := bytes.NewReader(c.unprocessedConfig) path := getCloudCacheFilePath(c.Cloud.ID) return artifact.AtomicStore(path, reader, c.Cloud.ID) } From b022e92e494a317da181803cd55cb0214234b2c9 Mon Sep 17 00:00:00 2001 From: Cheuk Tse Date: Wed, 4 Sep 2024 16:00:14 -0400 Subject: [PATCH 16/19] move revision up --- robot/impl/local_robot.go | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/robot/impl/local_robot.go b/robot/impl/local_robot.go index dab86cb9ecf..431aab4b6c2 100644 --- a/robot/impl/local_robot.go +++ b/robot/impl/local_robot.go @@ -1174,6 +1174,13 @@ func (r *localRobot) applyLocalModuleVersions(cfg *config.Config) { } func (r *localRobot) reconfigure(ctx context.Context, newConfig *config.Config, forceSync bool) { + r.configRevisionMu.Lock() + r.configRevision = config.Revision{ + Revision: newConfig.Revision, + LastUpdated: time.Now(), + } + r.configRevisionMu.Unlock() + var allErrs error // Sync Packages before reconfiguring rest of robot and resolving references to any packages @@ -1204,14 +1211,6 @@ func (r *localRobot) reconfigure(ctx context.Context, newConfig *config.Config, } } - // Update configRevision, as the robot is starting to reconfigure itself. - r.configRevisionMu.Lock() - r.configRevision = config.Revision{ - Revision: newConfig.Revision, - LastUpdated: time.Now(), - } - r.configRevisionMu.Unlock() - // Add default services and process their dependencies. Dependencies may // already come from config validation so we check that here. seen := make(map[resource.API]int) From b219be0ad7d947e3350a92df9ffb5629db34c073 Mon Sep 17 00:00:00 2001 From: Cheuk Tse Date: Wed, 4 Sep 2024 16:09:38 -0400 Subject: [PATCH 17/19] pr comments --- config/config.go | 17 +++++++++-------- config/reader.go | 2 +- config/reader_test.go | 14 +++++++------- config/watcher_test.go | 6 +++--- 4 files changed, 20 insertions(+), 19 deletions(-) diff --git a/config/config.go b/config/config.go index a9acfc2f452..ca708883684 100644 --- a/config/config.go +++ b/config/config.go @@ -71,9 +71,10 @@ type Config struct { // Revision contains the current revision of the config. Revision string - // unprocessedConfig stores the JSON marshalled unprocessed version of the config that will be cached. + // toCache stores the JSON marshalled version of the config to be cached. It should be a copy of + // the config pulled from cloud with minor changes. // This version is kept because the config is changed as it moves through the system. - unprocessedConfig []byte + toCache []byte } // NOTE: This data must be maintained with what is in Config. @@ -244,25 +245,25 @@ func (c Config) FindComponent(name string) *resource.Config { return nil } -// SetUnprocessedConfig sets unprocessedConfig with a copy of the config passed in. -func (c *Config) SetUnprocessedConfig(cfg *Config) error { +// SetToCache sets toCache with a marshalled copy of the config passed in. +func (c *Config) SetToCache(cfg *Config) error { md, err := json.MarshalIndent(cfg, "", " ") if err != nil { return err } - c.unprocessedConfig = md + c.toCache = md return nil } -// StoreToCache caches the unprocessedConfig. +// StoreToCache caches the toCache. func (c *Config) StoreToCache() error { - if c.unprocessedConfig == nil { + if c.toCache == nil { return errors.New("no unprocessed config to cache") } if err := os.MkdirAll(ViamDotDir, 0o700); err != nil { return err } - reader := bytes.NewReader(c.unprocessedConfig) + reader := bytes.NewReader(c.toCache) path := getCloudCacheFilePath(c.Cloud.ID) return artifact.AtomicStore(path, reader, c.Cloud.ID) } diff --git a/config/reader.go b/config/reader.go index c94dce9f53a..b85e00a1b09 100644 --- a/config/reader.go +++ b/config/reader.go @@ -301,7 +301,7 @@ func readFromCloud( unprocessedConfig.Cloud.TLSCertificate = tls.certificate unprocessedConfig.Cloud.TLSPrivateKey = tls.privateKey - if err := cfg.SetUnprocessedConfig(unprocessedConfig); err != nil { + if err := cfg.SetToCache(unprocessedConfig); err != nil { logger.Errorw("failed to set unprocessed config", "error", err) } return cfg, nil diff --git a/config/reader_test.go b/config/reader_test.go index 1c8cd32c04a..f77228d21d2 100644 --- a/config/reader_test.go +++ b/config/reader_test.go @@ -105,7 +105,7 @@ func TestFromReader(t *testing.T) { cachedConf := &Config{Cloud: cachedCloud} cfgToCache := &Config{Cloud: &Cloud{ID: robotPartID}} - cfgToCache.SetUnprocessedConfig(cachedConf) + cfgToCache.SetToCache(cachedConf) err := cfgToCache.StoreToCache() test.That(t, err, test.ShouldBeNil) defer clearCache(robotPartID) @@ -203,7 +203,7 @@ func TestStoreToCache(t *testing.T) { test.That(t, err.Error(), test.ShouldContainSubstring, "no unprocessed config to cache") // store our config to the cache - cfgToCache.SetUnprocessedConfig(cfg) + cfgToCache.SetToCache(cfg) err = cfgToCache.StoreToCache() test.That(t, err, test.ShouldBeNil) @@ -224,7 +224,7 @@ func TestStoreToCache(t *testing.T) { test.That(t, cloudCfg2, test.ShouldNotResemble, cfgToCache) // store the updated config to the cloud - cfgToCache.SetUnprocessedConfig(cfg) + cfgToCache.SetToCache(cfg) err = cfgToCache.StoreToCache() test.That(t, err, test.ShouldBeNil) @@ -321,7 +321,7 @@ func TestReadTLSFromCache(t *testing.T) { cfg.Cloud = nil cfgToCache := &Config{Cloud: &Cloud{ID: robotPartID}} - cfgToCache.SetUnprocessedConfig(cfg) + cfgToCache.SetToCache(cfg) err = cfgToCache.StoreToCache() test.That(t, err, test.ShouldBeNil) @@ -339,7 +339,7 @@ func TestReadTLSFromCache(t *testing.T) { cfg.Cloud = cloud cfgToCache := &Config{Cloud: &Cloud{ID: robotPartID}} - cfgToCache.SetUnprocessedConfig(cfg) + cfgToCache.SetToCache(cfg) err = cfgToCache.StoreToCache() test.That(t, err, test.ShouldBeNil) @@ -361,7 +361,7 @@ func TestReadTLSFromCache(t *testing.T) { cfg.Cloud = cloud cfgToCache := &Config{Cloud: &Cloud{ID: robotPartID}} - cfgToCache.SetUnprocessedConfig(cfg) + cfgToCache.SetToCache(cfg) err = cfgToCache.StoreToCache() test.That(t, err, test.ShouldBeNil) @@ -383,7 +383,7 @@ func TestReadTLSFromCache(t *testing.T) { cfg.Cloud = cloud cfgToCache := &Config{Cloud: &Cloud{ID: robotPartID}} - cfgToCache.SetUnprocessedConfig(cfg) + cfgToCache.SetToCache(cfg) err = cfgToCache.StoreToCache() test.That(t, err, test.ShouldBeNil) diff --git a/config/watcher_test.go b/config/watcher_test.go index 43a211f09fc..d1fcdf9c36c 100644 --- a/config/watcher_test.go +++ b/config/watcher_test.go @@ -278,7 +278,7 @@ func TestNewWatcherCloud(t *testing.T) { confToExpect.Cloud.TLSCertificate = certsToReturn.TLSCertificate confToExpect.Cloud.TLSPrivateKey = certsToReturn.TLSPrivateKey test.That(t, confToExpect.Ensure(true, logger), test.ShouldBeNil) - confToExpect.SetUnprocessedConfig(unprocessedFromCfg(confToExpect)) + confToExpect.SetToCache(unprocessedFromCfg(confToExpect)) newConf := <-watcher.Config() test.That(t, newConf, test.ShouldResemble, &confToExpect) @@ -316,7 +316,7 @@ func TestNewWatcherCloud(t *testing.T) { confToExpect.Cloud.TLSCertificate = certsToReturn.TLSCertificate confToExpect.Cloud.TLSPrivateKey = certsToReturn.TLSPrivateKey test.That(t, confToExpect.Ensure(true, logger), test.ShouldBeNil) - confToExpect.SetUnprocessedConfig(unprocessedFromCfg(confToExpect)) + confToExpect.SetToCache(unprocessedFromCfg(confToExpect)) newConf = <-watcher.Config() test.That(t, newConf, test.ShouldResemble, &confToExpect) @@ -368,7 +368,7 @@ func TestNewWatcherCloud(t *testing.T) { confToExpect.Cloud.TLSCertificate = certsToReturn.TLSCertificate confToExpect.Cloud.TLSPrivateKey = certsToReturn.TLSPrivateKey test.That(t, confToExpect.Ensure(true, logger), test.ShouldBeNil) - confToExpect.SetUnprocessedConfig(unprocessedFromCfg(confToExpect)) + confToExpect.SetToCache(unprocessedFromCfg(confToExpect)) newConf = <-watcher.Config() test.That(t, newConf, test.ShouldResemble, &confToExpect) From f61dfcbbb9f2e2113a9e980152aa5b4d4acf0e66 Mon Sep 17 00:00:00 2001 From: Cheuk Tse Date: Wed, 4 Sep 2024 16:18:23 -0400 Subject: [PATCH 18/19] update error --- config/reader.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/config/reader.go b/config/reader.go index b85e00a1b09..b0452a40b7a 100644 --- a/config/reader.go +++ b/config/reader.go @@ -302,7 +302,7 @@ func readFromCloud( unprocessedConfig.Cloud.TLSPrivateKey = tls.privateKey if err := cfg.SetToCache(unprocessedConfig); err != nil { - logger.Errorw("failed to set unprocessed config", "error", err) + logger.Errorw("failed to set toCache on config", "error", err) } return cfg, nil } From ebc3f8b307518483eb9c2c212a01a8fa1c56a2f1 Mon Sep 17 00:00:00 2001 From: Cheuk Tse Date: Wed, 4 Sep 2024 17:01:51 -0400 Subject: [PATCH 19/19] lint --- config/reader_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/config/reader_test.go b/config/reader_test.go index f77228d21d2..570d7fcb5d8 100644 --- a/config/reader_test.go +++ b/config/reader_test.go @@ -210,7 +210,7 @@ func TestStoreToCache(t *testing.T) { // read config from cloud, confirm consistency cloudCfg, err := readFromCloud(ctx, cfg, nil, true, false, logger) test.That(t, err, test.ShouldBeNil) - cloudCfg.unprocessedConfig = nil + cloudCfg.toCache = nil test.That(t, cloudCfg, test.ShouldResemble, cfg) // Modify our config @@ -220,7 +220,7 @@ func TestStoreToCache(t *testing.T) { // read config from cloud again, confirm that the cached config differs from cfg cloudCfg2, err := readFromCloud(ctx, cfg, nil, true, false, logger) test.That(t, err, test.ShouldBeNil) - cloudCfg2.unprocessedConfig = nil + cloudCfg2.toCache = nil test.That(t, cloudCfg2, test.ShouldNotResemble, cfgToCache) // store the updated config to the cloud @@ -233,7 +233,7 @@ func TestStoreToCache(t *testing.T) { // read updated cloud config, confirm that it now matches our updated cfg cloudCfg3, err := readFromCloud(ctx, cfg, nil, true, false, logger) test.That(t, err, test.ShouldBeNil) - cloudCfg3.unprocessedConfig = nil + cloudCfg3.toCache = nil test.That(t, cloudCfg3, test.ShouldResemble, cfg) }