diff --git a/internal/controllers/machinedeployment/mdutil/util.go b/internal/controllers/machinedeployment/mdutil/util.go index 9155ba05377e..c682ad96169e 100644 --- a/internal/controllers/machinedeployment/mdutil/util.go +++ b/internal/controllers/machinedeployment/mdutil/util.go @@ -39,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" ) @@ -374,38 +373,46 @@ func getMachineSetFraction(ms clusterv1.MachineSet, md clusterv1.MachineDeployme // 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, diff string, conditionMessages []string, err error) { +func MachineTemplateUpToDate(current, desired *clusterv1.MachineTemplateSpec) (upToDate bool, logMessages, conditionMessages []string) { currentCopy := MachineTemplateDeepCopyRolloutFields(current) desiredCopy := MachineTemplateDeepCopyRolloutFields(desired) 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 current.Spec.Bootstrap.ConfigRef != nil { - if !reflect.DeepEqual(currentCopy.Spec.Bootstrap.ConfigRef, desiredCopy.Spec.Bootstrap.ConfigRef) { + 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)) conditionMessages = append(conditionMessages, fmt.Sprintf("%s is not up-to-date", current.Spec.Bootstrap.ConfigRef.Kind)) } } else { - if !reflect.DeepEqual(currentCopy.Spec.Bootstrap.DataSecretName, desiredCopy.Spec.Bootstrap.DataSecretName) { - conditionMessages = append(conditionMessages, "spec.bootstrap.dataSecretName is not up to date") + 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)) conditionMessages = append(conditionMessages, fmt.Sprintf("%s is not up-to-date", currentCopy.Spec.InfrastructureRef.Kind)) } 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(conditionMessages) > 0 { - _, diff, err = compare.Diff(currentCopy, desiredCopy) - return false, diff, conditionMessages, err + return false, logMessages, conditionMessages } - return true, "", nil, nil + return true, nil, nil } // MachineTemplateDeepCopyRolloutFields copies a MachineTemplateSpec @@ -414,6 +421,9 @@ func MachineTemplateUpToDate(current, desired *clusterv1.MachineTemplateSpec) (u 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 @@ -460,19 +470,16 @@ func FindNewMachineSet(deployment *clusterv1.MachineDeployment, msList []*cluste var matchingMachineSets []*clusterv1.MachineSet var diffs []string for _, ms := range msList { - upToDate, diff, _, err := MachineTemplateUpToDate(&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) - } + 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 0d7d9ae30cae..6e5244a237b5 100644 --- a/internal/controllers/machinedeployment/mdutil/util_test.go +++ b/internal/controllers/machinedeployment/mdutil/util_test.go @@ -236,9 +236,12 @@ func TestMachineTemplateUpToDate(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" @@ -250,10 +253,10 @@ func TestMachineTemplateUpToDate(t *testing.T) { Name string current, desired *clusterv1.MachineTemplateSpec expectedUpToDate bool + expectedLogMessages1 []string + expectedLogMessages2 []string expectedConditionMessages1 []string expectedConditionMessages2 []string - diff1 string - diff2 string }{ { Name: "Same spec", @@ -293,207 +296,65 @@ func TestMachineTemplateUpToDate(t *testing.T) { expectedUpToDate: true, }, { - Name: "Spec changes, desired has different Version", - current: machineTemplate, - desired: machineTemplateWithDifferentVersion, - expectedUpToDate: 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, 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, - 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, 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, - 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, desired has different InfrastructureRef", + current: machineTemplate, + desired: machineTemplateWithDifferentInfraRef, + expectedUpToDate: false, + expectedLogMessages1: []string{"spec.infrastructureRef InfrastructureMachine/infra1, InfrastructureMachine/infra2 required"}, + expectedLogMessages2: []string{"spec.infrastructureRef InfrastructureMachine/infra2, InfrastructureMachine/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", - current: machineTemplate, - desired: machineTemplateWithDifferentBootstrap, - expectedUpToDate: 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 - }, - }`, - expectedConditionMessages1: []string{"BootstrapConfig is not up-to-date"}, - expectedConditionMessages2: []string{"spec.bootstrap.dataSecretName 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, - 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: "Spec changes, desired has different Bootstrap.ConfigRef", + current: machineTemplate, + desired: machineTemplateWithDifferentBootstrapConfigRef, + expectedUpToDate: false, + expectedLogMessages1: []string{"spec.bootstrap.configRef BootstrapConfig/bootstrap1, BootstrapConfig/bootstrap2 required"}, + expectedLogMessages2: []string{"spec.bootstrap.configRef BootstrapConfig/bootstrap2, BootstrapConfig/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 BootstrapConfig/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, @@ -512,20 +373,19 @@ func TestMachineTemplateUpToDate(t *testing.T) { t.Run(test.Name, func(t *testing.T) { g := NewWithT(t) - runTest := func(t1, t2 *clusterv1.MachineTemplateSpec, expectedDiff string, expectedConditionMessages []string) { + runTest := func(t1, t2 *clusterv1.MachineTemplateSpec, expectedLogMessages, expectedConditionMessages []string) { // Run - upToDate, diff, conditionMessages, err := MachineTemplateUpToDate(t1, t2) - g.Expect(err).ToNot(HaveOccurred()) + upToDate, logMessages, conditionMessages := MachineTemplateUpToDate(t1, t2) g.Expect(upToDate).To(Equal(test.expectedUpToDate)) - g.Expect(diff).To(BeComparableTo(expectedDiff)) + 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.current, test.desired, test.diff1, test.expectedConditionMessages1) + runTest(test.current, test.desired, test.expectedLogMessages1, test.expectedConditionMessages1) // Test the same case in reverse order - runTest(test.desired, test.current, test.diff2, test.expectedConditionMessages2) + runTest(test.desired, test.current, test.expectedLogMessages2, test.expectedConditionMessages2) }) } } @@ -594,29 +454,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 /old-infra-ref, /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 933ede47cb4e..1f1075ca976a 100644 --- a/internal/controllers/machineset/machineset_controller.go +++ b/internal/controllers/machineset/machineset_controller.go @@ -184,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 @@ -271,6 +272,7 @@ type scope struct { infrastructureObjectNotFound bool getAndAdoptMachinesForMachineSetSucceeded bool owningMachineDeployment *clusterv1.MachineDeployment + reconciliationTime time.Time } type machineSetReconcileFunc func(ctx context.Context, s *scope) (ctrl.Result, error) @@ -493,10 +495,7 @@ func (r *Reconciler) syncMachines(ctx context.Context, s *scope) (ctrl.Result, e for i := range machines { m := machines[i] - upToDateCondition, err := newMachineUpToDateCondition(s) - if err != nil { - return ctrl.Result{}, err - } + 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 @@ -547,7 +546,7 @@ func (r *Reconciler) syncMachines(ctx context.Context, s *scope) (ctrl.Result, e // Update Machine to propagate in-place mutable fields from the MachineSet. updatedMachine := r.computeDesiredMachine(machineSet, m) - err = ssa.Patch(ctx, r.Client, machineSetManagerName, updatedMachine, ssa.WithCachingProxy{Cache: r.ssaCache, Original: m}) + err := ssa.Patch(ctx, r.Client, machineSetManagerName, updatedMachine, ssa.WithCachingProxy{Cache: r.ssaCache, Original: m}) if err != nil { log.Error(err, "Failed to update Machine", "Machine", klog.KObj(updatedMachine)) return ctrl.Result{}, errors.Wrapf(err, "failed to update Machine %q", klog.KObj(updatedMachine)) @@ -597,39 +596,36 @@ func (r *Reconciler) syncMachines(ctx context.Context, s *scope) (ctrl.Result, e return ctrl.Result{}, nil } -func newMachineUpToDateCondition(s *scope) (*metav1.Condition, error) { - // If the current MachineSet is a stand-alone MachineSet, the MachineSet controller do not set an up-to-data condition - // on machines, allowing tools managing higher level abstractions to set this condition. +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, nil + return nil } // Determine current and desired state. - // If the current MachineSet is owned by a MachineDeployment, we mirror the what is implemented in the MachineDeployment controller - // to create new trigger rollouts (by creating new MachineSets). + // 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 Machine, which is - // considered an acceptable trade-off given that out of band changes are the exception (user should not change - // object owned by the system). - // However, In out of band changes happens, at least the system will ignore out of band changes consistently, both in the + // 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, err := mdutil.MachineTemplateUpToDate(current, desired) - if err != nil { - return nil, err - } + upToDate, _, conditionMessages := mdutil.MachineTemplateUpToDate(current, desired) - if s.owningMachineDeployment != nil && s.owningMachineDeployment.Spec.RolloutAfter != nil { - if !s.machineSet.CreationTimestamp.After(s.owningMachineDeployment.Spec.RolloutAfter.Time) { + 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") } @@ -641,14 +637,14 @@ func newMachineUpToDateCondition(s *scope) (*metav1.Condition, error) { Status: metav1.ConditionFalse, Reason: clusterv1.MachineNotUpToDateV1Beta2Reason, Message: strings.Join(conditionMessages, "; "), - }, nil + } } return &metav1.Condition{ Type: clusterv1.MachineUpToDateV1Beta2Condition, Status: metav1.ConditionTrue, Reason: clusterv1.MachineUpToDateV1Beta2Reason, - }, nil + } } // syncReplicas scales Machine resources up or down. diff --git a/internal/controllers/machineset/machineset_controller_test.go b/internal/controllers/machineset/machineset_controller_test.go index 79cc6801b422..fe834b5edeb7 100644 --- a/internal/controllers/machineset/machineset_controller_test.go +++ b/internal/controllers/machineset/machineset_controller_test.go @@ -2651,10 +2651,10 @@ func TestNewMachineUpToDateCondition(t *testing.T) { }, }, { - name: "not up-to-date, rollout After", + name: "up-to-date, spec.rolloutAfter not expired", machineDeployment: &clusterv1.MachineDeployment{ Spec: clusterv1.MachineDeploymentSpec{ - RolloutAfter: &metav1.Time{Time: reconciliationTime}, // rollout after now + 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"), @@ -2664,7 +2664,37 @@ func TestNewMachineUpToDateCondition(t *testing.T) { }, machineSet: &clusterv1.MachineSet{ ObjectMeta: metav1.ObjectMeta{ - CreationTimestamp: metav1.Time{Time: reconciliationTime.Add(-1 * 24 * time.Hour)}, // MS created before rollout after + 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{ @@ -2681,6 +2711,36 @@ func TestNewMachineUpToDateCondition(t *testing.T) { 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) { @@ -2689,10 +2749,10 @@ func TestNewMachineUpToDateCondition(t *testing.T) { s := &scope{ owningMachineDeployment: tt.machineDeployment, machineSet: tt.machineSet, + reconciliationTime: reconciliationTime, } - condition, err := newMachineUpToDateCondition(s) - g.Expect(err).ToNot(HaveOccurred()) + condition := newMachineUpToDateCondition(s) if tt.expectCondition != nil { g.Expect(condition).ToNot(BeNil()) g.Expect(*condition).To(v1beta2conditions.MatchCondition(*tt.expectCondition, v1beta2conditions.IgnoreLastTransitionTime(true)))