Eliminate need for "empty" provisioner#820
Conversation
metal3-io-bot
added
approved
|
/test-integration |
zaneb
force-pushed
the
empty-provisioner
branch
from
02023d3 to
174623b
Compare
metal3-io-bot
added
size/L
zaneb
changed the title
|
/test-integration |
andfasano
left a comment
andfasano
left a comment
There was a problem hiding this comment.
I like the approach, and I'd suggest to transform p.bmAccess() in a lazy getter to avoid modifying some Provisioner signatures (and still keeping a max single instantiation per loop as before)
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: andfasano, zaneb The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
The only real effect of that would be that you have to copy and paste the error handing to a lot of extra places. |
|
This lgtm but now needs a rebase |
Right, I meant IronicProvisioner private methods that have been changed to accept the bmcCredentials parameters - which make the relationship pretty loosely coupled (surely not a blocking a point, but my preference would be to avoid it). On a second thought, the current implementation fullfils the requirement expressed in #824 without triggering any error just thanks to the fact that the BMC credentials check have been postponed later into the |
zaneb
force-pushed
the
empty-provisioner
branch
from
174623b to
41d8966
Compare
|
/test-integration |
I did notice there's no test coverage of the With this refactor I guess it's a good time to add such coverage - I'm not blocking on it, but it could be added either in this PR or a followup. @andfasano what are your thoughts? |
I think either options would be fine. baremetal_controller_host_test and/or validatemanagementaccess_test/prepare_test could be good candidates for capturing that. |
The empty provisioner is a workaround for the fact that the ironic provisioner cannot handle instantiation when there are no BMC details available. The change cadeb28 that added it also had the effect of logging provisioner information on every reconcile when it was intended to be logged only once, at startup. Make it the ironic provisioner factory's responsibility to return an empty Provisioner at runtime when it cannot create an ironic Provisioner, and do only one-time startup configuration in main.go. Fixes metal3-io#819
Not every provisioner operation requires BMC access details, so only create them when needed. This allows us to reach the unmanaged state using the ironic provisioner when there are no BMC details specified. This also means that if the BMC details cannot be parsed correctly, rather than an error that causes constant requeues and is recorded only in the logs, we will record a registration failure that will be visible in the UI.
Now that the ironic provisioner only requires BMC details when registering (which does not occur in the unmanaged state), remove the empty provisioner workaround.
zaneb
force-pushed
the
empty-provisioner
branch
from
41d8966 to
da3bd7e
Compare
|
Now with more unit tests. Thanks for the suggestion @andfasano, in retrospect that was definitely required. |
|
/lgtm |
4 participants
Lazily create the BMCAccess information inside the ironic provisioner when needed for registration, rather than in the factory when creating a Provisioner. This eliminates the need for the empty provisioner workaround introduced in #748 to allow us to update the Host state to Unmanaged without creation of the provisioner failing.
Fixes #819