From bc96769c91698bfa19e7a5352d47a0544e76c2af Mon Sep 17 00:00:00 2001 From: Rafael da Fonseca Date: Fri, 19 Jan 2024 21:22:12 +0000 Subject: [PATCH 1/6] feat: allow configuring nginx worker reload behaviour, to prevent multiple concurrent worker reloads Signed-off-by: Rafael da Fonseca --- .../nginx-configuration/configmap.md | 1 + internal/ingress/controller/config/config.go | 8 ++++ internal/ingress/controller/nginx.go | 48 +++++++++++++++++-- .../ingress/controller/template/configmap.go | 12 +++++ 4 files changed, 66 insertions(+), 3 deletions(-) diff --git a/docs/user-guide/nginx-configuration/configmap.md b/docs/user-guide/nginx-configuration/configmap.md index 7038ca90d3..14124625b8 100644 --- a/docs/user-guide/nginx-configuration/configmap.md +++ b/docs/user-guide/nginx-configuration/configmap.md @@ -114,6 +114,7 @@ The following table shows a configuration option's name, type, and the default v |[worker-processes](#worker-processes)|string|``|| |[worker-cpu-affinity](#worker-cpu-affinity)|string|""|| |[worker-shutdown-timeout](#worker-shutdown-timeout)|string|"240s"|| +|[concurrently-reload-worker-processes](#concurrently-reload-worker-processes)|bool|"true"|| |[load-balance](#load-balance)|string|"round_robin"|| |[variables-hash-bucket-size](#variables-hash-bucket-size)|int|128|| |[variables-hash-max-size](#variables-hash-max-size)|int|2048|| diff --git a/internal/ingress/controller/config/config.go b/internal/ingress/controller/config/config.go index bad82b8b0b..e20b74fc71 100644 --- a/internal/ingress/controller/config/config.go +++ b/internal/ingress/controller/config/config.go @@ -474,6 +474,13 @@ type Configuration struct { // http://nginx.org/en/docs/ngx_core_module.html#worker_processes WorkerProcesses string `json:"worker-processes,omitempty"` + // 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 + ConcurrentlyReloadWorkers bool `json:"concurrently-reload-worker-processes,omitempty"` + // Defines a timeout for a graceful shutdown of worker processes // http://nginx.org/en/docs/ngx_core_module.html#worker_shutdown_timeout WorkerShutdownTimeout string `json:"worker-shutdown-timeout,omitempty"` @@ -842,6 +849,7 @@ func NewDefault() Configuration { UseGzip: false, UseGeoIP2: false, WorkerProcesses: strconv.Itoa(runtime.NumCPU()), + ConcurrentlyReloadWorkers: true, WorkerShutdownTimeout: "240s", VariablesHashBucketSize: 256, VariablesHashMaxSize: 2048, diff --git a/internal/ingress/controller/nginx.go b/internal/ingress/controller/nginx.go index 578d5b4e8d..23888d2774 100644 --- a/internal/ingress/controller/nginx.go +++ b/internal/ingress/controller/nginx.go @@ -35,6 +35,7 @@ import ( "syscall" "text/template" "time" + "unicode" proxyproto "github.com/armon/go-proxyproto" "github.com/eapache/channels" @@ -87,9 +88,10 @@ func NewNGINXController(config *Configuration, mc metric.Collector) *NGINXContro n := &NGINXController{ isIPV6Enabled: ing_net.IsIPv6Enabled(), - resolver: h, - cfg: config, - syncRateLimiter: flowcontrol.NewTokenBucketRateLimiter(config.SyncRateLimit, 1), + resolver: h, + cfg: config, + syncRateLimiter: flowcontrol.NewTokenBucketRateLimiter(config.SyncRateLimit, 1), + workersReloading: false, recorder: eventBroadcaster.NewRecorder(scheme.Scheme, apiv1.EventSource{ Component: "nginx-ingress-controller", @@ -227,6 +229,8 @@ type NGINXController struct { syncRateLimiter flowcontrol.RateLimiter + workersReloading bool + // stopLock is used to enforce that only a single call to Stop send at // a given time. We allow stopping through an HTTP endpoint and // allowing concurrent stoppers leads to stack traces. @@ -668,6 +672,12 @@ Error: %v // //nolint:gocritic // the cfg shouldn't be changed, and shouldn't be mutated by other processes while being rendered. func (n *NGINXController) OnUpdate(ingressCfg ingress.Configuration) error { + concurrentlyReloadWorkers := n.store.GetBackendConfiguration().ConcurrentlyReloadWorkers + if !concurrentlyReloadWorkers && n.workersReloading { + klog.InfoS("worker reload already in progress, requeuing reload") + return errors.New("worker reload already in progress, requeuing reload") + } + cfg := n.store.GetBackendConfiguration() cfg.Resolver = n.resolver @@ -733,9 +743,41 @@ func (n *NGINXController) OnUpdate(ingressCfg ingress.Configuration) error { return fmt.Errorf("%v\n%v", err, string(o)) } + // Reload status checking runs in a separate goroutine to avoid blocking the sync queue + if !concurrentlyReloadWorkers { + go n.awaitWorkersReload() + } + return nil } +// awaitWorkersReload checks if the number of workers has returned to the expected count +func (n *NGINXController) awaitWorkersReload() { + n.workersReloading = true + defer func() { n.workersReloading = false }() + + expectedWorkers := n.store.GetBackendConfiguration().WorkerProcesses + var numWorkers string + klog.V(3).Infof("waiting for worker count to be equal to %s", expectedWorkers) + for numWorkers != expectedWorkers { + time.Sleep(time.Second) + o, err := exec.Command("/bin/sh", "-c", "pgrep worker | wc -l").Output() + if err != nil { + klog.ErrorS(err, string(numWorkers)) + return + } + // cleanup any non-printable chars from shell output + numWorkers = strings.Map(func(r rune) rune { + if unicode.IsPrint(r) { + return r + } + return -1 + }, string(o)) + + klog.V(3).Infof("Currently running nginx worker processes: %s, expected %s", numWorkers, expectedWorkers) + } +} + // nginxHashBucketSize computes the correct NGINX hash_bucket_size for a hash // with the given longest key. func nginxHashBucketSize(longestString int) int { diff --git a/internal/ingress/controller/template/configmap.go b/internal/ingress/controller/template/configmap.go index 9dc019bccc..05fcffca4a 100644 --- a/internal/ingress/controller/template/configmap.go +++ b/internal/ingress/controller/template/configmap.go @@ -67,6 +67,7 @@ const ( luaSharedDictsKey = "lua-shared-dicts" plugins = "plugins" debugConnections = "debug-connections" + concurrentlyReloadWorkers = "concurrently-reload-worker-processes" ) var ( @@ -385,6 +386,17 @@ func ReadConfig(src map[string]string) config.Configuration { delete(conf, workerProcesses) } + if val, ok := conf[concurrentlyReloadWorkers]; ok { + boolVal, err := strconv.ParseBool(val) + if err != nil { + to.ConcurrentlyReloadWorkers = true + klog.Warningf("failed to parse concurrently-reload-worker-processes setting, valid values are true or false, found %s", val) + } else { + to.ConcurrentlyReloadWorkers = boolVal + } + delete(conf, concurrentlyReloadWorkers) + } + if val, ok := conf[plugins]; ok { to.Plugins = splitAndTrimSpace(val, ",") delete(conf, plugins) From 1e20a005895ecb404de08197792168fac8e16047 Mon Sep 17 00:00:00 2001 From: Rafael da Fonseca Date: Fri, 19 Jan 2024 21:57:11 +0000 Subject: [PATCH 2/6] appease linter, remove unnecessary log line Signed-off-by: Rafael da Fonseca --- internal/ingress/controller/nginx.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/internal/ingress/controller/nginx.go b/internal/ingress/controller/nginx.go index 23888d2774..dfc42dcb91 100644 --- a/internal/ingress/controller/nginx.go +++ b/internal/ingress/controller/nginx.go @@ -674,7 +674,6 @@ Error: %v func (n *NGINXController) OnUpdate(ingressCfg ingress.Configuration) error { concurrentlyReloadWorkers := n.store.GetBackendConfiguration().ConcurrentlyReloadWorkers if !concurrentlyReloadWorkers && n.workersReloading { - klog.InfoS("worker reload already in progress, requeuing reload") return errors.New("worker reload already in progress, requeuing reload") } @@ -763,7 +762,7 @@ func (n *NGINXController) awaitWorkersReload() { time.Sleep(time.Second) o, err := exec.Command("/bin/sh", "-c", "pgrep worker | wc -l").Output() if err != nil { - klog.ErrorS(err, string(numWorkers)) + klog.ErrorS(err, numWorkers) return } // cleanup any non-printable chars from shell output From 1b8f98397a6d62bd51ad707ff85ae7015cfe5d74 Mon Sep 17 00:00:00 2001 From: Rafael da Fonseca Date: Mon, 22 Jan 2024 12:07:03 +0000 Subject: [PATCH 3/6] Flip to using a positive behaviour flag instead of negative Signed-off-by: Rafael da Fonseca --- .../templates/controller-configmap.yaml | 1 + charts/ingress-nginx/values.yaml | 2 ++ docs/user-guide/nginx-configuration/configmap.md | 2 +- internal/ingress/controller/config/config.go | 4 ++-- internal/ingress/controller/nginx.go | 6 +++--- internal/ingress/controller/template/configmap.go | 12 ++++++------ 6 files changed, 15 insertions(+), 12 deletions(-) diff --git a/charts/ingress-nginx/templates/controller-configmap.yaml b/charts/ingress-nginx/templates/controller-configmap.yaml index 662a162040..08009d0db0 100644 --- a/charts/ingress-nginx/templates/controller-configmap.yaml +++ b/charts/ingress-nginx/templates/controller-configmap.yaml @@ -14,6 +14,7 @@ metadata: namespace: {{ include "ingress-nginx.namespace" . }} data: allow-snippet-annotations: "{{ .Values.controller.allowSnippetAnnotations }}" + enable-serial-reloads: "{{ .Values.controller.enableSerialReloads }}" {{- if .Values.controller.addHeaders }} add-headers: {{ include "ingress-nginx.namespace" . }}/{{ include "ingress-nginx.fullname" . }}-custom-add-headers {{- end }} diff --git a/charts/ingress-nginx/values.yaml b/charts/ingress-nginx/values.yaml index e010a2dbbb..6324c6fcc6 100644 --- a/charts/ingress-nginx/values.yaml +++ b/charts/ingress-nginx/values.yaml @@ -88,6 +88,8 @@ controller: # when users add those annotations. # Global snippets in ConfigMap are still respected allowSnippetAnnotations: false + # -- This configuration defines if NGINX workers should reload serially instead of concurrently when multiple changes that require reloads are queued + enableSerialReloads: false # -- Required for use with CNI based kubernetes installations (such as ones set up by kubeadm), # since CNI and hostport don't mix yet. Can be deprecated once https://github.com/kubernetes/kubernetes/issues/23920 # is merged diff --git a/docs/user-guide/nginx-configuration/configmap.md b/docs/user-guide/nginx-configuration/configmap.md index 14124625b8..1599275f40 100644 --- a/docs/user-guide/nginx-configuration/configmap.md +++ b/docs/user-guide/nginx-configuration/configmap.md @@ -114,7 +114,7 @@ The following table shows a configuration option's name, type, and the default v |[worker-processes](#worker-processes)|string|``|| |[worker-cpu-affinity](#worker-cpu-affinity)|string|""|| |[worker-shutdown-timeout](#worker-shutdown-timeout)|string|"240s"|| -|[concurrently-reload-worker-processes](#concurrently-reload-worker-processes)|bool|"true"|| +|[enable-serial-reloads](#enable-serial-reloads)|bool|"false"|| |[load-balance](#load-balance)|string|"round_robin"|| |[variables-hash-bucket-size](#variables-hash-bucket-size)|int|128|| |[variables-hash-max-size](#variables-hash-max-size)|int|2048|| diff --git a/internal/ingress/controller/config/config.go b/internal/ingress/controller/config/config.go index e20b74fc71..e7f3bc1d01 100644 --- a/internal/ingress/controller/config/config.go +++ b/internal/ingress/controller/config/config.go @@ -479,7 +479,7 @@ type Configuration struct { // 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 - ConcurrentlyReloadWorkers bool `json:"concurrently-reload-worker-processes,omitempty"` + WorkerSerialReloads bool `json:"enable-serial-reloads,omitempty"` // Defines a timeout for a graceful shutdown of worker processes // http://nginx.org/en/docs/ngx_core_module.html#worker_shutdown_timeout @@ -849,7 +849,7 @@ func NewDefault() Configuration { UseGzip: false, UseGeoIP2: false, WorkerProcesses: strconv.Itoa(runtime.NumCPU()), - ConcurrentlyReloadWorkers: true, + WorkerSerialReloads: false, WorkerShutdownTimeout: "240s", VariablesHashBucketSize: 256, VariablesHashMaxSize: 2048, diff --git a/internal/ingress/controller/nginx.go b/internal/ingress/controller/nginx.go index dfc42dcb91..ba4a0fd17e 100644 --- a/internal/ingress/controller/nginx.go +++ b/internal/ingress/controller/nginx.go @@ -672,8 +672,8 @@ Error: %v // //nolint:gocritic // the cfg shouldn't be changed, and shouldn't be mutated by other processes while being rendered. func (n *NGINXController) OnUpdate(ingressCfg ingress.Configuration) error { - concurrentlyReloadWorkers := n.store.GetBackendConfiguration().ConcurrentlyReloadWorkers - if !concurrentlyReloadWorkers && n.workersReloading { + workerSerialReloads := n.store.GetBackendConfiguration().WorkerSerialReloads + if workerSerialReloads && n.workersReloading { return errors.New("worker reload already in progress, requeuing reload") } @@ -743,7 +743,7 @@ func (n *NGINXController) OnUpdate(ingressCfg ingress.Configuration) error { } // Reload status checking runs in a separate goroutine to avoid blocking the sync queue - if !concurrentlyReloadWorkers { + if workerSerialReloads { go n.awaitWorkersReload() } diff --git a/internal/ingress/controller/template/configmap.go b/internal/ingress/controller/template/configmap.go index 05fcffca4a..ee9170e7b8 100644 --- a/internal/ingress/controller/template/configmap.go +++ b/internal/ingress/controller/template/configmap.go @@ -67,7 +67,7 @@ const ( luaSharedDictsKey = "lua-shared-dicts" plugins = "plugins" debugConnections = "debug-connections" - concurrentlyReloadWorkers = "concurrently-reload-worker-processes" + workerSerialReloads = "enable-serial-reloads" ) var ( @@ -386,15 +386,15 @@ func ReadConfig(src map[string]string) config.Configuration { delete(conf, workerProcesses) } - if val, ok := conf[concurrentlyReloadWorkers]; ok { + if val, ok := conf[workerSerialReloads]; ok { boolVal, err := strconv.ParseBool(val) if err != nil { - to.ConcurrentlyReloadWorkers = true - klog.Warningf("failed to parse concurrently-reload-worker-processes setting, valid values are true or false, found %s", val) + to.WorkerSerialReloads = false + klog.Warningf("failed to parse enable-serial-reloads setting, valid values are true or false, found %s", val) } else { - to.ConcurrentlyReloadWorkers = boolVal + to.WorkerSerialReloads = boolVal } - delete(conf, concurrentlyReloadWorkers) + delete(conf, workerSerialReloads) } if val, ok := conf[plugins]; ok { From c61462eb1cf8ddf115532d4158111eaa0b8c89b3 Mon Sep 17 00:00:00 2001 From: Rafael da Fonseca Date: Mon, 22 Jan 2024 12:35:50 +0000 Subject: [PATCH 4/6] Update helm-docs Signed-off-by: Rafael da Fonseca --- charts/ingress-nginx/README.md | 1 + 1 file changed, 1 insertion(+) diff --git a/charts/ingress-nginx/README.md b/charts/ingress-nginx/README.md index 7c351e4c80..afd71e5bcc 100644 --- a/charts/ingress-nginx/README.md +++ b/charts/ingress-nginx/README.md @@ -298,6 +298,7 @@ As of version `1.26.0` of this chart, by simply not providing any clusterIP valu | controller.electionID | string | `""` | Election ID to use for status update, by default it uses the controller name combined with a suffix of 'leader' | | controller.enableAnnotationValidations | bool | `false` | | | controller.enableMimalloc | bool | `true` | Enable mimalloc as a drop-in replacement for malloc. # ref: https://github.com/microsoft/mimalloc # | +| controller.enableSerialReloads | bool | `false` | This configuration defines if NGINX workers should reload serially instead of concurrently when multiple changes that require reloads are queued | | controller.enableTopologyAwareRouting | bool | `false` | This configuration enables Topology Aware Routing feature, used together with service annotation service.kubernetes.io/topology-mode="auto" Defaults to false | | controller.existingPsp | string | `""` | Use an existing PSP instead of creating one | | controller.extraArgs | object | `{}` | Additional command line arguments to pass to Ingress-Nginx Controller E.g. to specify the default SSL certificate you can use | From c84034ce4cf9b064e3db4266830ffcd9f83be376 Mon Sep 17 00:00:00 2001 From: Rafael da Fonseca Date: Mon, 29 Apr 2024 14:37:57 +0100 Subject: [PATCH 5/6] Avoid calling GetBackendConfiguration() twice, use clearer name for helm chart option Signed-off-by: Rafael da Fonseca --- charts/ingress-nginx/README.md | 2 +- charts/ingress-nginx/templates/controller-configmap.yaml | 2 +- charts/ingress-nginx/values.yaml | 2 +- internal/ingress/controller/nginx.go | 8 ++++---- 4 files changed, 7 insertions(+), 7 deletions(-) diff --git a/charts/ingress-nginx/README.md b/charts/ingress-nginx/README.md index 4d909a6295..d8a3f5bb2d 100644 --- a/charts/ingress-nginx/README.md +++ b/charts/ingress-nginx/README.md @@ -300,7 +300,7 @@ As of version `1.26.0` of this chart, by simply not providing any clusterIP valu | controller.electionTTL | string | `""` | Duration a leader election is valid before it's getting re-elected, e.g. `15s`, `10m` or `1h`. (Default: 30s) | | controller.enableAnnotationValidations | bool | `false` | | | controller.enableMimalloc | bool | `true` | Enable mimalloc as a drop-in replacement for malloc. # ref: https://github.com/microsoft/mimalloc # | -| controller.enableSerialReloads | bool | `false` | This configuration defines if NGINX workers should reload serially instead of concurrently when multiple changes that require reloads are queued | +| controller.enableWorkerSerialReloads | bool | `false` | This configuration defines if NGINX workers should reload serially instead of concurrently when multiple changes that require reloads are queued | | controller.enableTopologyAwareRouting | bool | `false` | This configuration enables Topology Aware Routing feature, used together with service annotation service.kubernetes.io/topology-mode="auto" Defaults to false | | controller.existingPsp | string | `""` | Use an existing PSP instead of creating one | | controller.extraArgs | object | `{}` | Additional command line arguments to pass to Ingress-Nginx Controller E.g. to specify the default SSL certificate you can use | diff --git a/charts/ingress-nginx/templates/controller-configmap.yaml b/charts/ingress-nginx/templates/controller-configmap.yaml index c97eb2fca7..50ae824a87 100644 --- a/charts/ingress-nginx/templates/controller-configmap.yaml +++ b/charts/ingress-nginx/templates/controller-configmap.yaml @@ -14,7 +14,7 @@ metadata: namespace: {{ include "ingress-nginx.namespace" . }} data: allow-snippet-annotations: "{{ .Values.controller.allowSnippetAnnotations }}" - enable-serial-reloads: "{{ .Values.controller.enableSerialReloads }}" + enable-serial-reloads: "{{ .Values.controller.enableWorkerSerialReloads }}" {{- if .Values.controller.addHeaders }} add-headers: {{ include "ingress-nginx.namespace" . }}/{{ include "ingress-nginx.fullname" . }}-custom-add-headers {{- end }} diff --git a/charts/ingress-nginx/values.yaml b/charts/ingress-nginx/values.yaml index 77af4d373f..2a361614c1 100644 --- a/charts/ingress-nginx/values.yaml +++ b/charts/ingress-nginx/values.yaml @@ -94,7 +94,7 @@ controller: # Global snippets in ConfigMap are still respected allowSnippetAnnotations: false # -- This configuration defines if NGINX workers should reload serially instead of concurrently when multiple changes that require reloads are queued - enableSerialReloads: false + enableWorkerSerialReloads: false # -- Required for use with CNI based kubernetes installations (such as ones set up by kubeadm), # since CNI and hostport don't mix yet. Can be deprecated once https://github.com/kubernetes/kubernetes/issues/23920 # is merged diff --git a/internal/ingress/controller/nginx.go b/internal/ingress/controller/nginx.go index a1d64dc2d9..f74b3245e6 100644 --- a/internal/ingress/controller/nginx.go +++ b/internal/ingress/controller/nginx.go @@ -677,14 +677,14 @@ Error: %v // //nolint:gocritic // the cfg shouldn't be changed, and shouldn't be mutated by other processes while being rendered. func (n *NGINXController) OnUpdate(ingressCfg ingress.Configuration) error { - workerSerialReloads := n.store.GetBackendConfiguration().WorkerSerialReloads + cfg := n.store.GetBackendConfiguration() + cfg.Resolver = n.resolver + + workerSerialReloads := cfg.WorkerSerialReloads if workerSerialReloads && n.workersReloading { return errors.New("worker reload already in progress, requeuing reload") } - cfg := n.store.GetBackendConfiguration() - cfg.Resolver = n.resolver - content, err := n.generateTemplate(cfg, ingressCfg) if err != nil { return err From fc7f2615d46367ed3b31aa0d2041d559c50fc6d4 Mon Sep 17 00:00:00 2001 From: Rafael da Fonseca Date: Mon, 29 Apr 2024 14:41:05 +0100 Subject: [PATCH 6/6] Fix helm-docs ordering Signed-off-by: Rafael da Fonseca --- charts/ingress-nginx/README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/charts/ingress-nginx/README.md b/charts/ingress-nginx/README.md index d8a3f5bb2d..8ea6bae85e 100644 --- a/charts/ingress-nginx/README.md +++ b/charts/ingress-nginx/README.md @@ -300,8 +300,8 @@ As of version `1.26.0` of this chart, by simply not providing any clusterIP valu | controller.electionTTL | string | `""` | Duration a leader election is valid before it's getting re-elected, e.g. `15s`, `10m` or `1h`. (Default: 30s) | | controller.enableAnnotationValidations | bool | `false` | | | controller.enableMimalloc | bool | `true` | Enable mimalloc as a drop-in replacement for malloc. # ref: https://github.com/microsoft/mimalloc # | -| controller.enableWorkerSerialReloads | bool | `false` | This configuration defines if NGINX workers should reload serially instead of concurrently when multiple changes that require reloads are queued | | controller.enableTopologyAwareRouting | bool | `false` | This configuration enables Topology Aware Routing feature, used together with service annotation service.kubernetes.io/topology-mode="auto" Defaults to false | +| controller.enableWorkerSerialReloads | bool | `false` | This configuration defines if NGINX workers should reload serially instead of concurrently when multiple changes that require reloads are queued | | controller.existingPsp | string | `""` | Use an existing PSP instead of creating one | | controller.extraArgs | object | `{}` | Additional command line arguments to pass to Ingress-Nginx Controller E.g. to specify the default SSL certificate you can use | | controller.extraContainers | list | `[]` | Additional containers to be added to the controller pod. See https://github.com/lemonldap-ng-controller/lemonldap-ng-controller as example. |