Skip to content

Allow cleanup of states that depend on prior runs outputs #36902

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

Open
wants to merge 6 commits into
base: f-controlling-destroys
Choose a base branch
from

Conversation

dsa0x
Copy link
Member

@dsa0x dsa0x commented Apr 17, 2025

The important changes here

  • The config inspection of skip_cleanup is now done in the config file
    • This led to some minor test changes
  • The cleanup nodes are now in a subgraph, so that we are not adding weird dynamic nodes and complicated edges into the graph.
  • After a destroy, we only update the in-memory state resources, and we leave the outputs
  • We now always write the state outputs to file, even after a destroy of resources. This way, if a run1 state's resources has been destroyed by a later run run3, run2 which references run1 can still access the output values, and use as input to its operations.

Target Release

1.13.x

CHANGELOG entry

  • This change is user-facing and I added a changelog entry.
  • This change is not user-facing.

@dsa0x dsa0x force-pushed the sams/cmd-test-cleanup branch 2 times, most recently from f45f122 to d9adf58 Compare April 17, 2025 12:20
@dsa0x dsa0x added the no-changelog-needed Add this to your PR if the change does not require a changelog entry label Apr 17, 2025
@dsa0x dsa0x marked this pull request as ready for review April 17, 2025 12:20
@dsa0x dsa0x requested a review from a team as a code owner April 17, 2025 12:20
Copy link
Member

@liamcervante liamcervante left a comment

Choose a reason for hiding this comment

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

I think we also want to update the normal cleanup operation as well right? If a run block fails to get destroyed, any dependent run blocks that have different state should not be destroyed as we might need the outputs from that state file?

@dsa0x
Copy link
Member Author

dsa0x commented Apr 22, 2025

I think we also want to update the normal cleanup operation as well right? If a run block fails to get destroyed, any dependent run blocks that have different state should not be destroyed as we might need the outputs from that state file?

@liamcervante this would be a change in behavior though. It would imply that a state clean up failing prevents other states from being cleaned up. But you are right, it might be necessary, in order to appropriately cleanup that failed state.

@liamcervante
Copy link
Member

Yes, I think it's required though - we need to keep dependent states in place otherwise the cleanup command just can't work. There's no breaking changes so I think its fine. The existing behaviour is just "do nothing", so this change being introduced alongside the state manifest file means the user is being left with a lot more information now.

@dsa0x dsa0x force-pushed the sams/cmd-test-cleanup branch 2 times, most recently from 855a272 to 1cfc4f0 Compare April 23, 2025 18:15
@dsa0x
Copy link
Member Author

dsa0x commented Apr 23, 2025

The important changes here

  • The config inspection of skip_cleanup is now done in the config file
    • This led to some minor test changes
  • The cleanup nodes are now in a subgraph, so that we are not adding weird dynamic nodes and complicated edges into the graph.
  • After a destroy, we only update the in-memory state resources, and we leave the outputs
  • We now always write the state outputs to file, even after a destroy of resources. This way, if a run1 state's resources has been destroyed by a later run run3, run2 which references run1 can still access the output values, and use as input to its operations.

@dsa0x dsa0x requested a review from liamcervante April 23, 2025 18:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-changelog-needed Add this to your PR if the change does not require a changelog entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants