[HPA] Properly update HPA when an additional metric is added to the spec #1462
Conversation
|
@kevinearls @pavolloffay Could you please take a look when you get a chance? This deviates from the |
|
Hi @moh-osman3 sorry, but I'm no longer working on this project. |
|
@kevinearls No worries, thanks for letting me know. @pavolloffay would you have some time to review this PR? It fixes an issue with updating the HPA when a second metric is added to the CRD and reapplied to the cluster. Thanks! |
@iblancasa Thanks for the review! I added to the e2e tests to now check the number of metrics in the HPA Metric Spec. Then I install an update with the same name and an additional metric. Let me know if you have any more suggestions on how to improve the e2e test. |
moh-osman3
force-pushed
the
mohosman/issue-1439/fix-HPA-update
branch
6 times, most recently
from
b9520d6
to
a7ea3e8
Compare
moh-osman3
requested review from
pavolloffay and
iblancasa
and removed request for
pavolloffay
| apiVersion: opentelemetry.io/v1alpha1 | ||
| kind: OpenTelemetryCollector | ||
| metadata: | ||
| name: simplest-set-utilization |
There was a problem hiding this comment.
Before your changes, the test was done against the simplest OpenTelemetryCollector instance. That one is just to check the old behavior still works. But it would make more sense to do the test against the simplest-set-utilization OpenTelemetryCollector instance (and will simplify your test).
There was a problem hiding this comment.
Hmm is your recommendation to remove simplest collector entirely? I think I'd like to keep it around for now because it also confirms HPA is created with default targetCPUUtilization (i.e. scale up and scale down happens without having to explicitly set the cpu utilization because the default is used).
There was a problem hiding this comment.
Happy to continue on e2e tests improvements on my followup PR though, so let me know what you think!
There was a problem hiding this comment.
Hmm is your recommendation to remove simplest collector entirely? I think I'd like to keep it around for now because it also confirms HPA is created with default targetCPUUtilization (i.e. scale up and scale down happens without having to explicitly set the cpu utilization because the default is used).
Not removing it completely but not checking if it scales. Just checking if the new approach is the one that scales up and down and can be updated (as you did).
There was a problem hiding this comment.
Ah I see, sounds good to me! I now removed the tracegen and scaleup/scaledown assertions from the test for the simplest collector. Thanks for the suggestions @iblancasa !
moh-osman3
force-pushed
the
mohosman/issue-1439/fix-HPA-update
branch
from
1f00e33
to
0468733
Compare
…pec (open-telemetry#1462) * remove patch logic and replace with update request * test an update that adds another metric * minor fixes * rename chloggen * split tests up by version * use patch again instead of update * add error check * address review feedback * delete commented line * fix chloggen description * fix lint * simplify the logic - always replace the existing metrics with desired * fix lint * add to e2e tests * fix e2e bug * address review feedback * decrease stabilization window * cleanup * simplify e2e
Currently the HPA fails to update properly when a new metric is added. setAutoscalerSpec assumes the HPA spec structure will remain static when it is updated, but this is not the case when a new metric is added to the spec. Now the existing MetricSpec copies the desired MetricSpec directly so that any changes to the structure will also be copied.
Resolves #1439