Skip to content

Commit 944b0c2

Browse files
sbueringersbueringer
committed
Fix review findings
1 parent aabc505 commit 944b0c2

File tree

5 files changed

+61
-34
lines changed

5 files changed

+61
-34
lines changed

‎controlplane/kubeadm/internal/controllers/inplace_canupdatemachine.go‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@ import (
3535
"sigs.k8s.io/cluster-api/controlplane/kubeadm/internal"
3636
"sigs.k8s.io/cluster-api/feature"
3737
"sigs.k8s.io/cluster-api/internal/util/compare"
38+
"sigs.k8s.io/cluster-api/internal/util/inplace"
3839
"sigs.k8s.io/cluster-api/internal/util/patch"
3940
"sigs.k8s.io/cluster-api/internal/util/ssa"
4041
)
@@ -307,10 +308,10 @@ func matchesMachineSpec(patched, desired *clusterv1.Machine) (equal bool, diff s
307308
// Note: Wrapping Machine specs in a Machine for proper formatting of the diff.
308309
return compare.Diff(
309310
&clusterv1.Machine{
310-
Spec: patched.Spec,
311+
Spec: *inplace.CleanupMachineSpecForDiff(&patched.Spec),
311312
},
312313
&clusterv1.Machine{
313-
Spec: desired.Spec,
314+
Spec: *inplace.CleanupMachineSpecForDiff(&desired.Spec),
314315
},
315316
)
316317
}

‎internal/controllers/machinedeployment/machinedeployment_canupdatemachineset.go‎

Lines changed: 20 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ limitations under the License.
1717
package machinedeployment
1818

1919
import (
20+
"cmp"
2021
"context"
2122
"fmt"
2223
"strings"
@@ -31,8 +32,8 @@ import (
3132
clusterv1 "sigs.k8s.io/cluster-api/api/core/v1beta2"
3233
runtimehooksv1 "sigs.k8s.io/cluster-api/api/runtime/hooks/v1alpha1"
3334
"sigs.k8s.io/cluster-api/controllers/external"
34-
"sigs.k8s.io/cluster-api/internal/controllers/machinedeployment/mdutil"
3535
"sigs.k8s.io/cluster-api/internal/util/compare"
36+
"sigs.k8s.io/cluster-api/internal/util/inplace"
3637
"sigs.k8s.io/cluster-api/internal/util/patch"
3738
)
3839

@@ -236,10 +237,10 @@ func matchesMachineSet(req *runtimehooksv1.CanUpdateMachineSetRequest) (bool, []
236237
if req.Current.BootstrapConfigTemplate.Object != nil && req.Desired.BootstrapConfigTemplate.Object != nil {
237238
match, diff, err = matchesUnstructuredSpec(req.Current.BootstrapConfigTemplate, req.Desired.BootstrapConfigTemplate)
238239
if err != nil {
239-
return false, nil, errors.Wrapf(err, "failed to match BootstrapConfigTemplate")
240+
return false, nil, errors.Wrapf(err, "failed to match %s", req.Current.BootstrapConfigTemplate.Object.GetObjectKind().GroupVersionKind().Kind)
240241
}
241242
if !match {
242-
reasons = append(reasons, fmt.Sprintf("BootstrapConfigTemplate cannot be updated in-place: %s", diff))
243+
reasons = append(reasons, fmt.Sprintf("%s cannot be updated in-place: %s", req.Current.BootstrapConfigTemplate.Object.GetObjectKind().GroupVersionKind().Kind, diff))
243244
}
244245
}
245246
match, diff, err = matchesUnstructuredSpec(req.Current.InfrastructureMachineTemplate, req.Desired.InfrastructureMachineTemplate)
@@ -259,29 +260,21 @@ func matchesMachineSet(req *runtimehooksv1.CanUpdateMachineSetRequest) (bool, []
259260

260261
func matchesMachineSetSpec(patched, desired *clusterv1.MachineSet) (equal bool, diff string, matchErr error) {
261262
// Note: Wrapping MachineSet specs in a MachineSet for proper formatting of the diff.
262-
cleanedUpPatchedMachineSet := &clusterv1.MachineSet{
263-
Spec: clusterv1.MachineSetSpec{
264-
Template: clusterv1.MachineTemplateSpec{
265-
Spec: patched.Spec.Template.Spec,
263+
return compare.Diff(
264+
&clusterv1.MachineSet{
265+
Spec: clusterv1.MachineSetSpec{
266+
Template: clusterv1.MachineTemplateSpec{
267+
Spec: *inplace.CleanupMachineSpecForDiff(&patched.Spec.Template.Spec),
268+
},
266269
},
267-
},
268-
}
269-
cleanedUpDesiredMachineSet := &clusterv1.MachineSet{
270-
Spec: clusterv1.MachineSetSpec{
271-
Template: clusterv1.MachineTemplateSpec{
272-
Spec: desired.Spec.Template.Spec,
270+
}, &clusterv1.MachineSet{
271+
Spec: clusterv1.MachineSetSpec{
272+
Template: clusterv1.MachineTemplateSpec{
273+
Spec: *inplace.CleanupMachineSpecForDiff(&desired.Spec.Template.Spec),
274+
},
273275
},
274276
},
275-
}
276-
277-
// Cleanup fields that are not the responsibility of the in-place update extension.
278-
// Remove in-place mutable fields.
279-
cleanedUpPatchedMachineSet.Spec.Template = *mdutil.MachineTemplateDeepCopyRolloutFields(&cleanedUpPatchedMachineSet.Spec.Template)
280-
cleanedUpDesiredMachineSet.Spec.Template = *mdutil.MachineTemplateDeepCopyRolloutFields(&cleanedUpDesiredMachineSet.Spec.Template)
281-
// Set refs equal.
282-
cleanedUpPatchedMachineSet.Spec.Template.Spec.Bootstrap.ConfigRef = cleanedUpDesiredMachineSet.Spec.Template.Spec.Bootstrap.ConfigRef
283-
cleanedUpPatchedMachineSet.Spec.Template.Spec.InfrastructureRef = cleanedUpDesiredMachineSet.Spec.Template.Spec.InfrastructureRef
284-
return compare.Diff(cleanedUpPatchedMachineSet, cleanedUpDesiredMachineSet)
277+
)
285278
}
286279

287280
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
335328

336329
templateObjects.CurrentInfraMachineTemplate, err = external.GetObjectFromContractVersionedRef(ctx, p.Client, oldMS.Spec.Template.Spec.InfrastructureRef, oldMS.Namespace)
337330
if err != nil {
338-
return nil, errors.Wrapf(err, "failed to get InfrastructureMachineTemplate from MachineSet %s", oldMS.Name)
331+
return nil, errors.Wrapf(err, "failed to get %s from MachineSet %s", cmp.Or(oldMS.Spec.Template.Spec.InfrastructureRef.Kind, "InfrastructureMachineTemplate"), oldMS.Name)
339332
}
340333
templateObjects.DesiredInfraMachineTemplate, err = external.GetObjectFromContractVersionedRef(ctx, p.Client, newMS.Spec.Template.Spec.InfrastructureRef, newMS.Namespace)
341334
if err != nil {
342-
return nil, errors.Wrapf(err, "failed to get InfrastructureMachineTemplate from MachineSet %s", newMS.Name)
335+
return nil, errors.Wrapf(err, "failed to get %s from MachineSet %s", cmp.Or(newMS.Spec.Template.Spec.InfrastructureRef.Kind, "InfrastructureMachineTemplate"), newMS.Name)
343336
}
344337

345338
if oldMS.Spec.Template.Spec.Bootstrap.ConfigRef.IsDefined() {
346339
templateObjects.CurrentBootstrapConfigTemplate, err = external.GetObjectFromContractVersionedRef(ctx, p.Client, oldMS.Spec.Template.Spec.Bootstrap.ConfigRef, oldMS.Namespace)
347340
if err != nil {
348-
return nil, errors.Wrapf(err, "failed to get BootstrapConfigTemplate from MachineSet %s", oldMS.Name)
341+
return nil, errors.Wrapf(err, "failed to get %s from MachineSet %s", cmp.Or(oldMS.Spec.Template.Spec.Bootstrap.ConfigRef.Kind, "BootstrapConfigTemplate"), oldMS.Name)
349342
}
350343
}
351344
if newMS.Spec.Template.Spec.Bootstrap.ConfigRef.IsDefined() {
352345
templateObjects.DesiredBootstrapConfigTemplate, err = external.GetObjectFromContractVersionedRef(ctx, p.Client, newMS.Spec.Template.Spec.Bootstrap.ConfigRef, newMS.Namespace)
353346
if err != nil {
354-
return nil, errors.Wrapf(err, "failed to get BootstrapConfigTemplate from MachineSet %s", newMS.Name)
347+
return nil, errors.Wrapf(err, "failed to get %s from MachineSet %s", cmp.Or(newMS.Spec.Template.Spec.Bootstrap.ConfigRef.Kind, "BootstrapConfigTemplate"), newMS.Name)
355348
}
356349
}
357350

‎internal/controllers/machinedeployment/machinedeployment_canupdatemachineset_test.go‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,7 @@ import (
4141
"sigs.k8s.io/cluster-api/util/test/builder"
4242
)
4343

44-
func Test_canUpdateMachine(t *testing.T) {
44+
func Test_canUpdateMachineSetInPlace(t *testing.T) {
4545
ns := metav1.NamespaceDefault
4646
oldMS := &clusterv1.MachineSet{
4747
ObjectMeta: metav1.ObjectMeta{
@@ -141,7 +141,7 @@ func Test_canUpdateMachine(t *testing.T) {
141141
wantErrorMessage: "found multiple CanUpdateMachineSet hooks (test-update-extension-1,test-update-extension-2): only one hook is supported",
142142
},
143143
{
144-
name: "Return false if canExtensionsUpdateMachine returns false",
144+
name: "Return false if canExtensionsUpdateMachineSet returns false",
145145
oldMS: oldMS,
146146
newMS: newMS,
147147
oldMSInfrastructureMachineTemplate: oldMSInfrastructureMachineTemplate,
@@ -161,7 +161,7 @@ func Test_canUpdateMachine(t *testing.T) {
161161
wantCanUpdateMachineSet: false,
162162
},
163163
{
164-
name: "Return true if canExtensionsUpdateMachine returns true",
164+
name: "Return true if canExtensionsUpdateMachineSet returns true",
165165
oldMS: oldMS,
166166
newMS: newMS,
167167
oldMSInfrastructureMachineTemplate: oldMSInfrastructureMachineTemplate,
@@ -403,7 +403,7 @@ func Test_canExtensionsUpdateMachineSet(t *testing.T) {
403403
},
404404
Status: {},
405405
}`,
406-
`BootstrapConfigTemplate cannot be updated in-place: &unstructured.Unstructured{
406+
`TestBootstrapConfigTemplate cannot be updated in-place: &unstructured.Unstructured{
407407
- Object: map[string]any{
408408
- "spec": map[string]any{
409409
- "template": map[string]any{"spec": map[string]any{"hello": string("world BootstrapConfigTemplate")}},
@@ -519,7 +519,7 @@ func Test_canExtensionsUpdateMachineSet(t *testing.T) {
519519
},
520520
wantCanUpdateMachineSet: false,
521521
wantReasons: []string{
522-
`BootstrapConfigTemplate cannot be updated in-place: &unstructured.Unstructured{
522+
`TestBootstrapConfigTemplate cannot be updated in-place: &unstructured.Unstructured{
523523
- Object: map[string]any{
524524
- "spec": map[string]any{
525525
- "template": map[string]any{"spec": map[string]any{"hello": string("world BootstrapConfigTemplate")}},

‎internal/controllers/machinedeployment/mdutil/util.go‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -412,6 +412,7 @@ func MachineTemplateUpToDate(current, desired *clusterv1.MachineTemplateSpec) (b
412412
// MachineTemplateDeepCopyRolloutFields copies a MachineTemplateSpec
413413
// and sets all fields that should be propagated in-place to nil and drops version from
414414
// external references.
415+
// Note: Please update inplace.CleanupMachineSpecForDiff accordingly if necessary.
415416
func MachineTemplateDeepCopyRolloutFields(template *clusterv1.MachineTemplateSpec) *clusterv1.MachineTemplateSpec {
416417
templateCopy := template.DeepCopy()
417418

‎internal/util/inplace/inplace.go‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,3 +34,35 @@ func IsUpdateInProgress(machine *clusterv1.Machine) bool {
3434

3535
return inPlaceUpdateInProgress || hasUpdateMachinePending
3636
}
37+
38+
// CleanupMachineSpecForDiff cleans up a MachineSpec for diff.
39+
// Note: Please update mdutil.MachineTemplateDeepCopyRolloutFields accordingly if necessary.
40+
func CleanupMachineSpecForDiff(spec *clusterv1.MachineSpec) *clusterv1.MachineSpec {
41+
spec = spec.DeepCopy()
42+
43+
// The following fields are set to their zero value so they are omitted from the comparison,
44+
// because they should never be the reason for an in-place update.
45+
46+
// Should never change.
47+
spec.ClusterName = ""
48+
49+
// Machine: should never change.
50+
// MachineSet: not responsibility of the in-place update extension.
51+
spec.Bootstrap = clusterv1.Bootstrap{}
52+
spec.InfrastructureRef = clusterv1.ContractVersionedObjectReference{}
53+
54+
// Machine: should never change.
55+
// MachineSet: should not be set.
56+
spec.ProviderID = ""
57+
58+
// Version & FailureDomain should be compared.
59+
60+
// Fields that are mutated in-place without a rollout.
61+
spec.MinReadySeconds = nil
62+
spec.ReadinessGates = nil
63+
spec.Deletion.NodeDrainTimeoutSeconds = nil
64+
spec.Deletion.NodeVolumeDetachTimeoutSeconds = nil
65+
spec.Deletion.NodeDeletionTimeoutSeconds = nil
66+
67+
return spec
68+
}

0 commit comments

Comments
 (0)