controllers: limit reconciliations to relevant events #2621
Conversation
openshift-ci
bot
added
the
approved
iamniting
force-pushed
the
watch
branch
5 times, most recently
from
625805f
to
31fb9a2
Compare
iamniting
requested review from
travisn,
umangachapagain,
nb-ohad,
Madhu-1 and
leelavg
iamniting
force-pushed
the
watch
branch
2 times, most recently
from
626a6a5
to
3bf381c
Compare
| Owns(&cephv1.CephObjectStore{}, builder.WithPredicates(predicate.GenerationChangedPredicate{})). | ||
| Owns(&cephv1.CephObjectStoreUser{}, builder.WithPredicates(predicate.GenerationChangedPredicate{})). | ||
| Owns(&cephv1.CephRBDMirror{}, builder.WithPredicates(predicate.GenerationChangedPredicate{})). | ||
| Owns(&corev1.PersistentVolumeClaim{}, builder.WithPredicates(predicate.GenerationChangedPredicate{}, pvcPredicate)). |
There was a problem hiding this comment.
@Madhu-1 @umangachapagain Do we really need the owns for PersistentVolumeClaim?
There was a problem hiding this comment.
i think this can be removed as well as we don't have any PVC Controller as of now. we removed it long back but we need to see if any other functionality depends on this one before removing it.
|
/test images |
|
/test ocs-operator-bundle-e2e-aws |
|
@parth-gr: changing LGTM is restricted to collaborators 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-sigs/prow repository. |
|
/hold |
openshift-ci
bot
added
the
do-not-merge/hold
|
Small suggestion, can we only fix the known entities that are queuing reconciles rather than including that with a blanket change? Rationale is, in current context, it helps a lot fixing the main problem and only debug side effects directly related to that (if we hit any)? |
|
/test ocs-operator-bundle-e2e-aws |
|
not able to find the log in successful run to verify it's working or not, based on a failed run I still see reconciles getting logged every minute in the last ~15/119 events (basically once stabilized I guess, from 3:52 in below log) |
They are every minute due to the cephcluster being updated its capacity field every minute. Earlier they were very frequent. |
Nevertheless, if what I'm asking makes less sense, I could standby for others to approve PR. Maybe the fix for bug is reduction of reconciles + an agreed/consistent logging guidelines. |
|
@iamniting Are we sure the path for an array item looks like this? |
Noobaa and CephCluster CRs' status.conditions[].lastHeartbeatTime and status.conditions[].lastTransitionTime etc change frequently, causing unnecessary StorageCluster reconciliations. Implement logic to filter out these frequent, non-essential changes and trigger reconciliations only on relevant events. Also add the owns for other objects as noobaa and cephcluster will trigger the recociles only for the relevant events. Previously, even though these objects were not owned by the controller, reconciliations were triggered due to status updates of the NooBaa and CephCluster CRs. Now the other objects should send thier own recocile requests because noobaa and ceph clkuster wont trigger the unnecessary reconciles and these objects can not rely on noobaa and ceph cluster. Signed-off-by: Nitin Goyal <nigoyal@redhat.com> Co-authored-by: Malay Kumar Parida <mparida@redhat.com>
I have tested it, It works like this only. |
|
/unhold |
openshift-ci
bot
removed
the
do-not-merge/hold
|
LGTM - This seems good from my side, not merging to allow others to comment. |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: iamniting, nb-ohad 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 |
|
/cherry-pick release-4.16 |
|
@iamniting: once the present PR merges, I will cherry-pick it on top of release-4.16 in a new PR and assign it to you. 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-sigs/prow repository. |
|
/cherry-pick release-4.15 |
|
@iamniting: once the present PR merges, I will cherry-pick it on top of release-4.15 in a new PR and assign it to you. 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-sigs/prow repository. |
|
/cherry-pick release-4.14 |
|
@iamniting: once the present PR merges, I will cherry-pick it on top of release-4.14 in a new PR and assign it to you. 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-sigs/prow repository. |
openshift-merge-bot
bot
merged commit 839beb1
into
red-hat-storage:main
|
@iamniting: #2621 failed to apply on top of branch "release-4.16": 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-sigs/prow repository. |
|
@iamniting: #2621 failed to apply on top of branch "release-4.15": 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-sigs/prow repository. |
|
@iamniting: #2621 failed to apply on top of branch "release-4.14": 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-sigs/prow repository. |
Noobaa and CephCluster CRs' status.conditions[].lastHeartbeatTime and status.conditions[].lastTransitionTime etc change frequently, causing unnecessary StorageCluster reconciliations. Implement logic to filter out these frequent, non-essential changes and trigger reconciliations only on relevant events.
Also add the owns for other objects as noobaa and cephcluster will trigger the recociles only for the relevant events. Previously, even though these objects were not owned by the controller, reconciliations were triggered due to status updates of the NooBaa and CephCluster CRs. Now the other objects should send thier own recocile requests because noobaa and ceph clkuster wont trigger the unnecessary reconciles and these objects can not rely on noobaa and ceph cluster.
Signed-off-by: Nitin Goyal nigoyal@redhat.com
Co-authored-by: Malay Kumar Parida mparida@redhat.com
Bug: https://bugzilla.redhat.com/show_bug.cgi?id=2280595
We are required to backport this till 4.14