-
Notifications
You must be signed in to change notification settings - Fork 110
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
RSDK-8598 - Replace cache after sync #4343
Changes from 17 commits
9bcade1
a4b7971
7d7b3b5
c75c474
e9a5d8e
e984c89
d8c478f
63f812b
c4ff5de
3c3786f
995c925
7f87e29
cf5a6d2
c9aad45
3b08396
5892f14
9105fd2
43f4756
b022e92
b219be0
f61dfcb
ebc3f8b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 sets unprocessedConfig with a copy of the config passed in. | ||
func (c *Config) SetUnprocessedConfig(cfg *Config) error { | ||
cpy, err := cfg.CopyOnlyPublicFields() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm assuming Having There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yep, it's the same. definitely could marshal earlier as well, went ahead and did that |
||
c.unprocessedConfig = cpy | ||
return err | ||
} | ||
|
||
// 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, "", " ") | ||
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 { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1174,13 +1174,6 @@ func (r *localRobot) applyLocalModuleVersions(cfg *config.Config) { | |
} | ||
|
||
func (r *localRobot) reconfigure(ctx context.Context, newConfig *config.Config, forceSync bool) { | ||
r.configRevisionMu.Lock() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is the consequence here that we now only report a newer config revision after* package downloading succeeds? Also I don't see the configRevisionMu being locked. Is that important? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the missing lock is a miss, added it back in. thinking about it more, the config revision should update at the top of the function so users will know the new config did get pulled and is being processed. In theory, it may make sense to rollback the revision if we exit reconfiguration if download fails, but I also think it's ok to keep the new revision as a sign that the new config did get loaded. whether we revert the revision or not, it will be unclear what state robot reconfiguration is in, so maybe we need to add more information in there There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I know @maximpertsov and I talked about how to represent "mixed" state when a new revision is being applied/doesn't apply completely.
I agree the only path to do better is to add more information. And I agree it's best to do that thinking/work in a separate ticket. |
||
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,14 +1183,34 @@ 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 { | ||
allErrs = multierr.Combine(allErrs, err) | ||
r.Logger().CErrorw(ctx, "reconfiguration aborted because cloud modules or packages download failed", "error", err) | ||
return | ||
} | ||
// 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 | ||
} | ||
|
||
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. | ||
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. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[minor] maybe we can shorten this field name - conceptually an "unprocessed" config is the marshaled config struct, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed the name to
toCache
to be clearer, what do you think?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep makes sense