Repository navigation
Conversation
kubernetes-prow
Bot
added
the
cncf-cla: yes
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: bjencks 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 |
kubernetes-prow
Bot
added
the
needs-ok-to-test
|
Hi @bjencks. 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. |
kubernetes-prow
Bot
added
the
size/XXL
|
/ok-to-test |
kubernetes-prow
Bot
added
ok-to-test
838ae1e to
2711d59
Compare
Add a new built-in problem daemon plugin (apiserver-monitor) that checks connectivity to the Kubernetes API server. It specifically creates a new client on every poll in order to test the full stack from TCP connection up. This required moving kube_rest.Config creation from problemclient into pkg/util (util.NewKubeClientConfig) and initializing util.GlobalKubeClientConfig in npdMain for both problemclient and apiserver-monitor to consume.
wangzhen127
left a comment
wangzhen127
left a comment
There was a problem hiding this comment.
Could you add e2e coverage? The unit tests only cover inClusterConfig=false over plain/insecure HTTP, so the real in-cluster path (CA + SA token + protobuf Accept against the real /version) never runs. Cheapest options: (a) a kustomize patch in test/e2e/manifests/ that adds --config.apiserver-monitor=/config/apiserver-monitor.json for the GCE cluster jobs — a smoke test of the in-cluster path on a real API server (DaemonSet stays Ready, problem_counter{reason="APIServerUnreachable"} stays 0); (b) in test/e2e/metriconly, a context that starts NPD with --apiserver-override=http://127.0.0.1:1?inClusterConfig=false and a short-interval config and asserts problem_gauge{type="APIServerUnreachable",reason="APIServerUnreachable"} == 1. Both work in metricsOnly mode since they check metrics; either needs its own short-interval config, which is another reason to settle the defaults.
| "strconv" | ||
|
|
||
| "k8s.io/apimachinery/pkg/runtime/schema" | ||
| "k8s.io/client-go/kubernetes/scheme" |
There was a problem hiding this comment.
This import (plus clientcmd) is what drags ~105 packages (all k8s.io/api/* groups and most of client-go) into every binary that imports pkg/util, which includes the system-log watchers and the Windows health checker. Measured master → PR: log-counter 19.1 → 31.9 MB (+67%, startup peak RSS 14.2 → 21.9 MB, ~5× init CPU from scheme registration), Windows health-checker.exe 15.2 → 21.4 MB (+41%); node-problem-detector itself is unchanged and links exactly one new package. log-counter is exec'd every 5 min by the default *-monitor-counter.json plugins, so this adds per-exec cost to an existing plugin — the opposite of the "no new code / cheap" goal. Please move this file into its own package (e.g. pkg/util/kubeclient) so only the k8s exporter and the apiserver monitor pay for it.
| /* | ||
| Copyright 2016 The Kubernetes Authors All rights reserved. | ||
|
|
||
| Licensed under the Apache License, Version 2.0 (the "License"); | ||
| you may not use this file except in compliance with the License. | ||
| You may obtain a copy of the License at | ||
|
|
||
| http://www.apache.org/licenses/LICENSE-2.0 | ||
|
|
||
| Unless required by applicable law or agreed to in writing, software | ||
| distributed under the License is distributed on an "AS IS" BASIS, | ||
| WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| See the License for the specific language governing permissions and | ||
| limitations under the License. | ||
| */ | ||
|
|
There was a problem hiding this comment.
The original configs.go header was Copyright 2014 Google Inc. All Rights Reserved.; a move/rename should keep the original header rather than replacing it with a new year/attribution.
| var err error | ||
| util.GlobalKubeClientConfig, err = util.NewKubeClientConfig(npdo.ApiServerOverride, npdo.QPS, npdo.Burst) | ||
| if err != nil { | ||
| klog.Warningf("Failed to create kube client config: %v", err) | ||
| } |
There was a problem hiding this comment.
Two regressions from building this unconditionally and downgrading the error to a warning:
--enable-k8s-exporter=falsewithout this monitor now always logsFailed to create kube client config: unable to load in-cluster configuration, ...on standalone/VM installs.- With the k8s exporter enabled and config creation failing, startup now dies with
panic: GlobalKubeClientConfig is not initialized(see problem_client.go) instead of master'spanic: unable to load in-cluster configuration, ....
Suggest building it only when npdo.EnableK8sExporter || len(*npdo.MonitorConfigPaths["apiserver-monitor"]) > 0 and failing fast with the real error in that case (or a lazy getter returning (cfg, err) so each consumer can surface the cause).
| if util.GlobalKubeClientConfig == nil { | ||
| panic("GlobalKubeClientConfig is not initialized") | ||
| } |
There was a problem hiding this comment.
This panic now hides the actual error, which was logged as a warning several lines earlier. Please surface the real cause here (see comment on npdMain).
| if util.GlobalKubeClientConfig == nil { | ||
| klog.Fatalf("GlobalKubeClientConfig is not initialized") | ||
| } |
There was a problem hiding this comment.
Same as the problemclient comment: klog.Fatalf("GlobalKubeClientConfig is not initialized") loses the underlying error.
| "invokeInterval": "30m", | ||
| "timeout": "30s", |
There was a problem hiding this comment.
30m/30s here vs 10s/5s in config.go — please align them. With metricsOnly: true (per the config.go comment) the interval is purely a detection-latency vs. API-server-load trade-off, because every poll is a fresh TCP+TLS handshake against kube-apiserver: 30 m is too coarse to be useful for alerting, while 10 s is ~500 handshakes/s on a 5k-node cluster. 1 m seems a defensible middle; whatever you pick, please document the per-poll cost next to it so operators of large clusters know what they're turning on.
| // Explicitly set Proxy so client-go's tlsTransportCache treats this config as | ||
| // uncacheable and creates a fresh http.Transport on each RESTClientFor call | ||
| // instead of reusing a cached transport and its already-established TCP connection. | ||
| kubeConfig.Proxy = http.ProxyFromEnvironment |
There was a problem hiding this comment.
Worth stating explicitly that this relies on an implementation detail of client-go's transport/cache.go (tlsConfigKey returns uncacheable when Proxy != nil), and that TestCheckAPIServer's new-connection assertion is the canary if a client-go bump changes it. (Setting it to http.ProxyFromEnvironment is semantically a no-op since that's client-go's default when Proxy is nil — good.)
| for _, condition := range a.conditions { | ||
| if err := problemmetrics.GlobalProblemMetricsManager.SetProblemGauge( | ||
| condition.Type, condition.Reason, condition.Status == types.True); err != nil { | ||
| klog.Errorf("Failed to update problem gauge metrics for problem %q, reason %q: %v", | ||
| condition.Type, condition.Reason, err) | ||
| } | ||
| } |
There was a problem hiding this comment.
This emits a perpetual problem_gauge{type="APIServerUnreachable",reason="APIServerIsReachable"} 0 series alongside the real one, which is what #1358(2) argued against for custompluginmonitor. It matches current CPM behaviour, so fine for consistency — but since metrics are the primary (or only) output of this monitor, worth confirming it's intentional: consumers must filter on reason="APIServerUnreachable", not just on type.
| newReason = condition.Type | ||
| newMessage = checkErr.Error() |
There was a problem hiding this comment.
The problem reason — which is also the reason label value on both metrics — is hard-coded to the condition type, whereas CPM rules let users configure it. Fine for a first cut; a configurable problem reason would let a config distinguish e.g. timeouts from refused connections. (The message only matters when conditions are reported; it embeds the full URL incl. ?timeout=… and is only refreshed on transitions.)
| * `--config.apiserver-monitor`: List of paths to apiserver monitor config files, comma-separated, e.g. | ||
| [config/apiserver-monitor.json](https://github.com/kubernetes/node-problem-detector/blob/master/config/apiserver-monitor.json). | ||
| Node problem detector will start a separate apiserver monitor for each configuration. |
There was a problem hiding this comment.
Please add a short pkg/apiservermonitor/README.md with the config schema including metricsOnly (the other first-party monitors have one and the table links to the package dir), and note in the flag docs that each poll opens a fresh TLS connection and what that costs at scale, plus that unless metricsOnly is set the condition/events are written to the Node whenever the k8s exporter is enabled.
Add a new built-in problem daemon plugin (apiserver-monitor) that checks connectivity to the Kubernetes API server.
It specifically creates a new client on every poll in order to test the full stack from TCP connection up.
This required moving kube_rest.Config creation from problemclient into pkg/util (util.NewKubeClientConfig) and initializing util.GlobalKubeClientConfig in npdMain for both problemclient and apiserver-monitor to consume.