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

Update autoscaling/v1 to autoscaling/v2beta2 #1071

Merged
merged 2 commits into from Nov 13, 2020

Conversation

pmalek-sumo
Copy link
Contributor

@pmalek-sumo pmalek-sumo commented Nov 6, 2020

Description

Update autoscaling/v1 to autoscaling/v2beta2 in order to enable custom metrics autoscaling and autoscaling based on memory utilization.

This PR adds targetMemoryUtilizationPercentage key to enable autoscaling based on memory utilization.

ref: https://kubernetes.io/docs/tasks/run-application/horizontal-pod-autoscale-walkthrough/#autoscaling-on-multiple-metrics-and-custom-metrics

Testing performed
  • ci/build.sh
  • Redeploy fluentd and fluentd-events pods
  • Confirm events, logs, and metrics are coming in

@sumo-drosiek
Copy link
Contributor

@frankreno @perk-sumo
Do we want to support more factors than CPU? IMHO, we should avoid to copy k8s templates to values.yaml if it's possible

@frankreno
Copy link
Contributor

I agree with @sumo-drosiek. I feel like we should be able to handle this w/o a breaking change as well. In the values.yaml we can have the same key we do today (targetCPUUtilizationPercentage). We can modify the HPA with new format in the template as opposed to the values. If customers want to add more metrucs, we can add an extraHPAMetrics property that the template could render no?

@pmalek-sumo
Copy link
Contributor Author

I agree with @sumo-drosiek. I feel like we should be able to handle this w/o a breaking change as well. In the values.yaml we can have the same key we do today (targetCPUUtilizationPercentage). We can modify the HPA with new format in the template as opposed to the values. If customers want to add more metrucs, we can add an extraHPAMetrics property that the template could render no?

@frankreno

The reason I'm hesitant on this approach is that

  • we would be "overriding" the "scope" (?) of targetCPUUtilizationPercentage (v1 vs v2beta) - this in itself wouldn't be that big of a deal
  • we already had a thread on slack about a customer who'd like to configure hpa based on memory, this will require either extraHPAMetrics as suggested or something like targetMemoryUtilizationPercentage which doesn't exist neither in v1 nor in v2beta2 which might confuse users

Not my call, just chippin' in :)

@perk-sumo
Copy link
Contributor

There is one problem with doing a breaking change and fully migrating - it's still a beta version.
I'd rather have some "scope" inconsistence than changing our configuration back and forth in case the hpa api changes.

@pmalek-sumo
Copy link
Contributor Author

There is one problem with doing a breaking change and fully migrating - it's still a beta version.
I'd rather have some "scope" inconsistence than changing our configuration back and forth in case the hpa api changes.

Ok, sure. Let's do that then.
One thing though: do we set the default and if so then what value do we want for targetMemoryUtilizationPercentage?

@perk-sumo
Copy link
Contributor

Let's set it at 50% but not use it by default.

Copy link
Contributor

@perk-sumo perk-sumo left a comment

Choose a reason for hiding this comment

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

Let's not change the values.yaml keys around autoscaling just yet.

@pmalek-sumo
Copy link
Contributor Author

Removing breaking-change label as we decided to use targetMemoryUtilizationPercentage key for autoscaling based on memory utilization

@pmalek-sumo
Copy link
Contributor Author

@perk-sumo Please take a look one more time

Copy link
Contributor

@perk-sumo perk-sumo left a comment

Choose a reason for hiding this comment

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

👍

@perk-sumo perk-sumo added this to the v2.0 milestone Nov 13, 2020
@pmalek-sumo pmalek-sumo merged commit 70b2a17 into master Nov 13, 2020
@pmalek-sumo pmalek-sumo deleted the update-autoscaling-v1-to-v2beta2 branch November 13, 2020 09:13
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.

Migrate hpa from autoscaling/v1 to autoscaling/v2beta2
5 participants