From d02f2b266534ee2b72e2a8e82c8ae650f69a7f3b Mon Sep 17 00:00:00 2001 From: Stefan Bueringer Date: Fri, 7 Nov 2025 09:43:25 +0100 Subject: [PATCH 1/4] MD: Implement CanUpdateMachineSet MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Stefan Büringer buringerst@vmware.com --- .../hooks/v1alpha1/inplaceupdate_types.go | 12 +- .../hooks/v1alpha1/zz_generated.openapi.go | 12 +- controllers/alias.go | 6 +- .../kubeadm/internal/control_plane.go | 8 +- .../controllers/inplace_canupdatemachine.go | 138 +-- .../inplace_canupdatemachine_test.go | 7 +- .../runtime-sdk/implement-extensions.md | 2 +- docs/proposals/20220221-runtime-SDK.md | 7 +- .../machine_controller_inplace_update.go | 3 +- .../machine_controller_inplace_update_test.go | 9 +- .../machinedeployment_canupdatemachineset.go | 351 +++++++ ...hinedeployment_canupdatemachineset_test.go | 930 ++++++++++++++++++ .../machinedeployment_controller.go | 10 +- .../machinedeployment_rollout_ondelete.go | 2 +- ...machinedeployment_rollout_ondelete_test.go | 4 +- .../machinedeployment_rollout_planner.go | 17 +- ...machinedeployment_rollout_rollingupdate.go | 11 +- ...nedeployment_rollout_rollingupdate_test.go | 23 +- .../machinedeployment_sync.go | 2 +- .../topology/cluster/patches/engine.go | 2 +- internal/util/patch/patch.go | 134 ++- main.go | 1 + test/e2e/cluster_in_place_update.go | 17 +- test/e2e/cluster_upgrade_runtimesdk.go | 7 +- test/e2e/quick_start.go | 6 +- test/e2e/scale.go | 5 +- .../config/tilt/extensionconfig.yaml | 4 +- .../handlers/inplaceupdate/handlers.go | 19 +- 28 files changed, 1541 insertions(+), 208 deletions(-) create mode 100644 internal/controllers/machinedeployment/machinedeployment_canupdatemachineset.go create mode 100644 internal/controllers/machinedeployment/machinedeployment_canupdatemachineset_test.go diff --git a/api/runtime/hooks/v1alpha1/inplaceupdate_types.go b/api/runtime/hooks/v1alpha1/inplaceupdate_types.go index 1f2accce966a..a301c39e9aa6 100644 --- a/api/runtime/hooks/v1alpha1/inplaceupdate_types.go +++ b/api/runtime/hooks/v1alpha1/inplaceupdate_types.go @@ -67,14 +67,17 @@ type CanUpdateMachineResponse struct { CommonResponse `json:",inline"` // machinePatch when applied to the current Machine spec, indicates changes handled in-place. + // Only fields in spec have to be covered by the patch. // +optional MachinePatch Patch `json:"machinePatch,omitempty,omitzero"` // infrastructureMachinePatch indicates infra Machine spec changes handled in-place. + // Only fields in spec have to be covered by the patch. // +optional InfrastructureMachinePatch Patch `json:"infrastructureMachinePatch,omitempty,omitzero"` // bootstrapConfigPatch indicates bootstrap config spec changes handled in-place. + // Only fields in spec have to be covered by the patch. // +optional BootstrapConfigPatch Patch `json:"bootstrapConfigPatch,omitempty,omitzero"` } @@ -119,14 +122,17 @@ type CanUpdateMachineSetRequest struct { // CanUpdateMachineSetRequestObjects groups objects for CanUpdateMachineSetRequest. type CanUpdateMachineSetRequestObjects struct { // machineSet is the full MachineSet object. + // Only fields in spec.template.spec have to be covered by the patch. // +required MachineSet clusterv1.MachineSet `json:"machineSet,omitempty,omitzero"` // infrastructureMachineTemplate is the provider-specific InfrastructureMachineTemplate object. + // Only fields in spec.template.spec have to be covered by the patch. // +required InfrastructureMachineTemplate runtime.RawExtension `json:"infrastructureMachineTemplate,omitempty,omitzero"` // bootstrapConfigTemplate is the provider-specific BootstrapConfigTemplate object. + // Only fields in spec.template.spec have to be covered by the patch. // +optional BootstrapConfigTemplate runtime.RawExtension `json:"bootstrapConfigTemplate,omitempty,omitzero"` } @@ -217,7 +223,8 @@ func init() { "Notes:\n" + "- This hook is called during the planning phase of updates\n" + "- Only spec is provided, status fields are not included\n" + - "- If no extension can cover the required changes, CAPI will fallback to rolling updates\n", + "- If no extension can cover the required changes, CAPI will fallback to rolling updates\n" + + "- Only fields in Machine/InfraMachine/BootstrapConfig spec have to be covered by patches\n", }) catalogBuilder.RegisterHook(CanUpdateMachineSet, &runtimecatalog.HookMeta{ @@ -230,7 +237,8 @@ func init() { "Notes:\n" + "- This hook is called during the planning phase of updates\n" + "- Only spec is provided, status fields are not included\n" + - "- If no extension can cover the required changes, CAPI will fallback to rolling updates\n", + "- If no extension can cover the required changes, CAPI will fallback to rolling updates\n" + + "- Only fields in MachineSet/InfraMachineTemplate/BootstrapConfigTemplate spec.template.spec have to be covered by patches\n", }) catalogBuilder.RegisterHook(UpdateMachine, &runtimecatalog.HookMeta{ diff --git a/api/runtime/hooks/v1alpha1/zz_generated.openapi.go b/api/runtime/hooks/v1alpha1/zz_generated.openapi.go index 52a2a54e106f..bd7f0207849d 100644 --- a/api/runtime/hooks/v1alpha1/zz_generated.openapi.go +++ b/api/runtime/hooks/v1alpha1/zz_generated.openapi.go @@ -1408,21 +1408,21 @@ func schema_api_runtime_hooks_v1alpha1_CanUpdateMachineResponse(ref common.Refer }, "machinePatch": { SchemaProps: spec.SchemaProps{ - Description: "machinePatch when applied to the current Machine spec, indicates changes handled in-place.", + Description: "machinePatch when applied to the current Machine spec, indicates changes handled in-place. Only fields in spec have to be covered by the patch.", Default: map[string]interface{}{}, Ref: ref("sigs.k8s.io/cluster-api/api/runtime/hooks/v1alpha1.Patch"), }, }, "infrastructureMachinePatch": { SchemaProps: spec.SchemaProps{ - Description: "infrastructureMachinePatch indicates infra Machine spec changes handled in-place.", + Description: "infrastructureMachinePatch indicates infra Machine spec changes handled in-place. Only fields in spec have to be covered by the patch.", Default: map[string]interface{}{}, Ref: ref("sigs.k8s.io/cluster-api/api/runtime/hooks/v1alpha1.Patch"), }, }, "bootstrapConfigPatch": { SchemaProps: spec.SchemaProps{ - Description: "bootstrapConfigPatch indicates bootstrap config spec changes handled in-place.", + Description: "bootstrapConfigPatch indicates bootstrap config spec changes handled in-place. Only fields in spec have to be covered by the patch.", Default: map[string]interface{}{}, Ref: ref("sigs.k8s.io/cluster-api/api/runtime/hooks/v1alpha1.Patch"), }, @@ -1505,20 +1505,20 @@ func schema_api_runtime_hooks_v1alpha1_CanUpdateMachineSetRequestObjects(ref com Properties: map[string]spec.Schema{ "machineSet": { SchemaProps: spec.SchemaProps{ - Description: "machineSet is the full MachineSet object.", + Description: "machineSet is the full MachineSet object. Only fields in spec.template.spec have to be covered by the patch.", Default: map[string]interface{}{}, Ref: ref("sigs.k8s.io/cluster-api/api/core/v1beta2.MachineSet"), }, }, "infrastructureMachineTemplate": { SchemaProps: spec.SchemaProps{ - Description: "infrastructureMachineTemplate is the provider-specific InfrastructureMachineTemplate object.", + Description: "infrastructureMachineTemplate is the provider-specific InfrastructureMachineTemplate object. Only fields in spec.template.spec have to be covered by the patch.", Ref: ref("k8s.io/apimachinery/pkg/runtime.RawExtension"), }, }, "bootstrapConfigTemplate": { SchemaProps: spec.SchemaProps{ - Description: "bootstrapConfigTemplate is the provider-specific BootstrapConfigTemplate object.", + Description: "bootstrapConfigTemplate is the provider-specific BootstrapConfigTemplate object. Only fields in spec.template.spec have to be covered by the patch.", Ref: ref("k8s.io/apimachinery/pkg/runtime.RawExtension"), }, }, diff --git a/controllers/alias.go b/controllers/alias.go index 0bf00eb64897..b8fc4d21e103 100644 --- a/controllers/alias.go +++ b/controllers/alias.go @@ -123,8 +123,9 @@ func (r *MachineSetReconciler) SetupWithManager(ctx context.Context, mgr ctrl.Ma // MachineDeploymentReconciler reconciles a MachineDeployment object. type MachineDeploymentReconciler struct { - Client client.Client - APIReader client.Reader + Client client.Client + APIReader client.Reader + RuntimeClient runtimeclient.Client // WatchFilterValue is the label value used to filter events prior to reconciliation. WatchFilterValue string @@ -134,6 +135,7 @@ func (r *MachineDeploymentReconciler) SetupWithManager(ctx context.Context, mgr return (&machinedeploymentcontroller.Reconciler{ Client: r.Client, APIReader: r.APIReader, + RuntimeClient: r.RuntimeClient, WatchFilterValue: r.WatchFilterValue, }).SetupWithManager(ctx, mgr, options) } diff --git a/controlplane/kubeadm/internal/control_plane.go b/controlplane/kubeadm/internal/control_plane.go index 924efa474790..af76f23da4e7 100644 --- a/controlplane/kubeadm/internal/control_plane.go +++ b/controlplane/kubeadm/internal/control_plane.go @@ -37,6 +37,7 @@ import ( "sigs.k8s.io/cluster-api/controllers/external" "sigs.k8s.io/cluster-api/controlplane/kubeadm/internal/etcd" "sigs.k8s.io/cluster-api/internal/hooks" + "sigs.k8s.io/cluster-api/internal/util/inplace" "sigs.k8s.io/cluster-api/util/collections" "sigs.k8s.io/cluster-api/util/conditions" "sigs.k8s.io/cluster-api/util/failuredomains" @@ -199,12 +200,7 @@ func (c *ControlPlane) MachinesToCompleteTriggerInPlaceUpdate() collections.Mach // MachinesToCompleteInPlaceUpdate returns Machines that still have to complete their in-place update. func (c *ControlPlane) MachinesToCompleteInPlaceUpdate() collections.Machines { - return c.Machines.Filter(func(machine *clusterv1.Machine) bool { - // Note: Checking both annotations here to make this slightly more robust. - // Theoretically only checking for IsPending would have been enough. - _, ok := machine.Annotations[clusterv1.UpdateInProgressAnnotation] - return ok || hooks.IsPending(runtimehooksv1.UpdateMachine, machine) - }) + return c.Machines.Filter(inplace.IsUpdateInProgress) } // FailureDomainWithMostMachines returns the fd with most machines in it and at least one eligible machine in it. diff --git a/controlplane/kubeadm/internal/controllers/inplace_canupdatemachine.go b/controlplane/kubeadm/internal/controllers/inplace_canupdatemachine.go index deaa84ea8610..2f8a93e254f3 100644 --- a/controlplane/kubeadm/internal/controllers/inplace_canupdatemachine.go +++ b/controlplane/kubeadm/internal/controllers/inplace_canupdatemachine.go @@ -17,19 +17,14 @@ limitations under the License. package controllers import ( - "bytes" "context" - "encoding/json" "fmt" - "runtime/debug" "strings" - jsonpatch "github.com/evanphx/json-patch/v5" "github.com/pkg/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" - kerrors "k8s.io/apimachinery/pkg/util/errors" "k8s.io/klog/v2" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" @@ -40,7 +35,7 @@ import ( "sigs.k8s.io/cluster-api/controlplane/kubeadm/internal" "sigs.k8s.io/cluster-api/feature" "sigs.k8s.io/cluster-api/internal/util/compare" - patchutil "sigs.k8s.io/cluster-api/internal/util/patch" + "sigs.k8s.io/cluster-api/internal/util/patch" "sigs.k8s.io/cluster-api/internal/util/ssa" ) @@ -75,7 +70,7 @@ func (r *KubeadmControlPlaneReconciler) canUpdateMachine(ctx context.Context, ma return false, nil } if len(extensionHandlers) > 1 { - return false, errors.Errorf("found multiple CanUpdateMachine hooks (%s) (more than one is not supported yet)", strings.Join(extensionHandlers, ",")) + return false, errors.Errorf("found multiple CanUpdateMachine hooks (%s): only one hook is supported", strings.Join(extensionHandlers, ",")) } canUpdateMachine, reasons, err := r.canExtensionsUpdateMachine(ctx, machine, machineUpToDateResult, extensionHandlers) @@ -186,19 +181,19 @@ func createRequest(ctx context.Context, c client.Client, currentMachine *cluster }, } var err error - req.Current.BootstrapConfig, err = convertToRawExtension(cleanupKubeadmConfig(currentKubeadmConfigForDiff)) + req.Current.BootstrapConfig, err = patch.ConvertToRawExtension(cleanupKubeadmConfig(currentKubeadmConfigForDiff)) if err != nil { return nil, err } - req.Desired.BootstrapConfig, err = convertToRawExtension(cleanupKubeadmConfig(desiredKubeadmConfigForDiff)) + req.Desired.BootstrapConfig, err = patch.ConvertToRawExtension(cleanupKubeadmConfig(desiredKubeadmConfigForDiff)) if err != nil { return nil, err } - req.Current.InfrastructureMachine, err = convertToRawExtension(cleanupUnstructured(currentInfraMachineForDiff)) + req.Current.InfrastructureMachine, err = patch.ConvertToRawExtension(cleanupUnstructured(currentInfraMachineForDiff)) if err != nil { return nil, err } - req.Desired.InfrastructureMachine, err = convertToRawExtension(cleanupUnstructured(desiredInfraMachineForDiff)) + req.Desired.InfrastructureMachine, err = patch.ConvertToRawExtension(cleanupUnstructured(desiredInfraMachineForDiff)) if err != nil { return nil, err } @@ -255,44 +250,21 @@ func cleanupUnstructured(u *unstructured.Unstructured) *unstructured.Unstructure return cleanedUpU } -func convertToRawExtension(object runtime.Object) (runtime.RawExtension, error) { - objectBytes, err := json.Marshal(object) - if err != nil { - return runtime.RawExtension{}, errors.Wrap(err, "failed to marshal object to JSON") - } - - objectUnstructured, ok := object.(*unstructured.Unstructured) - if !ok { - objectUnstructured = &unstructured.Unstructured{} - // Note: This only succeeds if object has apiVersion & kind set (which is always the case). - if err := json.Unmarshal(objectBytes, objectUnstructured); err != nil { - return runtime.RawExtension{}, errors.Wrap(err, "failed to Unmarshal object into Unstructured") - } - } - - // Note: Raw and Object are always both set and Object is always set as an Unstructured - // to simplify subsequent code in matchesUnstructuredSpec. - return runtime.RawExtension{ - Raw: objectBytes, - Object: objectUnstructured, - }, nil -} - func applyPatchesToRequest(ctx context.Context, req *runtimehooksv1.CanUpdateMachineRequest, resp *runtimehooksv1.CanUpdateMachineResponse) error { if resp.MachinePatch.IsDefined() { - if err := applyPatchToMachine(ctx, &req.Current.Machine, resp.MachinePatch); err != nil { + if err := patch.ApplyPatchToTypedObject(ctx, &req.Current.Machine, resp.MachinePatch, "spec"); err != nil { return err } } if resp.BootstrapConfigPatch.IsDefined() { - if _, err := applyPatchToObject(ctx, &req.Current.BootstrapConfig, resp.BootstrapConfigPatch); err != nil { + if _, err := patch.ApplyPatchToObject(ctx, &req.Current.BootstrapConfig, resp.BootstrapConfigPatch, "spec"); err != nil { return err } } if resp.InfrastructureMachinePatch.IsDefined() { - if _, err := applyPatchToObject(ctx, &req.Current.InfrastructureMachine, resp.InfrastructureMachinePatch); err != nil { + if _, err := patch.ApplyPatchToObject(ctx, &req.Current.InfrastructureMachine, resp.InfrastructureMachinePatch, "spec"); err != nil { return err } } @@ -300,98 +272,6 @@ func applyPatchesToRequest(ctx context.Context, req *runtimehooksv1.CanUpdateMac return nil } -func applyPatchToMachine(ctx context.Context, currentMachine *clusterv1.Machine, machinePath runtimehooksv1.Patch) error { - // Note: Machine needs special handling because it is not a runtime.RawExtension. Simply converting it here to - // a runtime.RawExtension so we can avoid making the code in applyPatchToObject more complex. - currentMachineRaw, err := convertToRawExtension(currentMachine) - if err != nil { - return err - } - - machineChanged, err := applyPatchToObject(ctx, ¤tMachineRaw, machinePath) - if err != nil { - return err - } - - if !machineChanged { - return nil - } - - // Note: json.Unmarshal can't be used directly on *currentMachine as json.Unmarshal does not unset fields. - patchedCurrentMachine := &clusterv1.Machine{} - if err := json.Unmarshal(currentMachineRaw.Raw, patchedCurrentMachine); err != nil { - return err - } - *currentMachine = *patchedCurrentMachine - return nil -} - -// applyPatchToObject applies the patch to the obj. -// Note: This is following the same general structure that is used in the applyPatchToRequest func in -// internal/controllers/topology/cluster/patches/engine.go. -func applyPatchToObject(ctx context.Context, obj *runtime.RawExtension, patch runtimehooksv1.Patch) (objChanged bool, reterr error) { - log := ctrl.LoggerFrom(ctx) - - if patch.PatchType == "" { - return false, errors.Errorf("failed to apply patch: patchType is not set") - } - - defer func() { - if r := recover(); r != nil { - log.Info(fmt.Sprintf("Observed a panic when applying patch: %v\n%s", r, string(debug.Stack()))) - reterr = kerrors.NewAggregate([]error{reterr, fmt.Errorf("observed a panic when applying patch: %v", r)}) - } - }() - - // Create a copy of obj.Raw. - // The patches will be applied to the copy and then only spec changes will be copied back to the obj. - patchedObject := bytes.Clone(obj.Raw) - var err error - - switch patch.PatchType { - case runtimehooksv1.JSONPatchType: - log.V(5).Info("Accumulating JSON patch", "patch", string(patch.Patch)) - jsonPatch, err := jsonpatch.DecodePatch(patch.Patch) - if err != nil { - log.Error(err, "Failed to apply patch: error decoding json patch (RFC6902)", "patch", string(patch.Patch)) - return false, errors.Wrap(err, "failed to apply patch: error decoding json patch (RFC6902)") - } - - if len(jsonPatch) == 0 { - // Return if there are no patches, nothing to do. - return false, nil - } - - patchedObject, err = jsonPatch.Apply(patchedObject) - if err != nil { - log.Error(err, "Failed to apply patch: error applying json patch (RFC6902)", "patch", string(patch.Patch)) - return false, errors.Wrap(err, "failed to apply patch: error applying json patch (RFC6902)") - } - case runtimehooksv1.JSONMergePatchType: - if len(patch.Patch) == 0 || bytes.Equal(patch.Patch, []byte("{}")) { - // Return if there are no patches, nothing to do. - return false, nil - } - - log.V(5).Info("Accumulating JSON merge patch", "patch", string(patch.Patch)) - patchedObject, err = jsonpatch.MergePatch(patchedObject, patch.Patch) - if err != nil { - log.Error(err, "Failed to apply patch: error applying json merge patch (RFC7386)", "patch", string(patch.Patch)) - return false, errors.Wrap(err, "failed to apply patch: error applying json merge patch (RFC7386)") - } - default: - return false, errors.Errorf("failed to apply patch: unknown patchType %s", patch.PatchType) - } - - // Overwrite the spec of obj with the spec of the patchedObject, - // to ensure that we only pick up changes to the spec. - if err := patchutil.PatchSpec(obj, patchedObject); err != nil { - return false, errors.Wrap(err, "failed to apply patch to object") - } - - return true, nil -} - func matchesMachine(req *runtimehooksv1.CanUpdateMachineRequest) (bool, []string, error) { var reasons []string match, diff, err := matchesMachineSpec(&req.Current.Machine, &req.Desired.Machine) diff --git a/controlplane/kubeadm/internal/controllers/inplace_canupdatemachine_test.go b/controlplane/kubeadm/internal/controllers/inplace_canupdatemachine_test.go index 71f15b12b72d..7662899f11fe 100644 --- a/controlplane/kubeadm/internal/controllers/inplace_canupdatemachine_test.go +++ b/controlplane/kubeadm/internal/controllers/inplace_canupdatemachine_test.go @@ -42,6 +42,7 @@ import ( "sigs.k8s.io/cluster-api/feature" fakeruntimeclient "sigs.k8s.io/cluster-api/internal/runtime/client/fake" "sigs.k8s.io/cluster-api/internal/util/compare" + "sigs.k8s.io/cluster-api/internal/util/patch" "sigs.k8s.io/cluster-api/internal/util/ssa" "sigs.k8s.io/cluster-api/util/test/builder" ) @@ -104,7 +105,7 @@ func Test_canUpdateMachine(t *testing.T) { canUpdateMachineGVH: {"test-update-extension-1", "test-update-extension-2"}, }, wantError: true, - wantErrorMessage: "found multiple CanUpdateMachine hooks (test-update-extension-1,test-update-extension-2) (more than one is not supported yet)", + wantErrorMessage: "found multiple CanUpdateMachine hooks (test-update-extension-1,test-update-extension-2): only one hook is supported", }, { name: "Return false if canExtensionsUpdateMachine returns false", @@ -518,7 +519,7 @@ func validateCanUpdateMachineRequests(currentMachine *clusterv1.Machine, machine mutator(currentInfraMachine) } currentInfraMachineBytes, _ := json.Marshal(currentInfraMachine) - reqCurrentInfraMachineBytes := bytes.TrimSuffix(req.Current.InfrastructureMachine.Raw, []byte("\n")) // Note: Somehow PatchSpec introduces a trailing \n. + reqCurrentInfraMachineBytes := bytes.TrimSuffix(req.Current.InfrastructureMachine.Raw, []byte("\n")) // Note: Somehow Patch introduces a trailing \n. if d := diff(reqCurrentInfraMachineBytes, currentInfraMachineBytes); d != "" { return fmt.Errorf("expected currentInfraMachine to be equal, got diff: %s", d) } @@ -1174,7 +1175,7 @@ func diff(a, b any) string { } func mustConvertToRawExtension(object runtime.Object) runtime.RawExtension { - raw, err := convertToRawExtension(object) + raw, err := patch.ConvertToRawExtension(object) if err != nil { panic(err) } diff --git a/docs/book/src/tasks/experimental-features/runtime-sdk/implement-extensions.md b/docs/book/src/tasks/experimental-features/runtime-sdk/implement-extensions.md index f07abb9664b2..82c8024ddc96 100644 --- a/docs/book/src/tasks/experimental-features/runtime-sdk/implement-extensions.md +++ b/docs/book/src/tasks/experimental-features/runtime-sdk/implement-extensions.md @@ -304,7 +304,7 @@ created, the extension will detect the associated service and discover the assoc check the status of the ExtensionConfig. Below is an example of `ExtensionConfig` - ```yaml -apiVersion: runtime.cluster.x-k8s.io/v1alpha1 +apiVersion: runtime.cluster.x-k8s.io/v1beta2 kind: ExtensionConfig metadata: annotations: diff --git a/docs/proposals/20220221-runtime-SDK.md b/docs/proposals/20220221-runtime-SDK.md index 1e719bf99968..ac008d094472 100644 --- a/docs/proposals/20220221-runtime-SDK.md +++ b/docs/proposals/20220221-runtime-SDK.md @@ -296,7 +296,7 @@ behavior is introduced by this proposal: The Cluster administrator is required to register available Runtime Extension server using the following CR: ```yaml -apiVersion: runtime.cluster.x-k8s.io/v1alpha1 +apiVersion: runtime.cluster.x-k8s.io/v1beta2 kind: ExtensionConfig metadata: name: "my-amazing-extensions" @@ -326,7 +326,7 @@ The `namespaceSelector` will enable targeting of a subset of Clusters. ```yaml -apiVersion: runtime.cluster.x-k8s.io/v1alpha1 +apiVersion: runtime.cluster.x-k8s.io/v1beta2 kind: ExtensionConfig metadata: name: "my-amazing-extensions" @@ -367,8 +367,7 @@ dependencies among Runtime Extensions, being it modeled with something similar t [systemd unit options](https://www.freedesktop.org/software/systemd/man/systemd.unit.html) or alternative approaches. The main reason behind that is that such type of feature introduces complexity and creates "pet" like relations across -components making the overall system more fragile. This is also consistent with the [avoid dependencies](#avoid-dependencies) -recommendation above. +components making the overall system more fragile. This is also consistent with the avoid dependencies recommendation. ## Runtime Hooks developer guide (CAPI internals) diff --git a/internal/controllers/machine/machine_controller_inplace_update.go b/internal/controllers/machine/machine_controller_inplace_update.go index ea1733ccd359..0070b4d11862 100644 --- a/internal/controllers/machine/machine_controller_inplace_update.go +++ b/internal/controllers/machine/machine_controller_inplace_update.go @@ -19,6 +19,7 @@ package machine import ( "context" "fmt" + "strings" "time" "github.com/pkg/errors" @@ -149,7 +150,7 @@ func (r *Reconciler) callUpdateMachineHook(ctx context.Context, s *scope) (ctrl. } if len(extensions) > 1 { - return ctrl.Result{}, "", errors.Errorf("multiple extensions registered for UpdateMachine hook: only one extension is supported, found %d extensions: %v", len(extensions), extensions) + return ctrl.Result{}, "", errors.Errorf("found multiple UpdateMachine hooks (%s): only one hook is supported", strings.Join(extensions, ",")) } // Note: When building request message, dropping status; Runtime extension should treat UpdateMachine diff --git a/internal/controllers/machine/machine_controller_inplace_update_test.go b/internal/controllers/machine/machine_controller_inplace_update_test.go index 05e2a55d9898..5f574eb41265 100644 --- a/internal/controllers/machine/machine_controller_inplace_update_test.go +++ b/internal/controllers/machine/machine_controller_inplace_update_test.go @@ -367,13 +367,8 @@ func TestCallUpdateMachineHook(t *testing.T) { Build() return &Reconciler{RuntimeClient: runtimeClient}, &scope{machine: newTestMachine(), infraMachine: newTestUnstructured("GenericInfrastructureMachine", "infrastructure.cluster.x-k8s.io/v1beta2", "infra")} }, - wantErr: true, - wantErrSubstrings: []string{ - "multiple extensions registered for UpdateMachine hook", - "only one extension is supported", - "ext-a", - "ext-b", - }, + wantErr: true, + wantErrSubstrings: []string{"found multiple UpdateMachine hooks (ext-a,ext-b): only one hook is supported"}, }, { name: "fails when hook invocation returns error", diff --git a/internal/controllers/machinedeployment/machinedeployment_canupdatemachineset.go b/internal/controllers/machinedeployment/machinedeployment_canupdatemachineset.go new file mode 100644 index 000000000000..a2dc9370ee62 --- /dev/null +++ b/internal/controllers/machinedeployment/machinedeployment_canupdatemachineset.go @@ -0,0 +1,351 @@ +/* +Copyright 2025 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package machinedeployment + +import ( + "context" + "fmt" + "strings" + + "github.com/pkg/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/klog/v2" + ctrl "sigs.k8s.io/controller-runtime" + + clusterv1 "sigs.k8s.io/cluster-api/api/core/v1beta2" + runtimehooksv1 "sigs.k8s.io/cluster-api/api/runtime/hooks/v1alpha1" + "sigs.k8s.io/cluster-api/controllers/external" + "sigs.k8s.io/cluster-api/internal/util/compare" + "sigs.k8s.io/cluster-api/internal/util/patch" +) + +func (p *rolloutPlanner) canUpdateMachineSetInPlace(ctx context.Context, oldMS, newMS *clusterv1.MachineSet) (bool, error) { + if p.overrideCanUpdateMachineSetInPlace != nil { + return p.overrideCanUpdateMachineSetInPlace(ctx, oldMS, newMS) + } + + log := ctrl.LoggerFrom(ctx) + + templateObjects, err := p.getTemplateObjects(ctx, oldMS, newMS) + if err != nil { + return false, err + } + + // MachineSet cannot be updated in-place if the getTemplateObjects func was not able to get all InfraMachineTemplates. + if templateObjects.CurrentInfraMachineTemplate == nil || + templateObjects.DesiredInfraMachineTemplate == nil { + return false, nil + } + // MachineSet cannot be updated in-place if the BootstrapConfigTemplate is set on the oldMS but not on the newMS or vice versa. + if (templateObjects.CurrentBootstrapConfigTemplate == nil && templateObjects.DesiredBootstrapConfigTemplate != nil) || + (templateObjects.CurrentBootstrapConfigTemplate != nil && templateObjects.DesiredBootstrapConfigTemplate == nil) { + return false, nil + } + + extensionHandlers, err := p.RuntimeClient.GetAllExtensions(ctx, runtimehooksv1.CanUpdateMachineSet, oldMS) + if err != nil { + return false, err + } + // MachineSet cannot be updated in-place if no CanUpdateMachineSet extensions are registered. + if len(extensionHandlers) == 0 { + return false, nil + } + if len(extensionHandlers) > 1 { + return false, errors.Errorf("found multiple CanUpdateMachineSet hooks (%s): only one hook is supported", strings.Join(extensionHandlers, ",")) + } + + canUpdateMachineSet, reasons, err := p.canExtensionsUpdateMachineSet(ctx, oldMS, newMS, templateObjects, extensionHandlers) + if err != nil { + return false, err + } + if !canUpdateMachineSet { + log.Info(fmt.Sprintf("MachineSet cannot be updated in-place by extensions: %s", strings.Join(reasons, ",")), "MachineSet", klog.KObj(oldMS)) + return false, nil + } + return true, nil +} + +// canExtensionsUpdateMachineSet calls CanUpdateMachineSet extensions to decide if a MachineSet can be updated in-place. +// Note: This is following the same general structure that is used in the Apply func in +// internal/controllers/topology/cluster/patches/engine.go. +func (p *rolloutPlanner) canExtensionsUpdateMachineSet(ctx context.Context, oldMS, newMS *clusterv1.MachineSet, templateObjects *templateObjects, extensionHandlers []string) (bool, []string, error) { + if p.overrideCanExtensionsUpdateMachineSet != nil { + return p.overrideCanExtensionsUpdateMachineSet(ctx, oldMS, newMS, templateObjects, extensionHandlers) + } + + log := ctrl.LoggerFrom(ctx) + + // Create the CanUpdateMachineSet request. + req, err := createRequest(oldMS, newMS, templateObjects) + if err != nil { + return false, nil, errors.Wrapf(err, "failed to generate CanUpdateMachineSet request") + } + + var reasons []string + for _, extensionHandler := range extensionHandlers { + // Call CanUpdateMachineSet extension. + resp := &runtimehooksv1.CanUpdateMachineSetResponse{} + if err := p.RuntimeClient.CallExtension(ctx, runtimehooksv1.CanUpdateMachineSet, oldMS, extensionHandler, req, resp); err != nil { + return false, nil, err + } + + // Apply patches from the CanUpdateMachineSet response to the request. + if err := applyPatchesToRequest(ctx, req, resp); err != nil { + return false, nil, errors.Wrapf(err, "failed to apply patches from extension %s to the CanUpdateMachineSet request", extensionHandler) + } + + // Check if current and desired objects are now matching. + var matches bool + matches, reasons, err = matchesMachineSet(req) + if err != nil { + return false, nil, errors.Wrapf(err, "failed to compare current and desired objects after calling extension %s", extensionHandler) + } + if matches { + return true, nil, nil + } + log.V(5).Info(fmt.Sprintf("MachineSet cannot be updated in-place yet after calling extension %s: %s", extensionHandler, strings.Join(reasons, ",")), "MachineSet", klog.KObj(oldMS)) + } + + return false, reasons, nil +} + +func createRequest(oldMS, newMS *clusterv1.MachineSet, templateObjects *templateObjects) (*runtimehooksv1.CanUpdateMachineSetRequest, error) { + // DeepCopy MachineSets to avoid mutations. + currentMachineSetForDiff := oldMS.DeepCopy() + currentBootstrapConfigTemplateForDiff := templateObjects.CurrentBootstrapConfigTemplate + currentInfraMachineTemplateForDiff := templateObjects.CurrentInfraMachineTemplate + + desiredMachineSetForDiff := newMS.DeepCopy() + desiredBootstrapConfigTemplateForDiff := templateObjects.DesiredBootstrapConfigTemplate + desiredInfraMachineTemplateForDiff := templateObjects.DesiredInfraMachineTemplate + + // Cleanup objects and create request. + req := &runtimehooksv1.CanUpdateMachineSetRequest{ + Current: runtimehooksv1.CanUpdateMachineSetRequestObjects{ + MachineSet: *cleanupMachineSet(currentMachineSetForDiff), + }, + Desired: runtimehooksv1.CanUpdateMachineSetRequestObjects{ + MachineSet: *cleanupMachineSet(desiredMachineSetForDiff), + }, + } + var err error + if currentBootstrapConfigTemplateForDiff != nil { + req.Current.BootstrapConfigTemplate, err = patch.ConvertToRawExtension(cleanupUnstructured(currentBootstrapConfigTemplateForDiff)) + if err != nil { + return nil, err + } + } + if desiredBootstrapConfigTemplateForDiff != nil { + req.Desired.BootstrapConfigTemplate, err = patch.ConvertToRawExtension(cleanupUnstructured(desiredBootstrapConfigTemplateForDiff)) + if err != nil { + return nil, err + } + } + req.Current.InfrastructureMachineTemplate, err = patch.ConvertToRawExtension(cleanupUnstructured(currentInfraMachineTemplateForDiff)) + if err != nil { + return nil, err + } + req.Desired.InfrastructureMachineTemplate, err = patch.ConvertToRawExtension(cleanupUnstructured(desiredInfraMachineTemplateForDiff)) + if err != nil { + return nil, err + } + + return req, nil +} + +func cleanupMachineSet(machine *clusterv1.MachineSet) *clusterv1.MachineSet { + return &clusterv1.MachineSet{ + // Set GVK because object is later marshalled with json.Marshal. + TypeMeta: metav1.TypeMeta{ + APIVersion: clusterv1.GroupVersion.String(), + Kind: "MachineSet", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: machine.Name, + Namespace: machine.Namespace, + Labels: machine.Labels, + Annotations: machine.Annotations, + }, + Spec: *machine.Spec.DeepCopy(), + } +} + +func cleanupUnstructured(u *unstructured.Unstructured) *unstructured.Unstructured { + cleanedUpU := &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": u.GetAPIVersion(), + "kind": u.GetKind(), + "spec": u.Object["spec"], + }, + } + cleanedUpU.SetName(u.GetName()) + cleanedUpU.SetNamespace(u.GetNamespace()) + cleanedUpU.SetLabels(u.GetLabels()) + cleanedUpU.SetAnnotations(u.GetAnnotations()) + return cleanedUpU +} + +func applyPatchesToRequest(ctx context.Context, req *runtimehooksv1.CanUpdateMachineSetRequest, resp *runtimehooksv1.CanUpdateMachineSetResponse) error { + if resp.MachineSetPatch.IsDefined() { + if err := patch.ApplyPatchToTypedObject(ctx, &req.Current.MachineSet, resp.MachineSetPatch, "spec.template.spec"); err != nil { + return err + } + } + + if resp.BootstrapConfigTemplatePatch.IsDefined() && req.Current.BootstrapConfigTemplate.Object != nil { + if _, err := patch.ApplyPatchToObject(ctx, &req.Current.BootstrapConfigTemplate, resp.BootstrapConfigTemplatePatch, "spec.template.spec"); err != nil { + return err + } + } + + if resp.InfrastructureMachineTemplatePatch.IsDefined() { + if _, err := patch.ApplyPatchToObject(ctx, &req.Current.InfrastructureMachineTemplate, resp.InfrastructureMachineTemplatePatch, "spec.template.spec"); err != nil { + return err + } + } + + return nil +} + +func matchesMachineSet(req *runtimehooksv1.CanUpdateMachineSetRequest) (bool, []string, error) { + var reasons []string + match, diff, err := matchesMachineSetSpec(&req.Current.MachineSet, &req.Desired.MachineSet) + if err != nil { + return false, nil, errors.Wrapf(err, "failed to match MachineSet") + } + if !match { + reasons = append(reasons, fmt.Sprintf("MachineSet cannot be updated in-place: %s", diff)) + } + if req.Current.BootstrapConfigTemplate.Object != nil && req.Desired.BootstrapConfigTemplate.Object != nil { + match, diff, err = matchesUnstructuredSpec(req.Current.BootstrapConfigTemplate, req.Desired.BootstrapConfigTemplate) + if err != nil { + return false, nil, errors.Wrapf(err, "failed to match BootstrapConfigTemplate") + } + if !match { + reasons = append(reasons, fmt.Sprintf("BootstrapConfigTemplate cannot be updated in-place: %s", diff)) + } + } + match, diff, err = matchesUnstructuredSpec(req.Current.InfrastructureMachineTemplate, req.Desired.InfrastructureMachineTemplate) + if err != nil { + return false, nil, errors.Wrapf(err, "failed to match %s", req.Current.InfrastructureMachineTemplate.Object.GetObjectKind().GroupVersionKind().Kind) + } + if !match { + reasons = append(reasons, fmt.Sprintf("%s cannot be updated in-place: %s", req.Current.InfrastructureMachineTemplate.Object.GetObjectKind().GroupVersionKind().Kind, diff)) + } + + if len(reasons) > 0 { + return false, reasons, nil + } + + return true, nil, nil +} + +func matchesMachineSetSpec(patched, desired *clusterv1.MachineSet) (equal bool, diff string, matchErr error) { + // Note: Wrapping MachineSet specs in a MachineSet for proper formatting of the diff. + return compare.Diff( + &clusterv1.MachineSet{ + Spec: clusterv1.MachineSetSpec{ + Template: clusterv1.MachineTemplateSpec{ + Spec: patched.Spec.Template.Spec, + }, + }, + }, + &clusterv1.MachineSet{ + Spec: clusterv1.MachineSetSpec{ + Template: clusterv1.MachineTemplateSpec{ + Spec: desired.Spec.Template.Spec, + }, + }, + }, + ) +} + +func matchesUnstructuredSpec(patched, desired runtime.RawExtension) (equal bool, diff string, matchErr error) { + // Note: Both patched and desired objects are always Unstructured as createRequest and + // applyPatchToObject are always setting objects as Unstructured. + patchedUnstructured, ok := patched.Object.(*unstructured.Unstructured) + if !ok { + return false, "", errors.Errorf("patched object is not an Unstructured") + } + desiredUnstructured, ok := desired.Object.(*unstructured.Unstructured) + if !ok { + return false, "", errors.Errorf("desired object is not an Unstructured") + } + + // Note: Wrapping Unstructured spec.template.specs in an Unstructured for proper formatting of the diff. + patchedSpecTemplateSpec, foundPatched, err := unstructured.NestedFieldNoCopy(patchedUnstructured.Object, "spec", "template", "spec") + if err != nil { + return false, "", errors.Errorf("could not read spec.template.spec from patched object") + } + desiredSpecTemplateSpec, foundDesired, err := unstructured.NestedFieldNoCopy(desiredUnstructured.Object, "spec", "template", "spec") + if err != nil { + return false, "", errors.Errorf("could not read spec.template.spec from desired object") + } + + cleanedUpPatchedUnstructured := &unstructured.Unstructured{Object: map[string]interface{}{}} + if foundPatched { + if err := unstructured.SetNestedField(cleanedUpPatchedUnstructured.Object, patchedSpecTemplateSpec, "spec", "template", "spec"); err != nil { + return false, "", errors.Errorf("could not write spec.template.spec to patched object for comparison") + } + } + cleanedUpDesiredUnstructured := &unstructured.Unstructured{Object: map[string]interface{}{}} + if foundDesired { + if err := unstructured.SetNestedField(cleanedUpDesiredUnstructured.Object, desiredSpecTemplateSpec, "spec", "template", "spec"); err != nil { + return false, "", errors.Errorf("could not write spec.template.spec to desired object for comparison") + } + } + + return compare.Diff(cleanedUpPatchedUnstructured, cleanedUpDesiredUnstructured) +} + +type templateObjects struct { + CurrentInfraMachineTemplate *unstructured.Unstructured + DesiredInfraMachineTemplate *unstructured.Unstructured + CurrentBootstrapConfigTemplate *unstructured.Unstructured + DesiredBootstrapConfigTemplate *unstructured.Unstructured +} + +func (p *rolloutPlanner) getTemplateObjects(ctx context.Context, oldMS, newMS *clusterv1.MachineSet) (*templateObjects, error) { + templateObjects := &templateObjects{} + var err error + + templateObjects.CurrentInfraMachineTemplate, err = external.GetObjectFromContractVersionedRef(ctx, p.Client, oldMS.Spec.Template.Spec.InfrastructureRef, oldMS.Namespace) + if err != nil { + return nil, errors.Wrapf(err, "failed to get InfrastructureMachineTemplate from MachineSet %s", oldMS.Name) + } + templateObjects.DesiredInfraMachineTemplate, err = external.GetObjectFromContractVersionedRef(ctx, p.Client, newMS.Spec.Template.Spec.InfrastructureRef, newMS.Namespace) + if err != nil { + return nil, errors.Wrapf(err, "failed to get InfrastructureMachineTemplate from MachineSet %s", newMS.Name) + } + + if oldMS.Spec.Template.Spec.Bootstrap.ConfigRef.IsDefined() { + templateObjects.CurrentBootstrapConfigTemplate, err = external.GetObjectFromContractVersionedRef(ctx, p.Client, oldMS.Spec.Template.Spec.Bootstrap.ConfigRef, oldMS.Namespace) + if err != nil { + return nil, errors.Wrapf(err, "failed to get BootstrapConfigTemplate from MachineSet %s", oldMS.Name) + } + } + if newMS.Spec.Template.Spec.Bootstrap.ConfigRef.IsDefined() { + templateObjects.DesiredBootstrapConfigTemplate, err = external.GetObjectFromContractVersionedRef(ctx, p.Client, newMS.Spec.Template.Spec.Bootstrap.ConfigRef, newMS.Namespace) + if err != nil { + return nil, errors.Wrapf(err, "failed to get BootstrapConfigTemplate from MachineSet %s", newMS.Name) + } + } + + return templateObjects, nil +} diff --git a/internal/controllers/machinedeployment/machinedeployment_canupdatemachineset_test.go b/internal/controllers/machinedeployment/machinedeployment_canupdatemachineset_test.go new file mode 100644 index 000000000000..878202e30867 --- /dev/null +++ b/internal/controllers/machinedeployment/machinedeployment_canupdatemachineset_test.go @@ -0,0 +1,930 @@ +/* +Copyright 2025 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package machinedeployment + +import ( + "bytes" + "context" + "encoding/json" + "fmt" + "testing" + + . "github.com/onsi/gomega" + "github.com/pkg/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + + clusterv1 "sigs.k8s.io/cluster-api/api/core/v1beta2" + runtimehooksv1 "sigs.k8s.io/cluster-api/api/runtime/hooks/v1alpha1" + runtimecatalog "sigs.k8s.io/cluster-api/exp/runtime/catalog" + "sigs.k8s.io/cluster-api/internal/contract" + fakeruntimeclient "sigs.k8s.io/cluster-api/internal/runtime/client/fake" + "sigs.k8s.io/cluster-api/internal/util/compare" + "sigs.k8s.io/cluster-api/internal/util/patch" + "sigs.k8s.io/cluster-api/util/test/builder" +) + +func Test_canUpdateMachine(t *testing.T) { + ns := metav1.NamespaceDefault + oldMS := &clusterv1.MachineSet{ + ObjectMeta: metav1.ObjectMeta{ + Name: "old-machineset", + Namespace: ns, + }, + } + newMS := &clusterv1.MachineSet{ + ObjectMeta: metav1.ObjectMeta{ + Name: "new-machineset", + Namespace: ns, + }, + } + oldMSInfrastructureMachineTemplate := builder.InfrastructureMachineTemplate(ns, "infrastructure-machine-template-1").Build() + newMSInfrastructureMachineTemplate := builder.InfrastructureMachineTemplate(ns, "infrastructure-machine-template-2").Build() + oldMSBootstrapConfigTemplate := builder.BootstrapTemplate(ns, "bootstrap-config-template-1").Build() + newMSBootstrapConfigTemplate := builder.BootstrapTemplate(ns, "bootstrap-config-template-2").Build() + + catalog := runtimecatalog.New() + _ = runtimehooksv1.AddToCatalog(catalog) + canUpdateMachineSetGVH, err := catalog.GroupVersionHook(runtimehooksv1.CanUpdateMachineSet) + if err != nil { + panic("unable to compute GVH") + } + + tests := []struct { + name string + oldMS *clusterv1.MachineSet + newMS *clusterv1.MachineSet + oldMSInfrastructureMachineTemplate *unstructured.Unstructured + newMSInfrastructureMachineTemplate *unstructured.Unstructured + oldMSBootstrapConfigTemplate *unstructured.Unstructured + newMSBootstrapConfigTemplate *unstructured.Unstructured + canExtensionsUpdateMachineSetFunc func(ctx context.Context, oldMS, newMS *clusterv1.MachineSet, templateObjects *templateObjects, extensionHandlers []string) (bool, []string, error) + getAllExtensionsResponses map[runtimecatalog.GroupVersionHook][]string + wantCanExtensionsUpdateMachineSetCalled bool + wantCanUpdateMachineSet bool + wantError bool + wantErrorMessage string + }{ + { + name: "Return error if oldMS infrastructureRef is not set", + oldMS: oldMS, + newMS: newMS, + wantError: true, + wantErrorMessage: "failed to get InfrastructureMachineTemplate from MachineSet old-machineset: cannot get object - object reference not set", + }, + { + name: "Return error if newMS infrastructureRef is not set", + oldMS: oldMS, + newMS: newMS, + oldMSInfrastructureMachineTemplate: oldMSInfrastructureMachineTemplate, + wantError: true, + wantErrorMessage: "failed to get InfrastructureMachineTemplate from MachineSet new-machineset: cannot get object - object reference not set", + }, + { + name: "Return false if bootstrap.configRef is only set on oldMS", + oldMS: oldMS, + newMS: newMS, + oldMSInfrastructureMachineTemplate: oldMSInfrastructureMachineTemplate, + newMSInfrastructureMachineTemplate: newMSInfrastructureMachineTemplate, + oldMSBootstrapConfigTemplate: oldMSBootstrapConfigTemplate, + wantCanUpdateMachineSet: false, + }, + { + name: "Return false if bootstrap.configRef is only set on newMS", + oldMS: oldMS, + newMS: newMS, + oldMSInfrastructureMachineTemplate: oldMSInfrastructureMachineTemplate, + newMSInfrastructureMachineTemplate: newMSInfrastructureMachineTemplate, + newMSBootstrapConfigTemplate: newMSBootstrapConfigTemplate, + wantCanUpdateMachineSet: false, + }, + { + name: "Return false if no CanUpdateMachineSet extensions registered", + oldMS: oldMS, + newMS: newMS, + oldMSInfrastructureMachineTemplate: oldMSInfrastructureMachineTemplate, + newMSInfrastructureMachineTemplate: newMSInfrastructureMachineTemplate, + oldMSBootstrapConfigTemplate: oldMSBootstrapConfigTemplate, + newMSBootstrapConfigTemplate: newMSBootstrapConfigTemplate, + getAllExtensionsResponses: map[runtimecatalog.GroupVersionHook][]string{}, + wantCanUpdateMachineSet: false, + }, + { + name: "Return error if more than one CanUpdateMachineSet extensions registered", + oldMS: oldMS, + newMS: newMS, + oldMSInfrastructureMachineTemplate: oldMSInfrastructureMachineTemplate, + newMSInfrastructureMachineTemplate: newMSInfrastructureMachineTemplate, + oldMSBootstrapConfigTemplate: oldMSBootstrapConfigTemplate, + newMSBootstrapConfigTemplate: newMSBootstrapConfigTemplate, + getAllExtensionsResponses: map[runtimecatalog.GroupVersionHook][]string{ + canUpdateMachineSetGVH: {"test-update-extension-1", "test-update-extension-2"}, + }, + wantError: true, + wantErrorMessage: "found multiple CanUpdateMachineSet hooks (test-update-extension-1,test-update-extension-2): only one hook is supported", + }, + { + name: "Return false if canExtensionsUpdateMachine returns false", + oldMS: oldMS, + newMS: newMS, + oldMSInfrastructureMachineTemplate: oldMSInfrastructureMachineTemplate, + newMSInfrastructureMachineTemplate: newMSInfrastructureMachineTemplate, + oldMSBootstrapConfigTemplate: oldMSBootstrapConfigTemplate, + newMSBootstrapConfigTemplate: newMSBootstrapConfigTemplate, + getAllExtensionsResponses: map[runtimecatalog.GroupVersionHook][]string{ + canUpdateMachineSetGVH: {"test-update-extension"}, + }, + canExtensionsUpdateMachineSetFunc: func(_ context.Context, _, _ *clusterv1.MachineSet, _ *templateObjects, extensionHandlers []string) (bool, []string, error) { + if len(extensionHandlers) != 1 || extensionHandlers[0] != "test-update-extension" { + return false, nil, errors.Errorf("unexpected error") + } + return false, []string{"can not update"}, nil + }, + wantCanExtensionsUpdateMachineSetCalled: true, + wantCanUpdateMachineSet: false, + }, + { + name: "Return true if canExtensionsUpdateMachine returns true", + oldMS: oldMS, + newMS: newMS, + oldMSInfrastructureMachineTemplate: oldMSInfrastructureMachineTemplate, + newMSInfrastructureMachineTemplate: newMSInfrastructureMachineTemplate, + oldMSBootstrapConfigTemplate: oldMSBootstrapConfigTemplate, + newMSBootstrapConfigTemplate: newMSBootstrapConfigTemplate, + getAllExtensionsResponses: map[runtimecatalog.GroupVersionHook][]string{ + canUpdateMachineSetGVH: {"test-update-extension"}, + }, + canExtensionsUpdateMachineSetFunc: func(_ context.Context, _, _ *clusterv1.MachineSet, _ *templateObjects, extensionHandlers []string) (bool, []string, error) { + if len(extensionHandlers) != 1 || extensionHandlers[0] != "test-update-extension" { + return false, nil, errors.Errorf("unexpected error") + } + return true, nil, nil + }, + wantCanExtensionsUpdateMachineSetCalled: true, + wantCanUpdateMachineSet: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + + runtimeClient := fakeruntimeclient.NewRuntimeClientBuilder(). + WithCatalog(catalog). + WithGetAllExtensionResponses(tt.getAllExtensionsResponses). + Build() + + oldMS := tt.oldMS.DeepCopy() + newMS := tt.newMS.DeepCopy() + + objs := []client.Object{ + builder.GenericInfrastructureMachineTemplateCRD, + builder.GenericBootstrapConfigTemplateCRD, + } + if tt.oldMSInfrastructureMachineTemplate != nil { + objs = append(objs, tt.oldMSInfrastructureMachineTemplate) + oldMS.Spec.Template.Spec.InfrastructureRef = contract.ObjToContractVersionedObjectReference(tt.oldMSInfrastructureMachineTemplate) + } + if tt.newMSInfrastructureMachineTemplate != nil { + objs = append(objs, tt.newMSInfrastructureMachineTemplate) + newMS.Spec.Template.Spec.InfrastructureRef = contract.ObjToContractVersionedObjectReference(tt.newMSInfrastructureMachineTemplate) + } + if tt.oldMSBootstrapConfigTemplate != nil { + objs = append(objs, tt.oldMSBootstrapConfigTemplate) + oldMS.Spec.Template.Spec.Bootstrap.ConfigRef = contract.ObjToContractVersionedObjectReference(tt.oldMSBootstrapConfigTemplate) + } + if tt.newMSBootstrapConfigTemplate != nil { + objs = append(objs, tt.newMSBootstrapConfigTemplate) + newMS.Spec.Template.Spec.Bootstrap.ConfigRef = contract.ObjToContractVersionedObjectReference(tt.newMSBootstrapConfigTemplate) + } + + fakeClient := fake.NewClientBuilder().WithObjects(objs...).Build() + + var canExtensionsUpdateMachineSetCalled bool + p := &rolloutPlanner{ + RuntimeClient: runtimeClient, + Client: fakeClient, + overrideCanExtensionsUpdateMachineSet: func(ctx context.Context, oldMS, newMS *clusterv1.MachineSet, templateObjects *templateObjects, extensionHandlers []string) (bool, []string, error) { + canExtensionsUpdateMachineSetCalled = true + return tt.canExtensionsUpdateMachineSetFunc(ctx, oldMS, newMS, templateObjects, extensionHandlers) + }, + } + + canUpdateMachineSet, err := p.canUpdateMachineSetInPlace(ctx, oldMS, newMS) + if tt.wantError { + g.Expect(err).To(HaveOccurred()) + g.Expect(err.Error()).To(Equal(tt.wantErrorMessage)) + } else { + g.Expect(err).ToNot(HaveOccurred()) + } + g.Expect(canUpdateMachineSet).To(Equal(tt.wantCanUpdateMachineSet)) + + g.Expect(canExtensionsUpdateMachineSetCalled).To(Equal(tt.wantCanExtensionsUpdateMachineSetCalled), "canExtensionsUpdateMachineSetCalled: actual: %t expected: %t", canExtensionsUpdateMachineSetCalled, tt.wantCanExtensionsUpdateMachineSetCalled) + }) + } +} + +func Test_canExtensionsUpdateMachineSet(t *testing.T) { + currentMachineSet := &clusterv1.MachineSet{ + ObjectMeta: metav1.ObjectMeta{ + Name: "old-machineset", + Namespace: metav1.NamespaceDefault, + ResourceVersion: "1", // This is set so we can verify cleanupMachineSet later. + }, + Spec: clusterv1.MachineSetSpec{ + Template: clusterv1.MachineTemplateSpec{ + Spec: clusterv1.MachineSpec{ + Version: "v1.30.0", + }, + }, + }, + } + desiredMachineSet := currentMachineSet.DeepCopy() + desiredMachineSet.Name = "new-machineset" + desiredMachineSet.Spec.Template.Spec.Version = "v1.31.0" + + currentBootstrapConfigTemplate := &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": builder.BootstrapGroupVersion.String(), + "kind": builder.TestBootstrapConfigTemplateKind, + "metadata": map[string]interface{}{ + "name": "bootstrap-config-template-1", + "namespace": metav1.NamespaceDefault, + "resourceVersion": "2", // This is set so we can verify cleanupUnstructured later. + }, + "spec": map[string]interface{}{ + "template": map[string]interface{}{ + "spec": map[string]interface{}{ + "hello": "world BootstrapConfigTemplate", + }, + }, + }, + }, + } + desiredBootstrapConfigTemplate := currentBootstrapConfigTemplate.DeepCopy() + desiredBootstrapConfigTemplate.SetName("bootstrap-config-template-2") + _ = unstructured.SetNestedField(desiredBootstrapConfigTemplate.Object, "in-place updated world BootstrapConfigTemplate", "spec", "template", "spec", "hello") + + currentInfraMachineTemplate := &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": builder.InfrastructureGroupVersion.String(), + "kind": builder.TestInfrastructureMachineTemplateKind, + "metadata": map[string]interface{}{ + "name": "infrastructure-machine-template-1", + "namespace": metav1.NamespaceDefault, + "resourceVersion": "2", // This is set so we can verify cleanupUnstructured later. + }, + "spec": map[string]interface{}{ + "template": map[string]interface{}{ + "spec": map[string]interface{}{ + "hello": "world InfraMachineTemplate", + }, + }, + }, + }, + } + desiredInfraMachineTemplate := currentInfraMachineTemplate.DeepCopy() + desiredInfraMachineTemplate.SetName("infrastructure-machine-template-2") + _ = unstructured.SetNestedField(desiredInfraMachineTemplate.Object, "in-place updated world InfraMachineTemplate", "spec", "template", "spec", "hello") + + responseWithEmptyPatches := &runtimehooksv1.CanUpdateMachineSetResponse{ + CommonResponse: runtimehooksv1.CommonResponse{Status: runtimehooksv1.ResponseStatusSuccess}, + MachineSetPatch: runtimehooksv1.Patch{ + PatchType: runtimehooksv1.JSONPatchType, + Patch: []byte("[]"), + }, + InfrastructureMachineTemplatePatch: runtimehooksv1.Patch{ + PatchType: runtimehooksv1.JSONMergePatchType, + Patch: []byte{}, + }, + BootstrapConfigTemplatePatch: runtimehooksv1.Patch{ + PatchType: runtimehooksv1.JSONMergePatchType, + Patch: []byte("{}"), + }, + } + patchToUpdateMachineSet := runtimehooksv1.Patch{ + PatchType: runtimehooksv1.JSONPatchType, + Patch: []byte(`[{"op":"replace","path":"/spec/template/spec/version","value":"v1.31.0"}]`), + } + patchToUpdateBootstrapConfigTemplate := runtimehooksv1.Patch{ + PatchType: runtimehooksv1.JSONPatchType, + Patch: []byte(`[{"op":"replace","path":"/spec/template/spec/hello","value":"in-place updated world BootstrapConfigTemplate"}]`), + } + patchToUpdateInfraMachineTemplate := runtimehooksv1.Patch{ + PatchType: runtimehooksv1.JSONPatchType, + Patch: []byte(`[{"op":"replace","path":"/spec/template/spec/hello","value":"in-place updated world InfraMachineTemplate"}]`), + } + emptyPatch := runtimehooksv1.Patch{ + PatchType: runtimehooksv1.JSONMergePatchType, + Patch: []byte{}, + } + + tests := []struct { + name string + newMS *clusterv1.MachineSet + templateObjects *templateObjects + extensionHandlers []string + callExtensionResponses map[string]runtimehooksv1.ResponseObject + callExtensionExpectedChanges map[string]func(runtime.Object) + wantCanUpdateMachineSet bool + wantReasons []string + wantError bool + wantErrorMessage string + }{ + { + name: "Return true if current and desired objects are equal and no patches are returned", + // Note: canExtensionsUpdateMachineSet should never be called if the objects are equal, but this is a simple first test case. + newMS: currentMachineSet, + templateObjects: &templateObjects{ + CurrentInfraMachineTemplate: currentInfraMachineTemplate, + DesiredInfraMachineTemplate: currentInfraMachineTemplate, + CurrentBootstrapConfigTemplate: currentBootstrapConfigTemplate, + DesiredBootstrapConfigTemplate: currentBootstrapConfigTemplate, + }, + extensionHandlers: []string{"test-update-extension"}, + callExtensionResponses: map[string]runtimehooksv1.ResponseObject{ + "test-update-extension": responseWithEmptyPatches, + }, + wantCanUpdateMachineSet: true, + }, + { + name: "Return false if current and desired objects are not equal and no patches are returned", + newMS: desiredMachineSet, + templateObjects: &templateObjects{ + CurrentInfraMachineTemplate: currentInfraMachineTemplate, + DesiredInfraMachineTemplate: desiredInfraMachineTemplate, + CurrentBootstrapConfigTemplate: currentBootstrapConfigTemplate, + DesiredBootstrapConfigTemplate: desiredBootstrapConfigTemplate, + }, + extensionHandlers: []string{"test-update-extension"}, + callExtensionResponses: map[string]runtimehooksv1.ResponseObject{ + "test-update-extension": responseWithEmptyPatches, + }, + wantCanUpdateMachineSet: false, + wantReasons: []string{ + `MachineSet cannot be updated in-place: &v1beta2.MachineSet{ + TypeMeta: {}, + ObjectMeta: {}, + Spec: v1beta2.MachineSetSpec{ + ClusterName: "", + Replicas: nil, + Selector: {}, + Template: v1beta2.MachineTemplateSpec{ + ObjectMeta: {}, + Spec: v1beta2.MachineSpec{ + ClusterName: "", + Bootstrap: {}, + InfrastructureRef: {}, +- Version: "v1.30.0", ++ Version: "v1.31.0", + ProviderID: "", + FailureDomain: "", + ... // 3 identical fields + }, + }, + MachineNaming: {}, + Deletion: {}, + }, + Status: {}, + }`, + `BootstrapConfigTemplate cannot be updated in-place: &unstructured.Unstructured{ +- Object: map[string]any{ +- "spec": map[string]any{ +- "template": map[string]any{"spec": map[string]any{"hello": string("world BootstrapConfigTemplate")}}, +- }, +- }, ++ Object: map[string]any{ ++ "spec": map[string]any{ ++ "template": map[string]any{ ++ "spec": map[string]any{"hello": string("in-place updated world BootstrapConfigTemplate")}, ++ }, ++ }, ++ }, + }`, + `TestInfrastructureMachineTemplate cannot be updated in-place: &unstructured.Unstructured{ +- Object: map[string]any{ +- "spec": map[string]any{ +- "template": map[string]any{"spec": map[string]any{"hello": string("world InfraMachineTemplate")}}, +- }, +- }, ++ Object: map[string]any{ ++ "spec": map[string]any{ ++ "template": map[string]any{ ++ "spec": map[string]any{"hello": string("in-place updated world InfraMachineTemplate")}, ++ }, ++ }, ++ }, + }`, + }, + }, + { + name: "Return true if current and desired objects are not equal and patches are returned that account for all diffs", + newMS: desiredMachineSet, + templateObjects: &templateObjects{ + CurrentInfraMachineTemplate: currentInfraMachineTemplate, + DesiredInfraMachineTemplate: desiredInfraMachineTemplate, + CurrentBootstrapConfigTemplate: currentBootstrapConfigTemplate, + DesiredBootstrapConfigTemplate: desiredBootstrapConfigTemplate, + }, + extensionHandlers: []string{"test-update-extension"}, + callExtensionResponses: map[string]runtimehooksv1.ResponseObject{ + "test-update-extension": &runtimehooksv1.CanUpdateMachineSetResponse{ + CommonResponse: runtimehooksv1.CommonResponse{Status: runtimehooksv1.ResponseStatusSuccess}, + MachineSetPatch: patchToUpdateMachineSet, + InfrastructureMachineTemplatePatch: patchToUpdateInfraMachineTemplate, + BootstrapConfigTemplatePatch: patchToUpdateBootstrapConfigTemplate, + }, + }, + wantCanUpdateMachineSet: true, + }, + { + name: "Return true if current and desired objects are not equal and patches are returned that account for all diffs (multiple extensions)", + newMS: desiredMachineSet, + templateObjects: &templateObjects{ + CurrentInfraMachineTemplate: currentInfraMachineTemplate, + DesiredInfraMachineTemplate: desiredInfraMachineTemplate, + CurrentBootstrapConfigTemplate: currentBootstrapConfigTemplate, + DesiredBootstrapConfigTemplate: desiredBootstrapConfigTemplate, + }, + extensionHandlers: []string{"test-update-extension-1", "test-update-extension-2", "test-update-extension-3"}, + callExtensionResponses: map[string]runtimehooksv1.ResponseObject{ + "test-update-extension-1": &runtimehooksv1.CanUpdateMachineSetResponse{ + CommonResponse: runtimehooksv1.CommonResponse{Status: runtimehooksv1.ResponseStatusSuccess}, + MachineSetPatch: patchToUpdateMachineSet, + }, + "test-update-extension-2": &runtimehooksv1.CanUpdateMachineSetResponse{ + CommonResponse: runtimehooksv1.CommonResponse{Status: runtimehooksv1.ResponseStatusSuccess}, + InfrastructureMachineTemplatePatch: patchToUpdateInfraMachineTemplate, + }, + "test-update-extension-3": &runtimehooksv1.CanUpdateMachineSetResponse{ + CommonResponse: runtimehooksv1.CommonResponse{Status: runtimehooksv1.ResponseStatusSuccess}, + BootstrapConfigTemplatePatch: patchToUpdateBootstrapConfigTemplate, + }, + }, + callExtensionExpectedChanges: map[string]func(runtime.Object){ + "test-update-extension-2": func(object runtime.Object) { + if machine, ok := object.(*clusterv1.MachineSet); ok { + // After the call to test-update-extension-1 we expect that patchToUpdateMachineSet is already applied. + machine.Spec.Template.Spec.Version = "v1.31.0" + } + }, + "test-update-extension-3": func(object runtime.Object) { + if machine, ok := object.(*clusterv1.MachineSet); ok { + // After the call to test-update-extension-1 we expect that patchToUpdateMachineSet is already applied. + machine.Spec.Template.Spec.Version = "v1.31.0" + } + if infraMachineTemplate, ok := object.(*unstructured.Unstructured); ok { + if infraMachineTemplate.GroupVersionKind().Kind == builder.TestInfrastructureMachineTemplateKind { + // After the call to test-update-extension-2 we expect that patchToUpdateInfraMachineTemplate is already applied. + _ = unstructured.SetNestedField(infraMachineTemplate.Object, "in-place updated world InfraMachineTemplate", "spec", "template", "spec", "hello") + } + } + }, + }, + wantCanUpdateMachineSet: true, + }, + { + name: "Return false if current and desired objects are not equal and patches are returned that only account for some diffs", + newMS: desiredMachineSet, + templateObjects: &templateObjects{ + CurrentInfraMachineTemplate: currentInfraMachineTemplate, + DesiredInfraMachineTemplate: desiredInfraMachineTemplate, + CurrentBootstrapConfigTemplate: currentBootstrapConfigTemplate, + DesiredBootstrapConfigTemplate: desiredBootstrapConfigTemplate, + }, + extensionHandlers: []string{"test-update-extension"}, + callExtensionResponses: map[string]runtimehooksv1.ResponseObject{ + "test-update-extension": &runtimehooksv1.CanUpdateMachineSetResponse{ + CommonResponse: runtimehooksv1.CommonResponse{Status: runtimehooksv1.ResponseStatusSuccess}, + MachineSetPatch: patchToUpdateMachineSet, + InfrastructureMachineTemplatePatch: emptyPatch, + BootstrapConfigTemplatePatch: emptyPatch, + }, + }, + wantCanUpdateMachineSet: false, + wantReasons: []string{ + `BootstrapConfigTemplate cannot be updated in-place: &unstructured.Unstructured{ +- Object: map[string]any{ +- "spec": map[string]any{ +- "template": map[string]any{"spec": map[string]any{"hello": string("world BootstrapConfigTemplate")}}, +- }, +- }, ++ Object: map[string]any{ ++ "spec": map[string]any{ ++ "template": map[string]any{ ++ "spec": map[string]any{"hello": string("in-place updated world BootstrapConfigTemplate")}, ++ }, ++ }, ++ }, + }`, + `TestInfrastructureMachineTemplate cannot be updated in-place: &unstructured.Unstructured{ +- Object: map[string]any{ +- "spec": map[string]any{ +- "template": map[string]any{"spec": map[string]any{"hello": string("world InfraMachineTemplate")}}, +- }, +- }, ++ Object: map[string]any{ ++ "spec": map[string]any{ ++ "template": map[string]any{ ++ "spec": map[string]any{"hello": string("in-place updated world InfraMachineTemplate")}, ++ }, ++ }, ++ }, + }`, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + + catalog := runtimecatalog.New() + _ = runtimehooksv1.AddToCatalog(catalog) + runtimeClient := fakeruntimeclient.NewRuntimeClientBuilder(). + WithCatalog(catalog). + WithCallExtensionValidations(validateCanUpdateMachineSetRequests(currentMachineSet, tt.newMS, tt.templateObjects, tt.callExtensionExpectedChanges)). + WithCallExtensionResponses(tt.callExtensionResponses). + Build() + + p := &rolloutPlanner{ + RuntimeClient: runtimeClient, + } + + canUpdateMachineSet, reasons, err := p.canExtensionsUpdateMachineSet(ctx, currentMachineSet, tt.newMS, tt.templateObjects, tt.extensionHandlers) + if tt.wantError { + g.Expect(err).To(HaveOccurred()) + g.Expect(err.Error()).To(Equal(tt.wantErrorMessage)) + } else { + g.Expect(err).ToNot(HaveOccurred()) + } + g.Expect(canUpdateMachineSet).To(Equal(tt.wantCanUpdateMachineSet)) + g.Expect(reasons).To(Equal(tt.wantReasons)) + }) + } +} + +func validateCanUpdateMachineSetRequests(currentMachineSet, newMS *clusterv1.MachineSet, templateObjects *templateObjects, callExtensionExpectedChanges map[string]func(runtime.Object)) func(name string, object runtimehooksv1.RequestObject) error { + return func(name string, req runtimehooksv1.RequestObject) error { + switch req := req.(type) { + case *runtimehooksv1.CanUpdateMachineSetRequest: + // Compare MachineSet + currentMachineSet := currentMachineSet.DeepCopy() + currentMachineSet.SetGroupVersionKind(clusterv1.GroupVersion.WithKind("MachineSet")) + currentMachineSet.ResourceVersion = "" // cleanupMachineSet drops ResourceVersion. + if mutator, ok := callExtensionExpectedChanges[name]; ok { + mutator(currentMachineSet) + } + if d := diff(req.Current.MachineSet, *currentMachineSet); d != "" { + return fmt.Errorf("expected currentMachineSet to be equal, got diff: %s", d) + } + desiredMachineSet := newMS.DeepCopy() + desiredMachineSet.SetGroupVersionKind(clusterv1.GroupVersion.WithKind("MachineSet")) + desiredMachineSet.ResourceVersion = "" // cleanupMachineSet drops ResourceVersion. + if d := diff(req.Desired.MachineSet, *desiredMachineSet); d != "" { + return fmt.Errorf("expected desiredMachineSet to be equal, got diff: %s", d) + } + + // Compare BootstrapConfigTemplate + currentBootstrapConfigTemplate := templateObjects.CurrentBootstrapConfigTemplate.DeepCopy() + currentBootstrapConfigTemplate.SetResourceVersion("") // cleanupUnstructured drops ResourceVersion. + if mutator, ok := callExtensionExpectedChanges[name]; ok { + mutator(currentBootstrapConfigTemplate) + } + currentBootstrapConfigTemplateBytes, _ := json.Marshal(currentBootstrapConfigTemplate) + if d := diff(req.Current.BootstrapConfigTemplate.Raw, currentBootstrapConfigTemplateBytes); d != "" { + return fmt.Errorf("expected currentBootstrapConfigTemplate to be equal, got diff: %s", d) + } + desiredBootstrapConfigTemplate := templateObjects.DesiredBootstrapConfigTemplate.DeepCopy() + desiredBootstrapConfigTemplate.SetResourceVersion("") // cleanupUnstructured drops ResourceVersion. + desiredBootstrapConfigTemplateBytes, _ := json.Marshal(desiredBootstrapConfigTemplate) + if d := diff(req.Desired.BootstrapConfigTemplate.Raw, desiredBootstrapConfigTemplateBytes); d != "" { + return fmt.Errorf("expected desiredBootstrapConfigTemplate to be equal, got diff: %s", d) + } + + // Compare InfraMachineTemplate + currentInfraMachineTemplate := templateObjects.CurrentInfraMachineTemplate.DeepCopy() + currentInfraMachineTemplate.SetResourceVersion("") // cleanupUnstructured drops ResourceVersion. + if mutator, ok := callExtensionExpectedChanges[name]; ok { + mutator(currentInfraMachineTemplate) + } + currentInfraMachineBytes, _ := json.Marshal(currentInfraMachineTemplate) + reqCurrentInfraMachineTemplateBytes := bytes.TrimSuffix(req.Current.InfrastructureMachineTemplate.Raw, []byte("\n")) // Note: Somehow Patch introduces a trailing \n. + if d := diff(reqCurrentInfraMachineTemplateBytes, currentInfraMachineBytes); d != "" { + return fmt.Errorf("expected currentInfraMachineTemplate to be equal, got diff: %s", d) + } + desiredInfraMachineTemplate := templateObjects.DesiredInfraMachineTemplate.DeepCopy() + desiredInfraMachineTemplate.SetResourceVersion("") // cleanupUnstructured drops ResourceVersion. + desiredInfraMachineTemplateBytes, _ := json.Marshal(desiredInfraMachineTemplate) + if d := diff(req.Desired.InfrastructureMachineTemplate.Raw, desiredInfraMachineTemplateBytes); d != "" { + return fmt.Errorf("expected desiredInfraMachineTemplate to be equal, got diff: %s", d) + } + + return nil + default: + return fmt.Errorf("unhandled request type %T", req) + } + } +} + +func Test_applyPatchesToRequest(t *testing.T) { + currentMachineSet := &clusterv1.MachineSet{ + // Set GVK because this is required by convertToRawExtension. + TypeMeta: metav1.TypeMeta{ + APIVersion: clusterv1.GroupVersion.String(), + Kind: "MachineSet", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "machineset-to-in-place-update", + Namespace: metav1.NamespaceDefault, + }, + Spec: clusterv1.MachineSetSpec{ + Template: clusterv1.MachineTemplateSpec{ + Spec: clusterv1.MachineSpec{ + Version: "v1.30.0", + }, + }, + }, + } + patchedMachineSet := currentMachineSet.DeepCopy() + patchedMachineSet.Spec.Template.Spec.Version = "v1.31.0" + + currentBootstrapConfigTemplate := &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": builder.BootstrapGroupVersion.String(), + "kind": builder.TestBootstrapConfigTemplateKind, + "metadata": map[string]interface{}{ + "name": "bootstrap-config-template-1", + "namespace": metav1.NamespaceDefault, + }, + "spec": map[string]interface{}{ + "template": map[string]interface{}{ + "spec": map[string]interface{}{ + "hello": "world BootstrapConfigTemplate", + }, + }, + }, + }, + } + patchedBootstrapConfigTemplate := currentBootstrapConfigTemplate.DeepCopy() + _ = unstructured.SetNestedField(patchedBootstrapConfigTemplate.Object, "in-place updated world BootstrapConfigTemplate", "spec", "template", "spec", "hello") + + currentInfraMachineTemplate := &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": builder.InfrastructureGroupVersion.String(), + "kind": builder.TestInfrastructureMachineTemplateKind, + "metadata": map[string]interface{}{ + "name": "infrastructure-machine-template-1", + "namespace": metav1.NamespaceDefault, + }, + "spec": map[string]interface{}{ + "template": map[string]interface{}{ + "spec": map[string]interface{}{ + "hello": "world InfraMachineTemplate", + }, + }, + }, + }, + } + patchedInfraMachineTemplate := currentInfraMachineTemplate.DeepCopy() + _ = unstructured.SetNestedField(patchedInfraMachineTemplate.Object, "in-place updated world InfraMachineTemplate", "spec", "template", "spec", "hello") + + responseWithEmptyPatches := &runtimehooksv1.CanUpdateMachineSetResponse{ + CommonResponse: runtimehooksv1.CommonResponse{Status: runtimehooksv1.ResponseStatusSuccess}, + MachineSetPatch: runtimehooksv1.Patch{ + PatchType: runtimehooksv1.JSONPatchType, + Patch: []byte("[]"), + }, + InfrastructureMachineTemplatePatch: runtimehooksv1.Patch{ + PatchType: runtimehooksv1.JSONMergePatchType, + Patch: []byte{}, + }, + BootstrapConfigTemplatePatch: runtimehooksv1.Patch{ + PatchType: runtimehooksv1.JSONMergePatchType, + Patch: []byte("{}"), + }, + } + patchToUpdateMachineSet := runtimehooksv1.Patch{ + PatchType: runtimehooksv1.JSONPatchType, + Patch: []byte(`[{"op":"replace","path":"/spec/template/spec/version","value":"v1.31.0"}]`), + } + patchToUpdateBootstrapConfigTemplate := runtimehooksv1.Patch{ + PatchType: runtimehooksv1.JSONPatchType, + Patch: []byte(`[{"op":"replace","path":"/spec/template/spec/hello","value":"in-place updated world BootstrapConfigTemplate"}]`), + } + patchToUpdateInfraMachineTemplate := runtimehooksv1.Patch{ + PatchType: runtimehooksv1.JSONPatchType, + Patch: []byte(`[{"op":"replace","path":"/spec/template/spec/hello","value":"in-place updated world InfraMachineTemplate"}]`), + } + jsonMergePatchToUpdateMachineSet := runtimehooksv1.Patch{ + PatchType: runtimehooksv1.JSONMergePatchType, + Patch: []byte(`{"spec":{"template":{"spec":{"version":"v1.31.0"}}}}`), + } + jsonMergePatchToUpdateBootstrapConfigTemplate := runtimehooksv1.Patch{ + PatchType: runtimehooksv1.JSONMergePatchType, + Patch: []byte(`{"spec":{"template":{"spec":{"hello":"in-place updated world BootstrapConfigTemplate"}}}}`), + } + jsonMergePatchToUpdateInfraMachineTemplate := runtimehooksv1.Patch{ + PatchType: runtimehooksv1.JSONMergePatchType, + Patch: []byte(`{"spec":{"template":{"spec":{"hello":"in-place updated world InfraMachineTemplate"}}}}`), + } + patchToUpdateMachineSetStatus := runtimehooksv1.Patch{ + PatchType: runtimehooksv1.JSONPatchType, + Patch: []byte(`[{"op":"add","path":"/status","value":{"observedGeneration": 10}}]`), + } + patchToUpdateBootstrapConfigTemplateStatus := runtimehooksv1.Patch{ + PatchType: runtimehooksv1.JSONPatchType, + Patch: []byte(`[{"op":"add","path":"/status","value":{"observedGeneration": 10}}]`), + } + patchToUpdateInfraMachineTemplateStatus := runtimehooksv1.Patch{ + PatchType: runtimehooksv1.JSONPatchType, + Patch: []byte(`[{"op":"add","path":"/status","value":{"observedGeneration": 10}}]`), + } + + tests := []struct { + name string + req *runtimehooksv1.CanUpdateMachineSetRequest + resp *runtimehooksv1.CanUpdateMachineSetResponse + wantReq *runtimehooksv1.CanUpdateMachineSetRequest + wantError bool + wantErrorMessage string + }{ + { + name: "No changes with no patches", + req: &runtimehooksv1.CanUpdateMachineSetRequest{ + Current: runtimehooksv1.CanUpdateMachineSetRequestObjects{ + MachineSet: *currentMachineSet, + InfrastructureMachineTemplate: mustConvertToRawExtension(currentInfraMachineTemplate), + BootstrapConfigTemplate: mustConvertToRawExtension(currentBootstrapConfigTemplate), + }, + }, + resp: responseWithEmptyPatches, + wantReq: &runtimehooksv1.CanUpdateMachineSetRequest{ + Current: runtimehooksv1.CanUpdateMachineSetRequestObjects{ + MachineSet: *currentMachineSet, + InfrastructureMachineTemplate: mustConvertToRawExtension(currentInfraMachineTemplate), + BootstrapConfigTemplate: mustConvertToRawExtension(currentBootstrapConfigTemplate), + }, + }, + }, + { + name: "Changes with patches", + req: &runtimehooksv1.CanUpdateMachineSetRequest{ + Current: runtimehooksv1.CanUpdateMachineSetRequestObjects{ + MachineSet: *currentMachineSet, + InfrastructureMachineTemplate: mustConvertToRawExtension(currentInfraMachineTemplate), + BootstrapConfigTemplate: mustConvertToRawExtension(currentBootstrapConfigTemplate), + }, + }, + resp: &runtimehooksv1.CanUpdateMachineSetResponse{ + CommonResponse: runtimehooksv1.CommonResponse{Status: runtimehooksv1.ResponseStatusSuccess}, + MachineSetPatch: patchToUpdateMachineSet, + InfrastructureMachineTemplatePatch: patchToUpdateInfraMachineTemplate, + BootstrapConfigTemplatePatch: patchToUpdateBootstrapConfigTemplate, + }, + wantReq: &runtimehooksv1.CanUpdateMachineSetRequest{ + Current: runtimehooksv1.CanUpdateMachineSetRequestObjects{ + MachineSet: *patchedMachineSet, + InfrastructureMachineTemplate: mustConvertToRawExtension(patchedInfraMachineTemplate), + BootstrapConfigTemplate: mustConvertToRawExtension(patchedBootstrapConfigTemplate), + }, + }, + }, + { + name: "Changes with JSON merge patches", + req: &runtimehooksv1.CanUpdateMachineSetRequest{ + Current: runtimehooksv1.CanUpdateMachineSetRequestObjects{ + MachineSet: *currentMachineSet, + InfrastructureMachineTemplate: mustConvertToRawExtension(currentInfraMachineTemplate), + BootstrapConfigTemplate: mustConvertToRawExtension(currentBootstrapConfigTemplate), + }, + }, + resp: &runtimehooksv1.CanUpdateMachineSetResponse{ + CommonResponse: runtimehooksv1.CommonResponse{Status: runtimehooksv1.ResponseStatusSuccess}, + MachineSetPatch: jsonMergePatchToUpdateMachineSet, + InfrastructureMachineTemplatePatch: jsonMergePatchToUpdateInfraMachineTemplate, + BootstrapConfigTemplatePatch: jsonMergePatchToUpdateBootstrapConfigTemplate, + }, + wantReq: &runtimehooksv1.CanUpdateMachineSetRequest{ + Current: runtimehooksv1.CanUpdateMachineSetRequestObjects{ + MachineSet: *patchedMachineSet, + InfrastructureMachineTemplate: mustConvertToRawExtension(patchedInfraMachineTemplate), + BootstrapConfigTemplate: mustConvertToRawExtension(patchedBootstrapConfigTemplate), + }, + }, + }, + { + name: "No changes with status patches", + req: &runtimehooksv1.CanUpdateMachineSetRequest{ + Current: runtimehooksv1.CanUpdateMachineSetRequestObjects{ + MachineSet: *currentMachineSet, + InfrastructureMachineTemplate: mustConvertToRawExtension(currentInfraMachineTemplate), + BootstrapConfigTemplate: mustConvertToRawExtension(currentBootstrapConfigTemplate), + }, + }, + resp: &runtimehooksv1.CanUpdateMachineSetResponse{ + CommonResponse: runtimehooksv1.CommonResponse{Status: runtimehooksv1.ResponseStatusSuccess}, + MachineSetPatch: patchToUpdateMachineSetStatus, + InfrastructureMachineTemplatePatch: patchToUpdateInfraMachineTemplateStatus, + BootstrapConfigTemplatePatch: patchToUpdateBootstrapConfigTemplateStatus, + }, + wantReq: &runtimehooksv1.CanUpdateMachineSetRequest{ + Current: runtimehooksv1.CanUpdateMachineSetRequestObjects{ + MachineSet: *currentMachineSet, + InfrastructureMachineTemplate: mustConvertToRawExtension(currentInfraMachineTemplate), + BootstrapConfigTemplate: mustConvertToRawExtension(currentBootstrapConfigTemplate), + }, + }, + }, + { + name: "Error if PatchType is not set but Patch is", + req: &runtimehooksv1.CanUpdateMachineSetRequest{ + Current: runtimehooksv1.CanUpdateMachineSetRequestObjects{ + MachineSet: *currentMachineSet, + InfrastructureMachineTemplate: mustConvertToRawExtension(currentInfraMachineTemplate), + BootstrapConfigTemplate: mustConvertToRawExtension(currentBootstrapConfigTemplate), + }, + }, + resp: &runtimehooksv1.CanUpdateMachineSetResponse{ + CommonResponse: runtimehooksv1.CommonResponse{Status: runtimehooksv1.ResponseStatusSuccess}, + MachineSetPatch: runtimehooksv1.Patch{ + // PatchType is missing + Patch: []byte(`[{"op":"add","path":"/status","value":{"observedGeneration": 10}}]`), + }, + }, + wantError: true, + wantErrorMessage: "failed to apply patch: patchType is not set", + }, + { + name: "Error if PatchType is set to an unknown value", + req: &runtimehooksv1.CanUpdateMachineSetRequest{ + Current: runtimehooksv1.CanUpdateMachineSetRequestObjects{ + MachineSet: *currentMachineSet, + InfrastructureMachineTemplate: mustConvertToRawExtension(currentInfraMachineTemplate), + BootstrapConfigTemplate: mustConvertToRawExtension(currentBootstrapConfigTemplate), + }, + }, + resp: &runtimehooksv1.CanUpdateMachineSetResponse{ + CommonResponse: runtimehooksv1.CommonResponse{Status: runtimehooksv1.ResponseStatusSuccess}, + MachineSetPatch: runtimehooksv1.Patch{ + PatchType: "UnknownType", + Patch: []byte(`[{"op":"add","path":"/status","value":{"observedGeneration": 10}}]`), + }, + }, + wantError: true, + wantErrorMessage: "failed to apply patch: unknown patchType UnknownType", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + + err := applyPatchesToRequest(ctx, tt.req, tt.resp) + if tt.wantError { + g.Expect(err).To(HaveOccurred()) + g.Expect(err.Error()).To(Equal(tt.wantErrorMessage)) + return + } + g.Expect(err).ToNot(HaveOccurred()) + + // Compare only the objects and avoid comparing runtime.RawExtension.Raw because + // Raw is slightly non-deterministic because it doesn't guarantee order of map keys. + g.Expect(tt.req.Current.MachineSet).To(BeComparableTo(tt.wantReq.Current.MachineSet)) + g.Expect(tt.req.Current.InfrastructureMachineTemplate.Object).To(BeComparableTo(tt.wantReq.Current.InfrastructureMachineTemplate.Object)) + g.Expect(tt.req.Current.BootstrapConfigTemplate.Object).To(BeComparableTo(tt.wantReq.Current.BootstrapConfigTemplate.Object)) + }) + } +} + +func diff(a, b any) string { + _, d, err := compare.Diff(a, b) + if err != nil { + return fmt.Sprintf("error during diff: %v", err) + } + return d +} + +func mustConvertToRawExtension(object runtime.Object) runtime.RawExtension { + raw, err := patch.ConvertToRawExtension(object) + if err != nil { + panic(err) + } + return raw +} diff --git a/internal/controllers/machinedeployment/machinedeployment_controller.go b/internal/controllers/machinedeployment/machinedeployment_controller.go index af2e9ebe67d0..d7014ba75490 100644 --- a/internal/controllers/machinedeployment/machinedeployment_controller.go +++ b/internal/controllers/machinedeployment/machinedeployment_controller.go @@ -39,6 +39,8 @@ import ( clusterv1 "sigs.k8s.io/cluster-api/api/core/v1beta2" "sigs.k8s.io/cluster-api/controllers/external" + runtimeclient "sigs.k8s.io/cluster-api/exp/runtime/client" + "sigs.k8s.io/cluster-api/feature" clientutil "sigs.k8s.io/cluster-api/internal/util/client" "sigs.k8s.io/cluster-api/internal/util/ssa" "sigs.k8s.io/cluster-api/util" @@ -70,8 +72,9 @@ const machineDeploymentManagerName = "capi-machinedeployment" // Reconciler reconciles a MachineDeployment object. type Reconciler struct { - Client client.Client - APIReader client.Reader + Client client.Client + APIReader client.Reader + RuntimeClient runtimeclient.Client // WatchFilterValue is the label value used to filter events prior to reconciliation. WatchFilterValue string @@ -84,6 +87,9 @@ func (r *Reconciler) SetupWithManager(ctx context.Context, mgr ctrl.Manager, opt if r.Client == nil || r.APIReader == nil { return errors.New("Client and APIReader must not be nil") } + if feature.Gates.Enabled(feature.InPlaceUpdates) && r.RuntimeClient == nil { + return errors.New("RuntimeClient must not be nil when InPlaceUpdates feature gate is enabled") + } predicateLog := ctrl.LoggerFrom(ctx).WithValues("controller", "machinedeployment") clusterToMachineDeployments, err := util.ClusterToTypedObjectsMapper(mgr.GetClient(), &clusterv1.MachineDeploymentList{}, mgr.GetScheme()) diff --git a/internal/controllers/machinedeployment/machinedeployment_rollout_ondelete.go b/internal/controllers/machinedeployment/machinedeployment_rollout_ondelete.go index a6050b7f1488..cd850fe5db14 100644 --- a/internal/controllers/machinedeployment/machinedeployment_rollout_ondelete.go +++ b/internal/controllers/machinedeployment/machinedeployment_rollout_ondelete.go @@ -32,7 +32,7 @@ import ( // rolloutOnDelete reconcile machine sets controlled by a MachineDeployment that is using the OnDelete strategy. func (r *Reconciler) rolloutOnDelete(ctx context.Context, md *clusterv1.MachineDeployment, msList []*clusterv1.MachineSet, machines collections.Machines, templateExists bool) error { - planner := newRolloutPlanner() + planner := newRolloutPlanner(r.Client, r.RuntimeClient) if err := planner.init(ctx, md, msList, machines.UnsortedList(), true, templateExists); err != nil { return err } diff --git a/internal/controllers/machinedeployment/machinedeployment_rollout_ondelete_test.go b/internal/controllers/machinedeployment/machinedeployment_rollout_ondelete_test.go index 817fe4268c15..72549d2bfb88 100644 --- a/internal/controllers/machinedeployment/machinedeployment_rollout_ondelete_test.go +++ b/internal/controllers/machinedeployment/machinedeployment_rollout_ondelete_test.go @@ -322,7 +322,7 @@ func TestReconcileOldMachineSetsOnDelete(t *testing.T) { for _, tt := range testCases { t.Run(tt.name, func(t *testing.T) { g := NewWithT(t) - planner := newRolloutPlanner() + planner := newRolloutPlanner(nil, nil) planner.scaleIntents = tt.scaleIntent planner.md = tt.machineDeployment planner.newMS = tt.newMachineSet @@ -580,7 +580,7 @@ func runOnDeleteTestCase(ctx context.Context, t *testing.T, tt onDeleteSequenceT fLogger.Logf("[MD controller] - Input to rollout planner\n%s", current) // Running a small subset of MD reconcile (the rollout logic and a bit of setReplicas) - p := newRolloutPlanner() + p := newRolloutPlanner(nil, nil) p.overrideComputeDesiredMS = func(ctx context.Context, deployment *clusterv1.MachineDeployment, currentNewMS *clusterv1.MachineSet) (*clusterv1.MachineSet, error) { log := ctrl.LoggerFrom(ctx) desiredNewMS := currentNewMS diff --git a/internal/controllers/machinedeployment/machinedeployment_rollout_planner.go b/internal/controllers/machinedeployment/machinedeployment_rollout_planner.go index ba1a794f616e..5d7ac25cd9e5 100644 --- a/internal/controllers/machinedeployment/machinedeployment_rollout_planner.go +++ b/internal/controllers/machinedeployment/machinedeployment_rollout_planner.go @@ -29,12 +29,16 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" clusterv1 "sigs.k8s.io/cluster-api/api/core/v1beta2" + runtimeclient "sigs.k8s.io/cluster-api/exp/runtime/client" "sigs.k8s.io/cluster-api/internal/controllers/machinedeployment/mdutil" "sigs.k8s.io/cluster-api/internal/util/hash" "sigs.k8s.io/cluster-api/util/annotations" ) type rolloutPlanner struct { + Client client.Client + RuntimeClient runtimeclient.Client + md *clusterv1.MachineDeployment revision string @@ -47,14 +51,17 @@ type rolloutPlanner struct { oldMSs []*clusterv1.MachineSet upToDateResults map[string]mdutil.UpToDateResult - scaleIntents map[string]int32 - overrideComputeDesiredMS func(ctx context.Context, deployment *clusterv1.MachineDeployment, currentMS *clusterv1.MachineSet) (*clusterv1.MachineSet, error) - overrideCanUpdateMachineSetInPlace func(oldMS *clusterv1.MachineSet) bool + scaleIntents map[string]int32 + overrideComputeDesiredMS func(ctx context.Context, deployment *clusterv1.MachineDeployment, currentMS *clusterv1.MachineSet) (*clusterv1.MachineSet, error) + overrideCanUpdateMachineSetInPlace func(ctx context.Context, oldMS, newMS *clusterv1.MachineSet) (bool, error) + overrideCanExtensionsUpdateMachineSet func(ctx context.Context, oldMS, newMS *clusterv1.MachineSet, templateObjects *templateObjects, extensionHandlers []string) (bool, []string, error) } -func newRolloutPlanner() *rolloutPlanner { +func newRolloutPlanner(c client.Client, runtimeClient runtimeclient.Client) *rolloutPlanner { return &rolloutPlanner{ - scaleIntents: make(map[string]int32), + Client: c, + RuntimeClient: runtimeClient, + scaleIntents: make(map[string]int32), } } diff --git a/internal/controllers/machinedeployment/machinedeployment_rollout_rollingupdate.go b/internal/controllers/machinedeployment/machinedeployment_rollout_rollingupdate.go index c352307ff518..15180e650f21 100644 --- a/internal/controllers/machinedeployment/machinedeployment_rollout_rollingupdate.go +++ b/internal/controllers/machinedeployment/machinedeployment_rollout_rollingupdate.go @@ -22,6 +22,7 @@ import ( "sort" "strings" + "github.com/pkg/errors" "k8s.io/apimachinery/pkg/util/sets" "k8s.io/klog/v2" "k8s.io/utils/ptr" @@ -37,7 +38,7 @@ import ( // rolloutRollingUpdate reconcile machine sets controlled by a MachineDeployment that is using the RolloutUpdate strategy. func (r *Reconciler) rolloutRollingUpdate(ctx context.Context, md *clusterv1.MachineDeployment, msList []*clusterv1.MachineSet, machines collections.Machines, templateExists bool) error { - planner := newRolloutPlanner() + planner := newRolloutPlanner(r.Client, r.RuntimeClient) if err := planner.init(ctx, md, msList, machines.UnsortedList(), true, templateExists); err != nil { return err } @@ -415,12 +416,10 @@ func (p *rolloutPlanner) reconcileInPlaceUpdateIntent(ctx context.Context) error } // Check if the MachineSet can update in place; if not, move to the next MachineSet. - canUpdateMachineSetInPlaceFunc := func(_ *clusterv1.MachineSet) bool { return false } - if p.overrideCanUpdateMachineSetInPlace != nil { - canUpdateMachineSetInPlaceFunc = p.overrideCanUpdateMachineSetInPlace + canUpdateInPlace, err := p.canUpdateMachineSetInPlace(ctx, oldMS, p.newMS) + if err != nil { + return errors.Wrapf(err, "failed to determine if MachineSet %s can be updated in-place", oldMS.Name) } - - canUpdateInPlace := canUpdateMachineSetInPlaceFunc(oldMS) log.V(5).Info(fmt.Sprintf("CanUpdate in-place decision for MachineSet %s: %t", oldMS.Name, canUpdateInPlace), "MachineSet", klog.KObj(oldMS)) if !canUpdateInPlace { diff --git a/internal/controllers/machinedeployment/machinedeployment_rollout_rollingupdate_test.go b/internal/controllers/machinedeployment/machinedeployment_rollout_rollingupdate_test.go index 293f03b1a17f..f76ee2133617 100644 --- a/internal/controllers/machinedeployment/machinedeployment_rollout_rollingupdate_test.go +++ b/internal/controllers/machinedeployment/machinedeployment_rollout_rollingupdate_test.go @@ -123,7 +123,7 @@ func TestReconcileReplicasPendingAcknowledgeMove(t *testing.T) { t.Run(tc.name, func(t *testing.T) { g := NewWithT(t) - planner := newRolloutPlanner() + planner := newRolloutPlanner(nil, nil) planner.md = tc.md planner.newMS = tc.newMS if tc.originalNewMS != nil { @@ -349,7 +349,7 @@ func TestReconcileNewMachineSet(t *testing.T) { t.Run(tc.name, func(t *testing.T) { g := NewWithT(t) - planner := newRolloutPlanner() + planner := newRolloutPlanner(nil, nil) planner.md = tc.machineDeployment planner.newMS = tc.newMachineSet planner.oldMSs = tc.oldMachineSets @@ -1154,16 +1154,16 @@ func TestReconcileInPlaceUpdateIntent(t *testing.T) { canUpdateCalls := make(map[string]bool) - planner := newRolloutPlanner() + planner := newRolloutPlanner(nil, nil) planner.md = tc.md planner.newMS = tc.newMS planner.oldMSs = tc.oldMS planner.machines = tc.machines planner.scaleIntents = tc.scaleIntents planner.upToDateResults = tc.upToDateResults - planner.overrideCanUpdateMachineSetInPlace = func(oldMS *clusterv1.MachineSet) bool { + planner.overrideCanUpdateMachineSetInPlace = func(_ context.Context, oldMS, _ *clusterv1.MachineSet) (bool, error) { canUpdateCalls[oldMS.Name] = true - return tc.canUpdateAnswer[oldMS.Name] + return tc.canUpdateAnswer[oldMS.Name], nil } err := planner.reconcileInPlaceUpdateIntent(ctx) @@ -1354,7 +1354,7 @@ type rollingUpdateSequenceTestCase struct { desiredMachineNames []string // overrideCanUpdateMachineSetInPlaceFunc allows to inject a function that will be used to perform the canUpdateMachineSetInPlace decision - overrideCanUpdateMachineSetInPlaceFunc func(oldMS *clusterv1.MachineSet) bool + overrideCanUpdateMachineSetInPlaceFunc func(ctx context.Context, oldMS, newMS *clusterv1.MachineSet) (bool, error) // skipLogToFileAndGoldenFileCheck allows to skip storing the log to file and golden file Check. // NOTE: this field is controlled by the test itself. @@ -1752,8 +1752,11 @@ func runRollingUpdateTestCase(ctx context.Context, t *testing.T, tt rollingUpdat fLogger.Logf("[MD controller] - Input to rollout planner\n%s", current) // Running a small subset of MD reconcile (the rollout logic and a bit of setReplicas) - p := newRolloutPlanner() - p.overrideCanUpdateMachineSetInPlace = tt.overrideCanUpdateMachineSetInPlaceFunc + p := newRolloutPlanner(nil, nil) + p.overrideCanUpdateMachineSetInPlace = func(_ context.Context, _, _ *clusterv1.MachineSet) (bool, error) { return false, nil } + if tt.overrideCanUpdateMachineSetInPlaceFunc != nil { + p.overrideCanUpdateMachineSetInPlace = tt.overrideCanUpdateMachineSetInPlaceFunc + } p.overrideComputeDesiredMS = func(ctx context.Context, deployment *clusterv1.MachineDeployment, currentNewMS *clusterv1.MachineSet) (*clusterv1.MachineSet, error) { log := ctrl.LoggerFrom(ctx) desiredNewMS := currentNewMS @@ -1849,8 +1852,8 @@ func runRollingUpdateTestCase(ctx context.Context, t *testing.T, tt rollingUpdat } } -func oldMSCanAlwaysUpdateInPlace(_ *clusterv1.MachineSet) bool { - return true +func oldMSCanAlwaysUpdateInPlace(_ context.Context, _, _ *clusterv1.MachineSet) (bool, error) { + return true, nil } func maxUnavailableBreachToleration() func(log *fileLogger, _ int, _ *rolloutScope, _, _ int32) bool { diff --git a/internal/controllers/machinedeployment/machinedeployment_sync.go b/internal/controllers/machinedeployment/machinedeployment_sync.go index 1d30c6155de3..caa2e03d437a 100644 --- a/internal/controllers/machinedeployment/machinedeployment_sync.go +++ b/internal/controllers/machinedeployment/machinedeployment_sync.go @@ -44,7 +44,7 @@ func (r *Reconciler) sync(ctx context.Context, md *clusterv1.MachineDeployment, // - identifying newMS and OldMS when necessary // - computing desired state for newMS and OldMS, including managing rollout related annotations and // in-place propagation of labels, annotations and other fields. - planner := newRolloutPlanner() + planner := newRolloutPlanner(r.Client, r.RuntimeClient) if err := planner.init(ctx, md, msList, machines.UnsortedList(), false, templateExists); err != nil { return err } diff --git a/internal/controllers/topology/cluster/patches/engine.go b/internal/controllers/topology/cluster/patches/engine.go index cb66e9aa55a6..428102b02b80 100644 --- a/internal/controllers/topology/cluster/patches/engine.go +++ b/internal/controllers/topology/cluster/patches/engine.go @@ -505,7 +505,7 @@ func applyPatchToRequest(ctx context.Context, req *runtimehooksv1.GeneratePatche // Overwrite the spec of template.Template with the spec of the patchedTemplate, // to ensure that we only pick up changes to the spec. - if err := patchutil.PatchSpec(&requestItem.Object, patchedTemplate); err != nil { + if err := patchutil.Patch(&requestItem.Object, patchedTemplate, "spec"); err != nil { log.Error(err, fmt.Sprintf("Failed to apply patch to template with uid %q", requestItem.UID)) return errors.Wrap(err, "failed to apply patch to template") } diff --git a/internal/util/patch/patch.go b/internal/util/patch/patch.go index 39a06afe8c11..795ce39850fa 100644 --- a/internal/util/patch/patch.go +++ b/internal/util/patch/patch.go @@ -18,19 +18,145 @@ limitations under the License. package patch import ( + "bytes" + "context" + "encoding/json" + "fmt" + "runtime/debug" "strings" + jsonpatch "github.com/evanphx/json-patch/v5" "github.com/pkg/errors" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/serializer" + kerrors "k8s.io/apimachinery/pkg/util/errors" "k8s.io/klog/v2" + ctrl "sigs.k8s.io/controller-runtime" + runtimehooksv1 "sigs.k8s.io/cluster-api/api/runtime/hooks/v1alpha1" "sigs.k8s.io/cluster-api/internal/contract" ) -// PatchSpec overwrites spec in object with spec of patchedObjectBytes. -func PatchSpec(object *runtime.RawExtension, patchedObjectBytes []byte) error { //nolint:revive // not going to call this func ObjectSpe to avoid stuttering +// ApplyPatchToTypedObject applies the patch to a typed obj. +func ApplyPatchToTypedObject[T any](ctx context.Context, currentMachine *T, machinePath runtimehooksv1.Patch, patchPath string) error { + // Note: Machine needs special handling because it is not a runtime.RawExtension. Simply converting it here to + // a runtime.RawExtension so we can avoid making the code in applyPatchToObject more complex. + currentMachineRaw, err := ConvertToRawExtension(currentMachine) + if err != nil { + return err + } + + machineChanged, err := ApplyPatchToObject(ctx, ¤tMachineRaw, machinePath, patchPath) + if err != nil { + return err + } + + if !machineChanged { + return nil + } + + // Note: json.Unmarshal can't be used directly on *currentMachine as json.Unmarshal does not unset fields. + patchedCurrentMachine := new(T) + if err := json.Unmarshal(currentMachineRaw.Raw, patchedCurrentMachine); err != nil { + return err + } + *currentMachine = *patchedCurrentMachine + return nil +} + +// ApplyPatchToObject applies the patch to the obj. +// Note: This is following the same general structure that is used in the applyPatchToRequest func in +// internal/controllers/topology/cluster/patches/engine.go. +func ApplyPatchToObject(ctx context.Context, obj *runtime.RawExtension, patch runtimehooksv1.Patch, patchPath string) (objChanged bool, reterr error) { + log := ctrl.LoggerFrom(ctx) + + if patch.PatchType == "" { + return false, errors.Errorf("failed to apply patch: patchType is not set") + } + + defer func() { + if r := recover(); r != nil { + log.Info(fmt.Sprintf("Observed a panic when applying patch: %v\n%s", r, string(debug.Stack()))) + reterr = kerrors.NewAggregate([]error{reterr, fmt.Errorf("observed a panic when applying patch: %v", r)}) + } + }() + + // Create a copy of obj.Raw. + // The patches will be applied to the copy and then only spec changes will be copied back to the obj. + patchedObject := bytes.Clone(obj.Raw) + var err error + + switch patch.PatchType { + case runtimehooksv1.JSONPatchType: + log.V(5).Info("Accumulating JSON patch", "patch", string(patch.Patch)) + jsonPatch, err := jsonpatch.DecodePatch(patch.Patch) + if err != nil { + log.Error(err, "Failed to apply patch: error decoding json patch (RFC6902)", "patch", string(patch.Patch)) + return false, errors.Wrap(err, "failed to apply patch: error decoding json patch (RFC6902)") + } + + if len(jsonPatch) == 0 { + // Return if there are no patches, nothing to do. + return false, nil + } + + patchedObject, err = jsonPatch.Apply(patchedObject) + if err != nil { + log.Error(err, "Failed to apply patch: error applying json patch (RFC6902)", "patch", string(patch.Patch)) + return false, errors.Wrap(err, "failed to apply patch: error applying json patch (RFC6902)") + } + case runtimehooksv1.JSONMergePatchType: + if len(patch.Patch) == 0 || bytes.Equal(patch.Patch, []byte("{}")) { + // Return if there are no patches, nothing to do. + return false, nil + } + + log.V(5).Info("Accumulating JSON merge patch", "patch", string(patch.Patch)) + patchedObject, err = jsonpatch.MergePatch(patchedObject, patch.Patch) + if err != nil { + log.Error(err, "Failed to apply patch: error applying json merge patch (RFC7386)", "patch", string(patch.Patch)) + return false, errors.Wrap(err, "failed to apply patch: error applying json merge patch (RFC7386)") + } + default: + return false, errors.Errorf("failed to apply patch: unknown patchType %s", patch.PatchType) + } + + // Overwrite the spec of obj with the spec of the patchedObject, + // to ensure that we only pick up changes to the spec. + if err := Patch(obj, patchedObject, patchPath); err != nil { + return false, errors.Wrap(err, "failed to apply patch to object") + } + + return true, nil +} + +// ConvertToRawExtension converts any object to a runtime.RawExtension. +func ConvertToRawExtension(object any) (runtime.RawExtension, error) { + objectBytes, err := json.Marshal(object) + if err != nil { + return runtime.RawExtension{}, errors.Wrap(err, "failed to marshal object to JSON") + } + + objectUnstructured, ok := object.(*unstructured.Unstructured) + if !ok { + objectUnstructured = &unstructured.Unstructured{} + // Note: This only succeeds if object has apiVersion & kind set (which is always the case). + if err := json.Unmarshal(objectBytes, objectUnstructured); err != nil { + return runtime.RawExtension{}, errors.Wrap(err, "failed to Unmarshal object into Unstructured") + } + } + + // Note: Raw and Object are always both set and Object is always set as an Unstructured + // to simplify subsequent code in matchesUnstructuredSpec. + return runtime.RawExtension{ + Raw: objectBytes, + Object: objectUnstructured, + }, nil +} + +// Patch overwrites spec in object with spec of patchedObjectBytes. +func Patch(object *runtime.RawExtension, patchedObjectBytes []byte, patchPath string) error { objectUnstructured, err := bytesToUnstructured(object.Raw) if err != nil { return errors.Wrap(err, "failed to convert object to Unstructured") @@ -44,8 +170,8 @@ func PatchSpec(object *runtime.RawExtension, patchedObjectBytes []byte) error { if err := CopySpec(CopySpecInput{ Src: patchedObjectUnstructured, Dest: objectUnstructured, - SrcSpecPath: "spec", - DestSpecPath: "spec", + SrcSpecPath: patchPath, + DestSpecPath: patchPath, }); err != nil { return errors.Wrap(err, "failed to apply patch to object") } diff --git a/main.go b/main.go index 8f6f050fafd8..473d0651ab4c 100644 --- a/main.go +++ b/main.go @@ -713,6 +713,7 @@ func setupReconcilers(ctx context.Context, mgr ctrl.Manager, watchNamespaces map if err := (&controllers.MachineDeploymentReconciler{ Client: mgr.GetClient(), APIReader: mgr.GetAPIReader(), + RuntimeClient: runtimeClient, WatchFilterValue: watchFilterValue, }).SetupWithManager(ctx, mgr, concurrency(machineDeploymentConcurrency)); err != nil { setupLog.Error(err, "Unable to create controller", "controller", "MachineDeployment") diff --git a/test/e2e/cluster_in_place_update.go b/test/e2e/cluster_in_place_update.go index d19f37740766..8f8e7ef4ab37 100644 --- a/test/e2e/cluster_in_place_update.go +++ b/test/e2e/cluster_in_place_update.go @@ -29,12 +29,14 @@ import ( . "github.com/onsi/gomega" corev1 "k8s.io/api/core/v1" apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/sets" "k8s.io/utils/ptr" "sigs.k8s.io/controller-runtime/pkg/client" bootstrapv1 "sigs.k8s.io/cluster-api/api/bootstrap/kubeadm/v1beta2" clusterv1 "sigs.k8s.io/cluster-api/api/core/v1beta2" + runtimev1 "sigs.k8s.io/cluster-api/api/runtime/v1beta2" "sigs.k8s.io/cluster-api/test/framework" "sigs.k8s.io/cluster-api/test/framework/clusterctl" "sigs.k8s.io/cluster-api/util" @@ -153,7 +155,7 @@ func ClusterInPlaceUpdateSpec(ctx context.Context, inputGetter func() ClusterInP // This is necessary so in CI this test doesn't influence other tests by enabling lifecycle hooks // in other test namespaces. namespaces := []string{namespace.Name} - extensionConfig := extensionConfig(input.ExtensionConfigName, input.ExtensionServiceNamespace, input.ExtensionServiceName, defaultAllHandlersToBlocking, namespaces...) + extensionConfig := extensionConfig(input.ExtensionConfigName, input.ExtensionServiceNamespace, input.ExtensionServiceName, false, defaultAllHandlersToBlocking, namespaces...) Expect(input.BootstrapClusterProxy.GetClient().Create(ctx, extensionConfig)). To(Succeed(), "Failed to create the ExtensionConfig") @@ -209,7 +211,13 @@ func ClusterInPlaceUpdateSpec(ctx context.Context, inputGetter func() ClusterInP // Doing multiple in-place updates for additional coverage. filePath := "/tmp/test" - for i, fileContent := range []string{"first in-place update", "second in-place update"} { + for i, fileContent := range []string{ + "first in-place update", + "second in-place update", + "third in-place update", + "fourth in-place update", + "five in-place update", + } { Byf("[%d] Trigger in-place update by modifying the files variable", i) originalCluster := cluster.DeepCopy() @@ -246,8 +254,7 @@ func ClusterInPlaceUpdateSpec(ctx context.Context, inputGetter func() ClusterInP // Ensure only in-place updates were executed and no Machine was re-created. machineObjectsAfterInPlaceUpdate = getMachineObjects(ctx, g, mgmtClient, cluster) g.Expect(machineNames(machineObjectsAfterInPlaceUpdate.ControlPlaneMachines)).To(Equal(machineNames(machineObjectsBeforeInPlaceUpdate.ControlPlaneMachines))) - // TODO(in-place): enable once MD/MS/Machine controller PRs are merged - // g.Expect(machineNames(machineObjectsAfterInPlaceUpdate.WorkerMachines)).To(Equal(machineNames(machineObjectsBeforeInPlaceUpdate.WorkerMachines))) + g.Expect(machineNames(machineObjectsAfterInPlaceUpdate.WorkerMachines)).To(Equal(machineNames(machineObjectsBeforeInPlaceUpdate.WorkerMachines))) }, input.E2EConfig.GetIntervals(specName, "wait-control-plane")...).Should(Succeed()) // Update machineObjectsBeforeInPlaceUpdate for the next round of in-place update. @@ -263,7 +270,7 @@ func ClusterInPlaceUpdateSpec(ctx context.Context, inputGetter func() ClusterInP if !input.SkipCleanup { if input.ExtensionServiceNamespace != "" && input.ExtensionServiceName != "" { Eventually(func() error { - return input.BootstrapClusterProxy.GetClient().Delete(ctx, extensionConfig(input.ExtensionConfigName, input.ExtensionServiceNamespace, input.ExtensionServiceName, true)) + return input.BootstrapClusterProxy.GetClient().Delete(ctx, &runtimev1.ExtensionConfig{ObjectMeta: metav1.ObjectMeta{Name: input.ExtensionConfigName}}) }, 10*time.Second, 1*time.Second).Should(Succeed(), "Deleting ExtensionConfig failed") } } diff --git a/test/e2e/cluster_upgrade_runtimesdk.go b/test/e2e/cluster_upgrade_runtimesdk.go index d18c411ff5da..211125a95605 100644 --- a/test/e2e/cluster_upgrade_runtimesdk.go +++ b/test/e2e/cluster_upgrade_runtimesdk.go @@ -203,7 +203,7 @@ func ClusterUpgradeWithRuntimeSDKSpec(ctx context.Context, inputGetter func() Cl // cluster lifecycle by default. Setting defaultAllHandlersToBlocking to true enforces that the test-extension // automatically creates the ConfigMap with blocking preloaded responses. Expect(input.BootstrapClusterProxy.GetClient().Create(ctx, - extensionConfig(input.ExtensionConfigName, input.ExtensionServiceNamespace, input.ExtensionServiceName, true, namespaces...))). + extensionConfig(input.ExtensionConfigName, input.ExtensionServiceNamespace, input.ExtensionServiceName, true, true, namespaces...))). To(Succeed(), "Failed to create the extension config") By("Creating a workload cluster; creation waits for BeforeClusterCreateHook to gate the operation") @@ -494,7 +494,7 @@ func ClusterUpgradeWithRuntimeSDKSpec(ctx context.Context, inputGetter func() Cl if !input.SkipCleanup { // Delete the extensionConfig first to ensure the BeforeDeleteCluster hook doesn't block deletion. Eventually(func() error { - return input.BootstrapClusterProxy.GetClient().Delete(ctx, extensionConfig(input.ExtensionConfigName, input.ExtensionServiceNamespace, input.ExtensionServiceName, true, namespace.Name)) + return input.BootstrapClusterProxy.GetClient().Delete(ctx, &runtimev1.ExtensionConfig{ObjectMeta: metav1.ObjectMeta{Name: input.ExtensionConfigName}}) }, 10*time.Second, 1*time.Second).Should(Succeed(), "Deleting ExtensionConfig failed") Byf("Deleting cluster %s", klog.KObj(clusterResources.Cluster)) @@ -629,7 +629,7 @@ func machineSetPreflightChecksTest(ctx context.Context, c client.Client, cluster // We make sure this cluster-wide object does not conflict with others by using a random generated // name and a NamespaceSelector selecting on the namespace of the current test. // Thus, this object is "namespaced" to the current test even though it's a cluster-wide object. -func extensionConfig(name, extensionServiceNamespace, extensionServiceName string, defaultAllHandlersToBlocking bool, namespaces ...string) *runtimev1.ExtensionConfig { +func extensionConfig(name, extensionServiceNamespace, extensionServiceName string, disableInPlaceUpdates, defaultAllHandlersToBlocking bool, namespaces ...string) *runtimev1.ExtensionConfig { cfg := &runtimev1.ExtensionConfig{ ObjectMeta: metav1.ObjectMeta{ // Note: We have to use a constant name here as we have to be able to reference it in the ClusterClass @@ -650,6 +650,7 @@ func extensionConfig(name, extensionServiceNamespace, extensionServiceName strin }, Settings: map[string]string{ "extensionConfigName": name, + "disableInPlaceUpdates": strconv.FormatBool(disableInPlaceUpdates), "defaultAllHandlersToBlocking": strconv.FormatBool(defaultAllHandlersToBlocking), }, }, diff --git a/test/e2e/quick_start.go b/test/e2e/quick_start.go index 36a4161fe9ac..3eeafac25698 100644 --- a/test/e2e/quick_start.go +++ b/test/e2e/quick_start.go @@ -27,8 +27,10 @@ import ( . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/utils/ptr" + runtimev1 "sigs.k8s.io/cluster-api/api/runtime/v1beta2" "sigs.k8s.io/cluster-api/test/framework" "sigs.k8s.io/cluster-api/test/framework/clusterctl" "sigs.k8s.io/cluster-api/util" @@ -186,7 +188,7 @@ func QuickStartSpec(ctx context.Context, inputGetter func() QuickStartSpecInput) // Add the ClusterClass namespace, if the ClusterClass is deployed in a separate namespace. namespaces = append(namespaces, clusterClassNamespace.Name) } - extensionConfig := extensionConfig(input.ExtensionConfigName, input.ExtensionServiceNamespace, input.ExtensionServiceName, defaultAllHandlersToBlocking, namespaces...) + extensionConfig := extensionConfig(input.ExtensionConfigName, input.ExtensionServiceNamespace, input.ExtensionServiceName, true, defaultAllHandlersToBlocking, namespaces...) Expect(input.BootstrapClusterProxy.GetClient().Create(ctx, extensionConfig)). To(Succeed(), "Failed to create the ExtensionConfig") @@ -252,7 +254,7 @@ func QuickStartSpec(ctx context.Context, inputGetter func() QuickStartSpecInput) if !input.SkipCleanup { if input.ExtensionServiceNamespace != "" && input.ExtensionServiceName != "" { Eventually(func() error { - return input.BootstrapClusterProxy.GetClient().Delete(ctx, extensionConfig(input.ExtensionConfigName, input.ExtensionServiceNamespace, input.ExtensionServiceName, true)) + return input.BootstrapClusterProxy.GetClient().Delete(ctx, &runtimev1.ExtensionConfig{ObjectMeta: metav1.ObjectMeta{Name: input.ExtensionConfigName}}) }, 10*time.Second, 1*time.Second).Should(Succeed(), "Deleting ExtensionConfig failed") } if input.DeployClusterClassInSeparateNamespace { diff --git a/test/e2e/scale.go b/test/e2e/scale.go index 42984e8c4883..345030500150 100644 --- a/test/e2e/scale.go +++ b/test/e2e/scale.go @@ -47,6 +47,7 @@ import ( controlplanev1 "sigs.k8s.io/cluster-api/api/controlplane/kubeadm/v1beta2" clusterv1beta1 "sigs.k8s.io/cluster-api/api/core/v1beta1" clusterv1 "sigs.k8s.io/cluster-api/api/core/v1beta2" + runtimev1 "sigs.k8s.io/cluster-api/api/runtime/v1beta2" "sigs.k8s.io/cluster-api/test/e2e/internal/log" "sigs.k8s.io/cluster-api/test/framework" "sigs.k8s.io/cluster-api/test/framework/clusterctl" @@ -292,7 +293,7 @@ func ScaleSpec(ctx context.Context, inputGetter func() ScaleSpecInput) { if !deployClusterInSeparateNamespaces { namespaces = append(namespaces, namespace.Name) } - extensionConfig := extensionConfig(input.ExtensionConfigName, input.ExtensionServiceNamespace, input.ExtensionServiceName, defaultAllHandlersToBlocking, namespaces...) + extensionConfig := extensionConfig(input.ExtensionConfigName, input.ExtensionServiceNamespace, input.ExtensionServiceName, true, defaultAllHandlersToBlocking, namespaces...) if deployClusterInSeparateNamespaces { extensionConfig.Spec.NamespaceSelector = &metav1.LabelSelector{ // Note: we are limiting the test extension to be used by the namespace where the test is run. @@ -519,7 +520,7 @@ func ScaleSpec(ctx context.Context, inputGetter func() ScaleSpecInput) { if !input.SkipCleanup { if input.ExtensionServiceNamespace != "" && input.ExtensionServiceName != "" { Eventually(func() error { - return input.BootstrapClusterProxy.GetClient().Delete(ctx, extensionConfig(input.ExtensionConfigName, input.ExtensionServiceNamespace, input.ExtensionServiceName, true)) + return input.BootstrapClusterProxy.GetClient().Delete(ctx, &runtimev1.ExtensionConfig{ObjectMeta: metav1.ObjectMeta{Name: input.ExtensionConfigName}}) }, 10*time.Second, 1*time.Second).Should(Succeed(), "Deleting ExtensionConfig failed") } } diff --git a/test/extension/config/tilt/extensionconfig.yaml b/test/extension/config/tilt/extensionconfig.yaml index ceb27024b1d1..cd91a9a4706e 100644 --- a/test/extension/config/tilt/extensionconfig.yaml +++ b/test/extension/config/tilt/extensionconfig.yaml @@ -1,4 +1,4 @@ -apiVersion: runtime.cluster.x-k8s.io/v1alpha1 +apiVersion: runtime.cluster.x-k8s.io/v1beta2 kind: ExtensionConfig metadata: annotations: @@ -17,4 +17,4 @@ spec: - key: kubernetes.io/metadata.name operator: In values: - - default # Note: this assumes the test extension is used by Cluster in the default namespace only \ No newline at end of file + - default # Note: this assumes the test extension is used by Cluster in the default namespace only diff --git a/test/extension/handlers/inplaceupdate/handlers.go b/test/extension/handlers/inplaceupdate/handlers.go index b2a32617f293..9b5c8524b5aa 100644 --- a/test/extension/handlers/inplaceupdate/handlers.go +++ b/test/extension/handlers/inplaceupdate/handlers.go @@ -32,6 +32,7 @@ import ( "gomodules.xyz/jsonpatch/v2" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/serializer" + "k8s.io/apimachinery/pkg/util/rand" "k8s.io/klog/v2" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" @@ -104,6 +105,11 @@ func (h *ExtensionHandlers) DoCanUpdateMachine(ctx context.Context, req *runtime log := ctrl.LoggerFrom(ctx).WithValues("Machine", klog.KObj(&req.Desired.Machine)) log.Info("CanUpdateMachine is called") + if req.Settings["disableInPlaceUpdates"] == "true" { + resp.Status = runtimehooksv1.ResponseStatusSuccess + return + } + currentMachine, desiredMachine, currentBootstrapConfig, desiredBootstrapConfig, currentInfraMachine, desiredInfraMachine, err := h.getObjectsFromCanUpdateMachineRequest(req) @@ -146,6 +152,11 @@ func (h *ExtensionHandlers) DoCanUpdateMachineSet(ctx context.Context, req *runt log := ctrl.LoggerFrom(ctx).WithValues("MachineSet", klog.KObj(&req.Desired.MachineSet)) log.Info("CanUpdateMachineSet is called") + if req.Settings["disableInPlaceUpdates"] == "true" { + resp.Status = runtimehooksv1.ResponseStatusSuccess + return + } + currentMachineSet, desiredMachineSet, currentBootstrapConfigTemplate, desiredBootstrapConfigTemplate, currentInfraMachineTemplate, desiredInfraMachineTemplate, err := h.getObjectsFromCanUpdateMachineSetRequest(req) @@ -193,12 +204,18 @@ func (h *ExtensionHandlers) DoUpdateMachine(ctx context.Context, req *runtimehoo log.Info("UpdateMachine response", "Machine", klog.KObj(&req.Desired.Machine), "status", resp.Status, "message", resp.Message, "retryAfterSeconds", resp.RetryAfterSeconds) }() + if req.Settings["disableInPlaceUpdates"] == "true" { + resp.Status = runtimehooksv1.ResponseStatusFailure + resp.Message = "Unexpected call to UpdateMachine hook after CanUpdateMachine or CanUpdateMachineSet did not return any patches" + return + } + key := klog.KObj(&req.Desired.Machine).String() // Note: We are intentionally not actually applying any in-place changes we are just faking them, // which is good enough for test purposes. if firstTimeCalled, ok := h.state.Load(key); ok { - if time.Since(firstTimeCalled.(time.Time)) > 20*time.Second { + if time.Since(firstTimeCalled.(time.Time)) > time.Duration(20+rand.Intn(10))*time.Second { h.state.Delete(key) resp.Status = runtimehooksv1.ResponseStatusSuccess resp.Message = "In-place update is done" From aabc5050ae653a1d53e8924b5aa7c2d278cdffc2 Mon Sep 17 00:00:00 2001 From: Stefan Bueringer Date: Fri, 7 Nov 2025 18:13:52 +0100 Subject: [PATCH 2/4] Fix & improve e2e test --- .../machinedeployment_canupdatemachineset.go | 32 ++++++++++++------- test/e2e/cluster_in_place_update.go | 9 +++--- 2 files changed, 25 insertions(+), 16 deletions(-) diff --git a/internal/controllers/machinedeployment/machinedeployment_canupdatemachineset.go b/internal/controllers/machinedeployment/machinedeployment_canupdatemachineset.go index a2dc9370ee62..14709c1a8710 100644 --- a/internal/controllers/machinedeployment/machinedeployment_canupdatemachineset.go +++ b/internal/controllers/machinedeployment/machinedeployment_canupdatemachineset.go @@ -31,6 +31,7 @@ import ( clusterv1 "sigs.k8s.io/cluster-api/api/core/v1beta2" runtimehooksv1 "sigs.k8s.io/cluster-api/api/runtime/hooks/v1alpha1" "sigs.k8s.io/cluster-api/controllers/external" + "sigs.k8s.io/cluster-api/internal/controllers/machinedeployment/mdutil" "sigs.k8s.io/cluster-api/internal/util/compare" "sigs.k8s.io/cluster-api/internal/util/patch" ) @@ -258,22 +259,29 @@ func matchesMachineSet(req *runtimehooksv1.CanUpdateMachineSetRequest) (bool, [] func matchesMachineSetSpec(patched, desired *clusterv1.MachineSet) (equal bool, diff string, matchErr error) { // Note: Wrapping MachineSet specs in a MachineSet for proper formatting of the diff. - return compare.Diff( - &clusterv1.MachineSet{ - Spec: clusterv1.MachineSetSpec{ - Template: clusterv1.MachineTemplateSpec{ - Spec: patched.Spec.Template.Spec, - }, + cleanedUpPatchedMachineSet := &clusterv1.MachineSet{ + Spec: clusterv1.MachineSetSpec{ + Template: clusterv1.MachineTemplateSpec{ + Spec: patched.Spec.Template.Spec, }, }, - &clusterv1.MachineSet{ - Spec: clusterv1.MachineSetSpec{ - Template: clusterv1.MachineTemplateSpec{ - Spec: desired.Spec.Template.Spec, - }, + } + cleanedUpDesiredMachineSet := &clusterv1.MachineSet{ + Spec: clusterv1.MachineSetSpec{ + Template: clusterv1.MachineTemplateSpec{ + Spec: desired.Spec.Template.Spec, }, }, - ) + } + + // Cleanup fields that are not the responsibility of the in-place update extension. + // Remove in-place mutable fields. + cleanedUpPatchedMachineSet.Spec.Template = *mdutil.MachineTemplateDeepCopyRolloutFields(&cleanedUpPatchedMachineSet.Spec.Template) + cleanedUpDesiredMachineSet.Spec.Template = *mdutil.MachineTemplateDeepCopyRolloutFields(&cleanedUpDesiredMachineSet.Spec.Template) + // Set refs equal. + cleanedUpPatchedMachineSet.Spec.Template.Spec.Bootstrap.ConfigRef = cleanedUpDesiredMachineSet.Spec.Template.Spec.Bootstrap.ConfigRef + cleanedUpPatchedMachineSet.Spec.Template.Spec.InfrastructureRef = cleanedUpDesiredMachineSet.Spec.Template.Spec.InfrastructureRef + return compare.Diff(cleanedUpPatchedMachineSet, cleanedUpDesiredMachineSet) } func matchesUnstructuredSpec(patched, desired runtime.RawExtension) (equal bool, diff string, matchErr error) { diff --git a/test/e2e/cluster_in_place_update.go b/test/e2e/cluster_in_place_update.go index 8f8e7ef4ab37..0e7d7c6fe5b7 100644 --- a/test/e2e/cluster_in_place_update.go +++ b/test/e2e/cluster_in_place_update.go @@ -246,15 +246,16 @@ func ClusterInPlaceUpdateSpec(ctx context.Context, inputGetter func() ClusterInP Namespace: clusterResources.Cluster.Namespace, ConditionType: clusterv1.ClusterWorkerMachinesUpToDateCondition, }) - for _, kubeadmConfig := range machineObjectsAfterInPlaceUpdate.KubeadmConfigByMachine { - g.Expect(kubeadmConfig.Spec.Files).To(ContainElement(HaveField("Path", filePath))) - g.Expect(kubeadmConfig.Spec.Files).To(ContainElement(HaveField("Content", fileContent))) - } // Ensure only in-place updates were executed and no Machine was re-created. machineObjectsAfterInPlaceUpdate = getMachineObjects(ctx, g, mgmtClient, cluster) g.Expect(machineNames(machineObjectsAfterInPlaceUpdate.ControlPlaneMachines)).To(Equal(machineNames(machineObjectsBeforeInPlaceUpdate.ControlPlaneMachines))) g.Expect(machineNames(machineObjectsAfterInPlaceUpdate.WorkerMachines)).To(Equal(machineNames(machineObjectsBeforeInPlaceUpdate.WorkerMachines))) + + for _, kubeadmConfig := range machineObjectsAfterInPlaceUpdate.KubeadmConfigByMachine { + g.Expect(kubeadmConfig.Spec.Files).To(ContainElement(HaveField("Path", filePath))) + g.Expect(kubeadmConfig.Spec.Files).To(ContainElement(HaveField("Content", fileContent))) + } }, input.E2EConfig.GetIntervals(specName, "wait-control-plane")...).Should(Succeed()) // Update machineObjectsBeforeInPlaceUpdate for the next round of in-place update. From ab5a46e5ff903145331e934c5944dc6272f88979 Mon Sep 17 00:00:00 2001 From: Stefan Bueringer Date: Mon, 10 Nov 2025 10:22:17 +0100 Subject: [PATCH 3/4] Fix review findings --- .../controllers/inplace_canupdatemachine.go | 5 +- .../machinedeployment_canupdatemachineset.go | 47 ++++++++----------- ...hinedeployment_canupdatemachineset_test.go | 10 ++-- .../machinedeployment/mdutil/util.go | 28 +++++++---- internal/util/inplace/inplace.go | 32 +++++++++++++ 5 files changed, 80 insertions(+), 42 deletions(-) diff --git a/controlplane/kubeadm/internal/controllers/inplace_canupdatemachine.go b/controlplane/kubeadm/internal/controllers/inplace_canupdatemachine.go index 2f8a93e254f3..73cb3deb45d3 100644 --- a/controlplane/kubeadm/internal/controllers/inplace_canupdatemachine.go +++ b/controlplane/kubeadm/internal/controllers/inplace_canupdatemachine.go @@ -35,6 +35,7 @@ import ( "sigs.k8s.io/cluster-api/controlplane/kubeadm/internal" "sigs.k8s.io/cluster-api/feature" "sigs.k8s.io/cluster-api/internal/util/compare" + "sigs.k8s.io/cluster-api/internal/util/inplace" "sigs.k8s.io/cluster-api/internal/util/patch" "sigs.k8s.io/cluster-api/internal/util/ssa" ) @@ -307,10 +308,10 @@ func matchesMachineSpec(patched, desired *clusterv1.Machine) (equal bool, diff s // Note: Wrapping Machine specs in a Machine for proper formatting of the diff. return compare.Diff( &clusterv1.Machine{ - Spec: patched.Spec, + Spec: *inplace.CleanupMachineSpecForDiff(&patched.Spec), }, &clusterv1.Machine{ - Spec: desired.Spec, + Spec: *inplace.CleanupMachineSpecForDiff(&desired.Spec), }, ) } diff --git a/internal/controllers/machinedeployment/machinedeployment_canupdatemachineset.go b/internal/controllers/machinedeployment/machinedeployment_canupdatemachineset.go index 14709c1a8710..198d7ff67f8a 100644 --- a/internal/controllers/machinedeployment/machinedeployment_canupdatemachineset.go +++ b/internal/controllers/machinedeployment/machinedeployment_canupdatemachineset.go @@ -17,6 +17,7 @@ limitations under the License. package machinedeployment import ( + "cmp" "context" "fmt" "strings" @@ -31,8 +32,8 @@ import ( clusterv1 "sigs.k8s.io/cluster-api/api/core/v1beta2" runtimehooksv1 "sigs.k8s.io/cluster-api/api/runtime/hooks/v1alpha1" "sigs.k8s.io/cluster-api/controllers/external" - "sigs.k8s.io/cluster-api/internal/controllers/machinedeployment/mdutil" "sigs.k8s.io/cluster-api/internal/util/compare" + "sigs.k8s.io/cluster-api/internal/util/inplace" "sigs.k8s.io/cluster-api/internal/util/patch" ) @@ -236,10 +237,10 @@ func matchesMachineSet(req *runtimehooksv1.CanUpdateMachineSetRequest) (bool, [] if req.Current.BootstrapConfigTemplate.Object != nil && req.Desired.BootstrapConfigTemplate.Object != nil { match, diff, err = matchesUnstructuredSpec(req.Current.BootstrapConfigTemplate, req.Desired.BootstrapConfigTemplate) if err != nil { - return false, nil, errors.Wrapf(err, "failed to match BootstrapConfigTemplate") + return false, nil, errors.Wrapf(err, "failed to match %s", req.Current.BootstrapConfigTemplate.Object.GetObjectKind().GroupVersionKind().Kind) } if !match { - reasons = append(reasons, fmt.Sprintf("BootstrapConfigTemplate cannot be updated in-place: %s", diff)) + reasons = append(reasons, fmt.Sprintf("%s cannot be updated in-place: %s", req.Current.BootstrapConfigTemplate.Object.GetObjectKind().GroupVersionKind().Kind, diff)) } } match, diff, err = matchesUnstructuredSpec(req.Current.InfrastructureMachineTemplate, req.Desired.InfrastructureMachineTemplate) @@ -259,29 +260,21 @@ func matchesMachineSet(req *runtimehooksv1.CanUpdateMachineSetRequest) (bool, [] func matchesMachineSetSpec(patched, desired *clusterv1.MachineSet) (equal bool, diff string, matchErr error) { // Note: Wrapping MachineSet specs in a MachineSet for proper formatting of the diff. - cleanedUpPatchedMachineSet := &clusterv1.MachineSet{ - Spec: clusterv1.MachineSetSpec{ - Template: clusterv1.MachineTemplateSpec{ - Spec: patched.Spec.Template.Spec, + return compare.Diff( + &clusterv1.MachineSet{ + Spec: clusterv1.MachineSetSpec{ + Template: clusterv1.MachineTemplateSpec{ + Spec: *inplace.CleanupMachineSpecForDiff(&patched.Spec.Template.Spec), + }, }, - }, - } - cleanedUpDesiredMachineSet := &clusterv1.MachineSet{ - Spec: clusterv1.MachineSetSpec{ - Template: clusterv1.MachineTemplateSpec{ - Spec: desired.Spec.Template.Spec, + }, &clusterv1.MachineSet{ + Spec: clusterv1.MachineSetSpec{ + Template: clusterv1.MachineTemplateSpec{ + Spec: *inplace.CleanupMachineSpecForDiff(&desired.Spec.Template.Spec), + }, }, }, - } - - // Cleanup fields that are not the responsibility of the in-place update extension. - // Remove in-place mutable fields. - cleanedUpPatchedMachineSet.Spec.Template = *mdutil.MachineTemplateDeepCopyRolloutFields(&cleanedUpPatchedMachineSet.Spec.Template) - cleanedUpDesiredMachineSet.Spec.Template = *mdutil.MachineTemplateDeepCopyRolloutFields(&cleanedUpDesiredMachineSet.Spec.Template) - // Set refs equal. - cleanedUpPatchedMachineSet.Spec.Template.Spec.Bootstrap.ConfigRef = cleanedUpDesiredMachineSet.Spec.Template.Spec.Bootstrap.ConfigRef - cleanedUpPatchedMachineSet.Spec.Template.Spec.InfrastructureRef = cleanedUpDesiredMachineSet.Spec.Template.Spec.InfrastructureRef - return compare.Diff(cleanedUpPatchedMachineSet, cleanedUpDesiredMachineSet) + ) } func matchesUnstructuredSpec(patched, desired runtime.RawExtension) (equal bool, diff string, matchErr error) { @@ -335,23 +328,23 @@ func (p *rolloutPlanner) getTemplateObjects(ctx context.Context, oldMS, newMS *c templateObjects.CurrentInfraMachineTemplate, err = external.GetObjectFromContractVersionedRef(ctx, p.Client, oldMS.Spec.Template.Spec.InfrastructureRef, oldMS.Namespace) if err != nil { - return nil, errors.Wrapf(err, "failed to get InfrastructureMachineTemplate from MachineSet %s", oldMS.Name) + return nil, errors.Wrapf(err, "failed to get %s from MachineSet %s", cmp.Or(oldMS.Spec.Template.Spec.InfrastructureRef.Kind, "InfrastructureMachineTemplate"), oldMS.Name) } templateObjects.DesiredInfraMachineTemplate, err = external.GetObjectFromContractVersionedRef(ctx, p.Client, newMS.Spec.Template.Spec.InfrastructureRef, newMS.Namespace) if err != nil { - return nil, errors.Wrapf(err, "failed to get InfrastructureMachineTemplate from MachineSet %s", newMS.Name) + return nil, errors.Wrapf(err, "failed to get %s from MachineSet %s", cmp.Or(newMS.Spec.Template.Spec.InfrastructureRef.Kind, "InfrastructureMachineTemplate"), newMS.Name) } if oldMS.Spec.Template.Spec.Bootstrap.ConfigRef.IsDefined() { templateObjects.CurrentBootstrapConfigTemplate, err = external.GetObjectFromContractVersionedRef(ctx, p.Client, oldMS.Spec.Template.Spec.Bootstrap.ConfigRef, oldMS.Namespace) if err != nil { - return nil, errors.Wrapf(err, "failed to get BootstrapConfigTemplate from MachineSet %s", oldMS.Name) + return nil, errors.Wrapf(err, "failed to get %s from MachineSet %s", cmp.Or(oldMS.Spec.Template.Spec.Bootstrap.ConfigRef.Kind, "BootstrapConfigTemplate"), oldMS.Name) } } if newMS.Spec.Template.Spec.Bootstrap.ConfigRef.IsDefined() { templateObjects.DesiredBootstrapConfigTemplate, err = external.GetObjectFromContractVersionedRef(ctx, p.Client, newMS.Spec.Template.Spec.Bootstrap.ConfigRef, newMS.Namespace) if err != nil { - return nil, errors.Wrapf(err, "failed to get BootstrapConfigTemplate from MachineSet %s", newMS.Name) + return nil, errors.Wrapf(err, "failed to get %s from MachineSet %s", cmp.Or(newMS.Spec.Template.Spec.Bootstrap.ConfigRef.Kind, "BootstrapConfigTemplate"), newMS.Name) } } diff --git a/internal/controllers/machinedeployment/machinedeployment_canupdatemachineset_test.go b/internal/controllers/machinedeployment/machinedeployment_canupdatemachineset_test.go index 878202e30867..0169dd63ff70 100644 --- a/internal/controllers/machinedeployment/machinedeployment_canupdatemachineset_test.go +++ b/internal/controllers/machinedeployment/machinedeployment_canupdatemachineset_test.go @@ -41,7 +41,7 @@ import ( "sigs.k8s.io/cluster-api/util/test/builder" ) -func Test_canUpdateMachine(t *testing.T) { +func Test_canUpdateMachineSetInPlace(t *testing.T) { ns := metav1.NamespaceDefault oldMS := &clusterv1.MachineSet{ ObjectMeta: metav1.ObjectMeta{ @@ -141,7 +141,7 @@ func Test_canUpdateMachine(t *testing.T) { wantErrorMessage: "found multiple CanUpdateMachineSet hooks (test-update-extension-1,test-update-extension-2): only one hook is supported", }, { - name: "Return false if canExtensionsUpdateMachine returns false", + name: "Return false if canExtensionsUpdateMachineSet returns false", oldMS: oldMS, newMS: newMS, oldMSInfrastructureMachineTemplate: oldMSInfrastructureMachineTemplate, @@ -161,7 +161,7 @@ func Test_canUpdateMachine(t *testing.T) { wantCanUpdateMachineSet: false, }, { - name: "Return true if canExtensionsUpdateMachine returns true", + name: "Return true if canExtensionsUpdateMachineSet returns true", oldMS: oldMS, newMS: newMS, oldMSInfrastructureMachineTemplate: oldMSInfrastructureMachineTemplate, @@ -403,7 +403,7 @@ func Test_canExtensionsUpdateMachineSet(t *testing.T) { }, Status: {}, }`, - `BootstrapConfigTemplate cannot be updated in-place: &unstructured.Unstructured{ + `TestBootstrapConfigTemplate cannot be updated in-place: &unstructured.Unstructured{ - Object: map[string]any{ - "spec": map[string]any{ - "template": map[string]any{"spec": map[string]any{"hello": string("world BootstrapConfigTemplate")}}, @@ -519,7 +519,7 @@ func Test_canExtensionsUpdateMachineSet(t *testing.T) { }, wantCanUpdateMachineSet: false, wantReasons: []string{ - `BootstrapConfigTemplate cannot be updated in-place: &unstructured.Unstructured{ + `TestBootstrapConfigTemplate cannot be updated in-place: &unstructured.Unstructured{ - Object: map[string]any{ - "spec": map[string]any{ - "template": map[string]any{"spec": map[string]any{"hello": string("world BootstrapConfigTemplate")}}, diff --git a/internal/controllers/machinedeployment/mdutil/util.go b/internal/controllers/machinedeployment/mdutil/util.go index f5839b2313fb..f98d4ba4475f 100644 --- a/internal/controllers/machinedeployment/mdutil/util.go +++ b/internal/controllers/machinedeployment/mdutil/util.go @@ -412,22 +412,34 @@ func MachineTemplateUpToDate(current, desired *clusterv1.MachineTemplateSpec) (b // MachineTemplateDeepCopyRolloutFields copies a MachineTemplateSpec // and sets all fields that should be propagated in-place to nil and drops version from // external references. +// Note: Please update inplace.CleanupMachineSpecForDiff accordingly if necessary. func MachineTemplateDeepCopyRolloutFields(template *clusterv1.MachineTemplateSpec) *clusterv1.MachineTemplateSpec { templateCopy := template.DeepCopy() + spec := templateCopy.Spec - // Moving MD from one cluster to another is not supported. - templateCopy.Spec.ClusterName = "" + // The following fields are set to their zero value so they are omitted from the comparison, + // because they should never be the reason for a rollout. // Drop labels and annotations templateCopy.Labels = nil templateCopy.Annotations = nil - // Drop node timeout values - templateCopy.Spec.MinReadySeconds = nil - templateCopy.Spec.ReadinessGates = nil - templateCopy.Spec.Deletion.NodeDrainTimeoutSeconds = nil - templateCopy.Spec.Deletion.NodeDeletionTimeoutSeconds = nil - templateCopy.Spec.Deletion.NodeVolumeDetachTimeoutSeconds = nil + // Should never change. + spec.ClusterName = "" + + // Bootstrap and InfrastructureRef should be compared. + + // Should not be set. + spec.ProviderID = "" + + // Version & FailureDomain should be compared. + + // Fields that are mutated in-place without a rollout. + spec.MinReadySeconds = nil + spec.ReadinessGates = nil + spec.Deletion.NodeDrainTimeoutSeconds = nil + spec.Deletion.NodeVolumeDetachTimeoutSeconds = nil + spec.Deletion.NodeDeletionTimeoutSeconds = nil return templateCopy } diff --git a/internal/util/inplace/inplace.go b/internal/util/inplace/inplace.go index 94d976cacf71..219d90eff676 100644 --- a/internal/util/inplace/inplace.go +++ b/internal/util/inplace/inplace.go @@ -34,3 +34,35 @@ func IsUpdateInProgress(machine *clusterv1.Machine) bool { return inPlaceUpdateInProgress || hasUpdateMachinePending } + +// CleanupMachineSpecForDiff cleans up a MachineSpec for diff. +// Note: Please update mdutil.MachineTemplateDeepCopyRolloutFields accordingly if necessary. +func CleanupMachineSpecForDiff(spec *clusterv1.MachineSpec) *clusterv1.MachineSpec { + spec = spec.DeepCopy() + + // The following fields are set to their zero value so they are omitted from the comparison, + // because they should never be the reason for an in-place update. + + // Should never change. + spec.ClusterName = "" + + // Machine: should never change. + // MachineSet: not responsibility of the in-place update extension. + spec.Bootstrap = clusterv1.Bootstrap{} + spec.InfrastructureRef = clusterv1.ContractVersionedObjectReference{} + + // Machine: should never change. + // MachineSet: should not be set. + spec.ProviderID = "" + + // Version & FailureDomain should be compared. + + // Fields that are mutated in-place without a rollout. + spec.MinReadySeconds = nil + spec.ReadinessGates = nil + spec.Deletion.NodeDrainTimeoutSeconds = nil + spec.Deletion.NodeVolumeDetachTimeoutSeconds = nil + spec.Deletion.NodeDeletionTimeoutSeconds = nil + + return spec +} From ece47f30cd1c4868f6378f3d34d2bcaefd0f2ba6 Mon Sep 17 00:00:00 2001 From: Stefan Bueringer Date: Mon, 10 Nov 2025 14:14:55 +0100 Subject: [PATCH 4/4] Fix review findings --- .../machinedeployment_canupdatemachineset_test.go | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/internal/controllers/machinedeployment/machinedeployment_canupdatemachineset_test.go b/internal/controllers/machinedeployment/machinedeployment_canupdatemachineset_test.go index 0169dd63ff70..493547073076 100644 --- a/internal/controllers/machinedeployment/machinedeployment_canupdatemachineset_test.go +++ b/internal/controllers/machinedeployment/machinedeployment_canupdatemachineset_test.go @@ -251,6 +251,18 @@ func Test_canExtensionsUpdateMachineSet(t *testing.T) { Template: clusterv1.MachineTemplateSpec{ Spec: clusterv1.MachineSpec{ Version: "v1.30.0", + Bootstrap: clusterv1.Bootstrap{ + ConfigRef: clusterv1.ContractVersionedObjectReference{ + APIGroup: builder.BootstrapGroupVersion.Group, + Kind: builder.TestBootstrapConfigTemplateKind, + Name: "bootstrap-config-template-1", + }, + }, + InfrastructureRef: clusterv1.ContractVersionedObjectReference{ + APIGroup: builder.InfrastructureGroupVersion.Group, + Kind: builder.TestInfrastructureMachineTemplateKind, + Name: "infrastructure-machine-template-1", + }, }, }, }, @@ -258,6 +270,9 @@ func Test_canExtensionsUpdateMachineSet(t *testing.T) { desiredMachineSet := currentMachineSet.DeepCopy() desiredMachineSet.Name = "new-machineset" desiredMachineSet.Spec.Template.Spec.Version = "v1.31.0" + // Note: Changes in refs should not influence the in-place rollout decision. + desiredMachineSet.Spec.Template.Spec.Bootstrap.ConfigRef.Name = "bootstrap-config-template-2" + desiredMachineSet.Spec.Template.Spec.InfrastructureRef.Name = "infrastructure-machine-template-2" currentBootstrapConfigTemplate := &unstructured.Unstructured{ Object: map[string]interface{}{