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.

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

Revert to original main-template without pod downtime #6661

Merged
merged 12 commits into from
Oct 18, 2024
Merged

Conversation

jjngx
Copy link
Contributor

@jjngx jjngx commented Oct 15, 2024

Proposed changes

This PR introduces the following changes:

  • when a user deletes custom main-template from the ConfigMap obj, NIC will revert to the original, default template
  • there is no need to restart NIC pod and introduce downtime anymore

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

Testing:

  • deploy NIC and verify initial ConfigMap:
➜  common git:(fix/github-5906) ✗ k describe configmaps -n nginx-ingress nginx-config
Name:         nginx-config
Namespace:    nginx-ingress
Labels:       <none>
Annotations:  <none>

Data
====

BinaryData
====

Events:
  Type    Reason   Age                From                      Message
  ----    ------   ----               ----                      -------
  Normal  Updated  42m (x9 over 20h)  nginx-ingress-controller  Configuration from nginx-ingress/nginx-config was updated
  Normal  Updated  31m (x3 over 35m)  nginx-ingress-controller  Configuration from nginx-ingress/nginx-config was updated
  • check content of /etc/nginx/nginx.conf in the NIC pod
nginx@nginx-ingress-68449896cb-4jjpp:/etc/nginx$ head nginx.conf
worker_processes  auto;

daemon off;

error_log  stderr notice;
pid        /var/lib/nginx/nginx.pid;
load_module modules/ngx_fips_check_module.so;

load_module modules/ngx_http_js_module.so;
  • apply custom main-template using ConfigMap
    beginning of the custom main-template
➜  common git:(fix/github-5906) ✗ head nginx-config-main-template-02.yaml
kind: ConfigMap
apiVersion: v1
metadata:
  name: nginx-config
  namespace: nginx-ingress
data:
  main-template: |
    # VERSION 2
    # main template from config map
    # this is a test
  • verify the main template is applied
I20241015 12:15:56.435890   1 verify.go:90] success, version 4 ensured. took: 161.217208ms
I20241015 12:15:56.436318   1 controller.go:262] Event(v1.ObjectReference{Kind:"ConfigMap", Namespace:"nginx-ingress", Name:"nginx-config", UID:"ae356055-5493-47fc-b107-5d16e7a71c10", APIVersion:"v1", ResourceVersion:"185894", FieldPath:""}): type: 'Normal' reason: 'Updated' Configuration from nginx-ingress/nginx-config was updated
2024/10/15 12:15:56 [notice] 78#78: exit
nginx@nginx-ingress-68449896cb-4jjpp:/etc/nginx$ head nginx.conf
# VERSION 2
# main template from config map
# this is a test
#
worker_processes  auto;
daemon off;
error_log  stderr notice;
pid        /var/lib/nginx/nginx.pid;
load_module modules/ngx_fips_check_module.so;
load_module modules/ngx_http_js_module.so;
➜  common git:(fix/github-5906) ✗ k describe configmaps -n nginx-ingress nginx-config
Name:         nginx-config
Namespace:    nginx-ingress
Labels:       <none>
Annotations:  <none>

Data
====
main-template:
----
# VERSION 2
# main template from config map
# this is a test
#
worker_processes  auto;
daemon off;
error_log  stderr notice;
pid        /var/lib/nginx/nginx.pid;
load_module modules/ngx_fips_check_module.so;
load_module modules/ngx_http_js_module.so;

events {
    worker_connections  1024;
}
. . . 
  • remove the custom main-template from the ConfigMap obj (apply an empty ConfigMap)
➜  common git:(fix/github-5906) ✗ kaf nginx-config.yaml
configmap/nginx-config configured
➜  common git:(fix/github-5906) ✗ k describe configmaps -n nginx-ingress nginx-config
Name:         nginx-config
Namespace:    nginx-ingress
Labels:       <none>
Annotations:  <none>

Data
====

BinaryData
====

Events:
  Type    Reason   Age                From                      Message
  ----    ------   ----               ----                      -------
  Normal  Updated  51m (x9 over 21h)  nginx-ingress-controller  Configuration from nginx-ingress/nginx-config was updated
  Normal  Updated  60s (x5 over 44m)  nginx-ingress-controller  Configuration from nginx-ingress/nginx-config was updated
  • verify /etc/nginx/nginx.conf is reverted back to the original config file
nginx@nginx-ingress-68449896cb-4jjpp:/etc/nginx$ head nginx.conf
worker_processes  auto;

daemon off;

error_log  stderr notice;
pid        /var/lib/nginx/nginx.pid;
load_module modules/ngx_fips_check_module.so;

load_module modules/ngx_http_js_module.so;

@github-actions github-actions bot added bug An issue reporting a potential bug go Pull requests that update Go code labels Oct 15, 2024
Copy link

codecov bot commented Oct 15, 2024

Codecov Report

Attention: Patch coverage is 86.66667% with 2 lines in your changes missing coverage. Please review.

Project coverage is 53.24%. Comparing base (5d85ad8) to head (2e94d36).
Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
internal/configs/configmaps.go 50.00% 0 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6661      +/-   ##
==========================================
+ Coverage   53.01%   53.24%   +0.22%     
==========================================
  Files          87       87              
  Lines       20053    20063      +10     
==========================================
+ Hits        10632    10683      +51     
+ Misses       9008     8950      -58     
- Partials      413      430      +17     

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

@jjngx jjngx self-assigned this Oct 15, 2024
@jjngx jjngx marked this pull request as ready for review October 16, 2024 10:48
@jjngx jjngx requested a review from a team as a code owner October 16, 2024 10:48
@jjngx jjngx merged commit ef2d77c into main Oct 18, 2024
81 checks passed
@jjngx jjngx deleted the fix/github-5906 branch October 18, 2024 17:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug An issue reporting a potential bug go Pull requests that update Go code
Projects
Status: Done 🚀
Development

Successfully merging this pull request may close these issues.

After removing the custom template, NIC does not restore to default template without restarting NIC pod
3 participants