Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Introducing http-endpoint #95

Closed
wants to merge 1 commit into from
Closed

Introducing http-endpoint #95

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
45 changes: 39 additions & 6 deletions cmd/livenessprobe/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,10 @@ package main
import (
"context"
"flag"
"fmt"
"net"
"net/http"
"os"
"sync"
"time"

Expand All @@ -32,12 +34,18 @@ import (
"github.com/kubernetes-csi/csi-lib-utils/rpc"
)

const (
defaultHealthzPort = "9808"
defaultHttpEndpoint = ":" + defaultHealthzPort
)

// Command line flags
var (
probeTimeout = flag.Duration("probe-timeout", time.Second, "Probe timeout in seconds")
probeTimeout = flag.Duration("probe-timeout", time.Second, "Probe timeout in seconds.")
csiAddress = flag.String("csi-address", "/run/csi/socket", "Address of the CSI driver socket.")
healthzPort = flag.String("health-port", "9808", "TCP ports for listening healthz requests")
metricsAddress = flag.String("metrics-address", "", "The TCP network address where the prometheus metrics endpoint will listen (example: `:8080`). The default is empty string, which means metrics endpoint is disabled.")
healthzPort = flag.String("health-port", defaultHealthzPort, fmt.Sprintf("(deprecated) TCP ports for listening healthz requests. The default is `%s`", defaultHealthzPort))
metricsAddress = flag.String("metrics-address", "", "(deprecated) The TCP network address where the prometheus metrics endpoint will listen (example: `:8080`). The default is empty string, which means the localhost + the port from `--health-port` is used. If set, the address must not resolve to either the value of `--http-endpoint` or localhost + the port from `--health-port`.")
httpEndpoint = flag.String("http-endpoint", defaultHttpEndpoint, fmt.Sprintf("The TCP network address where the HTTP server for diagnostics, including CSI driver health check and metrics. The default is `%s`.", defaultHttpEndpoint))
metricsPath = flag.String("metrics-path", "/metrics", "The HTTP path where prometheus metrics will be exposed. Default is `/metrics`.")
)

Expand Down Expand Up @@ -115,6 +123,18 @@ func main() {
klog.InitFlags(nil)
flag.Set("logtostderr", "true")
flag.Parse()

if *healthzPort != defaultHealthzPort && *httpEndpoint != defaultHttpEndpoint {
klog.Error("only one of `--health-port` and `--http-endpoint` can be explicitly set.")
os.Exit(1)
}
var addr string
if *healthzPort != defaultHealthzPort {
addr = net.JoinHostPort("0.0.0.0", *healthzPort)
} else {
addr = *httpEndpoint
}

metricsManager := metrics.NewCSIMetricsManager("" /* driverName */)
csiConn, err := acquireConnection(context.Background(), metricsManager)
if err != nil {
Expand All @@ -137,12 +157,25 @@ func main() {
}

mux := http.NewServeMux()
addr := net.JoinHostPort("0.0.0.0", *healthzPort)
metricsManager.RegisterToServer(mux, *metricsPath)
metricsManager.SetDriverName(csiDriverName)

if *metricsAddress != "" && *metricsAddress != addr {
// Remove once --metrics-address is removed
metricsMux := http.NewServeMux()
metricsManager.RegisterToServer(metricsMux, *metricsPath)
go func() {
klog.Infof("Separate metrics ServeMux listening at %q", *metricsAddress)
err := http.ListenAndServe(*metricsAddress, metricsMux)
Copy link
Contributor

Choose a reason for hiding this comment

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

So this means we start two servers. One for the metrics and the other for healthz? Is it possible to do it in one server?

I am trying to understand why metricsManager.RegisterToServer(mux, *metricsPath) will not work.

Copy link
Contributor

Choose a reason for hiding this comment

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

And could you remind me why do we need the http-endpoint?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And could you remind me why do we need the http-endpoint?

This is to be consistent with all other sidecars - every sidecar will have http-endpoint as the flag to specify where the its HTTP server should run (e.g. kubernetes-csi/external-provisioner#537). This also eliminates the limitation that health check and metrics must run on different ports.

So this means we start two servers. One for the metrics and the other for healthz? Is it possible to do it in one server?

This keeps the original behavior of being able to specify different ports for health check and metrics, to maintain backward compatibility. If metrics-address isn't set explicitly, we do exactly as you described.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, thanks! I think we might want to remove this on the next major release. But this looks good to me.

Copy link
Contributor

@Jiawei0227 Jiawei0227 Dec 11, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

So to make it as simple to use as possible, can we do this ?

Combine the metrics port and healthz port together. I don't see the benefit of separate them apart. So the change can just be: if http-endpoint is set, we just use that endpoint to expose metrics and healthz and ignore whatever the healthz and metrics_address setting. Otherwise keep the current behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The tricky thing is currently http-endpoint has a default value (to inherit the behavior of health-port), and unfortunately the golang flag package doesn't have the ability to distinguish between something that's set vs unset.

if err != nil {
klog.Fatalf("Failed to start prometheus metrics endpoint on specified address (%q) and path (%q): %s", *metricsAddress, *metricsPath, err)
}
}()
} else {
metricsManager.RegisterToServer(mux, *metricsPath)
}

mux.HandleFunc("/healthz", hp.checkProbe)
klog.Infof("Serving requests to /healthz on: %s", addr)
klog.Infof("ServeMux listening at %q", addr)
err = http.ListenAndServe(addr, mux)
if err != nil {
klog.Fatalf("failed to start http server with error: %v", err)
Expand Down