-
Notifications
You must be signed in to change notification settings - Fork 247
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
🌱Add test for baremetalhost controller updateEventHandler #1881
base: main
Are you sure you want to change the base?
🌱Add test for baremetalhost controller updateEventHandler #1881
Conversation
[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 |
Hi @troy0820. Thanks for your PR. I'm waiting for a metal3-io member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
8ae68ff
to
1e33d18
Compare
7fe7bfe
to
1e5696c
Compare
/ok-to-test |
/test metal3-bmo-e2e-test |
@troy0820: The specified target(s) for
The following commands are available to trigger optional jobs:
Use
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. |
/retest Not sure why the tests are not re-running, but I can push an empty commit. |
Thanks for the contribution! Let's squash those commits up. Let's get some eyes on this. |
15cc788
to
662161e
Compare
I am trying to understand this patch a bit, the original issue pointed by @s3rj1k was an edge case where code updated the status/BMH object even when there was no actual change in status, but it seems this patch is ignoring updating the object even when theres is any update on the status? Am I understanding it correctly? |
/cc @zaneb |
The object is updated but it doesn’t requeue the object in the event that the status last changed gets updated during a saveStatus. This was supposed to be handled by the predicate generation changed but seems to slip through. |
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'm not sure this has any effect.
It's probably safe as long as we always remember to return Requeue:true whenever we need to requeue.
I think we do, but I also thought that we avoid writing unchanged statuses (as Dmitry pointed out), yet according the the bug report we do not. (OTOH, I'm not sure whether to believe this, because when asked where this happens the response was "check the code", and yet upon checking the code it's clearly fine.)
/cc @andfasano, who appears to me to have already solved this exact issue in 2020 with #747. |
Running the test that I added as part of this change works without the patch included. So I can dismiss this change and leave the test but we probably should close the issue if it is fixed. |
662161e
to
611dffd
Compare
Signed-off-by: Troy Connor <[email protected]>
611dffd
to
2373f90
Compare
Reading over the bug report, the complaint was that we were doing no-op updates in the first place (although there is no evidence of this yet), thus causing extra work for other controllers that were watching the BMH. I'm fine with adding a test though. We could even test that all updates to the Status are ignored. |
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
/cc @kashifest |
What this PR does / why we need it:
When using this method, thesaveHostStatus
will requeue the object and create more reconciles for the object when the only thing that has changed is the lastupdated field in the status. This leads to multiple reconciles because this happens during each run through the reconciler.This adds a test to the baremetalhost controller to validate that the saveHostStatus will not requeue the object and create more reconciles when lastUpdated is updated.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Fixes #1253