Skip to content

Improve handling of failed updates - delete and new commits - #966

Merged
blampe merged 5 commits into
pulumi:masterfrom
nicu-da:nicuda/new_commit_failure
Jul 31, 2025
Merged

Improve handling of failed updates - delete and new commits#966
blampe merged 5 commits into
pulumi:masterfrom
nicu-da:nicuda/new_commit_failure

Conversation

@nicu-da

@nicu-da nicu-da commented Jul 16, 2025

Copy link
Copy Markdown
Contributor

Proposed changes

If the last update failed check the following:

  • If deleted, then respect the deleted mark
  • If new commit is detected, run update even if in the cooldown period

Related issues (optional)

Fixes #951

@github-actions

Copy link
Copy Markdown
Contributor

PR is now waiting for a maintainer to run the acceptance tests. This PR will only perform build and linting.
Note for the maintainer: To run the acceptance tests, please comment /run-acceptance-tests on the PR

@nicu-da

nicu-da commented Jul 16, 2025

Copy link
Copy Markdown
Contributor Author

cc @blampe

@github-actions

Copy link
Copy Markdown
Contributor

PR is now waiting for a maintainer to run the acceptance tests. This PR will only perform build and linting.
Note for the maintainer: To run the acceptance tests, please comment /run-acceptance-tests on the PR

@blampe

blampe commented Jul 30, 2025

Copy link
Copy Markdown
Contributor

/run-acceptance-tests

@pulumi-bot

Copy link
Copy Markdown
Contributor

@blampe blampe left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thank you for this!

Comment on lines +1091 to +1093
if stack.DeletionTimestamp != nil { // Marked for deletion (and has already been destroyed).
return true
}

@EronWright EronWright Jul 30, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It is incorrect to consider the DeletionTimestamp here, because the failed update might be the destroy operation that is triggered by the spec.destroyOnFinalize field and runs during finalization. The system must continue to retry the destroy op until it succeeds. That said, it does make sense to re-trigger a destroy if the commit has changed.

Also, if a stack was "stuck" in a failed destroy loop, the corrective action is to edit the spec and set spec.destroyOnFinalize to false. This causes the finalization routine to complete.

@EronWright

Copy link
Copy Markdown
Contributor

I took a minute to update the PR, please stand by.

updated the tests for clarity
@github-actions

Copy link
Copy Markdown
Contributor

PR is now waiting for a maintainer to run the acceptance tests. This PR will only perform build and linting.
Note for the maintainer: To run the acceptance tests, please comment /run-acceptance-tests on the PR

@EronWright
EronWright requested review from EronWright and blampe July 30, 2025 20:29
@EronWright

Copy link
Copy Markdown
Contributor

@nicu-da thanks again for the contribution!

@blampe I made a change, please re-review.

@blampe

blampe commented Jul 30, 2025

Copy link
Copy Markdown
Contributor

/run-acceptance-tests

@pulumi-bot

Copy link
Copy Markdown
Contributor

@blampe blampe left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thank you again!

@blampe
blampe merged commit f2a3513 into pulumi:master Jul 31, 2025
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Run update if new commit is detected even if in the cooldown period

4 participants