From d63871f70df8e41563c98f7abfebfa0fe0dcb791 Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Thu, 16 Jun 2022 17:15:22 -0700 Subject: [PATCH] core: Propagate check results accurately from plan to apply In an earlier commit we changed the states.CheckResults model to explicitly model the config object vs. dynamic checkable object hierarchy, but neglected to update the logic in Terraform Core to take that into account when propagating the object expansion decisions from the plan phase to the apply phase. That meant that we were incorrectly classifying zero-instance resources always as having an unknown number of instances, rather than possibly being known to have zero instances. This now follows the two-level heirarchy of the data structure, which has the nice side-effect that we can remove some of the special-case methods from checks.State that we were using to bulk-load data: the data is now shaped in the appropriate way to reload the data using the same method the plan phase would've used to record the results in the first place. --- internal/checks/state_report.go | 37 ----------------------------- internal/states/checks.go | 30 +++++++++++------------ internal/terraform/context_apply.go | 8 +++---- internal/terraform/context_walk.go | 26 +++++++++++++------- 4 files changed, 37 insertions(+), 64 deletions(-) diff --git a/internal/checks/state_report.go b/internal/checks/state_report.go index 63725d4af9..ccf35ac138 100644 --- a/internal/checks/state_report.go +++ b/internal/checks/state_report.go @@ -51,43 +51,6 @@ func (c *State) ReportCheckableObjects(configAddr addrs.ConfigCheckable, objectA } } -// ReportRestoredCheckableObjects is the interface by which Terraform Core -// should reload all of the checkable object addresses it previously determined -// during the planning phase in order to re-evaluate their results during -// the apply phase using updated data. -// -// If any of the given objects don't have a corresponding configuration object -// known to this check state then this function will panic. -func (c *State) ReportRestoredCheckableObjects(objectAddrs addrs.Set[addrs.Checkable]) { - c.mu.Lock() - defer c.mu.Unlock() - - // This is a similar principle as for ReportCheckableObjects except that - // we don't do the consistency checks that would normally ensure we only - // get one ReportCheckableObjects call per ConfigCheckable. - - for _, objectAddr := range objectAddrs { - configAddr := objectAddr.ConfigCheckable() - st, ok := c.statuses.GetOk(configAddr) - if !ok { - panic(fmt.Sprintf("checkable objects report for unknown configuration object %s", configAddr)) - } - - checks := make(map[addrs.CheckType][]Status, len(st.checkTypes)) - for checkType, count := range st.checkTypes { - // NOTE: This is intentionally a slice of count of the zero value - // of Status, which is StatusUnknown to represent that we don't - // yet have a report for that particular check. - checks[checkType] = make([]Status, count) - } - - if st.objects.Elems == nil { - st.objects = addrs.MakeMap[addrs.Checkable, map[addrs.CheckType][]Status]() - } - st.objects.Put(objectAddr, checks) - } -} - // ReportCheckResult is the interface by which Terraform Core should tell the // State object the result of a specific check for an object that was // previously registered with ReportCheckableObjects. diff --git a/internal/states/checks.go b/internal/states/checks.go index 2ae080cd45..181871a766 100644 --- a/internal/states/checks.go +++ b/internal/states/checks.go @@ -111,21 +111,6 @@ func NewCheckResults(source *checks.State) *CheckResults { return ret } -// AllCheckedObjects returns a set of all of the objects that have at least -// one check in the set of results. -func (r *CheckResults) AllCheckedObjects() addrs.Set[addrs.Checkable] { - if r == nil || len(r.ConfigResults.Elems) == 0 { - return nil - } - ret := addrs.MakeSet[addrs.Checkable]() - for _, configElem := range r.ConfigResults.Elems { - for _, objElem := range configElem.Value.ObjectResults.Elems { - ret.Add(objElem.Key) - } - } - return ret -} - // GetObjectResult looks up the result for a single object, or nil if there // is no such object. // @@ -180,3 +165,18 @@ func (r *CheckResults) DeepCopy() *CheckResults { return ret } + +// ObjectAddrsKnown determines whether the set of objects recorded in this +// aggregate is accurate (true) or if it's incomplete as a result of the +// run being interrupted before instance expansion. +func (r *CheckResultAggregate) ObjectAddrsKnown() bool { + if r.ObjectResults.Len() != 0 { + // If there are any object results at all then we definitely know. + return true + } + + // If we don't have any object addresses then we distinguish a known + // empty set of objects from an unknown set of objects by the aggregate + // status being unknown. + return r.Status != checks.StatusUnknown +} diff --git a/internal/terraform/context_apply.go b/internal/terraform/context_apply.go index 2ca1b87f8c..1d66628ad7 100644 --- a/internal/terraform/context_apply.go +++ b/internal/terraform/context_apply.go @@ -36,10 +36,10 @@ func (c *Context) Apply(plan *plans.Plan, config *configs.Config) (*states.State InputState: workingState, Changes: plan.Changes, - // We need to propagate the results of deciding which objects will - // have checks from the plan phase, since we won't re-run the - // instance expansion logic during the apply phase. - KnownCheckableObjects: plan.Checks.AllCheckedObjects(), + // We need to propagate the check results from the plan phase, + // because that will tell us which checkable objects we're expecting + // to see updated results from during the apply step. + PlanTimeCheckResults: plan.Checks, }) diags = diags.Append(walker.NonFatalDiagnostics) diags = diags.Append(walkDiags) diff --git a/internal/terraform/context_walk.go b/internal/terraform/context_walk.go index 199facc35f..523fa73854 100644 --- a/internal/terraform/context_walk.go +++ b/internal/terraform/context_walk.go @@ -3,7 +3,6 @@ package terraform import ( "log" - "github.com/hashicorp/terraform/internal/addrs" "github.com/hashicorp/terraform/internal/checks" "github.com/hashicorp/terraform/internal/configs" "github.com/hashicorp/terraform/internal/instances" @@ -25,12 +24,13 @@ type graphWalkOpts struct { Changes *plans.Changes Config *configs.Config - // KnownCheckableObjects is a set of objects that we know ahead of time - // ought to have checks run against them during this graph walk. - // Use this to propagate the decision about which objects are checkable - // from the plan phase into the apply phase, so that the apply phase - // doesn't need to recalcuate it. - KnownCheckableObjects addrs.Set[addrs.Checkable] + // PlanTimeCheckResults should be populated during the apply phase with + // the snapshot of check results that was generated during the plan step. + // + // This then propagates the decisions about which checkable objects exist + // from the plan phase into the apply phase without having to re-compute + // the module and resource expansion. + PlanTimeCheckResults *states.CheckResults MoveResults refactoring.MoveResults } @@ -116,7 +116,17 @@ func (c *Context) graphWalker(operation walkOperation, opts *graphWalkOpts) *Con } checkState := checks.NewState(opts.Config) - checkState.ReportRestoredCheckableObjects(opts.KnownCheckableObjects) + if opts.PlanTimeCheckResults != nil { + // We'll re-report all of the same objects we determined during the + // plan phase so that we can repeat the checks during the apply + // phase to finalize them. + for _, configElem := range opts.PlanTimeCheckResults.ConfigResults.Elems { + if configElem.Value.ObjectAddrsKnown() { + configAddr := configElem.Key + checkState.ReportCheckableObjects(configAddr, configElem.Value.ObjectResults.Keys()) + } + } + } return &ContextGraphWalker{ Context: c,