Skip to content
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

core: Don't panic if EvalMaybeResourceDeposedObject has no DeposedKey #23718

Merged
merged 1 commit into from
Jan 6, 2020

Conversation

apparentlymart
Copy link
Contributor

@apparentlymart apparentlymart commented Dec 19, 2019

This is a "should never happen" case, but we have reports of it actually happening. In order to try to collect a bit more data about what's going on here, we're changing what was previously a hard panic into a normal error message that can include the address of the instance we were working on and the action we were trying to do to it at the time.

The hope is to narrow down what situations can trigger this in order to find a reliable reproduction case in order to debug further. This also means that for those who do encounter this problem in the meantime Terraform will have a chance to shut down cleanly and therefore be more likely to be able to recover on a subsequent plan/apply cycle.

Further investigation of this will follow once we see a report or two of this updated error message.

This closes #23709, and is aiming to contribute to eventually figuring out #22974 in a later change.


This also includes some tweaks to some tests that are not directly related to this change but that behaved in unfortunate ways when I faked an occurrence of this problem in order to test the error messages. Neither of those changes affects the outcome of the change I've made here, but both of these tests will now behave better when receiving an error other than the one they are expecting.

This is a "should never happen" case, but we have reports of it actually
happening. In order to try to collect a bit more data about what's going
on here, we're changing what was previously a hard panic into a normal
error message that can include the address of the instance we were working
on and the action we were trying to do to it at the time.

The hope is to narrow down what situations can trigger this in order to
find a reliable reproduction case in order to debug further. This also
means that for those who _do_ encounter this problem in the meantime
Terraform will have a chance to shut down cleanly and therefore be more
likely to be able to recover on a subsequent plan/apply cycle.

Further investigation of this will follow once we see a report or two of
this updated error message.
@apparentlymart apparentlymart added this to the 0.12.19 milestone Dec 19, 2019
@apparentlymart apparentlymart requested a review from a team December 19, 2019 01:09
@apparentlymart apparentlymart self-assigned this Dec 19, 2019
@ghost ghost added the sdkv1 [PRs only] Marks changes that may potentially need to be ported to the plugi nSDK label Dec 19, 2019
@apparentlymart apparentlymart added bug core v0.12 Issues (primarily bugs) reported against v0.12 releases labels Dec 19, 2019
Copy link
Contributor

@mildwonkey mildwonkey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me (the failing e2e tests are unrelated).

@apparentlymart apparentlymart merged commit 7f8e087 into master Jan 6, 2020
@apparentlymart apparentlymart deleted the b-maybeundepose-crash branch January 6, 2020 18:23
@ghost
Copy link

ghost commented Feb 6, 2020

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.

@ghost ghost locked and limited conversation to collaborators Feb 6, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug core sdkv1 [PRs only] Marks changes that may potentially need to be ported to the plugi nSDK v0.12 Issues (primarily bugs) reported against v0.12 releases
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Guard against missing DeposedKey in EvalMaybeRestoreDeposedObject
2 participants