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 to v2beta2 API for HPA #2968

Merged
merged 1 commit into from Nov 10, 2020
Merged

Update to v2beta2 API for HPA #2968

merged 1 commit into from Nov 10, 2020

Conversation

qui
Copy link
Contributor

@qui qui commented Nov 7, 2020

The behavior field for HPA, which allows you to specify limits on scaling down, is in the v2beta2 API. This PR just updates the current code to use that API. I'll follow up with a separate PR to add the limits.

Here are the API docs I used: https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.19/#horizontalpodautoscaler-v2beta2-autoscaling

I think I've tested this for all the possible HPA branches with setup_kubernetes_job on kubestage, which means the objects are valid, but it would still be good to check that the new API requests seem equivalent to the old ones. Here are the autoscaling sections I tested:

  autoscaling:
    decision_policy: proportional
    metrics_provider: http
    setpoint: 0.4

  autoscaling:
    decision_policy: proportional
    metrics_provider: http
    setpoint: 0.4
    forecast_policy: moving_average

  autoscaling:
    metrics_provider: mesos_cpu

  horizontal_autoscaling:
    min_replicas: 3
    max_replicas: 15
    uwsgi:
      target_average_value: 0.5
      dimensions:
          service: blah

  horizontal_autoscaling:
    min_replicas: 3
    max_replicas: 15
    uwsgi:
      target_average_value: 0.5

  horizontal_autoscaling:
    min_replicas: 3
    max_replicas: 15
    cpu:
      target_average_value: 0.5

  horizontal_autoscaling:
    min_replicas: 3
    max_replicas: 15
    test-metric:
      target_value: 1
      signalflow_metrics_query: "const(value=1).publish()"

Also note this will cause a big bounce. Here's the config diff:

--- before	2020-11-06 16:33:36.570583229 -0800
+++ after	2020-11-06 16:37:27.717282484 -0800
@@ -8,12 +8,11 @@
               'finalizers': None,
               'generate_name': None,
               'generation': None,
-              'initializers': None,
-              'labels': {'paasta.yelp.com/config_sha': 'config3836dd19',
+              'labels': {'paasta.yelp.com/config_sha': 'config33f02c03',
                          'paasta.yelp.com/git_sha': 'abcdef',
                          'paasta.yelp.com/instance': 'instance',
                          'paasta.yelp.com/service': 'service',
-                         'yelp.com/paasta_config_sha': 'config3836dd19',
+                         'yelp.com/paasta_config_sha': 'config33f02c03',
                          'yelp.com/paasta_git_sha': 'abcdef',
                          'yelp.com/paasta_instance': 'instance',
                          'yelp.com/paasta_service': 'service'},
@@ -45,12 +44,11 @@
                                     'finalizers': None,
                                     'generate_name': None,
                                     'generation': None,
-                                    'initializers': None,
-                                    'labels': {'paasta.yelp.com/config_sha': 'config3836dd19',
+                                    'labels': {'paasta.yelp.com/config_sha': 'config33f02c03',
                                                'paasta.yelp.com/git_sha': 'abcdef',
                                                'paasta.yelp.com/instance': 'instance',
                                                'paasta.yelp.com/service': 'service',
-                                               'yelp.com/paasta_config_sha': 'config3836dd19',
+                                               'yelp.com/paasta_config_sha': 'config33f02c03',
                                                'yelp.com/paasta_git_sha': 'abcdef',
                                                'yelp.com/paasta_instance': 'instance',
                                                'yelp.com/paasta_service': 'service'},
@@ -145,6 +143,7 @@
                                                                            'ephemeral-storage': '1024Mi',
                                                                            'memory': '4096Mi'}},
                                                 'security_context': None,
+                                                'startup_probe': None,
                                                 'stdin': None,
                                                 'stdin_once': None,
                                                 'termination_message_path': None,
@@ -156,6 +155,7 @@
                                 'dns_config': None,
                                 'dns_policy': None,
                                 'enable_service_links': None,
+                                'ephemeral_containers': None,
                                 'host_aliases': None,
                                 'host_ipc': None,
                                 'host_network': None,
@@ -165,6 +165,8 @@
                                 'init_containers': None,
                                 'node_name': None,
                                 'node_selector': {'yelp.com/pool': 'default'},
+                                'overhead': None,
+                                'preemption_policy': None,
                                 'priority': None,
                                 'priority_class_name': None,
                                 'readiness_gates': None,
@@ -178,5 +180,6 @@
                                 'subdomain': None,
                                 'termination_grace_period_seconds': None,
                                 'tolerations': None,
+                                'topology_spread_constraints': None,
                                 'volumes': []}}},
  'status': None}

@qui qui requested review from EvanKrall and stug November 7, 2020 01:01
Copy link
Contributor

@stug stug left a comment

Choose a reason for hiding this comment

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

I am pretty sure this looks right, but I found that I was going a little cross-eyed trying to keep everything straight in my head.

Comment on lines -194 to -209
# For detail, https://github.com/kubernetes-client/python/issues/553
# This hack should be removed when the issue got fixed.
# This is no better way to work around rn.
class MonkeyPatchAutoScalingConditions(V2beta1HorizontalPodAutoscalerStatus):
@property
def conditions(self) -> Sequence[V2beta1HorizontalPodAutoscalerCondition]:
return super().conditions()

@conditions.setter
def conditions(
self, conditions: Optional[Sequence[V2beta1HorizontalPodAutoscalerCondition]]
) -> None:
self._conditions = list() if conditions is None else conditions


models.V2beta1HorizontalPodAutoscalerStatus = MonkeyPatchAutoScalingConditions
Copy link
Contributor

Choose a reason for hiding this comment

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

That issue looks like it's still open, but it sounds like you actually created HPAs on kubestage so I am assuming 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.

Yeah, the issue is only in the v2beta1 client spec I think. I am also assuming creating HPAs on kubestage proves that this issue is no longer relevant with the new API.

@qui qui merged commit 5fbe24e into master Nov 10, 2020
kawaiwanyelp added a commit that referenced this pull request Nov 11, 2020
This reverts commit 5fbe24e, reversing
changes made to 1ca29e3.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants