From 769af5872dc9a7d9fecdd6f782d763a24ce8e697 Mon Sep 17 00:00:00 2001 From: fabriziopandini Date: Fri, 8 Nov 2024 14:11:33 +0100 Subject: [PATCH] Add machine UpToDate condition to MachineSet --- .../machinedeployment/mdutil/util.go | 67 ++- .../machinedeployment/mdutil/util_test.go | 439 ++++++------------ .../machineset/machineset_controller.go | 82 +++- .../machineset/machineset_controller_test.go | 180 +++++++ 4 files changed, 445 insertions(+), 323 deletions(-) diff --git a/internal/controllers/machinedeployment/mdutil/util.go b/internal/controllers/machinedeployment/mdutil/util.go index 21b119a1861e..bf0694859cdb 100644 --- a/internal/controllers/machinedeployment/mdutil/util.go +++ b/internal/controllers/machinedeployment/mdutil/util.go @@ -20,6 +20,7 @@ package mdutil import ( "context" "fmt" + "reflect" "sort" "strconv" "strings" @@ -38,7 +39,6 @@ import ( ctrl "sigs.k8s.io/controller-runtime" clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" - "sigs.k8s.io/cluster-api/internal/util/compare" "sigs.k8s.io/cluster-api/util/conversion" ) @@ -371,13 +371,50 @@ func getMachineSetFraction(ms clusterv1.MachineSet, md clusterv1.MachineDeployme return integer.RoundToInt32(newMSsize) - *(ms.Spec.Replicas) } -// EqualMachineTemplate returns true if two given machineTemplateSpec are equal, -// ignoring all the in-place propagated fields, and the version from external references. -func EqualMachineTemplate(template1, template2 *clusterv1.MachineTemplateSpec) (equal bool, diff string, err error) { - t1Copy := MachineTemplateDeepCopyRolloutFields(template1) - t2Copy := MachineTemplateDeepCopyRolloutFields(template2) +// MachineTemplateUpToDate returns true if the current MachineTemplateSpec is up-to-date with a corresponding desired MachineTemplateSpec. +// Note: The comparison does not consider any in-place propagated fields, as well as the version from external references. +func MachineTemplateUpToDate(current, desired *clusterv1.MachineTemplateSpec) (upToDate bool, logMessages, conditionMessages []string) { + currentCopy := MachineTemplateDeepCopyRolloutFields(current) + desiredCopy := MachineTemplateDeepCopyRolloutFields(desired) - return compare.Diff(t1Copy, t2Copy) + if !reflect.DeepEqual(currentCopy.Spec.Version, desiredCopy.Spec.Version) { + logMessages = append(logMessages, fmt.Sprintf("spec.version %s, %s required", ptr.Deref(currentCopy.Spec.Version, "nil"), ptr.Deref(desiredCopy.Spec.Version, "nil"))) + conditionMessages = append(conditionMessages, fmt.Sprintf("Version %s, %s required", ptr.Deref(currentCopy.Spec.Version, "nil"), ptr.Deref(desiredCopy.Spec.Version, "nil"))) + } + + // Note: we return a message based on desired.bootstrap.ConfigRef != nil, but we always compare the entire bootstrap + // struct to catch cases when either configRef or dataSecretName is set in current vs desired (usually MachineTemplates + // have ConfigRef != nil, might be in some edge case dataSecret are used, but switching from one to another is not a + // common operation so it is acceptable to handle it in this way). + if currentCopy.Spec.Bootstrap.ConfigRef != nil { + if !reflect.DeepEqual(currentCopy.Spec.Bootstrap, desiredCopy.Spec.Bootstrap) { + logMessages = append(logMessages, fmt.Sprintf("spec.bootstrap.configRef %s %s, %s %s required", currentCopy.Spec.Bootstrap.ConfigRef.Kind, currentCopy.Spec.Bootstrap.ConfigRef.Name, ptr.Deref(desiredCopy.Spec.Bootstrap.ConfigRef, corev1.ObjectReference{}).Kind, ptr.Deref(desiredCopy.Spec.Bootstrap.ConfigRef, corev1.ObjectReference{}).Name)) + // Note: dropping "Template" suffix because conditions message will surface on machine. + conditionMessages = append(conditionMessages, fmt.Sprintf("%s is not up-to-date", strings.TrimSuffix(currentCopy.Spec.Bootstrap.ConfigRef.Kind, clusterv1.TemplateSuffix))) + } + } else { + if !reflect.DeepEqual(currentCopy.Spec.Bootstrap, desiredCopy.Spec.Bootstrap) { + logMessages = append(logMessages, fmt.Sprintf("spec.bootstrap.dataSecretName %s, %s required", ptr.Deref(currentCopy.Spec.Bootstrap.DataSecretName, "nil"), ptr.Deref(desiredCopy.Spec.Bootstrap.DataSecretName, "nil"))) + conditionMessages = append(conditionMessages, fmt.Sprintf("spec.bootstrap.dataSecretName %s, %s required", ptr.Deref(currentCopy.Spec.Bootstrap.DataSecretName, "nil"), ptr.Deref(desiredCopy.Spec.Bootstrap.DataSecretName, "nil"))) + } + } + + if !reflect.DeepEqual(currentCopy.Spec.InfrastructureRef, desiredCopy.Spec.InfrastructureRef) { + logMessages = append(logMessages, fmt.Sprintf("spec.infrastructureRef %s %s, %s %s required", currentCopy.Spec.InfrastructureRef.Kind, currentCopy.Spec.InfrastructureRef.Name, desiredCopy.Spec.InfrastructureRef.Kind, desiredCopy.Spec.InfrastructureRef.Name)) + // Note: dropping "Template" suffix because conditions message will surface on machine. + conditionMessages = append(conditionMessages, fmt.Sprintf("%s is not up-to-date", strings.TrimSuffix(currentCopy.Spec.InfrastructureRef.Kind, clusterv1.TemplateSuffix))) + } + + if !reflect.DeepEqual(currentCopy.Spec.FailureDomain, desiredCopy.Spec.FailureDomain) { + logMessages = append(logMessages, fmt.Sprintf("spec.failureDomain %s, %s required", ptr.Deref(currentCopy.Spec.FailureDomain, "nil"), ptr.Deref(desiredCopy.Spec.FailureDomain, "nil"))) + conditionMessages = append(conditionMessages, fmt.Sprintf("Failure domain %s, %s required", ptr.Deref(currentCopy.Spec.FailureDomain, "nil"), ptr.Deref(desiredCopy.Spec.FailureDomain, "nil"))) + } + + if len(logMessages) > 0 || len(conditionMessages) > 0 { + return false, logMessages, conditionMessages + } + + return true, nil, nil } // MachineTemplateDeepCopyRolloutFields copies a MachineTemplateSpec @@ -386,6 +423,9 @@ func EqualMachineTemplate(template1, template2 *clusterv1.MachineTemplateSpec) ( func MachineTemplateDeepCopyRolloutFields(template *clusterv1.MachineTemplateSpec) *clusterv1.MachineTemplateSpec { templateCopy := template.DeepCopy() + // Moving MD from one cluster to another is not supported. + templateCopy.Spec.ClusterName = "" + // Drop labels and annotations templateCopy.Labels = nil templateCopy.Annotations = nil @@ -413,7 +453,7 @@ func MachineTemplateDeepCopyRolloutFields(template *clusterv1.MachineTemplateSpe // NOTE: If we find a matching MachineSet which only differs in in-place mutable fields we can use it to // fulfill the intent of the MachineDeployment by just updating the MachineSet to propagate in-place mutable fields. // Thus we don't have to create a new MachineSet and we can avoid an unnecessary rollout. -// NOTE: Even after we changed EqualMachineTemplate to ignore fields that are propagated in-place we can guarantee that if there exists a "new machineset" +// NOTE: Even after we changed MachineTemplateUpToDate to ignore fields that are propagated in-place we can guarantee that if there exists a "new machineset" // using the old logic then a new machineset will definitely exist using the new logic. The new logic is looser. Therefore, we will // not face a case where there exists a machine set matching the old logic but there does not exist a machineset matching the new logic. // In fact previously not matching MS can now start matching the target. Since there could be multiple matches, lets choose the @@ -432,19 +472,16 @@ func FindNewMachineSet(deployment *clusterv1.MachineDeployment, msList []*cluste var matchingMachineSets []*clusterv1.MachineSet var diffs []string for _, ms := range msList { - equal, diff, err := EqualMachineTemplate(&ms.Spec.Template, &deployment.Spec.Template) - if err != nil { - return nil, "", errors.Wrapf(err, "failed to compare MachineDeployment spec template with MachineSet %s", ms.Name) - } - if equal { + upToDate, logMessages, _ := MachineTemplateUpToDate(&ms.Spec.Template, &deployment.Spec.Template) + if upToDate { matchingMachineSets = append(matchingMachineSets, ms) } else { - diffs = append(diffs, fmt.Sprintf("MachineSet %s: diff: %s", ms.Name, diff)) + diffs = append(diffs, fmt.Sprintf("MachineSet %s: diff: %s", ms.Name, strings.Join(logMessages, ", "))) } } if len(matchingMachineSets) == 0 { - return nil, fmt.Sprintf("couldn't find MachineSet matching MachineDeployment spec template: %s", strings.Join(diffs, ",")), nil + return nil, fmt.Sprintf("couldn't find MachineSet matching MachineDeployment spec template: %s", strings.Join(diffs, "; ")), nil } // If RolloutAfter is not set, pick the first matching MachineSet. diff --git a/internal/controllers/machinedeployment/mdutil/util_test.go b/internal/controllers/machinedeployment/mdutil/util_test.go index e178c47e35af..db2e958eaccf 100644 --- a/internal/controllers/machinedeployment/mdutil/util_test.go +++ b/internal/controllers/machinedeployment/mdutil/util_test.go @@ -171,7 +171,7 @@ func TestMachineSetsByDecreasingReplicas(t *testing.T) { } } -func TestEqualMachineTemplate(t *testing.T) { +func TestMachineTemplateUpToDate(t *testing.T) { machineTemplate := &clusterv1.MachineTemplateSpec{ ObjectMeta: clusterv1.ObjectMeta{ Labels: map[string]string{"l1": "v1"}, @@ -187,14 +187,14 @@ func TestEqualMachineTemplate(t *testing.T) { InfrastructureRef: corev1.ObjectReference{ Name: "infra1", Namespace: "default", - Kind: "InfrastructureMachine", + Kind: "InfrastructureMachineTemplate", APIVersion: "infrastructure.cluster.x-k8s.io/v1beta1", }, Bootstrap: clusterv1.Bootstrap{ ConfigRef: &corev1.ObjectReference{ Name: "bootstrap1", Namespace: "default", - Kind: "BootstrapConfig", + Kind: "BootstrapConfigTemplate", APIVersion: "bootstrap.cluster.x-k8s.io/v1beta1", }, }, @@ -236,9 +236,12 @@ func TestEqualMachineTemplate(t *testing.T) { machineTemplateWithDifferentInfraRefAPIVersion := machineTemplate.DeepCopy() machineTemplateWithDifferentInfraRefAPIVersion.Spec.InfrastructureRef.APIVersion = "infrastructure.cluster.x-k8s.io/v1beta2" - machineTemplateWithDifferentBootstrap := machineTemplate.DeepCopy() - machineTemplateWithDifferentBootstrap.Spec.Bootstrap.ConfigRef = nil - machineTemplateWithDifferentBootstrap.Spec.Bootstrap.DataSecretName = ptr.To("data-secret") + machineTemplateWithBootstrapDataSecret := machineTemplate.DeepCopy() + machineTemplateWithBootstrapDataSecret.Spec.Bootstrap.ConfigRef = nil + machineTemplateWithBootstrapDataSecret.Spec.Bootstrap.DataSecretName = ptr.To("data-secret1") + + machineTemplateWithDifferentBootstrapDataSecret := machineTemplateWithBootstrapDataSecret.DeepCopy() + machineTemplateWithDifferentBootstrapDataSecret.Spec.Bootstrap.DataSecretName = ptr.To("data-secret2") machineTemplateWithDifferentBootstrapConfigRef := machineTemplate.DeepCopy() machineTemplateWithDifferentBootstrapConfigRef.Spec.Bootstrap.ConfigRef.Name = "bootstrap2" @@ -247,279 +250,122 @@ func TestEqualMachineTemplate(t *testing.T) { machineTemplateWithDifferentBootstrapConfigRefAPIVersion.Spec.Bootstrap.ConfigRef.APIVersion = "bootstrap.cluster.x-k8s.io/v1beta2" tests := []struct { - Name string - Former, Latter *clusterv1.MachineTemplateSpec - Expected bool - Diff1 string - Diff2 string + Name string + current, desired *clusterv1.MachineTemplateSpec + expectedUpToDate bool + expectedLogMessages1 []string + expectedLogMessages2 []string + expectedConditionMessages1 []string + expectedConditionMessages2 []string }{ { Name: "Same spec", // Note: This test ensures that two MachineTemplates are equal even if the pointers differ. - Former: machineTemplate, - Latter: machineTemplateEqual, - Expected: true, - }, - { - Name: "Same spec, except latter does not have labels", - Former: machineTemplate, - Latter: machineTemplateWithEmptyLabels, - Expected: true, - }, - { - Name: "Same spec, except latter has different labels", - Former: machineTemplate, - Latter: machineTemplateWithDifferentLabels, - Expected: true, - }, - { - Name: "Same spec, except latter does not have annotations", - Former: machineTemplate, - Latter: machineTemplateWithEmptyAnnotations, - Expected: true, - }, - { - Name: "Same spec, except latter has different annotations", - Former: machineTemplate, - Latter: machineTemplateWithDifferentAnnotations, - Expected: true, - }, - { - Name: "Spec changes, latter has different in-place mutable spec fields", - Former: machineTemplate, - Latter: machineTemplateWithDifferentInPlaceMutableSpecFields, - Expected: true, - }, - { - Name: "Spec changes, latter has different ClusterName", - // Note: ClusterName is immutable, but EqualMachineTemplate should still work correctly independent of that. - Former: machineTemplate, - Latter: machineTemplateWithDifferentClusterName, - Expected: false, - Diff1: `&v1beta1.MachineTemplateSpec{ - ObjectMeta: {}, - Spec: v1beta1.MachineSpec{ -- ClusterName: "cluster1", -+ ClusterName: "cluster2", - Bootstrap: {ConfigRef: &{Kind: "BootstrapConfig", Namespace: "default", Name: "bootstrap1", APIVersion: "bootstrap.cluster.x-k8s.io", ...}}, - InfrastructureRef: {Kind: "InfrastructureMachine", Namespace: "default", Name: "infra1", APIVersion: "infrastructure.cluster.x-k8s.io", ...}, - ... // 7 identical fields - }, - }`, - Diff2: `&v1beta1.MachineTemplateSpec{ - ObjectMeta: {}, - Spec: v1beta1.MachineSpec{ -- ClusterName: "cluster2", -+ ClusterName: "cluster1", - Bootstrap: {ConfigRef: &{Kind: "BootstrapConfig", Namespace: "default", Name: "bootstrap1", APIVersion: "bootstrap.cluster.x-k8s.io", ...}}, - InfrastructureRef: {Kind: "InfrastructureMachine", Namespace: "default", Name: "infra1", APIVersion: "infrastructure.cluster.x-k8s.io", ...}, - ... // 7 identical fields - }, - }`, - }, - { - Name: "Spec changes, latter has different Version", - Former: machineTemplate, - Latter: machineTemplateWithDifferentVersion, - Expected: false, - Diff1: `&v1beta1.MachineTemplateSpec{ - ObjectMeta: {}, - Spec: v1beta1.MachineSpec{ - ClusterName: "cluster1", - Bootstrap: {ConfigRef: &{Kind: "BootstrapConfig", Namespace: "default", Name: "bootstrap1", APIVersion: "bootstrap.cluster.x-k8s.io", ...}}, - InfrastructureRef: {Kind: "InfrastructureMachine", Namespace: "default", Name: "infra1", APIVersion: "infrastructure.cluster.x-k8s.io", ...}, -- Version: &"v1.25.0", -+ Version: &"v1.26.0", - ProviderID: nil, - FailureDomain: &"failure-domain1", - ... // 4 identical fields - }, - }`, - Diff2: `&v1beta1.MachineTemplateSpec{ - ObjectMeta: {}, - Spec: v1beta1.MachineSpec{ - ClusterName: "cluster1", - Bootstrap: {ConfigRef: &{Kind: "BootstrapConfig", Namespace: "default", Name: "bootstrap1", APIVersion: "bootstrap.cluster.x-k8s.io", ...}}, - InfrastructureRef: {Kind: "InfrastructureMachine", Namespace: "default", Name: "infra1", APIVersion: "infrastructure.cluster.x-k8s.io", ...}, -- Version: &"v1.26.0", -+ Version: &"v1.25.0", - ProviderID: nil, - FailureDomain: &"failure-domain1", - ... // 4 identical fields - }, - }`, - }, - { - Name: "Spec changes, latter has different FailureDomain", - Former: machineTemplate, - Latter: machineTemplateWithDifferentFailureDomain, - Expected: false, - Diff1: `&v1beta1.MachineTemplateSpec{ - ObjectMeta: {}, - Spec: v1beta1.MachineSpec{ - ... // 3 identical fields - Version: &"v1.25.0", - ProviderID: nil, -- FailureDomain: &"failure-domain1", -+ FailureDomain: &"failure-domain2", - ReadinessGates: nil, - NodeDrainTimeout: nil, - ... // 2 identical fields - }, - }`, - Diff2: `&v1beta1.MachineTemplateSpec{ - ObjectMeta: {}, - Spec: v1beta1.MachineSpec{ - ... // 3 identical fields - Version: &"v1.25.0", - ProviderID: nil, -- FailureDomain: &"failure-domain2", -+ FailureDomain: &"failure-domain1", - ReadinessGates: nil, - NodeDrainTimeout: nil, - ... // 2 identical fields - }, - }`, - }, - { - Name: "Spec changes, latter has different InfrastructureRef", - Former: machineTemplate, - Latter: machineTemplateWithDifferentInfraRef, - Expected: false, - Diff1: `&v1beta1.MachineTemplateSpec{ - ObjectMeta: {}, - Spec: v1beta1.MachineSpec{ - ClusterName: "cluster1", - Bootstrap: {ConfigRef: &{Kind: "BootstrapConfig", Namespace: "default", Name: "bootstrap1", APIVersion: "bootstrap.cluster.x-k8s.io", ...}}, - InfrastructureRef: v1.ObjectReference{ - Kind: "InfrastructureMachine", - Namespace: "default", -- Name: "infra1", -+ Name: "infra2", - UID: "", - APIVersion: "infrastructure.cluster.x-k8s.io", - ... // 2 identical fields - }, - Version: &"v1.25.0", - ProviderID: nil, - ... // 5 identical fields - }, - }`, - Diff2: `&v1beta1.MachineTemplateSpec{ - ObjectMeta: {}, - Spec: v1beta1.MachineSpec{ - ClusterName: "cluster1", - Bootstrap: {ConfigRef: &{Kind: "BootstrapConfig", Namespace: "default", Name: "bootstrap1", APIVersion: "bootstrap.cluster.x-k8s.io", ...}}, - InfrastructureRef: v1.ObjectReference{ - Kind: "InfrastructureMachine", - Namespace: "default", -- Name: "infra2", -+ Name: "infra1", - UID: "", - APIVersion: "infrastructure.cluster.x-k8s.io", - ... // 2 identical fields - }, - Version: &"v1.25.0", - ProviderID: nil, - ... // 5 identical fields - }, - }`, - }, - { - Name: "Spec changes, latter has different Bootstrap", - Former: machineTemplate, - Latter: machineTemplateWithDifferentBootstrap, - Expected: false, - Diff1: `&v1beta1.MachineTemplateSpec{ - ObjectMeta: {}, - Spec: v1beta1.MachineSpec{ - ClusterName: "cluster1", - Bootstrap: v1beta1.Bootstrap{ -- ConfigRef: s"&ObjectReference{Kind:BootstrapConfig,Namespace:default,Name:bootstrap1,UID:,APIVersion:bootstrap.cluster.x-k8s.io,ResourceVersion:,FieldPath:,}", -+ ConfigRef: nil, -- DataSecretName: nil, -+ DataSecretName: &"data-secret", - }, - InfrastructureRef: {Kind: "InfrastructureMachine", Namespace: "default", Name: "infra1", APIVersion: "infrastructure.cluster.x-k8s.io", ...}, - Version: &"v1.25.0", - ... // 6 identical fields - }, - }`, - Diff2: `&v1beta1.MachineTemplateSpec{ - ObjectMeta: {}, - Spec: v1beta1.MachineSpec{ - ClusterName: "cluster1", - Bootstrap: v1beta1.Bootstrap{ -- ConfigRef: nil, -+ ConfigRef: s"&ObjectReference{Kind:BootstrapConfig,Namespace:default,Name:bootstrap1,UID:,APIVersion:bootstrap.cluster.x-k8s.io,ResourceVersion:,FieldPath:,}", -- DataSecretName: &"data-secret", -+ DataSecretName: nil, - }, - InfrastructureRef: {Kind: "InfrastructureMachine", Namespace: "default", Name: "infra1", APIVersion: "infrastructure.cluster.x-k8s.io", ...}, - Version: &"v1.25.0", - ... // 6 identical fields - }, - }`, - }, - { - Name: "Spec changes, latter has different Bootstrap.ConfigRef", - Former: machineTemplate, - Latter: machineTemplateWithDifferentBootstrapConfigRef, - Expected: false, - Diff1: `&v1beta1.MachineTemplateSpec{ - ObjectMeta: {}, - Spec: v1beta1.MachineSpec{ - ClusterName: "cluster1", - Bootstrap: v1beta1.Bootstrap{ - ConfigRef: &v1.ObjectReference{ - Kind: "BootstrapConfig", - Namespace: "default", -- Name: "bootstrap1", -+ Name: "bootstrap2", - UID: "", - APIVersion: "bootstrap.cluster.x-k8s.io", - ... // 2 identical fields - }, - DataSecretName: nil, - }, - InfrastructureRef: {Kind: "InfrastructureMachine", Namespace: "default", Name: "infra1", APIVersion: "infrastructure.cluster.x-k8s.io", ...}, - Version: &"v1.25.0", - ... // 6 identical fields - }, - }`, - Diff2: `&v1beta1.MachineTemplateSpec{ - ObjectMeta: {}, - Spec: v1beta1.MachineSpec{ - ClusterName: "cluster1", - Bootstrap: v1beta1.Bootstrap{ - ConfigRef: &v1.ObjectReference{ - Kind: "BootstrapConfig", - Namespace: "default", -- Name: "bootstrap2", -+ Name: "bootstrap1", - UID: "", - APIVersion: "bootstrap.cluster.x-k8s.io", - ... // 2 identical fields - }, - DataSecretName: nil, - }, - InfrastructureRef: {Kind: "InfrastructureMachine", Namespace: "default", Name: "infra1", APIVersion: "infrastructure.cluster.x-k8s.io", ...}, - Version: &"v1.25.0", - ... // 6 identical fields - }, - }`, - }, - { - Name: "Same spec, except latter has different InfrastructureRef APIVersion", - Former: machineTemplate, - Latter: machineTemplateWithDifferentInfraRefAPIVersion, - Expected: true, - }, - { - Name: "Same spec, except latter has different Bootstrap.ConfigRef APIVersion", - Former: machineTemplate, - Latter: machineTemplateWithDifferentBootstrapConfigRefAPIVersion, - Expected: true, + current: machineTemplate, + desired: machineTemplateEqual, + expectedUpToDate: true, + }, + { + Name: "Same spec, except desired does not have labels", + current: machineTemplate, + desired: machineTemplateWithEmptyLabels, + expectedUpToDate: true, + }, + { + Name: "Same spec, except desired has different labels", + current: machineTemplate, + desired: machineTemplateWithDifferentLabels, + expectedUpToDate: true, + }, + { + Name: "Same spec, except desired does not have annotations", + current: machineTemplate, + desired: machineTemplateWithEmptyAnnotations, + expectedUpToDate: true, + }, + { + Name: "Same spec, except desired has different annotations", + current: machineTemplate, + desired: machineTemplateWithDifferentAnnotations, + expectedUpToDate: true, + }, + { + Name: "Spec changes, desired has different in-place mutable spec fields", + current: machineTemplate, + desired: machineTemplateWithDifferentInPlaceMutableSpecFields, + expectedUpToDate: true, + }, + { + Name: "Spec changes, desired has different Version", + current: machineTemplate, + desired: machineTemplateWithDifferentVersion, + expectedUpToDate: false, + expectedLogMessages1: []string{"spec.version v1.25.0, v1.26.0 required"}, + expectedLogMessages2: []string{"spec.version v1.26.0, v1.25.0 required"}, + expectedConditionMessages1: []string{"Version v1.25.0, v1.26.0 required"}, + expectedConditionMessages2: []string{"Version v1.26.0, v1.25.0 required"}, + }, + { + Name: "Spec changes, desired has different FailureDomain", + current: machineTemplate, + desired: machineTemplateWithDifferentFailureDomain, + expectedUpToDate: false, + expectedLogMessages1: []string{"spec.failureDomain failure-domain1, failure-domain2 required"}, + expectedLogMessages2: []string{"spec.failureDomain failure-domain2, failure-domain1 required"}, + expectedConditionMessages1: []string{"Failure domain failure-domain1, failure-domain2 required"}, + expectedConditionMessages2: []string{"Failure domain failure-domain2, failure-domain1 required"}, + }, + { + Name: "Spec changes, desired has different InfrastructureRef", + current: machineTemplate, + desired: machineTemplateWithDifferentInfraRef, + expectedUpToDate: false, + expectedLogMessages1: []string{"spec.infrastructureRef InfrastructureMachineTemplate infra1, InfrastructureMachineTemplate infra2 required"}, + expectedLogMessages2: []string{"spec.infrastructureRef InfrastructureMachineTemplate infra2, InfrastructureMachineTemplate infra1 required"}, + expectedConditionMessages1: []string{"InfrastructureMachine is not up-to-date"}, + expectedConditionMessages2: []string{"InfrastructureMachine is not up-to-date"}, + }, + { + Name: "Spec changes, desired has different Bootstrap data secret", + current: machineTemplateWithBootstrapDataSecret, + desired: machineTemplateWithDifferentBootstrapDataSecret, + expectedUpToDate: false, + expectedLogMessages1: []string{"spec.bootstrap.dataSecretName data-secret1, data-secret2 required"}, + expectedLogMessages2: []string{"spec.bootstrap.dataSecretName data-secret2, data-secret1 required"}, + expectedConditionMessages1: []string{"spec.bootstrap.dataSecretName data-secret1, data-secret2 required"}, + expectedConditionMessages2: []string{"spec.bootstrap.dataSecretName data-secret2, data-secret1 required"}, + }, + { + Name: "Spec changes, desired has different Bootstrap.ConfigRef", + current: machineTemplate, + desired: machineTemplateWithDifferentBootstrapConfigRef, + expectedUpToDate: false, + expectedLogMessages1: []string{"spec.bootstrap.configRef BootstrapConfigTemplate bootstrap1, BootstrapConfigTemplate bootstrap2 required"}, + expectedLogMessages2: []string{"spec.bootstrap.configRef BootstrapConfigTemplate bootstrap2, BootstrapConfigTemplate bootstrap1 required"}, + expectedConditionMessages1: []string{"BootstrapConfig is not up-to-date"}, + expectedConditionMessages2: []string{"BootstrapConfig is not up-to-date"}, + }, + { + Name: "Spec changes, desired has data secret instead of Bootstrap.ConfigRef", + current: machineTemplate, + desired: machineTemplateWithBootstrapDataSecret, + expectedUpToDate: false, + expectedLogMessages1: []string{"spec.bootstrap.configRef BootstrapConfigTemplate bootstrap1, required"}, + expectedLogMessages2: []string{"spec.bootstrap.dataSecretName data-secret1, nil required"}, + expectedConditionMessages1: []string{"BootstrapConfig is not up-to-date"}, + expectedConditionMessages2: []string{"spec.bootstrap.dataSecretName data-secret1, nil required"}, + }, + { + Name: "Same spec, except desired has different InfrastructureRef APIVersion", + current: machineTemplate, + desired: machineTemplateWithDifferentInfraRefAPIVersion, + expectedUpToDate: true, + }, + { + Name: "Same spec, except desired has different Bootstrap.ConfigRef APIVersion", + current: machineTemplate, + desired: machineTemplateWithDifferentBootstrapConfigRefAPIVersion, + expectedUpToDate: true, }, } @@ -527,19 +373,19 @@ func TestEqualMachineTemplate(t *testing.T) { t.Run(test.Name, func(t *testing.T) { g := NewWithT(t) - runTest := func(t1, t2 *clusterv1.MachineTemplateSpec, expectedDiff string) { + runTest := func(t1, t2 *clusterv1.MachineTemplateSpec, expectedLogMessages, expectedConditionMessages []string) { // Run - equal, diff, err := EqualMachineTemplate(t1, t2) - g.Expect(err).ToNot(HaveOccurred()) - g.Expect(equal).To(Equal(test.Expected)) - g.Expect(diff).To(BeComparableTo(expectedDiff)) + upToDate, logMessages, conditionMessages := MachineTemplateUpToDate(t1, t2) + g.Expect(upToDate).To(Equal(test.expectedUpToDate)) + g.Expect(logMessages).To(Equal(expectedLogMessages)) + g.Expect(conditionMessages).To(Equal(expectedConditionMessages)) g.Expect(t1.Labels).NotTo(BeNil()) g.Expect(t2.Labels).NotTo(BeNil()) } - runTest(test.Former, test.Latter, test.Diff1) + runTest(test.current, test.desired, test.expectedLogMessages1, test.expectedConditionMessages1) // Test the same case in reverse order - runTest(test.Latter, test.Former, test.Diff2) + runTest(test.desired, test.current, test.expectedLogMessages2, test.expectedConditionMessages2) }) } } @@ -552,6 +398,7 @@ func TestFindNewMachineSet(t *testing.T) { twoAfterRolloutAfter := metav1.NewTime(oneAfterRolloutAfter.Add(time.Minute)) deployment := generateDeployment("nginx") + deployment.Spec.Template.Spec.InfrastructureRef.Kind = "InfrastructureMachineTemplate" deployment.Spec.Template.Spec.InfrastructureRef.Name = "new-infra-ref" deploymentWithRolloutAfter := deployment.DeepCopy() @@ -608,29 +455,11 @@ func TestFindNewMachineSet(t *testing.T) { expected: &matchingMSDiffersInPlaceMutableFields, }, { - Name: "Get nil if no MachineSet matches the desired intent of the MachineDeployment", - deployment: deployment, - msList: []*clusterv1.MachineSet{&oldMS}, - expected: nil, - createReason: fmt.Sprintf(`couldn't find MachineSet matching MachineDeployment spec template: MachineSet %s: diff: &v1beta1.MachineTemplateSpec{ - ObjectMeta: {}, - Spec: v1beta1.MachineSpec{ - ClusterName: "", - Bootstrap: {}, - InfrastructureRef: v1.ObjectReference{ - Kind: "", - Namespace: "", -- Name: "old-infra-ref", -+ Name: "new-infra-ref", - UID: "", - APIVersion: "", - ... // 2 identical fields - }, - Version: nil, - ProviderID: nil, - ... // 5 identical fields - }, - }`, oldMS.Name), + Name: "Get nil if no MachineSet matches the desired intent of the MachineDeployment", + deployment: deployment, + msList: []*clusterv1.MachineSet{&oldMS}, + expected: nil, + createReason: fmt.Sprintf(`couldn't find MachineSet matching MachineDeployment spec template: MachineSet %s: diff: spec.infrastructureRef InfrastructureMachineTemplate old-infra-ref, InfrastructureMachineTemplate new-infra-ref required`, oldMS.Name), }, { Name: "Get the MachineSet if reconciliationTime < rolloutAfter", diff --git a/internal/controllers/machineset/machineset_controller.go b/internal/controllers/machineset/machineset_controller.go index 133a99ada951..d2f6a5155404 100644 --- a/internal/controllers/machineset/machineset_controller.go +++ b/internal/controllers/machineset/machineset_controller.go @@ -51,6 +51,7 @@ import ( "sigs.k8s.io/cluster-api/controllers/noderefutil" "sigs.k8s.io/cluster-api/internal/contract" "sigs.k8s.io/cluster-api/internal/controllers/machine" + "sigs.k8s.io/cluster-api/internal/controllers/machinedeployment/mdutil" "sigs.k8s.io/cluster-api/internal/util/ssa" "sigs.k8s.io/cluster-api/util" "sigs.k8s.io/cluster-api/util/collections" @@ -183,8 +184,9 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (retres ct } s := &scope{ - cluster: cluster, - machineSet: machineSet, + cluster: cluster, + machineSet: machineSet, + reconciliationTime: time.Now(), } // Initialize the patch helper @@ -272,6 +274,7 @@ type scope struct { owningMachineDeployment *clusterv1.MachineDeployment remediationPreflightCheckErrMessage string scaleUpPreflightCheckErrMessage string + reconciliationTime time.Time } type machineSetReconcileFunc func(ctx context.Context, s *scope) (ctrl.Result, error) @@ -493,6 +496,9 @@ func (r *Reconciler) syncMachines(ctx context.Context, s *scope) (ctrl.Result, e log := ctrl.LoggerFrom(ctx) for i := range machines { m := machines[i] + + upToDateCondition := newMachineUpToDateCondition(s) + // If the machine is already being deleted, we only need to sync // the subset of fields that impact tearing down a machine if !m.DeletionTimestamp.IsZero() { @@ -507,12 +513,31 @@ func (r *Reconciler) syncMachines(ctx context.Context, s *scope) (ctrl.Result, e m.Spec.NodeDeletionTimeout = machineSet.Spec.Template.Spec.NodeDeletionTimeout m.Spec.NodeVolumeDetachTimeout = machineSet.Spec.Template.Spec.NodeVolumeDetachTimeout - if err := patchHelper.Patch(ctx, m); err != nil { + // Set machine's up to date condition + if upToDateCondition != nil { + v1beta2conditions.Set(m, *upToDateCondition) + } + + if err := patchHelper.Patch(ctx, m, patch.WithOwnedV1Beta2Conditions{Conditions: []string{clusterv1.MachineUpToDateV1Beta2Condition}}); err != nil { return ctrl.Result{}, err } continue } + // Patch the machine's up-to-date condition. + // Note: for the time being we continue to rely on the patch helper for setting conditions; In the future, if + // we will improve patch helper to support SSA, we can revisit this code and perform both this change and the others in place mutations in a single operation. + if upToDateCondition != nil { + patchHelper, err := patch.NewHelper(m, r.Client) + if err != nil { + return ctrl.Result{}, err + } + v1beta2conditions.Set(m, *upToDateCondition) + if err := patchHelper.Patch(ctx, m, patch.WithOwnedV1Beta2Conditions{Conditions: []string{clusterv1.MachineUpToDateV1Beta2Condition}}); err != nil { + return ctrl.Result{}, err + } + } + // Cleanup managed fields of all Machines. // We do this so that Machines that were created/patched before the controller adopted Server-Side-Apply (SSA) // (< v1.4.0) can also work with SSA. Otherwise, fields would be co-owned by our "old" "manager" and @@ -573,6 +598,57 @@ func (r *Reconciler) syncMachines(ctx context.Context, s *scope) (ctrl.Result, e return ctrl.Result{}, nil } +func newMachineUpToDateCondition(s *scope) *metav1.Condition { + // If the current MachineSet is a stand-alone MachineSet, the MachineSet controller does not set an up-to-date condition + // on Machines, allowing tools managing higher level abstractions to set this condition. + // This is also consistent with the fact that the MachineSet controller primarily takes care of the number of Machine + // replicas, it doesn't reconcile them (even if we have a few exceptions like in-place propagation of a few selected + // fields and remediation). + if s.owningMachineDeployment == nil { + return nil + } + + // Determine current and desired state. + // If the current MachineSet is owned by a MachineDeployment, we mirror what is implemented in the MachineDeployment controller + // to trigger rollouts (by creating new MachineSets). + // More specifically: + // - desired state for the Machine is the spec.Template of the MachineDeployment + // - current state for the Machine is the spec.Template of the MachineSet who owns the Machine + // Note: We are intentionally considering current spec from the MachineSet instead of spec from the Machine itself in + // order to surface info consistent with what the MachineDeployment controller uses to take decisions about rollouts. + // The downside is that the system will ignore out of band changes applied to controlled Machines, which is + // considered an acceptable trade-off given that out of band changes are the exception (users should not change + // objects owned by the system). + // However, if out of band changes happen, at least the system will ignore out of band changes consistently, both in the + // MachineDeployment controller and in the condition computed here. + current := &s.machineSet.Spec.Template + desired := &s.owningMachineDeployment.Spec.Template + + upToDate, _, conditionMessages := mdutil.MachineTemplateUpToDate(current, desired) + + if s.owningMachineDeployment.Spec.RolloutAfter != nil { + if s.owningMachineDeployment.Spec.RolloutAfter.Time.Before(s.reconciliationTime) && !s.machineSet.CreationTimestamp.After(s.owningMachineDeployment.Spec.RolloutAfter.Time) { + upToDate = false + conditionMessages = append(conditionMessages, "MachineDeployment spec.rolloutAfter expired") + } + } + + if !upToDate { + return &metav1.Condition{ + Type: clusterv1.MachineUpToDateV1Beta2Condition, + Status: metav1.ConditionFalse, + Reason: clusterv1.MachineNotUpToDateV1Beta2Reason, + Message: strings.Join(conditionMessages, "; "), + } + } + + return &metav1.Condition{ + Type: clusterv1.MachineUpToDateV1Beta2Condition, + Status: metav1.ConditionTrue, + Reason: clusterv1.MachineUpToDateV1Beta2Reason, + } +} + // syncReplicas scales Machine resources up or down. func (r *Reconciler) syncReplicas(ctx context.Context, s *scope) (ctrl.Result, error) { ms := s.machineSet diff --git a/internal/controllers/machineset/machineset_controller_test.go b/internal/controllers/machineset/machineset_controller_test.go index 1c9f529cb8f0..22ea76b7697d 100644 --- a/internal/controllers/machineset/machineset_controller_test.go +++ b/internal/controllers/machineset/machineset_controller_test.go @@ -2592,3 +2592,183 @@ func TestReconciler_reconcileDelete(t *testing.T) { }) } } + +func TestNewMachineUpToDateCondition(t *testing.T) { + reconciliationTime := time.Now() + tests := []struct { + name string + machineDeployment *clusterv1.MachineDeployment + machineSet *clusterv1.MachineSet + expectCondition *metav1.Condition + }{ + { + name: "no condition returned for stand-alone MachineSet", + machineDeployment: nil, + machineSet: &clusterv1.MachineSet{}, + expectCondition: nil, + }, + { + name: "up-to-date", + machineDeployment: &clusterv1.MachineDeployment{ + Spec: clusterv1.MachineDeploymentSpec{ + Template: clusterv1.MachineTemplateSpec{ + Spec: clusterv1.MachineSpec{ + Version: ptr.To("v1.31.0"), + }, + }, + }, + }, + machineSet: &clusterv1.MachineSet{ + Spec: clusterv1.MachineSetSpec{ + Template: clusterv1.MachineTemplateSpec{ + Spec: clusterv1.MachineSpec{ + Version: ptr.To("v1.31.0"), + }, + }, + }, + }, + expectCondition: &metav1.Condition{ + Type: clusterv1.MachineUpToDateV1Beta2Condition, + Status: metav1.ConditionTrue, + Reason: clusterv1.MachineUpToDateV1Beta2Reason, + }, + }, + { + name: "not up-to-date", + machineDeployment: &clusterv1.MachineDeployment{ + Spec: clusterv1.MachineDeploymentSpec{ + Template: clusterv1.MachineTemplateSpec{ + Spec: clusterv1.MachineSpec{ + Version: ptr.To("v1.31.0"), + }, + }, + }, + }, + machineSet: &clusterv1.MachineSet{ + Spec: clusterv1.MachineSetSpec{ + Template: clusterv1.MachineTemplateSpec{ + Spec: clusterv1.MachineSpec{ + Version: ptr.To("v1.30.0"), + }, + }, + }, + }, + expectCondition: &metav1.Condition{ + Type: clusterv1.MachineUpToDateV1Beta2Condition, + Status: metav1.ConditionFalse, + Reason: clusterv1.MachineNotUpToDateV1Beta2Reason, + Message: "Version v1.30.0, v1.31.0 required", + }, + }, + { + name: "up-to-date, spec.rolloutAfter not expired", + machineDeployment: &clusterv1.MachineDeployment{ + Spec: clusterv1.MachineDeploymentSpec{ + RolloutAfter: &metav1.Time{Time: reconciliationTime.Add(1 * time.Hour)}, // rollout after not yet expired + Template: clusterv1.MachineTemplateSpec{ + Spec: clusterv1.MachineSpec{ + Version: ptr.To("v1.31.0"), + }, + }, + }, + }, + machineSet: &clusterv1.MachineSet{ + ObjectMeta: metav1.ObjectMeta{ + CreationTimestamp: metav1.Time{Time: reconciliationTime.Add(-1 * time.Hour)}, // MS created before rollout after + }, + Spec: clusterv1.MachineSetSpec{ + Template: clusterv1.MachineTemplateSpec{ + Spec: clusterv1.MachineSpec{ + Version: ptr.To("v1.31.0"), + }, + }, + }, + }, + expectCondition: &metav1.Condition{ + Type: clusterv1.MachineUpToDateV1Beta2Condition, + Status: metav1.ConditionTrue, + Reason: clusterv1.MachineUpToDateV1Beta2Reason, + }, + }, + { + name: "not up-to-date, rollout After expired", + machineDeployment: &clusterv1.MachineDeployment{ + Spec: clusterv1.MachineDeploymentSpec{ + RolloutAfter: &metav1.Time{Time: reconciliationTime.Add(-1 * time.Hour)}, // rollout after expired + Template: clusterv1.MachineTemplateSpec{ + Spec: clusterv1.MachineSpec{ + Version: ptr.To("v1.31.0"), + }, + }, + }, + }, + machineSet: &clusterv1.MachineSet{ + ObjectMeta: metav1.ObjectMeta{ + CreationTimestamp: metav1.Time{Time: reconciliationTime.Add(-2 * time.Hour)}, // MS created before rollout after + }, + Spec: clusterv1.MachineSetSpec{ + Template: clusterv1.MachineTemplateSpec{ + Spec: clusterv1.MachineSpec{ + Version: ptr.To("v1.31.0"), + }, + }, + }, + }, + expectCondition: &metav1.Condition{ + Type: clusterv1.MachineUpToDateV1Beta2Condition, + Status: metav1.ConditionFalse, + Reason: clusterv1.MachineNotUpToDateV1Beta2Reason, + Message: "MachineDeployment spec.rolloutAfter expired", + }, + }, + { + name: "not up-to-date, rollout After expired and a new MS created", + machineDeployment: &clusterv1.MachineDeployment{ + Spec: clusterv1.MachineDeploymentSpec{ + RolloutAfter: &metav1.Time{Time: reconciliationTime.Add(-2 * time.Hour)}, // rollout after expired + Template: clusterv1.MachineTemplateSpec{ + Spec: clusterv1.MachineSpec{ + Version: ptr.To("v1.31.0"), + }, + }, + }, + }, + machineSet: &clusterv1.MachineSet{ + ObjectMeta: metav1.ObjectMeta{ + CreationTimestamp: metav1.Time{Time: reconciliationTime.Add(-1 * time.Hour)}, // MS created after rollout after + }, + Spec: clusterv1.MachineSetSpec{ + Template: clusterv1.MachineTemplateSpec{ + Spec: clusterv1.MachineSpec{ + Version: ptr.To("v1.31.0"), + }, + }, + }, + }, + expectCondition: &metav1.Condition{ + Type: clusterv1.MachineUpToDateV1Beta2Condition, + Status: metav1.ConditionTrue, + Reason: clusterv1.MachineUpToDateV1Beta2Reason, + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + + s := &scope{ + owningMachineDeployment: tt.machineDeployment, + machineSet: tt.machineSet, + reconciliationTime: reconciliationTime, + } + + condition := newMachineUpToDateCondition(s) + if tt.expectCondition != nil { + g.Expect(condition).ToNot(BeNil()) + g.Expect(*condition).To(v1beta2conditions.MatchCondition(*tt.expectCondition, v1beta2conditions.IgnoreLastTransitionTime(true))) + } else { + g.Expect(condition).To(BeNil()) + } + }) + } +}