Repository navigation
fix: follow-ups to the CAPI v1.13.5 upgrade - #410
Merged
Merged
Conversation
WassimKallel
requested review from
gauravgahlot,
gfariasalves-ionos,
jriedel-ionos,
mcbenjemaa,
piepmatz,
pulish,
schegi,
tinashe-mundangepfupfu and
wikkyk
as code owners
September 29, 2026 14:11
mneverov
previously approved these changes
Sep 29, 2026
WassimKallel
dismissed
mneverov’s stale review
September 29, 2026 14:45
The base branch was changed.
MachineProvisioned was only written while waiting for prerequisites (WaitingForClusterInfrastructure / WaitingForBootstrapData) and once provisioning finished. In between it kept the last "waiting" reason, so a VM being created still showed "Waiting for bootstrap data secret", and a failing step (e.g. an image lookup error) was only visible in the logs while the stale message was mirrored up to the Machine, KCP and Cluster. Until the machine is provisioned, set the condition to False/Provisioning before the reconcile steps and to False/ProvisioningFailed with the step error when one fails. A provisioned machine keeps its True condition, so a transient error doesn't mark a running machine as unprovisioned. Adds the MachineProvisioningReason and MachineProvisioningFailedReason constants. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
ReconcileServer returned (false, nil) when the bootstrap data Secret lookup was NotFound, so the step loop carried on. For a worker without a failover IP, FinalizeMachineProvisioning then set status.initialization.provisioned=true and MachineProvisioned=True before any server existed. provisioned is one-way per the CAPI contract, so the wrong signal sticks and hides later provisioning errors. The Secret can be missing while dataSecretName is already set, e.g. before it reaches the controller's cache. Requeue until it exists. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
controller-runtime v0.23 enables the priority queue by default, and with it the default rate limiter is per-item exponential backoff only. The previous default also had a global 10 qps / 100 burst token bucket. Without it, a shared failure (IONOS Cloud API outage, revoked credentials) makes retries against the API grow with the number of objects. Pass workqueue.DefaultTypedControllerRateLimiter explicitly, one limiter per controller as before. The priority queue itself stays on. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
imageMatchError only printed the label selector. The lookup also filters by the data center location and, with useMachineVersion (the default), by an image name containing the Machine's Kubernetes version. An error like 'found 0 images matching selector "e2etest=capic"' doesn't say which filter emptied the list. Include the location, the version filter and the number of images that matched the labels before the name filter, e.g.: found 0 images matching selector "e2etest=capic" in location "de/txl" with name containing "v1.34.11" (0 before the name filter) where 0 before the name filter points at labels/visibility rather than image naming. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
network.go carried a private copy of sigs.k8s.io/cluster-api/util IsControlPlaneMachine, which still exists unchanged in CAPI v1.13.5. Use the upstream helper, as main does, so the failover-IP decisions don't drift from upstream's definition. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
helpers.ExpOwnerReferenceAssertions and helpers.ExpFinalizersAssertion were verbatim subsets of the framework's maps, and helpers.KubernetesReferenceAssertions re-copied upstream's Secret and ConfigMap rules only to accept one more Secret owner. Local copies don't pick up upstream changes to ownership rules, which is how the v1.13 InfraCluster ownership change went unnoticed. Use framework.ExpOwnerReferenceAssertions and framework.ExpFinalizersAssertion directly (extra kinds such as MachinePool are only checked when present), and build KubernetesReferenceAssertions from a clone of the framework map whose Secret rule also accepts the IonosCloudCluster-owned credentials Secret. The upstream rule is replaced rather than added in a second map because AssertOwnerReferences runs every assertion registered for a kind. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
ValidateOwnerReferencesResilience and ValidateOwnerReferencesOnUpdate took identical lists of six assertion maps, so every change (like the topologyManaged flag for the KubeadmControlPlane map) had to be made twice. Build the list and the owner-graph filter once and share them with both checks, the finalizer check and the resourceVersion check. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
clusterv1.ReadyCondition is an untyped constant in v1beta2, so string() does nothing, and next to the bare provider constants it suggests a type conversion is needed. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Severity belongs to the deprecated v1beta1 conditions. These reasons are only used with metav1.Condition now. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- controller-gen v0.19.0 -> v0.20.0 (what CAPI v1.13.5 uses). The regenerated CRDs only change their version annotation. - envtest Kubernetes 1.34.1 -> 1.36.0, matching CAPI v1.13.5 and the newest management version it supports. - setup-envtest: replace the commit pin with the v0.25.1 tag. The pin's comment was waiting for Go 1.26; go.mod is on 1.27.1 now. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
WassimKallel
force-pushed
the
fix/capi-v1.13-follow-ups
branch
from
September 29, 2026 14:47
bcf7d98 to
b6e1190
Compare
mneverov
previously approved these changes
Sep 29, 2026
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The functional changes are coherent and tested; only a non-blocking misplaced documentation comment remains.
Review effort: Balanced
Findings: 1
Open (1)
What changed in this PR
Follow-up fixes for the CAPI v1.13.5 upgrade, improving provisioning correctness, diagnostics, rate limiting, and test maintenance.
Changes:
- Corrects machine provisioning state and bootstrap-secret requeue behavior.
- Restores controller rate limiting and improves image lookup diagnostics.
- Adopts upstream helpers and aligns development tools with CAPI v1.13.
| File | Description |
|---|---|
api/v1alpha1/ionoscloudmachine_types.go |
Adds provisioning condition reasons and updates comments. |
cmd/main.go |
Configures per-controller global rate limiting. |
Makefile |
Updates controller-gen and envtest versions. |
internal/controller/ionoscloudmachine_controller.go |
Reports provisioning progress and failures. |
internal/controller/ionoscloudmachine_controller_test.go |
Tests provisioning condition transitions. |
internal/service/cloud/server.go |
Requeues when the bootstrap Secret is unavailable. |
internal/service/cloud/server_test.go |
Updates missing-secret expectations. |
internal/service/cloud/image.go |
Expands image lookup diagnostics. |
internal/service/cloud/image_test.go |
Tests enhanced image errors. |
internal/service/cloud/network.go |
Uses CAPI’s control-plane helper. |
scope/machine.go |
Removes redundant condition conversion. |
scope/cluster.go |
Removes redundant condition conversion. |
test/e2e/capic_test.go |
Reuses owner-reference assertions and filters. |
test/e2e/helpers/ownerreference.go |
Extends upstream owner-reference assertions. |
test/e2e/helpers/finalizers.go |
Removes duplicated experimental assertions. |
config/crd/bases/infrastructure.cluster.x-k8s.io_ionoscloudmachines.yaml |
Updates generator version stamp. |
config/crd/bases/infrastructure.cluster.x-k8s.io_ionoscloudmachinetemplates.yaml |
Updates generator version stamp. |
config/crd/bases/infrastructure.cluster.x-k8s.io_ionoscloudclusters.yaml |
Updates generator version stamp. |
config/crd/bases/infrastructure.cluster.x-k8s.io_ionoscloudclustertemplates.yaml |
Updates generator version stamp. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- Go: go.mod requires 1.27.1, the README still said 1.26. - CAPIC v0.6 x CAPI v1.11, v0.7 x v1.11 and v0.7 x v1.12 were marked as validated, but no e2e run covers these pairs: the PR e2e job only tests the local build, and the upgrade suite (#379) pairs v0.6.3 with v1.8.12 and v0.7.0 with v1.10.10. Mark them as not validated, per the table's legend; the note below the table already says older releases may keep working with v1.11-v1.13 since the v1beta1 contract is still accepted. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Adding controllerOptions between the initFlags doc comment and the function attached that comment to controllerOptions, so its godoc said it parses the command line flags. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The IonosCloudMachine controller only watched IonosCloudMachines and Machines. A reconcile that sees a paused Cluster, or one whose infrastructure isn't provisioned yet, returns without requeueing, so the machine waits for an unrelated event. If that reconcile read a stale paused Cluster from the cache right as the Cluster was unpaused, nothing woke the machine up again. This made the Quickstart finalizer resilience check flaky: after the framework paused the Cluster, removed all finalizers and unpaused it, one IonosCloudMachine never got its finalizer back within 60s, while the others were rescued by later Machine updates. Watch Clusters and enqueue their IonosCloudMachines (selected by the cluster-name label) on paused transitions or when the infrastructure becomes provisioned, as CAPD does. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
WassimKallel
enabled auto-merge (squash)
September 29, 2026 16:49
mneverov
approved these changes
Sep 30, 2026
WassimKallel
disabled auto-merge
September 30, 2026 08:21
WassimKallel
enabled auto-merge (squash)
September 30, 2026 08:21
|
WassimKallel
disabled auto-merge
September 30, 2026 09:26
WassimKallel
enabled auto-merge (squash)
September 30, 2026 09:26
WassimKallel
disabled auto-merge
September 30, 2026 09:55
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #398, addressing the gaps found while reviewing it. Each fix is a separate commit so they can be reviewed (or dropped) individually.
Behaviour fixes
4b5e209fix(controller): keep MachineProvisioned in step with provisioningMachineProvisionednow showsProvisioning, orProvisioningFailedwith the step error, instead of keeping a staleWaitingForBootstrapData. That stale message is what hid the image-lookup failure in the #379/#398 e2e runs. A provisioned machine stays True on transient errors. Adds two reason constants and a unit test.26db5b9fix(cloud): requeue while the bootstrap secret is not found(false, nil), soFinalizeMachineProvisioningcould mark a worker provisioned before any server existed. It now requeues.a3bc251fix: restore the global rate limit for the provider controllersa3e1926fix(cmd): move the initFlags doc comment back above initFlagscontrollerOptionswas inserted betweeninitFlagsand its doc comment, so the comment documented the wrong function (Copilot review).567cce1fix(controller): reconcile IonosCloudMachines on Cluster unpauseDiagnostics
e42310afix(cloud): report location and name filter in image lookup errorsfound 0 images matching selector …now also reports the location, the version name filter, and the count before the name filter.Cleanups
0402862refactor(cloud): use util.IsControlPlaneMachine againnetwork.gonow matchesmain.abf9140test(e2e): use upstream's owner-reference and finalizer maps2449ea4test(e2e): build the quickstart owner-reference assertions oncec0f652brefactor(scope): drop no-op string() around clusterv1.ReadyConditionReadyConditionis untyped in v1beta2.80a4882docs(api): drop "(Severity=Info)" from condition reason commentsTooling
b6e1190chore(build): align tool versions with CAPI v1.13Documentation
453db57docs: correct the Go version and unvalidated compatibility cellsgo.modrequires 1.27.1. v0.6 × CAPI v1.11, v0.7 × v1.11 and v0.7 × v1.12 were marked ✓ although no e2e run covers those pairs; now ☓ (not validated), per the table's legend.Notes for reviewers
ProvisioningandProvisioningFailedonMachineProvisioned(commit 1).valuewould silently point all three atcore-components.yaml, so it's not worth the edits it saves.Testing
golangci-lint0 issues, unit tests pass.make testpasses (including the API integration suite on envtest 1.36.0),go vet -tags e2e ./test/e2e/...clean.🤖 Generated with Claude Code