controllers: fix dynamic crd predicate

[leelavg]

if a single predicate reject the event then the runtime doesn't queue the reconcile for that controller and for dynamic crd feature multiple crds should be checked for event and at the same time presence of a single matching crd with corresponding predicate functions should initiate the reconcile.

this commit uses helper function in predicate and creates the required logical conditions and refactor the crd predicate to directly take bools based on crd availability.

[leelavg]

/hold

for testing.

[leelavg]

@rewantsoni ptal.

[leelavg]

if a single predicate reject the event then the runtime doesn't queue the reconcile

• sample https://github.com/kubernetes-sigs/controller-runtime/blob/8e44a4307f70a4d5f4c5aaf0869e2bec7167f4ab/pkg/internal/source/event_handler.go#L83-L87

	for _, p := range e.predicates {
		if !p.Create(c) {
			return
		}
	}

similar for other events can be found in same file.

[rewantsoni]

We have moved to EventTypePredicate in ocs-client op to make the predicate more generic, could we align?

[leelavg]

that doesn't solve the issue, possible to take a look at the description and mentioned EventTypePredicate solves it?

[rewantsoni]

/lgtm

[leelavg]

/unhold

Loaned a cluster which already had provider-client setup and didn't test for virt sc creation but tested for restart when the crd state changes as follows

disregard the slight time diff, seems to be time skew b/n my lab machine and ocp in another region.

================================= when crd is installed, ocs-op restarts within a second

> date --utc && k create -f virt-crd.yaml
Wed Dec  4 07:06:11 UTC 2024
customresourcedefinition.apiextensions.k8s.io/virtualmachines.kubevirt.io created

[...]

{"level":"error","ts":"2024-12-04T07:05:18Z","msg":"Reconciler error","controller":"storagecluster","controllerGroup":"ocs.openshift.io","controllerKind":"[...]
{"level":"info","ts":"2024-12-04T07:05:19Z","logger":"cmd","msg":"Go Version: go1.22.10"}
{"level":"info","ts":"2024-12-04T07:05:19Z","logger":"cmd","msg":"Go OS/Arch: linux/amd64"}
{"level":"info","ts":"2024-12-04T07:05:19Z","logger":"cmd","msg":"Cluster is running on OpenShift."}


================================= when crd is deleted, ocs-op restarts within 5 seconds

> date --utc && k delete -f virt-crd.yaml
Wed Dec  4 07:09:04 UTC 2024
customresourcedefinition.apiextensions.k8s.io "virtualmachines.kubevirt.io" deleted

[...]

{"level":"error","ts":"2024-12-04T07:08:08Z","msg":"Reconciler error","controller":"storagecluster","controllerGroup":"ocs.openshift.io","controllerKind":"[...]
{"level":"info","ts":"2024-12-04T07:08:12Z","logger":"cmd","msg":"Go Version: go1.22.10"}
{"level":"info","ts":"2024-12-04T07:08:12Z","logger":"cmd","msg":"Go OS/Arch: linux/amd64"}
{"level":"info","ts":"2024-12-04T07:08:12Z","logger":"cmd","msg":"Cluster is running on OpenShift."}

[leelavg]

#2911 (comment)

• rebased and added a comment.

[leelavg]

/jira backport release-4.18

[openshift-ci-robot]

@leelavg: The following backport issues have been created:

Queuing cherrypicks to the requested branches to be created after this PR merges:
/cherrypick release-4.18

In response to this:

/jira backport release-4.18

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 openshift-eng/jira-lifecycle-plugin repository.

[openshift-cherrypick-robot]

@openshift-ci-robot: once the present PR merges, I will cherry-pick it on top of release-4.18 in a new PR and assign it to you.

In response to this:

@leelavg: The following backport issues have been created:

Queuing cherrypicks to the requested branches to be created after this PR merges:
/cherrypick release-4.18

In response to this:

/jira backport release-4.18

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 openshift-eng/jira-lifecycle-plugin 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-sigs/prow repository.

[nb-ohad]

/hold

[nb-ohad]

@leelavg we do not want to couple to predicate, each one should stay separate. The event predicate is a generic predicate to filter events based on event type. If the relationship between the predicates is wrong (and AND operation) let's fix that and not change the predicate themselves. Same goes for the order

[leelavg]

let's fix that and not change the predicate themselves

• the fix is to change the predicate, older predicate function is updated in client-op and we reusing it here as well.

each one should stay separate.

• we have the Or, And to mix the predicate and we aren't clubbing both

[nb-ohad]

the fix is to change the predicate, the older predicate function is updated in client-op and we reusing it here as well.

Maybe I am misunderstanding the problem or the change. I can see the move from the coupled predicate to the simpler decoupled predicate (name predicate and event type predicate) What I don't understand is the need for an AND(OR(AND, AND))) structure. This does not seem reasonable to me

Can you please describe your filtering logic in words first (maybe in the description), that will help in deciphering the change and reviewing the code

[nb-ohad]

Looking at the code again. You are using the tools the wrong way which complicates things.

Your condition is logically 2 distinct conditions:

• filter for a fixed list of CRD names
• filter events for creation only if CRD is does not exists or it is a delete event

This boils down to filtering first on names, then filtering on event types:

predicate.Or(
    util.NamePredicate(crd1Name),
    util.NamePredicaet(crd2Name),
),
util.EventTypePredicate(
    !crd1Available || !crd2Available2,
    false,
    true,
    false,
)

[leelavg]

Can you please describe your filtering logic in words first

• I mentioned the "why" in pr description and "what" is being done in the code comments.

Your condition is logically 2 distinct conditions:
filter for a fixed list of CRD names
filter events for creation only if CRD is does not exists or it is a delete event

• I don't think so, the event (create/delete) for a crd (name) is one distinct set. With the suggestion, I need the change for addition or deletion on watching new CRD in two places

or it is a delete event

• not for all delete events, if the required crd didn't exist when we started the controller we don't want a reconcile if a delete event is trigerred on that

[nb-ohad]

I don't think so, the event (create/delete) for a crd (name) is one distinct set. With the suggestion, I need the change for addition or deletion on watching new CRD in two places

No it doesn't, the queued event does not care

not for all delete events, if the required crd didn't exist when we started the controller we don't want a reconcile if a delete event is trigerred on that

This is why we have the named filter to only trigger for named CRDs. Add to that the fact that you cannot get a DELETE event for a CR that does not exists from the start

[leelavg]

This boils down to filtering first on names, then filtering on event types:

• took the suggestion based on an offline discussion, thanks.

[nb-ohad]

/lgtm

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: leelavg, nb-ohad, rewantsoni

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 [nb-ohad]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[leelavg]

/unhold
based on #2911 (comment)

[openshift-cherrypick-robot]

@openshift-ci-robot: new pull request created: #2922

In response to this:

@leelavg: The following backport issues have been created:

Queuing cherrypicks to the requested branches to be created after this PR merges:
/cherrypick release-4.18

In response to this:

/jira backport release-4.18

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 openshift-eng/jira-lifecycle-plugin 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-sigs/prow repository.