-
Notifications
You must be signed in to change notification settings - Fork 545
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
OCPBUGS-17157: *: detect when all objects are labelled, restart #3028
OCPBUGS-17157: *: detect when all objects are labelled, restart #3028
Conversation
@stevekuznetsov: This pull request references Jira Issue OCPBUGS-17157, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: The bug has been updated to refer to the pull request using the external bug tracker. In response to this:
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. |
4201200
to
99461d2
Compare
99461d2
to
365a2ec
Compare
365a2ec
to
e77fae7
Compare
e77fae7
to
a079ab1
Compare
1ef8099
to
c7f21a3
Compare
func ServiceAccountFilter(isServiceAccountReferenced func(namespace, name string) bool) func(object metav1.Object) bool { | ||
return func(object metav1.Object) bool { | ||
return HasOLMOwnerRef(object) || HasOLMLabel(object) || isServiceAccountReferenced(object.GetNamespace(), object.GetName()) | ||
} | ||
} | ||
|
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.
Am I correct in assuming that these filters that are split out are used in special cases where we don't want (or can't) use the filters
map?
If so, maybe call out the different use cases somehow? I'm a little concerned that a future commit might add something to the filters
map that ends up being overwritten by some other code that blindly changes the map key to point to another function.
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.
Yeah, these are special filters that can't be expressed in a simple way since they require other state inputs. When we're running these filters in the labelling controller, we can answer their other state needs with informers. When we're doing the at-startup check, though, we give them live client calls. I agree that it's easy to break this if you're not careful but I would suggest that we guard against that with careful review to this code in the future rather than some sort of super-clever coding paradigm. I don't know that this needs to get any more complex than it already is.
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.
can't be expressed in a simple way since they require other state inputs.
Ah I see that now.
I would suggest that we guard against that with careful review to this code in the future rather than some sort of super-clever coding paradigm. I don't know that this needs to get any more complex than it already is.
Totally agree! But I don't think a helper function would add too much complexity:
type filtersMap map[schema.GroupVersionResource]func(metav1.Object) bool
func mustAddFilter(m filtersMap, gvr schema.GroupVersionResource, filter func(metav1.Object) bool) {
if _, ok := m[gvr]; ok {
panic("programmer error: cannot add filter to map: already contains key " + gvr.String())
}
m[gvr] = filter
}
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.
That function covers the one case of error, but the other and more insidious one is that you need to start & run every labeller you check for in Validate()
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.
No super strong feelings here as long as we're confident we can page this knowledge in fairly easily in the future. Maybe just more well-placed comments is enough.
Signed-off-by: Steve Kuznetsov <[email protected]>
Signed-off-by: Steve Kuznetsov <[email protected]>
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: joelanford, perdasilva, stevekuznetsov 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 |
New changes are detected. LGTM label has been removed. |
@stevekuznetsov: This pull request references Jira Issue OCPBUGS-17157, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: In response to this:
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. |
Signed-off-by: Steve Kuznetsov <[email protected]>
2f02237
to
3552193
Compare
03302ff
@stevekuznetsov: Jira Issue OCPBUGS-17157: Some pull requests linked via external trackers have merged:
The following pull requests linked via external trackers have not merged: These pull request must merge or be unlinked from the Jira bug in order for it to move to the next state. Once unlinked, request a bug refresh with Jira Issue OCPBUGS-17157 has not been moved to the MODIFIED state. In response to this:
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. |
When all of the k8s objects that need labels have them, we are good to exit the process. The next Pod that start up will detect that all labels are present and be able to filter informers going forward.