Finish implementation of unknown count/for_each in data sources #35122
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
I checked this with James, and it was to temporarily speed up test cycle times on a slippery bug.
…Allowed` Some of the methods on `Deferred` were using `d.externalDependencyDeferred` to report whether some items were deferred, without first checking that `d.deferralAllowed` was even turned on. Then, to accommodate that, we were making sure to only pass the external dependency deferral signal (AKA the whole component is deferred) in to the `Deferred` struct if deferral is allowed. This was an artifact of the order things got implemented in; the "is thing deferred" checks predated the `deferralAllowed` field, and the call sites of those methods were behind other guards at the time. Anyway, this commit flips things so we now always propagate the received `ExternalDependencyDeferred` value in to the `Deferred`, but the `Deferred` takes the global on/off switch into account before relying on that value for its final answers about a given possible deferral.
This should be sound 100% of the time, because the only way anything can be deferred in an apply is if we _planned_ to defer it... in other words, if deferral was allowed during the plan. The reason we need this enabled is to allow proper construction of wildcard key addresses for data sources that are deferred due to unknown count or for_each values. Unlike with resources, we don't have a convenient space in the plan to stash info about a data source deferral; data sources don't have "planned changes" per se (they're unmanaged and thus can't be changed), so they don't result in a "deferred change" and must be re-checked during the apply. However, since applies are relying on the plan's info about deferred _actual_ changes, they don't need to construct the (still somewhat expensive) resource graph, so we should opt them out of that graph construction to avoid a performance regression.
|
I think you will need to look for a deferred data source by checking for the absence of the other options.
I think the short circuit I also think allowing |
jbardin
approved these changes
May 8, 2024
|
Reminder for the merging maintainer: if this is a user-visible change, please update the changelog on the appropriate release branch. |
|
(Changelog already has a line to this effect, so I'm leaving it be.) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This PR adds tests for deferring data sources due to unknown count/for_each values, and makes the necessary behavior changes to get those tests passing.
For reviewers: here's the main things to validate about this PR.
deferralAllowed = truein applies? If not, can we think of a safer way to re-process data sources that should be deferred (preferably without significantly changing the semantics of the plan)?Deferred's informational methods to do early negative returns ifdeferralAllowed == false?Target Release
1.9.x
Draft CHANGELOG entry
EXPERIMENTS
The deferred actions experiment now supports deferring data sources due to unknown count/for_each values.