destroy: implement --continue-on-error #15727
Merged
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
Changelog[uncommitted] (2024-03-21)Features
|
Frassle
reviewed
Mar 19, 2024
| assert.ErrorContains(t, err, "intentionally failed delete") | ||
| assert.NotNil(t, snap) | ||
| assert.Len(t, snap.Resources, 4) // We expect 2 resources + 2 providers | ||
| require.NoError(t, snap.VerifyIntegrity()) |
| opts Options // The options for this current deployment. | ||
| preview bool // Whether or not we are doing a preview. | ||
| pendingNews sync.Map // Resources that have been created but are pending a RegisterResourceOutputs. | ||
| ignoreErrors bool // True if we want to ignore any errors and continue executing steps. |
There was a problem hiding this comment.
Do we need this flag now? Can't we just use the opts.ContinueOnError flag?
There was a problem hiding this comment.
I think we still need it. With this ignoreErrors flag, we completely ignore any errors, and don't remove any steps from anywhere, while with opts.ContinueOnError we remove some steps when deleting resources. I'm still trying to figure out what exactly we'll need for opts.ContinueOnError for up, but I suspect there's also going to be something slightly different than ignoreErrors. I think of ignoreErrors more similar to how I would expect a --force flag to behave.
tgummerer
force-pushed
the
tg/up-continue-on-error
branch
2 times, most recently
from
2909541
to
ad46c0a
Compare
Frassle
reviewed
Mar 21, 2024
pkg/engine/deployment.go
Outdated
| @@ -196,6 +196,10 @@ func newDeployment(ctx *Context, info *deploymentContext, opts *deploymentOption | |||
| }() | |||
|
|
|||
| opts.trustDependencies = proj.TrustResourceDependencies() | |||
| if !opts.trustDependencies && opts.ContinueOnError { | |||
There was a problem hiding this comment.
oh we should just clean this up, TrustResourceDependencies is always true. If you raise another PR to just remove the trustDependencies methods and fields we can approve that quickly and then not worry about it here.
There was a problem hiding this comment.
Yeah I noticed it's always true currently, but got stopped by the comment about it potentially being useful for bringing up new languages. But I guess really new languages should just get this right and we shouldn't have to worry about it in the engine.
| _, _, _, _, err = monitor.RegisterResource("pkgB:m:typB", "failing", true, deploytest.ResourceOptions{ | ||
| Dependencies: []resource.URN{dependency}, | ||
| }) | ||
| assert.NoError(t, err) |
There was a problem hiding this comment.
We should register another resource after this one. Otherwise it might just be "failing" is the last resource to be deleted and we're not actually testing that we're continuing.
There was a problem hiding this comment.
I think with the way we compute the antichains currently this test will always fail without continuing on error. However it doesn't hurt to add register another resource after this, to future proof the test, so I'll add it.
Implement a `--continue-on-error` flag for `pulumi destroy`. This makes sure we continue processing destroys even if errors occur. We will only continue for resources which do not depend on the failed resource, to make sure we always have a valid snapshot available, and to not leave any orphaned resources behind. Resources that fail to destroy will still continue to be managed by pulumi, and the process ends in an error to indicate to the user that there were problems and the destroy didn't succeed cleanly. The output will continue to be the same as for failed destroys, except we now may succeed in destroying more resources than before.
tgummerer
force-pushed
the
tg/up-continue-on-error
branch
from
ad46c0a
to
d7deebe
Compare
Frassle
approved these changes
Mar 22, 2024
github-merge-queue bot
pushed a commit
that referenced
this pull request
Mar 27, 2024
Tentative changelog: ### Features - [docs] Implement constructor syntax examples for every resource in typescript, python, csharp and go [#15624](#15624) - [engine] Send output values with property dependency information to transform functions [#15637](#15637) - [engine] Add a --continue-on-error flag to pulumi destroy [#15727](#15727) - [sdk/go] Make `property.Map` keyed by `string` not `MapKey` [#15767](#15767) - [sdk/python] Improve the error message when depends_on is passed objects of the wrong type [#15737](#15737) ### Bug Fixes - [auto/{go,nodejs,python}] Make sure to read complete lines before trying to deserialize them as engine events [#15778](#15778) - [cli/plugin] Fix installing local language plugins on Windows [#15715](#15715) - [engine] Don't delete stack outputs on failed deployments [#15754](#15754) - [engine] Fix a panic when updating provider version in a run using --target [#15716](#15716) - [engine] Handle that Assets & Archives can be returned from providers without content. [#15736](#15736) - [engine] Fix the engine trying to delete a protected resource caught in a replace chain [#15776](#15776) - [sdkgen/docs] Add missing newline for `Coming soon!` [#15783](#15783) - [programgen/dotnet] Fix generated code for a list of resources used in resource option DependsOn [#15773](#15773) - [programgen/{dotnet,go}] Fixes emitted code for object expressions assigned to properties of type Any [#15770](#15770) - [sdk/go] Fix lookup of plugin and program dependencies when using Go workspaces [#15743](#15743) - [sdk/nodejs] Export automation.tag.TagMap type [#15774](#15774) - [sdk/python] Wait only for pending outputs in the Python SDK, not all pending asyncio tasks [#15744](#15744) ### Miscellaneous - [sdk/nodejs] Reorganize function serialization tests [#15753](#15753) - [sdk/nodejs] Move mockpackage tests to closure integration tests [#15757](#15757)
Merged
github-merge-queue bot
pushed a commit
that referenced
this pull request
Mar 28, 2024
Tentative changelog: ### Features - [docs] Implement constructor syntax examples for every resource in typescript, python, csharp and go [#15624](#15624) - [docs] Implement YAML constructor syntax examples in the docs [#15791](#15791) - [engine] Send output values with property dependency information to transform functions [#15637](#15637) - [engine] Add a --continue-on-error flag to pulumi destroy [#15727](#15727) - [sdk/go] Make `property.Map` keyed by `string` not `MapKey` [#15767](#15767) - [sdk/nodejs] Make function serialization work with typescript 4 and 5 [#15761](#15761) - [sdk/python] Improve the error message when depends_on is passed objects of the wrong type [#15737](#15737) ### Bug Fixes - [auto/{go,nodejs,python}] Make sure to read complete lines before trying to deserialize them as engine events [#15778](#15778) - [cli/plugin] Fix installing local language plugins on Windows [#15715](#15715) - [engine] Don't delete stack outputs on failed deployments [#15754](#15754) - [engine] Fix a panic when updating provider version in a run using --target [#15716](#15716) - [engine] Handle that Assets & Archives can be returned from providers without content. [#15736](#15736) - [engine] Fix the engine trying to delete a protected resource caught in a replace chain [#15776](#15776) - [sdkgen/docs] Add missing newline for `Coming soon!` [#15783](#15783) - [programgen/dotnet] Fix generated code for a list of resources used in resource option DependsOn [#15773](#15773) - [programgen/{dotnet,go}] Fixes emitted code for object expressions assigned to properties of type Any [#15770](#15770) - [sdk/go] Fix lookup of plugin and program dependencies when using Go workspaces [#15743](#15743) - [sdk/nodejs] Export automation.tag.TagMap type [#15774](#15774) - [sdk/python] Wait only for pending outputs in the Python SDK, not all pending asyncio tasks [#15744](#15744) ### Miscellaneous - [sdk/nodejs] Reorganize function serialization tests [#15753](#15753) - [sdk/nodejs] Move mockpackage tests to closure integration tests [#15757](#15757)
12 tasks
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.
Implement a
--continue-on-errorflag forpulumi destroy. This makes sure we continue processing destroys even if errors occur. We will only continue for resources which do not depend on the failed resource, to make sure we always have a valid snapshot available, and to not leave any orphaned resources behind.Resources that fail to destroy will still continue to be managed by pulumi, and the process ends in an error to indicate to the user that there were problems and the destroy didn't succeed cleanly.
The output will continue to be the same as for failed destroys, except we now may succeed in destroying more resources than before.
/cc #3304