[Federation] Update hpa e2e utils to enable reuse in fed hpa tests#51277
[Federation] Update hpa e2e utils to enable reuse in fed hpa tests#51277k8s-github-robot merged 2 commits intokubernetes:masterfrom
Conversation
|
/sig federation |
| func NewDynamicResourceConsumer(name, kind string, replicas, initCPUTotal, initMemoryTotal, initCustomMetric int, cpuLimit, memLimit int64, f *framework.Framework) *ResourceConsumer { | ||
| return newResourceConsumer(name, kind, replicas, initCPUTotal, initMemoryTotal, initCustomMetric, dynamicConsumptionTimeInSeconds, | ||
| dynamicRequestSizeInMillicores, dynamicRequestSizeInMegabytes, dynamicRequestSizeCustomMetric, cpuLimit, memLimit, f) | ||
| func NewDynamicResourceConsumer(name, nsName, kind string, replicas, initCPUTotal, initMemoryTotal, initCustomMetric int, cpuLimit, memLimit int64, clientset clientset.Interface, internalClientset *internalclientset.Clientset) *ResourceConsumer { |
There was a problem hiding this comment.
I don't really like the idea of flattening the Framework parameter and passing the individual clientsets instead. Can you convince me otherwise?
There was a problem hiding this comment.
The use of framework here is to consume only the cluster clientset, cluster internalclientset and namespace. The resource consumer, the controller rc that controls the resource consumer and communications across using local service discovery, need only these 3 for their correct functioning. The e2e framework, however has hell a lot many things, including loggers, piping/redirection of stuff, their own beforeeach/aftereach (and many more things I did not explore). The fw we create and use in fed e2e tests is the extention of the e2e fw with subtle differences (for example it stores fed clientset, and clientsets for all clusters). The easier way for me to reuse these rc utils to drive the fed test was to create the resource consumer directly into the specific cluster, I want the utilisation driven into and create a matching object (deployment/rs) in federation to control the distribution. I did explore creating a dummy fw with only the cluster clientset, cluster internalclientset and namespace for that particular cluster set, and use that instead (rather then flattening it); but was afraid of some inbuilt suff on that fw (for example beforeeach/aftereach) interfering with the actual fed e2e fw which is used to drive the fed e2e test.
Meanwhile, I think using the dummy fw, as mentioned above might work here, however, this somehow felt the right way to make these utils not really depend on it.
There was a problem hiding this comment.
OK, fair enough. I accept your argument. Given that, the rest of the PR LGTM.
| controllerName: name + "-ctrl", | ||
| kind: kind, | ||
| framework: f, | ||
| nsName: nsName, |
There was a problem hiding this comment.
Can you explain why you decided to do this? Is it because Federation e2e tests don't use Framework? If that is the case, it seems better to fix that instead?
|
/lgtm Would anyone in @kubernetes/sig-autoscaling-pr-reviews like to take a quick look at this? Seems pretty uncontentious to me. If I don't hear any objections in the next few days, I'd like to get this merged in time for v1.8. |
|
/test all |
|
/retest |
1 similar comment
|
/retest |
I will. Seems pretty reasonable to me. /lgtm |
|
/approve no-issue |
|
/assign @madhusudancs |
|
/test pull-kubernetes-e2e-gce-bazel |
|
/retest |
1 similar comment
|
/retest |
|
/approve no-issue |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: DirectXMan12, irfanurrehman, quinton-hoole, spiffxp Associated issue requirement bypassed by: spiffxp The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
|
/test all [submit-queue is verifying that this PR is safe to merge] |
|
Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.. |
This PR is to enable reuse of some hpa e2e utils in federation, facilitating #50168
cc @mwielgus @quinton-hoole
cc @kubernetes/sig-federation-pr-reviews
Release note: