From c2935ca35cfe06ae3b20d6898ec5fd034e0be58b Mon Sep 17 00:00:00 2001 From: Manuel Alejandro de Brito Fontes Date: Sun, 1 Sep 2019 14:21:24 -0400 Subject: [PATCH] Refactor health checks and wait until NGINX process ends --- Makefile | 1 + build/build.sh | 9 ++++ cmd/nginx/main.go | 12 ++++- cmd/waitshutdown/main.go | 43 +++++++++++++++ internal/ingress/controller/checker.go | 52 +++++++++++-------- internal/ingress/controller/checker_test.go | 5 +- internal/ingress/controller/config/config.go | 2 +- internal/ingress/controller/controller.go | 3 ++ internal/ingress/controller/nginx.go | 2 +- internal/ingress/controller/nginx_test.go | 11 ++-- .../metric/collectors/nginx_status_test.go | 4 +- internal/nginx/main.go | 34 +++--------- rootfs/etc/nginx/template/nginx.tmpl | 2 +- test/e2e-image/overlay/deployment-e2e.yaml | 9 ++++ test/e2e/lua/dynamic_configuration.go | 2 +- 15 files changed, 124 insertions(+), 67 deletions(-) create mode 100644 cmd/waitshutdown/main.go diff --git a/Makefile b/Makefile index 9e0cc95646..8fedfc130a 100644 --- a/Makefile +++ b/Makefile @@ -108,6 +108,7 @@ container: clean-container .container-$(ARCH) mkdir -p $(TEMP_DIR)/rootfs cp bin/$(ARCH)/nginx-ingress-controller $(TEMP_DIR)/rootfs/nginx-ingress-controller cp bin/$(ARCH)/dbg $(TEMP_DIR)/rootfs/dbg + cp bin/$(ARCH)/wait-shutdown $(TEMP_DIR)/rootfs/wait-shutdown cp -RP ./* $(TEMP_DIR) $(SED_I) "s|BASEIMAGE|$(BASEIMAGE)|g" $(DOCKERFILE) diff --git a/build/build.sh b/build/build.sh index 3a8c9f7731..45cae4e88d 100755 --- a/build/build.sh +++ b/build/build.sh @@ -60,3 +60,12 @@ go build \ -X ${PKG}/version.COMMIT=${GIT_COMMIT} \ -X ${PKG}/version.REPO=${REPO_INFO}" \ -o "bin/${ARCH}/dbg" "${PKG}/cmd/dbg" + + +go build \ + "${GOBUILD_FLAGS}" \ + -ldflags "-s -w \ + -X ${PKG}/version.RELEASE=${TAG} \ + -X ${PKG}/version.COMMIT=${GIT_COMMIT} \ + -X ${PKG}/version.REPO=${REPO_INFO}" \ + -o "bin/${ARCH}/wait-shutdown" "${PKG}/cmd/waitshutdown" diff --git a/cmd/nginx/main.go b/cmd/nginx/main.go index ab5e4ee9d2..a787fdded9 100644 --- a/cmd/nginx/main.go +++ b/cmd/nginx/main.go @@ -131,7 +131,7 @@ func main() { mux := http.NewServeMux() if conf.EnableProfiling { - registerProfiler(mux) + go registerProfiler() } registerHealthz(ngx, mux) @@ -265,7 +265,9 @@ func registerMetrics(reg *prometheus.Registry, mux *http.ServeMux) { } -func registerProfiler(mux *http.ServeMux) { +func registerProfiler() { + mux := http.NewServeMux() + mux.HandleFunc("/debug/pprof/", pprof.Index) mux.HandleFunc("/debug/pprof/heap", pprof.Index) mux.HandleFunc("/debug/pprof/mutex", pprof.Index) @@ -276,6 +278,12 @@ func registerProfiler(mux *http.ServeMux) { mux.HandleFunc("/debug/pprof/profile", pprof.Profile) mux.HandleFunc("/debug/pprof/symbol", pprof.Symbol) mux.HandleFunc("/debug/pprof/trace", pprof.Trace) + + server := &http.Server{ + Addr: fmt.Sprintf(":10255"), + Handler: mux, + } + klog.Fatal(server.ListenAndServe()) } func startHTTPServer(port int, mux *http.ServeMux) { diff --git a/cmd/waitshutdown/main.go b/cmd/waitshutdown/main.go new file mode 100644 index 0000000000..69ec011c81 --- /dev/null +++ b/cmd/waitshutdown/main.go @@ -0,0 +1,43 @@ +/* +Copyright 2019 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package main + +import ( + "os" + "os/exec" + "time" + + "k8s.io/ingress-nginx/internal/nginx" + "k8s.io/klog" +) + +func main() { + err := exec.Command("bash", "-c", "pkill -SIGTERM -f nginx-ingress-controller").Run() + if err != nil { + klog.Errorf("unexpected error terminating ingress controller: %v", err) + os.Exit(1) + } + + // wait for the NGINX process to terminate + timer := time.NewTicker(time.Second * 1) + for range timer.C { + if !nginx.IsRunning() { + timer.Stop() + break + } + } +} diff --git a/internal/ingress/controller/checker.go b/internal/ingress/controller/checker.go index 8114452a5d..0b6a6c3736 100644 --- a/internal/ingress/controller/checker.go +++ b/internal/ingress/controller/checker.go @@ -25,7 +25,6 @@ import ( "github.com/ncabatoff/process-exporter/proc" "github.com/pkg/errors" - "k8s.io/klog" "k8s.io/ingress-nginx/internal/nginx" ) @@ -37,41 +36,48 @@ func (n NGINXController) Name() string { // Check returns if the nginx healthz endpoint is returning ok (status code 200) func (n *NGINXController) Check(_ *http.Request) error { - statusCode, _, err := nginx.NewGetStatusRequest(nginx.HealthPath) - if err != nil { - klog.Errorf("healthcheck error: %v", err) - return err + if n.isShuttingDown { + return fmt.Errorf("the ingress controller is shutting down") } - if statusCode != 200 { - klog.Errorf("healthcheck error: %v", statusCode) - return fmt.Errorf("ingress controller is not healthy") + // check the nginx master process is running + fs, err := proc.NewFS("/proc", false) + if err != nil { + return errors.Wrap(err, "reading /proc directory") } - statusCode, _, err = nginx.NewGetStatusRequest("/is-dynamic-lb-initialized") + f, err := ioutil.ReadFile(nginx.PID) if err != nil { - klog.Errorf("healthcheck error: %v", err) - return err + return errors.Wrapf(err, "reading %v", nginx.PID) } - if statusCode != 200 { - klog.Errorf("healthcheck error: %v", statusCode) - return fmt.Errorf("dynamic load balancer not started") + pid, err := strconv.Atoi(strings.TrimRight(string(f), "\r\n")) + if err != nil { + return errors.Wrapf(err, "reading NGINX PID from file %v", nginx.PID) } - // check the nginx master process is running - fs, err := proc.NewFS("/proc", false) + _, err = fs.NewProc(pid) if err != nil { - return errors.Wrap(err, "unexpected error reading /proc directory") + return errors.Wrapf(err, "checking for NGINX process with PID %v", pid) } - f, err := ioutil.ReadFile(nginx.PID) + + statusCode, _, err := nginx.NewGetStatusRequest(nginx.HealthPath) if err != nil { - return errors.Wrapf(err, "unexpected error reading %v", nginx.PID) + return errors.Wrapf(err, "checking if NGINX is running") } - pid, err := strconv.Atoi(strings.TrimRight(string(f), "\r\n")) + + if statusCode != 200 { + return fmt.Errorf("ingress controller is not healthy (%v)", statusCode) + } + + statusCode, _, err = nginx.NewGetStatusRequest("/is-dynamic-lb-initialized") if err != nil { - return errors.Wrapf(err, "unexpected error reading the nginx PID from %v", nginx.PID) + return errors.Wrapf(err, "checking if the dynamic load balancer started") } - _, err = fs.NewProc(pid) - return err + + if statusCode != 200 { + return fmt.Errorf("dynamic load balancer not started") + } + + return nil } diff --git a/internal/ingress/controller/checker_test.go b/internal/ingress/controller/checker_test.go index 6e7f2f29e8..674b0d9f16 100644 --- a/internal/ingress/controller/checker_test.go +++ b/internal/ingress/controller/checker_test.go @@ -35,12 +35,11 @@ import ( func TestNginxCheck(t *testing.T) { mux := http.NewServeMux() - listener, err := net.Listen("unix", nginx.StatusSocket) + listener, err := net.Listen("tcp", fmt.Sprintf(":%v", nginx.StatusPort)) if err != nil { - t.Fatalf("crating unix listener: %s", err) + t.Fatalf("crating tcp listener: %s", err) } defer listener.Close() - defer os.Remove(nginx.StatusSocket) server := &httptest.Server{ Listener: listener, diff --git a/internal/ingress/controller/config/config.go b/internal/ingress/controller/config/config.go index 7de5fa7a51..e005cc85f5 100644 --- a/internal/ingress/controller/config/config.go +++ b/internal/ingress/controller/config/config.go @@ -796,8 +796,8 @@ type TemplateConfig struct { EnableMetrics bool PID string - StatusSocket string StatusPath string + StatusPort int StreamSocket string } diff --git a/internal/ingress/controller/controller.go b/internal/ingress/controller/controller.go index b33bf15944..cea3f5a18e 100644 --- a/internal/ingress/controller/controller.go +++ b/internal/ingress/controller/controller.go @@ -37,6 +37,7 @@ import ( "k8s.io/ingress-nginx/internal/ingress/annotations/proxy" ngx_config "k8s.io/ingress-nginx/internal/ingress/controller/config" "k8s.io/ingress-nginx/internal/k8s" + "k8s.io/ingress-nginx/internal/nginx" "k8s.io/klog" ) @@ -268,6 +269,8 @@ func (n *NGINXController) getStreamServices(configmapName string, proto apiv1.Pr n.cfg.ListenPorts.SSLProxy, n.cfg.ListenPorts.Health, n.cfg.ListenPorts.Default, + 10255, // profiling port + nginx.StatusPort, } reserverdPorts := sets.NewInt(rp...) // svcRef format: <(str)namespace>/<(str)service>:<(intstr)port>[:<("PROXY")decode>:<("PROXY")encode>] diff --git a/internal/ingress/controller/nginx.go b/internal/ingress/controller/nginx.go index f640ba54b7..c5764f6452 100644 --- a/internal/ingress/controller/nginx.go +++ b/internal/ingress/controller/nginx.go @@ -605,8 +605,8 @@ func (n NGINXController) generateTemplate(cfg ngx_config.Configuration, ingressC HealthzURI: nginx.HealthPath, PID: nginx.PID, - StatusSocket: nginx.StatusSocket, StatusPath: nginx.StatusPath, + StatusPort: nginx.StatusPort, StreamSocket: nginx.StreamSocket, } diff --git a/internal/ingress/controller/nginx_test.go b/internal/ingress/controller/nginx_test.go index 9d56c98f0c..ac3a157432 100644 --- a/internal/ingress/controller/nginx_test.go +++ b/internal/ingress/controller/nginx_test.go @@ -17,6 +17,7 @@ limitations under the License. package controller import ( + "fmt" "io" "io/ioutil" "net" @@ -148,16 +149,15 @@ func TestIsDynamicConfigurationEnough(t *testing.T) { } func TestConfigureDynamically(t *testing.T) { - listener, err := net.Listen("unix", nginx.StatusSocket) + listener, err := net.Listen("tcp", fmt.Sprintf(":%v", nginx.StatusPort)) if err != nil { - t.Errorf("crating unix listener: %s", err) + t.Fatalf("crating unix listener: %s", err) } defer listener.Close() - defer os.Remove(nginx.StatusSocket) streamListener, err := net.Listen("unix", nginx.StreamSocket) if err != nil { - t.Errorf("crating unix listener: %s", err) + t.Fatalf("crating unix listener: %s", err) } defer streamListener.Close() defer os.Remove(nginx.StreamSocket) @@ -319,12 +319,11 @@ func TestConfigureDynamically(t *testing.T) { } func TestConfigureCertificates(t *testing.T) { - listener, err := net.Listen("unix", nginx.StatusSocket) + listener, err := net.Listen("tcp", fmt.Sprintf(":%v", nginx.StatusPort)) if err != nil { t.Fatalf("crating unix listener: %s", err) } defer listener.Close() - defer os.Remove(nginx.StatusSocket) streamListener, err := net.Listen("unix", nginx.StreamSocket) if err != nil { diff --git a/internal/ingress/metric/collectors/nginx_status_test.go b/internal/ingress/metric/collectors/nginx_status_test.go index f3c4ff9e93..ea9229728a 100644 --- a/internal/ingress/metric/collectors/nginx_status_test.go +++ b/internal/ingress/metric/collectors/nginx_status_test.go @@ -21,7 +21,6 @@ import ( "net" "net/http" "net/http/httptest" - "os" "testing" "time" @@ -97,7 +96,7 @@ func TestStatusCollector(t *testing.T) { for _, c := range cases { t.Run(c.name, func(t *testing.T) { - listener, err := net.Listen("unix", nginx.StatusSocket) + listener, err := net.Listen("tcp", fmt.Sprintf(":%v", nginx.StatusPort)) if err != nil { t.Fatalf("crating unix listener: %s", err) } @@ -145,7 +144,6 @@ func TestStatusCollector(t *testing.T) { cm.Stop() listener.Close() - os.Remove(nginx.StatusSocket) }) } } diff --git a/internal/nginx/main.go b/internal/nginx/main.go index 5a86ea7cf6..d43924bdf3 100644 --- a/internal/nginx/main.go +++ b/internal/nginx/main.go @@ -28,7 +28,6 @@ import ( "time" ps "github.com/mitchellh/go-ps" - "github.com/tv42/httpunix" "k8s.io/klog" ) @@ -38,8 +37,8 @@ var TemplatePath = "/etc/nginx/template/nginx.tmpl" // PID defines the location of the pid file used by NGINX var PID = "/tmp/nginx.pid" -// StatusSocket defines the location of the unix socket used by NGINX for the status server -var StatusSocket = "/tmp/nginx-status-server.sock" +// StatusPort port used by NGINX for the status server +var StatusPort = 10256 // HealthPath defines the path used to define the health check location in NGINX var HealthPath = "/healthz" @@ -56,17 +55,12 @@ var StreamSocket = "/tmp/ingress-stream.sock" var statusLocation = "nginx-status" -var httpClient *http.Client - -func init() { - httpClient = buildUnixSocketClient(HealthCheckTimeout) -} - // NewGetStatusRequest creates a new GET request to the internal NGINX status server func NewGetStatusRequest(path string) (int, []byte, error) { - url := fmt.Sprintf("%v://%v%v", httpunix.Scheme, statusLocation, path) + url := fmt.Sprintf("http://127.0.0.1:%v%v", StatusPort, path) - res, err := httpClient.Get(url) + client := http.Client{} + res, err := client.Get(url) if err != nil { return 0, nil, err } @@ -82,14 +76,15 @@ func NewGetStatusRequest(path string) (int, []byte, error) { // NewPostStatusRequest creates a new POST request to the internal NGINX status server func NewPostStatusRequest(path, contentType string, data interface{}) (int, []byte, error) { - url := fmt.Sprintf("%v://%v%v", httpunix.Scheme, statusLocation, path) + url := fmt.Sprintf("http://127.0.0.1:%v%v", StatusPort, path) buf, err := json.Marshal(data) if err != nil { return 0, nil, err } - res, err := httpClient.Post(url, contentType, bytes.NewReader(buf)) + client := http.Client{} + res, err := client.Post(url, contentType, bytes.NewReader(buf)) if err != nil { return 0, nil, err } @@ -142,19 +137,6 @@ func readFileToString(path string) (string, error) { return string(contents), nil } -func buildUnixSocketClient(timeout time.Duration) *http.Client { - u := &httpunix.Transport{ - DialTimeout: 1 * time.Second, - RequestTimeout: timeout, - ResponseHeaderTimeout: timeout, - } - u.RegisterLocation(statusLocation, StatusSocket) - - return &http.Client{ - Transport: u, - } -} - // Version return details about NGINX func Version() string { flag := "-v" diff --git a/rootfs/etc/nginx/template/nginx.tmpl b/rootfs/etc/nginx/template/nginx.tmpl index 9b449a7e43..079d7a16d9 100755 --- a/rootfs/etc/nginx/template/nginx.tmpl +++ b/rootfs/etc/nginx/template/nginx.tmpl @@ -558,7 +558,7 @@ http { # default server, used for NGINX healthcheck and access to nginx stats server { - listen unix:{{ .StatusSocket }}; + listen 127.0.0.1:{{ .StatusPort }}; set $proxy_upstream_name "internal"; keepalive_timeout 0; diff --git a/test/e2e-image/overlay/deployment-e2e.yaml b/test/e2e-image/overlay/deployment-e2e.yaml index f8df465a65..2219414f8d 100644 --- a/test/e2e-image/overlay/deployment-e2e.yaml +++ b/test/e2e-image/overlay/deployment-e2e.yaml @@ -22,5 +22,14 @@ spec: - name: nginx-ingress-controller livenessProbe: timeoutSeconds: 1 + initialDelaySeconds: 1 + periodSeconds: 2 readinessProbe: timeoutSeconds: 1 + initialDelaySeconds: 1 + periodSeconds: 2 + lifecycle: + preStop: + exec: + command: + - /wait-shutdown diff --git a/test/e2e/lua/dynamic_configuration.go b/test/e2e/lua/dynamic_configuration.go index 811ac036e6..a2941f7a62 100644 --- a/test/e2e/lua/dynamic_configuration.go +++ b/test/e2e/lua/dynamic_configuration.go @@ -199,7 +199,7 @@ var _ = framework.IngressNginxDescribe("Dynamic Configuration", func() { It("sets controllerPodsCount in Lua general configuration", func() { // https://github.com/curl/curl/issues/936 - curlCmd := fmt.Sprintf("curl --fail --silent --unix-socket %v http://localhost/configuration/general", nginx.StatusSocket) + curlCmd := fmt.Sprintf("curl --fail --silent http://localhost:%v/configuration/general", nginx.StatusPort) output, err := f.ExecIngressPod(curlCmd) Expect(err).ToNot(HaveOccurred())