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

fix: address an issue in the rollout controller where bindings might not receive status refresh after binding spec changes #1049

Merged
merged 7 commits into from
Feb 25, 2025

Conversation

michaelawyu
Copy link
Contributor

Description of your changes

This PR fixes an issue in the rollout controller where bindings might not receive status refresh after binding spec changes.

Specifically, when

a) a Bound binding with up to date resource/override snapshot; or
b) an Unscheduled binding that has not been deleted yet

has its spec changed (e.g., apply strategy update), rollout controller will stop processing them and such bindings will no longer have fresh RolloutStarted condition added to their status. As a result, work generator will refuse to process these bindings and the system will be stuck.

I have:

  • Run make reviewable to ensure this PR is ready for review.

How has this code been tested

  • Unit tests
  • Integration tests
  • [] E2E tests will be added in a separate PR (enable the new work applier)

Special notes for your reviewer

N/A

// cluster, upgrading to a newer resource/override snapshot, or removing) but are blocked by
// the rollout strategy.
//
// Update the status first, so that if the rolling out (updateBindings func) fails in the
Copy link
Contributor

Choose a reason for hiding this comment

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

what does this comment mean?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi Ryan! This (Update the status first...) was the original comment I believe.

@michaelawyu
Copy link
Contributor Author

Per offline discussions, will temporarily drop the change that populates the RolloutStarted condition for unscheduled bindings (as the work generator currently handles unscheduled bindings as an exception) and evaluate later how we could further improve the flow.

@ryanzhang-oss ryanzhang-oss merged commit 317f9e8 into Azure:main Feb 25, 2025
12 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.

2 participants