Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

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

Refactor of the value cache #2151

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
+369 −161
Open

Refactor of the value cache #2151

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
924df73
Merge scope hierarchy for evaluation
wildum Nov 25, 2024
cc9c0a3
get rid of parent scope
wildum Nov 25, 2024
00bd528
refactor value_cache
wildum Nov 26, 2024
8bc2094
review feedback
wildum Dec 9, 2024
1aa5bf8
review feedback
wildum Jan 7, 2025
4dcc8ee
make post evaluation errors more explicit
wildum Jan 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 31 additions & 14 deletions internal/runtime/internal/controller/loader.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,9 @@ func (l *Loader) Apply(options ApplyOptions) diag.Diagnostics {
l.cm.controllerEvaluation.Set(1)
defer l.cm.controllerEvaluation.Set(0)

l.cache.SetScope(options.ArgScope)
if options.ArgScope != nil {
l.cache.UpdateScopeVariables(options.ArgScope.Variables)
}

for key, value := range options.Args {
l.cache.CacheModuleArgument(key, value)
Expand All @@ -166,7 +168,7 @@ func (l *Loader) Apply(options ApplyOptions) diag.Diagnostics {

var (
components = make([]ComponentNode, 0)
componentIDs = make([]ComponentID, 0)
componentIDs = make(map[string]ComponentID)
wildum marked this conversation as resolved.
Show resolved Hide resolved
services = make([]*ServiceNode, 0, len(l.services))
)

Expand Down Expand Up @@ -200,7 +202,7 @@ func (l *Loader) Apply(options ApplyOptions) diag.Diagnostics {
switch n := n.(type) {
case ComponentNode:
components = append(components, n)
componentIDs = append(componentIDs, n.ID())
componentIDs[n.ID().String()] = n.ID()

if err = l.evaluate(logger, n); err != nil {
var evalDiags diag.Diagnostics
Expand Down Expand Up @@ -260,7 +262,13 @@ func (l *Loader) Apply(options ApplyOptions) diag.Diagnostics {
l.componentNodes = components
l.serviceNodes = services
l.graph = &newGraph
l.cache.SyncIDs(componentIDs)
err := l.cache.SyncIDs(componentIDs)
if err != nil {
wildum marked this conversation as resolved.
Show resolved Hide resolved
diags.Add(diag.Diagnostic{
Severity: diag.SeverityLevelError,
Message: fmt.Sprintf("Failed to update the internal cache with the new list of components: %s", err),
})
}
l.blocks = options.ComponentBlocks
if l.globals.OnExportsChange != nil && l.cache.ExportChangeIndex() != l.moduleExportIndex {
l.moduleExportIndex = l.cache.ExportChangeIndex()
Expand Down Expand Up @@ -615,7 +623,7 @@ func (l *Loader) wireGraphEdges(g *dag.Graph) diag.Diagnostics {

// Finally, wire component references.
l.cache.mut.RLock()
refs, nodeDiags := ComponentReferences(n, g, l.log, l.cache.scope, l.globals.MinStability)
refs, nodeDiags := ComponentReferences(n, g, l.log, l.cache.GetScope(), l.globals.MinStability)
l.cache.mut.RUnlock()
for _, ref := range refs {
g.AddEdge(dag.Edge{From: n, To: ref.Target})
Expand Down Expand Up @@ -645,7 +653,7 @@ func (l *Loader) wireCustomComponentNode(g *dag.Graph, cc *CustomComponentNode)
// Variables returns the Variables the Loader exposes for other components to
// reference.
func (l *Loader) Variables() map[string]interface{} {
return l.cache.BuildContext().Variables
return l.cache.GetScope().Variables
}

// Components returns the current set of loaded components.
Expand Down Expand Up @@ -703,7 +711,10 @@ func (l *Loader) EvaluateDependants(ctx context.Context, updatedNodes []*QueuedN
switch parentNode := parent.Node.(type) {
case ComponentNode:
// Make sure we're in-sync with the current exports of parent.
l.cache.CacheExports(parentNode.ID(), parentNode.Exports())
err := l.cache.CacheExports(parentNode.ID(), parentNode.Exports())
if err != nil {
level.Error(l.log).Log("msg", "failed to cache exports during evaluation", "err", err)
}
case *ImportConfigNode:
// Update the scope with the imported content.
l.componentNodeManager.customComponentReg.updateImportContent(parentNode)
Expand Down Expand Up @@ -780,7 +791,7 @@ func (l *Loader) concurrentEvalFn(n dag.Node, spanCtx context.Context, tracer tr
var err error
switch n := n.(type) {
case BlockNode:
ectx := l.cache.BuildContext()
ectx := l.cache.GetScope()

// RLock before evaluate to prevent Evaluating while the config is being reloaded
l.mut.RLock()
Expand Down Expand Up @@ -819,7 +830,7 @@ func (l *Loader) concurrentEvalFn(n dag.Node, spanCtx context.Context, tracer tr
// evaluate constructs the final context for the BlockNode and
// evaluates it. mut must be held when calling evaluate.
func (l *Loader) evaluate(logger log.Logger, bn BlockNode) error {
ectx := l.cache.BuildContext()
ectx := l.cache.GetScope()
err := bn.Evaluate(ectx)
return l.postEvaluate(logger, bn, err)
}
Expand All @@ -829,12 +840,18 @@ func (l *Loader) evaluate(logger log.Logger, bn BlockNode) error {
func (l *Loader) postEvaluate(logger log.Logger, bn BlockNode, err error) error {
switch c := bn.(type) {
case ComponentNode:
// Always update the cache both the arguments and exports, since both might
// change when a component gets re-evaluated. We also want to cache the arguments and exports in case of an error
l.cache.CacheArguments(c.ID(), c.Arguments())
l.cache.CacheExports(c.ID(), c.Exports())
// Always update the cache both the exports, since that it might change when a component gets re-evaluated.
// We also want to cache it in case of an error
err2 := l.cache.CacheExports(c.ID(), c.Exports())
if err2 != nil {
level.Error(logger).Log("msg", "failed to cache exports after evaluation", "err", err2)
// Don't mask the previous evaluation error.
wildum marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we join errors here so we don't lose anything?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that it might be confusing because these are two different errors. The evaluation error is probably the most important here so it gets returned via the diag to be nicely printed and the other one is just logged as an additional error.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the likelihood of errors in CachedExports is low, but if we do have them it would be important to go and fix them as they likely mean there's a proper bug instead of a user error. I'll leave it up to you.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I used the join and handled the cases separately to be more explicit.

if err == nil {
return err2
}
}
case *ArgumentConfigNode:
if _, found := l.cache.moduleArguments[c.Label()]; !found {
if _, found := l.cache.GetModuleArgument(c.Label()); !found {
if c.Optional() {
l.cache.CacheModuleArgument(c.Label(), c.Default())
} else {
Expand Down
Loading