⚠️ adding multi namespaces cache #267

shawn-hurley · 2018-12-28T06:27:16Z

This will add the ability to set up a multi-namespace cache. This also has a backward incompatible change in the Informers interface which effects the cache interface.

This is based on the discussion from the BoF.
/cc @DirectXMan12 @mhrivnak @hasbro17

Wanted to get some early feedback before starting on tests.

DirectXMan12

overall looks pretty solid. couple of comments inline, but it's pretty simple and straightforward to understand. Needs godoc on the internal objects, plus a bit more explanation of when you might want to use this, but in general 👍

pkg/cache/multi_namespace_cache.go

sebgl · 2019-02-14T13:34:28Z

Drive-by comment: I think this is great and would love to see it merged 🎉

mengqiy

Need rebase.

mengqiy · 2019-02-15T19:19:12Z

pkg/cache/multi_namespace_cache.go

+	if err != nil {
+		return err
+	}
+	return json.Unmarshal(data, list)


Can we avoid marshal and then unmarshal to improve performance?

I don't have a good way to do this when say someone is asking for a PodList that I can think of.

Maybe we could do some reflection bits to make sure that the Items field is there and use the scheme to create a new Object each time. I don't know how much better performance this would be though, WDYT?

Something like this: https://github.com/shawn-hurley/controller-runtime/blob/feature/poc-reflection-mulitnamespaced-cache-list/pkg/cache/multi_namespace_cache.go#L153-L172

I think the code snippet you linked look reasonable. I believe it can improve performance by avoiding some unnecessary work.
But what confused me is why you treat ResourceVersion specially. How about other metadata fields?

so here was my thinking:

SelfLink string - would think that this also does not make sense because we are combining multiple calls into one list. There is not a link that one could call to get the same list. This is also optional, so I figured it was fine to ignore.

Continue string - I think this also does not make a ton of sense because we should be handling this value in the cache by getting all the values. I would imagine that this is not necessary to set.

and the only other field is ResourceVersion which we will need.

Does that make sense?

It makes sense.
Re. Continue string, I found we currently don't support it in controller-runtime's ListOptions comparing to upstream ListOptions. So I guess we don't need to worry about it ATM.

@droot @DirectXMan12 thoughts regarding the reflection bits linked above?

I am pushing up an even better implementation in my mind. It will use api-machinery to manipulate the list, instead of using reflection directly.

mengqiy · 2019-02-15T19:21:06Z

pkg/cache/multi_namespace_cache.go

+		}(ns, cache)
+	}
+	<-stopCh
+	return nil


Should the multiNamespaceCache main goroutine wait for each cache goroutine to finish before returning?
Or it doesn't matter?

I don't think that it matters, just following the pattern that I think we started here:
https://github.com/kubernetes-sigs/controller-runtime/blob/master/pkg/cache/internal/deleg_map.go#L60-L62

Thoughts?

shawn-hurley · 2019-02-17T23:45:44Z

@DirectXMan12 or @mengqiy can you guys re-start the build I think it was a flake.

mengqiy

Using apimachinery helpers make it look much better!

pkg/cache/multi_namespace_cache.go

* Add Multinamespace Cache type * ⚠️ Change the GetInformer methods to return a controller runtime Informer interface * Add multinamespace Informer type to handle namespaced infromers * ⚠️ move NewCacheFunc from manager package to Cache pacakge

mengqiy · 2019-02-21T19:39:11Z

/lgtm
Let's wait by EOD before merging just in case others may want to take a look.

k8s-ci-robot · 2019-02-22T18:18:43Z

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by: shawn-hurley

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

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

Before fc804a4 (kubernetes-sigs#267), the `cache.Informers` interface methods returned `k8s.io/client-go/tools/cache.SharedIndexInformer`s which were by default compatible with the built-in `source.Informer`. Users could create arbitrary `cache.Cache` instances (or get them from the `Manager`) and then use `controller.Watch` to drive a controller with a `source.Informer` from the `cache.Cache`. With fc804a4, the `cache.Informers` interface was changed to return `cache.Informer` instances; however, `source.Informer` was not updated to accept a `cache.Informer`, and so users can no longer use the built-in `source.Informer` with `cache.Cache`. The `cache.Informer` interface appears to satisfy the needs of `source.Informer`. This commit broadens `source.Informer` to accept a `source.Informer`, restoring the prior capability while remaining compatible with `SharedIndexInformer` use.

Before fc804a4 (kubernetes-sigs#267), the `cache.Informers` interface methods returned `k8s.io/client-go/tools/cache.SharedIndexInformer`s which were by default compatible with the built-in `source.Informer`. Users could create arbitrary `cache.Cache` instances (or get them from the `Manager`) and then use `controller.Watch` to drive a controller with a `source.Informer` from the `cache.Cache`. With fc804a4, the `cache.Informers` interface was changed to return `cache.Informer` instances; however, `source.Informer` was not updated to accept a `cache.Informer`, and so users can no longer use the built-in `source.Informer` with `cache.Cache`. The `cache.Informer` interface appears to satisfy the needs of `source.Informer`. This commit broadens `source.Informer` to accept a `cache.Informer`, restoring the prior capability while remaining compatible with `SharedIndexInformer` use.

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Dec 28, 2018

k8s-ci-robot requested review from droot and pwittrock December 28, 2018 06:27

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Dec 28, 2018

shawn-hurley mentioned this pull request Jan 3, 2019

WIP/POC: pkg/cache;pkg/manager - Adding multi name spaced cache operator-framework/operator-sdk#785

Closed

DirectXMan12 suggested changes Jan 4, 2019

View reviewed changes

pkg/cache/multi_namespace_cache.go Outdated Show resolved Hide resolved

pkg/cache/multi_namespace_cache.go Outdated Show resolved Hide resolved

pkg/cache/multi_namespace_cache.go Outdated Show resolved Hide resolved

pkg/cache/multi_namespace_cache.go Show resolved Hide resolved

shawn-hurley force-pushed the feature/poc-multinamespaced-cache branch from 9ad3d39 to 2f6a0b3 Compare January 22, 2019 21:26

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 Jan 22, 2019

shawn-hurley force-pushed the feature/poc-multinamespaced-cache branch from 5c59791 to a6adef3 Compare January 23, 2019 18:38

shawn-hurley changed the title ~~WIP/POC ⚠️ adding multi namespaced cache.~~ ⚠️ adding multi namespaces cache Jan 24, 2019

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 24, 2019

shawn-hurley force-pushed the feature/poc-multinamespaced-cache branch from a6adef3 to 35bbe4b Compare January 24, 2019 16:11

shawn-hurley force-pushed the feature/poc-multinamespaced-cache branch from 35bbe4b to b8dca46 Compare February 1, 2019 16:58

sebgl mentioned this pull request Feb 14, 2019

Support multi-namespace watches elastic/cloud-on-k8s#405

Closed

mengqiy reviewed Feb 15, 2019

View reviewed changes

shawn-hurley force-pushed the feature/poc-multinamespaced-cache branch from b8dca46 to 5559c19 Compare February 17, 2019 17:57

mengqiy reviewed Feb 21, 2019

View reviewed changes

pkg/cache/multi_namespace_cache.go Outdated Show resolved Hide resolved

pkg/cache/multi_namespace_cache.go Outdated Show resolved Hide resolved

shawn-hurley force-pushed the feature/poc-multinamespaced-cache branch from e2d83e8 to 72316bb Compare February 21, 2019 13:46

shawn-hurley force-pushed the feature/poc-multinamespaced-cache branch from 72316bb to fc804a4 Compare February 21, 2019 14:15

k8s-ci-robot assigned mengqiy Feb 21, 2019

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

mengqiy added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 22, 2019

k8s-ci-robot merged commit 68ae79e into kubernetes-sigs:master Feb 22, 2019

shawn-hurley deleted the feature/poc-multinamespaced-cache branch February 22, 2019 18:41

shawn-hurley mentioned this pull request Mar 13, 2019

Allow restricting the Cache's ListWatch to a set of namespaces #218

Closed

shawn-hurley mentioned this pull request Mar 26, 2019

REQUEST: New membership for @shawn-hurley kubernetes/org#655

Closed

6 tasks

ironcladlou mentioned this pull request Apr 3, 2019

⚠️ source: make the Informer source compatible with cache.Informer #383

Merged

rifelpet mentioned this pull request Sep 13, 2019

[WIP] Upgrade controller-runtime kubernetes-sigs/aws-load-balancer-controller#1023

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

⚠️ adding multi namespaces cache #267

⚠️ adding multi namespaces cache #267

shawn-hurley commented Dec 28, 2018 •

edited

Loading

DirectXMan12 left a comment

sebgl commented Feb 14, 2019 •

edited

Loading

mengqiy left a comment

mengqiy Feb 15, 2019

shawn-hurley Feb 17, 2019

shawn-hurley Feb 18, 2019

mengqiy Feb 19, 2019

shawn-hurley Feb 20, 2019

mengqiy Feb 20, 2019

mengqiy Feb 20, 2019

shawn-hurley Feb 20, 2019

mengqiy Feb 15, 2019

shawn-hurley Feb 17, 2019

shawn-hurley commented Feb 17, 2019

mengqiy left a comment

mengqiy commented Feb 21, 2019

k8s-ci-robot commented Feb 22, 2019

⚠️ adding multi namespaces cache #267

⚠️ adding multi namespaces cache #267

Conversation

shawn-hurley commented Dec 28, 2018 • edited Loading

DirectXMan12 left a comment

Choose a reason for hiding this comment

sebgl commented Feb 14, 2019 • edited Loading

mengqiy 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

shawn-hurley commented Feb 17, 2019

mengqiy left a comment

Choose a reason for hiding this comment

mengqiy commented Feb 21, 2019

k8s-ci-robot commented Feb 22, 2019

shawn-hurley commented Dec 28, 2018 •

edited

Loading

sebgl commented Feb 14, 2019 •

edited

Loading