|
| 1 | +<!-- |
| 2 | + This work is licensed under a Creative Commons Attribution 3.0 |
| 3 | + Unported License. |
| 4 | +
|
| 5 | + http://creativecommons.org/licenses/by/3.0/legalcode |
| 6 | +--> |
| 7 | + |
| 8 | +# pre-provisioning kernel argument support |
| 9 | + |
| 10 | +## Status |
| 11 | + |
| 12 | +planned |
| 13 | + |
| 14 | +## Summary |
| 15 | + |
| 16 | +This document proposes a new BareMetalHost spec attribute intended for |
| 17 | +configuring the pre-provisioning image's (usually IPA) kernel arguments. |
| 18 | + |
| 19 | +## Motivation |
| 20 | + |
| 21 | +There are situations where a user would need node specific kernel arguments |
| 22 | +for the pre-provisioning image. The reason could be the use of a mixed set of |
| 23 | +hardware, troubleshooting provisioning or boot issues, selectively configuring |
| 24 | +custom logic e.g. enabling IPA plugins or other services embedded in the |
| 25 | +pre-provisioning image. |
| 26 | + |
| 27 | +### Goals |
| 28 | + |
| 29 | +1. Implement a string type optional spec field named `preprovisioningExtraKernelParams` |
| 30 | +2. Extend the generic `Provisioner` interface to support the new optional field |
| 31 | +3. Implement support for the `preprovisioningExtraKernelParams` in the Ironic provisioner |
| 32 | +4. Combine the values of the `preprovisioningExtraKernelParams` and the |
| 33 | + `preprovisioningImage`'s `status.extraKernelParams` field if a |
| 34 | + `preprovisioningImage` is refferences by the BMH and teh `status.extraKernelParams` |
| 35 | + is not empty |
| 36 | +5. Introducing a `preprovisioningExtraKernelParams` related status field that |
| 37 | + would display the last applied `preprovisioningExtraKernelParams` value. |
| 38 | + Let's call this field `status.lastAppliedPreprovisioningExtraKernelParams` |
| 39 | + |
| 40 | +### Non-Goals |
| 41 | + |
| 42 | +1. To implement any sort validation for the new optional field |
| 43 | +2. Providing a default value for the new spec field |
| 44 | +3. Implement conditional logic to overwrite default kernel parameters on |
| 45 | + per node basis |
| 46 | + |
| 47 | +## Proposal |
| 48 | + |
| 49 | +This document proposes a new BareMetalHost spec attribute intended for |
| 50 | +configuring the pre-provisioning image's (usually IPA) kernel arguments. |
| 51 | + |
| 52 | +The proposal involves the introduction of a new spec field thus a modification |
| 53 | +to the BMO API. The API change is intended to be backward compatible as the |
| 54 | +new `preprovisioningExtraKernelParams` field is optional. |
| 55 | + |
| 56 | +### Implementation Details/Notes/Constraints |
| 57 | + |
| 58 | +The `preprovisioningExtraKernelParams` field would be located directly under `spec` |
| 59 | +section. The format of the new field would be a list/array of individual |
| 60 | +strings. The new field would be optional and it would be considered |
| 61 | +either a NOOP or a cleaning operation depending on the previous value of the |
| 62 | +field in case it would be not present or have no value or would have an empty |
| 63 | +array as a value. |
| 64 | + |
| 65 | +The proposed BMH looks like the following: |
| 66 | + |
| 67 | +```yaml |
| 68 | +apiVersion: metal3.io/v1alpha1 |
| 69 | +kind: BareMetalHost |
| 70 | +metadata: |
| 71 | + name: bm0 |
| 72 | +spec: |
| 73 | + online: true |
| 74 | + bmc: |
| 75 | + address: idrac://192.168.122.1:6230/ |
| 76 | + credentialsName: bm0-bmc-secret |
| 77 | + preprovisioningExtraKernelParams: "--timeout 60000 systemd.journald.forward_to_console=yes ipa-debug=1" |
| 78 | + ... |
| 79 | + ... |
| 80 | + ... |
| 81 | + bootMACAddress: 52:54:00:b7:b2:6f |
| 82 | +... |
| 83 | +... |
| 84 | +... |
| 85 | + |
| 86 | +``` |
| 87 | + |
| 88 | +The example contains a snippet of a common IPA kernel argument list. |
| 89 | +The `preprovisioningExtraKernelParams` string would be received by the `Provisioner` |
| 90 | +interface as is. The handling of the list would be up to the particular |
| 91 | +implementations of the `Provisioner` interface. |
| 92 | + |
| 93 | +Given the BMH snippet from above, the kernel parameter list generated by |
| 94 | +the `Ironic provisioner` and posted to the Ironic Node API would look like |
| 95 | +almost the same with one important addition. The ironic provisioner will |
| 96 | +always prepend the `%default%` substring to the `preprovisioningExtraKernelParams`. |
| 97 | + |
| 98 | +The addition of the "%default%" will cause Ironic to inject the extra |
| 99 | +kernel parameters provided via the ironic config file and ironic-image |
| 100 | +environment variables. The inclusion of "%default%" is beneficial from |
| 101 | +multiple point of view, it makes the API based kernel parameter update logic |
| 102 | +consistent with the same feature used for dynamically built `preprovisioningImage`s. |
| 103 | +An other benefit is that kernel parameters coming from environment variables |
| 104 | +might contain credentials like ssh keys, or other sensitive information |
| 105 | +that would cause a security issue to display in a spec field. |
| 106 | + |
| 107 | +It is possible to implement an annotation or an other spec field to |
| 108 | +force BMO to overwrite the node specific kernel arguments. Implementing |
| 109 | +logic to conditionally overwrite the parameters might prove useful in some |
| 110 | +scenarios but it would completely deviate from how users are applying |
| 111 | +kernel parameters both with pre built or with dynamically build pre provisioning |
| 112 | +images thus using this feature might cause unexpected issues. |
| 113 | + |
| 114 | +In this proposal I am proposing the minimal viable feature to add node |
| 115 | +specific kernel arguments, in case in the future there will be interest in |
| 116 | +fully overwriting kernel parameters then the proposed feature can be extended |
| 117 | +with the relevant logic. |
| 118 | + |
| 119 | +The `status.lastAppliedPreprovisioningExtraKernelParams` would have some limitations. |
| 120 | +An Ironic node's or other provisioners relevant API endpoint could be updated |
| 121 | +with extra kernel parameters such a way to cause the preprovisioning agent to not |
| 122 | +boot properly or behave incorrectly even though the HTTP API patching was |
| 123 | +successful. The status in the aforementioned case would only show what was |
| 124 | +accepted by the provisioner API last time but would not provide info about |
| 125 | +the sensibility of the value and wouldn't even mean that the value is actively |
| 126 | +used because an updated `preprovisioningExtraKernelParams` value would be only |
| 127 | +used at the next suitable life cycle operation. |
| 128 | + |
| 129 | +### Risks and Mitigations |
| 130 | + |
| 131 | +Providing a faulty list kernel arguments might result in boot issues |
| 132 | +or unexpected behavior during the life cycle of the pre-provisioning image. |
| 133 | + |
| 134 | +The user has to be aware that the kernel arguments supplied here will fully or |
| 135 | +partially overwrite the default arguments depending on how the particular |
| 136 | +`Provisioner` is handling the argument list. In case of Ironic provisioner |
| 137 | +the `preprovisioningExtraKernelParams` will completely replace the default argument |
| 138 | +list used by Ironic. |
| 139 | + |
| 140 | +## Design Details |
| 141 | + |
| 142 | +Most of the design is very straightforward and easy to combine with the |
| 143 | +reconcile logic, the support for node specific pre-provisioning kernel args |
| 144 | +also already exist in Ironic, so right after the feature is implemented in BMO |
| 145 | +it can be used without waiting for Ironic implementation to arrive. |
| 146 | + |
| 147 | +The only challenge is the process of updating/cleaning the previously applied |
| 148 | +kernel argument list, this cleanup/update process highly depends on the |
| 149 | +particular provisioner. |
| 150 | + |
| 151 | +This document proposes extending the existing "NodeUpdate" functions of the |
| 152 | +Ironic provisioner utilized during provisioning, inspection and registration |
| 153 | +with updating the kernel parameters of the Ironic driver_info. In case of |
| 154 | +servicing and preparing states/operations "NodeUpdate"s have to be added this |
| 155 | +based on the user's workflow there might be a few extra node updates based on |
| 156 | +how many tines are the servicing or preparing logic is run. |
| 157 | + |
| 158 | +### Work Items |
| 159 | + |
| 160 | +- Implement a string type optional spec field named `preprovisioningExtraKernelParams` |
| 161 | +- Extend the generic `Provisioner` interface to support the new optional field |
| 162 | +- Implement support for the `preprovisioningExtraKernelParams` in the Ironic driver |
| 163 | +- Extend the data types used in different operational states to hold |
| 164 | + an entry for `preprovisioningExtraKernelParams` |
| 165 | +- Implement unit tests, and align existing unit tests as needed |
| 166 | +- Add a check to the existing BMO e2e workflow that checks that the redfish |
| 167 | + images and the PXE config files to verify that the provisioner image will |
| 168 | + receive the correct parameters. Logging into a running IPA could also work |
| 169 | + but in e2e tests Metal3 CI is using upstream IPA images that don't allow |
| 170 | + users to log in via neither serial or ssh connections. |
| 171 | + |
| 172 | +### Dependencies |
| 173 | + |
| 174 | +- Ironic in case of the `Ironic Provisioner` |
| 175 | + |
| 176 | +### Test Plan |
| 177 | + |
| 178 | +- Unit tests for the functions |
| 179 | +- Integration testing with VMs, most likely as part of the existing |
| 180 | + e2e workflow. |
| 181 | + |
| 182 | +### Upgrade / Downgrade Strategy |
| 183 | + |
| 184 | +- In case of upgrade user would not need to do anything |
| 185 | +- In case of downgrade the user has to remove the field from the BMH |
| 186 | + to both make the BMH compatible with the older API and to allow the |
| 187 | + particular provisioner in use to return using the defaults arguments list. |
| 188 | + |
| 189 | +### Version Skew Strategy |
| 190 | + |
| 191 | +None |
| 192 | + |
| 193 | +## Drawbacks |
| 194 | + |
| 195 | +None |
| 196 | + |
| 197 | +## Alternatives |
| 198 | + |
| 199 | +None |
| 200 | + |
| 201 | +## Reference |
| 202 | + |
| 203 | +None |
0 commit comments