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

feat: move the rollout controller to event driven model #1029

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

ryanzhang-oss
Copy link
Contributor

@ryanzhang-oss ryanzhang-oss commented Jan 31, 2025

Description of your changes

  1. move the rollout controller to event driven model
  2. minor fixes on comments and logs
  3. fix the bug that rollout got stuck when the master snapshot is deleted but the binding is not synced yet.

Fixes #

I have:

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

How has this code been tested

Special notes for your reviewer

@ryanzhang-oss ryanzhang-oss marked this pull request as draft February 1, 2025 00:50
Copy link
Contributor

@michaelawyu michaelawyu left a comment

Choose a reason for hiding this comment

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

Hi Ryan! I've added some comments, PTAL 🙏

apis/placement/v1beta1/binding_types.go Show resolved Hide resolved
pkg/controllers/rollout/controller.go Outdated Show resolved Hide resolved
pkg/controllers/rollout/controller.go Outdated Show resolved Hide resolved
pkg/controllers/rollout/controller.go Show resolved Hide resolved
pkg/controllers/rollout/controller.go Show resolved Hide resolved
@ryanzhang-oss ryanzhang-oss marked this pull request as ready for review February 7, 2025 23:50
@ryanzhang-oss
Copy link
Contributor Author

The bug was the workGenerator controller will mark the binding as overridden failed but rollout controller doesn't consider that binding has "FAILED". This leads to the roll out stuck because the binding can be ready thus it prevent the maxNumber to go over.

There needs to be a follow up on the workGenerator/applier to differentiate (best effort) user error/not retriable error from retriable.

Copy link
Contributor

@michaelawyu michaelawyu left a comment

Choose a reason for hiding this comment

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

LGTM ;) Has some minor comments; will add them later -> merging the PR now upon test completion to unblock progress

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