-
Notifications
You must be signed in to change notification settings - Fork 38.7k
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
Implement external metric in HPA #60243
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -299,6 +299,45 @@ func (a *HorizontalController) computeReplicasForMetrics(hpa *autoscalingv2.Hori | |
}, | ||
} | ||
} | ||
case autoscalingv2.ExternalMetricSourceType: | ||
if metricSpec.External.TargetAverageValue != nil { | ||
replicaCountProposal, utilizationProposal, timestampProposal, err = a.replicaCalc.GetExternalPerPodMetricReplicas(currentReplicas, metricSpec.External.TargetAverageValue.MilliValue(), metricSpec.External.MetricName, hpa.Namespace, metricSpec.External.MetricSelector) | ||
if err != nil { | ||
a.eventRecorder.Event(hpa, v1.EventTypeWarning, "FailedGetExternalMetric", err.Error()) | ||
setCondition(hpa, autoscalingv2.ScalingActive, v1.ConditionFalse, "FailedGetExternalMetric", "the HPA was unable to compute the replica count: %v", err) | ||
return 0, "", nil, time.Time{}, fmt.Errorf("failed to get %s external metric: %v", metricSpec.External.MetricName, err) | ||
} | ||
metricNameProposal = fmt.Sprintf("external metric %s(%+v)", metricSpec.External.MetricName, metricSpec.External.MetricSelector) | ||
statuses[i] = autoscalingv2.MetricStatus{ | ||
Type: autoscalingv2.ExternalMetricSourceType, | ||
External: &autoscalingv2.ExternalMetricStatus{ | ||
MetricSelector: metricSpec.External.MetricSelector, | ||
MetricName: metricSpec.External.MetricName, | ||
CurrentAverageValue: resource.NewMilliQuantity(utilizationProposal, resource.DecimalSI), | ||
}, | ||
} | ||
} else if metricSpec.External.TargetValue != nil { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Has it been validated manually e2e? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes |
||
replicaCountProposal, utilizationProposal, timestampProposal, err = a.replicaCalc.GetExternalMetricReplicas(currentReplicas, metricSpec.External.TargetValue.MilliValue(), metricSpec.External.MetricName, hpa.Namespace, metricSpec.External.MetricSelector) | ||
if err != nil { | ||
a.eventRecorder.Event(hpa, v1.EventTypeWarning, "FailedGetExternalMetric", err.Error()) | ||
setCondition(hpa, autoscalingv2.ScalingActive, v1.ConditionFalse, "FailedGetExternalMetric", "the HPA was unable to compute the replica count: %v", err) | ||
return 0, "", nil, time.Time{}, fmt.Errorf("failed to get external metric %s: %v", metricSpec.External.MetricName, err) | ||
} | ||
metricNameProposal = fmt.Sprintf("external metric %s(%+v)", metricSpec.External.MetricName, metricSpec.External.MetricSelector) | ||
statuses[i] = autoscalingv2.MetricStatus{ | ||
Type: autoscalingv2.ExternalMetricSourceType, | ||
External: &autoscalingv2.ExternalMetricStatus{ | ||
MetricSelector: metricSpec.External.MetricSelector, | ||
MetricName: metricSpec.External.MetricName, | ||
CurrentValue: *resource.NewMilliQuantity(utilizationProposal, resource.DecimalSI), | ||
}, | ||
} | ||
} else { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we have a check for this in validate logic? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
errMsg := "invalid external metric source: neither a value target nor an average value target was set" | ||
a.eventRecorder.Event(hpa, v1.EventTypeWarning, "FailedGetExternalMetric", errMsg) | ||
setCondition(hpa, autoscalingv2.ScalingActive, v1.ConditionFalse, "FailedGetExternalMetric", "the HPA was unable to compute the replica count: %v", err) | ||
return 0, "", nil, time.Time{}, fmt.Errorf(errMsg) | ||
} | ||
default: | ||
errMsg := fmt.Sprintf("unknown metric source type %q", string(metricSpec.Type)) | ||
a.eventRecorder.Event(hpa, v1.EventTypeWarning, "InvalidMetricSourceType", errMsg) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -42,4 +42,8 @@ type MetricsClient interface { | |
// GetObjectMetric gets the given metric (and an associated timestamp) for the given | ||
// object in the given namespace | ||
GetObjectMetric(metricName string, namespace string, objectRef *autoscaling.CrossVersionObjectReference) (int64, time.Time, error) | ||
|
||
// GetExternalMetric gets all the values of a given external metric | ||
// that match the specified selector. | ||
GetExternalMetric(metricName string, namespace string, selector labels.Selector) ([]int64, time.Time, error) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Lets include Millis in the name of the metrics. It is not obvious from the method name that it returns the value in millis. Here and elsewhere. Can be next PR. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, I'll do it in a PR later on (it's just a naming change, no functional impact). |
||
} |
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.
Has it been verified manually E2e?
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.
Yes