Closed hs0210 closed 2 years ago
@dtantsur @zaneb PTAL - my impression here is that it would be better to avoid the large vendor dependency on BMO - it also couples the terraform provider to some internal BMO APIs.
Would it perhaps be better to refactor the needed code into a new package/module, or would it be better to just reimplement a minimal copy of the same logic here?
I can definitely see some value in making the bmc package its own go module. This would be helpful for the webhook as well.
If the ironic RAID support is going to be shared then I agree it at least belongs in its own package, and maybe its own go module to minimise dependencies.
@ardaguclu what do you think?
When I looked at the new dependencies come along with this change;
baremetalhost "github.com/metal3-io/baremetal-operator/apis/metal3.io/v1alpha1"
"github.com/metal3-io/baremetal-operator/pkg/bmc"
"github.com/metal3-io/baremetal-operator/pkg/provisioner/ironic"
baremetalhost "github.com/metal3-io/baremetal-operator/apis/metal3.io/v1alpha1"
has it's own go module already.
I totally agree with you, these two packages can be in a separate go module. I think, contextually it also serves as ironic wrapper.
"github.com/metal3-io/baremetal-operator/pkg/bmc"
"github.com/metal3-io/baremetal-operator/pkg/provisioner/ironic"
Honestly, I've been long dreaming of creating something like ironic-go, which would be a more or less metal3-agnostic ironic module. Time to do it?
UPD: I'm definitely against copy-pasting logic, even as a temporary measure.
Honestly, I've been long dreaming of creating something like ironic-go, which would be a more or less metal3-agnostic ironic module. Time to do it?
You know better than me how much BMO specific logic spread in these packages. But if it is possible, it sounds like a good idea.
Honestly, I've been long dreaming of creating something like ironic-go, which would be a more or less metal3-agnostic ironic module. Time to do it?
@dtantsur how would this differ from the ironic code that exists in gophercloud? I get that gophercloud is mostly a pretty minimal go binding to the API, but I'm wondering if such metal3-agnostic logic could live there, and if not - how do we reason about what goes into ironic-go vs gophercloud?
I've found the gophercloud maintainer to be pretty responsive, is it worth having some discussion before creating a net-new module/repo?
I checked how viable to decouple bmc and ironic packages into new go module or repository;
provisioner
package and if we decouple it, that creates circular dependency. First thing might be cleaning provisioner
package usage in here or some code who need provisioner
like https://github.com/metal3-io/baremetal-operator/blob/51384b4efbb4b10454a2343f981306ef9ecaf2a4/pkg/provisioner/ironic/raid.go#L22 stay in here and rest of code is moved to new go.mod or repository.As far as I can see in this PR, https://github.com/metal3-io/baremetal-operator/tree/master/pkg/provisioner/ironic is also used, that means bmc and ironic should be decoupled from main go module.
My current concerns are: 1: Since it takes a lot of time to create ironic-go, is it possible to target only the following 2 packages that depend on this PR first?
"github.com/metal3-io/baremetal-operator/pkg/bmc"
"github.com/metal3-io/baremetal-operator/pkg/provisioner/ironic"
2: In this case, how do we reason about what goes into ironic-go vs gophercloud? (This is @hardys 's queston)
Is there some reason that the size of the (unused) dependencies is actually an issue? It's unlikely to cause conflicts with anything else in this module.
I think the issue is reusing internal APIs. IMHO we should revert metal3-io/baremetal-operator#989 and do a more minimal change like moving the RAID code to a separate package.
LGTM, but maybe someone also might want to look at raid changes.
/retest
/retest
The test failure looks real
github.com/metal3-io/baremetal-operator/pkg/provisioner/ironic/clients
../../../../pkg/mod/github.com/metal3-io/baremetal-operator@v0.0.0-20211203102512-3572353e42e5/pkg/provisioner/ironic/clients/auth.go:38:18: undefined: os.ReadFile
note: module requires Go 1.16
Looks like we need to bump the go version in the CI environment
This now needs a rebase since the go bump PR landed - thanks for fixing that! :)
@hardys Thanks! The CI has passed, PTAL. :)
I think, we can merge this PR and start testing in installer PR.
/lgtm
Hi, we are corresponding to https://github.com/metal3-io/baremetal-operator/pull/1063#issuecomment-1034407549 and IMO this will not block this patch's review.
It seems like we ideally need e.g bmc.AccessDetailsFromDriverInfo
IMHO, the information provided by DriverInfo(as well as other methods) is not sufficient to instantiate AccessDetails, unless we modify the existing DriverInfo method.
Considering that the installer has instantiated accessDetails, it might be better to call BuildBIOSSettings directly on the installer side to process the BIOS.
Please continue to review this part. Thanks!
/retest
@rhjanders: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test
message.
/ok-to-test
/retest
@rhjanders: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test
message.
@rhjanders: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test
message.
/ok-to-test
@rhjanders: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test
message.
When I tried to update the PRs to test the features, the installer fail to compile due to some conflicts caused by different versions of gophercloud in BMO and installer.
BMO has been using gophercloud:v0.22.0; but installer is still using v0.17.0. So I tried to update gophercloud vendored by installer to the latest. After resolving some dependency conflicts, the installer compiled successfully, but got a panic during IPI. I sent issue https://github.com/openshift/installer/issues/5653, PTAL, thanks!
/ok-to-test
The CI failed by issue https://github.com/openshift-metal3/terraform-provider-ironic/issues/63, and I have sent PR https://github.com/openshift-metal3/terraform-provider-ironic/pull/64 to fix it. PTAL.
Then dependencies problem mentioned above has been resolved after https://github.com/openshift/installer/pull/5507 and I have closed the issue.
So could we continue the review please? Is my idea here acceptable? PTAL, thanks!
@zaneb @hardys @ardaguclu Hi, @dtantsur has helped sent https://github.com/metal3-io/baremetal-operator/pull/1094 to expose func CheckRAIDInterface
which should be merged soon, so this PR is not blocked by https://github.com/metal3-io/baremetal-operator/pull/1063 any more.
Could we continue the review? Let's move on to the discussion where is the right place to process the BIOS. Thanks very much!
Thanks to the merge of https://github.com/metal3-io/baremetal-operator/pull/1094, I have updated this PR accordingly. Everything about this PR is up to date now, please continue to review.
@dtantsur About processing the BIOS on installer side, could you help take a look? Can we handle it this way?
/approve
I think this is good to go. Thank you!
[APPROVALNOTIFIER] This PR is APPROVED
This pull-request has been approved by: dtantsur
The full list of commands accepted by this bot can be found here.
The pull request process is described here
/lgtm
/lgtm
This PR adds corresponding processing to convert RAID and BIOS configuration into clean steps to make baremetal IPI deployments support RAID and BIOS configuration for master nodes.
Based on: