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

Cleanup prometheus metrics after a reload #2726

Merged
merged 1 commit into from
Jul 12, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@ FOCUS ?= .*
# number of parallel test
E2E_NODES ?= 3

NODE_IP ?= $(shell minikube ip)

ifeq ($(GOHOSTOS),darwin)
SED_I=sed -i ''
endif
Expand Down Expand Up @@ -165,6 +167,7 @@ static-check:
.PHONY: test
test:
@$(DEF_VARS) \
NODE_IP=$(NODE_IP) \
DOCKER_OPTS="--net=host" \
build/go-in-docker.sh build/test.sh

Expand All @@ -180,6 +183,7 @@ e2e-test:
FOCUS=$(FOCUS) \
E2E_NODES=$(E2E_NODES) \
DOCKER_OPTS="--net=host" \
NODE_IP=$(NODE_IP) \
build/go-in-docker.sh build/e2e-tests.sh

.PHONY: cover
Expand Down
11 changes: 4 additions & 7 deletions build/e2e-tests.sh
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,10 @@ if [ -z "${E2E_NODES}" ]; then
echo "E2E_NODES must be set"
exit 1
fi
if [ -z "${NODE_IP}" ]; then
echo "NODE_IP must be set"
exit 1
fi

SCRIPT_ROOT=$(dirname ${BASH_SOURCE})/..

Expand All @@ -46,13 +50,6 @@ if ! [ -x "$(command -v kubectl)" ]; then
chmod +x ${TEST_BINARIES}/kubectl
fi

if ! [ -x "$(command -v minikube)" ]; then
echo "downloading minikube..."
curl -sSLo ${TEST_BINARIES}/minikube \
https://storage.googleapis.com/minikube/releases/latest/minikube-linux-amd64
chmod +x ${TEST_BINARIES}/minikube
fi

ginkgo build ./test/e2e

ginkgo \
Expand Down
1 change: 1 addition & 0 deletions build/go-in-docker.sh
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ GOARCH=${GOARCH}
PWD=${PWD}
BUSTED_ARGS=${BUSTED_ARGS:-""}
REPO_INFO=${REPO_INFO:-local}
NODE_IP=${NODE_IP:-127.0.0.1}
EOF

docker run \
Expand Down
35 changes: 18 additions & 17 deletions cmd/nginx/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import (
"time"

"github.com/golang/glog"
"github.com/prometheus/client_golang/prometheus"
"github.com/prometheus/client_golang/prometheus/promhttp"

metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
Expand All @@ -39,9 +40,8 @@ import (
"k8s.io/client-go/tools/clientcmd"

"k8s.io/ingress-nginx/internal/file"
"k8s.io/ingress-nginx/internal/ingress/annotations/class"
"k8s.io/ingress-nginx/internal/ingress/controller"
"k8s.io/ingress-nginx/internal/ingress/metric/collector"
"k8s.io/ingress-nginx/internal/ingress/metric"
"k8s.io/ingress-nginx/internal/k8s"
"k8s.io/ingress-nginx/internal/net/ssl"
"k8s.io/ingress-nginx/version"
Expand Down Expand Up @@ -118,25 +118,20 @@ func main() {

conf.Client = kubeClient

ngx := controller.NewNGINXController(conf, fs)
reg := prometheus.NewRegistry()
Copy link
Contributor

Choose a reason for hiding this comment

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

The default registry enables the GoCollector and ProcessCollector. I think we should keep these metrics by adding:
``
reg.MustRegister(prometheus.NewGoCollector())
reg.MustRegister(prometheus.NewProcessCollector(os.Getpid(), ""))

Copy link
Member Author

Choose a reason for hiding this comment

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

done

mc, err := metric.NewCollector(conf.ListenPorts.Status, reg)
if err != nil {
glog.Fatalf("Error creating prometheus collectos: %v", err)
}
mc.Start()

ngx := controller.NewNGINXController(conf, mc, fs)
go handleSigterm(ngx, func(code int) {
os.Exit(code)
})

mux := http.NewServeMux()
go registerHandlers(conf.EnableProfiling, conf.ListenPorts.Health, ngx, mux)

err = collector.InitNGINXStatusCollector(conf.Namespace, class.IngressClass, conf.ListenPorts.Status)

if err != nil {
glog.Fatalf("Error creating metric collector: %v", err)
}

err = collector.NewInstance(conf.Namespace, class.IngressClass)
if err != nil {
glog.Fatalf("Error creating unix socket server: %v", err)
}
go registerHandlers(conf.EnableProfiling, conf.ListenPorts.Health, ngx, mux, reg)

ngx.Start()
}
Expand Down Expand Up @@ -240,14 +235,20 @@ func handleFatalInitError(err error) {
err)
}

func registerHandlers(enableProfiling bool, port int, ic *controller.NGINXController, mux *http.ServeMux) {
func registerHandlers(
enableProfiling bool,
port int,
ic *controller.NGINXController,
mux *http.ServeMux,
reg *prometheus.Registry) {

// expose health check endpoint (/healthz)
healthz.InstallHandler(mux,
healthz.PingHealthz,
ic,
)

mux.Handle("/metrics", promhttp.Handler())
mux.Handle("/metrics", promhttp.HandlerFor(reg, promhttp.HandlerOpts{}))
Copy link
Contributor

@discordianfish discordianfish Jul 11, 2018

Choose a reason for hiding this comment

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

Handler() will instrument the handler too. If you want to keep this, use this instead:

       mux.Handle(
               "/metrics",
               promhttp.InstrumentMetricHandler(
                       reg,
                       promhttp.HandlerFor(reg, promhttp.HandlerOpts{}),
               ),
       )

Copy link
Member Author

Choose a reason for hiding this comment

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

@discordianfish InstrumentMetricHandler is not available in 0.8. If safe to use prometheus/client_golang master?

Copy link
Contributor

Choose a reason for hiding this comment

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

I just realized this too :)
I think you can just use InstrumentHandler() from prometheus (instead of promhttp). Using master should be fine too though with the dependencies vendored anyway (it's what I did).


mux.HandleFunc("/build", func(w http.ResponseWriter, r *http.Request) {
w.WriteHeader(http.StatusOK)
Expand Down
2 changes: 1 addition & 1 deletion cmd/nginx/main_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ func TestHandleSigterm(t *testing.T) {
t.Fatalf("Unexpected error: %v", err)
}

ngx := controller.NewNGINXController(conf, fs)
ngx := controller.NewNGINXController(conf, nil, fs)

go handleSigterm(ngx, func(code int) {
if code != 1 {
Expand Down
100 changes: 82 additions & 18 deletions internal/ingress/controller/controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import (
"time"

"github.com/golang/glog"
"github.com/mitchellh/hashstructure"

apiv1 "k8s.io/api/core/v1"
extensions "k8s.io/api/extensions/v1beta1"
Expand Down Expand Up @@ -148,38 +149,43 @@ func (n *NGINXController) syncIngress(interface{}) error {
}
}

pcfg := ingress.Configuration{
Backends: upstreams,
Servers: servers,
TCPEndpoints: n.getStreamServices(n.cfg.TCPConfigMapName, apiv1.ProtocolTCP),
UDPEndpoints: n.getStreamServices(n.cfg.UDPConfigMapName, apiv1.ProtocolUDP),
PassthroughBackends: passUpstreams,

ConfigurationChecksum: n.store.GetBackendConfiguration().Checksum,
pcfg := &ingress.Configuration{
Backends: upstreams,
Servers: servers,
TCPEndpoints: n.getStreamServices(n.cfg.TCPConfigMapName, apiv1.ProtocolTCP),
UDPEndpoints: n.getStreamServices(n.cfg.UDPConfigMapName, apiv1.ProtocolUDP),
PassthroughBackends: passUpstreams,
BackendConfigChecksum: n.store.GetBackendConfiguration().Checksum,
}

if n.runningConfig.Equal(&pcfg) {
if n.runningConfig.Equal(pcfg) {
glog.V(3).Infof("No configuration change detected, skipping backend reload.")
return nil
}

if n.cfg.DynamicConfigurationEnabled && n.IsDynamicConfigurationEnough(&pcfg) {
if n.cfg.DynamicConfigurationEnabled && n.IsDynamicConfigurationEnough(pcfg) {
glog.Infof("Changes handled by the dynamic configuration, skipping backend reload.")
} else {
glog.Infof("Configuration changes detected, backend reload required.")

err := n.OnUpdate(pcfg)
hash, _ := hashstructure.Hash(pcfg, &hashstructure.HashOptions{
TagName: "json",
})

pcfg.ConfigurationChecksum = fmt.Sprintf("%v", hash)

err := n.OnUpdate(*pcfg)
if err != nil {
IncReloadErrorCount()
ConfigSuccess(false)
n.metricCollector.IncReloadErrorCount()
n.metricCollector.ConfigSuccess(hash, false)
glog.Errorf("Unexpected failure reloading the backend:\n%v", err)
return err
}

glog.Infof("Backend successfully reloaded.")
ConfigSuccess(true)
IncReloadCount()
setSSLExpireTime(servers)
n.metricCollector.ConfigSuccess(hash, true)
n.metricCollector.IncReloadCount()
n.metricCollector.SetSSLExpireTime(servers)
}

if n.cfg.DynamicConfigurationEnabled {
Expand All @@ -191,7 +197,7 @@ func (n *NGINXController) syncIngress(interface{}) error {
// it takes time for NGINX to start listening on the configured ports
time.Sleep(1 * time.Second)
}
err := configureDynamically(&pcfg, n.cfg.ListenPorts.Status)
err := configureDynamically(pcfg, n.cfg.ListenPorts.Status)
if err == nil {
glog.Infof("Dynamic reconfiguration succeeded.")
} else {
Expand All @@ -200,7 +206,11 @@ func (n *NGINXController) syncIngress(interface{}) error {
}(isFirstSync)
}

n.runningConfig = &pcfg
ri := getRemovedIngresses(n.runningConfig, pcfg)
re := getRemovedHosts(n.runningConfig, pcfg)
n.metricCollector.RemoveMetrics(ri, re)

n.runningConfig = pcfg

return nil
}
Expand Down Expand Up @@ -1112,3 +1122,57 @@ func extractTLSSecretName(host string, ing *extensions.Ingress,

return ""
}

// getRemovedHosts returns a list of the hostsnames
// that are not associated anymore to the NGINX configuration.
func getRemovedHosts(rucfg, newcfg *ingress.Configuration) []string {
old := sets.NewString()
new := sets.NewString()

for _, s := range rucfg.Servers {
if !old.Has(s.Hostname) {
old.Insert(s.Hostname)
}
}

for _, s := range newcfg.Servers {
if !new.Has(s.Hostname) {
new.Insert(s.Hostname)
}
}

return old.Difference(new).List()
}

func getRemovedIngresses(rucfg, newcfg *ingress.Configuration) []string {
oldIngresses := sets.NewString()
newIngresses := sets.NewString()

for _, server := range rucfg.Servers {
for _, location := range server.Locations {
if location.Ingress == nil {
continue
}

ingKey := k8s.MetaNamespaceKey(location.Ingress)
if !oldIngresses.Has(ingKey) {
oldIngresses.Insert(ingKey)
}
}
}

for _, server := range newcfg.Servers {
for _, location := range server.Locations {
if location.Ingress == nil {
continue
}

ingKey := k8s.MetaNamespaceKey(location.Ingress)
if !newIngresses.Has(ingKey) {
newIngresses.Insert(ingKey)
}
}
}

return oldIngresses.Difference(newIngresses).List()
}
Loading