baremetal: send full ignition to masters#4359
baremetal: send full ignition to masters#4359kirankt wants to merge 1 commit intoopenshift:masterfrom kirankt:bm-full-ignition
Conversation
openshift-ci-robot
added
the
do-not-merge/work-in-progress
|
/label platform/baremetal |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
/assign @hardys |
staebler
left a comment
staebler
left a comment
There was a problem hiding this comment.
Can you give me a synopsis of what has been done to get around the reasons for the revert?
There was a problem hiding this comment.
So we're extracting the merge out of the stub ignition: what happens to user customizations? Does the ignition from MCS contain them? I know there were some discussions about that but I haven't followed the progress.
There was a problem hiding this comment.
Yes, this is just a minor rework of extracting the URL and CA cert. Regarding user customizations, the current idea is to compare the original stub with any user customizations and, if they are different, the installer will create the appropriate machineconfig manifest for the change. This way when MCO becomes active, the final rendered ignition will contain the user's modification. This PR will go to work as usual and pass the URL and cert to the TF provider, which will extract the rendered ignition.
There was a problem hiding this comment.
Forgot to mention that there will be another PR that will be responsible for creating the machineconfig to reconcile any changes to the stub ignition.
There was a problem hiding this comment.
Any customizations should be accessible via the MCS rendered config if we rebase this on #4413
kirankt
changed the title
openshift-ci-robot
removed
the
do-not-merge/work-in-progress
|
/assign @abhinavdahiya . Hi Abhinav. This is an old revert (reworked a little bit), where in baremetal platform, we send pointer ignition url and root ca cert to our terraform provider for fetching full ignition. Context: we need this to be able to support more complex networking setups (bonds, vlans, etc) prior to booting in baremetal infrastructure. |
|
@kirankt As asked here, I am still looking for a synopsis of how this overcomes the reason for the initial revert. Is it the future PR mentioned here that is expected to overcome the deficiency? If so, then I don't see how we can merge this PR without having the resolution of the future PR in place first. If not, then please help me understand more clearly. |
Hi @staebler I'm sorry I completely missed that comment. The original idea for this PR was to pass the ignition url and its cert to our terraform provider (ironic), which fetched the full config from MCS and built the config drive for baremetal. We did something similar in cluster-api-provider-baremetal (CAPBM) for workers and day 2. Couple things misfired. 1. The CAPBM was not able to contact the MCS from within the cluster. This was by design (an explicit iptables OUTPUT rule on the workers). 2. End users were customizing the pointer ignition configs. e.g https://github.com/openshift/installer/blob/master/docs/user/customization.md#os-customization-unvalidated and our changes broke that interface. So, we reverted. Now, we have couple options we are pursuing. We have couple enhancements we are pursuing: Speaking of CAPBM, rather than using MCS to fetch the full ignition, we plan on fetching the rendered machineconfig object directly and using it to create the config drive for the machines. The work is pending. We still need to sort out RBAC rules to fetch the object. Hope this helps. And thank you for reviewing this PR. |
remove need for sending url and rootca. extract these from stub ignition co-authored-by: Steve Hardy <shardy@redhat.com>
|
@kirankt: 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. |
|
@staebler this should wait until we have a PR merged for openshift/enhancements#540 - planning to get a PR up based on https://github.com/kirankt/installer/tree/ignition-update-check ASAP then we can rebase this PR on top /hold |
openshift-ci-robot
added
the
do-not-merge/hold
#4413 now posted for review, I'll rebase this on top and re-test |
| return nil, err | ||
| } | ||
| ignitionURL := masterIgn.Ignition.Config.Merge[0].Source | ||
| ignitionURLCACert := masterIgn.Ignition.Security.TLS.CertificateAuthorities[0].Source |
There was a problem hiding this comment.
terraform-provider-ignition currently expects just the base64 encoded cert, but this passes the entire dataurl - we need to strip the dataurl prefix here until/unless we rework the provider to accept the full dataurl
|
@hardys: Closed this PR. 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. |
7 participants
This PR is an old revert which sends full ignition to the masters via the ironic terraform provider.
See PR: #3276
co-authored-by: Steve Hardy shardy@redhat.com