Improve observability of node authorizer #92466

mborsz · 2020-06-24T13:02:16Z

What type of PR is this?
/kind feature

What this PR does / why we need it:
It make it possible to check if node authorizer is working and if it's able to process all incoming events.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Add node_authorizer_actions_duration_seconds metric that can be used to estimate load to node authorizer.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

/assign @wojtek-t
/assign @liggitt

plugin/pkg/auth/authorizer/node/graph.go

liggitt · 2020-06-24T14:22:21Z

plugin/pkg/auth/authorizer/node/metrics.go

+
+var (
+	// NodeAuthorizerActionsDuration is a histogram of duration of graph actions in node authorizer.
+	NodeAuthorizerActionsDuration = metrics.NewHistogramVec(


is there a reason to export this? do we want things from other packages referencing this?

liggitt · 2020-06-24T14:22:54Z

plugin/pkg/auth/authorizer/node/metrics.go

+			Name:           "actions_duration_seconds",
+			Help:           "Histogram of duration of graph actions in node authorizer.",
+			StabilityLevel: metrics.ALPHA,
+			// Start with 0.1ms with the last bucket being [~200ms, Inf)
+			Buckets: metrics.ExponentialBuckets(0.0001, 2, 12),


cc @logicalhan @kubernetes/sig-instrumentation-pr-reviews for feedback on metric type/name/construction

actions seems a bit generic but the metric suffix is in line with current instrumentation guidance (i.e. duration_seconds)

plugin/pkg/auth/authorizer/node/graph.go

logicalhan · 2020-06-25T20:41:03Z

plugin/pkg/auth/authorizer/node/metrics.go

+	)
+)
+
+func init() {


Would you mind modifying this to the following signature?

func RegisterMetrics(r Registry) { r.MustRegister(graphActionsDuration) }

And then instead of importing this package to instantiate the metric as a side-effect, we can explicitly invoke the MustRegister function with the metric registry in some boot sequence code-path. This is the pattern we are trying to move towards. We do this currently in the kubelet (https://github.com/kubernetes/kubernetes/blob/v1.19.0-alpha.0/pkg/kubelet/server/server.go#L368-L373).

You can also just pass in the legacy registry instead of a custom one, if your metrics endpoint is already just using the legacyregistry. This will make it easier for us to get rid of the legacyregistry later on (which we intend to do, since it is deprecated).

I changed to this pattern with one diff: legancyregistry is a package and not an object, so I cannot call RegisterMetrics(legacyregistry). I changed the signature to accept function to call, like: RegisterMetrics(legacyregistry.MustRegister). Please let me know if this works.

It's not possible to do that this way with legacyregistry:

I added a node.RegisterMetrics(legacyregistry.MustRegistry) call to node/config (where we decide if we are going to use node authorizer)

This node/config can be called multiple times in a single binary (e.g. in our tests)

This triggers panic due to multiple metric registration

I have to add sync.Once to dedupe metrics registration, but this way, if RegisterMetric will be called twice for multiple metric registries, the second call will be completely ignored leading to not registering this

I changed this to the pattern that is used everywhere in kube-apiserver for legacyregistry: hardcoding legacyregistry in MustRegister and calling MustRegister on the node/config which seems to be the best option we have currently in kube-apiserver.

Yeah, it's a bit entangled in the apiserver currently.

logicalhan · 2020-06-25T20:49:47Z

plugin/pkg/auth/authorizer/node/metrics.go

+			Help:           "Histogram of duration of graph actions in node authorizer.",
+			StabilityLevel: metrics.ALPHA,
+			// Start with 0.1ms with the last bucket being [~200ms, Inf)
+			Buckets: metrics.ExponentialBuckets(0.0001, 2, 12),


Are you sure you don't need granularity for this metric above 200 milliseconds?

It should. Graph struct is doing purely in-memory operations, so 200 ms for a single call seems to be way too much.

fejta-bot · 2020-09-23T21:21:16Z

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

fejta-bot · 2020-10-23T22:04:32Z

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

mborsz · 2020-10-28T14:05:12Z

/remove-lifecycle rotten

mborsz · 2020-10-28T14:33:41Z

PTAL

ehashman · 2020-10-30T22:11:08Z

@mborsz can we get tests passing for review? At least one of these failures doesn't look like a flake at a glance

wojtek-t · 2020-10-31T18:58:17Z

Agree - this one seems relevant: https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/92466/pull-kubernetes-integration/1321459745499910144
@mborsz - can you please take a look?

mborsz · 2020-11-05T11:50:32Z

/retest

mborsz · 2020-11-05T12:22:54Z

Sorry, tests should be fixed now.

ehashman · 2020-11-05T17:56:31Z

/assign @logicalhan

mborsz · 2020-11-09T15:34:16Z

Friendly ping

logicalhan

/lgtm
/approve

wojtek-t

Just one small comment from me.

wojtek-t · 2020-11-09T17:24:22Z

plugin/pkg/auth/authorizer/node/graph.go

+	start := time.Now()
+	defer func() {
+		graphActionsDuration.WithLabelValues("AddPod").Observe(time.Since(start).Seconds())
+	}()


Why is DeletePod not intstrumented?

Yes, good point. I added metrics to DeletePod and SetNodeConfigMap.

* Adding some metrics to the graph * Adding log message when node authorizer has synced Change-Id: I3447d6bc389a0b82ded1db2a7a4ae41d79486c2b

wojtek-t · 2020-11-10T07:44:40Z

/lgtm
/approve

[Based on Han`s approval from sig-instrumentation perspective]

k8s-ci-robot · 2020-11-10T07:45:08Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: logicalhan, mborsz, wojtek-t

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/kubeapiserver/authorizer/OWNERS~~ [wojtek-t]
~~plugin/pkg/auth/authorizer/node/OWNERS~~ [wojtek-t]

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

k8s-ci-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Jun 24, 2020

k8s-ci-robot assigned liggitt and wojtek-t Jun 24, 2020

k8s-ci-robot requested review from liggitt and tallclair June 24, 2020 13:03

k8s-ci-robot added sig/auth Categorizes an issue or PR as relevant to SIG Auth. sig/instrumentation Categorizes an issue or PR as relevant to SIG Instrumentation. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jun 24, 2020

liggitt reviewed Jun 24, 2020

View reviewed changes

plugin/pkg/auth/authorizer/node/graph.go Outdated Show resolved Hide resolved

liggitt reviewed Jun 24, 2020

View reviewed changes

mborsz force-pushed the nmetrics branch from cc1a49b to 122a1b1 Compare June 25, 2020 13:54

logicalhan reviewed Jun 25, 2020

View reviewed changes

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Sep 23, 2020

k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Oct 23, 2020

k8s-ci-robot removed the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label Oct 28, 2020

mborsz force-pushed the nmetrics branch from 122a1b1 to 7e0ad77 Compare October 28, 2020 14:32

k8s-ci-robot added area/apiserver sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. labels Oct 28, 2020

mborsz force-pushed the nmetrics branch 2 times, most recently from 14a65ca to 325efd5 Compare November 5, 2020 10:53

mborsz force-pushed the nmetrics branch from 325efd5 to 20a0e67 Compare November 5, 2020 12:33

k8s-ci-robot assigned logicalhan Nov 5, 2020

logicalhan approved these changes Nov 9, 2020

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 9, 2020

wojtek-t reviewed Nov 9, 2020

View reviewed changes

Improve observability of node authorizer:

4d81f7e

* Adding some metrics to the graph * Adding log message when node authorizer has synced Change-Id: I3447d6bc389a0b82ded1db2a7a4ae41d79486c2b

mborsz force-pushed the nmetrics branch from 20a0e67 to 4d81f7e Compare November 10, 2020 07:41

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 10, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 10, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 10, 2020

k8s-ci-robot merged commit 4628c60 into kubernetes:master Nov 10, 2020

k8s-ci-robot added this to the v1.20 milestone Nov 10, 2020

github-actions bot mentioned this pull request Nov 18, 2020

Week Ending November 15, 2020 dev-obs/actus#273

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve observability of node authorizer #92466

Improve observability of node authorizer #92466

mborsz commented Jun 24, 2020

liggitt Jun 24, 2020

mborsz Jun 25, 2020

liggitt Jun 24, 2020

logicalhan Jun 25, 2020 •

edited

logicalhan Jun 25, 2020

mborsz Oct 28, 2020

mborsz Nov 5, 2020

logicalhan Nov 9, 2020

logicalhan Jun 25, 2020

mborsz Oct 28, 2020

fejta-bot commented Sep 23, 2020

fejta-bot commented Oct 23, 2020

mborsz commented Oct 28, 2020

mborsz commented Oct 28, 2020

ehashman commented Oct 30, 2020

wojtek-t commented Oct 31, 2020

mborsz commented Nov 5, 2020

mborsz commented Nov 5, 2020

ehashman commented Nov 5, 2020

mborsz commented Nov 9, 2020

logicalhan left a comment

wojtek-t left a comment

wojtek-t Nov 9, 2020

mborsz Nov 10, 2020

wojtek-t commented Nov 10, 2020

k8s-ci-robot commented Nov 10, 2020

Improve observability of node authorizer #92466

Improve observability of node authorizer #92466

Conversation

mborsz commented Jun 24, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

logicalhan Jun 25, 2020 • edited

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

fejta-bot commented Sep 23, 2020

fejta-bot commented Oct 23, 2020

mborsz commented Oct 28, 2020

mborsz commented Oct 28, 2020

ehashman commented Oct 30, 2020

wojtek-t commented Oct 31, 2020

mborsz commented Nov 5, 2020

mborsz commented Nov 5, 2020

ehashman commented Nov 5, 2020

mborsz commented Nov 9, 2020

logicalhan left a comment

Choose a reason for hiding this comment

wojtek-t left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wojtek-t commented Nov 10, 2020

k8s-ci-robot commented Nov 10, 2020

logicalhan Jun 25, 2020 •

edited