P&F: Enable support for indexes in watch tracker #105974

wojtek-t · 2021-10-28T13:30:41Z

NONE

/kind bug
/priority important-soon
/sig api-machinery

/assign @MikeSpreitzer @tkashem

wojtek-t · 2021-10-28T13:31:55Z

staging/src/k8s.io/apiserver/pkg/util/flowcontrol/watch_tracker.go

+	return w.forgetWatch(identifier, index)
+}
+
+func (w *watchTracker) updateIndexLocked(identifier *watchIdentifier, index *indexValue, incr int) {


Obviously we want to add tests for it before submitting - but I wanted to get your thoughts if this is acceptable (for the sake of making progress on it).
@MikeSpreitzer @tkashem

fedebongio · 2021-10-28T20:09:36Z

/triage accepted

tkashem · 2021-10-29T15:18:22Z

/lgtm

tkashem · 2021-10-29T15:19:09Z

didn't realize that verify is failing, looks good to me otherwise

/lgtm cancel

MikeSpreitzer

It is certainly safe while we have the usage of these watch counts disabled!

Considering that we had no usage of watch counts in earlier releases, I think this would be a safe step to take even the watch counts get used.

MikeSpreitzer · 2021-10-29T19:29:16Z

staging/src/k8s.io/apiserver/pkg/util/flowcontrol/watch_tracker.go

+// builtinIndexes represents of set of indexes registered in
+// watchcache that are indexing watches and increase speed of
+// their processing.


It would be good to include an explanation of what the keys and values are.

MikeSpreitzer · 2021-10-29T19:31:06Z

staging/src/k8s.io/apiserver/pkg/util/flowcontrol/watch_tracker.go

 	watchCount map[watchIdentifier]int
 }

 func NewWatchTracker() WatchTracker {
 	return &watchTracker{
 		watchCount: make(map[watchIdentifier]int),
+		indexes:    getBuiltinIndexes(),


I would order the fields in this construction the same as in the struct definition.

MikeSpreitzer · 2021-10-29T19:37:57Z

staging/src/k8s.io/apiserver/pkg/util/flowcontrol/watch_tracker.go

+		// empty value (we assume that number of those for empty value is not
+		// smaller than for any other value). That's not always true, but in


I think the assumption being made here is rather that the count for not-unset non-empty value stays tiny in absolute terms.

Not exactly (and what you're saying is also not true in practice - e.g. each kubelet is watching for its own pods, which means that there are N such watches with N being number of nodes).

The observation is that for a given object create/update/delete the amount of work (also filtering given index implementaion in watchcache) is proportional to:

watches interested in all objects (those that are "unset" here)

watches interssted in the value coming from that object

So we're basically assuming (which seem to be true in practice too), that the difference between "unset + my-value" is small comparing to "unset + empty".

Tried to clarify the comment.

wojtek-t

@MikeSpreitzer @tkashem - PTAL

wojtek-t · 2021-11-01T19:31:31Z

staging/src/k8s.io/apiserver/pkg/util/flowcontrol/watch_tracker.go

+// builtinIndexes represents of set of indexes registered in
+// watchcache that are indexing watches and increase speed of
+// their processing.


wojtek-t · 2021-11-01T19:31:43Z

staging/src/k8s.io/apiserver/pkg/util/flowcontrol/watch_tracker.go

 	watchCount map[watchIdentifier]int
 }

 func NewWatchTracker() WatchTracker {
 	return &watchTracker{
 		watchCount: make(map[watchIdentifier]int),
+		indexes:    getBuiltinIndexes(),


wojtek-t · 2021-11-01T19:45:46Z

staging/src/k8s.io/apiserver/pkg/util/flowcontrol/watch_tracker.go

+		// empty value (we assume that number of those for empty value is not
+		// smaller than for any other value). That's not always true, but in


Not exactly (and what you're saying is also not true in practice - e.g. each kubelet is watching for its own pods, which means that there are N such watches with N being number of nodes).

The observation is that for a given object create/update/delete the amount of work (also filtering given index implementaion in watchcache) is proportional to:

watches interested in all objects (those that are "unset" here)

watches interssted in the value coming from that object

So we're basically assuming (which seem to be true in practice too), that the difference between "unset + my-value" is small comparing to "unset + empty".

Tried to clarify the comment.

MikeSpreitzer

Yes, this looks like a good start.

MikeSpreitzer · 2021-11-01T20:10:48Z

/lgtm

k8s-ci-robot · 2021-11-01T20:10:53Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: MikeSpreitzer, 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:

~~staging/src/k8s.io/apiserver/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-triage-robot · 2021-11-01T22:52:29Z

The Kubernetes project has merge-blocking tests that are currently too flaky to consistently pass.

This bot retests PRs for certain kubernetes repos according to the following rules:

The PR does have any do-not-merge/* labels
The PR does not have the needs-ok-to-test label
The PR is mergeable (does not have a needs-rebase label)
The PR is approved (has cncf-cla: yes, lgtm, approved labels)
The PR is failing tests required for merge

You can:

Review the full test history for this PR
Prevent this bot from retesting with /lgtm cancel or /hold
Help make our tests less flaky by following our Flaky Tests Guide

/retest

k8s-ci-robot assigned MikeSpreitzer Oct 28, 2021

k8s-ci-robot added the release-note-none Denotes a PR that doesn't merit a release note. label Oct 28, 2021

k8s-ci-robot assigned tkashem Oct 28, 2021

wojtek-t mentioned this pull request Oct 28, 2021

[WIP][DO NOT REVIEW] Adjust P&F watch tracking to accomodate for watchcache filtering #105876

Closed

k8s-ci-robot requested review from sttts and yue9944882 October 28, 2021 13:31

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. area/apiserver labels Oct 28, 2021

wojtek-t commented Oct 28, 2021

View reviewed changes

k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Oct 28, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 29, 2021

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 29, 2021

MikeSpreitzer reviewed Oct 29, 2021

View reviewed changes

wojtek-t added 2 commits November 1, 2021 20:35

P&F: Update WatchTracker interface to pass more information

12746f4

Enable support for indexes in WatchTracker

21ec77d

wojtek-t force-pushed the pf_watch_support_8 branch from 3a8f222 to 21ec77d Compare November 1, 2021 19:52

wojtek-t commented Nov 1, 2021

View reviewed changes

MikeSpreitzer approved these changes Nov 1, 2021

View reviewed changes

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

k8s-ci-robot merged commit 88f8974 into kubernetes:master Nov 2, 2021

k8s-ci-robot added this to the v1.23 milestone Nov 2, 2021

wojtek-t mentioned this pull request Nov 3, 2021

P&F fix watch tracker bug #106121

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

P&F: Enable support for indexes in watch tracker #105974

P&F: Enable support for indexes in watch tracker #105974

wojtek-t commented Oct 28, 2021

wojtek-t Oct 28, 2021

fedebongio commented Oct 28, 2021

tkashem commented Oct 29, 2021

tkashem commented Oct 29, 2021 •

edited

MikeSpreitzer left a comment

MikeSpreitzer Oct 29, 2021

wojtek-t Nov 1, 2021

MikeSpreitzer Oct 29, 2021

wojtek-t Nov 1, 2021

MikeSpreitzer Oct 29, 2021

wojtek-t Nov 1, 2021

wojtek-t left a comment

wojtek-t Nov 1, 2021

wojtek-t Nov 1, 2021

wojtek-t Nov 1, 2021

MikeSpreitzer left a comment

MikeSpreitzer commented Nov 1, 2021

k8s-ci-robot commented Nov 1, 2021

k8s-triage-robot commented Nov 1, 2021

		// empty value (we assume that number of those for empty value is not
		// smaller than for any other value). That's not always true, but in

P&F: Enable support for indexes in watch tracker #105974

P&F: Enable support for indexes in watch tracker #105974

Conversation

wojtek-t commented Oct 28, 2021

Choose a reason for hiding this comment

fedebongio commented Oct 28, 2021

tkashem commented Oct 29, 2021

tkashem commented Oct 29, 2021 • edited

MikeSpreitzer 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

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

Choose a reason for hiding this comment

MikeSpreitzer left a comment

Choose a reason for hiding this comment

MikeSpreitzer commented Nov 1, 2021

k8s-ci-robot commented Nov 1, 2021

k8s-triage-robot commented Nov 1, 2021

tkashem commented Oct 29, 2021 •

edited