-
Notifications
You must be signed in to change notification settings - Fork 44
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
fix: remediator ignores no resource/kind match #1348
fix: remediator ignores no resource/kind match #1348
Conversation
if err != nil { | ||
nt.T.Fatal(err) | ||
} | ||
Conflicts: 0, |
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.
The intent of the check here is to confirm the remediator detects the drift, but couldn't correct it.
Instead of checking no conflicts, what about adding a new metric for the remediator error, and validating a remediator error is recorded in the metrics? We can still check no conflicts, but it is just no enough.
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.
I added handling of NoMatchErrors as conflicts using the existing metric. This should make it report the error metric consistently in this test.
If we want to add a new metric, it'll need a one-pager, because we also need to separate management conflicts from resource conflict, and list all the cases to make sure they're handled correctly. The management conflict reporting is spotty at best today. It only works for RootSyncs and only sometimes. I don't really have time to do this now, due to the watch filter project.
7d18566
to
44ee0ad
Compare
- Update remediator to record NoMatchErrors as a resource conflict. These are reported as metrics and cause retry, but are still not reported as sync errors, for now. - Update comments in TestCRDDeleteBeforeRemoveCustomResourceV1 to explain why a conflict is expected and which ones might be encountered due to race condition.
44ee0ad
to
33376bf
Compare
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.
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: sdowell 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 |
6299a2e
into
GoogleContainerTools:main
These are reported as metrics and cause retry, but are still not
reported as sync errors, for now.
to explain why a conflict is expected and which ones might be
encountered due to race condition.