Skip to content
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

Jump to bottom

Create unique lease obj for each NIC installed via Helm #6372

Merged
merged 7 commits into from
Sep 10, 2024
Merged

Create unique lease obj for each NIC installed via Helm #6372

merged 7 commits into from
Sep 10, 2024

Conversation

jjngx
Copy link
Contributor

@jjngx jjngx commented Sep 9, 2024

Proposed changes

This PR addresses #5388.

User can install multiple NIC in the same K8s Namespace via Helm. Each time user installs the NIC, a new, unique name is generated for the Lease. User can also specify name for the Lease by setting up value for the controller.reportIngressStatus.leaderElectionLockName parameter.

Testing:

List deployed NICs:

➜  nginx-ingress git:(fix/helm-election) ✗ helm list
NAME                	NAMESPACE	REVISION	UPDATED                             	STATUS  	CHART              	APP VERSION
nginx-ingress-test-1	default  	1       	2024-09-08 21:25:51.638054 +0100 IST	deployed	nginx-ingress-1.4.0	3.7.0
nginx-ingress-test-2	default  	1       	2024-09-08 21:26:57.050805 +0100 IST	deployed	nginx-ingress-1.4.0	3.7.0
nginx-ingress-test-3	default  	1       	2024-09-08 21:28:13.50333 +0100 IST 	deployed	nginx-ingress-1.4.0	3.7.0
nginx-ingress-test-4	default  	1       	2024-09-08 21:30:49.769087 +0100 IST	deployed	nginx-ingress-1.4.0	3.7.0
➜  kubernetes-ingress git:(fix/helm-election) ✗ kubectl get leases.coordination.k8s.io
NAME                                   HOLDER                                             AGE
nginx-ingress-leader                   nginx-ingress-test-4-controller-688fbb96cc-s577t   9h
nginx-ingress-test-1-leader-election   nginx-ingress-test-1-controller-6cc7cbf4dc-bsnts   9h
nginx-ingress-test-2-leader-election   nginx-ingress-test-2-controller-5b4fd765c7-zlkqs   9h
nginx-ingress-test-3-leader-election   nginx-ingress-test-3-controller-bdbc77fd4-ftm7k    9h
  • 1st Lease name was explicitly provided when installing NIC via Helm.
  • 2nd, 3rd and 4th Lease names were generated automatically.

Checklist

Before creating a PR, run through this checklist and mark each as complete.

  • I have read the CONTRIBUTING doc
  • I have added tests that prove my fix is effective or that my feature works
  • I have checked that all unit tests pass after adding my changes
  • I have updated necessary documentation
  • I have rebased my branch onto main
  • I will ensure my PR is targeting the main branch and pulling from my branch from my own fork

@jjngx jjngx requested a review from a team as a code owner September 9, 2024 06:22
@github-actions github-actions bot added bug An issue reporting a potential bug helm_chart Pull requests that update the Helm Chart labels Sep 9, 2024
@jjngx
Copy link
Contributor Author

jjngx commented Sep 9, 2024

Deploy 2 NIC controllers using Helm:

➜  nginx-ingress git:(fix/helm-election) ✗ helm install nginx-ingress-test-1 .  --set controller.image.repository=nginx-ingress --set controller.image.tag=jjngx
walk.go:74: found symbolic link in path: /Users/j.jarosz/code/kubernetes-ingress/charts/nginx-ingress/crds resolves to /Users/j.jarosz/code/kubernetes-ingress/config/crd/bases. Contents of linked file included and used
NAME: nginx-ingress-test-1
LAST DEPLOYED: Mon Sep  9 13:37:11 2024
NAMESPACE: default
STATUS: deployed
REVISION: 1
TEST SUITE: None
NOTES:
NGINX Ingress Controller 3.7.0 has been installed.

For release notes for this version please see: https://docs.nginx.com/nginx-ingress-controller/releases/

Installation and upgrade instructions: https://docs.nginx.com/nginx-ingress-controller/installation/installing-nic/installation-with-helm/
➜  nginx-ingress git:(fix/helm-election) ✗ helm install nginx-ingress-test-2 .  --set controller.image.repository=nginx-ingress --set controller.image.tag=jjngx --set controller.ingressClass.name=nginx2
walk.go:74: found symbolic link in path: /Users/j.jarosz/code/kubernetes-ingress/charts/nginx-ingress/crds resolves to /Users/j.jarosz/code/kubernetes-ingress/config/crd/bases. Contents of linked file included and used
NAME: nginx-ingress-test-2
LAST DEPLOYED: Mon Sep  9 13:37:31 2024
NAMESPACE: default
STATUS: deployed
REVISION: 1
TEST SUITE: None
NOTES:
NGINX Ingress Controller 3.7.0 has been installed.

For release notes for this version please see: https://docs.nginx.com/nginx-ingress-controller/releases/

Installation and upgrade instructions: https://docs.nginx.com/nginx-ingress-controller/installation/installing-nic/installation-with-helm/

Check pods

➜  nginx-ingress git:(fix/helm-election) ✗ k get pods
NAME                                               READY   STATUS    RESTARTS   AGE
nginx-ingress-test-1-controller-6cc7cbf4dc-xh7tr   1/1     Running   0          24s
nginx-ingress-test-2-controller-5b4fd765c7-wrfkp   1/1     Running   0          4s

Check leases

➜  nginx-ingress git:(fix/helm-election) ✗ k get leases.coordination.k8s.io
NAME                                   HOLDER                                             AGE
nginx-ingress-test-1-leader-election   nginx-ingress-test-1-controller-6cc7cbf4dc-xh7tr   29s
nginx-ingress-test-2-leader-election   nginx-ingress-test-2-controller-5b4fd765c7-wrfkp   9s
➜  nginx-ingress git:(fix/helm-election) ✗ helm list
NAME                	NAMESPACE	REVISION	UPDATED                             	STATUS  	CHART              	APP VERSION
nginx-ingress-test-1	default  	1       	2024-09-09 13:37:11.543674 +0100 IST	deployed	nginx-ingress-1.4.0	3.7.0
nginx-ingress-test-2	default  	1       	2024-09-09 13:37:31.105931 +0100 IST	deployed	nginx-ingress-1.4.0	3.7.0

Uninstall NICs:

➜  nginx-ingress git:(fix/helm-election) ✗ k get leases.coordination.k8s.io
NAME                                   HOLDER                                             AGE
nginx-ingress-test-1-leader-election   nginx-ingress-test-1-controller-6cc7cbf4dc-xh7tr   29s
nginx-ingress-test-2-leader-election   nginx-ingress-test-2-controller-5b4fd765c7-wrfkp   9s
➜  nginx-ingress git:(fix/helm-election) ✗ helm list
NAME                	NAMESPACE	REVISION	UPDATED                             	STATUS  	CHART              	APP VERSION
nginx-ingress-test-1	default  	1       	2024-09-09 13:37:11.543674 +0100 IST	deployed	nginx-ingress-1.4.0	3.7.0
nginx-ingress-test-2	default  	1       	2024-09-09 13:37:31.105931 +0100 IST	deployed	nginx-ingress-1.4.0	3.7.0

➜  nginx-ingress git:(fix/helm-election) ✗ k get leases
NAME                                   HOLDER                                             AGE
nginx-ingress-test-1-leader-election   nginx-ingress-test-1-controller-6cc7cbf4dc-xh7tr   4m26s
nginx-ingress-test-2-leader-election   nginx-ingress-test-2-controller-5b4fd765c7-wrfkp   4m6s

Uninstall:

➜  nginx-ingress git:(fix/helm-election) ✗ helm uninstall nginx-ingress-test-1
release "nginx-ingress-test-1" uninstalled

Verify:

➜  nginx-ingress git:(fix/helm-election) ✗ k get leases
NAME                                   HOLDER                                             AGE
nginx-ingress-test-2-leader-election   nginx-ingress-test-2-controller-5b4fd765c7-wrfkp   4m25s
➜  nginx-ingress git:(fix/helm-election) ✗ helm uninstall nginx-ingress-test-2
release "nginx-ingress-test-2" uninstalled
➜  nginx-ingress git:(fix/helm-election) ✗ k get leases
No resources found in default namespace.

@jjngx jjngx linked an issue Sep 9, 2024 that may be closed by this pull request
Ensure a unique lease object is created for each install of NIC via Helm #5388
Closed
@codecov Codecov
Copy link

codecov bot commented Sep 9, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 53.20%. Comparing base (a83b7be) to head (a1f5fa4).
Report is 11 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #6372      +/-   ##
==========================================
+ Coverage   53.14%   53.20%   +0.05%     
==========================================
  Files          81       83       +2     
  Lines       15967    16002      +35     
==========================================
+ Hits         8486     8514      +28     
- Misses       7076     7081       +5     
- Partials      405      407       +2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@vepatel vepatel self-requested a review September 9, 2024 15:55
@jjngx
Copy link
Contributor Author

jjngx commented Sep 10, 2024 •
edited
Loading

Testing upgrade with three replicas:

step 1

➜  nginx-ingress git:(fix/helm-election) ✗ helm install ngx36 oci://ghcr.io/nginxinc/charts/nginx-ingress --version 1.3.2 --set controller.replicaCount=3
Pulled: ghcr.io/nginxinc/charts/nginx-ingress:1.3.2
Digest: sha256:c2810b728c7f735d26ab024ec4569037c1eb2e11ff1fb2e867dccbef3b1dfcf9
NAME: ngx36
LAST DEPLOYED: Tue Sep 10 15:13:10 2024
NAMESPACE: default
STATUS: deployed
REVISION: 1
TEST SUITE: None
NOTES:
NGINX Ingress Controller 3.6.2 has been installed.

For release notes for this version please see: https://docs.nginx.com/nginx-ingress-controller/releases/

Installation and upgrade instructions: https://docs.nginx.com/nginx-ingress-controller/installation/installing-nic/installation-with-helm/

step 2

➜  nginx-ingress git:(fix/helm-election) ✗ k get pods
NAME                                              READY   STATUS    RESTARTS   AGE
ngx36-nginx-ingress-controller-5dd467744c-6w6zk   1/1     Running   0          59s
ngx36-nginx-ingress-controller-5dd467744c-ntlpf   1/1     Running   0          59s
ngx36-nginx-ingress-controller-5dd467744c-pn6x4   1/1     Running   0          59s
➜  nginx-ingress git:(fix/helm-election) ✗ k get leases.coordination.k8s.io
NAME                   HOLDER                                            AGE
nginx-ingress-leader   ngx36-nginx-ingress-controller-5dd467744c-6w6zk   45s

➜  nginx-ingress git:(fix/helm-election) ✗ helm list
NAME 	NAMESPACE	REVISION	UPDATED                             	STATUS  	CHART              	APP VERSION
ngx36	default  	1       	2024-09-10 15:13:10.134175 +0100 IST	deployed	nginx-ingress-1.3.2	3.6.2

step 3 - CRDs

➜  nginx-ingress git:(fix/helm-election) ✗ k get customresourcedefinitions.apiextensions.k8s.io
NAME                                         CREATED AT
apdoslogconfs.appprotectdos.f5.com           2024-09-10T14:13:09Z
apdospolicies.appprotectdos.f5.com           2024-09-10T14:13:09Z
aplogconfs.appprotect.f5.com                 2024-09-10T14:13:09Z
appolicies.appprotect.f5.com                 2024-09-10T14:13:09Z
apusersigs.appprotect.f5.com                 2024-09-10T14:13:09Z
dnsendpoints.externaldns.nginx.org           2024-09-10T14:13:09Z
dosprotectedresources.appprotectdos.f5.com   2024-09-10T14:13:09Z
globalconfigurations.k8s.nginx.org           2024-09-10T14:13:09Z
policies.k8s.nginx.org                       2024-09-10T14:13:09Z
transportservers.k8s.nginx.org               2024-09-10T14:13:09Z
virtualserverroutes.k8s.nginx.org            2024-09-10T14:13:09Z
virtualservers.k8s.nginx.org                 2024-09-10T14:13:09Z
➜  nginx-ingress git:(fix/helm-election) ✗ kaf ../../config/crd/bases
Warning: resource customresourcedefinitions/aplogconfs.appprotect.f5.com is missing the kubectl.kubernetes.io/last-applied-configuration annotation which is required by kubectl apply. kubectl apply should only be used on resources created declaratively by either kubectl create --save-config or kubectl apply. The missing annotation will be patched automatically.
customresourcedefinition.apiextensions.k8s.io/aplogconfs.appprotect.f5.com configured
Warning: resource customresourcedefinitions/appolicies.appprotect.f5.com is missing the kubectl.kubernetes.io/last-applied-configuration annotation which is required by kubectl apply. kubectl apply should only be used on resources created declaratively by either kubectl create --save-config or kubectl apply. The missing annotation will be patched automatically.
customresourcedefinition.apiextensions.k8s.io/appolicies.appprotect.f5.com configured
Warning: resource customresourcedefinitions/apusersigs.appprotect.f5.com is missing the kubectl.kubernetes.io/last-applied-configuration annotation which is required by kubectl apply. kubectl apply should only be used on resources created declaratively by either kubectl create --save-config or kubectl apply. The missing annotation will be patched automatically.
customresourcedefinition.apiextensions.k8s.io/apusersigs.appprotect.f5.com configured
Warning: resource customresourcedefinitions/apdoslogconfs.appprotectdos.f5.com is missing the kubectl.kubernetes.io/last-applied-configuration annotation which is required by kubectl apply. kubectl apply should only be used on resources created declaratively by either kubectl create --save-config or kubectl apply. The missing annotation will be patched automatically.
customresourcedefinition.apiextensions.k8s.io/apdoslogconfs.appprotectdos.f5.com configured
Warning: resource customresourcedefinitions/apdospolicies.appprotectdos.f5.com is missing the kubectl.kubernetes.io/last-applied-configuration annotation which is required by kubectl apply. kubectl apply should only be used on resources created declaratively by either kubectl create --save-config or kubectl apply. The missing annotation will be patched automatically.
customresourcedefinition.apiextensions.k8s.io/apdospolicies.appprotectdos.f5.com configured
Warning: resource customresourcedefinitions/dosprotectedresources.appprotectdos.f5.com is missing the kubectl.kubernetes.io/last-applied-configuration annotation which is required by kubectl apply. kubectl apply should only be used on resources created declaratively by either kubectl create --save-config or kubectl apply. The missing annotation will be patched automatically.
customresourcedefinition.apiextensions.k8s.io/dosprotectedresources.appprotectdos.f5.com configured
Warning: resource customresourcedefinitions/dnsendpoints.externaldns.nginx.org is missing the kubectl.kubernetes.io/last-applied-configuration annotation which is required by kubectl apply. kubectl apply should only be used on resources created declaratively by either kubectl create --save-config or kubectl apply. The missing annotation will be patched automatically.
customresourcedefinition.apiextensions.k8s.io/dnsendpoints.externaldns.nginx.org configured
Warning: resource customresourcedefinitions/globalconfigurations.k8s.nginx.org is missing the kubectl.kubernetes.io/last-applied-configuration annotation which is required by kubectl apply. kubectl apply should only be used on resources created declaratively by either kubectl create --save-config or kubectl apply. The missing annotation will be patched automatically.
customresourcedefinition.apiextensions.k8s.io/globalconfigurations.k8s.nginx.org configured
Warning: resource customresourcedefinitions/policies.k8s.nginx.org is missing the kubectl.kubernetes.io/last-applied-configuration annotation which is required by kubectl apply. kubectl apply should only be used on resources created declaratively by either kubectl create --save-config or kubectl apply. The missing annotation will be patched automatically.
customresourcedefinition.apiextensions.k8s.io/policies.k8s.nginx.org configured
Warning: resource customresourcedefinitions/transportservers.k8s.nginx.org is missing the kubectl.kubernetes.io/last-applied-configuration annotation which is required by kubectl apply. kubectl apply should only be used on resources created declaratively by either kubectl create --save-config or kubectl apply. The missing annotation will be patched automatically.
customresourcedefinition.apiextensions.k8s.io/transportservers.k8s.nginx.org configured
Warning: resource customresourcedefinitions/virtualserverroutes.k8s.nginx.org is missing the kubectl.kubernetes.io/last-applied-configuration annotation which is required by kubectl apply. kubectl apply should only be used on resources created declaratively by either kubectl create --save-config or kubectl apply. The missing annotation will be patched automatically.
customresourcedefinition.apiextensions.k8s.io/virtualserverroutes.k8s.nginx.org configured
Warning: resource customresourcedefinitions/virtualservers.k8s.nginx.org is missing the kubectl.kubernetes.io/last-applied-configuration annotation which is required by kubectl apply. kubectl apply should only be used on resources created declaratively by either kubectl create --save-config or kubectl apply. The missing annotation will be patched automatically.
customresourcedefinition.apiextensions.k8s.io/virtualservers.k8s.nginx.org configured

step 4 - upgrade helm (NIC 3.7.0 from main br)

➜  nginx-ingress git:(fix/helm-election) ✗ helm upgrade ngx36 .
walk.go:74: found symbolic link in path: /Users/j.jarosz/code/kubernetes-ingress/charts/nginx-ingress/crds resolves to /Users/j.jarosz/code/kubernetes-ingress/config/crd/bases. Contents of linked file included and used
Release "ngx36" has been upgraded. Happy Helming!
NAME: ngx36
LAST DEPLOYED: Tue Sep 10 15:16:49 2024
NAMESPACE: default
STATUS: deployed
REVISION: 2
TEST SUITE: None
NOTES:
NGINX Ingress Controller 3.7.0 has been installed.

For release notes for this version please see: https://docs.nginx.com/nginx-ingress-controller/releases/

Installation and upgrade instructions: https://docs.nginx.com/nginx-ingress-controller/installation/installing-nic/installation-with-helm/

If you are upgrading from a version of the chart that uses older Custom Resource Definitions (CRD) it is necessary to manually upgrade the CRDs as this is not managed by Helm.
To update to the latest version of the CRDs:
  $ kubectl apply -f https://raw.githubusercontent.com/nginxinc/kubernetes-ingress/v3.7.0/deploy/crds.yaml

More details on upgrading the CRDs: https://docs.nginx.com/nginx-ingress-controller/installation/installing-nic/installation-with-helm/#upgrading-the-crds
➜  nginx-ingress git:(fix/helm-election) ✗ k get pods
NAME                                              READY   STATUS    RESTARTS   AGE
ngx36-nginx-ingress-controller-787775887d-4mqzb   1/1     Running   0          24s
ngx36-nginx-ingress-controller-787775887d-7ftnd   1/1     Running   0          30s
ngx36-nginx-ingress-controller-787775887d-kcxsq   1/1     Running   0          27s
➜  nginx-ingress git:(fix/helm-election) ✗ k logs ngx36-nginx-ingress-controller-787775887d-4mqzb
NGINX Ingress Controller Version=3.7.0-SNAPSHOT Commit=a0e0cb7b4e76570017ca5df4c9070aa3216be40c Date=2024-09-09T12:36:23Z DirtyState=true Arch=linux/amd64 Go=go1.23.1
I20240910 14:16:58.704812       1 flags.go:315] Starting with flags: ["-nginx-plus=false" "-nginx-reload-timeout=60000" "-enable-app-protect=false" "-enable-app-protect-dos=false" "-nginx-configmaps=default/ngx36-nginx-ingress" "-ingress-class=nginx" "-health-status=false" "-health-status-uri=/nginx-health" "-nginx-debug=false" "-v=1" "-nginx-status=true" "-nginx-status-port=8080" "-nginx-status-allow-cidrs=127.0.0.1" "-report-ingress-status" "-external-service=ngx36-nginx-ingress-controller" "-enable-leader-election=true" "-leader-election-lock-name=ngx36-nginx-ingress-leader-election" "-enable-prometheus-metrics=true" "-prometheus-metrics-listen-port=9113" "-prometheus-tls-secret=" "-enable-service-insight=false" "-service-insight-listen-port=9114" "-service-insight-tls-secret=" "-enable-custom-resources=true" "-enable-snippets=false" "-disable-ipv6=false" "-enable-tls-passthrough=false" "-enable-cert-manager=false" "-enable-oidc=false" "-enable-external-dns=false" "-default-http-listener-port=80" "-default-https-listener-port=443" "-ready-status=true" "-ready-status-port=8081" "-enable-latency-metrics=false" "-ssl-dynamic-reload=true" "-enable-telemetry-reporting=true" "-weight-changes-dynamic-reload=false"]
I20240910 14:16:58.744217       1 main.go:297] Kubernetes version: 1.31.0
I20240910 14:16:58.765487       1 main.go:444] Using nginx version: nginx/1.27.1
I20240910 14:16:58.789506       1 main.go:872] Pod label updated: ngx36-nginx-ingress-controller-787775887d-4mqzb
2024/09/10 14:16:58 [notice] 22#22: using the "epoll" event method
2024/09/10 14:16:58 [notice] 22#22: nginx/1.27.1
2024/09/10 14:16:58 [notice] 22#22: built by gcc 12.2.0 (Debian 12.2.0-14)
2024/09/10 14:16:58 [notice] 22#22: OS: Linux 6.10.0-linuxkit
2024/09/10 14:16:58 [notice] 22#22: getrlimit(RLIMIT_NOFILE): 1048576:1048576
2024/09/10 14:16:58 [notice] 22#22: start worker processes
2024/09/10 14:16:58 [notice] 22#22: start worker process 24
. . .
2024/09/10 14:16:52 [notice] 22#22: start worker process 32
I20240910 14:16:52.993610       1 listener.go:50] Starting prometheus listener on: :9113/metrics
I0910 14:16:53.003432       1 leaderelection.go:254] attempting to acquire leader lease default/ngx36-nginx-ingress-leader-election...
I0910 14:16:53.069473       1 leaderelection.go:268] successfully acquired lease default/ngx36-nginx-ingress-leader-election
2024/09/10 14:16:53 [notice] 22#22: signal 1 (SIGHUP) received from 38, reconfiguring
2024/09/10 14:16:53 [notice] 22#22: reconfiguring
. . .

Remove old Lease

➜  nginx-ingress git:(fix/helm-election) ✗ k get leases.coordination.k8s.io
NAME                                  HOLDER                                            AGE
nginx-ingress-leader                  ngx36-nginx-ingress-controller-5dd467744c-6w6zk   4m19s
ngx36-nginx-ingress-leader-election   ngx36-nginx-ingress-controller-787775887d-7ftnd   58s
➜  nginx-ingress git:(fix/helm-election) ✗ k delete leases.coordination.k8s.io nginx-ingress-leader
lease.coordination.k8s.io "nginx-ingress-leader" deleted
➜  nginx-ingress git:(fix/helm-election) ✗ k get leases.coordination.k8s.io
NAME                                  HOLDER                                            AGE
ngx36-nginx-ingress-leader-election   ngx36-nginx-ingress-controller-787775887d-7ftnd   74s

@github-actions GitHub Actions
Copy link

github-actions bot commented Sep 10, 2024 •
edited
Loading

✅ All required contributors have signed the F5 CLA for this PR. Thank you!
Posted by the CLA Assistant Lite bot.

@jjngx
Copy link
Contributor Author

jjngx commented Sep 10, 2024

I have hereby read the F5 CLA and agree to its terms

@jjngx jjngx merged commit e6b9db3 into main Sep 10, 2024
79 checks passed
@jjngx jjngx deleted the fix/helm-election branch September 10, 2024 16:20
@shaun-nx shaun-nx mentioned this pull request Sep 11, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pdabelf5 pdabelf5 pdabelf5 approved these changes

@vepatel vepatel vepatel approved these changes

Assignees
No one assigned
Labels
bug An issue reporting a potential bug helm_chart Pull requests that update the Helm Chart
Projects
NGINX Ingress Controller
Status: Done 🚀
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Ensure a unique lease object is created for each install of NIC via Helm
3 participants
@jjngx @vepatel @pdabelf5