ssa: apply hpa drift detection fix to autoscaling/v2 #449
Conversation
ssa/utils.go
Outdated
| @@ -448,6 +448,7 @@ func fixHorizontalPodAutoscaler(object *unstructured.Unstructured) error { | |||
| if object.GetKind() == "HorizontalPodAutoscaler" { | |||
| switch object.GetAPIVersion() { | |||
| case "autoscaling/v2beta2": | |||
| case "autoscaling/v2": | |||
| var d hpav2beta2.HorizontalPodAutoscaler | |||
There was a problem hiding this comment.
Do we want to continue to use the hpav2beta2 here instead of moving to hpav2?
There was a problem hiding this comment.
Good catch! I think we can do hpav2beta2 for autoscaling/v2beta2 and then hpav2 for autoscaling/v2. It would mean duplicating the code in the two switch cases. wdyt?
hiddeco
added
area/server-side-apply
Makefile
Outdated
| @@ -17,7 +17,7 @@ GO_TEST_ARGS ?= -race | |||
| ENVTEST_ARCH ?= amd64 | |||
|
|
|||
| # Kubernetes versions to use envtest with | |||
| ENVTEST_KUBERNETES_VERSION?=1.25 | |||
| ENVTEST_KUBERNETES_VERSION?=latest | |||
There was a problem hiding this comment.
This may break our attempt to have hermetic builds. fluxcd/flux2#3323 has previous discussion around it.
There was a problem hiding this comment.
I will pin it to 1.26.0
ssa/utils.go
Outdated
| @@ -21,6 +21,7 @@ import ( | |||
| "encoding/json" | |||
| "fmt" | |||
| "io" | |||
| hpav2 "k8s.io/api/autoscaling/v2" | |||
There was a problem hiding this comment.
This can be moved below with the other third party dependencies.
| @@ -427,6 +428,17 @@ func SetNativeKindsDefaults(objects []*unstructured.Unstructured) error { | |||
| return fmt.Errorf("%s validation error: %w", FmtUnstructured(u), err) | |||
| } | |||
| u.Object = out | |||
| case "autoscaling/v2": | |||
There was a problem hiding this comment.
Third case with almost the same code. Maybe it's time to write a common function for it, unless it has any known performance impact?
Something like
func setHPADefaults(u *unstructured.Unstructured, obj interface{}) errorand pass the typed variable d as an empty interface.
There was a problem hiding this comment.
The loop uses a specific type
var metrics []hpav2.MetricSpec # we use hpav2beta2 for autoscaling/v2beta2
```.
Should we default all to []hpav2.MetricSpec?
There was a problem hiding this comment.
That's for fixHorizontalPodAutoscaler(). The above suggestion is for SetNativeKindsDefaults().
I think something similar can be done for fixHorizontalPodAutoscaler() as well, but may not be pretty 😄 Maybe give it a try, multiple common functions that return empty interface or some generic type. Already sounds a bit complicated. But there may be some simple way to do it.
There was a problem hiding this comment.
Ah, sorry I missed that.
I attempted for fixHorizontalPodAutoscaler. It wasn't pretty but I will give it another shot.
There was a problem hiding this comment.
Although I am not sure what default is being set for HPA in that function. The HPA part seems to just convert to and from the typed struct while the rest set a missing default. The tests pass when I take out the switch case for the HPA in SetNativeKindsDefaults
There was a problem hiding this comment.
If the tests continue to pass, maybe we are missing some test case for it?
There was a problem hiding this comment.
Not sure what should be tested, since the function isn't setting/unsetting any values for HPA kinds. @/stefanprodan is probably the best person to say
There was a problem hiding this comment.
The previous workaround for HPA bug in #207 isn't related to SetNativeKindsDefaults(). And the first time these defaults were added for HPA was in #160 without any comment or test for HPA itself. An update to it was in #165 which was due to some incompatibility in the HPA API, no tests or any explicitly documented reason around it.
If the v2beta2 API and v2 API are compatible, I think adding a new case for v2 HPA in SetNativeKindsDefaults() may not be needed. The actual fix for the issue that this PR is addressing may be enough with the change in fixHorizontalPodAutoscaler().
There was a problem hiding this comment.
The missing int to string conversion is not specific to PodSpec, that's why I used the conversion for HPA too.
There was a problem hiding this comment.
We can figure out if this is still the case for HPA or it was a Kubernetes 1.19 bug only, @somtochiama has created an issue here #454 so I'll merge this as it is and we can remove code in another PR.
Signed-off-by: Somtochi Onyekwere <somtochionyekwere@gmail.com>
somtochiama
force-pushed
the
v2-autoscaling
branch
from
bc5b042
to
60dda9e
Compare
Fixes: #437
Fixes: fluxcd/flux2#3277
The drift detection fix introduced here: #207 was only applied to
autoscaling/v2beta2.This PR adds the
autoscaling/v2apiVersion since the same issue still persists and updates client-go to1.26Signed-off-by: Somtochi Onyekwere somtochionyekwere@gmail.com