-
Notifications
You must be signed in to change notification settings - Fork 39.8k
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
HPA Status Conditions #46550
Conversation
cc @kubernetes/sig-autoscaling-pr-reviews |
170d968
to
9019cb2
Compare
9019cb2
to
b24ee9a
Compare
@@ -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") |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
pkg/apis/autoscaling/types.go
Outdated
@@ -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, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/conditions/Conditions
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
pkg/apis/autoscaling/types.go
Outdated
// HorizontalPodAutoscalerCondition describes the state of | ||
// a HorizontalPodAutoscaler at a certain point. | ||
type HorizontalPodAutoscalerCondition struct { | ||
// type describes the current condition |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
pkg/apis/autoscaling/types.go
Outdated
|
||
// 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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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? |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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... |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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".
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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") |
There was a problem hiding this comment.
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? |
There was a problem hiding this comment.
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) { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?
This PR will need approves for kubectl and API. |
@smarterclayton it's alpha (part of v2alpha1) in 1.7 |
/approve Under those terms. If @Kargakis thinks this is a progressing condition type we can change that post code freeze or in beta |
@jszczepkowski any comments, or is this good to go? |
/assign @derekwaynecarr |
/lgtm |
@falenn: changing LGTM is restricted to assignees, and only kubernetes org members may be assigned issues. In response to this:
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. |
@k8s-bot pull-kubernetes-federation-e2e-gce test this |
LGTM |
@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.
ba7705d
to
c8fdeb0
Compare
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.
rebased and fix the test issue caused by the rebase |
/lgtm |
[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 |
/retest |
@k8s-bot pull-kubernetes-e2e-kops-aws test this |
1 similar comment
@k8s-bot pull-kubernetes-e2e-kops-aws test this |
Automatic merge from submit-queue |
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: