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

HPA Status Conditions #46550

Merged

Conversation

DirectXMan12
Copy link
Contributor

This PR introduces conditions to the status of the HorizontalPodAutoscaler (in autoscaling/v2alpha1).
The conditions whether or not the autoscaler is actively scaling, and why. This gives greater visibility
into the current status of the autoscaler, similarly to how conditions work for pods, nodes, etc.

kubectl describe has been updated to the display the conditions affecting a given HPA.

Implements kubernetes/enhancements#264 (alpha in 1.7)

Release note:

Introduces status conditions to the HorizontalPodAutoscaler in autoscaling/v2alpha1, indicating the current status of a given HorizontalPodAutoscaler, and why it is or is not scaling.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 26, 2017
@k8s-github-robot k8s-github-robot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels May 26, 2017
@DirectXMan12
Copy link
Contributor Author

cc @kubernetes/sig-autoscaling-pr-reviews

@k8s-ci-robot k8s-ci-robot added the sig/autoscaling Categorizes an issue or PR as relevant to SIG Autoscaling. label May 26, 2017
@piosz piosz assigned mwielgus and unassigned piosz May 29, 2017
@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 29, 2017
@DirectXMan12 DirectXMan12 force-pushed the feature/hpa-status-conditions branch from 170d968 to 9019cb2 Compare May 30, 2017 22:18
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 30, 2017
@DirectXMan12 DirectXMan12 force-pushed the feature/hpa-status-conditions branch from 9019cb2 to b24ee9a Compare May 30, 2017 22:35
@@ -2529,6 +2529,13 @@ func describeHorizontalPodAutoscaler(hpa *autoscaling.HorizontalPodAutoscaler, e
w.Write(LEVEL_0, "failed to check Replication Controller\n")
}
}
if len(hpa.Status.Conditions) > 0 {
w.Write(LEVEL_0, "Conditions:\n Type\tStatus\tReason\tMessage\n")
Copy link
Contributor

Choose a reason for hiding this comment

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

Why LEVEL_0 is used for the header but LEVEL_1 is used for the content?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The table is slightly indented, TBH, this is mostly copied from another one of the printers. I'll fix it up a bit.

Copy link
Contributor

Choose a reason for hiding this comment

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

CC @kubernetes/sig-cli-misc

@@ -202,6 +202,62 @@ type HorizontalPodAutoscalerStatus struct {

// CurrentMetrics is the last read state of the metrics used by this autoscaler.
CurrentMetrics []MetricStatus

// conditions is the set of conditions required for this autoscaler to scale its target,
Copy link
Contributor

Choose a reason for hiding this comment

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

s/conditions/Conditions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

// HorizontalPodAutoscalerCondition describes the state of
// a HorizontalPodAutoscaler at a certain point.
type HorizontalPodAutoscalerCondition struct {
// type describes the current condition
Copy link
Contributor

Choose a reason for hiding this comment

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

s/type/Type? (here and in all other comments in this struct)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

public API types use lowercase starting characters in their godoc.

// HorizontalPodAutoscalerCondition describes the state of
// a HorizontalPodAutoscaler at a certain point.
type HorizontalPodAutoscalerCondition struct {
// type describes the current condition
Copy link
Contributor

Choose a reason for hiding this comment

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

s/type/Type? (here and in all other comments in this struct)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

public API types use lowercase starting characters in their godoc.

// HorizontalPodAutoscalerCondition describes the state of
// a HorizontalPodAutoscaler at a certain point.
type HorizontalPodAutoscalerCondition struct {
// type describes the current condition
Copy link
Contributor

Choose a reason for hiding this comment

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

s/type/Type? (here and in all other comments in this struct)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed


// conditions is the set of conditions required for this autoscaler to scale its target,
// and indicates whether or not those conditions are met.
Conditions []HorizontalPodAutoscalerCondition
Copy link
Contributor

@jszczepkowski jszczepkowski May 31, 2017

Choose a reason for hiding this comment

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

(optional) Have you consider using here map: ConditionType -> HorizontalPodAutoscalerCondition?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

a) I wanted to follow the model of the rest of the conditions objects, and
b) maps are not recommended as per the Kubernetes API conventions.

func setConditionInList(inputList []autoscalingv2.HorizontalPodAutoscalerCondition, conditionType autoscalingv2.HorizontalPodAutoscalerConditionType, status v1.ConditionStatus, reason, message string, args ...interface{}) []autoscalingv2.HorizontalPodAutoscalerCondition {
resList := inputList
var existingCond *autoscalingv2.HorizontalPodAutoscalerCondition
for i, condition := range resList {
Copy link
Contributor

@jszczepkowski jszczepkowski May 31, 2017

Choose a reason for hiding this comment

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

If inputList is a map, it will be simpler and faster.

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 realize that, but then you have to convert from list to map and back to list. This way, we can operate directly on the API object field.

@@ -247,6 +249,7 @@ func (a *HorizontalController) computeReplicasForMetrics(hpa *autoscalingv2.Hori
replicaCountProposal, utilizationProposal, timestampProposal, err = a.replicaCalc.GetObjectMetricReplicas(currentReplicas, metricSpec.Object.TargetValue.MilliValue(), metricSpec.Object.MetricName, hpa.Namespace, &metricSpec.Object.Target)
if err != nil {
a.eventRecorder.Event(hpa, v1.EventTypeWarning, "FailedGetObjectMetric", err.Error())
setCondition(hpa, autoscalingv2.CanComputeReplicas, v1.ConditionFalse, "FailedGetObjectMetric", "the HPA was unable to compute the replica count: %v", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: the message "the HPA was unable to compute the replica count" seems to be redundant: it has the same meaning as condition type

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 figured I'd err on the side of being a bit more verbose for the human-readable message.

@@ -383,10 +395,12 @@ func (a *HorizontalController) reconcileAutoscaler(hpav1Shared *autoscalingv1.Ho

rescale := true

// TODO: conditions for current below/above?
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't understand, can you elaborate?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

whoops, that escaped cleanup when it shouldn't have. I was pondering whether or not to have a condition indicating that the current replica count was outside the scale limits, but I decided against it for now.

if scale.Spec.Replicas == 0 {
// Autoscaling is disabled for this resource
desiredReplicas = 0
rescale = false
// TODO: put a condition for this...
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't understand, can you elaborate?

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 was considering a condition for "ScalingDisabled" or somesuch.

}

if rescale {
scale.Spec.Replicas = desiredReplicas
_, err = a.scaleNamespacer.Scales(hpa.Namespace).Update(hpa.Spec.ScaleTargetRef.Kind, scale)
if err != nil {
a.eventRecorder.Eventf(hpa, v1.EventTypeWarning, "FailedRescale", "New size: %d; reason: %s; error: %v", desiredReplicas, rescaleReason, err.Error())
setCondition(hpa, autoscalingv2.CanAccessScale, v1.ConditionFalse, "FailedUpdate", "the HPA controller was unable to update the target scale: %v", err)
Copy link
Contributor

@jszczepkowski jszczepkowski May 31, 2017

Choose a reason for hiding this comment

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

We have "FailedRescale" event, which for me is clearer than "FailedUpdated".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's for symmetry with the other reason for failure -- FailedGet (for when we fail to get the scale) vs FailedUpdate (for when we fail to update the scale).

return fmt.Errorf("failed to rescale %s: %v", reference, err)
}
setCondition(hpa, autoscalingv2.CanAccessScale, v1.ConditionTrue, "SucceededUpdate", "the HPA controller was able to update the target scale to %d", desiredReplicas)
Copy link
Contributor

Choose a reason for hiding this comment

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

We have "SuccessfulRescale" event, which for me is clearer than "SuccessfulUpdated".

if desiredReplicas == 0 {
switch {
case desiredReplicas > scaleUpLimit:
setCondition(hpa, autoscalingv2.DesiredOutsideRange, v1.ConditionTrue, "ScaleUpLimit", "the desired replica count increasing faster than the maximum scale rate")
Copy link
Contributor

Choose a reason for hiding this comment

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

Optional (here and in all cases): consider adding desired replicas values (before and after modification) to the condition message.

// clear the message so that we can easily compare
for i := range actualConditions {
actualConditions[i].Message = ""
// TODO: check this?
Copy link
Contributor

Choose a reason for hiding this comment

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

?

}

func TestConditionInvalidSelector(t *testing.T) {

Copy link
Contributor

Choose a reason for hiding this comment

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

?

@jszczepkowski
Copy link
Contributor

This PR will need approves for kubectl and API.

@DirectXMan12
Copy link
Contributor Author

@smarterclayton it's alpha (part of v2alpha1) in 1.7

@smarterclayton
Copy link
Contributor

/approve

Under those terms. If @Kargakis thinks this is a progressing condition type we can change that post code freeze or in beta

@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 1, 2017
@DirectXMan12
Copy link
Contributor Author

@jszczepkowski any comments, or is this good to go?

@DirectXMan12
Copy link
Contributor Author

/assign @derekwaynecarr

@derekwaynecarr
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 1, 2017
@k8s-ci-robot
Copy link
Contributor

@falenn: changing LGTM is restricted to assignees, and only kubernetes org members may be assigned issues.

In response to this:

This message was created automatically by mail delivery software.

A message that you sent could not be delivered to one or more of its
recipients. This is a temporary error. The following address(es) deferred:

[email protected]
Domain imwiz.com has exceeded the max emails per hour (165/150 (110%)) allowed. Message will be reattempted later

------- This is a copy of the message, including all the headers. ------
Received: from o3.sgmail.github.com ([192.254.112.98]:38451)
by box969.bluehost.com with esmtps (TLSv1.2:ECDHE-RSA-AES128-GCM-SHA256:128)
(Exim 4.87)
(envelope-from [email protected])
id 1dGU7g-000Q72-Nt
for [email protected]; Thu, 01 Jun 2017 11:43:01 -0600
DKIM-Signature: v=1; a=rsa-sha1; c=relaxed/relaxed; d=github.com;
h=from:reply-to:to:cc:in-reply-to:references:subject:mime-version:content-type:content-transfer-encoding:list-id:list-archive:list-post:list-unsubscribe;
s=s20150108; bh=mcqEUz55eVSD0nigTa5K5kAY2+Y=; b=k9oSqvPNI0Z3xlpY
bQ48DbtsLxbMszW7k45wKUdZs10q/CORClSLF1fW2iP3JYVSLj1UX5OwQ4cPWu65
d9f/eMHMzbVh0o4SB7Wh+DLK/+R5hYUl9R2Z3BgAhgHzKQT52aPV5QOEilX+RH1K
nA+wzaKSP3ZUjueBFnrmkELXxkg=
Received: by filter1156p1mdw1.sendgrid.net with SMTP id filter1156p1mdw1-14109-593051EC-48
2017-06-01 17:42:04.675683417 +0000 UTC
Received: from github-smtp2a-ext-cp1-prd.iad.github.net (github-smtp2a-ext-cp1-prd.iad.github.net [192.30.253.16])
by ismtpd0005p1iad1.sendgrid.net (SG) with ESMTP id dIRUwIavR1-MYa_ApySh5A
for [email protected]; Thu, 01 Jun 2017 17:42:04.589 +0000 (UTC)
Date: Thu, 01 Jun 2017 10:42:04 -0700
From: Derek Carr [email protected]
Reply-To: kubernetes/kubernetes [email protected]
To: kubernetes/kubernetes [email protected]
Cc: Subscribed [email protected]
Message-ID: kubernetes/kubernetes/pull/46550/[email protected]
In-Reply-To: kubernetes/kubernetes/pull/[email protected]
References: kubernetes/kubernetes/pull/[email protected]
Subject: Re: [kubernetes/kubernetes] HPA Status Conditions (#46550)
Mime-Version: 1.0
Content-Type: multipart/alternative;
boundary="--==_mimepart_593051ec7c958_26113f8cea11fc2c6477d";
charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: list
X-GitHub-Sender: derekwaynecarr
X-GitHub-Recipient: falenn
X-GitHub-Reason: subscribed
List-ID: kubernetes/kubernetes <kubernetes.kubernetes.github.com>
List-Archive: https://github.com/kubernetes/kubernetes
List-Post: mailto:[email protected]
List-Unsubscribe: mailto:unsub+000ab60ab2fa03aa402f92a4f2324706c1cead717bfc218f92cf00000001154813ec92a169ce0dd05adc@reply.github.com,
https://github.com/notifications/unsubscribe/AAq2Cl9RPWa3eJB5JmH8bI_whKaXMyuYks5r_vfsgaJpZM4NoKYJ
X-Auto-Response-Suppress: All
X-GitHub-Recipient-Address: [email protected]
X-SG-EID: APO41b8ovafPb3SK9rw3vFLViRpqytTOR28jbslbz7Drq7SpjaGkvYLqW1CLoaF7eXBmYDRYGgmqaC
CZtGyDh5CATev/tCmmpVh18sly1XLEo0lxiy/IcnKhEZsvY/i9ksxpUItF/98D89iNAi6pdZ7I6vNW
GvNtIw1S49fBGq5URuMI7UfXM5lMenqEFPPeTHW1JU+UqZQN0X6BJm+8qw==
X-Spam-Status: No, score=-2.8
X-Spam-Score: -27
X-Spam-Bar: --
X-Ham-Report: Spam detection software, running on the system "box969.bluehost.com",
has NOT identified this incoming email as spam. The original
message has been attached to this so you can view it or label
similar future email. If you have any questions, see
root@localhost for details.

Content preview: /lgtm -- You are receiving this because you are subscribed
to this thread. Reply to this email directly or view it on GitHub: #46550 (comment)
[...]

Content analysis details: (-2.8 points, 4.0 required)

pts rule name description


-0.0 RP_MATCHES_RCVD Envelope sender domain matches handover relay domain
-0.0 SPF_PASS SPF: sender matches SPF record
0.0 HTML_MESSAGE BODY: HTML included in message
0.7 HTML_IMAGE_ONLY_20 BODY: HTML: images with 1600-2000 bytes of words
-0.1 DKIM_VALID_AU Message has a valid DKIM or DK signature from author's
domain
-0.1 DKIM_VALID Message has at least one valid DKIM or DK signature
0.1 DKIM_SIGNED Message has a DKIM or DK signature, not necessarily valid
-2.8 RCVD_IN_MSPIKE_H2 RBL: Average reputation (+2)
[192.254.112.98 listed in wl.mailspike.net]
-0.6 AWL AWL: Adjusted score from AWL reputation of From: address
X-Spam-Flag: NO

----==_mimepart_593051ec7c958_26113f8cea11fc2c6477d
Content-Type: text/plain;
charset=UTF-8
Content-Transfer-Encoding: 7bit

/lgtm

--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
#46550 (comment)
----==_mimepart_593051ec7c958_26113f8cea11fc2c6477d
Content-Type: text/html;
charset=UTF-8
Content-Transfer-Encoding: 7bit

/lgtm


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.

<script type="application/json" data-scope="inboxmarkup">{"api_version":"1.0","publisher":{"api_key":"05dde50f1d1a384dd78767c55493e4bb","name":"GitHub"},"entity":{"external_key":"github/kubernetes/kubernetes","title":"kubernetes/kubernetes","subtitle":"GitHub repository","main_image_url":"https://cloud.githubusercontent.com/assets/143418/17495839/a5054eac-5d88-11e6-95fc-7290892c7bb5.png","avatar_image_url":"https://cloud.githubusercontent.com/assets/143418/15842166/7c72db34-2c0b-11e6-9aed-b52498112777.png","action":{"name":"Open in GitHub","url":"https://github.com/kubernetes/kubernetes"}},"updates":{"snippets":[{"icon":"PERSON","message":"@derekwaynecarr in #46550: /lgtm"}],"action":{"name":"View Pull Request","url":"https://github.com//pull/46550#issuecomment-305567021"}}}</script>

----==_mimepart_593051ec7c958_26113f8cea11fc2c6477d--

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@DirectXMan12
Copy link
Contributor Author

@k8s-bot pull-kubernetes-federation-e2e-gce test this

@calebamiles calebamiles modified the milestone: v1.7 Jun 2, 2017
@jszczepkowski
Copy link
Contributor

LGTM

@mikedanese
Copy link
Member

@k8s-bot test this

This commit adds the new API status conditions to the API types.
The field exists as a field in autoscaling/v2alpha1, and is
round-tripped through an annotation in autoscaling/v1.
@DirectXMan12 DirectXMan12 force-pushed the feature/hpa-status-conditions branch from ba7705d to c8fdeb0 Compare June 5, 2017 15:21
This commit causes the HPA controller to set a variety of status
conditions using the new `Status.Conditions` field of
autoscaling/v2alpha1.  These provide insight into the current state
of the HPA, and generally correspond to similar events being emitted.
This commit updates `kubectl describe` to display the new HPA
status conditions.  This should make it easier for users to discern
the current state of the HPA.
This commit updates the generated autoscaling files to be up-to-date
with the HPA status condition changes.
@DirectXMan12
Copy link
Contributor Author

rebased and fix the test issue caused by the rebase

@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 5, 2017
@derekwaynecarr
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 5, 2017
@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: DirectXMan12, derekwaynecarr, smarterclayton

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@DirectXMan12
Copy link
Contributor Author

/retest

#43236

@DirectXMan12
Copy link
Contributor Author

@k8s-bot pull-kubernetes-e2e-kops-aws test this

1 similar comment
@foxish
Copy link
Contributor

foxish commented Jun 5, 2017

@k8s-bot pull-kubernetes-e2e-kops-aws test this

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 7bbc615 into kubernetes:master Jun 5, 2017
@DirectXMan12 DirectXMan12 deleted the feature/hpa-status-conditions branch June 6, 2017 15:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/autoscaling Categorizes an issue or PR as relevant to SIG Autoscaling. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.