OCPBUGS-41892: Single Watch on GWAPI CRD #1165
Conversation
openshift-ci
bot
added
the
do-not-merge/work-in-progress
openshift-ci-robot
added
jira/severity-moderate
|
@Thealisyed: This pull request references Jira Issue OCPBUGS-41892, which is invalid:
Comment 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 openshift-eng/jira-lifecycle-plugin repository. |
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@Thealisyed: This pull request references Jira Issue OCPBUGS-41892, which is valid. The bug has been moved to the POST state. 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 openshift-eng/jira-lifecycle-plugin repository. |
|
/test e2e-aws-ovn-single-node |
|
/retest-required |
Thealisyed
changed the title
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
There was a problem hiding this comment.
(Whoops, I've had this review pending, waiting for me to submit it, for 5 days...)
Is it true that the existing watch only generates reconciliation requests when the Gateway API feature is not enabled?
In any case, please add a little more detail in the commit message. For example:
gatewayapi: Use single watch for CRDs
Replace the multiple watches for Gateway API CRDs with a single CRD watch, with a predicate to filter CRDs based on the gatewayapiv1.GroupName. This ensures that only relevant CRDs are watched, reducing unnecessary reconciliation requests and log messages.
This commit fixes OCPBUGS-41892.
https://issues.redhat.com/browse/OCPBUGS-41892
Miciah
added
the
priority/backlog
|
/assign |
|
@Thealisyed please investigate the failures in the CI tests, especially e2e-aws-gatewayapi, which is the GWAPI test. I ran a retest to see if the issue with imagestreams had gone away. |
|
/retest |
| return crd.Spec.Group == gatewayapiv1.GroupName | ||
| }) | ||
|
|
||
| if err = c.Watch(source.Kind[client.Object](operatorCache, &apiextensionsv1.CustomResourceDefinition{}, handler.EnqueueRequestsFromMapFunc(toFeatureGate), crdPredicate)); err != nil { |
There was a problem hiding this comment.
I'm not sure why we need to use handler.EnqueueRequestsFromMapFunc(toFeatureGate) here. Can you add a comment as to why it is used instead of handler.EnqueueRequestForObject{}? I suspect it may have been a mistake that we should remove.
There was a problem hiding this comment.
My understanding is that handler.EnqueueRequestsFromMapFunc(toFeatureGate) ensures that any changes to the watched CRDs trigger a reconcile request specifically for the feature gate, maintaining its correct state and configuration. This targeted reconciliation is crucial for feature gate-specific logic.
On the other hand, handler.EnqueueRequestForObject{} would enqueue the object itself for reconciliation but may not ensure the correct handling of feature gate-specific logic.
There was a problem hiding this comment.
Can you clarify:
trigger a reconcile request specifically for the feature gate, maintaining its correct state and configuration.
We don't reconcile feature gate changes and the only change that can happen is for a feature to move from disabled to enabled. We do reconcile Gateway API crds: https://github.com/openshift/cluster-ingress-operator/blob/master/pkg/operator/controller/gatewayapi/controller.go#L106
There was a problem hiding this comment.
Thank you for the clarification.
I understand now that we don't reconcile feature gate changes and the only change that can happen is for a feature to move from disabled to enabled.
Given this, I will update it to use handler.EnqueueRequestForObject{} accordingly then!
…atch with a predicate to filter CRDs based on the gatewayapiv1.GroupName.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@Thealisyed: The following tests failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
A single CRD watch was added with a predicate to filter CRDs based on the gatewayapiv1.GroupName. This ensures that only relevant CRDs are watched, reducing unnecessary logging when the Gateway API feature is not enabled.