[bug] Serial worker reloads can potentially block dynamic endpoint changes

[mrparkers]

What happened:

We have an ingress-nginx deployment that uses enable-serial-reloads to prevent too many nginx workers from spawning when many ingress changes are made within close proximity to one another. This configuration does not seem to have detailed documentation in the website docs, but you can see the docs in the code here:

ingress-nginx/internal/ingress/controller/config/config.go

Lines 475 to 480 in d1dc3e8

	// Defines whether multiple concurrent reloads of worker processes should occur.
	// Set this to false to prevent more than n x 2 workers to exist at any time, to avoid potential OOM situations and high CPU load
	// With this setting on false, configuration changes in the queue will be re-queued with an exponential backoff, until the number of worker process is the expected value.
	// By default new worker processes are spawned every time there's a change that cannot be applied dynamically with no upper limit to the number of running workers
	// http://nginx.org/en/docs/ngx_core_module.html#worker_processes
	WorkerSerialReloads bool `json:"enable-serial-reloads,omitempty"`

I think the intention behind enable-serial-reloads is to prevent too many configuration reloads that require new nginx worker processes to be launched. However, this configuration option seems to also unintentionally prevent dynamic configuration reloads (reloads that do not require a new nginx worker process). This seems to be a bug, because it can lead to a situation where a particularly long nginx reload blocks all dynamic endpoint changes for the controller. If a deployment rollout happens during a reload that requires a new nginx worker process, the updated list of endpoints does not seem to be picked up by the controller until the reload is finished.

What you expected to happen:

If enable-serial-reloads is set to true, I expect reloads that require new nginx worker processes to be re-queued if another such reload is already in progress, but I expect for dynamic reloads to remain intact. Endpoint updates should still happen even during a particularly long reload.

Code path:

Given a pending change that requires a configuration reload, which also includes endpoint changes:

• syncIngresses is called from the task queue:

  ingress-nginx/internal/ingress/controller/controller.go

  Line 175 in d1dc3e8

  func (n *NGINXController) syncIngress(interface{}) error {

• utilingress.IsDynamicConfigurationEnough is used to determine if a nginx reload is needed:

  ingress-nginx/internal/ingress/controller/controller.go

  Line 195 in d1dc3e8

  if !utilingress.IsDynamicConfigurationEnough(pcfg, n.runningConfig) {

• onUpdate is called:

  ingress-nginx/internal/ingress/controller/controller.go

  Lines 207 to 214 in d1dc3e8

  	err = n.OnUpdate(*pcfg)
  	if err != nil {
  	n.metricCollector.IncReloadErrorCount()
  	n.metricCollector.ConfigSuccess(hash, false)
  	klog.Errorf("Unexpected failure reloading the backend:\n%v", err)
  	n.recorder.Eventf(k8s.IngressPodDetails, apiv1.EventTypeWarning, "RELOAD", fmt.Sprintf("Error reloading NGINX: %v", err))
  	return err
  	}

• Inside onUpdate, an error is thrown due to an nginx reload already happening:

  ingress-nginx/internal/ingress/controller/nginx.go

  Lines 682 to 685 in dc3acbd

  	workerSerialReloads := cfg.WorkerSerialReloads
  	if workerSerialReloads && n.workersReloading {
  	return errors.New("worker reload already in progress, requeuing reload")
  	}

• Thus, the dynamic configuration never happens, lower in the syncIngresses function:

  ingress-nginx/internal/ingress/controller/controller.go

  Line 240 in d1dc3e8

  err := n.configureDynamically(pcfg)

NGINX Ingress controller version (exec into the pod and run /nginx-ingress-controller --version): v1.12.0

Kubernetes version (use kubectl version):

Client Version: v1.31.3
Kustomize Version: v5.4.2
Server Version: v1.29.12-eks-2d5f260

Environment:

• Cloud provider or hardware configuration: AWS EKS
• OS (e.g. from /etc/os-release): Amazon Linux 2
• Kernel (e.g. uname -a): Linux ip-10-2-52-194.us-west-2.compute.internal 5.10.223-212.873.amzn2.x86_64 #1 SMP Wed Aug 7 16:53:32 UTC 2024 x86_64 x86_64 x86_64 GNU/Linux
• Install tools:
  • Helm

[k8s-ci-robot]

This issue is currently awaiting triage.

If Ingress contributors determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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-sigs/prow repository.

[strongjz]

So the issue is here when the on update would fail and return an err.

What you're asking is that it continues and does the dynamic load.

I see skip dynamic configuration in the first template rendering in here, which makes sense

So it the order of operations would be

1. check for first-time render
2. check dynamic
3. check nondynamic

Then, return whatever error happens.

Also, I'm unsure why the controller can retry a nondynamic reload like we do with dynamic since the nginx.go OnUpdate will stop the update during a reload.