oadp-1.5: fix(controller): prevent NodeAgent restarts from ESO metadata updates #1998
Conversation
openshift-ci
bot
added
the
approved
Fixes ESO-234: NodeAgent was restarting every ~30s when External Secrets Operator managed the cloud-credentials secret. ESO's metadata-only updates were triggering unnecessary DPA reconciliations. Changes: - Updated labelHandler.Update() to skip reconciliation for Secret objects when only metadata changes (ResourceVersion, annotations, etc.) - Added comprehensive unit tests for labelHandler covering all scenarios - Maintains backward compatibility for non-Secret resources This prevents unnecessary NodeAgent daemonset updates while preserving reconciliation for actual data or label changes. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
kaovilai
force-pushed
the
fix-ESO-234-nodeagent-restarts
branch
from
a45ae09 to
e8018df
Compare
kaovilai
changed the title
|
/cherry-pick oadp-dev |
|
@kaovilai: once the present PR merges, I will cherry-pick it on top of DetailsIn 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. |
|
/retest |
| if evt.ObjectNew.GetLabels()[oadpv1alpha1.OadpOperatorLabel] == "" || dpaname == "" { | ||
| return | ||
| } | ||
|
|
There was a problem hiding this comment.
Why not to use predicate around line 158 and do the predicate:
return !equality.Semantic.DeepEqual(oldSecret.Data, newSecret.Data) ||
!equality.Semantic.DeepEqual(oldSecret.StringData, newSecret.StringData) ||
!equality.Semantic.DeepEqual(oldSecret.Labels, newSecret.Labels)There was a problem hiding this comment.
We only want this applied to secret watcher. The change is specific to secret. Doing secret type comparison for every reconcile when its not necessarily a secret event is imo not necessary.
Now if we didn't have labelHandler added to secret watch, perhaps it would be easier to modify existing "global" predicate.
Since we already have labelHandler, I rather scope it to that handler.
There was a problem hiding this comment.
The semantic.deepequal is cool..
weshayutin
added
the
do-not-merge/hold
|
we will still fix, wes don't want accidental merge during code-freeze. reopen later and/or new pr. current change will go to oadp-dev |
|
@kaovilai: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions 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. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kaovilai, shubham-pampattiwar, weshayutin The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/lgtm |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
5 participants
Fixes ESO-234: NodeAgent was restarting every ~30s when External Secrets
Operator managed the cloud-credentials secret. ESO's metadata-only updates
were triggering unnecessary DPA reconciliations.
Changes:
when only metadata changes (ResourceVersion, annotations, etc.)
This prevents unnecessary NodeAgent daemonset updates while preserving
reconciliation for actual data or label changes.
🤖 Generated with Claude Code
Co-Authored-By: Claude [email protected]
Why the changes were made
How to test the changes made