✨ pkg/manager/*: Add MetricsServingDisabled option

[lilic]

MetricsServingDisabled option when set to true disables serving metrics
by manager.

Closes #336

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: LiliC
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: droot

If they are not already assigned, you can assign the PR to them by writing /assign @droot in a comment when ready.

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

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

[DirectXMan12]

Setting the metrics port to zero should disable serving as well, is that not working, or is it just not sufficiently intuitive (it's kind-a weird, so it's presumably easy to miss).

[lilic]

Setting the metrics port to zero should disable serving as well, is that not working, or is it just not sufficiently intuitive (it's kind-a weird, so it's presumably easy to miss).

Sorry if I didn't make myself clear enough, we would just like to start serving the metrics at an earlier point and not when .Start() is called. Meaning call the exposed function in this PR .ServeMetrics(...), but still pass the correct port. Hope that makes more sense. :)

[DirectXMan12]

Ah, ok. The use case makes sense, but I'd kind-of prefer to just extract the general bits of serveMetrics out into a helper such that it's callable separately. Then, you can just disable metrics serving from the manager, can call ServeMetrics at your leisure. Exposing the separate DisableMetricsServing and ServeMetrics seems confusing, especially since ServeMetrics doesn't actually use much of the internals of the manager.

[DirectXMan12]

does that work for you and/or make sense?

[lilic]

@DirectXMan12 If I understand correctly we would get rid of calling serveMetrics() in .Start() and instead the user will always have to call the new helper function ServeMetrics separately?

[DirectXMan12]

I mean:

1. Have a helper in pkg/metrics called ServeMetrics that takes in a net.Listener and does what .serveMetrics does (which is serve metrics from the global CR registry)

2. make Manager#serveMetrics just call metrics.ServeMetrics

3. You could just set MetricsPort: "0" in the manager config, and still get the same behavior by calling metrics.ServeMetrics manually.

This way, users continue to get the same behavior, but you can customize when you serve metrics without needing to re-implement the logic in serveMetrics (which basically just sets up an HTTP server and the Prometheus handler for the global CR registry):

controller-runtime/pkg/manager/internal.go

Lines 170 to 194 in 6649bdb

	func (cm *controllerManager) serveMetrics(stop <-chan struct{}) {
	handler := promhttp.HandlerFor(metrics.Registry, promhttp.HandlerOpts{
	ErrorHandling: promhttp.HTTPErrorOnError,
	})
	// TODO(JoelSpeed): Use existing Kubernetes machinery for serving metrics
	mux := http.NewServeMux()
	mux.Handle("/metrics", handler)
	server := http.Server{
	Handler: mux,
	}
	// Run the server
	go func() {
	if err := server.Serve(cm.metricsListener); err != nil && err != http.ErrServerClosed {
	cm.errChan <- err
	}
	}()
	// Shutdown the server when stop is closed
	select {
	case <-stop:
	if err := server.Shutdown(context.Background()); err != nil {
	cm.errChan <- err
	}
	}
	}

[lilic]

@DirectXMan12 Thanks for your suggestion and time to look at this! That implementation would work for our use case. I will close this PR and open a new one (probably not anytime soon as I am off to vacation for a week.) Thanks again!

[DirectXMan12]

SGTM. Enjoy your vacation :-)