🌱Add watch for ClusterResourceSet resources #3410

sedefsavas · 2020-07-28T17:12:34Z

What this PR does / why we need it:
This PR adds watch on ClusterResourceSet resources (ConfigMaps/Secrets), so that for the resources that is missing during Apply will be reconciled on creation or update of resources.

One down side of this is everytime a resource is created/updated, all CRS objects in the same namespace will be reconciled. Although CRS is added as owner of the resources during CRS creation since newly created resources were not there, they will not have owner refs and we cannot use owner references to get only the CRSs that has the created resource.

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

sedefsavas · 2020-07-28T17:15:58Z

Although there is no creation/update of Secrets, resourceToClusterResourceSet() is contantly getting triggered. There should be something I am missing in predicates.
@vincepri any ideas?

util/predicates/configmap_predicates.go

exp/addons/controllers/clusterresourceset_controller.go

util/predicates/configmap_predicates.go

util/predicates/secret_predicates.go

vincepri · 2020-07-28T20:50:21Z

/assign
/milestone v0.3.8

ncdc · 2020-07-29T16:59:06Z

exp/addons/controllers/clusterresourceset_controller.go

+
+	for _, crs := range crsList.Items {
+		for _, resource := range crs.Spec.Resources {
+			if resource.Kind == o.Object.GetObjectKind().GroupVersionKind().Kind && crs.Name == o.Meta.GetName() {


Technically we should be checking on both Group and Kind, as someone could create a foo.io/v123 Secret or ConfigMap.

Do we need to worry about core vs empty group equivalency here when comparing the group, though? I think core types are probably the one exception where it might be ok to avoid checking group.

I'm suggesting we worry about ensuring group="",Kind="Secret" and not matching on group="foo.io",Kind="Secret" - wouldn't that make sense?

Do we need to worry about also testing for group == "core" in addition to group == "" ?

No - despite being known as the core group, the only valid value for that group is ""

If we store the runtime.Scheme on r (ClusterResourceSetReconciler), you should be able to use apiutil.GVKForObject

It was originally there but I had Sedef remove it because we weren't using it 😄. Would it make more sense to do the func-that-returns-a-func approach so we don't have to do a lookup every time?

This is the mapping function's parameter:

func (r *ClusterResourceSetReconciler) resourceToClusterResourceSet(o handler.MapObject) []ctrl.Request {

Cannot reach o's Kind.

Edit: page refreshed late. Ignore this, it was before the suggestions.

Yeah, func createMapFunc(kind schema.GroupKind) func(o handler.MapObject) []ctrl.Request { return func(...) ....}

We removed the Scheme in the Binding controller, we still have it here, so used that one.

sedefsavas · 2020-07-29T20:11:55Z

I will change the predicates to watch only creation because I see continues ConfigMap updates coming for ConfigMaps like: capi-kubeadm-bootstrap-system, cert-manager-cainjector-leader-election, capv-controller-manager-runtime etc.

I need to make sure this is specific to updates, because earlier I was observing constant Secret creations but now can't observe that anymore.

cc @vincepri @ncdc @detiber

ncdc · 2020-07-29T20:16:14Z

I would either

Skip the predicates for now, as the map function will still do what we need (it may get called more than necessary), or
Keep them and just return true for create + update

ncdc · 2020-07-29T20:16:52Z

I would not expect the leader election configmap updates to put an overwhelming burden on the system

ncdc · 2020-07-30T17:41:37Z

Correction, our default leaderelection updates are every 2 seconds, and we have multiple leaderelection configmaps, so we should try to mitigate this amount of updates:

Switch from configmap to lease record (separate type)
Reduce the update frequency
?

vincepri · 2020-07-30T18:08:50Z

Reduce the update frequency

We provide flags today to change the frequency, we could change those defaults. Should we do this in a separate PR/issue? Do you prefer having this in v0.3.8 or wait for later?

ncdc · 2020-07-30T18:40:04Z

Definitely a separate PR. Maybe an issue too so we can discuss?

As for 0.3.8, if we don't make any changes to the lease update frequency, do you have any concerns about watching ConfigMaps?

sedefsavas · 2020-07-30T18:52:25Z

/retest

sedefsavas · 2020-07-30T18:54:22Z

Please note:

Removed watching for Updates until we fix frequent ConfigMap updates related to leader election.
Only watching Create events.

sedefsavas · 2020-07-30T18:54:57Z

/retest

sedefsavas · 2020-07-30T19:15:18Z

/retest

util/predicates/configmap_predicates.go

util/predicates/secret_predicates.go

sedefsavas · 2020-07-30T23:34:19Z

/restest

sedefsavas · 2020-07-31T16:15:24Z

/restest

sedefsavas · 2020-07-31T16:15:30Z

/retest

sedefsavas · 2020-07-31T17:36:40Z

I think this PR is good to go.
cc @vincepri

vincepri

/approve
/assign @CecileRobertMichon @detiber
for final lgtm

k8s-ci-robot · 2020-07-31T20:08:08Z

[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

detiber · 2020-07-31T20:09:02Z

/lgtm

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 Jul 28, 2020

k8s-ci-robot requested review from CecileRobertMichon and justinsb July 28, 2020 17:12

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

sedefsavas force-pushed the resource_watch branch from c2e8be0 to 864545c Compare July 28, 2020 17:14

detiber reviewed Jul 28, 2020

View reviewed changes

util/predicates/configmap_predicates.go Outdated Show resolved Hide resolved

exp/addons/controllers/clusterresourceset_controller.go Outdated Show resolved Hide resolved

detiber reviewed Jul 28, 2020

View reviewed changes

exp/addons/controllers/clusterresourceset_controller.go Outdated Show resolved Hide resolved

vincepri reviewed Jul 28, 2020

View reviewed changes

exp/addons/controllers/clusterresourceset_controller.go Outdated Show resolved Hide resolved

vincepri reviewed Jul 28, 2020

View reviewed changes

util/predicates/configmap_predicates.go Outdated Show resolved Hide resolved

vincepri reviewed Jul 28, 2020

View reviewed changes

util/predicates/secret_predicates.go Outdated Show resolved Hide resolved

k8s-ci-robot assigned vincepri Jul 28, 2020

k8s-ci-robot added this to the v0.3.8 milestone Jul 28, 2020

ncdc reviewed Jul 29, 2020

View reviewed changes

sedefsavas changed the title ~~[WIP] 🌱Add watch for ClusterResourceSet resources~~ 🌱Add watch for ClusterResourceSet resources Jul 30, 2020

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 Jul 30, 2020

sedefsavas force-pushed the resource_watch branch from e27945d to 07cfd79 Compare July 30, 2020 19:10

detiber reviewed Jul 30, 2020

View reviewed changes

util/predicates/configmap_predicates.go Outdated Show resolved Hide resolved

util/predicates/secret_predicates.go Outdated Show resolved Hide resolved

util/predicates/secret_predicates.go Outdated Show resolved Hide resolved

sedefsavas force-pushed the resource_watch branch from 7af5c5b to 493dd36 Compare July 30, 2020 22:35

Add watch for ClusterResourceSet resources

8d3fb61

sedefsavas force-pushed the resource_watch branch from 493dd36 to 8d3fb61 Compare July 30, 2020 23:27

k8s-ci-robot assigned CecileRobertMichon Jul 31, 2020

vincepri approved these changes Jul 31, 2020

View reviewed changes

k8s-ci-robot assigned detiber Jul 31, 2020

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

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

k8s-ci-robot merged commit 9e4e82a into kubernetes-sigs:master Jul 31, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🌱Add watch for ClusterResourceSet resources #3410

🌱Add watch for ClusterResourceSet resources #3410

sedefsavas commented Jul 28, 2020 •

edited

Loading

sedefsavas commented Jul 28, 2020

vincepri commented Jul 28, 2020

ncdc Jul 29, 2020

detiber Jul 29, 2020

ncdc Jul 29, 2020

detiber Jul 29, 2020

ncdc Jul 29, 2020

vincepri Jul 30, 2020 •

edited

Loading

ncdc Jul 30, 2020

sedefsavas Jul 30, 2020 •

edited

Loading

ncdc Jul 30, 2020

sedefsavas Jul 30, 2020

sedefsavas commented Jul 29, 2020

ncdc commented Jul 29, 2020

ncdc commented Jul 29, 2020

ncdc commented Jul 30, 2020

vincepri commented Jul 30, 2020

ncdc commented Jul 30, 2020

sedefsavas commented Jul 30, 2020

sedefsavas commented Jul 30, 2020

sedefsavas commented Jul 30, 2020

sedefsavas commented Jul 30, 2020

sedefsavas commented Jul 30, 2020

sedefsavas commented Jul 31, 2020

sedefsavas commented Jul 31, 2020

sedefsavas commented Jul 31, 2020

vincepri left a comment

k8s-ci-robot commented Jul 31, 2020

detiber commented Jul 31, 2020

🌱Add watch for ClusterResourceSet resources #3410

🌱Add watch for ClusterResourceSet resources #3410

Conversation

sedefsavas commented Jul 28, 2020 • edited Loading

sedefsavas commented Jul 28, 2020

vincepri commented Jul 28, 2020

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 Jul 30, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sedefsavas Jul 30, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sedefsavas commented Jul 29, 2020

ncdc commented Jul 29, 2020

ncdc commented Jul 29, 2020

ncdc commented Jul 30, 2020

vincepri commented Jul 30, 2020

ncdc commented Jul 30, 2020

sedefsavas commented Jul 30, 2020

sedefsavas commented Jul 30, 2020

sedefsavas commented Jul 30, 2020

sedefsavas commented Jul 30, 2020

sedefsavas commented Jul 30, 2020

sedefsavas commented Jul 31, 2020

sedefsavas commented Jul 31, 2020

sedefsavas commented Jul 31, 2020

vincepri left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jul 31, 2020

detiber commented Jul 31, 2020

sedefsavas commented Jul 28, 2020 •

edited

Loading

vincepri Jul 30, 2020 •

edited

Loading

sedefsavas Jul 30, 2020 •

edited

Loading