Refactor recommender to follow NewXXXController pattern#9462
Refactor recommender to follow NewXXXController pattern#9462adrianmoisey wants to merge 2 commits intokubernetes:masterfrom
Conversation
|
[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 |
9f62c17 to
3c00d17
Compare
There was a problem hiding this comment.
This doesn't run under CI for now, that needs to be hooked up in the future.
3c00d17 to
7f1f64a
Compare
There was a problem hiding this comment.
general question for now, I will look at the code in more detail later:
I assume if we are trying to emulate kubernetes and we still have the plan of eventually making VPA components business loops event-driven, then we will eventually adopt their pattern of controllers and reconcilers, instead of something like controller-runtime + envtest.
Long term, I don't have a strong opinion either way. That's possibly something we should figure out. I don't have any controller-runtime experience. The reason I went this direction for this PR was that I spent time familiarising myself with the patterns in k/k, and the path to getting this done in VPA became more obvious (ie: this PR). What I'm not sure about is if this PR moves us away from controller-runtime + envtest in the future, or potentially towards that. /hold |
|
/cc iamzili |
And include a script to run integration tests only. It requires etcd to be installed.
7f1f64a to
085711e
Compare
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
This is my third attempt at #8934
This PR replaces #9156
I spent some time looking at k/k's controllers, and tried to copy that pattern. This PR isn't perfect, I think in theory any informer should be created outside of
NewRecommenderController()and passed in. For VPA that's quite a lot of informers, so I tried to keep this slim.I also tried to keep this change minimal, I plan to make more follow up PRs for the other 2 VPA components, and more tests.
I also decided against envtest, since I was looking at k/k patterns.
I also only made 1 test as an example, I plan to fill those out later.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
AI assisted
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: