Serve diagnostic endpoints independently of the k8s exporter - #1347
Serve diagnostic endpoints independently of the k8s exporter#1347css521 wants to merge 1 commit into
Conversation
/healthz, /conditions and /debug/pprof were registered inside k8sexporter.NewExporterOrDie, which returns early when --enable-k8s-exporter=false. Running NPD without a cluster therefore lost all three even with --port set, though README documents --port as what controls this server. Move the server into pkg/httpserver, started from npdMain next to the Prometheus exporter it mirrors and controlled by --port alone. The Kubernetes exporter still owns the node conditions and supplies them through the optional ConditionsGetter interface; without it /conditions reports no conditions, which is the same response as an enabled exporter that has not recorded any condition yet.
|
Welcome @css521! |
|
Hi @css521. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: css521 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
/kind bug
What this PR does / why we need it:
/healthz,/conditionsand/debug/pprofwere registered insidek8sexporter.NewExporterOrDie, which returns early when--enable-k8s-exporter=false. Running NPD outside a cluster — where that flag is what avoids a panic at startup — therefore lost all three endpoints even with--portset, while--prometheus-portkept working because the Prometheus exporter starts its own server.README already documents
--portas "The port to bind the node problem detector server. Use 0 to disable.", so this makes the implementation match the flag it is documented under.This PR moves the server to
pkg/httpserver, started fromnpdMainnext to the Prometheus exporter it mirrors, controlled by--portalone.Which issue(s) this PR fixes:
Fixes #1332
Special notes for your reviewer:
GetConditions()and satisfies the optionalhttpserver.ConditionsGetterinterface, whichnpdMainpasses to the server when that exporter is enabled. That keeps the condition manager private to the exporter and avoids handing the HTTP server a Kubernetes dependency it otherwise does not need./conditionswith the exporter disabled. It serves no conditions rather than 404, so the endpoint's presence does not depend on the flag. The body is byte-identical to what an enabled exporter returns before it has recorded any condition (null, fromutil.ReturnHTTPJsonon a nil slice), so no client can tell the two apart by parsing — there is a test pinning exactly that. Happy to switch it to 404 or an explicit empty array if you would rather the two be distinguishable.--enable-k8s-exporter=falsenecessary in the first place is untouched; this only stops that flag from taking the diagnostic endpoints down with it.Verification — ran the issue's reproduction against both binaries,
--enable-k8s-exporter=false --port=20256 --prometheus-port=20257 --config.custom-plugin-monitor=config/custom-plugin-monitor.json::20256/healthz200ok:20256/conditions200null:20256/debug/pprof/cmdline200:20257/metrics200200go build ./...,go vetand the tests for the touched packages are clean. One pre-existing note:gofmt -l ./cmdreportsnode_problem_detector_test.goon unmodifiedmasteras well, so I left it alone rather than mixing a reformat into this change.Does this PR introduce a user-facing change?: