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

Remove reloads when there is no endpoints #3367

Merged
merged 1 commit into from
Nov 6, 2018

Conversation

aledbf
Copy link
Member

@aledbf aledbf commented Nov 5, 2018

Which issue this PR fixes: fixes #3315

This PR removes the need to reload NGINX when there is a change in the number of endpoints (in particular 0).
Without this PR, when the replica count is 0 we had this

I1106 12:19:13.390606       6 nginx.go:629] NGINX configuration diff:
--- /etc/nginx/nginx.conf	2018-11-06 12:18:56.514692638 +0000
+++ /tmp/new-nginx-cfg561164329	2018-11-06 12:19:13.387865859 +0000
@@ -1,5 +1,5 @@
 
-# Configuration checksum: 14086898480532449296
+# Configuration checksum: 8624603592146078350
 
 # setup custom paths that do not require root access
 pid /tmp/nginx.pid;
@@ -533,7 +533,7 @@
 			
 			port_in_redirect off;
 			
-			set $proxy_upstream_name "default-http-svc-80";
+			set $proxy_upstream_name "";
 			
 			client_max_body_size                    1m;

which triggered the reload. With the introduced changes there is no difference in the configuration file

@k8s-ci-robot k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Nov 5, 2018
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Nov 5, 2018
@aledbf aledbf added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 5, 2018
@@ -820,7 +820,7 @@ func post(url string, data interface{}) error {
return err
}

glog.V(2).Infof("Posting to %s", url)
glog.V(2).Infof("Posting to %s JSON payload %s", url, string(buf))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need to be careful with this since the payload includes private key as well in dynamic certificate mode

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed. I added that temporarily to help to debug.

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 6, 2018
@ElvinEfendi
Copy link
Member

This PR removes the need to reload NGINX when there is a change in the number of endpoints (in particular 0).

to be clear only when number of endpoints is 0.

@ElvinEfendi
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 6, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aledbf, ElvinEfendi

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot merged commit 08d5ffa into kubernetes:master Nov 6, 2018
@aledbf aledbf deleted the 503-restart branch November 6, 2018 14:52
@aledbf aledbf mentioned this pull request Nov 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Service with type ExternalName was not working properly due to unresolved address
3 participants