Skip to content

add additional log & UT for HPA #127394

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

add additional log & UT for HPA #127394

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
87 changes: 78 additions & 9 deletions pkg/controller/podautoscaler/horizontal.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,8 @@ import (
var (
scaleUpLimitFactor = 2.0
scaleUpLimitMinimum = 4.0
// recommendationOutputLimit is the maximum number of recommendations that will be logged.
recommendationOutputLimit = 100
)

var (
Expand Down Expand Up @@ -845,9 +847,11 @@ func (a *HorizontalController) reconcileAutoscaler(ctx context.Context, hpaShare

logger.V(4).Info("Proposing desired replicas",
"desiredReplicas", metricDesiredReplicas,
"currentReplicas", currentReplicas,
"metric", metricName,
"timestamp", metricTimestamp,
"scaleTarget", reference)
"scaleTarget", reference,
Copy link
Member

Choose a reason for hiding this comment

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

I checked and found that the modified logs below also use reference := fmt.Sprintf("%s/%s/%s", hpa.Spec.ScaleTargetRef.Kind, hpa.Namespace, hpa.Spec.ScaleTargetRef.Name) many times. Then you can directly use the following method at the top level of the call(Maybe under line 815), and the logs below will also have this key-value pair.

If other key-value pairs are used multiple times, you can also add them here.

if loggerV := logger.V(4); loggerV.Enabled() {
	logger = klog.LoggerWithValues(
		logger,
		"scaleTarget", reference,
		"currentReplicas", currentReplicas,
		...
	)
	ctx = klog.NewContext(ctx, logger)
}

Copy link
Author

Choose a reason for hiding this comment

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

I think it's for logging, and is only utilized 3 times within this method. The rest are used in other methods. Switching to loggerV with V(4) might add complexity and reduce readability. It also can't be used for other verbose levels. I would suggest we keep it simple.

Copy link
Member

Choose a reason for hiding this comment

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

It makes sense.

"metrics", metricStatuses)

rescaleMetric := ""
if metricDesiredReplicas > desiredReplicas {
Expand All @@ -861,9 +865,9 @@ func (a *HorizontalController) reconcileAutoscaler(ctx context.Context, hpaShare
rescaleReason = "All metrics below target"
}
if hpa.Spec.Behavior == nil {
desiredReplicas = a.normalizeDesiredReplicas(hpa, key, currentReplicas, desiredReplicas, minReplicas)
desiredReplicas = a.normalizeDesiredReplicas(ctx, hpa, key, currentReplicas, desiredReplicas, minReplicas)
} else {
desiredReplicas = a.normalizeDesiredReplicasWithBehaviors(hpa, key, currentReplicas, desiredReplicas, minReplicas)
desiredReplicas = a.normalizeDesiredReplicasWithBehaviors(ctx, hpa, key, currentReplicas, desiredReplicas, minReplicas)
}
rescale = desiredReplicas != currentReplicas
}
Expand All @@ -885,6 +889,7 @@ func (a *HorizontalController) reconcileAutoscaler(ctx context.Context, hpaShare
a.storeScaleEvent(hpa.Spec.Behavior, key, currentReplicas, desiredReplicas)
logger.Info("Successfully rescaled",
"HPA", klog.KObj(hpa),
"scaleTarget", reference,
"currentReplicas", currentReplicas,
"desiredReplicas", desiredReplicas,
"reason", rescaleReason)
Expand Down Expand Up @@ -942,7 +947,7 @@ func (a *HorizontalController) stabilizeRecommendation(key string, prenormalized

// normalizeDesiredReplicas takes the metrics desired replicas value and normalizes it based on the appropriate conditions (i.e. < maxReplicas, >
// minReplicas, etc...)
func (a *HorizontalController) normalizeDesiredReplicas(hpa *autoscalingv2.HorizontalPodAutoscaler, key string, currentReplicas int32, prenormalizedDesiredReplicas int32, minReplicas int32) int32 {
func (a *HorizontalController) normalizeDesiredReplicas(ctx context.Context, hpa *autoscalingv2.HorizontalPodAutoscaler, key string, currentReplicas int32, prenormalizedDesiredReplicas int32, minReplicas int32) int32 {
stabilizedRecommendation := a.stabilizeRecommendation(key, prenormalizedDesiredReplicas)
if stabilizedRecommendation != prenormalizedDesiredReplicas {
setCondition(hpa, autoscalingv2.AbleToScale, v1.ConditionTrue, "ScaleDownStabilized", "recent recommendations were higher than current one, applying the highest recent recommendation")
Expand All @@ -957,6 +962,16 @@ func (a *HorizontalController) normalizeDesiredReplicas(hpa *autoscalingv2.Horiz
} else {
setCondition(hpa, autoscalingv2.ScalingLimited, v1.ConditionTrue, reason, "%s", message)
}
logger := klog.FromContext(ctx)
reference := fmt.Sprintf("%s/%s/%s", hpa.Spec.ScaleTargetRef.Kind, hpa.Namespace, hpa.Spec.ScaleTargetRef.Name)
logger.V(4).Info("Normalized desired replicas",
"scaleTarget", reference,
"currentReplicas", currentReplicas,
"desiredReplicas", desiredReplicas,
"minReplicas", minReplicas,
"stabilizedRecommendation", stabilizedRecommendation,
"prenormalizedDesiredReplicas", prenormalizedDesiredReplicas,
"reason", reason)

return desiredReplicas
}
Expand All @@ -977,7 +992,7 @@ type NormalizationArg struct {
// 2. Apply the scale up/down limits from the hpaSpec.Behaviors (i.e. add no more than 4 pods)
// 3. Apply the constraints period (i.e. add no more than 4 pods per minute)
// 4. Apply the stabilization (i.e. add no more than 4 pods per minute, and pick the smallest recommendation during last 5 minutes)
func (a *HorizontalController) normalizeDesiredReplicasWithBehaviors(hpa *autoscalingv2.HorizontalPodAutoscaler, key string, currentReplicas, prenormalizedDesiredReplicas, minReplicas int32) int32 {
func (a *HorizontalController) normalizeDesiredReplicasWithBehaviors(ctx context.Context, hpa *autoscalingv2.HorizontalPodAutoscaler, key string, currentReplicas, prenormalizedDesiredReplicas, minReplicas int32) int32 {
a.maybeInitScaleDownStabilizationWindow(hpa)
normalizationArg := NormalizationArg{
Key: key,
Expand All @@ -987,21 +1002,42 @@ func (a *HorizontalController) normalizeDesiredReplicasWithBehaviors(hpa *autosc
MaxReplicas: hpa.Spec.MaxReplicas,
CurrentReplicas: currentReplicas,
DesiredReplicas: prenormalizedDesiredReplicas}
stabilizedRecommendation, reason, message := a.stabilizeRecommendationWithBehaviors(normalizationArg)
stabilizedRecommendation, reason, message := a.stabilizeRecommendationWithBehaviors(ctx, normalizationArg)
normalizationArg.DesiredReplicas = stabilizedRecommendation
if stabilizedRecommendation != prenormalizedDesiredReplicas {
// "ScaleUpStabilized" || "ScaleDownStabilized"
setCondition(hpa, autoscalingv2.AbleToScale, v1.ConditionTrue, reason, "%s", message)
} else {
setCondition(hpa, autoscalingv2.AbleToScale, v1.ConditionTrue, "ReadyForNewScale", "recommended size matches current size")
}
desiredReplicas, reason, message := a.convertDesiredReplicasWithBehaviorRate(normalizationArg)

logger := klog.FromContext(ctx)
reference := fmt.Sprintf("%s/%s/%s", hpa.Spec.ScaleTargetRef.Kind, hpa.Namespace, hpa.Spec.ScaleTargetRef.Name)
logger.V(4).Info("Normalized desired replicas with behaviors - after stabilized recommendation",
"scaleTarget", reference,
"currentReplicas", currentReplicas,
"minReplicas", minReplicas,
"stabilizedRecommendation", stabilizedRecommendation,
"prenormalizedDesiredReplicas", prenormalizedDesiredReplicas,
"reason", reason,
"message", message)

desiredReplicas, reason, message := a.convertDesiredReplicasWithBehaviorRate(ctx, normalizationArg)
if desiredReplicas == stabilizedRecommendation {
setCondition(hpa, autoscalingv2.ScalingLimited, v1.ConditionFalse, reason, "%s", message)
} else {
setCondition(hpa, autoscalingv2.ScalingLimited, v1.ConditionTrue, reason, "%s", message)
}

logger.V(4).Info("Normalized desired replicas with behaviors - after rated recommendation",
"scaleTarget", reference,
"currentReplicas", currentReplicas,
"minReplicas", minReplicas,
"stabilizedRecommendation", stabilizedRecommendation,
"desiredReplicas", desiredReplicas,
"reason", reason,
"message", message)

return desiredReplicas
}

Expand Down Expand Up @@ -1090,7 +1126,7 @@ func (a *HorizontalController) storeScaleEvent(behavior *autoscalingv2.Horizonta
// stabilizeRecommendationWithBehaviors:
// - replaces old recommendation with the newest recommendation,
// - returns {max,min} of recommendations that are not older than constraints.Scale{Up,Down}.DelaySeconds
func (a *HorizontalController) stabilizeRecommendationWithBehaviors(args NormalizationArg) (int32, string, string) {
func (a *HorizontalController) stabilizeRecommendationWithBehaviors(ctx context.Context, args NormalizationArg) (int32, string, string) {
now := time.Now()

foundOldSample := false
Expand Down Expand Up @@ -1128,6 +1164,24 @@ func (a *HorizontalController) stabilizeRecommendationWithBehaviors(args Normali
if recommendation > downRecommendation {
recommendation = downRecommendation
}
// Only keep the recommendations and ignore timestamp for logging, keep last recommendationOutputLimit recommendations
var recommendations = make([]int32, 0, recommendationOutputLimit)
start := len(a.recommendations[args.Key]) - recommendationOutputLimit
if start < 0 {
start = 0
}
for i := start; i < len(a.recommendations[args.Key]); i++ {
recommendations = append(recommendations, a.recommendations[args.Key][i].recommendation)
}

logger := klog.FromContext(ctx)
logger.V(4).Info("Stabilizing recommendation",
"key", args.Key,
"currentReplicas", args.CurrentReplicas,
"desiredReplicas", args.DesiredReplicas,
"upRecommendation", upRecommendation,
"downRecommendation", downRecommendation,
"recommendations", recommendations)

// Record the unstabilized recommendation.
if foundOldSample {
Expand All @@ -1150,14 +1204,22 @@ func (a *HorizontalController) stabilizeRecommendationWithBehaviors(args Normali

// convertDesiredReplicasWithBehaviorRate performs the actual normalization, given the constraint rate
// It doesn't consider the stabilizationWindow, it is done separately
func (a *HorizontalController) convertDesiredReplicasWithBehaviorRate(args NormalizationArg) (int32, string, string) {
func (a *HorizontalController) convertDesiredReplicasWithBehaviorRate(ctx context.Context, args NormalizationArg) (int32, string, string) {
var possibleLimitingReason, possibleLimitingMessage string
logger := klog.FromContext(ctx)

if args.DesiredReplicas > args.CurrentReplicas {
a.scaleUpEventsLock.RLock()
defer a.scaleUpEventsLock.RUnlock()
a.scaleDownEventsLock.RLock()
defer a.scaleDownEventsLock.RUnlock()
logger.V(4).Info("Converting desired replicas with behavior rate - scale up",
"key", args.Key,
"currentReplicas", args.CurrentReplicas,
"desiredReplicas", args.DesiredReplicas,
"scaleUpEvents", a.scaleUpEvents[args.Key],
"scaleDownEvents", a.scaleDownEvents[args.Key])

scaleUpLimit := calculateScaleUpLimitWithScalingRules(args.CurrentReplicas, a.scaleUpEvents[args.Key], a.scaleDownEvents[args.Key], args.ScaleUpBehavior)

if scaleUpLimit < args.CurrentReplicas {
Expand All @@ -1181,6 +1243,13 @@ func (a *HorizontalController) convertDesiredReplicasWithBehaviorRate(args Norma
defer a.scaleUpEventsLock.RUnlock()
a.scaleDownEventsLock.RLock()
defer a.scaleDownEventsLock.RUnlock()
logger.V(4).Info("Converting desired replicas with behavior rate - scale down",
"key", args.Key,
"currentReplicas", args.CurrentReplicas,
"desiredReplicas", args.DesiredReplicas,
"scaleUpEvents", a.scaleUpEvents[args.Key],
"scaleDownEvents", a.scaleDownEvents[args.Key])

scaleDownLimit := calculateScaleDownLimitWithBehaviors(args.CurrentReplicas, a.scaleUpEvents[args.Key], a.scaleDownEvents[args.Key], args.ScaleDownBehavior)

if scaleDownLimit > args.CurrentReplicas {
Expand Down
42 changes: 40 additions & 2 deletions pkg/controller/podautoscaler/horizontal_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4315,6 +4315,42 @@ func TestScalingWithRules(t *testing.T) {
expectedReplicas: 16,
expectedCondition: "ScaleUpLimit",
},
{
name: "scaleUp with percent policy and no previous events, thus scaled up within range",
scaleUpEvents: generateEventsUniformDistribution([]int{0}, 120),
scaleDownEvents: generateEventsUniformDistribution([]int{0}, 120),
specMinReplicas: 3,
specMaxReplicas: 6,
scaleUpRules: generateScalingRules(0, 0, 20, 600, 300),
currentReplicas: 3,
prenormalizedDesiredReplicas: 4,
expectedReplicas: 4,
expectedCondition: "DesiredWithinRange",
},
{
name: "scaleUp with both pod and percent policy and previous scale up events, thus no scale up",
scaleUpEvents: generateEventsUniformDistribution([]int{2}, 120),
scaleDownEvents: generateEventsUniformDistribution([]int{0}, 120),
specMinReplicas: 3,
specMaxReplicas: 6,
scaleUpRules: generateScalingRules(1, 600, 20, 600, 300),
currentReplicas: 3,
prenormalizedDesiredReplicas: 4,
expectedReplicas: 3,
expectedCondition: "ScaleUpLimit",
},
{
name: "scaleUp with percent policy and previous events but still be able to scale up with limited rate",
scaleUpEvents: generateEventsUniformDistribution([]int{1}, 600),
scaleDownEvents: generateEventsUniformDistribution([]int{0}, 600),
specMinReplicas: 3,
specMaxReplicas: 20,
scaleUpRules: generateScalingRules(0, 0, 20, 600, 300),
currentReplicas: 10,
prenormalizedDesiredReplicas: 12,
expectedReplicas: 11,
expectedCondition: "ScaleUpLimit",
},
// ScaleDown with PeriodSeconds usage
{
name: "scaleDown with default policy and previous events",
Expand Down Expand Up @@ -4486,6 +4522,7 @@ func TestScalingWithRules(t *testing.T) {
},
}

tCtx := ktesting.Init(t)
for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {

Expand All @@ -4510,7 +4547,7 @@ func TestScalingWithRules(t *testing.T) {
CurrentReplicas: tc.currentReplicas,
}

replicas, condition, _ := hc.convertDesiredReplicasWithBehaviorRate(arg)
replicas, condition, _ := hc.convertDesiredReplicasWithBehaviorRate(tCtx, arg)
assert.Equal(t, tc.expectedReplicas, replicas, "expected replicas do not match with converted replicas")
assert.Equal(t, tc.expectedCondition, condition, "HPA condition does not match with expected condition")
})
Expand Down Expand Up @@ -4865,6 +4902,7 @@ func TestNormalizeDesiredReplicasWithBehavior(t *testing.T) {
scaleDownStabilizationWindowSeconds: 300,
},
}
tCtx := ktesting.Init(t)
for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
hc := HorizontalController{
Expand All @@ -4883,7 +4921,7 @@ func TestNormalizeDesiredReplicasWithBehavior(t *testing.T) {
StabilizationWindowSeconds: &tc.scaleDownStabilizationWindowSeconds,
},
}
r, _, _ := hc.stabilizeRecommendationWithBehaviors(arg)
r, _, _ := hc.stabilizeRecommendationWithBehaviors(tCtx, arg)
assert.Equal(t, tc.expectedStabilizedReplicas, r, "expected replicas do not match")
if tc.expectedRecommendations != nil {
if !assert.Len(t, hc.recommendations[tc.key], len(tc.expectedRecommendations), "stored recommendations differ in length") {
Expand Down