-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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" | ||
|
@@ -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 | ||
|
||
|
@@ -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{}) | ||
|
@@ -74,6 +79,11 @@ func main() { | |
flags.AddGoFlagSet(flag.CommandLine) | ||
flags.Parse(os.Args) | ||
|
||
go func() { | ||
http.Handle("/metrics", promhttp.Handler()) | ||
log.Fatal(http.ListenAndServe(metricsListen, nil)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. FYI we typically use |
||
}() | ||
|
||
zoneRules, err := dns.ParseZoneRules(zones) | ||
if err != nil { | ||
glog.Errorf("unexpected zone flags: %q", err) | ||
|
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
Port is configurable via
-metrics-listen-addr
.There was a problem hiding this comment.
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.