Replace Status port using a socket #3684

aledbf · 2019-01-21T14:32:18Z

What this PR does / why we need it:

Which issue this PR fixes: fixes #3457

The new status server is configured like this:

	# default server, used for NGINX healthcheck and access to nginx stats
	server {
		listen unix:/tmp/nginx-status-server.sock;
		set $proxy_upstream_name "internal";
		
		keepalive_timeout 0;
		gzip off;
		
		access_log off;
		
		location /healthz {
			return 200;
		}
		
		location /is-dynamic-lb-initialized {
			content_by_lua_block {
				local configuration = require("configuration")
				local backend_data = configuration.get_backends_data()
				if not backend_data then
				ngx.exit(ngx.HTTP_INTERNAL_SERVER_ERROR)
				return
				end
				
				ngx.say("OK")
				ngx.exit(ngx.HTTP_OK)
			}
		}
		
		location /nginx_status {
			stub_status on;
		}
		
		location /configuration {
			# this should be equals to configuration_data dict
			client_max_body_size                    10m;
			client_body_buffer_size                 10m;
			proxy_buffering                         off;
			
			content_by_lua_block {
				configuration.call()
			}
		}
		
		location / {
			content_by_lua_block {
				ngx.exit(ngx.HTTP_NOT_FOUND)
			}
		}
	}
}

ElvinEfendi · 2019-01-30T18:44:49Z

make sense 👍

But I don't see how it's going to fix #3457. That issue seems to be result of Nginx not having enough resources - so this PR will help a bit since it reduces overhead but when the server is under more load the same thing will happen.

aledbf · 2019-01-30T19:10:06Z

But I don't see how it's going to fix #3457.

Please check comments after #3457 (comment)

For some reason, the health checks start failing without valid reason. If you enter in the container and run netstat -lnp nginx is listening in all the interfaces but the go client cannot reach 127.0.0.1:18080.
Replacing the port for a unix socket removes this issue, removes a port from the ingress controller and also encapsulates the server.

ElvinEfendi · 2019-01-30T19:34:09Z

@aledbf I support switching to using UNIX socket. And this might indeed fix the issue but it also might not. It'd be nice to understand

nginx is listening in all the interfaces but the go client cannot reach 127.0.0.1:18080

--

Again, this PR is a right direction!

aledbf · 2019-01-30T19:37:37Z

It'd be nice to understand
nginx is listening in all the interfaces but the go client cannot reach 127.0.0.1:18080

Agree. The issue here is that I cannot reproduce this issue and why I am waiting for feedback from #3457.

alexkursell · 2019-02-06T16:36:09Z

cmd/nginx/flags.go

@@ -161,6 +160,7 @@ Feature backed by OpenResty Lua libraries. Requires that OCSP stapling is not en
 	)

 	flags.MarkDeprecated("sort-backends", "Feature removed because of the lua load balancer that removed the need of reloads for change in endpoints")
+	flags.MarkDeprecated("status-port", `Status port is a unix socket.`)


This also applies to sort-backends, but why are we marking these flags deprecated when we are removing them? Aren't they either deprecated or removed, but not both?

To avoid breaking existing deployments using the flags. We can remove the flags in 0.24

When 0.22 was released, using --sort-backends generated an error (unknown flag: --sort-backends) because the flag was removed. MarkDeprecated doesn't do anything if the flag is removed. See also @ElvinEfendi comment here: #3655 (comment)

ElvinEfendi · 2019-02-06T17:44:52Z

internal/ingress/controller/nginx.go

 		ControllerPodsCount: pcfg.ControllerPodsCount,
 	})
 	if err != nil {
 		return err
 	}

+	if statusCode != http.StatusCreated {
+		return fmt.Errorf("unexpected error code: %d", statusCode)
+	}


I wonder why did we not do this before. Also where are you not doing the same above for backends?

check added to backends

ElvinEfendi · 2019-02-06T18:51:50Z

internal/nginx/main.go

+
+// NewGetStatusRequest creates a new GET request to the internal NGINX status server
+func NewGetStatusRequest(path string) (int, []byte, error) {
+	client := buildUnixSocketClient()


why do you build the client for every request? why not build it once and use the same client always?

ElvinEfendi · 2019-02-06T18:58:56Z

internal/nginx/main.go

+// StreamSocket defines the location of the unix socket used by NGINX for the NGINX stream configuration socket
+var StreamSocket = "/tmp/ingress-stream.sock"
+
+var nginxUnixLocation = "nginx-status"


nitpick: statusLocation probably makes more sense since this is an alias to StatusSocket

ElvinEfendi · 2019-02-06T19:15:45Z

rootfs/etc/nginx/template/nginx.tmpl

-        listen {{ $all.ListenPorts.Status }} default_server {{ if $all.Cfg.ReusePort }}reuseport{{ end }} backlog={{ $all.BacklogSize }};
-        {{ if $IsIPV6Enabled }}listen [::]:{{ $all.ListenPorts.Status }} default_server {{ if $all.Cfg.ReusePort }}reuseport{{ end }} backlog={{ $all.BacklogSize }};{{ end }}
-        set $proxy_upstream_name "-";
+        listen unix:/tmp/nginx-status-server.sock;


Can you avoid hardcoding this here and use https://github.com/kubernetes/ingress-nginx/pull/3684/files#diff-6c2cb34e0bc02a92a477db1ea8101b69R34 instead?

ElvinEfendi · 2019-02-06T19:16:04Z

rootfs/etc/nginx/template/nginx.tmpl

        }
-
-        {{ template "CUSTOM_ERRORS" (buildCustomErrorDeps $all.ProxySetHeaders $cfg.CustomHTTPErrors $all.EnableMetrics) }}


Why are you deleting this now? Why did we even have this here, does not make sense

Why are you deleting this now?

Because before this change we publish the server to port 18080.

Why did we even have this here, does not make sense

Keep in mind we had more content before 0.16 (like vts pages)

ElvinEfendi · 2019-02-06T19:39:39Z

/lgtm
/approve

ElvinEfendi · 2019-02-06T21:27:18Z

Please don't modify history during review process, makes it hard to review new changes :)

ElvinEfendi · 2019-02-06T21:29:28Z

/lgtm

I guess the change was to avoid PID hardcoding.

k8s-ci-robot · 2019-02-06T21:29:44Z

[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:

~~OWNERS~~ [ElvinEfendi,aledbf]

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

aledbf · 2019-02-06T22:49:06Z

Please don't modify history during review process, makes it hard to review new changes :)

Sorry about that but I didn't want a new commit just to fix a template in the issue.

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jan 21, 2019

k8s-ci-robot requested review from antoineco and ElvinEfendi January 21, 2019 14:32

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 21, 2019

aledbf mentioned this pull request Jan 21, 2019

Regular crash of ngnix ingress controller pods after upgrade to 0.20.0 image #3457

Closed

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jan 22, 2019

aledbf force-pushed the health branch 2 times, most recently from 107afcc to d637a05 Compare January 25, 2019 14:29

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 25, 2019

aledbf force-pushed the health branch from d637a05 to 946ed73 Compare January 25, 2019 15:03

aledbf changed the title ~~WIP: Add log for health checks errors~~ WIP: Replace Status port using a socket Jan 25, 2019

aledbf force-pushed the health branch 3 times, most recently from ff6027f to a95213d Compare January 28, 2019 19:50

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 28, 2019

aledbf mentioned this pull request Jan 30, 2019

Add debug binary to the docker image #3686

Merged

aledbf force-pushed the health branch 3 times, most recently from 9fe173e to 897439d Compare February 6, 2019 16:27

aledbf changed the title ~~WIP: Replace Status port using a socket~~ Replace Status port using a socket Feb 6, 2019

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 Feb 6, 2019

aledbf assigned ElvinEfendi Feb 6, 2019

aledbf force-pushed the health branch from 897439d to e0b8334 Compare February 6, 2019 16:30

alexkursell reviewed Feb 6, 2019

View reviewed changes

ElvinEfendi reviewed Feb 6, 2019

View reviewed changes

aledbf force-pushed the health branch 2 times, most recently from 18bb412 to 15f56c6 Compare February 6, 2019 17:56

ElvinEfendi reviewed Feb 6, 2019

View reviewed changes

aledbf force-pushed the health branch from 15f56c6 to b329652 Compare February 6, 2019 19:00

ElvinEfendi reviewed Feb 6, 2019

View reviewed changes

aledbf force-pushed the health branch from b329652 to eef1d54 Compare February 6, 2019 19:29

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 6, 2019

aledbf added 2 commits February 6, 2019 18:00

Replace Status port using a socket

34b0580

Add httpunix dependency

c228619

aledbf force-pushed the health branch from eef1d54 to c228619 Compare February 6, 2019 21:00

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 6, 2019

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 6, 2019

k8s-ci-robot merged commit 17e788b into kubernetes:master Feb 6, 2019

aledbf deleted the health branch February 6, 2019 23:07

ac-hibbert mentioned this pull request Apr 25, 2019

ingress-nginx crashes on reload of configuration #4041

Closed

ac-hibbert mentioned this pull request Jul 8, 2019

ingress-nginx crashes on reload of configuration #4284

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace Status port using a socket #3684

Replace Status port using a socket #3684

aledbf commented Jan 21, 2019 •

edited

Loading

ElvinEfendi commented Jan 30, 2019

aledbf commented Jan 30, 2019

ElvinEfendi commented Jan 30, 2019

aledbf commented Jan 30, 2019

alexkursell Feb 6, 2019

aledbf Feb 6, 2019

alexkursell Feb 6, 2019 •

edited

Loading

aledbf Feb 6, 2019

ElvinEfendi Feb 6, 2019

aledbf Feb 6, 2019

ElvinEfendi Feb 6, 2019

aledbf Feb 6, 2019

ElvinEfendi Feb 6, 2019

aledbf Feb 6, 2019

ElvinEfendi Feb 6, 2019

aledbf Feb 6, 2019

ElvinEfendi Feb 6, 2019 •

edited

Loading

aledbf Feb 6, 2019

ElvinEfendi commented Feb 6, 2019

ElvinEfendi commented Feb 6, 2019

ElvinEfendi commented Feb 6, 2019

k8s-ci-robot commented Feb 6, 2019

aledbf commented Feb 6, 2019

		}

		{{ template "CUSTOM_ERRORS" (buildCustomErrorDeps $all.ProxySetHeaders $cfg.CustomHTTPErrors $all.EnableMetrics) }}

Replace Status port using a socket #3684

Replace Status port using a socket #3684

Conversation

aledbf commented Jan 21, 2019 • edited Loading

ElvinEfendi commented Jan 30, 2019

aledbf commented Jan 30, 2019

ElvinEfendi commented Jan 30, 2019

aledbf commented Jan 30, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexkursell Feb 6, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ElvinEfendi Feb 6, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ElvinEfendi commented Feb 6, 2019

ElvinEfendi commented Feb 6, 2019

ElvinEfendi commented Feb 6, 2019

k8s-ci-robot commented Feb 6, 2019

aledbf commented Feb 6, 2019

aledbf commented Jan 21, 2019 •

edited

Loading

alexkursell Feb 6, 2019 •

edited

Loading

ElvinEfendi Feb 6, 2019 •

edited

Loading