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

Removing triggers from ScaledObject doesn't reflect in the HPA it manages #1699

Closed
bishoybassem opened this issue Mar 25, 2021 · 0 comments · Fixed by #1768
Closed

Removing triggers from ScaledObject doesn't reflect in the HPA it manages #1699

bishoybassem opened this issue Mar 25, 2021 · 0 comments · Fixed by #1768
Labels
bug Something isn't working

Comments

@bishoybassem
Copy link

Report

I have a ScaledObject with several rabbitmq triggers, and when deleting one of them from the SO definition, changes are not propagated to the underlying HPA. Adding triggers work fine though.

Example SO:

apiVersion: keda.sh/v1alpha1
kind: ScaledObject
metadata:
  name: consumer
  namespace: keda2
spec:
  advanced:
    horizontalPodAutoscalerConfig:
      behavior:
        scaleDown:
          stabilizationWindowSeconds: 60
        scaleUp:
          stabilizationWindowSeconds: 60
  scaleTargetRef:
    name: consumer
  pollingInterval: 5
  minReplicaCount: 1
  maxReplicaCount: 10
  triggers:
  - type: rabbitmq
    metadata:
      protocol: amqp
      queueName: keda-tests
      queueLength: "5"
    authenticationRef:
      name: keda-trigger-auth-rabbitmq-conn
  - type: rabbitmq
    metadata:
      protocol: amqp
      queueName: keda-tests2
      queueLength: "5"
    authenticationRef:
      name: keda-trigger-auth-rabbitmq-conn

which creates the following HPA:

kubectl get hpa keda-hpa-consumer
NAME                REFERENCE             TARGETS                MINPODS   MAXPODS   REPLICAS   AGE
keda-hpa-consumer   Deployment/consumer   0/5 (avg), 0/5 (avg)   1         10        1          13s

modify the SO and delete the 2nd trigger:

apiVersion: keda.sh/v1alpha1
kind: ScaledObject
metadata:
  name: consumer
  namespace: keda2
spec:
  advanced:
    horizontalPodAutoscalerConfig:
      behavior:
        scaleDown:
          stabilizationWindowSeconds: 60
        scaleUp:
          stabilizationWindowSeconds: 60
  scaleTargetRef:
    name: consumer
  pollingInterval: 5
  minReplicaCount: 1
  maxReplicaCount: 10
  triggers:
  - type: rabbitmq
    metadata:
      protocol: amqp
      queueName: keda-tests
      queueLength: "5"
    authenticationRef:
      name: keda-trigger-auth-rabbitmq-conn

check the HPA:

kubectl get hpa keda-hpa-consumer
NAME                                  REFERENCE                               TARGETS                        MINPODS   MAXPODS   REPLICAS   AGE
keda-hpa-consumer                     Deployment/consumer                     0/5 (avg), <unknown>/5 (avg)   1         10        1          15m

notice that KEDA's metrics server is indeed not exposing metrics for the trigger that I deleted, hence the "unknown" current value, and can be confirmed in the events:

Events:
  Type     Reason                   Age                   From                       Message
  ----     ------                   ----                  ----                       -------
  Warning  FailedGetExternalMetric  24s (x26 over 6m47s)  horizontal-pod-autoscaler  unable to get external metric keda2/rabbitmq-keda-tests2/&LabelSelector{MatchLabels:map[string]string{scaledObjectName: consumer,},MatchExpressions:[]LabelSelectorRequirement{},}: unable to fetch metrics from external metrics API: No matching metrics found for rabbitmq-keda-tests2

Expected Behavior

Keda operator should propagate all SO modifications to the underlying HPA

Actual Behavior

The HPA stays with old triggers, which could cause unexpected behaviors.

Steps to Reproduce the Problem

  1. create a SO with multiple triggers
  2. check SO & HPA resources
  3. delete one of the SO triggers and apply
  4. check SO (doesn't show the deleted trigger) & HPA resources (still contains the deleted metric)

Logs from KEDA operator

2021-03-25T08:44:34.107Z	INFO	controllers.ScaledObject	Creating a new HPA	{"ScaledObject.Namespace": "keda2", "ScaledObject.Name": "consumer", "HPA.Namespace": "keda2", "HPA.Name": "keda-hpa-consumer"}
2021-03-25T08:44:34.225Z	INFO	controllers.ScaledObject	Initializing Scaling logic according to ScaledObject Specification	{"ScaledObject.Namespace": "keda2", "ScaledObject.Name": "consumer"}
2021-03-25T08:44:34.298Z	INFO	controllers.ScaledObject	Reconciling ScaledObject	{"ScaledObject.Namespace": "keda2", "ScaledObject.Name": "consumer"}
2021-03-25T08:44:39.012Z	INFO	controllers.ScaledObject	Reconciling ScaledObject	{"ScaledObject.Namespace": "keda2", "ScaledObject.Name": "consumer"}
2021-03-25T08:54:54.408Z	INFO	controllers.ScaledObject	Reconciling ScaledObject	{"ScaledObject.Namespace": "keda2", "ScaledObject.Name": "consumer"}
2021-03-25T08:54:54.427Z	INFO	controllers.ScaledObject	Initializing Scaling logic according to ScaledObject Specification	{"ScaledObject.Namespace": "keda2", "ScaledObject.Name": "consumer"}
2021-03-25T08:55:06.532Z	INFO	controllers.ScaledObject	Reconciling ScaledObject	{"ScaledObject.Namespace": "keda2", "ScaledObject.Name": "consumer"}

KEDA Version

2.2.0

Kubernetes Version

1.18

Platform

Any

Scaler Details

RabbitMQ

@bishoybassem bishoybassem added the bug Something isn't working label Mar 25, 2021
coderanger added a commit to coderanger/keda that referenced this issue Apr 30, 2021
coderanger added a commit to coderanger/keda that referenced this issue Apr 30, 2021
…rray.

DeepDerivative considers it fine if the found HPA has extra elements in the array as long as the ones that are there in the computed HPA match the found. In our case, that causes updating the HPA to be skipped if you remove the last trigger (or any number as long as they are at the end). Checking the lengths first is quick and should cover all of these wonky cases.

Fixes kedacore#1699.
coderanger added a commit to coderanger/keda that referenced this issue Apr 30, 2021
…dacore#1699.

Test is currently failing, as expected.

Signed-off-by: Noah Kantrowitz <noah@coderanger.net>
coderanger added a commit to coderanger/keda that referenced this issue Apr 30, 2021
…rray.

DeepDerivative considers it fine if the found HPA has extra elements in the array as long as the ones that are there in the computed HPA match the found. In our case, that causes updating the HPA to be skipped if you remove the last trigger (or any number as long as they are at the end). Checking the lengths first is quick and should cover all of these wonky cases.

Fixes kedacore#1699.

Signed-off-by: Noah Kantrowitz <noah@coderanger.net>
coderanger added a commit to coderanger/keda that referenced this issue May 4, 2021
…dacore#1699.

Test is currently failing, as expected.

Signed-off-by: Noah Kantrowitz <noah@coderanger.net>
coderanger added a commit to coderanger/keda that referenced this issue May 4, 2021
…rray.

DeepDerivative considers it fine if the found HPA has extra elements in the array as long as the ones that are there in the computed HPA match the found. In our case, that causes updating the HPA to be skipped if you remove the last trigger (or any number as long as they are at the end). Checking the lengths first is quick and should cover all of these wonky cases.

Fixes kedacore#1699.

Signed-off-by: Noah Kantrowitz <noah@coderanger.net>
zroubalik pushed a commit that referenced this issue May 4, 2021
…array (#1768)

* 🎨 Revive the functional test harness and add a regression test for #1699.

Test is currently failing, as expected.

Signed-off-by: Noah Kantrowitz <noah@coderanger.net>

* 🐛 Fix incorrect handling when removing from the end of the triggers array.

DeepDerivative considers it fine if the found HPA has extra elements in the array as long as the ones that are there in the computed HPA match the found. In our case, that causes updating the HPA to be skipped if you remove the last trigger (or any number as long as they are at the end). Checking the lengths first is quick and should cover all of these wonky cases.

Fixes #1699.

Signed-off-by: Noah Kantrowitz <noah@coderanger.net>

* 📝 Update CHANGELOG.

Signed-off-by: Noah Kantrowitz <noah@coderanger.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant