Skip to content
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

[WIP] Send full ignition to hosts #455

Closed
wants to merge 1 commit into from
Closed

[WIP] Send full ignition to hosts #455

wants to merge 1 commit into from

Conversation

kirankt
Copy link
Contributor

@kirankt kirankt commented Mar 17, 2020

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.

Creating this PR to foster dialog to see if this is the correct approach or if there is a better way.

WIP

openshift/installer#3276
openshift-metal3/terraform-provider-ironic#38

@metal3-io-bot metal3-io-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 17, 2020
@metal3-io-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: kirankt
To complete the pull request process, please assign hardys
You can assign the PR to them by writing /assign @hardys in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@metal3-io-bot
Copy link
Contributor

Hi @kirankt. Thanks for your PR.

I'm waiting for a metal3-io member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

@metal3-io-bot metal3-io-bot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 17, 2020
@russellb
Copy link
Member

/hold

I don't think this is the right place for this. baremetal-operator is not an OpenShift specific component, and this problem is an OpenShift problem. It should be solved at a layer outside of this and sorted out before config is given to the BMO.

@metal3-io-bot metal3-io-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 17, 2020
@kirankt
Copy link
Contributor Author

kirankt commented Mar 17, 2020

/hold

I don't think this is the right place for this. baremetal-operator is not an OpenShift specific component, and this problem is an OpenShift problem. It should be solved at a layer outside of this and sorted out before config is given to the BMO.

I chose this spot because its the nearest (and most convenient) point to creation of the config drive.

The problem is that when the user-data secret assets are created in the installer, the (bootstrap node and) MCS haven't started yet. To be frank, I'm not 100% sure where this might be moved.

I'm also not sure what the history is regarding the use of CAPBM v1alpha1 in BMO, but we might move this logic into CAPM3 v1alpha2 or newer. This might be a good place but it would mean that we'll some significant code refactor in the installer:

https://github.com/kirankt/cluster-api-provider-metal3/blob/81579be69ebb5a63001da5d1a54609c66c7ed91b/baremetal/metal3machine_manager.go#L263

@russellb
Copy link
Member

It seems like the user data secret given to the BMO could just contain the full config if that's what we wanted?

Anyway, let's discuss elsewhere and can come back here if there is a general baremetal-operator enhancement to propose.

/close

@metal3-io-bot
Copy link
Contributor

@russellb: Closed this PR.

In response to this:

It seems like the user data secret given to the BMO could just contain the full config if that's what we wanted?

Anyway, let's discuss elsewhere and can come back here if there is a general baremetal-operator enhancement to propose.

/close

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.

@stbenjam
Copy link
Member

In OpenShift, I think the problem is the BareMetalHost gets created before the full ignition is available by the installer. It might be challenging to refactor that.

I think this could be done in a generic way in the BMO that lets one represent user data as a URL instead of a secret reference.

Although, I agree the ignition-specific code isn't really appropriate to Metal3.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants