DHCP-less IPAM proposal #373
Conversation
metal3-io-bot
added
do-not-merge/work-in-progress
|
Thanks for the review @pierrecregut - and apologies for not tagging you originally, I wasn't sure of your github ID but I see now I should have figured it out 😂 |
|
@hardys At some point the document was moved to the cluster-api-provider-metal3. But the solution should work at the baremetal-operator level and should not imply the use of of a cluster. I would also have not changed the title to include CAPM3 explicitly. |
@pierrecregut this was in response to our previous discussion and an attempt to constrain the scope of this proposal such that it's actionable - we are exploring some BMO/BMH API dependencies, but the main aim is to extend the current CAPM3 templating so the existing IPAM integration works in DHCP-less situations? |
|
Please rebase at convenient time to get the new markdownlinter check. |
hardys
force-pushed
the
dhcp-less
branch
2 times, most recently
from
9f29eb4
to
3b00020
Compare
I move the proposal back to the top-level since it now contains changes for CAPM3 and BMO, and I can adjust the PR title if that makes things clearer. |
|
/retitle DHCP-less IPAM proposal |
metal3-io-bot
changed the title
metal3-io-bot
removed
the
do-not-merge/work-in-progress
|
Thanks everyone for your feedback so far - I've reworked the proposal and tried to take account of all the comments, please let me know if there are any further suggestions or things that I missed. |
hardys
requested review from
dtantsur,
rdoxenham,
pierrecregut,
zaneb and
Rozzii
There was a problem hiding this comment.
@hardys To summarize my comments: the proposal is great except may be the attempt for reclaiming IP that looks complex. There are two sections where alternatives are described and finally we should address how to give feedback when the template cannot expand on a host.
| the pre-provisioning phase as described above. To reduce the required size of the pool, the IPClaim will be deleted after the | ||
| preprovisioning phase is completed, e.g the BMH resource becomes available. | ||
|
|
||
| In the provisioning phase another pool, associated with a cluster is used to template networkData as in the existing process. |
There was a problem hiding this comment.
So if I understand well, you want to use two network configuration for the IPA ? One during inspection and another during provisioning ? This seems complex and I thought that the same configuration would be used. If you really want to delete the claim. I would do it after the node is provisioned but this requires to also erase the preprovisioningNetworkDataName and so what it points to, so that when we upgrade the node we regenerate the data with a usable IP. Is IP address reuse on the provisioning network worth the complexity ?
There was a problem hiding this comment.
The first use-case Preprovisioning - Common IPPool covers the case where the same network configuration is used for IPA and during provisioning, I agree this is the simpler and probably more common case.
My understanding is there are also a use-case where a separate network is used for the pre-provisioning phase, so I'm trying to cover that scenario here.
I agree perhaps reclaiming the IPClaim after inspection introduces undue complexity, but if we don't do that the provisioning network address range potentially becomes a lot larger (number of deployed BMH's vs the concurrency allowed by BMO), perhaps that's a worthwhile tradeoff and we can consider the IPClaim deletion in future if it proves necessary?
There was a problem hiding this comment.
The use case where there'd be two is if you have hosts that are dynamically assigned to clusters (and the networks reconfigured at the same time). I'm not sure this exists in the wild, but it would be good to allow it.
In the more common case of the network staying the same, I think you really want the IP address not to change. The one in the inspection data is the one you want to have in the cluster. So I think ideally you only want to delete the preprovisioning IP claim at provisioning time, and only if a separate provisioned IP claim exists.
In fact, I think you have to keep the claim until after provisioning starts, because the host will continue to use its preprovisioning IP to talk to ironic (and that is the only the host can get provisioned). The "preprovisioning" phase extends up until the BMH hits the provisioned state (i.e. after ironic reboots the host after writing the image to disk, which I think is when the node moves to active), not just after inspection.
There was a problem hiding this comment.
You can support dynamically assigning hosts to clusters without changing the IPA network configuration. It is true that the network used by Kubernetes needs to be reconfigured and be visible from the capm3 controller. On the other hand the IPA only needs to see Ironic (and os image repository). Servers should not see each other. As the requirements are really different, the best solution is to keep two separate network configurations for IPA and CAPM3 and in that case ipclaims are on different pools. If you have cluster definitions and baremetalhosts on different clusters (as with #429 or #408) I am not sure there is an easy path to keep ipclaims synchronized. If you really need to do something like that with a single network (that is what we were doing in old versions of Kanod and yes it was a bad idea), I would put the ipam on the baremetalHost side and push IPs through BaremetalHost annotations and that is why in our "template for BMH" experiment BmhData, metadata could also be exported as BMH annotations.
| a customized ISO, it can embed the configuration instead of Ironic to achieve the same result. | ||
|
|
||
| ### Network configuration failure can't be reflected in status | ||
|
|
There was a problem hiding this comment.
I agree that this part is out of the scope of the proposal and it is not worse than when the IPA fails because of a DHCP configuration problem. But we should address the feedback given when the templater fails to generate the preprovisioningNetworkData for a given baremetal host. Feedback at the level of the template would be strange if the reason is specific to a BMH. A condition on the BMH looks weird as this is not performed by the main controller. A solution would be to at least generate an event associated to the BMH.
There was a problem hiding this comment.
Thanks this is a good point - there are probably a few error-paths to consider:
baremetalhost.metal3.io/preprovisioningnetworkdata-referencevalue does not reference a valid resource (perhaps BMO should validate this, but just check the resource exists?)- Problem with the DataTemplate resource - the schema is enforced by the API but AFAICS for
Metal3DataTemplateany other error will be reported via the resource status, so I think we'll do the same forMetal3PreprovisioningDataTemplategiven they will likely share the same code - Some other problem in CAPM3 related to generating and setting the Secret, here we don't have a Machine resource during the pre-provisioning phase so will have to do as you suggest and generate an event associated with the BMH
| There are several alternative approaches to solve the problem of preventing BMO starting inspection immediately on BMH registration: | ||
|
|
||
| * Add a new BareMetalHost API `PreprovisioningNetworkDataRequired` which defaults to false, but when set to true will describe that the host cannot move from Registering -> Inspecting until `preprovisioningNetworkDataName` has been set. | ||
| * Create the BMH with a preprovisioningNetworkDataName pointing to an empty Secret. BMO refuses to start inspecting until the Secret contains some data. |
There was a problem hiding this comment.
The principle that necessary data can be specified as non existing resource and that the BMH will just wait and will not err until the data is provided is nice. Even with the current approach, this could be used by template custom resource that can generate the data but without the power to modify the BMH fields. And the principle can be generalized to other data (userData, networkData).
|
/lgtm |
|
/cc @lentzi90 @kashifest please take a look |
|
@Rozzii: GitHub didn't allow me to request PR reviews from the following users: please, take, a, look. Note that only metal3-io members and repo collaborators can review this PR, and authors cannot review their own PRs. In 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. |
There was a problem hiding this comment.
Sorry for the much delayed review.
Thanks for the proposal and great work on the writeup!
I have two concerns/questions:
- Using an annotation may seem like an easy solution but I worry that it will not be nice in the long run. We get no schema validation (typos will be nasty to find) and no way of evolving gracefully with conversions between API versions. To me the annotation feels like we are bolting on something rather than building a proper feature with full support because of this.
There were some alternatives described with the disadvantage that they would not handle the association of template to BMH. Why is this? Could we not have a proper BMH API for that? - There are discussions on-going around how to make CAPM3 support multi-tenancy. One of the use-cases I see is that the owner of the BMH does not have to be the same as the owner of the (Metal3)Machine. By handling the preprovisioningNetworkData through CAPM3 we would essentially tie them together even more and make multi-tenancy harder.
My naive proposal would be that BMO should have the new controller that handles the preprovisioningNetworkData and CAPM3 should not know or care about this. If one team is owning the whole thing there is no difference, but if it is split up, I expect that the team owning the BMHs would also handle preprovisioning. What do you think about this?
|
Hi @hardys, there is few open review comments/questions here, are you planning on continuing/finishing this? |
Hi @tuminoid yes I would still like to finish this but have been busy with other tasks - I see @lentzi90 has some feedback so I will look into those, and perhaps we may need to have some discussions on the weekly call to reach consensus. |
Add code review suggestion Co-authored-by: Lennart Jern <lennart.jern@gmail.com>
|
New changes are detected. LGTM label has been removed. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Rozzii 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 |
1 similar comment
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Rozzii 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 |
Add code review suggestion Co-authored-by: Lennart Jern <lennart.jern@gmail.com>
Please could you check the previous thread where this was discussed with @pierrecregut and @zaneb in some detail - the consensus was that adding an annotation was a lightweight option which seems more appropriate given that the BMH doesn't consume this template (it's layered on top via another controller) I'm open to revisiting that direction but it would be really helpful if you could articulate a specific alternative so we can understand the pros/cons vs the minimally invasive annotation choice previously agreed.
I'm not sure this is true, check the
Logically what you describe makes sense, we could have a new controller as described in this proposal added to BMO instead of CAPM3, but practically my concern is it'll basically duplicate the existing CAPM3 controller which handles networkData - we'd need to find a way to handle that which won't make future refactoring (for example to enable templating of nmstate format as an alternative to cloud-init) more difficult. @pierrecregut @zaneb what are your thoughts? Is it worth reworking this proposal again to consider e.g adding a |
|
On one hand, I also agree that explicit fields are better than labels or annotations. I am not sure there is an issue with version migration as it is just the reference but there is also the burnt in documentation of the CRD that makes them much more usable. On the other hand, it is also clear that there is no real consensus on the network format. So the field must be able to consume different templates, so different resource kinds. A ref with a kind and a name may be fine. It is still true that the content will not be handled by the BareMetalHost controller itself but another one even if a default may be bundled in the BmhController pod. |
Thanks for the pointer, I have now gone through the thread and understand better the reasoning. I would like to suggest at minimum one change in the design, and also to give a couple of comparisons to alternatives that we could consider. The minimum change I would like is that BMO should not care about the annotation. Instead, the This flow is identical to how cert-manager handles annotations on Ingress resources and should be familiar to many users. Note that this way of working is limited. It is not possible to request any special settings through the annotation since it is only a reference to another resource. The second comparison I want to make is to PersistentVolume(Claims) and StatefulSets. I think this is also similar to what we want to achieve and it is solved in a different way. StatefulSets have volumeClaimTemplates that provide a way to dynamically create PersistentVolumeClaims for the Pods, that would then be filled from a StorageClass. I think this has the advantage of giving the user better control than the annotation, while also automating claim creation. However, it would be a larger API change, and we do not have the same need for dynamically creating the claims since the BMHs don't scale. I still wanted to give this as an alternative to consider, but I am fine with the annotation as described above.
You are right! Somehow I missed that. It does address the use-case form the "user point of view". I.e. one user can manage the BMHs and preprovisioning, while another is "consuming" the BMHs for running workload. This is all good. I still have a concern when thinking one step further though. If CAPM3 is going to handle the preprovisioningNetworkData and directly interact with the BMHs, then the user must delegate enough privileges to CAPM3 to be able to do this. My long term goal is to add an indirection so that CAPM3 would not need this access. The point is to avoid giving the "consumer" access to BMC credentials, Ironic endpoints and so on. However, I admit that CAPM3 for now (even with the multi-tenancy proposal) needs this access so adding the preprovisioningNetworkData is just one more thing. If we can agree that CAPM3 will just read the annotation and create the secret, then that is good enough for me. My concern would be if CAPM3 would directly change the BMH object to add the preprovisioningNetworkData.
Good point. I am fine ignoring this for now. As we progress with multi-tenancy and "BMH claims", we may need to revisit, but I don't want to block this proposal based on some imaginary future proposal and design. |
I am not sure code reuse which (a go issue) and location of controllers need to be mixed. We could have a module (separate to avoid linking dependencies of BMO controller and capm3 controller) in BMO repository that handles network templating with respect to a set of meta-data sources. It would also contain the types describing networks with marshalling annotations. It would be used for the PreprovisioningTemplate controller in BMO with only BareMetalHost as source and in CAPM3 by the DataTemplate controller with machine, M3M and BareMetalHost as sources. Defining the preprovisioning configuration is really a responsibility of the teams handling the hardware because its primary purpose is to talk to the Ironic instance they manage. They have no reason to deploy capm3 on their cluster if BMH and cluster management are separate. |
| There are several potential workarounds for this situation, but it would be preferable for a common solution | ||
| to be found which can benefit both Metal3 and the broader Ironic community, as such solving this problem is considered | ||
| outside the scope of this proposal, but we will follow and engage with the | ||
| [Ironic community spec related to this problem](https://review.opendev.org/c/openstack/ironic-specs/+/906324/1/specs/approved/fix-vmedia-boot-config.rst ) |
There was a problem hiding this comment.
After discussing upstream on IRC the following patches were mentioned which should help resolve the config-2 issue:
https://review.opendev.org/c/openstack/ironic-python-agent-builder/+/920184
https://review.opendev.org/c/openstack/ironic-python-agent-builder/+/915170
https://review.opendev.org/q/I9b74ec977fabc0a7f8ed6f113595a3f1624f6ee6
I will update to reflect this latest status
@lentzi90 my main concern with this suggestion is we'd end up with two slightly different user interfaces for preprovisioningNetworkData and the existing networkData - I guess that could be mitigated by ensuring the "new" behavior is possible for both, e.g check for a secret name on the BMH and only try to set it when unset (permissions for which could be potentially removed in future) |
|
After reading through the multitenancy proposal, I think Lennart raises a good point here. The original sin of Metal³ has always been that we lumped everything into one BMH object and didn't make any distinction between different roles that might be involved - forget multitenancy for a second, we don't even distinguish between a hardware administrator and someone who is allowed to provision. If you can provision a host you can do anything to it. CAPM3 already crossed the line a bit by giving the user control of the networkData. Note that in OpenStack, the networkData and metaData are provided internally by ironic itself, and only the userData is under the control of the user via the Nova API. We added the preprovisioningNetworkData field largely so we could keep what is clearly an admin operation (affects whether we can boot IPA at all) separate from the user-supplied data that CAPM3 adds. Now we are finding that CAPM3 doing IPAM for the provisioning image is not actually sufficient, since it needs to be done before provisioning. Perhaps this is an opportunity to reconsider whether this functionality belongs on the CAPI side of the fence at all. Whether a desire to add a host to a CAPI cluster should be sufficient to imply permission to do that. In Metal³ we are missing a component that covers some functionality in OpenStack that is spread between Nova, Ironic, and Neutron: given a request to provision a host, find one we're permitted to use, connect it to the right networks, and supply it with the right networkData. Partly that was intentional, because as Steve and I both know it turns out nobody actually wants their datacenter to act like a cloud where they don't care where the resources are coming from. The HostClaim proposal is essentially a proposal to build that component, starting with disconnecting a request to provision a host from any specific host (which I think may be a mistake), but inevitably leading to demands that we add the ability to reconfigure networks at the switch level (which I also think may be a mistake). Personally, I'd prefer to pursue multitenancy by separating the different roles clearly and making space for anybody who wants to to build another component in the middle to bridge the gap between user and admin. It's not clear that one size fits all, nor that we want the Metal³ project to be the sole owner of that problem. In the meantime, it seems to me that IPAM fits pretty clearly on the 'admin' side of the user-admin divide, and is something that should be configured as part of creating the BMH resources, not as part of provisioning them, in applications where those two roles are separate. I'm not sure that this changes this proposal at all, except that maybe |
|
Thanks for the additional feedback @zaneb, one question below:
If we're in agreement this should be aligned with the BMO/BMH not CAPM3, perhaps we should consider a more explicit BMH API addition instead of the annotation (this was previously discussed but rejected because I envisaged extending the current CAPM3 model), and add a new controller to the BMO which can handle templating as an alternative to the current CAPM3 flow? e.g something like: apiVersion: ipam.metal3.io/v1alpha1
kind: IPPool
metadata:
name: pool-1
spec:
clusterName: cluster
---
apiVersion: metal3.io/v1alpha1
kind: NetworkDataTemplate
metadata:
name: preprov-data-template
spec:
format: cloud-init # So we can add e.g nmstate without replacing the resource type
networkData:
networks:
ipv4:
- id: eth0
ipAddressFromIPPool: pool-1
---
apiVersion: metal3.io/v1alpha1
kind: BareMetalHost
spec:
preprovisioningNetworkDataTemplate: preprov-data-template
# networkDataTemplate could also be added as an alternative to current CAPM3 templating
... |
Absolutely yes. I think we only discussed an annotation to avoid the reverse dependency of BMO on CAPM3? The new API solves this issue. |
|
That seems plausible, although it feels a bit weird having 2 separate APIs in the BMH for the same thing. I think we are in agreement this should be aligned with the BMO/BMH not CAPM3 at least. |
Yeah a BMO controller writing back to the spec does seem awkward - another option could be to create the secret and track it in the BMH status, then when when we read e.g preprovisioningNetworkData we check for (and when needed generate) a template-generated secret if no user-provided one is present in the spec? This is similar to how the Metal3Machine API works and would also mean if anyone mixes the new BMH APIs with the existing CAPM3 ones the old/existing approach would take precedence, users would opt in to the new behaviour by not setting any networkData in Metal3DataTemplate or Metal3Machine and instead setting networkDataTemplate on the BMH. |
|
@lentzi90 any feedback on the alternate direction discussed above? It could help with the multi-tenancy direction so that tenant users no longer have any need to directly interact with *networkData, they would only need to specify metaData/userData at the machine level, with networking handled by the BMH owner. |
I personally don't find this weird at all but I don't know what is standard in Kubernetes.
Works for me too. |
Draft proposal to explore how we can enable the CAPM3 IPAM flow in a DHCP-less environment, following on from recent discussions on Slack
/cc @Rozzii @dtantsur