feat: add HorizontalPodAutoscaler support for EnvoyProxy API#2257
feat: add HorizontalPodAutoscaler support for EnvoyProxy API#2257Xunzhuo merged 5 commits intoenvoyproxy:mainfrom
Conversation
63c8be5 to
3c73483
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #2257 +/- ##
==========================================
- Coverage 64.37% 64.33% -0.04%
==========================================
Files 112 112
Lines 15796 15874 +78
==========================================
+ Hits 10168 10213 +45
- Misses 4986 5011 +25
- Partials 642 650 +8 ☔ View full report in Codecov by Sentry. |
3c73483 to
f6ef209
Compare
6f89560 to
324c9e4
Compare
|
/retest |
215988e to
b437a60
Compare
There was a problem hiding this comment.
i think it's necessary to make labels configurable, not so rush in this PR, we can do it in the future
There was a problem hiding this comment.
no specific scenarios on my mind right now. it's not a strong option, can be considered if someone runs into it in the future.
internal/infrastructure/kubernetes/proxy/resource_provider_test.go
Outdated
Show resolved
Hide resolved
api/v1alpha1/shared_types.go
Outdated
There was a problem hiding this comment.
I don't think this's the right direction, at last, you need borrow everything from k8s.io.autoscaling.v2.HorizontalPodAutoScalerSpec.
There was a problem hiding this comment.
i borrow everything from HPASpec https://github.com/kubernetes/api/blob/6946e304d7b686796dc3f3bfaad56928484abf40/autoscaling/v2/types.go#L51, only exclude scaleTargetRef, because it will be automatically refers to the EnvoyProxy Deployment resource, it means i am trying to not let user specify it, because the deployment name is generated by EG. thought @zirain ?
There was a problem hiding this comment.
if we still provide scaleTargetRef, then technically user could specify unrelated resource than Envoy Proxy Deployment
There was a problem hiding this comment.
that's something I didn't wanted/expected, so I'm -1 on this.
I'd like to let other maintainers make the dicession.
There was a problem hiding this comment.
Current approach looks well, any better ideas, IMO HPA fields are hard to abstract.
b437a60 to
d92295a
Compare
|
/retest |
1 similar comment
|
/retest |
d92295a to
03c193d
Compare
Signed-off-by: Ardika Bagus <me@ardikabs.com>
Signed-off-by: Ardika Bagus <me@ardikabs.com>
Signed-off-by: Ardika Bagus <me@ardikabs.com>
Signed-off-by: Ardika Bagus <me@ardikabs.com>
cd2b31e to
3ad932e
Compare
|
/retest |
Xunzhuo
left a comment
There was a problem hiding this comment.
LGTM, I have tested locally and worked as expect.
What type of PR is this?
Add Horizontal Pod Autoscaler support for the Envoy Proxy deployment.
What this PR does / why we need it:
Here are the summaries of these changes:
envoyHpafield under EnvoyProxy API to enable Horizontal Pod Autoscaler.reflect.DeepEqualtocmp.Equalwith theIgnoreFieldhelper forreplicasfield once HorizontalPodAutoscaler was used while comparing the live object of the EnvoyProxy Deployment spec and the generated Envoy Proxy Deployment spec.ResourceRenderinterface, namedHorizontalPodAutoscaler()to fetch the HPA object from renderer if any.Which issue(s) this PR fixes:
Address #703