Skip to content
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.

Sign up for GitHub

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

Jump to bottom

support cpu/memory scaler #1215

Merged
merged 7 commits into from
Oct 13, 2020
Merged

support cpu/memory scaler #1215

merged 7 commits into from
Oct 13, 2020

Conversation

silenceper
Copy link
Contributor

@silenceper silenceper commented Oct 3, 2020 •
edited
Loading

Signed-off-by: silenceper [email protected]

  • Provide CPU/Memory scaler
  • remove horizontalPodAutoscalerConfig.resourceMetrics
apiVersion: keda.sh/v1alpha1
kind: ScaledObject
metadata:
  name: cron-scaledobject
  namespace: default
spec:
  scaleTargetRef:
    name: my-deployment
  advanced:                                          # Optional. Section to specify advanced options
    restoreToOriginalReplicaCount: true/false        # Optional. Default: false
    horizontalPodAutoscalerConfig:                   # Optional. Section to specify HPA related options
      behavior:                                      # Optional. Use to modify HPA's scaling behavior
        scaleDown:
          stabilizationWindowSeconds: 300
          policies:
          - type: Percent
            value: 100
            periodSeconds: 15
  triggers:
  - type: cpu/memory                           # provider cpu/memory scaler
    metadata:
      type: value/ utilization/ averagevalue
      value: 60

Checklist

  • Commits are signed with Developer Certificate of Origin (DCO)
  • Tests have been added
  • A PR is opened to update the documentation on add cpu/memory scaler keda-docs#277
  • Changelog has been updated

Fixes #1183

@silenceper silenceper mentioned this pull request Oct 3, 2020
Merged
@silenceper silenceper marked this pull request as ready for review October 3, 2020 04:03
zroubalik
zroubalik reviewed Oct 5, 2020
View reviewed changes
Copy link
Member

@zroubalik zroubalik left a comment

Choose a reason for hiding this comment

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

Could you please add tests for this scaler? (unit tests for sure and e2d would be nice to have)

CHANGELOG.md Show resolved Hide resolved
pkg/scalers/cpu_memory_scaler.go Outdated Show resolved Hide resolved
pkg/scalers/cpu_memory_scaler.go Show resolved Hide resolved
@silenceper
Copy link
Contributor Author

PTAL @zroubalik

zroubalik
zroubalik reviewed Oct 12, 2020
View reviewed changes
Copy link
Member

@zroubalik zroubalik left a comment

Choose a reason for hiding this comment

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

minor nits

CHANGELOG.md Show resolved Hide resolved
pkg/scaling/scale_handler.go Show resolved Hide resolved
zroubalik
zroubalik reviewed Oct 12, 2020
View reviewed changes
pkg/scaling/scale_handler.go
@@ -254,7 +264,9 @@ func (h *scaleHandler) checkScaledJobScalers(ctx context.Context, scalers []scal
scalerLogger.Info("Scaler is active")
}
}
maxValue = min(scaledJob.MaxReplicaCount(), devideWithCeil(queueLength, targetAverageValue))
if targetAverageValue != 0 {
Copy link
Member

Choose a reason for hiding this comment

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

@TsuyoshiUshio could you please check, that this is ok?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added this judgment to avoid that if only the cpu/memory scaler is used in the ScalerJobs, the targetAverageValue is 0 (of course this is meaningless), which causes the devideWithCeil method to panic

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @zroubalik @silenceper
It looks good to me. TargetAverageValue is should be bigger than 1. In case of 0, that means no scale. So that This logic is correct. Thank you for adding the condition!

Copy link
Member

Choose a reason for hiding this comment

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

Thanks @TsuyoshiUshio!

zroubalik
zroubalik approved these changes Oct 12, 2020
View reviewed changes
Copy link
Member

@zroubalik zroubalik left a comment

Choose a reason for hiding this comment

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

LGTM

TsuyoshiUshio
TsuyoshiUshio approved these changes Oct 12, 2020
View reviewed changes
Copy link
Contributor

@TsuyoshiUshio TsuyoshiUshio left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I approve, LGTM. However Can I clarify one thing? @silenceper
According to the code, the usage of cpu/memory scaler is for HPA only, right? Not for ScaledJob. I mean you add resourcemetricNames on ScaledOjbect only. If so we might consider documentation for it. Thank you for your contribution.

@silenceper
Copy link
Contributor Author

silenceper commented Oct 13, 2020 •
edited
Loading

@TsuyoshiUshio
docs was add in kedacore/keda-docs#277

@tomkerkhove
Copy link
Member

Looks like we are good to go, right @zroubalik @TsuyoshiUshio ?

@zroubalik
Copy link
Member

@silenceper thanks for the contribution!

@zroubalik zroubalik merged commit a17ac2a into kedacore:v2 Oct 13, 2020
@silenceper silenceper deleted the f-resource-scaler branch October 13, 2020 11:37
@knepe knepe mentioned this pull request Oct 20, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zzxwill zzxwill zzxwill left review comments

@TsuyoshiUshio TsuyoshiUshio TsuyoshiUshio approved these changes

@zroubalik zroubalik zroubalik approved these changes

@ahmelsayed ahmelsayed Awaiting requested review from ahmelsayed

Assignees

@ahmelsayed ahmelsayed

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Provide CPU/Memory scaler
6 participants
@silenceper @tomkerkhove @zroubalik @TsuyoshiUshio @zzxwill @ahmelsayed