-
Notifications
You must be signed in to change notification settings - Fork 4.3k
AEP-8898: Standardize VPA status condition handling #8898
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
base: master
Are you sure you want to change the base?
Conversation
c0768dc to
b984d62
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: adrianmoisey The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
copyright@2025 Gitlab.com |
|
This PR may require API review. If so, when the changes are ready, complete the pre-review checklist and request an API review. Status of requested reviews is tracked in the API Review project. |
|
|
||
| ### Add new conditions | ||
|
|
||
| In addition to changing existing conditions, this AEP also proposes to add new conditions. The purpose of this AEP is to retrofit conditions that should have always been there. The list is a work in progress and will be amended as new retrofitted conditions are required. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the list is a work in progress and will likely be updated, I think it is worth adding a Graduation Criteria section.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not really sure what that section would say.
What do we considered "done" here? What conditions do we need?
I'm also not sure if there's value in graduating these changes. Can conditions be alpha? Why would they be?
Do you have any ideas?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the first phase should focus on fixing the current conditions (aligning with SIG Architecture) and adding any new conditions that are required. Once we determine that no further conditions are needed, we can promote this to beta.
My main concern is that we might add or change a condition and later remove or modify it, which could break third-party consumers that are relying on that condition.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My main concern is that we might add or change a condition and later remove or modify it, which could break third-party consumers that are relying on that condition.
Since most of this enhancement is discussing expanding existing conditions (setting status: False) it's a change that does not require going through beta stage. Same thing applies to entirely new conditions. But I agree with @omerap12 about adding and later removing conditions - here I'd advice caution. It's better to not add a new condition, than add them prematurely and then remove it afterwards, which might potentially break users.
What type of PR is this?
/kind feature
/kind api-change
What this PR does / why we need it:
Created due to #8705
This PR adds an AEP to improve the VPA's status conditions to give users a better way to monitor their VPAs. It also allows the e2e tests to be improved with looking for a status condition, rather than sleeping and watching the end result.
Which issue(s) this PR fixes:
N/A
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: