baremetal: send full ignition to masters #4427
Conversation
|
/label platform/baremetal |
|
/hold #4413 and openshift-metal3/terraform-provider-ironic#46 need to merge first |
openshift-ci-robot
added
the
do-not-merge/hold
hardys
force-pushed
the
bm-full-ignition
branch
from
56bba15
to
71b83d0
Compare
There was a problem hiding this comment.
A couple minor comments on a first pass, not super familiar with the RawExt ignition stuff. Would be good to get MCO folks to review.
Note, you'll also need to update the dependency graph.
hardys
force-pushed
the
bm-full-ignition
branch
from
71b83d0
to
7f4beab
Compare
Ah I missed that in 4413, I'll push a follow-up, thanks! |
|
/retest |
openshift-ci-robot
removed
the
do-not-merge/hold
hardys
force-pushed
the
bm-full-ignition
branch
3 times, most recently
from
5022ed6
to
cc9b11d
Compare
This restores the work which was previously done via openshift#3276 but then reverted via openshift#3589 due to breaking users who customized the pointer ignition config in IPI deployments. A solution to that has been proposed via openshift#4413 - see openshift/enhancements#540 for more details. Note that some additional changes beyond the initial implementation were required due to the MCO now supporting multiple ignition versions, thus this depends on openshift-metal3/terraform-provider-ironic#46 Co-Authored-By: Steven Hardy <[email protected]>
hardys
force-pushed
the
bm-full-ignition
branch
from
cc9b11d
to
98dc381
Compare
| @@ -33,3 +28,18 @@ variable "instance_infos" { | |||
| type = list(map(string)) | |||
| description = "Instance information for hosts" | |||
| } | |||
|
|
|||
| variable "master_ignition_url" { | |||
There was a problem hiding this comment.
Sorry for the confusion around the naming. These didn't need to change, as you correctly stated that there is already context in the path. You can leave it as is or change it back: Your call.
There was a problem hiding this comment.
@staebler since CI passed I'm fine to leave it with the master prefix - it also makes it slightly more explicit when debugging and looking at the tfvars directly I guess
|
/test e2e-metal-ipi-virtualmedia Just want to test the disabled/virtualmedia code path since it's so fragile, but the changes here look good to me! |
|
/test e2e-metal-ipi-virtualmedia |
|
@staebler Is there anything left from your side to get this approved? We'll need you to do it :) |
| @@ -34,7 +40,7 @@ type config struct { | |||
| } | |||
|
|
|||
| // TFVars generates bare metal specific Terraform variables. | |||
| func TFVars(libvirtURI, bootstrapProvisioningIP, bootstrapOSImage, externalBridge, externalMAC, provisioningBridge, provisioningMAC string, platformHosts []*baremetal.Host, image, ironicUsername, ironicPassword string) ([]byte, error) { | |||
| func TFVars(libvirtURI, bootstrapProvisioningIP, bootstrapOSImage, externalBridge, externalMAC, provisioningBridge, provisioningMAC string, platformHosts []*baremetal.Host, image, ironicUsername, ironicPassword, ignition string) ([]byte, error) { | |||
There was a problem hiding this comment.
I was still hoping to get this changed from ignition to masterIgnition, but I won't hold up the PR for it.
There was a problem hiding this comment.
Thanks, I can push a follow-up PR to make that consistent
openshift-ci-robot
added
the
approved
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kirankt, staebler 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 |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
6 similar comments
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
@hardys: The following test failed, say
Full PR test history. Your PR dashboard. 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. |
This doesn't work for IPI baremetal deployments driven via hive, because there are firewall rules that prevent access to the bootstrap MCS from the pod running the installer. This was implemented in: openshift#4427 But we ran into problems making the same approach work for worker machines ref: openshift#4456 We're now looking at other approaches to resolve the network-config requirements driving that work, so switching back to the pointer config for masters seems reasonable, particularly given this issue discovered for hive deployments. Conflicts: pkg/tfvars/baremetal/baremetal.go This reverts commit 98dc381.
This doesn't work for IPI baremetal deployments driven via hive, because there are firewall rules that prevent access to the bootstrap MCS from the pod running the installer. This was implemented in: openshift#4427 But we ran into problems making the same approach work for worker machines ref: openshift#4456 We're now looking at other approaches to resolve the network-config requirements driving that work, so switching back to the pointer config for masters seems reasonable, particularly given this issue discovered for hive deployments. Conflicts: pkg/tfvars/baremetal/baremetal.go This reverts commit 98dc381.
This doesn't work for IPI baremetal deployments driven via hive, because there are firewall rules that prevent access to the bootstrap MCS from the pod running the installer. This was implemented in: openshift#4427 But we ran into problems making the same approach work for worker machines ref: openshift#4456 We're now looking at other approaches to resolve the network-config requirements driving that work, so switching back to the pointer config for masters seems reasonable, particularly given this issue discovered for hive deployments. Conflicts: pkg/tfvars/baremetal/baremetal.go This reverts commit 98dc381.
This doesn't work for IPI baremetal deployments driven via hive, because there are firewall rules that prevent access to the bootstrap MCS from the pod running the installer. This was implemented in: openshift#4427 But we ran into problems making the same approach work for worker machines ref: openshift#4456 We're now looking at other approaches to resolve the network-config requirements driving that work, so switching back to the pointer config for masters seems reasonable, particularly given this issue discovered for hive deployments. Conflicts: pkg/tfvars/baremetal/baremetal.go This reverts commit 98dc381.
This restores the work which was previously done via #3276 but then reverted via #3589 due to breaking users who customized the pointer ignition config in IPI deployments.
A solution to that has been proposed via #4413 which this PR depends on, so when that lands this work can safely be restored, see openshift/enhancements#540 for more details.
Note that some additional changes beyond the initial implementation were required due to the MCO now supporting multiple
ignition versions, thus this PR depends on openshift-metal3/terraform-provider-ironic#46 which will need to land and be vendored here before the PR merges.
This replaces #4359