feat: implement CAPI failure domain contract for zones by emanuelebosetti · Pull Request #720 · ionos-cloud/cluster-api-provider-proxmox

emanuelebosetti · 2026-04-13T10:04:25Z

Issue: #717
Description of changes:

Map existing CAPMOX zones to CAPI failure domains so that KubeadmControlPlane automatically distributes control plane nodes across Proxmox zones.

API changes (additive, backwards-compatible):

Add Nodes []string to ZoneConfigSpec for per-zone node lists
Add ControlPlane *bool to ZoneConfigSpec (defaults to true)
Add FailureDomains to ProxmoxClusterStatus (CAPI v1beta2 contract)
Add spec.failureDomain to ProxmoxMachineSpec (InfraMachine contract)
Add GetZoneNodes() helper on ProxmoxCluster
Add FailureDomainNotReady condition reason

Controller changes:

Add reconcileFailureDomains in cluster controller (sorted by name to prevent reconcile loops)
Read Machine.Spec.FailureDomain in machine controller with retryable FailureDomainNotReady condition
Use effectiveZone/effectiveAllowedNodes on MachineScope to avoid spec mutation from the controller
Pass zone override to GetInClusterPools for IPAM pool selection
Set Zone on NodeLocation in createVM
Update scheduler to use GetEffectiveAllowedNodes()

Conversion:

Restore Status.FailureDomains and Spec.FailureDomain in v1alpha1 conversion webhook
Add manual conversion stub for ProxmoxMachineSpec.FailureDomain

Testing performed:

Unit tests for API types, scheduler, VM service helpers, and machine scope
Validated on a real Proxmox cluster (10 nodes, 2 zones) with 3 CP + 2 worker nodes
KCP correctly distributes CP replicas round-robin across failure domains
Workers pinned to target zone, IPAM pools selected per-zone
make lint test verify passed
Full backwards compatibility verified — clusters without zoneConfig work as before

wikkyk · 2026-04-13T15:01:05Z

Thanks!
If you could please provide attribution if you used AI/coding agent(s) to develop your PR.

emanuelebosetti · 2026-04-13T16:33:32Z

I've amended the commit to include the Co-Authored-By trailer for the AI coding agent used during development.
Let me know if there are any additional attribution policies or guidelines I should follow for AI-assisted contributions to this project.

wikkyk · 2026-04-15T08:25:17Z

Thanks. No, we don't have any other policies. The field moves fast and we just didn't get to writing things down yet.

emanuelebosetti · 2026-04-15T11:06:48Z

Thanks. No, we don't have any other policies. The field moves fast and we just didn't get to writing things down yet.

We're going through the same process internally — introducing AI across dev teams and figuring out contribution policies.

Three references we found particularly useful:

Could be useful input for #716 as well.

emanuelebosetti · 2026-04-21T08:00:48Z

Hi @wikkyk , do you think it would be possible to include these failure domain changes in the upcoming v0.9.0 release? Happy to address any remaining feedback if needed. Thanks!

wikkyk · 2026-04-29T12:22:29Z

Sure. v0.9.0 is still a fair bit far off, though. This only adds a field so it could reasonably be released in the 0.8 series.

However, as the linter points out, the type of the FailureDomain field has the wrong type. It's optional, so it should be a pointer. You'll need to re-check your pointer handling.

Address review feedback (ionos-cloud#720): the field is optional with no validation, so kube-api-linter requires a pointer. Switch ProxmoxMachineSpec.FailureDomain to *string in v1alpha2 and regen deepcopy. Move the v1alpha2-only restore of FailureDomain into restoreProxmoxMachineSpec so both ProxmoxMachine and ProxmoxMachineTemplate ConvertTo paths preserve the field across the hub→spoke→hub roundtrip. The pointer change shifted the symptom from "" to nil and exposed the pre-existing missing restore on the Template path (TestFuzzyConversion/for_ProxmoxMachineTemplate/hub-spoke-hub). Validated end-to-end on a 10-node Proxmox test cluster: KCP rolled three fresh control plane Machines and both MachineDeployments rolled their workers through the failureDomain path (Machine.Spec.FailureDomain → ApplyFailureDomainNodes → IPAM zone scope → createVM) without errors. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

emanuelebosetti · 2026-05-06T14:32:22Z

@wikkyk pushed the pointer fix as a separate commit. Could you re-trigger the workflows? Thanks!

wikkyk · 2026-05-07T09:22:11Z

Don't worry about the test workflow 'failing', it just can't upload results to sonarqube. I'm happy with this PR, just rename fd/fds as I pointed out inline to avoid the confusion.

wikkyk · 2026-05-18T11:19:34Z

Please rebase this PR on current main.

Address review feedback (ionos-cloud#720): the field is optional with no validation, so kube-api-linter requires a pointer. Switch ProxmoxMachineSpec.FailureDomain to *string in v1alpha2 and regen deepcopy. Move the v1alpha2-only restore of FailureDomain into restoreProxmoxMachineSpec so both ProxmoxMachine and ProxmoxMachineTemplate ConvertTo paths preserve the field across the hub→spoke→hub roundtrip. The pointer change shifted the symptom from "" to nil and exposed the pre-existing missing restore on the Template path (TestFuzzyConversion/for_ProxmoxMachineTemplate/hub-spoke-hub). Validated end-to-end on a 10-node Proxmox test cluster: KCP rolled three fresh control plane Machines and both MachineDeployments rolled their workers through the failureDomain path (Machine.Spec.FailureDomain → ApplyFailureDomainNodes → IPAM zone scope → createVM) without errors. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

emanuelebosetti · 2026-06-18T14:58:28Z

Hi @wikkyk
I've rebased the PR on current main as requested

Map existing CAPMOX zones to CAPI failure domains so that KubeadmControlPlane automatically distributes control plane nodes across Proxmox zones. API changes: - Add Nodes []string to ZoneConfigSpec for per-zone node lists - Add ControlPlane *bool to ZoneConfigSpec (defaults to true) - Add FailureDomains to ProxmoxClusterStatus (CAPI v1beta2 contract) - Add spec.failureDomain to ProxmoxMachineSpec (InfraMachine contract) - Add GetZoneNodes() helper on ProxmoxCluster - Add FailureDomainNotReady condition reason Controller changes: - Add reconcileFailureDomains in cluster controller (sorted by name) - Read Machine.Spec.FailureDomain in machine controller with retryable FailureDomainNotReady condition - Use effectiveZone/effectiveAllowedNodes on MachineScope to avoid spec mutation from the controller - Pass zone override to GetInClusterPools for IPAM pool selection - Set Zone on NodeLocation in createVM - Update scheduler to use GetEffectiveAllowedNodes() Conversion: - Restore Status.FailureDomains and Spec.FailureDomain in v1alpha1 conversion webhook - Add manual conversion stub for ProxmoxMachineSpec.FailureDomain Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Address review feedback (ionos-cloud#720): the field is optional with no validation, so kube-api-linter requires a pointer. Switch ProxmoxMachineSpec.FailureDomain to *string in v1alpha2 and regen deepcopy. Move the v1alpha2-only restore of FailureDomain into restoreProxmoxMachineSpec so both ProxmoxMachine and ProxmoxMachineTemplate ConvertTo paths preserve the field across the hub→spoke→hub roundtrip. The pointer change shifted the symptom from "" to nil and exposed the pre-existing missing restore on the Template path (TestFuzzyConversion/for_ProxmoxMachineTemplate/hub-spoke-hub). Validated end-to-end on a 10-node Proxmox test cluster: KCP rolled three fresh control plane Machines and both MachineDeployments rolled their workers through the failureDomain path (Machine.Spec.FailureDomain → ApplyFailureDomainNodes → IPAM zone scope → createVM) without errors. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

emanuelebosetti · 2026-06-30T09:57:22Z

Hi @wikkyk, rebased against 0.9.0

wikkyk · 2026-07-06T10:02:52Z

+		Spec: ProxmoxClusterSpec{
+			ZoneConfigs: []ZoneConfigSpec{
+				{
+					Zone:  ptr.To("zone-a"),


Incomplete port/rebase? ptr.To won't pass make verify. Use new instead.

wikkyk · 2026-07-06T10:14:26Z

 	clusterv1.Convert_int32_To_Pointer_int32(src.NumSockets, ok, restored.NumSockets, &dst.NumSockets)
 	clusterv1.Convert_int32_To_Pointer_int32(src.MemoryMiB, ok, restored.MemoryMiB, &dst.MemoryMiB)

+	// Restore FailureDomain (v1alpha2-only field, set by CAPI machine controller).


This comment is somewhat superfluous.

wikkyk · 2026-07-06T10:14:53Z

+// Convert_v1alpha2_ProxmoxMachineSpec_To_v1alpha1_ProxmoxMachineSpec handles
+// the lossy conversion of ProxmoxMachineSpec from v1alpha2 to v1alpha1.
+// The FailureDomain field is intentionally dropped (it does not exist in v1alpha1
+// and is restored from annotation on ConvertTo).


This comment is extraneous.

wikkyk · 2026-07-06T10:21:32Z

+	// Use effective allowed nodes (failure-domain override > machine spec > cluster spec).
+	allowedNodes := machineScope.GetEffectiveAllowedNodes()
+	if len(allowedNodes) == 0 {
+		allowedNodes = machineScope.InfraCluster.ProxmoxCluster.Spec.AllowedNodes


This comment now describes an implementation detail that might change in the future.
Also, the if(allowedNodes)==0 doesn't make much sense to me. GetEffectiveAllowedNodes either returns the effective nodes override or ProxmoxMachine.Spec.AllowedNodes, yet then when it's called, you also check InfraCluster.ProxmoxCluster.Spec.AllowedNodes. It feels like the implementation of GetEffectiveAllowedNodes is incomplete.

wikkyk · 2026-07-06T10:23:58Z

+	// controlPlane indicates whether this zone is eligible for control plane machines.
+	// Defaults to true when not set.
+	// +optional
+	ControlPlane *bool `json:"controlPlane,omitempty"`


I really don't like the name of this field. Without reading the comment, I would've guessed that it indicates that it is a control plane, which doesn't make a lot of sense, leading to confusion. The name needs to be more explicit about what this field is and does.

wikkyk

I like the idea but the implementation feels like an attempt at making the minimal change. It doesn't seem fully thought through. I don't like the implementation of GetEffectiveAllowedNodes and I don't like the way every place that touches something that can be overridden needs to check if it might be. It's easy to miss a call/usage site, particularly when merging new changes.

That's not to say that anything in this PR is very bad, the idea is sound and the implementation mostly is sound too. However, merging this as-is (even after applying the cosmetic changes) would increase the maintenance burden.

wikkyk · 2026-07-06T10:45:43Z

For example, this will conflict with #784.

emanuelebosetti requested review from 65278, mcbenjemaa, pborn-ionos and wikkyk as code owners April 13, 2026 10:04

emanuelebosetti had a problem deploying to e2e April 13, 2026 10:04 — with GitHub Actions Failure

emanuelebosetti mentioned this pull request Apr 13, 2026

feat: implement CAPI failure domain contract for v1alpha2 zones #717

Open

emanuelebosetti force-pushed the feat/failure-domains branch from 2350e2b to 37265c7 Compare April 13, 2026 16:30

emanuelebosetti had a problem deploying to e2e April 13, 2026 16:30 — with GitHub Actions Failure

wikkyk mentioned this pull request Apr 16, 2026

Document AI policy #716

Closed

emanuelebosetti temporarily deployed to e2e May 5, 2026 11:46 — with GitHub Actions Inactive

wikkyk reviewed May 7, 2026

View reviewed changes

Comment thread internal/controller/proxmoxmachine_controller.go Outdated

wikkyk reviewed May 7, 2026

View reviewed changes

Comment thread internal/controller/proxmoxcluster_controller.go Outdated

emanuelebosetti temporarily deployed to e2e May 8, 2026 07:42 — with GitHub Actions Inactive

wikkyk previously approved these changes May 13, 2026

View reviewed changes

wikkyk force-pushed the main branch 2 times, most recently from 4b68fd4 to a1d0e00 Compare May 13, 2026 12:26

This was referenced May 13, 2026

Support for ha groups #563

Closed

Rack Awareness / machinedeployment proxmox host anti-affinity #577

Open

wikkyk force-pushed the main branch from 89802a6 to d6fde2d Compare May 14, 2026 12:20

wikkyk self-requested a review May 18, 2026 11:05

emanuelebosetti force-pushed the feat/failure-domains branch from 1debab9 to ee1d111 Compare June 12, 2026 13:15

emanuelebosetti had a problem deploying to e2e June 12, 2026 13:15 — with GitHub Actions Error

emanuelebosetti and others added 3 commits June 30, 2026 10:41

refactor(controller): rename fd to faildom and fds to faildoms

d825a1a

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

emanuelebosetti dismissed wikkyk’s stale review via d825a1a June 30, 2026 08:41

emanuelebosetti force-pushed the feat/failure-domains branch from ee1d111 to d825a1a Compare June 30, 2026 08:41

emanuelebosetti requested a deployment to e2e June 30, 2026 08:42 — with GitHub Actions Waiting

wikkyk reviewed Jul 6, 2026

View reviewed changes

wikkyk requested changes Jul 6, 2026

View reviewed changes

wikkyk mentioned this pull request Jul 6, 2026

Port of previous PR: support VM templates on local storage per node #784

Open

wikkyk linked an issue Jul 7, 2026 that may be closed by this pull request

feat: implement CAPI failure domain contract for v1alpha2 zones #717

Open

Uh oh!

Conversation

emanuelebosetti commented Apr 13, 2026

Uh oh!

wikkyk commented Apr 13, 2026

Uh oh!

emanuelebosetti commented Apr 13, 2026

Uh oh!

wikkyk commented Apr 15, 2026

Uh oh!

emanuelebosetti commented Apr 15, 2026

Uh oh!

emanuelebosetti commented Apr 21, 2026

Uh oh!

wikkyk commented Apr 29, 2026

Uh oh!

emanuelebosetti commented May 6, 2026

Uh oh!

Uh oh!

Uh oh!

wikkyk commented May 7, 2026

Uh oh!

wikkyk commented May 18, 2026

Uh oh!

emanuelebosetti commented Jun 18, 2026

Uh oh!

emanuelebosetti commented Jun 30, 2026

Uh oh!

wikkyk Jul 6, 2026

Choose a reason for hiding this comment

Uh oh!

wikkyk Jul 6, 2026

Choose a reason for hiding this comment

Uh oh!

wikkyk Jul 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wikkyk Jul 6, 2026

Choose a reason for hiding this comment

Uh oh!

wikkyk Jul 6, 2026

Choose a reason for hiding this comment

Uh oh!

wikkyk left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wikkyk commented Jul 6, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

wikkyk Jul 6, 2026 •

edited

Loading

wikkyk left a comment •

edited

Loading