-
Notifications
You must be signed in to change notification settings - Fork 9.6k
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
backend/local: create local state file if backend write fails #14423
Conversation
Note for the future: one thing we will need to contend with here is situations where Terraform is running in automation, such as Terraform Enterprise. Such systems may want to have special handling for this situation, to ensure that the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extremely well done. One request for changes.
I think Apply is the right place to do this. Refresh is the only other command that modifies state and its not a big deal if that doesn't get persisted.
backend/local/backend_apply.go
Outdated
// UX, so we should definitely avoid doing this if at all possible, | ||
// but at least the user has _some_ path to recover if we end up | ||
// here for some reason. | ||
jsonState, jsonErr := json.MarshalIndent(applyState, "", " ") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you use terraform.WriteState
to a bytes.Buffer here instead? Its possible that JSON encoding directly may not produce the correct state.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 makes sense! I'd forgotten about that function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with the same change that @mitchellh requested.
I wonder if we eventually should try to handle this better directly with BackupState? While this doesn't replace the current use for BackupState, since the user may want the previous rather than an intermediate state, maybe BackupState should record all writes in versioned files, or at least the original and the latest write? On a related note, I've been trying to come up with a nice way to prevent users from aborting a run and then starting new run before recovering the state (after ignoring the UI warning of course), and losing the state (e.g. |
fd22890
to
d60b251
Compare
This is now updated based on the feedback. To me this feels different than This situation is rather unique to the apply action. All other state-modifying commands don't benefit from this, because they don't make any new resources that Terraform needs to start tracking. I'd rather give users a straightforward error if, for example, |
Change updated in response to review feedback
In the old remote state system we had the idea of a local backup, which is actually still present for the legacy backends but no longer applies for the new-style backends like the s3 backend. It's problematic when an apply runs for long enough that someone's time-limited AWS STS credentials expire and then Terraform fails and can't persist state to S3. To reduce the risk of lost state, here we add some extra fallback code for the local apply operation in particular. If either state writing or state persisting fail then we attempt to write the state to a special backup file errored.tfstate, and produce an error message that guides the user on how to retry uploading this state. In the unlikely event that we can't write to local disk either (e.g. permissions problems) we take a last-ditch attempt to dump the JSON onto stdout and advise the user to manually copy it into a file for import. If even that doesn't work for some reason, we assume a critical Terraform bug (JSON-serialization problem with states?) and bail out with an apologetic error message. This is implemented for the apply command in particular because this is the one command where new objects are created in real APIs that we don't want to lose track of. For other operations it's less bad to just generate a simple error message and have the user retry. This fixes #14298.
d60b251
to
28ebf6e
Compare
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further. |
In the state system we have the idea of a local backup, which is actually still present and used when the local backend is active, but is no longer used when a remote backend is active.
This is problematic when an apply runs for long enough that someone's time-limited AWS STS credentials expire and then Terraform fails and can't persist state to S3.
To reduce the risk of lost state, here we add some extra fallback code for the local apply operation in particular. If either state writing or state persisting fail then we attempt to write the state to a special backup file
errored.tfstate
, and produce an error message that guides the user on how to retry uploading this state.In the unlikely event that we can't write to local disk either (e.g. permissions problems) we take a last-ditch attempt to dump the JSON onto stdout and advise the user to manually copy it into a file for import. If even that doesn't work for some reason, we assume a critical Terraform bug (JSON-serialization problem with states?) and bail out with an apologetic error message.
This new behavior is added to the apply operation, rather than simply making the existing local backup system work for remote backends again, because doing it at the apply layer means we can give better feedback to the user and more easily have the additional fallback of printing to stdout.
This is implemented for the apply operation in particular because this is the one operation where new objects are created in real APIs that we don't want to lose track of. For other operations it's less bad to just generate a simple error message and have the user retry, since no new objects will have been created.
This fixes #14298. @phinze, I think this would also address part of what you apparently ran into earlier today where you needed to abort a Terraform process with expired STS credentials without losing state.