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

Provide a command for handling pending creates #9778

Closed
wants to merge 13 commits into from

Conversation

iwahbe
Copy link
Member

@iwahbe iwahbe commented Jun 3, 2022

Description

Provides an interactive command to clear pending creates.

Fixes #4265

Checklist

  • I have added tests that prove my fix is effective or that my feature works
  • Yes, there are changes in this PR that warrants bumping the Pulumi Service API version

@iwahbe iwahbe self-assigned this Jun 3, 2022
@Frassle
Copy link
Member

Frassle commented Jun 6, 2022

I like this :)

Luke said we might want to make refresh just clear these all out as well, like it does for pending deletes/updates.

@iwahbe
Copy link
Member Author

iwahbe commented Jun 6, 2022

@Frassle I read through the discussion on #4265 and it seemed like we wanted to clear everything but creates with a pulumi refresh and warn on pending creates. (implemented in #9293). I agree with you that forgetting about potentially created resources is scary and I would be very surprised if pulumi ever did so without my explicit say so. I'll confirm with @lukehoban that this is within his expectations.

@iwahbe iwahbe requested review from Frassle and Zaid-Ajaj June 6, 2022 22:41
remove that operation from the "pending_operations" section of the file. Once this is complete,
use 'pulumi stack import' to import the repaired stack.`
which are not tracked by pulumi. To mark a pending CREATE operation as successful, use 'pulumi stack pending
resolve-create'. To mark a pending CREATE operation as failed, use 'pulumi state pending clear-create'.`
Copy link
Member

Choose a reason for hiding this comment

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

It feels like we could make this experience even more "pleasant" by further simplifying the warning and the set of actions needed here.

In particular, the message currently suggests three different actions, with not a ton of clarity on how to decide between these:

  • pulumi refresh
  • pulumi stack pending resolve-create
  • pulumi stack pending clear-create

Perhaps we could simplify this to:

  • To clear out pending operations run pulumi stack pending clear
  • If a resources was created in your cloud provider but not recorded by Pulumi, you can bring it under management by instead running pulumi stack pending adopt.

I think there is an argument that we only need to even suggest the first, and could just direct to docs to learn about other options. But that may be going too far.

This would then avoid relying on refresh as a path to incidentally address this - since that only really made sense if it was going to fully address the problem, which we have decided we are not comfortable with.

I'm also curious - what is the guidance for automated scenarios (CI/CD, Automation API, etc?). Is it that users should just live with the warnings, until they are ready to manually run one of these commands from outside the automated system? Or is it that they should add logic to the automation to clear these out but log/record the cleared pending operations?

Copy link
Member

Choose a reason for hiding this comment

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

Related to the above - do we really still think refresh should partially clear pending operations but not fully? It feels like we've now changed our overall approach, and should revisit that as part of a consistent approach to pending operations? Does it make sense to undo that and incorporate non-creates into the new pulumi stack pending ... commands? Either as a pending clear that covers everything as noted above, or as a separate command for non-creates?

Copy link
Member

Choose a reason for hiding this comment

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

Related to the above - do we really still think refresh should partially clear pending operations but not fully?

I think it's ok for refresh to fix the pending operations that it can fix non-interactively. If we had pending deletes or updates refresh is able to correctly resolve the state to what is true. It's only pending creates where it needs help from a user to tell it if the create actually worked or not.

To clear out pending operations run pulumi stack pending clear

I think I would be confused and think that clears out all pending operations.

what is the guidance for automated scenarios (CI/CD, Automation API, etc?). Is it that users should just live with the warnings, until they are ready to manually run one of these commands from outside the automated system? Or is it that they should add logic to the automation to clear these out but log/record the cleared pending operations?

I think you could do either. Depends on how well your team respond to alerts. You could alert for pending creates then just clear them and assume that someone will pick up the alert and clean up the problematic resource. Or you could keep hitting the warning on every deploy so that it's eventually noticed and let the team run the fix up command manually.

Copy link
Member Author

Choose a reason for hiding this comment

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

Under the current design, commands in the pulumi state subdomain are only to manipulate the state. They don't remove refresh from the resolve pending path. They provide the human interface to allow refresh to fully resolve the pending operations. pulumi state pending resolve-create provides an interface to convert a pending create into a pending import, which can then be resolved by running pulumi refresh.

Related to the above - do we really still think refresh should partially clear pending operations but not fully? It feels like we've now changed our overall approach, and should revisit that as part of a consistent approach to pending operations? Does it make sense to undo that and incorporate non-creates into the new pulumi stack pending ... commands? Either as a pending clear that covers everything as noted above, or as a separate command for non-creates?

I think of it as pulumi refresh updates the state to reflect reality as much as possible by looking to the providers. This is still necessary, since pulumi state only performs local and self-contained edits to the state. We need both commands since they do fundamentally different things.

I'm also curious - what is the guidance for automated scenarios (CI/CD, Automation API, etc?). Is it that users should just live with the warnings, until they are ready to manually run one of these commands from outside the automated system? Or is it that they should add logic to the automation to clear these out but log/record the cleared pending operations?

Teams can decide for themselves, but I would expect that the pulumi state subcommand are employed manually when something has gone wrong. They should not be needed in the common case.

Perhaps we could simplify this to:

  • To clear out pending operations run pulumi stack pending clear
  • If a resources was created in your cloud provider but not recorded by Pulumi, you can bring it under management by instead running pulumi stack pending adopt.

We really only need pending to fix pending creates. I think pulumi refresh works well for other pending ops and we should not replace it with manual (even CI assisted) state manipulation. To avoid confusion, I think we need some reference to the "create" operation in pulumi state pending clear, hence clear-create.

Copy link
Member

Choose a reason for hiding this comment

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

Let's try to drive to resolution here, sorry for not chiming in earlier. I have yet another alternative:

  • Keep pulumi refresh to do what it does today for pending updates and deletes. It makes sense to ambient resolve updates and deletes once the CLI knows the true state of those resources.
  • Introduce a command to ask for an action for each pending operation. If it's a create, then it's Import (give ID), Clear, or Skip. If it's an update or delete, then Refresh, Clear, or Skip.

Now, it's not exactly clear what that command is. It could be pulumi stack pending resolve or pulumi state pending resolve (but it needs to read data from the cloud, eventually). Could it just be a part of pulumi refresh though?

pulumi refresh --clear-pending // deletes all pending creates and then refreshes updates/deletes

pulumi refresh // interactive
// checks for pending creates
// presents a choice for Import (ask for ID) or Clear for each one (maybe also Skip)
// once done, proceeds to the normal refresh flow

Is it weird? Apologies if we discussed this elsewhere and I missed.

@iwahbe
Copy link
Member Author

iwahbe commented Aug 17, 2022

Closed in favor of #10394.

@iwahbe iwahbe closed this Aug 17, 2022
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.

Easier way to remove/clear pending operations
4 participants