-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Baremetal: Support to send full ignition to masters #3276
Baremetal: Support to send full ignition to masters #3276
Conversation
Hi @kirankt. 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. 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. |
/label platform/baremetal |
Build FAILURE, see build http://10.8.144.11:8080/job/dev-tools/1612/ |
Looks good, but I think there are a few places where we can remove the old ignition_master stuff to only rely on the URL? Also we need to start thinking about how to handle the user-data secrets that end up in the cluster (for both masters and workers) e.g check:
We have an ordering problem here, because similar to the terraform assets, the providerSpec containing this is created in the installer, before the MCS on the bootstrap/cluster is available. So we may need to modify the BareMetalHost schema to add a UserDataUrl or similar, which will require discussion in the upstream metal3 community |
Regarding the schema addition, can't we just deduce the URL using the APIVIP and the familiar ignition port number? |
The problem isn't deducing the URL, it's that the cluster-api-provider-barmetal expects the full UserData (not a URL to download from) similar to the terraform provider, and at the point of creating the secret we only have the pointer/wrapper config (same as the situation with the terraform provider). So, we probably need some new fields (same as the terraform provider) defined in the BMH schema, which can then be provided via the machineset provider spec, then some code to download the rendered config (which could either be in the cluster-api-provider or perhaps the baremetal-operator) |
Regarding the schema addition, can't we just deduce the URL using the APIVIP and the familiar ignition port number?
Got it. Thanks. Will get going on the BMH/CAPM3 side. |
@hardys I've tried to render the ignition (based on your original example) in BMO before we create the config drive. Please take a look and let me know what you think. |
Build FAILURE, see build http://10.8.144.11:8080/job/dev-tools/1640/ |
/hold |
@hardys is this a good time to remove the 'WIP' label from this PR? Or do we wait until the CAPBM PR merges? openshift/cluster-api-provider-baremetal#65 |
/ok-to-test |
/hold cancel |
@kirankt Have a look at the tf-fmt and golint failures, you'll have to address those. Have a look at https://www.terraform.io/docs/commands/fmt.html |
@abhinavdahiya. Ping. Need your feedback/approval for this PR ASAP. We're trying to make it into the 4.5 release. If you're busy, please assign it to someone else in your team. |
…r will track will track installer-specific TF changes made to the baremetal platform
/test e2e-metal-ipi |
Installer team advises against using a partial implementation of Ignition handling to resolve the stub to the MCS url and fetch that. If in the future the stub adds additional required functionality that will need to be considered by baremetal team. However this appears to be localized to baremetal therefore we'll approve this but please consider moving to a more complete Ignition handler to avoid future risk. /approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: sdodson 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 |
/lgtm |
Thanks, we'll have discussions during 4.6 planning about how to move away from this. |
/test e2e-aws-upgrade |
/test e2e-aws |
/test e2e-aws-upgrade |
3 similar comments
/test e2e-aws-upgrade |
/test e2e-aws-upgrade |
/test e2e-aws-upgrade |
/test e2e-aws-upgrade |
@kirankt: 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. |
/test golint |
This was merged in openshift#3276 but had the side-effect of breaking an interface some users were already reliant on, where additional config is added to the pointer ignition output via `create ignition-configs` Downstream bz https://bugzilla.redhat.com/show_bug.cgi?id=1833483 reported this issue, and we've taken the decision to revert this to resolve that interface regression, and look at other ways to satisfy the requirements that led to openshift#3276 This reverts commit f67d61a.
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 <shardy@redhat.com>
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 <shardy@redhat.com>
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 <shardy@redhat.com>
Today, the installer uses a stub ignition that simply points to the full rendered ignition hosted by the bootstrap. However, the full ignition may contain configuration to configure network interfaces with bonding, vlans, etc.
That means the full ignition would need to be sent to the masters instead of the stub, because the hosts may not have network connectivity until after ignition is run. This PR enables the baremetal platform to optionally send full ignition to masters.
Depends on:
openshift-metal3/terraform-provider-ironic#38
metal3-io/baremetal-operator#455openshift/cluster-api-provider-baremetal#65