baremetal: Enable Ironic on the bootstrap VM #2079
Conversation
openshift-ci-robot
added
the
do-not-merge/work-in-progress
openshift-ci-robot
added
the
size/L
|
Note - I'm not sure if the increase to 6G is still needed, I added it when I was having issues with libpod (since resolved via #1941) - I need to re-test with the default memory and prove everything still works, if so I'll drop that patch from the series. |
|
/label platform/baremetal |
Note that the metal3 CI results on this PR won't be valid, because we need corresponding dev-scripts changes, I'll test it via openshift-metal3/dev-scripts#642 when it's fully working locally (still debugging some issues atm). |
data/data/bootstrap/baremetal/files/usr/local/bin/startironic.sh.template
Outdated
Show resolved
Hide resolved
data/data/bootstrap/baremetal/files/usr/local/bin/startironic.sh.template
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
We're not supporting survey input right now, but support via install-config makes sense. If not specified in install-config, we'd just use the default (and not any survey input)
There was a problem hiding this comment.
I removed all the FIXME comments for now, and added a reference to #2091
There was a problem hiding this comment.
I'm not clear on what the fix is here - change ironic to only bind to the provisioning interface? ensure the installer is only using that IP?
There was a problem hiding this comment.
Ah thanks for the reminder, yeah the fix is to bind only on the provisioning nic, but I couldn't do that previously because terraform was accessing ironic on the host via the baremetal bridge - I'll re-test with metal3-io/ironic-image#56 locally as I think it should work now that this series switches terraform to reference the BootstrapProvisioningIP.
There was a problem hiding this comment.
I removed the FIXME comments for now, added a link to #2091 and will follow-up wrt the bind IP question via update of the ironic image (which can only really happen after this initial integration lands I think).
|
Build FAILURE, see build http://10.8.144.11:8080/job/dev-tools/929/ |
|
Build FAILURE, see build http://10.8.144.11:8080/job/dev-tools/930/ |
data/data/bootstrap/baremetal/files/usr/local/bin/startironic.sh.template
Outdated
Show resolved
Hide resolved
data/data/bootstrap/baremetal/files/usr/local/bin/startironic.sh.template
Outdated
Show resolved
Hide resolved
data/data/bootstrap/baremetal/files/usr/local/bin/startironic.sh.template
Outdated
Show resolved
Hide resolved
data/data/bootstrap/baremetal/files/usr/local/bin/startironic.sh.template
Outdated
Show resolved
Hide resolved
data/data/bootstrap/baremetal/files/usr/local/bin/startironic.sh.template
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
looks like there's an alignment issue here
|
Build FAILURE, see build http://10.8.144.11:8080/job/dev-tools/938/ |
hardys
force-pushed
the
issue_2060
branch
3 times, most recently
from
d3c3b18 to
c3d38f7
Compare
|
Build FAILURE, see build http://10.8.144.11:8080/job/dev-tools/940/ |
|
Build FAILURE, see build http://10.8.144.11:8080/job/dev-tools/942/ |
openshift-ci-robot
added
the
needs-rebase
Discussion during code review indicates this is not required
|
Ok I addressed all comments except the validation of *ProvisioningIP, perhaps we can address that via a follow-up ref #2210 ? |
|
This is ok to merge without a BZ if we believe it has met the typical engineering review bar for such a PR. |
openshift-ci-robot
removed
the
do-not-merge/hold
pkg/types/baremetal/platform.go
Outdated
pkg/types/baremetal/platform.go
Outdated
There was a problem hiding this comment.
nit, this is used for Machine objects. in case of DR.. even master machines should be recreate able.. so I think we should drop this from the API.
There was a problem hiding this comment.
Ack yes it could also be used to rebuild master machines, will update
pkg/types/baremetal/platform.go
Outdated
There was a problem hiding this comment.
Can't have terraform in public API docs. that are used to create the control-plane machines
These were unclear so improve the clarity of the comments
|
@abhinavdahiya thanks for the prompt review, latest comments addressed |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: abhinavdahiya, hardys, russellb The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci-robot
added
the
approved
|
Build FAILURE, see build http://10.8.144.11:8080/job/dev-tools/1036/ |
|
Build FAILURE, see build http://10.8.144.11:8080/job/dev-tools/1037/ |
|
@hardys: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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. I understand the commands that are listed here. |
As mentioned in openshift#2079 we should validate these IPs as they must be on a different subnet than the machine CIDR, these IPs are used to configure a dedicated network for provisioning machines via pxe. Closes: openshift#2210
As mentioned in openshift#2079 we should validate these IPs as they must be on a different subnet than the machine CIDR, these IPs are used to configure a dedicated network for provisioning machines via pxe. Closes: openshift#2210
8 participants
This series enables Ironic on the bootstrap VM (Issue #2060), based on work first started via openshift-metal3/kni-installer#100
Since this also reworks our image handling to use the internal image references, it also completes issue #2064 and I think may complete issue #2037
Note this depends on #2061