✨ Select Metal3DataTemplate per failure domain from BMH placement - #3589
✨ Select Metal3DataTemplate per failure domain from BMH placement#3589shibaPuppy wants to merge 1 commit into
Conversation
|
[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 |
metal3-io-bot
added
size/XL
|
Hi @shibaPuppy. 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 Tip We noticed you've done this a few times! Consider joining the org to skip this step and gain Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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-sigs/prow repository. |
cd5d65b to
b3df74f
Compare
|
/cc @Rozzii This is quite a large PR, I don't recommend forcing this through the review pocess for 1.14 as the feature freeze will happen this week. I will mark this as a 1.15 target and review when I have some time. |
|
/ok-to-test |
metal3-io-bot
added
ok-to-test
There was a problem hiding this comment.
Pull request overview
Adds failure-domain-specific data templates based on actual BareMetalHost placement and reports the resulting failure domain.
Changes:
- Adds API fields, CRD validation, conversion, and deepcopy support.
- Selects data templates from BareMetalHost failure-domain labels.
- Adds controller and webhook tests.
Reviewed changes
Copilot reviewed 9 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
internal/webhooks/v1beta2/metal3machinetemplate_webhook.go |
Validates template namespaces. |
internal/webhooks/v1beta2/metal3machinetemplate_webhook_test.go |
Tests namespace validation. |
controllers/metal3machine_controller.go |
Implements placement reporting and template selection. |
controllers/metal3machine_controller_test.go |
Tests placement and selection behavior. |
config/crd/bases/infrastructure.cluster.x-k8s.io_metal3machinetemplates.yaml |
Exposes mapping schema. |
config/crd/bases/infrastructure.cluster.x-k8s.io_metal3machines.yaml |
Exposes failure-domain status. |
api/v1beta2/zz_generated.deepcopy.go |
Adds generated deepcopy support. |
api/v1beta2/metal3machinetemplate_types.go |
Defines failure-domain mappings. |
api/v1beta2/metal3machine_types.go |
Defines placement status. |
api/v1beta1/zz_generated.conversion.go |
Records manual conversion requirements. |
api/v1beta1/conversion.go |
Preserves new fields across conversion. |
Files not reviewed (2)
- api/v1beta1/zz_generated.conversion.go: Generated file
- api/v1beta2/zz_generated.deepcopy.go: Generated file
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if err := r.Client.Get(ctx, key, m3mt); err != nil { | ||
| if apierrors.IsNotFound(err) { | ||
| log.V(baremetal.VerbosityLevelDebug).Info("Source Metal3MachineTemplate not found, keeping dataTemplate", | ||
| baremetal.LogFieldMetal3MachineTemplate, templateName) | ||
| return nil |
There was a problem hiding this comment.
The scenario is real, but I think the lookup is the right trade-off here — happy to change it if you disagree.
In CAPI an infrastructure template is a live clone source: KCP and MachineSet hold an infrastructureRef to it, so deleting one that is still in use already breaks scale-up and rollout independently of this feature. CAPM3 also already relies on this lookup — the Metal3MachineTemplate controller resolves its clones through the same cloned-from annotations to propagate automatedCleaningMode.
The exposure is also narrow: with the claim-existence guard above, the default template can only be used if the source template is deleted in the window between the Metal3Machine being cloned and its Metal3DataClaim being created. The missing template is logged rather than silently ignored.
If you would rather fail closed, the alternative is to requeue instead of proceeding when the source template is missing. I avoided it because it would also block machines whose template was intentionally removed after a rollout, but I am fine either way.
…ement - failureDomainDataTemplates on Metal3MachineTemplate selects the Metal3DataTemplate matching the failure-domain label of the BMH the machine is actually placed on, before metadata rendering. - Metal3Machine reports the placement failure domain in spec.failureDomain and the new status.failureDomain; CAPI surfaces them to the Machine. Hosts without the label stay outside the FD scheme. Signed-off-by: shibaPuppy <sub951@naver.com>
b3df74f to
5af9a5c
Compare
|
@Rozzii |
1.15 review cycle just started so this PR is on my review list. |
What this PR does / why we need it:
Metal3MachineTemplate references a single
dataTemplate, so every control-plane node gets the same network configuration even when failure domains map to different network segments (bonding, VLANs, subnets, IP pools).Running one KCP per network layout is not possible, and workers need per-rack network configuration as well.
This PR adds an optional
failureDomainDataTemplateslist toMetal3MachineTemplateSpecmapping failure domain names to Metal3DataTemplate references.When a Metal3Machine cloned from the template is associated with a BareMetalHost whose
infrastructure.cluster.x-k8s.io/failure-domainlabel matches an entry, the machine'sdataTemplateis overridden with the mapped reference before its metadata is rendered (guarded bystatus.renderedDataand Metal3DataClaim existence, so an already rendered or claimed machine is never rewritten).Design notes, refining the proposal in the issue:
Machine.spec.failureDomain.pickHost()treats the assigned failure domain as a soft preference and can fall back to a host in a different domain; keying the network configuration off the assignment would then inject the wrong subnet/VLAN into the node.Keying off the placement makes the rendered network data correct regardless of fallback, and works for MachineDeployment workers with no assigned failure domain at all.
spec.failureDomain(surfaced toMachine.spec.failureDomainby the Machine controller) and in the newstatus.failureDomain, following the v1beta2 contract.The assigned value keeps being mirrored Machine→Metal3Machine until a host is associated; afterwards placement wins.
CAPM3 never writes the core Machine object.
dataTemplate.Behavioral note for users not using the new field: once a consumed BMH carries the failure-domain label, the Metal3Machine starts reporting the placement failure domain (and the Machine converges to it).
This only makes the existing fields reflect actual placement; dataTemplate and network data are unchanged unless
failureDomainDataTemplatesis set.Validation: duplicate failure domains are rejected via
listType=map, an emptydataTemplate.namevia CRD CEL, and cross-namespace references via the Metal3MachineTemplate webhook.failureDomainkeys are capped at 63 characters to match the label value limit.Both new fields round-trip through v1beta1 conversion.
Fixes #2716
Checklist: