Skip to content

Commit 98f3b33

Browse files
ciarams87ciarams87
committed
Fix HPA race condition by reading deployment replicas instead of HPA status (#4214)
When HPA is enabled, read the current Deployment.Spec.Replicas directly instead of HPA.Status.DesiredReplicas, which is eventually consistent and lags behind deployment changes. This prevents the controller from overwriting HPA's replica count with stale values, eliminating pod churn and connection drops. Fixes race condition where HPA scales down → NGF reads stale HPA status → NGF overwrites deployment with old replica count → pods restart.
1 parent bd30d48 commit 98f3b33

File tree

2 files changed

+194
-80
lines changed

2 files changed

+194
-80
lines changed

‎internal/controller/provisioner/objects.go‎

Lines changed: 56 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -686,33 +686,69 @@ func (p *NginxProvisioner) buildNginxDeployment(
686686
}
687687
}
688688

689-
var replicas *int32
690-
if deploymentCfg.Replicas != nil {
691-
replicas = deploymentCfg.Replicas
692-
}
693-
694-
if isAutoscalingEnabled(&deploymentCfg) {
695-
ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
696-
defer cancel()
697-
698-
hpa := &autoscalingv2.HorizontalPodAutoscaler{}
699-
err := p.k8sClient.Get(ctx, types.NamespacedName{
700-
Namespace: objectMeta.Namespace,
701-
Name: objectMeta.Name,
702-
}, hpa)
703-
if err == nil && hpa.Status.DesiredReplicas > 0 {
704-
// overwrite with HPA's desiredReplicas
705-
replicas = helpers.GetPointer(hpa.Status.DesiredReplicas)
706-
}
707-
}
708-
689+
// Determine replica count based on HPA status
690+
replicas := p.determineReplicas(objectMeta, deploymentCfg)
709691
if replicas != nil {
710692
deployment.Spec.Replicas = replicas
711693
}
712694

713695
return deployment, nil
714696
}
715697

698+
// determineReplicas determines the appropriate replica count for a deployment based on HPA status.
699+
//
700+
// HPA Replicas Management Strategy:
701+
//
702+
// When an HPA is managing a deployment, we must read the current deployment's replicas
703+
// from the cluster and use that value, rather than trying to set our own value or read
704+
// from HPA.Status.DesiredReplicas (which is eventually consistent and stale).
705+
//
706+
// Why we can't use HPA.Status.DesiredReplicas:
707+
// - HPA.Status updates lag behind Deployment.Spec.Replicas changes
708+
// - When HPA scales down: HPA writes Deployment.Spec → then updates its own Status
709+
// - If we read Status during this window, we get the OLD value and overwrite HPA's new value
710+
// - This creates a race condition causing pod churn
711+
//
712+
// Our approach:
713+
// - When HPA exists: Read current deployment replicas from cluster and use that
714+
// - When HPA doesn't exist yet: Set replicas for initial deployment creation
715+
// - When HPA exists but Deployment doesn't exist yet: Set replicas for initial deployment creation
716+
// - When HPA is disabled: Set replicas normally.
717+
func (p *NginxProvisioner) determineReplicas(
718+
objectMeta metav1.ObjectMeta,
719+
deploymentCfg ngfAPIv1alpha2.DeploymentSpec,
720+
) *int32 {
721+
replicas := deploymentCfg.Replicas
722+
723+
if !isAutoscalingEnabled(&deploymentCfg) {
724+
return replicas
725+
}
726+
727+
ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
728+
defer cancel()
729+
730+
hpa := &autoscalingv2.HorizontalPodAutoscaler{}
731+
err := p.k8sClient.Get(ctx, types.NamespacedName{
732+
Namespace: objectMeta.Namespace,
733+
Name: objectMeta.Name,
734+
}, hpa)
735+
if err != nil {
736+
return replicas
737+
}
738+
739+
existingDeployment := &appsv1.Deployment{}
740+
err = p.k8sClient.Get(ctx, types.NamespacedName{
741+
Namespace: objectMeta.Namespace,
742+
Name: objectMeta.Name,
743+
}, existingDeployment)
744+
745+
if err == nil && existingDeployment.Spec.Replicas != nil {
746+
replicas = existingDeployment.Spec.Replicas
747+
}
748+
749+
return replicas
750+
}
751+
716752
// applyPatches applies the provided patches to the given object.
717753
func applyPatches(obj client.Object, patches []ngfAPIv1alpha2.Patch) error {
718754
if len(patches) == 0 {

‎internal/controller/provisioner/objects_test.go‎

Lines changed: 138 additions & 60 deletions
Original file line numberDiff line numberDiff line change
@@ -392,78 +392,156 @@ func TestBuildNginxResourceObjects_NginxProxyConfig(t *testing.T) {
392392

393393
func TestBuildNginxResourceObjects_DeploymentReplicasFromHPA(t *testing.T) {
394394
t.Parallel()
395-
g := NewWithT(t)
396395

397-
// Create a fake HPA with status.desiredReplicas set
398-
hpa := &autoscalingv2.HorizontalPodAutoscaler{
399-
ObjectMeta: metav1.ObjectMeta{
400-
Name: "gw-nginx",
401-
Namespace: "default",
396+
tests := []struct {
397+
currentReplicas *int32
398+
configReplicas *int32
399+
name string
400+
description string
401+
expectedValue int32
402+
hpaExists bool
403+
deploymentExists bool
404+
expectedNil bool
405+
}{
406+
{
407+
name: "HPA exists - use current deployment replicas",
408+
hpaExists: true,
409+
deploymentExists: true,
410+
currentReplicas: helpers.GetPointer(int32(8)),
411+
configReplicas: helpers.GetPointer(int32(5)),
412+
expectedNil: false,
413+
expectedValue: 8,
414+
description: "When HPA exists, read current deployment replicas (set by HPA)",
402415
},
403-
Status: autoscalingv2.HorizontalPodAutoscalerStatus{
404-
DesiredReplicas: 7,
416+
{
417+
name: "HPA does not exist - use configured replicas",
418+
hpaExists: false,
419+
deploymentExists: false,
420+
configReplicas: helpers.GetPointer(int32(3)),
421+
expectedNil: false,
422+
expectedValue: 3,
423+
description: "When HPA doesn't exist yet (initial creation), use configured replicas",
405424
},
406-
}
407-
408-
agentTLSSecret := &corev1.Secret{
409-
ObjectMeta: metav1.ObjectMeta{
410-
Name: agentTLSTestSecretName,
411-
Namespace: ngfNamespace,
425+
{
426+
name: "HPA enabled but doesn't exist, no configured replicas",
427+
hpaExists: false,
428+
deploymentExists: false,
429+
configReplicas: nil,
430+
expectedNil: true,
431+
description: "When HPA enabled but doesn't exist and no replicas configured, don't set replicas",
412432
},
413-
Data: map[string][]byte{"tls.crt": []byte("tls")},
414433
}
415434

416-
fakeClient := fake.NewFakeClient(agentTLSSecret, hpa)
435+
for _, tc := range tests {
436+
t.Run(tc.name, func(t *testing.T) {
437+
t.Parallel()
438+
g := NewWithT(t)
417439

418-
provisioner := &NginxProvisioner{
419-
cfg: Config{
420-
GatewayPodConfig: &config.GatewayPodConfig{
421-
Namespace: ngfNamespace,
422-
Version: "1.0.0",
423-
Image: "ngf-image",
424-
},
425-
AgentTLSSecretName: agentTLSTestSecretName,
426-
},
427-
baseLabelSelector: metav1.LabelSelector{
428-
MatchLabels: map[string]string{"app": "nginx"},
429-
},
430-
k8sClient: fakeClient,
431-
}
440+
agentTLSSecret := &corev1.Secret{
441+
ObjectMeta: metav1.ObjectMeta{
442+
Name: agentTLSTestSecretName,
443+
Namespace: ngfNamespace,
444+
},
445+
Data: map[string][]byte{"tls.crt": []byte("tls")},
446+
}
432447

433-
gateway := &gatewayv1.Gateway{
434-
ObjectMeta: metav1.ObjectMeta{
435-
Name: "gw",
436-
Namespace: "default",
437-
},
438-
Spec: gatewayv1.GatewaySpec{
439-
Listeners: []gatewayv1.Listener{{Port: 80}},
440-
},
441-
}
448+
var fakeClient client.Client
449+
switch {
450+
case tc.hpaExists && tc.deploymentExists:
451+
// Create a fake HPA and existing deployment
452+
hpa := &autoscalingv2.HorizontalPodAutoscaler{
453+
ObjectMeta: metav1.ObjectMeta{
454+
Name: "gw-nginx",
455+
Namespace: "default",
456+
},
457+
Status: autoscalingv2.HorizontalPodAutoscalerStatus{
458+
DesiredReplicas: 7,
459+
},
460+
}
461+
existingDeployment := &appsv1.Deployment{
462+
ObjectMeta: metav1.ObjectMeta{
463+
Name: "gw-nginx",
464+
Namespace: "default",
465+
},
466+
Spec: appsv1.DeploymentSpec{
467+
Replicas: tc.currentReplicas,
468+
Selector: &metav1.LabelSelector{
469+
MatchLabels: map[string]string{"app": "nginx"},
470+
},
471+
},
472+
}
473+
fakeClient = fake.NewFakeClient(agentTLSSecret, hpa, existingDeployment)
474+
case tc.hpaExists:
475+
hpa := &autoscalingv2.HorizontalPodAutoscaler{
476+
ObjectMeta: metav1.ObjectMeta{
477+
Name: "gw-nginx",
478+
Namespace: "default",
479+
},
480+
Status: autoscalingv2.HorizontalPodAutoscalerStatus{
481+
DesiredReplicas: 7,
482+
},
483+
}
484+
fakeClient = fake.NewFakeClient(agentTLSSecret, hpa)
485+
default:
486+
fakeClient = fake.NewFakeClient(agentTLSSecret)
487+
}
442488

443-
resourceName := "gw-nginx"
444-
nProxyCfg := &graph.EffectiveNginxProxy{
445-
Kubernetes: &ngfAPIv1alpha2.KubernetesSpec{
446-
Deployment: &ngfAPIv1alpha2.DeploymentSpec{
447-
Replicas: nil, // Should be overridden by HPA
448-
Autoscaling: &ngfAPIv1alpha2.AutoscalingSpec{Enable: true},
449-
},
450-
},
451-
}
489+
provisioner := &NginxProvisioner{
490+
cfg: Config{
491+
GatewayPodConfig: &config.GatewayPodConfig{
492+
Namespace: ngfNamespace,
493+
Version: "1.0.0",
494+
Image: "ngf-image",
495+
},
496+
AgentTLSSecretName: agentTLSTestSecretName,
497+
},
498+
baseLabelSelector: metav1.LabelSelector{
499+
MatchLabels: map[string]string{"app": "nginx"},
500+
},
501+
k8sClient: fakeClient,
502+
}
452503

453-
objects, err := provisioner.buildNginxResourceObjects(resourceName, gateway, nProxyCfg)
454-
g.Expect(err).ToNot(HaveOccurred())
504+
gateway := &gatewayv1.Gateway{
505+
ObjectMeta: metav1.ObjectMeta{
506+
Name: "gw",
507+
Namespace: "default",
508+
},
509+
Spec: gatewayv1.GatewaySpec{
510+
Listeners: []gatewayv1.Listener{{Port: 80}},
511+
},
512+
}
455513

456-
// Find the deployment object
457-
var deployment *appsv1.Deployment
458-
for _, obj := range objects {
459-
if d, ok := obj.(*appsv1.Deployment); ok {
460-
deployment = d
461-
break
462-
}
514+
resourceName := "gw-nginx"
515+
nProxyCfg := &graph.EffectiveNginxProxy{
516+
Kubernetes: &ngfAPIv1alpha2.KubernetesSpec{
517+
Deployment: &ngfAPIv1alpha2.DeploymentSpec{
518+
Replicas: tc.configReplicas,
519+
Autoscaling: &ngfAPIv1alpha2.AutoscalingSpec{Enable: true},
520+
},
521+
},
522+
}
523+
524+
objects, err := provisioner.buildNginxResourceObjects(resourceName, gateway, nProxyCfg)
525+
g.Expect(err).ToNot(HaveOccurred())
526+
527+
// Find the deployment object
528+
var deployment *appsv1.Deployment
529+
for _, obj := range objects {
530+
if d, ok := obj.(*appsv1.Deployment); ok {
531+
deployment = d
532+
break
533+
}
534+
}
535+
g.Expect(deployment).ToNot(BeNil())
536+
537+
if tc.expectedNil {
538+
g.Expect(deployment.Spec.Replicas).To(BeNil(), tc.description)
539+
} else {
540+
g.Expect(deployment.Spec.Replicas).ToNot(BeNil(), tc.description)
541+
g.Expect(*deployment.Spec.Replicas).To(Equal(tc.expectedValue), tc.description)
542+
}
543+
})
463544
}
464-
g.Expect(deployment).ToNot(BeNil())
465-
g.Expect(deployment.Spec.Replicas).ToNot(BeNil())
466-
g.Expect(*deployment.Spec.Replicas).To(Equal(int32(7)))
467545
}
468546

469547
func TestBuildNginxResourceObjects_Plus(t *testing.T) {

0 commit comments

Comments
 (0)