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.

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

docs: add details about HPA via KEDA #9202

Merged
merged 1 commit into from Nov 23, 2021
Merged

docs: add details about HPA via KEDA #9202

merged 1 commit into from Nov 23, 2021

Conversation

thotz
Copy link
Contributor

@thotz thotz commented Nov 18, 2021

Description of your changes:
By default HPA can use details about memory or CPU consumption for
autoscaling, but also it can use custom metrics as well. There are alot
provides supports HPA via customer and one of them is KEDA project. Here
it is done with help of Prometheus Scaler

Signed-off-by: Jiffin Tony Thottan thottanjiffin@gmail.com
Which issue is resolved by this Pull Request:
Resolves #

Checklist:

  • Commit Message Formatting: Commit titles and messages follow guidelines in the developer guide.
  • Skip Tests for Docs: Add the flag for skipping the build if this is only a documentation change. See here for the flag.
  • Skip Unrelated Tests: Add a flag to run tests for a specific storage provider. See test options.
  • Reviewed the developer guide on Submitting a Pull Request
  • Documentation has been updated, if necessary.
  • Unit tests have been added, if necessary.
  • Integration tests have been added, if necessary.
  • Pending release notes updated with breaking and/or notable changes, if necessary.
  • Upgrade from previous release is tested and upgrade user guide is updated, if necessary.
  • Code generation (make codegen) has been run to update object specifications, if necessary.

@thotz thotz requested a review from travisn November 18, 2021 10:40
Documentation/ceph-monitoring.md Outdated Show resolved Hide resolved
@@ -210,3 +210,30 @@ labels:
prometheus: k8s
[...]
```
Copy link
Member

Choose a reason for hiding this comment

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

It would be nice to have a table of contents at the top so new topics like this can be more easily discovered, but that is really a separate question from this PR.

Documentation/ceph-monitoring.md Outdated Show resolved Hide resolved
Documentation/ceph-monitoring.md Outdated Show resolved Hide resolved
Documentation/ceph-monitoring.md Show resolved Hide resolved
Copy link
Member

@travisn travisn left a comment

Choose a reason for hiding this comment

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

The doc looks good, the yaml linter just has a few whitespace issues to fix.

@thotz
Copy link
Contributor Author

thotz commented Nov 22, 2021

The doc looks good, the yaml linter just has a few whitespace issues to fix.

@travisn : May I skip validate-yaml for the keda.yaml file :

diff --git a/tests/scripts/github-action-helper.sh b/tests/scripts/github-action-helper.sh
index 25b717af8..aea4f1325 100755
--- a/tests/scripts/github-action-helper.sh
+++ b/tests/scripts/github-action-helper.sh
@@ -132,8 +132,8 @@ function build_rook_all() {
 function validate_yaml() {
   cd cluster/examples/kubernetes/ceph
   kubectl create -f crds.yaml -f common.yaml
-  # skipping folders and some yamls that are only for openshift.
-  manifests="$(find . -maxdepth 1 -type f -name '*.yaml' -and -not -name '*openshift*' -and -not -name 'scc*')"
+  # skipping folders and some yamls that are only for openshift, keda etc.
+  manifests="$(find . -maxdepth 1 -type f -name '*.yaml' -and -not -name '*openshift*' -and -not -name 'scc*' -and -not -name 'keda*')"
   with_f_arg="$(echo "$manifests" | awk '{printf " -f %s",$1}')" # don't add newline

Otherwise I keda need to be installed by deafult

Copy link
Member

@travisn travisn left a comment

Choose a reason for hiding this comment

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

@thotz Instead of skipping the yaml validation, how about following the pattern we use for the volume replication CRDs and install them as part of the validate_yaml() test?

cluster/examples/kubernetes/ceph/keda-sample-rgw.yaml Outdated Show resolved Hide resolved
By default HPA can use details about memory or CPU consumption for
autoscaling, but also it can use custom metrics as well. There are alot
provides supports HPA via customer and one of them is KEDA project. Here
it is done with help of Prometheus Scaler

Signed-off-by: Jiffin Tony Thottan <thottanjiffin@gmail.com>
@travisn travisn merged commit d340bcc into rook:master Nov 23, 2021
mergify bot added a commit that referenced this pull request Nov 24, 2021
docs: add details about HPA via KEDA (backport #9202)
TomHellier pushed a commit to TomHellier/rook that referenced this pull request Nov 24, 2021
docs: add details about HPA via KEDA (backport rook#9202)
Signed-off-by: Tom Hellier <me@tomhellier.com>
@thotz thotz deleted the hpakedadoc branch November 25, 2021 05:36
Copy link
Member

@BlaineEXE BlaineEXE left a comment

Choose a reason for hiding this comment

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

@thotz did you manually test that Rook RGWs support automatic scale-out like this?

I am concerned that this won't really work with Rook, because any time the CephObjectStore is reconciled, Rook will reset the replicas to 1.

Comment on lines +176 to +180
#create the KEDA CRDS
keda_version=2.4.0
keda_url="https://github.com/kedacore/keda/releases/download/v${keda_version}/keda-${keda_version}.yaml"
kubectl apply -f "${keda_url}"

Copy link
Member

@BlaineEXE BlaineEXE Apr 5, 2022

Choose a reason for hiding this comment

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

@thotz Why was this included (to install KEDA) but no integration test written to test KEDA integration with Rook RGWs?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants