Backport of core: Don't try to register separate checkable resources for each module instance into v1.3 #31875
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
teamterraform
force-pushed
the
backport/b-check-resource-multi-expand/extremely-key-sheep
branch
from
03df26e
to
5fadcb0
Compare
We were previously _trying_ to handle diagnostics here but were not quite doing it right because we were testing whether the resulting error was nil rather than appending it to the diagnostics and then seeing if the result has errors. The difference here is important because it allows DynamicExpand to return warnings without associated errors when needed. Previously the graph walker would treat a warnings-only result as if it were an error. Ideally we'd change DynamicExpand to return diagnostics directly, but we previously decided against that because there were so many implementors to update, and my intent for this change is to be surgical in the update so we minimize risk of backporting the change into patch releases.
We previously did two levels of DynamicExpand to go from ConfigResource to AbsResource and then from AbsResource to AbsResourceInstance. We'll now do the full expansion from ConfigResource to AbsResourceInstance in a single DynamicExpand step inside nodeExpandPlannableResource. The new approach is essentially functionally equivalent to the old except that it fixes a bug in the previous implementation: we will now call checkState.ReportCheckableObjects only once for the entire set of instances for a particular resource, which is what the checkable objects infrastructure expects so that it can always mention all of the checkable objects in the check report even if we bail out partway through due to a downstream error. This is essentially the same code but now turned into additional methods on nodeExpandPlannableResource instead of having the extra graph node type. This has the further advantage of this now being straight-through code with standard control flow, instead of the unusual inversion of control we were doing before bouncing in and out of different Execute and DynamicExpand implementations to get this done.
We use a non-pointer value for this particular node, which means that there can never be two root nodes in the same graph: the graph implementation will just coalesce them together when a second one is added. Our resource expansion code is relying on that coalescing so that it can subsume together multiple graphs for different modules instances into a single mega-graph with all instances across all module instances, with any root nodes coalescing together to produce a single root. This also updates one of the context tests that exercises resource expansion so that it will generate multiple resource instance nodes per module and thus potentially have multiple roots to coalesce together. However, we aren't currently explicitly validating the return values from DynamicExpand and so this test doesn't actually fail if the coalescing doesn't happen. We may choose to validate the DynamicExpand result in a later commit in order to make it more obvious if future modifications fail to uphold this invariant.
apparentlymart
force-pushed
the
backport/b-check-resource-multi-expand/extremely-key-sheep
branch
from
5fadcb0
to
66c3db6
Compare
|
Reminder for the merging maintainer: if this is a user-visible change, please update the changelog on the appropriate release branch. |
|
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Backport
This PR is auto-generated from #31860 to be assessed for backporting due to the inclusion of the label 1.3-backport.
The below text is copied from the body of the original PR.
Over in #31846 we learned that the logic for registering the "checkable object instances" for resources with custom condition checks was running once per containing module instance, rather than once total to cover all instances of a resource across all module instances.
The checks infrastructure expects to be told the instances for each checkable object exactly once so that we can ensure it always either has complete information or no information. This is part of how we guarantee that we'll either always return the full set of instances for a checkable object or mark the check as "error" so that check-presenting UIs can produce a consistent set of instances on each run.
Unfortunately our previous approach to handling the two-level expansion of resources inside modules was to use two levels of
DynamicExpand, which is a special inversion-of-control technique we use as part of the concurrent graph walk. That meant that there was no place in the code which had access to all of the instances of a particular resource at once.It turns out that the double-
DynamicExpandisn't really necessary here, and was largely just a convenience when we were implementing multi-instance modules to avoid disrupting the existing resource expansion logic too much. The module expansion logic is all just local computation anyway, and it was holding a lock on the state for its duration, so it was getting no material benefit from being called concurrently by the graph walker.To correct the bug, this reworks the logic to just be one big DynamicExpand that uses normal straight-through code to gather all of the instances of a resource together into a dynamic subgraph, and register them with the checks system. This makes the control flow clearer while also ensuring that by the end of this function we know exactly which set of resource instances we're expecting.
I had originally intended to find a less invasive fix for this for backporting into the v1.3 branch to fix the bug, but the graph walker inversion of control meant that before there was no reasonable correct place to call
ReportCheckableObjectsobjects from.To minimize the risk of doing such refactoring I made an effort to modify the actual logic as little as possible: what were previously the
ExecuteandDynamicExpandmethods ofNodePlannableResourceare nownodeExpandPlannableResource.expandResourceInstancesandnodeExpandPlannableResource.resourceInstanceSubgraphrespectively, with them chained together as direct method calls rather than indirectly chained using an intermediate dynamic subgraph.Moving some logic that was previously in an
Executemethod to instead be in aDynamicExpandmethod exposed a logic error in the graph walker's handling ofDynamicExpandwhich I corrected here in the initial commit.This refactoring did not regress any existing tests except the ones that were directly testing
NodePlannableResource, which are now removed altogether because that graph node type no longer exists. I added a new test to cover the resource-related portion of #31846.Issue #31846 seems to actually be capturing two separate but related bugs, where the other is about output values. This PR does not attempt to address the output value bug, since its root cause seems unrelated and so I intend to tackle it as a separate PR.