-
Notifications
You must be signed in to change notification settings - Fork 462
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[controller-manager] Switch remaining Project
controllers to controller-runtime
#6667
[controller-manager] Switch remaining Project
controllers to controller-runtime
#6667
Conversation
Opened #6669 /test pull-gardener-e2e-kind |
/assign |
2dcf326
to
59d8feb
Compare
59d8feb
to
22f8d05
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice PR!
Few nits.
pkg/controllermanager/controller/project/activity/reconciler_test.go
Outdated
Show resolved
Hide resolved
pkg/controllermanager/controller/project/activity/reconciler.go
Outdated
Show resolved
Hide resolved
Earlier, it only worked when the used Quota was the first one in a SecretBinding.
22f8d05
to
3c3ad53
Compare
Thanks! |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rfranzke 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:
Approvers can indicate their approval by writing |
@rfranzke: The following test failed, say
Full PR test history. Your PR dashboard. Command help for this repository. 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. |
/test pull-gardener-integration ref #6679 (comment) |
Project
controller to controller-runtime
Project
controller to controller-runtime
UpdateFunc: func(e event.UpdateEvent) bool { | ||
oldObjMeta, err := meta.Accessor(e.ObjectOld) | ||
if err != nil { | ||
return false | ||
} | ||
|
||
objMeta, err := meta.Accessor(e.ObjectNew) | ||
if err != nil { | ||
return false | ||
} | ||
|
||
_, oldObjHasLabel := oldObjMeta.GetLabels()[v1beta1constants.LabelSecretBindingReference] | ||
_, newObjHasLabel := objMeta.GetLabels()[v1beta1constants.LabelSecretBindingReference] | ||
|
||
return oldObjHasLabel && newObjHasLabel |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previously, it was like this,
gardener/pkg/controllermanager/controller/project/project_activity_control.go
Lines 103 to 123 in 958fec3
func (c *Controller) projectActivityObjectUpdate(ctx context.Context, oldObj, newObj interface{}, withLabel bool) { | |
oldObjMeta, err := meta.Accessor(oldObj) | |
if err != nil { | |
return | |
} | |
newObjMeta, err := meta.Accessor(newObj) | |
if err != nil { | |
return | |
} | |
if withLabel { | |
// skip queueing if the object(secret or quota) doesn't have the "referred by a secretbinding" label | |
_, oldObjHasLabel := oldObjMeta.GetLabels()[v1beta1constants.LabelSecretBindingReference] | |
_, newObjHasLabel := newObjMeta.GetLabels()[v1beta1constants.LabelSecretBindingReference] | |
if !oldObjHasLabel && !newObjHasLabel { | |
return | |
} | |
} else if oldObjMeta.GetGeneration() == newObjMeta.GetGeneration() { | |
return | |
} |
which is not the case now.
On simple words, we were acting if either of the object has the label. If the new doesn't have the label and the old had, which means the deletion of the last secretbinding referring this secret. Don't we want to consider this as "activity"?
If not, then we should only consider the newObject. The first time the secret is "referred" the oldObject won't have label, but we want to consider this as activity too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't remember to have changed this on purpose, so probably it was a mistake. @shafeeqes can you open a PR to fix it and preserve the old behaviour?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, but should we consider the deletion of the last secretbinding
(which can be in a different namespace) as an activity?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, let's consider it as activity 👍🏻
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, then I'll go ahead.
Project
controller to controller-runtime
Project
controllers to controller-runtime
How to categorize this PR?
/area dev-productivity scalability
/kind enhancement
What this PR does / why we need it:
Refactor the remaining
Project
controller tocontroller-runtime
. The main reconciler was already refactored as part of #6645.Which issue(s) this PR fixes:
Part of #4251
Special notes for your reviewer:
Generally, we want to follow this cookbook while refactoring existing controllers:
envtest
controller-runtime
I successfully stress-tested both new integration tests against an existing Gardener system.
Release note: