Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 don't know enough about how the plugins work in Ironic, but looking solely at the count makes me a little nervous.
Are there any default or builtin drivers that are always loaded, even if they are not listed in the config?
Would it make sense to compare the list of names, instead of just the count?
We've put some logic in the baremetal-operator to do a similar check, but it just waits for a non-zero count. Should that be made more sophisticated, too? That won't help the OpenShift installer, which the approach here does take care of.
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 don't think so, there is a default value for enabled_hardware_types if not mentioned in the config
I guess it would be more definite, although I'm not sure it would be worth the extra complexity
Checking for a non-zero count is not reliable as the drivers are loaded individually, resulting in
https://bugzilla.redhat.com/show_bug.cgi?id=1902653
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.
To do this the BMO would need to know what the full list of drivers is
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.
In the BMO we could look for the driver we're going to need for the host we're reconciling, so we don't need the full list. @andfasano did that work, I think, so maybe he has ideas about how to extend it to handle this case.
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.
Not sure if there's a better place, but we could extract the driver name from the
BareMetalHost.Spec.BMC.Address
of the provisioned host and then check for it in the dependencies check here https://github.com/metal3-io/baremetal-operator/blob/d6eaf6774de775d83bdd47e04ea3d171ba7d4346/pkg/provisioner/ironic/dependencies.go#L66