Change hfs controller to be level-triggered when handling bmh updates

[bfournie]

Change to use a poll cycle in the hfs reconciler to get data from Ironic instead of relying on state difference. In addition, the following changes were made to address follow-on comments in
#938.

• changed name of condition UpdateRequested to FirmwareSettingsChangeDetected
• add each hostFirmwareSettings as owner of its FirmwareSchema so that garbage collection will only be done when all removed
• Change the SetOwnerReference/SetControllerReference calls to before Create so the Update can be removed
• When building BIOS settings for cleaning make sure the hfs settings don't overwrite the vendor-agnostic settings
• When generating the hash for the firmwareSchema include the schema values in addition to the names

[bfournie]

/test-integration

[elfosardo]

/lgtm

[metal3-io-bot]

@elfosardo: adding LGTM is restricted to approvers and reviewers in OWNERS files.

Details

In response to this:

/lgtm

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/test-infra repository.

[bfournie]

/cc @zaneb
/cc @andfasano

[metal3-io-bot]

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

• /test generate
• /test gofmt
• /test golint
• /test gomod
• /test gosec
• /test manifestlint
• /test markdownlint
• /test shellcheck
• /test unit

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

• generate
• gofmt
• golint
• gomod
• gosec
• manifestlint
• unit

Details

In response to this:

/test e2e-metal-ipi
/test e2e-metal-ipi-ovn-ipv6

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/test-infra repository.

[andfasano]

After reviewing all the changes, on a second thought I'm getting convinced that probably the relationships between BMH and HFS could be relaxed a little bit, possibly leading to a simplified approach that will honor all the points previously discussed. Following the main general points:

1. BMH needs only to wait, in the Preparing state, for the related HFS resource to be ready (and here using the conditions looks fine). If the HFS is not ready, it will re-enqueue a reconciliation request before proceeding. If it is, the cleaning steps could be taken as usual.
2. HFS controller will stop listening BMH events, and it should take care only of filling (and validating) the HFS resource by fetching the hw settings at every reconcile loop, ensuring that its Conditions properly reflect the current situation. The reconciliation will be by default reenqueued on a reasonable time basis (ie 5 minutes, to be decided), to ensure that any external changes to the hw settings will be captured (for example, if any setting will be changed after the cleaning step in the Preparing state).
3. Given that, the responsibility to create an (empty) HFS resource if not present will come back under the BMH controller (something to be performed at every reconcile loop, if not under deletion)

I understand that this means a significant change to the current design, but the overall idea is to look for a simplified approach that maybe could be reused also in future for other similar situations.

Wdyt?

[bfournie]

After reviewing all the changes, on a second thought I'm getting convinced that probably the relationships between BMH and HFS could be relaxed a little bit, possibly leading to a simplified approach that will honor all the points previously discussed. Following the main general points:

1. BMH needs only to wait, in the Preparing state, for the related HFS resource to be ready (and here using the conditions looks fine). If the HFS is not ready, it will re-enqueue a reconciliation request before proceeding. If it is, the cleaning steps could be taken as usual.
2. HFS controller will stop listening BMH events, and it should take care only of filling (and validating) the HFS resource by fetching the hw settings at every reconcile loop, ensuring that its Conditions properly reflect the current situation. The reconciliation will be by default reenqueued on a reasonable time basis (ie 5 minutes, to be decided), to ensure that any external changes to the hw settings will be captured (for example, if any setting will be changed after the cleaning step in the Preparing state).
3. Given that, the responsibility to create an (empty) HFS resource if not present will come back under the BMH controller (something to be performed at every reconcile loop, if not under deletion)

I understand that this means a significant change to the current design, but the overall idea is to look for a simplified approach that maybe could be reused also in future for other similar situations.

Wdyt?

I think its a good idea and will simplify the hfs controller. I've updated the PR to:

1. Remove the watch of bmh resource in hfs controller
2. Remove the hfs EventUpdate handler filtering
3. Create the hfs resource in bmh controller when registering
4. Add a requeue to the hfs reconciler to poll the data from Ironic and pick up any changes as a result of cleaning (or other events)
5. In bmh controller, before entering the preparing state check that the hfs is ready

[bfournie]

/test-v1b1-centos-integration

[bfournie]

/test-v1b1-integration

[andfasano]

/test-v1b1-integration

[andfasano]

Thanks @bfournie for the patch, the code looks simpler and the responsibilities across the controllers well assigned, addressing the points previously discussed.

/approve

[bfournie]

/test-v1b1-centos-integration
/test-v1b1-integration

[bfournie]

/test-v1b1-centos-integration

[bfournie]

/retest

[bfournie]

/test-v1b1-centos-integration
/test-v1b1-integration

[bfournie]

/test-v1b1-centos-integration
/test-v1b1-integration

[bfournie]

/test-v1b1-centos-integration
/test-v1b1-integration

[metal3-io-bot]

[APPROVALNOTIFIER] This PR is APPROVED

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

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 [andfasano]

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

[dtantsur]

/lgtm