⚠️ Accept options in remote.NewClusterCacheTracker #4693

ykakarap · 2021-05-27T19:54:26Z

What this PR does / why we need it:
This PR add an new optional options argument to the NewClusterCacheTracker function.
This new options argument can be used to control the objects that the client should not cache. Defaults to:

corev1.ConfigMap{}
corev1.Secret{}

It can also be used to pass a desired logger. Defaults to ctrl.Log.WithName("remote").WithName("ClusterCacheTracker")

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 #4146

ykakarap · 2021-05-27T19:54:40Z

/cc @vincepri

vincepri

/retitle ⚠️ Accept options in remote.NewClusterCacheTracker

vincepri · 2021-05-27T20:04:36Z

controllers/remote/cluster_cache.go

+type clusterCacheTrackerOptions struct {
+	log                   logr.Logger
+	clientDisableCacheFor []client.Object
+}


Could we just export this instead of having variable functions?

Suggested change

type clusterCacheTrackerOptions struct {

log logr.Logger

clientDisableCacheFor []client.Object

}

type ClusterCacheTrackerOptions struct {

Logger logr.Logger

ClientDisableCacheFor []client.Object

}

vincepri · 2021-05-27T20:05:29Z

controllers/remote/cluster_cache.go

+
+func setDefaultOptions(opts *clusterCacheTrackerOptions) {
+	if opts.log == nil {
+		opts.log = ctrl.Log.WithName("remote").WithName("ClusterCacheTracker")


Should NullLogger be the default here?

+1. Will change it

fabriziopandini

/lgtm
Two nits, not blocking

fabriziopandini · 2021-05-28T10:38:54Z

exp/addons/controllers/suite_test.go

@@ -38,7 +38,7 @@ func TestMain(m *testing.M) {
 	fmt.Println("Creating new test environment")
 	testEnv = helpers.NewTestEnvironment()

-	trckr, err := remote.NewClusterCacheTracker(log.NullLogger{}, testEnv.Manager)
+	trckr, err := remote.NewClusterCacheTracker(testEnv.Manager, remote.SetLogger(log.NullLogger{}))


Suggested change

trckr, err := remote.NewClusterCacheTracker(testEnv.Manager, remote.SetLogger(log.NullLogger{}))

trckr, err := remote.NewClusterCacheTracker(testEnv.Manager)

Give it is already the default

fabriziopandini · 2021-05-28T10:41:29Z

controllers/remote/cluster_cache.go

+
+// SetClientDisableCacheFor allows to set the objects that should not be
+// cached by the client.
+func SetClientDisableCacheFor(objs []client.Object) Option {


What about

Suggested change

func SetClientDisableCacheFor(objs []client.Object) Option {

func SetClientDisableCacheFor(objs ...client.Object) Option {

So the callers can simply do SetClientDisableCacheFor(Object1, Object2) without creating an array/slice.

FYI, I suggested above to not use functions, but instead just expose the Options struct

@vincepri Regarding the options argument, are you suggesting to change the function signature from

func NewClusterCacheTracker(manager ctrl.Manager, options ...Option) (*ClusterCacheTracker, error) {...}

to

func NewClusterCacheTracker(manager ctrl.Manager, options ClusterCacheTrackerOptions) (*ClusterCacheTracker, error) {...}

this will make options a mandatory argument and I remember you suggesting to make it an optional one.

If we want to keep the options argument as optional we will have to do

func NewClusterCacheTracker(manager ctrl.Manager, options ...ClusterCacheTrackerOptions) (*ClusterCacheTracker, error) {...}

and then perform merging of structs to construct the final options and this doesn't look like a pattern followed elsewhere in the project.

vincepri · 2021-05-28T15:09:01Z

/kind release-blocking

vincepri · 2021-06-02T19:51:02Z

controllers/remote/cluster_cache.go

+type ClusterCacheTrackerOptions struct {
+	Log                   logr.Logger
+	ClientDisableCacheFor []client.Object
+}


Suggested change

type ClusterCacheTrackerOptions struct {

Log logr.Logger

ClientDisableCacheFor []client.Object

}

type ClusterCacheTrackerOptions struct {

// Log is the logger used throughout the lifecycle of caches.

// Defaults to a no-op logger if it's not set.

Log logr.Logger

// ClientDisableCacheFor instructs the Client to never cache the following objects,

// it'll instead query the API server directly.

ClientDisableCacheFor []client.Object

}

vincepri

One last comment, rest LGTM

ykakarap · 2021-06-02T20:09:47Z

@vincepri addressed comments.

ykakarap · 2021-06-02T20:54:40Z

/test pull-cluster-api-e2e-main

enxebre · 2021-06-03T08:57:18Z

controllers/remote/cluster_cache.go

-	client client.Client
-	scheme *runtime.Scheme
+	log                   logr.Logger
+	clientDisableCacheFor []client.Object


May be name this clientUncachedObjects (or just uncachedObjects since this is a field of ClusterCacheTracker ) to be consistent NewDelegatingClientInput

k8s-ci-robot · 2021-06-04T18:00:08Z

@ykakarap: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
pull-cluster-api-apidiff-main	`1fe22fc`	link	`/test pull-cluster-api-apidiff-main`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

vincepri

/approve
/lgtm

k8s-ci-robot · 2021-06-04T18:25:06Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vincepri

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~~ [vincepri]

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 cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 27, 2021

k8s-ci-robot requested review from fabriziopandini and vincepri May 27, 2021 19:54

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label May 27, 2021

vincepri reviewed May 27, 2021

View reviewed changes

k8s-ci-robot changed the title ~~🌱 add optional options argument to NewClusterCacheTracker function~~ ⚠️ Accept options in remote.NewClusterCacheTracker May 27, 2021

ykakarap force-pushed the expose_uncache_option_tracker branch from afdcf89 to 49cce11 Compare May 27, 2021 21:30

fabriziopandini reviewed May 28, 2021

View reviewed changes

k8s-ci-robot assigned fabriziopandini May 28, 2021

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

k8s-ci-robot added kind/release-blocking Issues or PRs that need to be closed before the next CAPI release needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels May 28, 2021

ykakarap force-pushed the expose_uncache_option_tracker branch from 49cce11 to 03a862e Compare June 2, 2021 18:25

k8s-ci-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jun 2, 2021

ykakarap force-pushed the expose_uncache_option_tracker branch from 03a862e to 002d323 Compare June 2, 2021 18:32

vincepri reviewed Jun 2, 2021

View reviewed changes

ykakarap force-pushed the expose_uncache_option_tracker branch from 002d323 to d58e314 Compare June 2, 2021 20:09

enxebre reviewed Jun 3, 2021

View reviewed changes

accept options in remote.NewClusterCacheTracker

1fe22fc

ykakarap force-pushed the expose_uncache_option_tracker branch from d58e314 to 1fe22fc Compare June 4, 2021 17:58

vincepri approved these changes Jun 4, 2021

View reviewed changes

k8s-ci-robot assigned vincepri Jun 4, 2021

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

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 4, 2021

k8s-ci-robot merged commit e29ffc2 into kubernetes-sigs:master Jun 4, 2021

k8s-ci-robot added this to the v0.4 milestone Jun 4, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

⚠️ Accept options in remote.NewClusterCacheTracker #4693

⚠️ Accept options in remote.NewClusterCacheTracker #4693

ykakarap commented May 27, 2021

ykakarap commented May 27, 2021

vincepri left a comment

vincepri May 27, 2021

vincepri May 27, 2021

ykakarap May 27, 2021

fabriziopandini left a comment

fabriziopandini May 28, 2021

fabriziopandini May 28, 2021

vincepri May 28, 2021

ykakarap Jun 1, 2021

vincepri commented May 28, 2021

vincepri Jun 2, 2021

vincepri left a comment

ykakarap commented Jun 2, 2021

ykakarap commented Jun 2, 2021

enxebre Jun 3, 2021 •

edited

Loading

k8s-ci-robot commented Jun 4, 2021

vincepri left a comment

k8s-ci-robot commented Jun 4, 2021

	trckr, err := remote.NewClusterCacheTracker(testEnv.Manager, remote.SetLogger(log.NullLogger{}))
	trckr, err := remote.NewClusterCacheTracker(testEnv.Manager)

	func SetClientDisableCacheFor(objs []client.Object) Option {
	func SetClientDisableCacheFor(objs ...client.Object) Option {

⚠️ Accept options in remote.NewClusterCacheTracker #4693

⚠️ Accept options in remote.NewClusterCacheTracker #4693

Conversation

ykakarap commented May 27, 2021

ykakarap commented May 27, 2021

vincepri 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

fabriziopandini 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

vincepri commented May 28, 2021

Choose a reason for hiding this comment

vincepri left a comment

Choose a reason for hiding this comment

ykakarap commented Jun 2, 2021

ykakarap commented Jun 2, 2021

enxebre Jun 3, 2021 • edited Loading

Choose a reason for hiding this comment

k8s-ci-robot commented Jun 4, 2021

vincepri left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jun 4, 2021

enxebre Jun 3, 2021 •

edited

Loading