From ba2040ec592b802be0574eb085edabe017be8de0 Mon Sep 17 00:00:00 2001 From: Matheus Fidelis Date: Tue, 19 Mar 2024 19:20:36 -0300 Subject: [PATCH 1/2] feature(leader_ttl): feature to customize ttl to leader be re-elected --- charts/ingress-nginx/README.md | 1 + charts/ingress-nginx/templates/_params.tpl | 3 + charts/ingress-nginx/values.yaml | 2 + docs/user-guide/cli-arguments.md | 1 + internal/ingress/controller/controller.go | 1 + internal/ingress/controller/nginx.go | 5 +- internal/ingress/controller/status.go | 11 ++-- pkg/flags/flags.go | 8 +++ pkg/flags/flags_test.go | 69 ++++++++++++++++++++++ 9 files changed, 93 insertions(+), 8 deletions(-) diff --git a/charts/ingress-nginx/README.md b/charts/ingress-nginx/README.md index 1f912ea09ef..f5ec8f06a9c 100644 --- a/charts/ingress-nginx/README.md +++ b/charts/ingress-nginx/README.md @@ -297,6 +297,7 @@ As of version `1.26.0` of this chart, by simply not providing any clusterIP valu | controller.dnsConfig | object | `{}` | Optionally customize the pod dnsConfig. | | controller.dnsPolicy | string | `"ClusterFirst"` | Optionally change this to ClusterFirstWithHostNet in case you have 'hostNetwork: true'. By default, while using host network, name resolution uses the host's DNS. If you wish nginx-controller to keep resolving names inside the k8s network, use ClusterFirstWithHostNet. | | controller.electionID | string | `""` | Election ID to use for status update, by default it uses the controller name combined with a suffix of 'leader' | +| controller.electionTTL | string | `""` | Duration a leader election is valid before it's getting re-elected. If not set, the default value is 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.enableTopologyAwareRouting | bool | `false` | This configuration enables Topology Aware Routing feature, used together with service annotation service.kubernetes.io/topology-mode="auto" Defaults to false | diff --git a/charts/ingress-nginx/templates/_params.tpl b/charts/ingress-nginx/templates/_params.tpl index b2b54db1942..48569a8b0c6 100644 --- a/charts/ingress-nginx/templates/_params.tpl +++ b/charts/ingress-nginx/templates/_params.tpl @@ -63,6 +63,9 @@ {{- if .Values.controller.disableLeaderElection }} - --disable-leader-election=true {{- end }} +{{- if .Values.controller.electionTTL }} +- --election-ttl={{ .Values.controller.electionTTL }} +{{- end }} {{- range $key, $value := .Values.controller.extraArgs }} {{- /* Accept keys without values or with false as value */}} {{- if eq ($value | quote | len) 2 }} diff --git a/charts/ingress-nginx/values.yaml b/charts/ingress-nginx/values.yaml index 5be9519dfbd..25277deb30b 100644 --- a/charts/ingress-nginx/values.yaml +++ b/charts/ingress-nginx/values.yaml @@ -85,6 +85,8 @@ controller: enableTopologyAwareRouting: false # -- This configuration disable Nginx Controller Leader Election disableLeaderElection: false + # -- Duration a leader election is valid before it's getting re-elected. If not set, the default value is 30s. + electionTTL: "" # -- This configuration defines if Ingress Controller should allow users to set # their own *-snippet annotations, otherwise this is forbidden / dropped # when users add those annotations. diff --git a/docs/user-guide/cli-arguments.md b/docs/user-guide/cli-arguments.md index 275c753ec22..cfdbd3074f5 100644 --- a/docs/user-guide/cli-arguments.md +++ b/docs/user-guide/cli-arguments.md @@ -22,6 +22,7 @@ They are set in the container spec of the `ingress-nginx-controller` Deployment | `--disable-sync-events` | Disables the creation of 'Sync' Event resources, but still logs them | | `--dynamic-configuration-retries` | Number of times to retry failed dynamic configuration before failing to sync an ingress. (default 15) | | `--election-id` | Election id to use for Ingress status updates. (default "ingress-controller-leader") | +| `--election-ttl` | Duration a leader election is valid before it's getting re-elected. (Default: 30s) | | `--enable-metrics` | Enables the collection of NGINX metrics. (default true) | | `--enable-ssl-chain-completion` | Autocomplete SSL certificate chains with missing intermediate CA certificates. Certificates uploaded to Kubernetes must have the "Authority Information Access" X.509 v3 extension for this to succeed. (default false)| | `--enable-ssl-passthrough` | Enable SSL Passthrough. (default false) | diff --git a/internal/ingress/controller/controller.go b/internal/ingress/controller/controller.go index 495fead1549..db786a15c95 100644 --- a/internal/ingress/controller/controller.go +++ b/internal/ingress/controller/controller.go @@ -91,6 +91,7 @@ type Configuration struct { UpdateStatus bool UseNodeInternalIP bool ElectionID string + ElectionTTL time.Duration UpdateStatusOnShutdown bool HealthCheckHost string diff --git a/internal/ingress/controller/nginx.go b/internal/ingress/controller/nginx.go index 1d385e50633..0df5409f4da 100644 --- a/internal/ingress/controller/nginx.go +++ b/internal/ingress/controller/nginx.go @@ -274,8 +274,9 @@ func (n *NGINXController) Start() { if !n.cfg.DisableLeaderElection { electionID := n.cfg.ElectionID setupLeaderElection(&leaderElectionConfig{ - Client: n.cfg.Client, - ElectionID: electionID, + Client: n.cfg.Client, + ElectionID: electionID, + ElectionTTL: n.cfg.ElectionTTL, OnStartedLeading: func(stopCh chan struct{}) { if n.syncStatus != nil { go n.syncStatus.Run(stopCh) diff --git a/internal/ingress/controller/status.go b/internal/ingress/controller/status.go index 6bab7c2ada8..5a169e1c334 100644 --- a/internal/ingress/controller/status.go +++ b/internal/ingress/controller/status.go @@ -36,7 +36,8 @@ import ( type leaderElectionConfig struct { Client clientset.Interface - ElectionID string + ElectionID string + ElectionTTL time.Duration OnStartedLeading func(chan struct{}) OnStoppedLeading func() @@ -107,13 +108,11 @@ func setupLeaderElection(config *leaderElectionConfig) { LockConfig: resourceLockConfig, } - ttl := 30 * time.Second - elector, err = leaderelection.NewLeaderElector(leaderelection.LeaderElectionConfig{ Lock: lock, - LeaseDuration: ttl, - RenewDeadline: ttl / 2, - RetryPeriod: ttl / 4, + LeaseDuration: config.ElectionTTL, + RenewDeadline: config.ElectionTTL / 2, + RetryPeriod: config.ElectionTTL / 4, Callbacks: callbacks, }) diff --git a/pkg/flags/flags.go b/pkg/flags/flags.go index d7157fa7958..5891f636b31 100644 --- a/pkg/flags/flags.go +++ b/pkg/flags/flags.go @@ -132,6 +132,9 @@ Requires setting the publish-service parameter to a valid Service reference.`) electionID = flags.String("election-id", "ingress-controller-leader", `Election id to use for Ingress status updates.`) + electionTTL = flags.Duration("election-ttl", 30*time.Second, + `Duration a leader election is valid before it's getting re-elected`) + updateStatusOnShutdown = flags.Bool("update-status-on-shutdown", true, `Update the load-balancer status of Ingress objects when the controller shuts down. Requires the update-status parameter.`) @@ -314,6 +317,10 @@ https://blog.maxmind.com/2019/12/18/significant-changes-to-accessing-and-using-g } } + if *electionTTL <= 0 { + *electionTTL = 30 * time.Second + } + histogramBuckets := &collectors.HistogramBuckets{ TimeBuckets: *timeBuckets, LengthBuckets: *lengthBuckets, @@ -327,6 +334,7 @@ https://blog.maxmind.com/2019/12/18/significant-changes-to-accessing-and-using-g KubeConfigFile: *kubeConfigFile, UpdateStatus: *updateStatus, ElectionID: *electionID, + ElectionTTL: *electionTTL, EnableProfiling: *profiling, EnableMetrics: *enableMetrics, MetricsPerHost: *metricsPerHost, diff --git a/pkg/flags/flags_test.go b/pkg/flags/flags_test.go index 54b35dbca36..e51d5fa6ccb 100644 --- a/pkg/flags/flags_test.go +++ b/pkg/flags/flags_test.go @@ -19,6 +19,7 @@ package flags import ( "os" "testing" + "time" ) func TestNoMandatoryFlag(t *testing.T) { @@ -143,3 +144,71 @@ func TestIfLeaderElectionDisabledFlagIsFalse(t *testing.T) { t.Fatalf("Expected --disable-leader-election and conf.DisableLeaderElection as false, but found: %v", conf.DisableLeaderElection) } } + +func TestLeaderElectionTTLDefaultValue(t *testing.T) { + ResetForTesting(func() { t.Fatal("Parsing failed") }) + + oldArgs := os.Args + defer func() { os.Args = oldArgs }() + os.Args = []string{"cmd", "--http-port", "80", "--https-port", "443"} + + _, conf, err := ParseFlags() + if err != nil { + t.Fatalf("Unexpected error parsing default flags: %v", err) + } + + if conf.ElectionTTL != 30*time.Second { + t.Fatalf("Expected --election-ttl and conf.ElectionTTL as 30s, but found: %v", conf.ElectionTTL) + } +} + +func TestLeaderElectionTTLParseValueInSeconds(t *testing.T) { + ResetForTesting(func() { t.Fatal("Parsing failed") }) + + oldArgs := os.Args + defer func() { os.Args = oldArgs }() + os.Args = []string{"cmd", "--http-port", "80", "--https-port", "443", "--election-ttl", "10s"} + + _, conf, err := ParseFlags() + if err != nil { + t.Fatalf("Unexpected error parsing default flags: %v", err) + } + + if conf.ElectionTTL != 10*time.Second { + t.Fatalf("Expected --election-ttl and conf.ElectionTTL as 10s, but found: %v", conf.ElectionTTL) + } +} + +func TestLeaderElectionTTLParseValueInMinutes(t *testing.T) { + ResetForTesting(func() { t.Fatal("Parsing failed") }) + + oldArgs := os.Args + defer func() { os.Args = oldArgs }() + os.Args = []string{"cmd", "--http-port", "80", "--https-port", "443", "--election-ttl", "10m"} + + _, conf, err := ParseFlags() + if err != nil { + t.Fatalf("Unexpected error parsing default flags: %v", err) + } + + if conf.ElectionTTL != 10*time.Minute { + t.Fatalf("Expected --election-ttl and conf.ElectionTTL as 10m, but found: %v", conf.ElectionTTL) + } +} + +func TestLeaderElectionTTLParseValueInHours(t *testing.T) { + ResetForTesting(func() { t.Fatal("Parsing failed") }) + + oldArgs := os.Args + defer func() { os.Args = oldArgs }() + os.Args = []string{"cmd", "--http-port", "80", "--https-port", "443", "--election-ttl", "1h"} + + _, conf, err := ParseFlags() + if err != nil { + t.Fatalf("Unexpected error parsing default flags: %v", err) + } + + if conf.ElectionTTL != 1*time.Hour { + t.Fatalf("Expected --election-ttl and conf.ElectionTTL as 1h, but found: %v", conf.ElectionTTL) + } +} From 76fe2160f8a3c552d0739d7b8d280efc438231fc Mon Sep 17 00:00:00 2001 From: Matheus Fidelis Date: Thu, 28 Mar 2024 09:13:58 -0300 Subject: [PATCH 2/2] fix(review): docs --- charts/ingress-nginx/README.md | 2 +- charts/ingress-nginx/values.yaml | 2 +- docs/user-guide/cli-arguments.md | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/charts/ingress-nginx/README.md b/charts/ingress-nginx/README.md index f5ec8f06a9c..e7298b5b7fe 100644 --- a/charts/ingress-nginx/README.md +++ b/charts/ingress-nginx/README.md @@ -297,7 +297,7 @@ As of version `1.26.0` of this chart, by simply not providing any clusterIP valu | controller.dnsConfig | object | `{}` | Optionally customize the pod dnsConfig. | | controller.dnsPolicy | string | `"ClusterFirst"` | Optionally change this to ClusterFirstWithHostNet in case you have 'hostNetwork: true'. By default, while using host network, name resolution uses the host's DNS. If you wish nginx-controller to keep resolving names inside the k8s network, use ClusterFirstWithHostNet. | | controller.electionID | string | `""` | Election ID to use for status update, by default it uses the controller name combined with a suffix of 'leader' | -| controller.electionTTL | string | `""` | Duration a leader election is valid before it's getting re-elected. If not set, the default value is 30s. | +| 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.enableTopologyAwareRouting | bool | `false` | This configuration enables Topology Aware Routing feature, used together with service annotation service.kubernetes.io/topology-mode="auto" Defaults to false | diff --git a/charts/ingress-nginx/values.yaml b/charts/ingress-nginx/values.yaml index 25277deb30b..3741323c468 100644 --- a/charts/ingress-nginx/values.yaml +++ b/charts/ingress-nginx/values.yaml @@ -85,7 +85,7 @@ controller: enableTopologyAwareRouting: false # -- This configuration disable Nginx Controller Leader Election disableLeaderElection: false - # -- Duration a leader election is valid before it's getting re-elected. If not set, the default value is 30s. + # -- Duration a leader election is valid before it's getting re-elected, e.g. `15s`, `10m` or `1h`. (Default: 30s) electionTTL: "" # -- This configuration defines if Ingress Controller should allow users to set # their own *-snippet annotations, otherwise this is forbidden / dropped diff --git a/docs/user-guide/cli-arguments.md b/docs/user-guide/cli-arguments.md index cfdbd3074f5..f8fdc2ddbb9 100644 --- a/docs/user-guide/cli-arguments.md +++ b/docs/user-guide/cli-arguments.md @@ -22,7 +22,7 @@ They are set in the container spec of the `ingress-nginx-controller` Deployment | `--disable-sync-events` | Disables the creation of 'Sync' Event resources, but still logs them | | `--dynamic-configuration-retries` | Number of times to retry failed dynamic configuration before failing to sync an ingress. (default 15) | | `--election-id` | Election id to use for Ingress status updates. (default "ingress-controller-leader") | -| `--election-ttl` | Duration a leader election is valid before it's getting re-elected. (Default: 30s) | +| `--election-ttl` | Duration a leader election is valid before it's getting re-elected, e.g. `15s`, `10m` or `1h`. (Default: 30s) | | `--enable-metrics` | Enables the collection of NGINX metrics. (default true) | | `--enable-ssl-chain-completion` | Autocomplete SSL certificate chains with missing intermediate CA certificates. Certificates uploaded to Kubernetes must have the "Authority Information Access" X.509 v3 extension for this to succeed. (default false)| | `--enable-ssl-passthrough` | Enable SSL Passthrough. (default false) |