Skip to content

Update the Kubernetes autoscaling API to v2 (#2722) #2842

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Oct 11, 2023

Conversation

Lunik
Copy link
Contributor

@Lunik Lunik commented Oct 6, 2023

Description

This PR update the Kubernetes Autoscaling API to v2.

This allow support for Kubernetes cluster in version v1.26 and latest. However, this drop the support for Kubernetes clusters in version under v1.23

Which issue(s) this PR fixes:

Fixes #2722
Fixes #2838

Testing

  • Unitests with ./hack/runtests.sh
  • Builded the fission/fission-bundle image locally with goreleaser and replaced the executor image
  • Deployed the following function :
fission env create --name nodejs --image fission/node-env --poolsize 1 --version 3
fission function create --name hello-js --env nodejs --code  functions/hello.js --minscale=0 --maxscale=5 --executortype newdeploy
  • Verify :
kubectl version

Client Version: version.Info{Major:"1", Minor:"27", GitVersion:"v1.27.2", GitCommit:"7f6f68fdabc4df88cfea2dcf9a19b2b830f1e647", GitTreeState:"clean", BuildDate:"2023-05-17T14:20:07Z", GoVersion:"go1.20.4", Compiler:"gc", Platform:"darwin/arm64"}
Kustomize Version: v5.0.1
Server Version: version.Info{Major:"1", Minor:"27", GitVersion:"v1.27.2", GitCommit:"7f6f68fdabc4df88cfea2dcf9a19b2b830f1e647", GitTreeState:"clean", BuildDate:"2023-05-17T14:13:28Z", GoVersion:"go1.20.4", Compiler:"gc", Platform:"linux/arm64"}

kubectl get hpa
NAME                                                     REFERENCE                                                           TARGETS         MINPODS   MAXPODS   REPLICAS   AGE
newdeploy-hello-js-fission-functions-b6d5-29977d82419c   Deployment/newdeploy-hello-js-fission-functions-b6d5-29977d82419c   <unknown>/80%   1         5         1          2m24s

Checklist:

  • I ran tests as well as code linting locally to verify my changes.
  • I have done manual verification of my changes, changes working as expected.
  • I have added new tests to cover my changes.
  • My changes follow contributing guidelines of Fission.
  • I have signed all of my commits.

@codecov
Copy link

codecov bot commented Oct 9, 2023

Codecov Report

Merging #2842 (b874136) into main (b43b318) will not change coverage.
The diff coverage is 66.66%.

@@           Coverage Diff           @@
##             main    #2842   +/-   ##
=======================================
  Coverage   20.76%   20.76%           
=======================================
  Files          68       68           
  Lines        7372     7372           
=======================================
  Hits         1531     1531           
  Misses       5727     5727           
  Partials      114      114           
Flag Coverage Δ
unittests 20.76% <66.66%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
pkg/fission-cli/cmd/function/create.go 27.25% <100.00%> (ø)
pkg/utils/otel/attributes.go 0.00% <0.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@sanketsudake
Copy link
Member

@Lunik You will need to update auto generated files.

make all-generators

It updates deep copy file and CRD definition for the function.

 crds/v1/fission.io_functions.yaml         | 13 +++++++------
 pkg/apis/core/v1/zz_generated.deepcopy.go |  6 +++---
 2 files changed, 10 insertions(+), 9 deletions(-)

@Lunik
Copy link
Contributor Author

Lunik commented Oct 9, 2023

This should looks good with ac7f53b

Copy link
Member

@sanketsudake sanketsudake left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also need to replace all
.AutoscalingV2beta2() calls with .AutoscalingV2()

These are referred via kubernetes client

-> % ack ".AutoscalingV2()"
pkg/executor/util/hpa/hpa.go
123:			existingHpa, err = hpaops.kubernetesClient.AutoscalingV2beta2().HorizontalPodAutoscalers(depl.ObjectMeta.Namespace).Update(ctx, existingHpa, metav1.UpdateOptions{})
132:		cHpa, err := hpaops.kubernetesClient.AutoscalingV2beta2().HorizontalPodAutoscalers(depl.ObjectMeta.Namespace).Create(ctx, hpa, metav1.CreateOptions{})
135:				cHpa, err = hpaops.kubernetesClient.AutoscalingV2beta2().HorizontalPodAutoscalers(depl.ObjectMeta.Namespace).Get(ctx, hpaName, metav1.GetOptions{})
148:	return hpaops.kubernetesClient.AutoscalingV2beta2().HorizontalPodAutoscalers(ns).Get(ctx, name, metav1.GetOptions{})
152:	_, err := hpaops.kubernetesClient.AutoscalingV2beta2().HorizontalPodAutoscalers(hpa.ObjectMeta.Namespace).Update(ctx, hpa, metav1.UpdateOptions{})
157:	return hpaops.kubernetesClient.AutoscalingV2beta2().HorizontalPodAutoscalers(ns).Delete(ctx, name, metav1.DeleteOptions{})

pkg/executor/reaper/reaper.go
59:		err := kubeClient.AutoscalingV2beta2().HorizontalPodAutoscalers(kubeobj.Namespace).Delete(ctx, kubeobj.Name, metav1.DeleteOptions{})
183:		hpaList, err := client.AutoscalingV2beta2().HorizontalPodAutoscalers(namespace).List(ctx, listOps)
196:				err := client.AutoscalingV2beta2().HorizontalPodAutoscalers(hpa.ObjectMeta.Namespace).Delete(ctx, hpa.ObjectMeta.Name, metav1.DeleteOptions{})

pkg/fission-cli/cmd/support/resources/kubernetes.go
119:		objs, err := res.client.AutoscalingV2beta2().HorizontalPodAutoscalers(metav1.NamespaceAll).List(ctx, metav1.ListOptions{LabelSelector: res.selector})

@sanketsudake
Copy link
Member

Also please rebase the PR as I have updated Kubernetes version in CI.

Lunik added 3 commits October 10, 2023 23:44
- k8s.io/api/autoscaling/v2

Signed-off-by: Lunik <lunik@tiwabbit.fr>
Signed-off-by: Lunik <lunik@tiwabbit.fr>
Signed-off-by: Lunik <lunik@tiwabbit.fr>
@Lunik Lunik force-pushed the fix/2722-update-autoscaling-api-v2 branch from ac7f53b to b874136 Compare October 10, 2023 21:47
Copy link
Member

@sanketsudake sanketsudake left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM code changes.

We will need change CI versions again in a separate PR and do couple of changes in the CI.

Thanks for raising PR 🎉

@sanketsudake sanketsudake merged commit 3762ff8 into fission:main Oct 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants