Skip to content

Add validations related to minReplicas and maxReplicas before updating HPA #451

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
AVSBharadwaj wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add validations related to minReplicas and maxReplicas before updating HPA #451

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
1 change: 1 addition & 0 deletions pkg/event/event.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,4 +20,5 @@ const (
RestartDeployment = "RestartDeployment"

WarningHittingHardMaxReplicaLimit = "HitHardMaxReplicaLimit"
WarningReplicaValidation = "ReplicaValidationWarning"
)
49 changes: 47 additions & 2 deletions pkg/hpa/service.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -412,24 +412,44 @@ func (c *Service) ChangeHPAFromTortoiseRecommendation(tortoise *autoscalingv1bet
return nil, tortoise, fmt.Errorf("get maxReplicas recommendation: %w", err)
}

// Apply user-specified maxReplicas limit if set
if tortoise.Spec.MaxReplicas != nil && recommendMax > *tortoise.Spec.MaxReplicas {
recommendMax = *tortoise.Spec.MaxReplicas
}

// Apply cluster-wide maxReplicas limit
if recommendMax > c.maximumMaxReplica {
c.recorder.Event(tortoise, corev1.EventTypeWarning, event.WarningHittingHardMaxReplicaLimit, fmt.Sprintf("MaxReplica (%v) suggested from Tortoise (%s/%s) hits a cluster-wide maximum replica number (%v). It wouldn't be a problem until the replica number actually grows to %v though, you may want to reach out to your cluster admin.", recommendMax, tortoise.Namespace, tortoise.Name, c.maximumMaxReplica, c.maximumMaxReplica))
recommendMax = c.maximumMaxReplica
}

hpa.Spec.MaxReplicas = recommendMax
// Ensure maxReplicas is at least minimumMinReplicas
if recommendMax < c.minimumMinReplicas {
c.recorder.Event(tortoise, corev1.EventTypeWarning, event.WarningReplicaValidation, fmt.Sprintf("MaxReplica (%v) is below minimum allowed replicas (%v), adjusting to minimum", recommendMax, c.minimumMinReplicas))
recommendMax = c.minimumMinReplicas
}

recommendMin, err := GetReplicasRecommendation(tortoise.Status.Recommendations.Horizontal.MinReplicas, now)
if err != nil {
return nil, tortoise, fmt.Errorf("get minReplicas recommendation: %w", err)
}

// Apply cluster-wide minReplicas limit
if recommendMin > c.maximumMinReplica {
c.recorder.Event(tortoise, corev1.EventTypeWarning, event.WarningReplicaValidation, fmt.Sprintf("MinReplica (%v) suggested from Tortoise (%s/%s) hits a cluster-wide maximum min replica number (%v), capping to maximum", recommendMin, tortoise.Namespace, tortoise.Name, c.maximumMinReplica))
recommendMin = c.maximumMinReplica
// We don't change the maxReplica because it's dangerous to limit.
}

// Ensure minReplicas is at least minimumMinReplicas
if recommendMin < c.minimumMinReplicas {
c.recorder.Event(tortoise, corev1.EventTypeWarning, event.WarningReplicaValidation, fmt.Sprintf("MinReplica (%v) is below minimum allowed replicas (%v), adjusting to minimum", recommendMin, c.minimumMinReplicas))
recommendMin = c.minimumMinReplicas
}

// Ensure minReplicas doesn't exceed maxReplicas
if recommendMin > recommendMax {
Copy link
Copy Markdown
Contributor

@djahandarie djahandarie Jul 1, 2025

Choose a reason for hiding this comment

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

I feel like there's a bug with the recommender logic if it ever returns recommendMin > recommendMax, should there also be a fix there?

Copy link
Copy Markdown
Collaborator Author

@AVSBharadwaj AVSBharadwaj Jul 1, 2025

Choose a reason for hiding this comment

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

The recommender will not result in such a recommendations ( where recommendMin > recommendMax) . This is added just for completion sake to cover all possible cases.

c.recorder.Event(tortoise, corev1.EventTypeWarning, event.WarningReplicaValidation, fmt.Sprintf("MinReplica (%v) exceeds MaxReplica (%v), adjusting MaxReplica to MinReplica", recommendMin, recommendMax))
recommendMax = recommendMin
}

if recordMetrics {
Expand Down Expand Up @@ -459,6 +479,31 @@ func (c *Service) ChangeHPAFromTortoiseRecommendation(tortoise *autoscalingv1bet
minToActuallyApply = recommendMin
}

// Final validation: Ensure minReplicas doesn't exceed maxReplicas after phase-specific adjustments
if minToActuallyApply > recommendMax {
if tortoise.Status.TortoisePhase == autoscalingv1beta3.TortoisePhaseBackToNormal {
// During BackToNormal, temporarily increase maxReplicas to allow safe gradual reduction
// This preserves the safety mechanism while ensuring HPA validity
originalMaxReplicas := recommendMax
recommendMax = minToActuallyApply
c.recorder.Event(tortoise, corev1.EventTypeNormal, event.Working,
fmt.Sprintf("Temporarily increased MaxReplicas from %v to %v during BackToNormal phase to allow safe gradual MinReplicas reduction",
originalMaxReplicas, minToActuallyApply))
} else {
// For other phases, cap minReplicas to maxReplicas
minToActuallyApply = recommendMax
c.recorder.Event(tortoise, corev1.EventTypeWarning, event.WarningReplicaValidation,
fmt.Sprintf("MinReplicas was capped to MaxReplicas (%v) to prevent HPA validation error", recommendMax))
}
}

// Ensure final minReplicas is at least minimumMinReplicas
if minToActuallyApply < c.minimumMinReplicas {
c.recorder.Event(tortoise, corev1.EventTypeWarning, event.WarningReplicaValidation, fmt.Sprintf("Final MinReplicas (%v) is below minimum allowed replicas (%v), adjusting to minimum", minToActuallyApply, c.minimumMinReplicas))
minToActuallyApply = c.minimumMinReplicas
}

hpa.Spec.MaxReplicas = recommendMax
hpa.Spec.MinReplicas = &minToActuallyApply
if tortoise.Spec.UpdateMode != autoscalingv1beta3.UpdateModeOff && recordMetrics {
// We don't want to record applied* metric when UpdateMode is Off.
Expand Down
Loading