Add integration tests for recommender #9156
Conversation
Configure a struct that can be passed around, and populate that with CLI flags
Since it requires setup
k8s-ci-robot
added
the
release-note-none
k8s-ci-robot
added
kind/cleanup
|
[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 |
k8s-ci-robot
added
approved
| // HamsterTargetRef is CrossVersionObjectReference of hamster app | ||
| var HamsterTargetRef = &autoscaling.CrossVersionObjectReference{ | ||
| APIVersion: "apps/v1", | ||
| Kind: "Deployment", | ||
| Name: "hamster-deployment", | ||
| } |
There was a problem hiding this comment.
This is moved out of the e2e directory.
I didn't want to give the integration tests its own go.mod file, and I didn't want to make the VPA depend on things inside the e2e tests, so handle that, I moved this out of e2e so that it could be used in the integration tests.
|
/hold |
k8s-ci-robot
added
the
do-not-merge/hold
maxcao13
left a comment
maxcao13
left a comment
There was a problem hiding this comment.
I feel like we could move the flag refactoring to a separate PR to merge first, before we introduce this new test suite. It seems to me that we are doing both things in one PR, which is a little hard to review, but for the record I do like the changes :-)
| } | ||
|
|
||
| // ValidateCommonConfig performs validation of the common flags | ||
| func ValidateCommonConfig(config *CommonFlags) { |
There was a problem hiding this comment.
This seems to be only used in the admission controller right now.
| } | ||
|
|
||
| // Configure the recommender to watch only the watched namespace | ||
| config := recommender_config.DefaultRecommenderConfig() |
There was a problem hiding this comment.
Some of the stuff in each test seems repeatable. Since we use ginkgo in the regular e2es, can we re-use it here and have some sort of beforeEach which will setup the configurations for the component under test?
| // ValidateAdmissionControllerConfig performs validation of the admission-controller flags | ||
| func ValidateAdmissionControllerConfig(config *AdmissionControllerConfig) { | ||
| if len(config.CommonFlags.VpaObjectNamespace) > 0 && len(config.CommonFlags.IgnoredVpaObjectNamespaces) > 0 { | ||
| klog.ErrorS(nil, "--vpa-object-namespace and --ignored-vpa-object-namespaces are mutually exclusive and can't be set together.") | ||
| klog.FlushAndExit(klog.ExitFlushTimeout, 1) | ||
| } | ||
| } |
There was a problem hiding this comment.
Do we still need this? I think we have that common controller config validation right?
| _ = prometheus.Register(usageRecommendationRelativeDiff) | ||
| _ = prometheus.Register(usageMissingRecommendationCounter) |
There was a problem hiding this comment.
Just curious why the changes here?
3 participants
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
Currently the VPA doesn't have "integration" tests, which allows us to test various CLI flags to each component. The purpose of this change is to set the foundation to allow us to add integration tests.
The idea here is that we can write tests that execute faster without needing an entire Kubernetes cluster to validate these changes.
In theory this change also pushes us closer to the modern patterns that other controllers are using.
This PR only works on the updater, but if it makes sense to make similar changes to the admission-controller or updater, I'm happy to do so (I think the config change does make sense for all components).
This PR also highlighted that we use stop channels and contexts in some places where we could get away with only the context. That's something we can fix in future PRs.
Once this is accepted, I also need to figure out how to add this to CI.
Which issue(s) this PR fixes:
Relates to #8934
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:
/cc @maxcao13 @omerap12
I don't have much envtest experience, so happy to make adjustments if there are better ways of handling any of this