-
Notifications
You must be signed in to change notification settings - Fork 10.1k
refactor: Change how we pass backend state around when creating a plan file #37984
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
Conversation
This helps with navigating ambiguity around the word backend. The new name should indicate that the value represents a `backend` block, not a more general interpretation of what a backend is.
…to a lack of data. Don't change it if pluggable state storage is in use.
…sing that data when creating a plan file.
…ly isn't valid for `stateStoreConfigState` to be nil I'm about 90% sure that backendConfigState being nil is ok <_<
… file with the expected state_store configuration data
Co-authored-by: Radek Simko <[email protected]>
…d.Backend` is encountered when managing a state store
…backend config state to downstream logic. In future this should be simplified, either via refactoring or changes affecting the implied local backend
…e with reading data from the local file This has the disadvantage of requiring more I/O; we read the file an additional time per operation.
…nstead of requiring the file to be re-read a second time in commands that use Operations.
|
The equivalence tests failed. Please investigate here. |
|
@radeksimko - Tagging you here as we can potentially discuss these changes in relation to this thread #37956 (comment) |
|
After discussion we agreed that a better solution would be to tackle the behemoth that is the Meta struct. It'd be nicer to have all backend-related logic scoped inside a struct that isn't the Meta but is still embedded within the different Command structs in the code base. Due to that, and how this PR is "rearranging a mess into a sightlier prettier mess" 😆 , I'm closing this PR. |
Background
All commands that use a backend (or state store) to access state will use the Meta struct's backend (and via that, Backend) method to obtain a configured backend for use in subsequent command logic. During that logic, Terraform also checks that the backend configuration is unchanged since the last
init. If the configuration and backend state file do not match then that logic will return an error telling users to runinit.Downstream logic within individual commands may want to use data in the backend state file. Instead of re-reading that data from disk the data is currently stored in the Meta, in-memory. This happens in backendFromConfig, which is the method where Terraform compares backend state file contents to the configuration. Once backendFromConfig has returned we're either in an error state (in which case the in-memory copy of the backend state file isn't used) or we know that the backend state file is up-to-date and can be used as a source of truth.
Here is how the data is stored in-memory on the Meta:
terraform/internal/command/meta_backend.go
Lines 728 to 735 in c9d0240
Alternative approaches
Re-read the backend state file from disk when needed
Instead of storing the data in-memory we could just have the downstream logic read the backend state file again. This is implemented in 8b9be31.
This approach isn't ideal because although this data is only needed by the
plancommand, ALL commands that use Operations will run the altered logic. This means every plan/apply/query/refresh/state show/console command would have an extra step of reading from a file on disk, despite that data only being required forplanoperations. This has an unnecessary impact on performance.Store the backend state file contents in the Meta, not just subsets of the data
Originally the Meta would store only the description of the Backend kept in the backend state file. After #37956 the Meta has two mutually exclusive fields to store either backend or state store config state.
The changes in 676b2a6 propose storing the entire backend state file in the Meta. This means interpretation of the data, including how to handle the lack of backend state, is the responsibility of the logic that's specific to the
plancommand. Only theplancommand will need special logic interpreting the lack of backend state file data as an indication of using the implicit local backend.By the time the
plancommand's own specific logic is executed we know that Terraform has determined that the working directory has been initialised and the configuration is unchanged. Therefore the plan command can confidently interpret the lack of a backend state file as meaning an implicit local backend is in use.In this approach we avoid re-reading from the backend state file from disk, so there isn't any impact due to increased I/O.
Target Release
1.15.x
Rollback Plan
Changes to Security Controls
Are there any changes to security controls (access controls, encryption, logging) in this pull request? If so, explain.
CHANGELOG entry