Propose BMH specific preprovisioningExtraKernelParams spec field#552
Propose BMH specific preprovisioningExtraKernelParams spec field#552Rozzii wants to merge 1 commit intometal3-io:mainfrom
Conversation
5b3ba4c to
37d08d8
Compare
|
Something that might be interesting to |
lentzi90
left a comment
There was a problem hiding this comment.
Nice! I have just a few nits/typos and one suggestion to clarify how the strings are concatenated (with spaces?)
37d08d8 to
86fa735
Compare
| `preProvisionKernelArgs` | ||
| 2. Extend the generic `Provisioner` interface to support the new optional field | ||
| 3. Implement support for the `preProvisionKernelArgs` in the Ironic provisioner | ||
|
|
There was a problem hiding this comment.
You also need to extend the PreprovisioningImage object to accept these params.
There was a problem hiding this comment.
We can keep them separate, I didn't intend to make this new spec to be applied for the "PreprovisioningImage" I thought that already has a spec for this purpose.
There was a problem hiding this comment.
That will create a wild inconsistency that will cause users file bugs for me with "preprovisioningExtraKernelParams does not work" :( Because they'll expect OpenShift's version of Metal3 to work the same way as upstream, and without PPI support it won't.
There was a problem hiding this comment.
Btw it's fair if you ask me to follow-up with PPI support myself if you don't have cycles for that. I just want it to be in the design.
There was a problem hiding this comment.
I have already looked into it, my original impression was that the user is already able to set the Kernel Args via the preprovisioningImage CR's spec but now I understand that it is not possible as on that CR the kernel args are only part of the status and not the spec. I have also checked https://github.com/openshift/image-customization-controller so I am more familiar with the use-case of the preprovisioningImage CR.
Based on what I know now It makes sense to include preprovisioningImage in the proposal.
86fa735 to
adaaa1c
Compare
|
WIP implementation: metal3-io/baremetal-operator#2576 at the moment without tests, but functionally this already worked . EDIT: status and the preprovImage CR support is missing ATM |
adaaa1c to
32dba0a
Compare
f09084e to
2b1740b
Compare
| 5. Introducing a `preprovisioningExtraKernelParams` related status field that | ||
| would display the last applied `preprovisioningExtraKernelParams` value. | ||
| Let's call this field `status.lastAppliedPreprovisioningExtraKernelParams` |
There was a problem hiding this comment.
Perhaps something shorter would be easier but I don't have strong opinion on this
Example, status.lastAppliedKernelParams
There was a problem hiding this comment.
I have been thinking about this, and our offline discussions and now I am on the opinion that I should separate this status topic to a different proposal/discussion, because right now we don't really provide status for applying provisioning or preprvisioning data to Ironic API so we might need a more comprehensive approach to give feedback about what, user/network/meta data and what preprovisioning data was applied. We might also need a status flag to show if the previously mentioned data references/values have changed since the last LCM operation (inspect,provision,deprovision,clean, service).
If I would leave the status topic out from the proposal I think it would be still aligned with our current feedback mechanism (or the lack of it) and then we can improve our feedback mechanism with the followup proposal.
| 4. Combine the values of the `preprovisioningExtraKernelParams` and the | ||
| `preprovisioningImage`'s `status.extraKernelParams` field if a | ||
| `preprovisioningImage` is refferences by the BMH and teh `status.extraKernelParams` | ||
| is not empty |
There was a problem hiding this comment.
Is it a good idea to construct a value of a field from two different sources? I imagine things might go wrong, for example how do we handle a case when prepovisioningImage object referenced by BMH doesn't exist for some reason?
Does this mean that a node might have different kernel parameters compared to what is asked via .spec.preprovisioningExtraKernelParams, in case preprovisioningImage status.extraKernelParams has something non-empty? Should not we have a single source of truth for defining the parameters and I guess the status sub-resource was never meant for defining the parameters but more for summarizing the current state right? Perhaps whoever writes into status.extraKernelParams should only rely on BMH spec and not something else? Or is this a transitional state that we eventually target to move to a single source?
|
Issues go stale after 90d of inactivity. If this issue is safe to close now please do so with /lifecycle stale |
|
/remove-lifecycle stale |
|
Issues go stale after 90d of inactivity. If this issue is safe to close now please do so with /lifecycle stale |
|
/remove-lifecycle stale |
2b1740b to
c6a2be2
Compare
|
[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 |
c6a2be2 to
5208756
Compare
01e2eeb to
502ecb2
Compare
This commit: - Introduces the proposal for BMH support for additional node specific kernel command line arguments for non dynamically built pre-provisioning images. - Proposal also contains provisions on how preprovisioningImage CRs are handled together with the new BMH spec. The motivation behind the commit can be found in the proposal. Signed-off-by: Adam Rozman <adam.rozman@est.tech>
502ecb2 to
7588e32
Compare
This commit:
together with the new BMH spec.
The motivation behind the commit can be found in the proposal.