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.

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

Dag for local data eval #13155

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 4 additions & 1 deletion command/build.go
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,9 @@ func (c *BuildCommand) RunContext(buildCtx context.Context, cla *BuildArgs) int
return ret
}

diags = packerStarter.Initialize(packer.InitializeOptions{})
diags = packerStarter.Initialize(packer.InitializeOptions{
UseSequential: cla.UseSequential,
})
ret = writeDiags(c.Ui, nil, diags)
if ret != 0 {
return ret
Expand Down Expand Up @@ -439,6 +441,7 @@ Options:
-var-file=path JSON or HCL2 file containing user variables, can be used multiple times.
-warn-on-undeclared-var Display warnings for user variable files containing undeclared variables.
-ignore-prerelease-plugins Disable the loading of prerelease plugin binaries (x.y.z-dev).
-use-sequential-evaluation Fallback to using a sequential approach for local/datasource evaluation.
`

return strings.TrimSpace(helpText)
Expand Down
10 changes: 10 additions & 0 deletions command/cli.go
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,12 @@ type MetaArgs struct {
// WarnOnUndeclared does not have a common default, as the default varies per sub-command usage.
// Refer to individual command FlagSets for usage.
WarnOnUndeclaredVar bool
// UseSequential specifies to use a sequential/phased approach for
// evaluating datasources/locals instead of a DAG.
//
// This allows users to fall-back to using the approach used by Packer
// before the introduction of a DAG in case they run in an impasse/bug.
UseSequential bool
}

func (ba *BuildArgs) AddFlagSets(flags *flag.FlagSet) {
Expand All @@ -90,6 +96,7 @@ func (ba *BuildArgs) AddFlagSets(flags *flag.FlagSet) {
flags.Var(flagOnError, "on-error", "")

flags.BoolVar(&ba.MetaArgs.WarnOnUndeclaredVar, "warn-on-undeclared-var", false, "Show warnings for variable files containing undeclared variables.")
flags.BoolVar(&ba.MetaArgs.UseSequential, "use-sequential-evaluation", false, "Fallback to using a sequential approach for local/datasource evaluation.")

flags.BoolVar(&ba.ReleaseOnly, "ignore-prerelease-plugins", false, "Disable the loading of prerelease plugin binaries (x.y.z-dev).")

Expand Down Expand Up @@ -156,6 +163,7 @@ type ConsoleArgs struct {

func (fa *FixArgs) AddFlagSets(flags *flag.FlagSet) {
flags.BoolVar(&fa.Validate, "validate", true, "")
flags.BoolVar(&fa.MetaArgs.UseSequential, "use-sequential-evaluation", false, "Fallback to using a sequential approach for local/datasource evaluation.")

fa.MetaArgs.AddFlagSets(flags)
}
Expand All @@ -171,6 +179,7 @@ func (va *ValidateArgs) AddFlagSets(flags *flag.FlagSet) {
flags.BoolVar(&va.NoWarnUndeclaredVar, "no-warn-undeclared-var", false, "Ignore warnings for variable files containing undeclared variables.")
flags.BoolVar(&va.EvaluateDatasources, "evaluate-datasources", false, "evaluate datasources for validation (HCL2 only, may incur costs)")
flags.BoolVar(&va.ReleaseOnly, "ignore-prerelease-plugins", false, "Disable the loading of prerelease plugin binaries (x.y.z-dev).")
flags.BoolVar(&va.MetaArgs.UseSequential, "use-sequential-evaluation", false, "Fallback to using a sequential approach for local/datasource evaluation.")

va.MetaArgs.AddFlagSets(flags)
}
Expand All @@ -184,6 +193,7 @@ type ValidateArgs struct {
}

func (va *InspectArgs) AddFlagSets(flags *flag.FlagSet) {
flags.BoolVar(&va.MetaArgs.UseSequential, "use-sequential-evaluation", false, "Fallback to using a sequential approach for local/datasource evaluation.")
va.MetaArgs.AddFlagSets(flags)
}

Expand Down
11 changes: 7 additions & 4 deletions command/console.go
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,9 @@ func (c *ConsoleCommand) RunContext(ctx context.Context, cla *ConsoleArgs) int {
return ret
}

_ = packerStarter.Initialize(packer.InitializeOptions{})
_ = packerStarter.Initialize(packer.InitializeOptions{
UseSequential: cla.UseSequential,
})

// Determine if stdin is a pipe. If so, we evaluate directly.
if c.StdinPiped() {
Expand All @@ -83,9 +85,10 @@ Usage: packer console [options] [TEMPLATE]
interpolation.

Options:
-var 'key=value' Variable for templates, can be used multiple times.
-var-file=path JSON or HCL2 file containing user variables.
-config-type Set to 'hcl2' to run in HCL2 mode when no file is passed. Defaults to json.
-var 'key=value' Variable for templates, can be used multiple times.
-var-file=path JSON or HCL2 file containing user variables.
-config-type Set to 'hcl2' to run in HCL2 mode when no file is passed. Defaults to json.
-use-sequential-evaluation Fallback to using a sequential approach for local/datasource evaluation.
`

return strings.TrimSpace(helpText)
Expand Down
7 changes: 6 additions & 1 deletion command/hcl2_upgrade.go
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,12 @@ func (c *HCL2UpgradeCommand) RunContext(_ context.Context, cla *HCL2UpgradeArgs)
}

core := hdl.(*packer.Core)
if err := core.Initialize(packer.InitializeOptions{}); err != nil {
if err := core.Initialize(packer.InitializeOptions{
// Note: this is always true here as the DAG is only usable for
// HCL2 configs, so since the command only works on JSON templates,
// we can safely use the phased approach, which changes nothing.
UseSequential: true,
}); err != nil {
c.Ui.Error(fmt.Sprintf("Ignoring following initialization error: %v", err))
}
tpl := core.Template
Expand Down
7 changes: 5 additions & 2 deletions command/inspect.go
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,9 @@ func (c *InspectCommand) RunContext(ctx context.Context, cla *InspectArgs) int {
}

// here we ignore init diags to allow unknown variables to be used
_ = packerStarter.Initialize(packer.InitializeOptions{})
_ = packerStarter.Initialize(packer.InitializeOptions{
UseSequential: cla.UseSequential,
})

return packerStarter.InspectConfig(packer.InspectConfigOptions{
Ui: c.Ui,
Expand All @@ -66,7 +68,8 @@ Usage: packer inspect TEMPLATE

Options:

-machine-readable Machine-readable output
-machine-readable Machine-readable output
-use-sequential-evaluation Fallback to using a sequential approach for local/datasource evaluation.
`

return strings.TrimSpace(helpText)
Expand Down
2 changes: 2 additions & 0 deletions command/validate.go
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,7 @@ func (c *ValidateCommand) RunContext(ctx context.Context, cla *ValidateArgs) int

diags = packerStarter.Initialize(packer.InitializeOptions{
SkipDatasourcesExecution: !cla.EvaluateDatasources,
UseSequential: cla.UseSequential,
})
ret = writeDiags(c.Ui, nil, diags)
if ret != 0 {
Expand Down Expand Up @@ -124,6 +125,7 @@ Options:
-no-warn-undeclared-var Disable warnings for user variable files containing undeclared variables.
-evaluate-datasources Evaluate data sources during validation (HCL2 only, may incur costs); Defaults to false.
-ignore-prerelease-plugins Disable the loading of prerelease plugin binaries (x.y.z-dev).
-use-sequential-evaluation Fallback to using a sequential approach for local/datasource evaluation.
`

return strings.TrimSpace(helpText)
Expand Down
220 changes: 218 additions & 2 deletions hcl2template/parser.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,14 @@ import (
"fmt"
"os"
"path/filepath"
"reflect"

"github.com/hashicorp/go-version"
"github.com/hashicorp/hcl/v2"
"github.com/hashicorp/hcl/v2/ext/dynblock"
"github.com/hashicorp/hcl/v2/hclparse"
packersdk "github.com/hashicorp/packer-plugin-sdk/packer"
"github.com/hashicorp/packer/internal/dag"
"github.com/hashicorp/packer/packer"
"github.com/zclconf/go-cty/cty"
)
Expand Down Expand Up @@ -295,10 +297,224 @@ func filterVarsFromLogs(inputOrLocal Variables) {
}
}

func (cfg *PackerConfig) detectBuildPrereqDependencies() hcl.Diagnostics {
var diags hcl.Diagnostics

for _, ds := range cfg.Datasources {
dependencies := GetVarsByType(ds.block, "data")
dependencies = append(dependencies, GetVarsByType(ds.block, "local")...)

for _, dep := range dependencies {
// If something is locally aliased as `local` or `data`, we'll falsely
// report it as a local variable, which is not necessarily what we
// want to process here, so we continue.
//
// Note: this is kinda brittle, we should understand scopes to accurately
Copy link
Contributor

Choose a reason for hiding this comment

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

You mention this is brittle, can you think of a scenario where this would cause valid dependencies to be ignored?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So on this, I don't have an example to be fair, I had tried to replicate a concern of mine with local aliases in a for expression, but the local aliases weren't returned by the hcl function, so it's mostly a non-concern at this point.
I can remove this comment until we do encounter a real-life example that highlights this problem, but at the present time, it seems somewhat solid on the templates we have in the repo.

// mark something from an expression as a reference to a local variable.
// No real good solution for this now, besides maybe forbidding something
// to be locally aliased as `local`.
if len(dep) < 2 {
continue
}
rs, err := NewRefStringFromDep(dep)
if err != nil {
diags = diags.Append(&hcl.Diagnostic{
Severity: hcl.DiagError,
Summary: "failed to process datasource dependency",
Detail: fmt.Sprintf("An error occurred while processing a dependency for data source %s: %s",
ds.Name(), err),
})
continue
}

err = ds.RegisterDependency(rs)
if err != nil {
diags = diags.Append(&hcl.Diagnostic{
Severity: hcl.DiagError,
Summary: "failed to register datasource dependency",
Detail: fmt.Sprintf("An error occurred while registering %q as a dependency for data source %s: %s",
rs, ds.Name(), err),
})
}
}

cfg.Datasources[ds.Ref()] = ds
}

for _, loc := range cfg.LocalBlocks {
dependencies := FilterTraversalsByType(loc.Expr.Variables(), "data")
dependencies = append(dependencies, FilterTraversalsByType(loc.Expr.Variables(), "local")...)

for _, dep := range dependencies {
// If something is locally aliased as `local` or `data`, we'll falsely
// report it as a local variable, which is not necessarily what we
// want to process here, so we continue.
//
// Note: this is kinda brittle, we should understand scopes to accurately
// mark something from an expression as a reference to a local variable.
// No real good solution for this now, besides maybe forbidding something
// to be locally aliased as `local`.
if len(dep) < 2 {
continue
}
rs, err := NewRefStringFromDep(dep)
if err != nil {
diags = diags.Append(&hcl.Diagnostic{
Severity: hcl.DiagError,
Summary: "failed to process local dependency",
Detail: fmt.Sprintf("An error occurred while processing a dependency for local variable %s: %s",
loc.LocalName, err),
})
continue
}

err = loc.RegisterDependency(rs)
if err != nil {
diags = diags.Append(&hcl.Diagnostic{
Severity: hcl.DiagError,
Summary: "failed to register local dependency",
Detail: fmt.Sprintf("An error occurred while registering %q as a dependency for local variable %s: %s",
rs, loc.LocalName, err),
})
}
}
}

return diags
}

func (cfg *PackerConfig) buildPrereqsDAG() (*dag.AcyclicGraph, error) {
retGraph := dag.AcyclicGraph{}

verticesMap := map[string]dag.Vertex{}

// Do a first pass to create all the vertices
for ref := range cfg.Datasources {
// We keep a reference to the datasource separately from where it
// is used to avoid getting bit by the loop semantics.
//
// This `ds` local variable is the same object for every loop
// so if we directly use the address of this object, we'll end
// up referencing the last node of the loop for each vertex,
// leading to implicit cycles.
//
// However by capturing it locally in this loop, we have a
// reference to the actual datasource block, so it ends-up being
// the right instance for each vertex.
ds := cfg.Datasources[ref]
v := retGraph.Add(&ds)
verticesMap[fmt.Sprintf("data.%s", ds.Name())] = v
}
// Note: locals being references to the objects already, we can safely
// use the reference returned by the local loop.
for _, local := range cfg.LocalBlocks {
v := retGraph.Add(local)
verticesMap[fmt.Sprintf("local.%s", local.LocalName)] = v
}

// Connect the vertices together
//
// Vertices that don't have dependencies will be connected to the
// root vertex of the graph
for _, ds := range cfg.Datasources {
dsName := fmt.Sprintf("data.%s", ds.Name())

for _, dep := range ds.Dependencies {
retGraph.Connect(
dag.BasicEdge(verticesMap[dsName],
verticesMap[dep.String()]))
}
}
for _, loc := range cfg.LocalBlocks {
locName := fmt.Sprintf("local.%s", loc.LocalName)

for _, dep := range loc.dependencies {
retGraph.Connect(
dag.BasicEdge(verticesMap[locName],
verticesMap[dep.String()]))
}
}

if err := retGraph.Validate(); err != nil {
return nil, err
}

return &retGraph, nil
}

func (cfg *PackerConfig) evaluateBuildPrereqs(skipDatasources bool) hcl.Diagnostics {
Copy link
Contributor

Choose a reason for hiding this comment

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

The logic in this parser seems minimally unit tested, I notice that we have the packer_tests that invoke a larger section of the code, but I feel like it may be worth adding more testing to the logic in the parser, maybe we could make these functions public instead of private and invoke them in unit tests, just small tests that make sure we skip things aliased as local for example

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 agree it is not unit tested, and to be frank there are a bunch of pre-requisites to make this unit-testable, at the moment we rely on the Initialize tests (and subsequent phases) to test it, at least for regression checks.

Also to be clear: that code is in a parser.go file, but at this point we're not doing any kind of parsing, that was done before, it's kinda clumsy to have that logic in this file. I opted to add it here for this PR as this is where the phased code was already located, but honestly we should split this logic into separate files to avoid the confusion,

diags := cfg.detectBuildPrereqDependencies()
if diags.HasErrors() {
return diags
}

graph, err := cfg.buildPrereqsDAG()
if err != nil {
return diags.Append(&hcl.Diagnostic{
Severity: hcl.DiagError,
Summary: "failed to prepare execution graph",
Detail: fmt.Sprintf("An error occurred while building the graph for datasources/locals: %s", err),
})
}

walkFunc := func(v dag.Vertex) hcl.Diagnostics {
var diags hcl.Diagnostics

switch bl := v.(type) {
case *DatasourceBlock:
diags = cfg.evaluateDatasource(*bl, skipDatasources)
case *LocalBlock:
var val *Variable
if cfg.LocalVariables == nil {
cfg.LocalVariables = make(Variables)
}
val, diags = cfg.evaluateLocalVariable(bl)
// Note: clumsy a bit, but we won't add the variable as `nil` here
// unless no errors have been reported during evaluation.
//
// This prevents Packer from panicking down the line, as initialisation
// doesn't stop if there are diags, so if `val` is nil, it crashes.
if !diags.HasErrors() {
cfg.LocalVariables[bl.LocalName] = val
}
default:
diags = diags.Append(&hcl.Diagnostic{
Severity: hcl.DiagError,
Summary: "unsupported DAG node type",
Detail: fmt.Sprintf("A node of type %q was added to the DAG, but cannot be "+
"evaluated as it is unsupported. "+
"This is a Packer bug, please report it so we can investigate.",
reflect.TypeOf(v).String()),
})
}

if diags.HasErrors() {
return diags
}

return nil
}

for _, vtx := range graph.ReverseTopologicalOrder() {
vtxDiags := walkFunc(vtx)
if vtxDiags.HasErrors() {
diags = diags.Extend(vtxDiags)
return diags
}
}

return nil
}

func (cfg *PackerConfig) Initialize(opts packer.InitializeOptions) hcl.Diagnostics {
diags := cfg.InputVariables.ValidateValues()
diags = append(diags, cfg.evaluateDatasources(opts.SkipDatasourcesExecution)...)
diags = append(diags, cfg.evaluateLocalVariables(cfg.LocalBlocks)...)

if opts.UseSequential {
diags = diags.Extend(cfg.evaluateDatasources(opts.SkipDatasourcesExecution))
diags = diags.Extend(cfg.evaluateLocalVariables(cfg.LocalBlocks))
} else {
diags = diags.Extend(cfg.evaluateBuildPrereqs(opts.SkipDatasourcesExecution))
}

filterVarsFromLogs(cfg.InputVariables)
filterVarsFromLogs(cfg.LocalVariables)
Expand Down
Loading