[DevEx] controller/rbac: edge based events for applies #4307

sttts · 2023-07-10T14:27:43Z

Description of your changes

The Applied RBAC roles event is repeated again and again:

default          9m59s (x450 over 3d4h)   Normal    ApplyRoles               Namespace/upbound-system                                         Applied RBAC Roles
default          9m59s (x455 over 3d4h)   Normal    ApplyRoles               Namespace/default                                                Applied RBAC Roles
default          9m59s (x440 over 3d4h)   Normal    ApplyRoles               Namespace/kube-system                                            Applied RBAC Roles
default          9m59s (x437 over 3d4h)   Normal    ApplyRoles               Namespace/kube-public                                            Applied RBAC Roles
default          9m59s (x484 over 3d4h)   Normal    ApplyRoles               Namespace/kube-node-lease                                        Applied RBAC Roles

Similar for

default          2m39s (x13 over 69m)     Normal   BindClusterRole          ProviderRevision/upbound-provider-aws-s3-dbc7f981d81f       Bound system ClusterRole to provider ServiceAccount(s)
default          2m39s (x13 over 69m)     Normal   BindClusterRole          ProviderRevision/upbound-provider-family-aws-d095ac1e13dd   Bound system ClusterRole to provider ServiceAccount(s)

This PR supresses the events if nothing happens, and it prints up to the first 3 role names in case it's not an noop. With that the event is actually useful.

I have:

Read and followed Crossplane's contribution process.
Added or updated unit and E2E tests for my change.
Run make reviewable to ensure this PR is ready for review.
~~Added backport release-x.y labels to auto-backport this PR if necessary.~~

jbw976

Seems reasonable to only create an event when we do something - do you have an example from your testing of what events in this style look like now?

jbw976 · 2023-07-10T14:43:53Z

internal/controller/rbac/namespace/reconciler.go

@@ -210,9 +213,15 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (reco
 		}

 		log.Debug("Applied RBAC Role")


would it be useful in this debug log statement to include the name of the RBAC role that was just applied?

it actually is, look the log.WithValues above :) Had the same thought.

jbw976 · 2023-07-10T14:45:22Z

internal/controller/rbac/namespace/reconciler.go

 	}

-	r.record.Event(ns, event.Normal(reasonApplyRoles, "Applied RBAC Roles"))
+	sort.Strings(applied)
+	if len(applied) > 3 {


is this > 3 pattern pretty common in upstream k8s? i feel like i may have seen this before - not 💯 on it though, as I have hazy memories of thinking to myself "well why didn't you just tell me the rest of them too then?" 😂

more curious than anything - not blocking from me 😁

I think we use something like that in OpenShift. No strong opinions. I thought 3 is a good number :) 10 might be too.

negz

This looks great, thank you. FWIW there's a similar event WRT selecting a Composition in the XR controller that I've been meaning to fix if you're feeling motivated to do so.

Few style nits, but nothing blocking.

internal/controller/rbac/provider/binding/reconciler.go

negz · 2023-07-10T20:00:39Z

internal/controller/rbac/definition/reconciler.go

+	if len(applied) > 3 {
+		r.record.Event(d, event.Normal(reasonApplyRoles, fmt.Sprintf("Applied RBAC ClusterRoles: %s, %s, %s, and %d more", applied[0], applied[1], applied[2], len(applied)-3)))
+	} else if len(applied) > 0 {
+		r.record.Event(d, event.Normal(reasonApplyRoles, fmt.Sprintf("Applied RBAC ClusterRoles: %s", strings.Join(applied, ", "))))


Given this pattern is repeated a few times I wonder whether a little utility could help? I'm on the fence. Normally I prefer to avoid such little utilities for things like this, until they're almost everywhere, but I must admit I'm a little (unreasonably?) allergic to if then else statements.

I'm thinking something like:

r.record.Event(d, event.Normal(reasonApplyRoles, maybeTruncate("Applied RBAC ClusterRoles", applied))) // maybeTruncate needs a better name :) func maybeTruncate(prefix string, names []string) string { if len(names) > 3 { return fmt.Sprintf("%s: %s, and %d more", prefix, strings.Join(names[:3], ", "), len(names)-3) } return fmt.Sprintf("%s: %s", prefix, strings.Join(names, ", ")) }

added. Any good place to put it? Have 3 copies now.

I could imagine this being added as a utility in crossplane-runtime somewhere (not sure which package). I'm fine leaving a few copies scattered around for now though - we can figure out a better home if and when it becomes more widely used.

ack. Leaving it. We can deduplicate later.

negz · 2023-07-10T20:03:06Z

PS the PR / checklist-completed check is failing because it wants you to explicitly ~strikethrough~ any checklist items that you don't intend to do for this PR.

sttts · 2023-07-11T12:26:21Z

internal/controller/apiextensions/claim/reconciler.go

@@ -528,8 +529,12 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (reco
 		return reconcile.Result{Requeue: true}, errors.Wrap(r.client.Status().Update(ctx, cm), errUpdateClaimStatus)
 	}

-	log.Debug("Successfully applied composite resource")
-	record.Event(cm, event.Normal(reasonCompositeConfigure, "Successfully applied composite resource"))
+	if oldRV != cp.GetResourceVersion() {


@negz not sure I am a fan of this construct. Am open for better patterns.

See my comments about the available applicators in Slack.

Perhaps something like:

err := r.client.Apply(ctx, cp, resource.AllowUpdateIf(func(current, desired) bool { !cmp.Equal(current, desired) })) switch { case resource.IsNotAllowed(err): log.Debug("Skipped no-op composite resource apply") case err != nil: // Current error handling block default: log.Debug("Successfully applied composite resource") record.Event(cm, event.Normal(reasonCompositeConfigure, "Successfully applied composite resource")) }

I believe this would disallow the apply (i.e. patch) entirely if this reconcile didn't actually change cp. Using cmp.Equal on the entire *Unstructured feels inelegant but this approach at least seems syntactically more pleasant to me.

https://pkg.go.dev/github.com/crossplane/crossplane-runtime@v0.19.2/pkg/resource#AllowUpdateIf

Was wondering whether this can still lead to multiple events when the informers are lagging behind. Just because an update is attempted does not mean an actual update happened.

Was wondering whether this can still lead to multiple events when the informers are lagging behind.

FWIW I think the controller-runtime machinery automatically dedupes multiple events in the queue to result in one reconcile call.

and in any case this will eventually settle. If two or more, but few events are creating on rare races, that's fine. Just cosmetics (different to the RV topic in the applicator which is about consistency).

Signed-off-by: Dr. Stefan Schimanski <stefan.schimanski@upbound.io>

…nding apply Signed-off-by: Dr. Stefan Schimanski <stefan.schimanski@upbound.io>

Signed-off-by: Dr. Stefan Schimanski <stefan.schimanski@upbound.io>

sttts requested review from a team and negz as code owners July 10, 2023 14:27

sttts requested a review from MisterMX July 10, 2023 14:27

jbw976 reviewed Jul 10, 2023

View reviewed changes

sttts force-pushed the sttts-namespace-reconciler-rbac-event branch 2 times, most recently from 7dce9bd to c72cfaf Compare July 10, 2023 15:11

sttts changed the title ~~controller/rbac/namespace: edge based events for role updates~~ controller/rbac/{provider,namespace}: edge based events for applies Jul 10, 2023

sttts force-pushed the sttts-namespace-reconciler-rbac-event branch from c72cfaf to 6ec954e Compare July 10, 2023 15:22

sttts changed the title ~~controller/rbac/{provider,namespace}: edge based events for applies~~ controller/rbac: edge based events for applies Jul 10, 2023

negz approved these changes Jul 10, 2023

View reviewed changes

sttts requested a review from muvaf as a code owner July 11, 2023 12:25

sttts commented Jul 11, 2023

View reviewed changes

sttts changed the title ~~controller/rbac: edge based events for applies~~ [DevEx] controller/rbac: edge based events for applies Jul 11, 2023

sttts force-pushed the sttts-namespace-reconciler-rbac-event branch 5 times, most recently from f26326f to cb730d8 Compare July 11, 2023 13:32

sttts added 5 commits July 11, 2023 17:00

controller/rbac/namespace: edge based events for role updates

731d33a

Signed-off-by: Dr. Stefan Schimanski <stefan.schimanski@upbound.io>

controller/rbac/provider/binding: edge based events for ClusterRoleBi…

e9ec860

…nding apply Signed-off-by: Dr. Stefan Schimanski <stefan.schimanski@upbound.io>

controller/rbac/definition: edge based events for ClusterRole apply

0702d10

Signed-off-by: Dr. Stefan Schimanski <stefan.schimanski@upbound.io>

controller/rbac/provider/roles: edge based events for ClusterRole apply

63a3eea

Signed-off-by: Dr. Stefan Schimanski <stefan.schimanski@upbound.io>

Address comments: firstNAndSomeMore

bb58425

Signed-off-by: Dr. Stefan Schimanski <stefan.schimanski@upbound.io>

sttts force-pushed the sttts-namespace-reconciler-rbac-event branch from cb730d8 to fd64e44 Compare July 11, 2023 15:01

xrd/claim: event triggered composite update event

4a0bb29

Signed-off-by: Dr. Stefan Schimanski <stefan.schimanski@upbound.io>

sttts force-pushed the sttts-namespace-reconciler-rbac-event branch from fd64e44 to 4a0bb29 Compare July 14, 2023 10:10

negz approved these changes Jul 17, 2023

View reviewed changes

turkenh merged commit 91a1e06 into crossplane:master Jul 31, 2023
16 checks passed

jbw976 added this to the v1.14 milestone Aug 9, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DevEx] controller/rbac: edge based events for applies #4307

[DevEx] controller/rbac: edge based events for applies #4307

sttts commented Jul 10, 2023 •

edited

Loading

jbw976 left a comment

jbw976 Jul 10, 2023

sttts Jul 10, 2023 •

edited

Loading

jbw976 Jul 10, 2023

sttts Jul 10, 2023

negz left a comment

negz Jul 10, 2023

sttts Jul 11, 2023

negz Jul 12, 2023

sttts Jul 31, 2023

negz commented Jul 10, 2023 •

edited

Loading

sttts Jul 11, 2023 •

edited

Loading

negz Jul 12, 2023

sttts Jul 13, 2023

negz Jul 14, 2023

sttts Jul 14, 2023

sttts Jul 14, 2023

		@@ -210,9 +213,15 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (reco
		}

		log.Debug("Applied RBAC Role")

[DevEx] controller/rbac: edge based events for applies #4307

[DevEx] controller/rbac: edge based events for applies #4307

Conversation

sttts commented Jul 10, 2023 • edited Loading

Description of your changes

jbw976 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sttts Jul 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

negz 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

negz commented Jul 10, 2023 • edited Loading

sttts Jul 11, 2023 • edited Loading

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

sttts commented Jul 10, 2023 •

edited

Loading

sttts Jul 10, 2023 •

edited

Loading

negz commented Jul 10, 2023 •

edited

Loading

sttts Jul 11, 2023 •

edited

Loading