Support RAID and BIOS configuration for master nodes of baremetal IPI deployments#5196
Conversation
|
Hi @hs0210. Thanks for your PR. I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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. |
openshift-ci
Bot
added
the
needs-ok-to-test
hs0210
changed the title
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
/ok-to-test |
openshift-ci
Bot
added
ok-to-test
|
/label platform/baremetal |
hs0210
force-pushed
the
ipi-support-raid-bios
branch
from
b0a4053 to
fd01aa2
Compare
|
This won't work without the implementation of raid and bios processing on the terraform-provider-ironic side. |
hs0210
force-pushed
the
ipi-support-raid-bios
branch
from
93ba7f3 to
b8123f9
Compare
hs0210
force-pushed
the
ipi-support-raid-bios
branch
2 times, most recently
from
d7235bf to
55aaae9
Compare
openshift-ci
Bot
added
the
needs-rebase
hs0210
force-pushed
the
ipi-support-raid-bios
branch
from
55aaae9 to
ad7032c
Compare
openshift-ci
Bot
removed
the
needs-rebase
hs0210
force-pushed
the
ipi-support-raid-bios
branch
from
ad7032c to
e679664
Compare
There was a problem hiding this comment.
I'm not sure this is the standard way directly using asset.File in here. Because that brings about Marshalling&Unmarshalling like below.
There was a problem hiding this comment.
Thanks for your review, I understand your concerns. I'm not sure if this is a standard way either, but I saw cases where Unmarshalling is used to process asset.File data. And currently the configuration of RAID and BISO can only be obtained from mastersAsset.HostFiles. It seems that using Marshalling&Unmarshalling is a relatively straightforward way.
There was a problem hiding this comment.
Unmarshalling the asset file is appropriate.
There was a problem hiding this comment.
I would take it a step further and suggest that you should be getting all of the configuration for the host from the asset file instead of from the install config. The install config is used as input into the asset file. But after the masters asset is created, the host files become the source of truth. The user is permitted to modify the host files on-disk, and the installer should respect those changes.
There was a problem hiding this comment.
The user is permitted to modify the host files on-disk, and the installer should respect those changes.
@staebler Thanks for your review. This is indeed reasonable. Considering that this is to modify the existing implementation, I will open a new PR to do what you suggested in the future.
There was a problem hiding this comment.
@staebler We hope that this feature can be merged into OCP 4.10, could we implement the improvements you mentioned later?
There was a problem hiding this comment.
Yes, I did not mean to suggest that any changes needed to be made to this PR. I was only supporting the notion that reading the asset files is acceptable, and in many ways recommended.
hs0210
changed the title
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
openshift-ci
Bot
added
the
do-not-merge/hold
hs0210
force-pushed
the
ipi-support-raid-bios
branch
2 times, most recently
from
1e92c35 to
3f99aec
Compare
| "name": host.Name, | ||
| "port_address": host.BootMACAddress, | ||
| "bmc_address": host.BMC.Address, | ||
| "bmc_disable_certificate_verification": host.BMC.DisableCertificateVerification, |
There was a problem hiding this comment.
The bmc address and flag to disable certificate validation already exists in the driver_info - see here:
So I don't think we should add these new terraform APIs, the provider should just consume the existing driver_info data?
There was a problem hiding this comment.
It may be that DisableCertificateVerification can be derived from driver_info, but Address cannot. Please see the detailed statement in PR53's comments.
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
1 similar comment
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/skip |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
6 similar comments
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/test ci/prow/e2e-metal-ipi-ovn-ipv6 |
|
@rhjanders: The specified target(s) for
The following commands are available to trigger optional jobs:
Use
DetailsIn response to this:
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. |
|
/test e2e-metal-ipi-ovn-ipv6 |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
7 similar comments
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/test ci/prow/e2e-metal-ipi-ovn-ipv6 |
|
@rhjanders: The specified target(s) for
The following commands are available to trigger optional jobs:
Use
DetailsIn response to this:
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. |
|
/test e2e-metal-ipi-ovn-ipv6 |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
@hs0210: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. I understand the commands that are listed here. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
1 similar comment
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
Support RAID and BIOS configuration for master nodes of baremetal IPI deployments
15 participants
This PR adds the support of RAID and BIOS configuration for master nodes of baremetal IPI deployments.
Based on:
Signed-off-by: Hu Shuai hus.fnst@cn.fujitsu.com