feat: add keda autoscaler for container and application service (only… #2265
Conversation
|
Qovery can create a Preview Environment for this PR.
This comment has been generated from Qovery AI 🤖.
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## staging #2265 +/- ##
===========================================
+ Coverage 47.45% 47.58% +0.13%
===========================================
Files 1249 1260 +11
Lines 22896 22967 +71
Branches 6703 6751 +48
===========================================
+ Hits 10865 10929 +64
+ Misses 9956 9937 -19
- Partials 2075 2101 +26
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
pggb25
force-pushed
the
feat/keda-cluster
branch
10 times, most recently
from
041a81e to
aed396c
Compare
pggb25
force-pushed
the
feat/keda-cluster
branch
7 times, most recently
from
bf0d6ba to
9d0dfd3
Compare
RemiBonnet
left a comment
RemiBonnet
left a comment
There was a problem hiding this comment.
Thanks Pierre 🙏
I added a few comments, but I think it would be better if I rewrite this part
The logic feels too complex for what are just small settings, and it currently affects some sensitive parts of the codebase. I think this logic should be isolated, as it is now, it’s hard to maintain and risky. Let’s talk about it tomorrow!
| if (onChange) onChange(e) | ||
| setCurrentValue(e.currentTarget.value) | ||
| }} | ||
| onKeyDown={(e) => { |
There was a problem hiding this comment.
You can't change UI component need to remove it
| * sanitizeKubernetesName('app__with--special!!!chars') // 'app-with-special-chars' | ||
| * sanitizeKubernetesName('-leading-and-trailing-') // 'leading-and-trailing' | ||
| */ | ||
| export function sanitizeKubernetesName(name: string): string { |
| autoscaling_scaler_type: __, | ||
| autoscaling_polling_interval: ___, | ||
| autoscaling_cooldown_period: ____, | ||
| scalers: _____, |
There was a problem hiding this comment.
Need to remove the fields field below?
| min_running_instances: number | ||
| max_running_instances: number | ||
|
|
||
| // Autoscaling mode selection |
There was a problem hiding this comment.
Can we have an specific interface for keda and extends this one? (you can probably extends from the api doc)
| </Section> | ||
|
|
||
| {!['JOB', 'TERRAFORM'].includes(service?.serviceType || '') && displayInstanceLimits && ( | ||
| <Section className="gap-4"> |
There was a problem hiding this comment.
For all "keda" part we need to have an field KedaSettings inside the domain services
| payload, | ||
| }, | ||
| { | ||
| onSuccess: () => { |
There was a problem hiding this comment.
Need to have an try catch instead of add it inside the onSuccess
|
|
||
| editAdvancedSettings({ | ||
| serviceId: service.id, | ||
| payload: advancedPayload as any, |
|
|
||
| // HPA fields | ||
| hpa_metric_type: (() => { | ||
| const settings = advancedSettings as Record<string, any> | undefined |
| return 'CPU' | ||
| })(), | ||
| hpa_cpu_average_utilization_percent: (() => { | ||
| const settings = advancedSettings as Record<string, any> | undefined |
| return 60 | ||
| })(), | ||
| hpa_memory_average_utilization_percent: (() => { | ||
| const settings = advancedSettings as Record<string, any> | undefined |
pggb25
force-pushed
the
feat/keda-cluster
branch
from
9d0dfd3 to
1f8d71f
Compare
pggb25
force-pushed
the
feat/keda-cluster
branch
5 times, most recently
from
c84918e to
48658e9
Compare
pggb25
force-pushed
the
feat/keda-cluster
branch
6 times, most recently
from
f4af56a to
b6ef302
Compare
pggb25
force-pushed
the
feat/keda-cluster
branch
from
b6ef302 to
ce41f2a
Compare
pggb25
force-pushed
the
feat/keda-cluster
branch
from
c03a9b5 to
7195bf1
Compare
RemiBonnet
left a comment
RemiBonnet
left a comment
There was a problem hiding this comment.
Thanks Pierre!
I added few comments and if you can all new files inside console-shared should be in your folder keda same for new files inside the util-services 🙏
| @@ -0,0 +1,88 @@ | |||
| import type { ScaledObjectStatusDto } from 'qovery-ws-typescript-axios/dist/api' | |||
There was a problem hiding this comment.
Could you add an folder keda and add files associated inside ?
keda
- scaled-object-status/scaled-object-status
- keda-settings/keda-settings
- autoscaling-payload/autoscaling-payload
| @@ -0,0 +1,88 @@ | |||
| import type { ScaledObjectStatusDto } from 'qovery-ws-typescript-axios/dist/api' | |||
There was a problem hiding this comment.
| import type { ScaledObjectStatusDto } from 'qovery-ws-typescript-axios/dist/api' | |
| import type { ScaledObjectStatusDto } from 'qovery-ws-typescript-axios' |
| import { isHelmGitSource, isHelmRepositorySource, isJobContainerSource, isJobGitSource } from '@qovery/shared/enums' | ||
| import { type ApplicationGeneralData, type JobGeneralData } from '@qovery/shared/interfaces' | ||
| import { joinArgsWithQuotes, parseCmd } from '@qovery/shared/util-js' | ||
| import { convertAutoscalingResponseToRequest } from '@qovery/shared/util-services' |
There was a problem hiding this comment.
See my comment below. It would be better to add it directly inside the keda folder.
Keeping a dedicated keda folder makes sense because we’re not sure about the future of this feature, the design may change, so having the file in a single place is safer and easier to maintain
| extractAndProcessAutoscaling, | ||
| } from './autoscaling-payload' | ||
|
|
||
| type TestAutoscalingRequest = { |
There was a problem hiding this comment.
You can't have a type here, should be import from the .ts and not the spec
| const gpu = Number(resourcesData['gpu']) | ||
| const variableImportRequest = prepareVariableImportRequest(variablesData) | ||
|
|
||
| // Parse KEDA autoscaling if autoscaling mode is KEDA (or legacy autoscaling_enabled for backward compatibility) |
There was a problem hiding this comment.
Could you create an util in the folder keda of your domain?
| ], | ||
| } | ||
|
|
||
| const result = convertAutoscalingResponseToRequest(response as any) |
There was a problem hiding this comment.
You need to remove all as any in your PR
| }) | ||
|
|
||
| it('should use default values for KEDA when not provided', () => { | ||
| const formData: any = { |
There was a problem hiding this comment.
You need to remove all any in your PR
| runningPods?: number | ||
| } | ||
|
|
||
| export function KedaSettings({ |
There was a problem hiding this comment.
console-shared will be deprecated so you need to add KedaSettings in a folder keda in the services. See the comment below !
| <div className="mb-5"> | ||
| <label className="mb-3 block text-sm font-medium text-neutral-400">Autoscaling metric</label> | ||
| <div className="flex flex-col gap-3"> | ||
| <InputRadio |
There was a problem hiding this comment.
This component InputRadio is deprecated you need to use RadioGroup
… qovery admin)
Summary
Issue:
Screenshots / Recordings
Testing
yarn testoryarn test -u(if you need to regenerate snapshots)yarn formatyarn lintPR Checklist
.cursor/rules)feat(service): add new Terraform service) - required for semantic-release