🌱Add test for baremetalhost controller updateEventHandler

[troy0820]

What this PR does / why we need it:

When using this method, the saveHostStatus 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

[metal3-io-bot]

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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

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.

[tuminoid]

/ok-to-test

[troy0820]

/test metal3-bmo-e2e-test

[metal3-io-bot]

@troy0820: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

• /test generate
• /test gomod
• /test manifestlint
• /test markdownlint
• /test shellcheck
• /test test

The following commands are available to trigger optional jobs:

• /test metal3-centos-e2e-basic-test-main
• /test metal3-centos-e2e-feature-test-main-features
• /test metal3-centos-e2e-feature-test-main-pivoting
• /test metal3-centos-e2e-feature-test-main-remediation
• /test metal3-centos-e2e-integration-test-main
• /test metal3-dev-env-integration-test-centos-main
• /test metal3-dev-env-integration-test-ubuntu-main
• /test metal3-e2e-1-29-1-30-upgrade-test-main
• /test metal3-e2e-clusterctl-upgrade-test-main
• /test metal3-ubuntu-e2e-basic-test-main
• /test metal3-ubuntu-e2e-feature-test-main-features
• /test metal3-ubuntu-e2e-feature-test-main-pivoting
• /test metal3-ubuntu-e2e-feature-test-main-remediation
• /test metal3-ubuntu-e2e-integration-test-main

Use /test all to run the following jobs that were automatically triggered:

• generate
• gomod
• manifestlint

Details

In response to this:

/test metal3-bmo-e2e-test

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.

[troy0820]

/retest

Not sure why the tests are not re-running, but I can push an empty commit.

[tuminoid]

Thanks for the contribution! Let's squash those commits up.

Let's get some eyes on this.
/cc @kashifest @Rozzii @dtantsur @elfosardo

[kashifest]

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?

[dtantsur]

/cc @zaneb

[troy0820]

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?

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.

[zaneb]

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.)

[zaneb]

/cc @andfasano, who appears to me to have already solved this exact issue in 2020 with #747.

[troy0820]

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.

[zaneb]

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.
There was no suggestion that BMO itself was doing too many reconciles after such an update. Extra reconciles for that and a much broader superset of cases were already eliminated by #747.

I'm fine with adding a test though. We could even test that all updates to the Status are ignored.

[Rozzii]

/lgtm

[tuminoid]

/cc @kashifest

[tuminoid]

/cc @kashifest @zaneb @dtantsur @elfosardo
Can we get an approver for this?

[kashifest]

/approve

[metal3-io-bot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kashifest

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [kashifest]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment