Skip to content

Filter out status updates from the reconcile loop#747

Merged
metal3-io-bot merged 1 commit intometal3-io:masterfrom
andfasano:fix-status-updates
Dec 11, 2020
Merged

Filter out status updates from the reconcile loop#747
metal3-io-bot merged 1 commit intometal3-io:masterfrom
andfasano:fix-status-updates

Conversation

@andfasano
Copy link
Copy Markdown
Member

@andfasano andfasano commented Dec 9, 2020

Everytime the BMH status is saved the LastUpdated field is updated with the current time, and a new update is generated if the marshalled string results different from the previous version (up to the seconds), producing more reconcile loops than needed.
This PR filters out all the updates on the Status by ignoring all those ones that do not increase the Generation field (except for the finalizers and annotations) [1]

[1] https://www.openshift.com/blog/kubernetes-operators-best-practices

@metal3-io-bot metal3-io-bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Dec 9, 2020
@metal3-io-bot
Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andfasano, dhellmann

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:

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

@metal3-io-bot metal3-io-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 9, 2020
@dhellmann
Copy link
Copy Markdown
Member

/test-integration

Copy link
Copy Markdown
Member

@zaneb zaneb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great that you found that doc! I've been wondering about how this was supposed to work since forever.
In BMO I think we are pretty good at always returning Requeue=true when we want to requeue, instead of relying on the status write to trigger it (like some other controllers, e.g. CAPBM, do), so this ought to work. One comment inline.

Comment thread controllers/metal3.io/baremetalhost_controller.go Outdated
@metal3-io-bot metal3-io-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Dec 11, 2020
@andfasano
Copy link
Copy Markdown
Member Author

/test-integration

@zaneb
Copy link
Copy Markdown
Member

zaneb commented Dec 11, 2020

I checked all of the places that Reconcile() returns, and confirmed that the only ones where we're not explicitly requesting a requeue are where we really don't want it.
/lgtm
/test-integration

@metal3-io-bot metal3-io-bot added the lgtm Indicates that a PR is ready to be merged. label Dec 11, 2020
@metal3-io-bot metal3-io-bot merged commit 0fde063 into metal3-io:master Dec 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants