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

Export kubernetes client metrics from dns-controller. #4612

Merged
merged 1 commit into from
Apr 1, 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
8 changes: 6 additions & 2 deletions Gopkg.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions dns-controller/cmd/dns-controller/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,11 @@ go_library(
"//protokube/pkg/gossip/dns/provider:go_default_library",
"//protokube/pkg/gossip/mesh:go_default_library",
"//vendor/github.com/golang/glog:go_default_library",
"//vendor/github.com/prometheus/client_golang/prometheus/promhttp:go_default_library",
"//vendor/github.com/spf13/pflag:go_default_library",
"//vendor/k8s.io/client-go/kubernetes:go_default_library",
"//vendor/k8s.io/client-go/rest:go_default_library",
"//vendor/k8s.io/kubernetes/pkg/client/metrics/prometheus:go_default_library",
],
)

Expand Down
12 changes: 11 additions & 1 deletion dns-controller/cmd/dns-controller/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,14 +21,18 @@ import (
"flag"
"fmt"
"io"
"log"
"net/http"
"os"
"strings"

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

"k8s.io/client-go/kubernetes"
"k8s.io/client-go/rest"
_ "k8s.io/kubernetes/pkg/client/metrics/prometheus" // for client metric registration

"k8s.io/kops/dns-controller/pkg/dns"
"k8s.io/kops/dns-controller/pkg/watchers"
Expand All @@ -50,7 +54,7 @@ var (

func main() {
fmt.Printf("dns-controller version %s\n", BuildVersion)
var dnsServer, dnsProviderID, gossipListen, gossipSecret, watchNamespace string
var dnsServer, dnsProviderID, gossipListen, gossipSecret, watchNamespace, metricsListen string
var gossipSeeds, zones []string
var watchIngress bool

Expand All @@ -66,6 +70,7 @@ func main() {
flags.StringVar(&gossipSecret, "gossip-secret", gossipSecret, "Secret to use to secure gossip")
flags.StringVar(&watchNamespace, "watch-namespace", "", "Limits the functionality for pods, services and ingress to specific namespace, by default all")
flag.IntVar(&route53.MaxBatchSize, "route53-batch-size", route53.MaxBatchSize, "Maximum number of operations performed per changeset batch")
flag.StringVar(&metricsListen, "metrics-listen-addr", ":3999", "The address on which to listen for Prometheus metrics.")

// Trick to avoid 'logging before flag.Parse' warning
flag.CommandLine.Parse([]string{})
Expand All @@ -74,6 +79,11 @@ func main() {
flags.AddGoFlagSet(flag.CommandLine)
flags.Parse(os.Args)

go func() {
http.Handle("/metrics", promhttp.Handler())
Copy link
Contributor

Choose a reason for hiding this comment

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

My only concern is that we have no way to shut this new feature off. That capability adds more complexity to this PR, but it we probably need the capability to control that functionality.

@robinpercy thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

@chrislovecnm It's not a particularly invasive feature. But after looking at how we do it elsewhere (eg calico), I think we should expose it and allow it to be disabled via the API, for consistency.

@tomwilkie We could add two variables to the ExternalDNS API, one to toggle the feature, and the other to override the port. It would be similar to how the WatchIngress flag was added in #2504

Let us know if you don't have time to add that in, and maybe we can merge this as-is and do the API stuff in another PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah no problem, will do it tomorrow.

and the other to override the port

Port is configurable via -metrics-listen-addr.

Copy link
Contributor

Choose a reason for hiding this comment

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

Great thanks.

Since kops generates the manifest for dns-controller at cluster-build time, we need to add a field for -metrics-listen-addr to the internal model and API in order for users to be able to specify it. Unfortunately, there are a few hoops to jump through :/. Let me know if you have any questions. #2504 is the best example I could find.

log.Fatal(http.ListenAndServe(metricsListen, nil))
Copy link
Member

Choose a reason for hiding this comment

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

FYI we typically use glog

}()

zoneRules, err := dns.ParseZoneRules(zones)
if err != nil {
glog.Errorf("unexpected zone flags: %q", err)
Expand Down

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading