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

autoscaling: Fix auto-rollback for instance refresh #39007

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

arianvp
Copy link
Contributor

@arianvp arianvp commented Aug 23, 2024

Description

The auto-rollback behaviour for instance refresh relies on the fact that the desired configuration is not modified until the instance refresh finishes. If the instance refresh fails, it uses the values from the Auto Scaling Group for the rollback.

So when we do an Instance Refresh we should not use UpdateAutoScalingGroup for modify the LaunchTemplate or MixedInstancesPolicy.

Instead we should always rely on the DesiredConfiguration parameter of StartInstanceRefresh

StartInstanceRefresh will modify the Auto Scaling Group for us on success or else rollback (if enabled) to the previous version.

If Auto Rollback is disabled; and the instance will simply fail, causing the apply to fail.

Relations

Closres #34189

References

Output from Acceptance Testing

% make testacc TESTS=TestAccXXX PKG=ec2

...

Copy link

Community Note

Voting for Prioritization

  • Please vote on this pull request by adding a 👍 reaction to the original post to help the community and maintainers prioritize this pull request.
  • Please see our prioritization guide for information on how we prioritize.
  • Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for issue followers and do not help prioritize the request.

For Submitters

  • Review the contribution guide relating to the type of change you are making to ensure all of the necessary steps have been taken.
  • For new resources and data sources, use skaff to generate scaffolding with comments detailing common expectations.
  • Whether or not the branch has been rebased will not impact prioritization, but doing so is always a welcome surprise.

@github-actions github-actions bot added service/autoscaling Issues and PRs that pertain to the autoscaling service. needs-triage Waiting for first response or review from a maintainer. labels Aug 23, 2024
@arianvp
Copy link
Contributor Author

arianvp commented Aug 23, 2024

I didn't test this yet. But I think the change like this makes sense. The function is huge though. Suggestions on how to restructure the code to make this change more obvious would be appreciated

The auto-rollback behaviour for instance refresh relies
on the fact that the desired configuration is *not* modified
until the instance refresh finishes. If the instance refresh fails,
it uses the values from the Auto Scaling Group for the rollback.

So when we do an Instance Refresh we should not use UpdateAutoScalingGroup
for modify the LaunchTemplate or MixedInstancesPolicy.

Instead we should always rely on the DesiredConfiguration parameter
of StartInstanceRefresh

StartInstanceRefresh will modify the Auto Scaling Group for us on success
or else rollback (if enabled) to the previous version.

If Auto Rollback is disabled; then the instance refresh will simply fail,
causing the apply to fail.
@arianvp
Copy link
Contributor Author

arianvp commented Aug 23, 2024

This resource does not wait for the instance refresh to complete.

This might be problematic. As we can't observe the change to the autoscaling group after the instance refresh completed. Breaking my entire idea.

I think how instance refresh is currently implemented is kind of broken. I'm not sure if we can make it work :/

@arianvp
Copy link
Contributor Author

arianvp commented Aug 23, 2024

I guess what we can do is read any in progress instance refreshes in the read operation and use that as the "current" state for LaunchTemplate and MixedInstancesPolicy.

@justinretzolk justinretzolk added bug Addresses a defect in current functionality. and removed needs-triage Waiting for first response or review from a maintainer. labels Aug 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Addresses a defect in current functionality. service/autoscaling Issues and PRs that pertain to the autoscaling service.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants