Multi list watch #969

brancz · 2019-11-13T13:32:11Z

What this PR does / why we need it:

Previously kube-state-metrics created multiple ListerWatchers, with
multiple reflectors for a single store. This caused that on "replace"
events, the entire store was replaced with the content of a single
reflector instead of the union of all reflectors acting on a single
store.

This patch changes the architecture to have a single reflector that is
fed by a MultiListerWatcher acting on a single store, which as a result
fixes the replace actions.

This needs a little bit more testing on my side, but I wanted to open this already to give others the chance to validate as well.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):

Fixes #942

@lilic @tariq1890

@paxbit as the original reporter, could you try this out on your setup?

brancz · 2019-11-13T13:41:03Z

My apologies, somehow I recall this working yesterday, but now I'm getting mod errors that look the same as in #949.

paxbit · 2019-11-14T11:21:52Z

@brancz Ok, thanks! I'll try as soon as you're happy regarding #949.
Also atm. I seem unable to build ksm from source using make container as it fails with:
go: k8s.io/autoscaler/vertical-pod-autoscaler@v0.0.0-20191114093134-0e82aa045fa1 requires k8s.io/api@v0.0.0: reading k8s.io/api/go.mod at revision v0.0.0: unknown revision v0.0.0
I'm not much of a go guy and cannot spend time digging into whats causing this. Do you have any suggestions?

tariq1890 · 2019-11-15T18:25:02Z

@brancz Can you rebase this?

brancz · 2019-11-15T23:26:16Z

Rebased, and now everything should build fine @paxbit @tariq1890 .

brancz · 2019-11-15T23:45:58Z

Putting on hold until we verify that it works as expected.

/hold

tariq1890 · 2019-11-15T23:55:44Z

go.mod

@@ -3,7 +3,9 @@ module k8s.io/kube-state-metrics
 require (
 	github.com/brancz/gojsontoyaml v0.0.0-20190425155809-e8bd32d46b3d
 	github.com/campoy/embedmd v1.0.0
+	github.com/coreos/prometheus-operator v0.34.0


This is a rather heavy dependency. I'd actually prefer either of the two approaches below:

i) Move the MultiListWatcher to its own project/module.
OR
ii) We could copy the implementation code over to this project. If we choose this approach, we can tweak it a bit to use klog instead of a NoOp logger implementation of a foreign (in this project's context) log project.

Preference for approach 2.

i) I don't think we would move this into its own project from the prometheus-operator as we in prometheus-operator would find it hard to maintain that and bump deps there etc.

ii) so someone, I think istio did this :D with this exact pkg, they copied it over to their project a while ago, but never kept up with our bug fixes and they ended up with a huge bug we fixed. What I mean is, we would need to maintain both pkgs then.

While I agree being dependent on prom-operator is a huge thing, I would like to see how much code we would have to maintain in that pkg in here? @brancz how much work do you think this would be to move it to ksm?

It’s already a well encapsulated package there so copying it over is trivial.

tariq1890 · 2019-11-15T23:56:01Z

go.mod

 	github.com/dgryski/go-jump v0.0.0-20170409065014-e1f439676b57
+	github.com/go-kit/kit v0.9.0


I would recommend not importing this.

tariq1890 · 2019-11-15T23:56:16Z

go.mod

 	k8s.io/klog v1.0.0
 )

+replace (


I believe we don't need these anymore.

Confirmed that we can remove these. Once you add the revision hashes I'd mentioned earlier, run go mod tidy and go mod vendor, it should work.

tariq1890 · 2019-11-15T23:57:22Z

internal/store/builder.go

-		go reflector.Run(b.ctx.Done())
-	}
+	lwf := func(ns string) cache.ListerWatcher { return listWatchFunc(b.kubeClient, ns) }
+	lw := listwatch.MultiNamespaceListerWatcher(log.NewNopLogger(), b.namespaces, nil, lwf)


I am not too sure about injecting a Noop logger. There definitely is value in logging the ops here. This goes back to my first comment.

paxbit · 2019-11-19T11:30:32Z

@brancz @lilic Built it today and it is running for 20m without showing the old behavior. Will observe it over time today till some time after lunch. If it's still ok at 4pm CET today I'll post a LGTM later.

lilic · 2019-11-19T12:26:49Z

Thanks @paxbit for testing!

paxbit · 2019-11-19T14:36:26Z

@lilic @brancz Still getting coherent results after ~3.5h runtime. LGTM. Thanks :) Appreciate the support!

tariq1890 · 2019-11-24T08:21:08Z

pkg/listwatch/listwatch.go

@@ -0,0 +1,269 @@
+// This was originally copied from the Prometheus Operator project. Due to


Would there be a license breach if we just add this as 2019 Copyright of Kube-state-metrics? Considering my earlier review comments, we would be modifying the signature of ListWatch method to avoid the go-kit dependency and other changes should trickle down.

Thoughts? cc @lilic

I would keep this, or like suggested above just vendor this in. We want this in ASAP to fix the bug, so I think just merging it ASAP and fixing things in a future release would be best, wdyt @brancz @tariq1890

brancz · 2019-12-04T10:06:30Z

Fixed all comments I believe. @lilic @tariq1890 can you give this a re-review?

tariq1890 · 2019-12-04T10:38:54Z

@brancz I pulled down your branch and confirmed that you don't need the replace directives. Providing my last set of review comments.

tariq1890 · 2019-12-04T10:39:15Z

go.mod

@@ -15,8 +16,17 @@ require (
 	k8s.io/api v0.0.0-20191112020540-7f9008e52f64
 	k8s.io/apimachinery v0.0.0-20191111054156-6eb29fdf75dc
 	k8s.io/autoscaler/vertical-pod-autoscaler v0.0.0-20191115143342-4cf961056038
-	k8s.io/client-go v0.0.0-20191109102209-3c0d1af94be5
+	k8s.io/client-go v12.0.0+incompatible


Suggested change

k8s.io/client-go v12.0.0+incompatible

k8s.io/client-go v0.0.0-20191114101535-6c5935290e33

tariq1890 · 2019-12-04T10:40:37Z

go.mod

@@ -15,8 +16,17 @@ require (
 	k8s.io/api v0.0.0-20191112020540-7f9008e52f64


Suggested change

k8s.io/api v0.0.0-20191112020540-7f9008e52f64

k8s.io/api v0.0.0-20191114100352-16d7abae0d2a

Previously kube-state-metrics created multiple ListerWatchers, with multiple reflectors for a single store. This caused that on "replace" events, the entire store was replaced with the content of a single reflector instead of the union of all reflectors acting on a single store. This patch changes the architecture to have a single reflector that is fed by a MultiListerWatcher acting on a single store, which as a result fixes the replace actions.

brancz · 2019-12-04T14:01:26Z

Reverted the mod updates. Please take a look.

tariq1890

/lgtm

k8s-ci-robot · 2019-12-04T14:11:38Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: brancz, tariq1890

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:

~~OWNERS~~ [brancz,tariq1890]

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

brancz · 2019-12-04T16:53:18Z

/hold cancel

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Nov 13, 2019

k8s-ci-robot requested review from andyxning and zouyee November 13, 2019 13:32

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

brancz force-pushed the multi-list-watch branch from 9229270 to 0c098a1 Compare November 15, 2019 23:05

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Nov 15, 2019

brancz force-pushed the multi-list-watch branch from 0c098a1 to 0fdad19 Compare November 15, 2019 23:13

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 15, 2019

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 15, 2019

tariq1890 reviewed Nov 15, 2019

View reviewed changes

brancz force-pushed the multi-list-watch branch 2 times, most recently from 8c9a396 to c617c36 Compare November 19, 2019 23:23

tariq1890 reviewed Nov 24, 2019

View reviewed changes

brancz force-pushed the multi-list-watch branch 4 times, most recently from 0fd9275 to 9b34f39 Compare December 4, 2019 09:52

tariq1890 reviewed Dec 4, 2019

View reviewed changes

brancz force-pushed the multi-list-watch branch from 9b34f39 to f4bbe71 Compare December 4, 2019 13:36

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Dec 4, 2019

tariq1890 approved these changes Dec 4, 2019

View reviewed changes

k8s-ci-robot assigned tariq1890 Dec 4, 2019

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 4, 2019

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 4, 2019

k8s-ci-robot merged commit a39d9ee into kubernetes:master Dec 4, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Multi list watch #969

Multi list watch #969

brancz commented Nov 13, 2019

brancz commented Nov 13, 2019 •

edited

Loading

paxbit commented Nov 14, 2019

tariq1890 commented Nov 15, 2019

brancz commented Nov 15, 2019

brancz commented Nov 15, 2019

tariq1890 Nov 15, 2019

tariq1890 Nov 16, 2019

lilic Nov 16, 2019

brancz Nov 16, 2019

tariq1890 Nov 15, 2019

tariq1890 Nov 15, 2019

tariq1890 Dec 4, 2019

tariq1890 Nov 15, 2019

paxbit commented Nov 19, 2019

lilic commented Nov 19, 2019

paxbit commented Nov 19, 2019

tariq1890 Nov 24, 2019

lilic Dec 3, 2019

brancz commented Dec 4, 2019

tariq1890 commented Dec 4, 2019

tariq1890 Dec 4, 2019 •

edited

Loading

tariq1890 Dec 4, 2019

brancz commented Dec 4, 2019

tariq1890 left a comment

k8s-ci-robot commented Dec 4, 2019

brancz commented Dec 4, 2019

		github.com/dgryski/go-jump v0.0.0-20170409065014-e1f439676b57
		github.com/go-kit/kit v0.9.0

		@@ -0,0 +1,269 @@
		// This was originally copied from the Prometheus Operator project. Due to

	k8s.io/client-go v12.0.0+incompatible
	k8s.io/client-go v0.0.0-20191114101535-6c5935290e33

		@@ -15,8 +16,17 @@ require (
		k8s.io/api v0.0.0-20191112020540-7f9008e52f64

	k8s.io/api v0.0.0-20191112020540-7f9008e52f64
	k8s.io/api v0.0.0-20191114100352-16d7abae0d2a

Multi list watch #969

Multi list watch #969

Conversation

brancz commented Nov 13, 2019

brancz commented Nov 13, 2019 • edited Loading

paxbit commented Nov 14, 2019

tariq1890 commented Nov 15, 2019

brancz commented Nov 15, 2019

brancz commented Nov 15, 2019

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

paxbit commented Nov 19, 2019

lilic commented Nov 19, 2019

paxbit commented Nov 19, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brancz commented Dec 4, 2019

tariq1890 commented Dec 4, 2019

tariq1890 Dec 4, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brancz commented Dec 4, 2019

tariq1890 left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Dec 4, 2019

brancz commented Dec 4, 2019

brancz commented Nov 13, 2019 •

edited

Loading

tariq1890 Dec 4, 2019 •

edited

Loading