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 49.14% 47.53% -1.61%
===========================================
Files 811 1263 +452
Lines 16526 23002 +6476
Branches 4854 6757 +1903
===========================================
+ Hits 8121 10935 +2814
- Misses 6880 9964 +3084
- Partials 1525 2103 +578
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)
...ib/application-settings/ui/application-settings-resources/application-settings-resources.tsx
Show resolved
Hide resolved
| 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
10 times, most recently
from
7ec2455 to
7a94741
Compare
pggb25
force-pushed
the
feat/keda-cluster
branch
4 times, most recently
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
| <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
| ) : null} | ||
|
|
||
| {/* Mode NONE: Fixed instances */} | ||
| {autoscalingMode === 'NONE' && ( |
There was a problem hiding this comment.
@pggb25 Could you use ts-pattern's match method instead ? That way we're sure all cases are covered.
...ib/application-settings/ui/application-settings-resources/application-settings-resources.tsx
Show resolved
Hide resolved
| ): Record<string, unknown> { | ||
| const autoscalingMode = formData['autoscaling_mode'] || 'NONE' | ||
|
|
||
| switch (autoscalingMode) { |
There was a problem hiding this comment.
@pggb25 Instead of a switch case, can you please use ts-pattern's match function 🙏
| const autoscalingMode = request.autoscaling_mode | ||
|
|
||
| // KEDA mode: use scalers | ||
| if (autoscalingMode === 'KEDA' && request.scalers?.length) { |
There was a problem hiding this comment.
@pggb25 Here, I'd suggest to use ts-pattern's match function as well, instead of a type-unsafe condition
pggb25
force-pushed
the
feat/keda-cluster
branch
8 times, most recently
from
c57c64a to
900321c
Compare
| const isProduction = environmentMode === EnvironmentModeEnum.PRODUCTION | ||
| const hasLowMinimum = (minRunningInstances ?? 0) <= 1 | ||
|
|
pggb25
force-pushed
the
feat/keda-cluster
branch
from
900321c to
5dbde4f
Compare
… 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