feat: add HorizontalPodAutoscaler support for EnvoyProxy API #2257
Conversation
ardikabs
force-pushed
the
feat/hpaSpec
branch
from
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. |
ardikabs
force-pushed
the
feat/hpaSpec
branch
from
3c73483 to
f6ef209
Compare
ardikabs
force-pushed
the
feat/hpaSpec
branch
2 times, most recently
from
6f89560 to
324c9e4
Compare
|
/retest |
ardikabs
force-pushed
the
feat/hpaSpec
branch
from
215988e to
b437a60
Compare
| }, | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Namespace: r.Namespace, | ||
| Name: r.Name(), |
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
| @@ -275,3 +276,38 @@ const ( | |||
| // https://github.com/google/re2/wiki/Syntax. | |||
| StringMatchRegularExpression StringMatchType = "RegularExpression" | |||
| ) | |||
|
|
|||
| // KubernetesHorizontalPodAutoscalerSpec defines Kubernetes Horizontal Pod Autoscaler settings of Envoy Proxy Deployment | |||
| type KubernetesHorizontalPodAutoscalerSpec struct { | |||
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.
ardikabs
force-pushed
the
feat/hpaSpec
branch
from
b437a60 to
d92295a
Compare
|
/retest |
1 similar comment
|
/retest |
ardikabs
force-pushed
the
feat/hpaSpec
branch
from
d92295a to
03c193d
Compare
Signed-off-by: Ardika Bagus <[email protected]>
Signed-off-by: Ardika Bagus <[email protected]>
Signed-off-by: Ardika Bagus <[email protected]>
Signed-off-by: Ardika Bagus <[email protected]>
ardikabs
force-pushed
the
feat/hpaSpec
branch
from
cd2b31e to
3ad932e
Compare
|
/retest |
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