add dynamic config metrics #57527

mtaufen · 2017-12-21T22:58:27Z

This PR exports config-releated metrics from the Kubelet.
The Guages for active, assigned, and last-known-good config can be used
to identify config versions and produce aggregate counts across several
nodes. The error-reporting Gauge can be used to determine whether a node
is experiencing a config-related error, and to prodouce an aggregate
count of nodes in an error state.

kubernetes/enhancements#281

The Kubelet now exports metrics that report the assigned (node_config_assigned), last-known-good (node_config_last_known_good), and active (node_config_active) config sources, and a metric indicating whether the node is experiencing a config-related error (node_config_error). The config source metrics always report the value 1, and carry the node_config_name, node_config_uid, node_config_resource_version, and node_config_kubelet_key labels, which identify the config version. The error metric reports 1 if there is an error, 0 otherwise.

crassirostris

Overall approach SGTM

crassirostris · 2018-01-22T18:26:04Z

pkg/kubelet/metrics/metrics.go

+
+func SetAssignedNodeConfigSource(val string) {
+	// TODO(mtaufen): will this eliminate previous metric, or generate a parallel one?
+	AssignedNodeConfigSource.WithLabelValues(val).Set(1)


This approach, implemented like this, is going to result in memory leak, because old values are not GC'd, as you noted in the comment

@loburm @x13n FYI, this is the usecase for string metrics

crassirostris · 2018-01-22T18:27:59Z

pkg/kubelet/metrics/metrics.go

+	AssignedNodeConfigSourceKey = "assigned_node_config_source"
+	// AssignedNodeConfigSourceOkayKey's metric indicates whether the currently assigned config source and the config it contains is valid
+	AssignedNodeConfigSourceOkayKey       = "assigned_node_config_source_okay"
+	AssignedNodeConfigSourceStatusTrue    = 1


Please use string values, performance considerations in this case do not outweigh the decreased readability

As in, just report a string value in another label key-value pair?

Since prometheus doesn't support string valued metrics:
https://prometheus.io/docs/concepts/metric_types/
prometheus/prometheus#2227

crassirostris · 2018-01-22T18:28:50Z

pkg/kubelet/metrics/metrics.go

+	// AssignedNodeConfigSourceKey's metric reports the config source the Kubelet believes is currently assigned to it
+	AssignedNodeConfigSourceKey = "assigned_node_config_source"
+	// AssignedNodeConfigSourceOkayKey's metric indicates whether the currently assigned config source and the config it contains is valid
+	AssignedNodeConfigSourceOkayKey       = "assigned_node_config_source_okay"


Nit: IMHO ok is more widely used than okay

fair enough

crassirostris · 2018-01-22T18:30:32Z

pkg/kubelet/kubeletconfig/controller.go

 	utillog.Infof("starting controller")

+	// set metrics when all is said and done
+	var using string


using is a bad name, is says nothing and spans a huge scope

Also, if you change the logic below later, it may be used uninitialized

I don't understand that global defer at all, why not leave setting metrics local?

crassirostris · 2018-01-22T18:36:06Z

pkg/kubelet/metrics/metrics.go

@@ -101,7 +122,7 @@ var (
 			Help:      "Interval in microseconds between relisting in PLEG.",
 		},
 	)
-	// Metrics of remote runtime operations.
+	// Metrics for remote runtime operations


Mostly I see a slow convergence towards comments being complete sentences, with full stop at the end and so on.

crassirostris · 2018-01-22T18:38:37Z

pkg/kubelet/metrics/metrics.go

 	DevicePluginRegistrationCountKey = "device_plugin_registration_count"
 	DevicePluginAllocationLatencyKey = "device_plugin_alloc_latency_microseconds"
+	// Metric keys for node config sources
+	// AssignedNodeConfigSourceKey's metric reports the config source the Kubelet believes is currently assigned to it
+	AssignedNodeConfigSourceKey = "assigned_node_config_source"


Why the assigned_node prefix? It's little big confusing, why not just config_source or dynamic_config_source?

There are three config references you'd want the Kubelet to report:

What it believes its currently assigned source is.

What it believes its last-known-good source is.

Which of the above two it's actually using.

NodeConfigSource is the type of the ConfigSource field on the Node object and is used to point the Kubelet at a config source, hence node_config_source.

Could do:

node_config_source_assigned

node_config_source_last_known_good

node_config_source_using (or some name other than using)

crassirostris · 2018-01-22T18:39:28Z

pkg/kubelet/metrics/metrics.go

+	AssignedNodeConfigSourceStatusUnknown = 0
+	AssignedNodeConfigSourceStatusFalse   = -1
+	// LastKnownGoodNodeConfigSourceKey's metric indicates what the Kubelet is treating as it's last-known-good config source
+	LastKnownGoodNodeConfigSourceKey = "last_known_good_node_config_source"


Try to make these hierarchical, that helps, e.g. dynamic_config_last_known_source

mtaufen · 2018-01-23T17:30:31Z

Thanks @crassirostris for taking a look at this

mtaufen · 2018-01-23T21:05:46Z

/retest

mtaufen · 2018-05-22T17:33:50Z

pkg/kubelet/metrics/metrics.go

+	ActiveConfigKey               = "node_config_active"
+	LastKnownGoodConfigKey        = "node_config_last_known_good"
+	ConfigErrorKey                = "node_config_error"
+	ConfigNameLabelKey            = "node_config_name" // this is a fully resolved name, e.g. an API path or a URL, "Name" is short but a little confusing name for this key...


@dashpole any naming suggestions for this?

SelfLink is the URL representing an object in ObjectMeta. Maybe "node_config_self_link"? This should be equivalent to the self link for the configmap, right?

@dashpole and I settled on node_config_source, as it accurately identifies what the corresponding value describes while being reasonably generic ("local", or a SelfLink, or maybe some URL in the future).

mtaufen · 2018-05-22T17:34:38Z

/retest

mtaufen · 2018-05-22T17:37:33Z

/status approved-for-milestone

mtaufen · 2018-05-22T17:43:10Z

/remove-lifecycle stale

k8s-github-robot · 2018-05-22T17:43:48Z

[MILESTONENOTIFIER] Milestone Pull Request: Up-to-date for process

@crassirostris @dashpole @dchen1107 @mtaufen

Pull Request Labels

sig/node: Pull Request will be escalated to these SIGs if needed.
priority/important-soon: Escalate to the pull request owners and SIG owner; move out of milestone after several unsuccessful escalation attempts.
kind/feature: New functionality.

Help

mtaufen · 2018-05-22T19:56:57Z

/retest

This PR exports config-releated metrics from the Kubelet. The Guages for active, assigned, and last-known-good config can be used to identify config versions and produce aggregate counts across several nodes. The error-reporting Gauge can be used to determine whether a node is experiencing a config-related error, and to prodouce an aggregate count of nodes in an error state.

dashpole · 2018-05-22T22:19:47Z

/lgtm

dchen1107 · 2018-05-23T17:37:41Z

/lgtm

k8s-ci-robot · 2018-05-23T17:37:48Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dashpole, dchen1107, mtaufen

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~pkg/kubelet/OWNERS~~ [dchen1107]
~~test/e2e_node/OWNERS~~ [dchen1107]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-github-robot · 2018-05-24T02:44:19Z

Automatic merge from submit-queue (batch tested with PRs 64013, 63896, 64139, 57527, 62102). If you want to cherry-pick this change to another branch, please follow the instructions here.

mtaufen added area/kubelet area/kubelet-api area/monitoring do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. sig/node Categorizes an issue or PR as relevant to SIG Node. labels Dec 21, 2017

mtaufen self-assigned this Dec 21, 2017

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Dec 21, 2017

mtaufen added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 21, 2017

mtaufen force-pushed the kc-metric branch 3 times, most recently from e4fec46 to 1c6b5e2 Compare December 24, 2017 23:01

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Dec 24, 2017

mtaufen force-pushed the kc-metric branch from 42f8595 to a9dad8f Compare December 27, 2017 02:05

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 4, 2018

mtaufen changed the title ~~(WIP) node config source metrics~~ (WIP) PoC node config source metrics Jan 19, 2018

mtaufen assigned crassirostris Jan 19, 2018

mtaufen force-pushed the kc-metric branch from a9dad8f to 33569f6 Compare January 19, 2018 22:14

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 19, 2018

mtaufen force-pushed the kc-metric branch from 33569f6 to 23a9a26 Compare January 19, 2018 22:24

mtaufen changed the title ~~(WIP) PoC node config source metrics~~ PoC node config source metrics Jan 19, 2018

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 19, 2018

crassirostris reviewed Jan 22, 2018

View reviewed changes

mtaufen force-pushed the kc-metric branch from 999073c to 1a81dd5 Compare January 23, 2018 18:58

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 30, 2018

mtaufen mentioned this pull request Feb 15, 2018

Dynamic Kubelet Configuration kubernetes/enhancements#281

Closed

27 tasks

mtaufen force-pushed the kc-metric branch from 1ed8216 to 1faff12 Compare May 22, 2018 17:26

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 22, 2018

mtaufen changed the title ~~PoC node config source metrics~~ add dynamic config metrics May 22, 2018

mtaufen removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label May 22, 2018

mtaufen assigned dashpole May 22, 2018

mtaufen commented May 22, 2018

View reviewed changes

mtaufen force-pushed the kc-metric branch from 1faff12 to 4aa1a2e Compare May 22, 2018 17:36

mtaufen added this to the v1.11 milestone May 22, 2018

k8s-ci-robot added the status/approved-for-milestone label May 22, 2018

mtaufen assigned dchen1107 May 22, 2018

mtaufen force-pushed the kc-metric branch 3 times, most recently from fb81c29 to e951978 Compare May 22, 2018 19:51

mtaufen force-pushed the kc-metric branch from e951978 to fd3432e Compare May 22, 2018 21:09

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 22, 2018

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 23, 2018

k8s-github-robot merged commit 731eaec into kubernetes:master May 24, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add dynamic config metrics #57527

add dynamic config metrics #57527

mtaufen commented Dec 21, 2017 •

edited

Loading

crassirostris left a comment

crassirostris Jan 22, 2018

crassirostris Jan 22, 2018

crassirostris Jan 22, 2018

mtaufen Jan 23, 2018

crassirostris Jan 22, 2018

mtaufen Jan 23, 2018

crassirostris Jan 22, 2018

crassirostris Jan 22, 2018

crassirostris Jan 22, 2018

crassirostris Jan 22, 2018

mtaufen Jan 23, 2018

crassirostris Jan 22, 2018

mtaufen commented Jan 23, 2018

mtaufen commented Jan 23, 2018

mtaufen May 22, 2018

dashpole May 22, 2018

mtaufen May 22, 2018

mtaufen commented May 22, 2018

mtaufen commented May 22, 2018

mtaufen commented May 22, 2018

k8s-github-robot commented May 22, 2018

mtaufen commented May 22, 2018

dashpole commented May 22, 2018

dchen1107 commented May 23, 2018

k8s-ci-robot commented May 23, 2018

k8s-github-robot commented May 24, 2018

add dynamic config metrics #57527

add dynamic config metrics #57527

Conversation

mtaufen commented Dec 21, 2017 • edited Loading

crassirostris left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtaufen commented Jan 23, 2018

mtaufen commented Jan 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtaufen commented May 22, 2018

mtaufen commented May 22, 2018

mtaufen commented May 22, 2018

k8s-github-robot commented May 22, 2018

mtaufen commented May 22, 2018

dashpole commented May 22, 2018

dchen1107 commented May 23, 2018

k8s-ci-robot commented May 23, 2018

k8s-github-robot commented May 24, 2018

mtaufen commented Dec 21, 2017 •

edited

Loading