Conversation
…into bucketwithscale
ghost
added
reviewed/ok-to-test
gardener-robot-ci-1
added
reviewed/ok-to-test
ggaurav10
force-pushed
the
bucketwithscale
branch
from
fefd653 to
6b1d221
Compare
gardener-robot-ci-2
added
the
reviewed/ok-to-test
gardener-robot-ci-1
removed
the
reviewed/ok-to-test
gardener-robot-ci-2
added
the
reviewed/ok-to-test
gardener-robot-ci-1
removed
the
reviewed/ok-to-test
ghost
added
the
reviewed/ok-to-test
gardener-robot-ci-1
removed
the
reviewed/ok-to-test
ghost
added
the
reviewed/ok-to-test
gardener-robot-ci-1
removed
the
reviewed/ok-to-test
…iners' VPA minAllowed values
ghost
added
the
reviewed/ok-to-test
ghost
added
reviewed/ok-to-test
amshuman-kr
left a comment
amshuman-kr
left a comment
There was a problem hiding this comment.
Thanks a lot @ggaurav10 for the substantial changes ❤️
The PR really looks good now!
Only some minor questions/suggestions 🙂
gardener-robot-ci-1
added
reviewed/ok-to-test
amshuman-kr
left a comment
amshuman-kr
left a comment
There was a problem hiding this comment.
Thanks @ggaurav10 for good test coverage! As discussed offline, the main code line looks good.
Just a few questions and perhaps a 3-4 more test cases to cover some other scenarios.
Also, would it be possible to use the relevant formula (or even a test utility function call) in the expected value rather than code it in as a constant? That might make the test cases more readable.
controllers/hvpa_controller_test.go
Outdated
| return h.Status.OverrideScaleUpStabilization | ||
| }, timeout).Should(BeTrue()) | ||
|
|
||
| // Update VPA status, let HVPA scale |
There was a problem hiding this comment.
Should we cover the update HPA and let HVPA scale case too?
There was a problem hiding this comment.
Also, can we include a couple of more entries where HPA and/or VPA deployment is disabled?
controllers/hvpa_controller_test.go
Outdated
| }, | ||
|
|
||
| Entry("UpdateMode Auto, Should Scale only memory", &data{ | ||
| Entry("UpdateMode Auto, scale up, paradoxical scaling, replicas increases, resources per replica decreases", &data{ |
There was a problem hiding this comment.
| Entry("UpdateMode Auto, scale up, paradoxical scaling, replicas increases, resources per replica decreases", &data{ | |
| Entry("UpdateMode Auto, scale up, paradoxical scaling, replicas increases, resources per replica decrease is blocked", &data{ |
| }, | ||
| }, | ||
| }), | ||
| Entry("UpdateMode Auto, overall scale up, paradoxical scaling, but replicas decrease - should not be considered paradoxical", &data{ |
There was a problem hiding this comment.
If I understood right, this case is where the current replicas and requests are such that they are not within the right effective scaling interval for the currentReplicas and the recommendation results in a scale up but the effective scaling interval has the desiredReplicas < currentReplicas. Is that correct?
There was a problem hiding this comment.
Related to this scenario, could we also cover change in scaling interval spec such that the current replicas and requests that were valid for the effective scaling interval earlier but are now no longer valid after the spec change (for no change in recommendations, scale up and down)?
There was a problem hiding this comment.
Is that correct?
Yes
Related to this scenario, could we also cover change in scaling interval spec such that the current replicas and requests that were valid for the effective scaling interval earlier but are now no longer valid after the spec change (for no change in recommendations, scale up and down)?
will add in next commit
controllers/hvpa_controller_test.go
Outdated
| }, | ||
| }), | ||
| Entry("UpdateMode Auto, Proportional scaling", &data{ | ||
| Entry("UpdateMode Auto, scale down hysteresis", &data{ |
There was a problem hiding this comment.
| Entry("UpdateMode Auto, scale down hysteresis", &data{ | |
| Entry("UpdateMode Auto, scale down hysteresis based on scaling intervals overlap", &data{ |
There was a problem hiding this comment.
Is the scale up side of this covered?
There was a problem hiding this comment.
Can we add a couple of cases (scale up and down) without scaling interval gaps configured?
There was a problem hiding this comment.
scale down hysteresis due to ScalingIntervalsOverlap is covered in this case. scale up hysteresis is provided by minChange which is covered in test case - UpdateMode Auto, scale up blocked due to minChange
There was a problem hiding this comment.
scale up hysteresis is provided by minChange which is covered in test case - UpdateMode Auto, scale up blocked due to minChange
Understood. But I meant a specific case for scale up hysteresis where minChange doesn't block. I.e. scale up happens within the current bucket even though the next bucket's min total resources is higher than the scaled up total resources. WDYT?
There was a problem hiding this comment.
ah ok.. sure. will add.
gardener-robot-ci-1
added
the
reviewed/ok-to-test
ghost
removed
the
reviewed/ok-to-test
|
|
gardener-robot
added
lifecycle/stale
gardener-robot
added
size/xl
ghost
added
the
reviewed/ok-to-test
gardener-robot-ci-1
removed
the
reviewed/ok-to-test
gardener-robot-ci-1
added
the
reviewed/ok-to-test
gardener-robot-ci-2
removed
the
reviewed/ok-to-test
gardener-robot
added
lifecycle/rotten
|
/close in favour of #88 |
7 participants
What this PR does / why we need it:
hvpa redesign
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Refer this for design details
Release note: