Merge pull request #10688 from sbueringer/pr-md-rollout-log

🌱 Log reason for MachineDeployment rollouts / MachineSet creations

Author: k8s-ci-robot

internal/controllers/machinedeployment/machinedeployment_sync.go

@@ -79,7 +79,10 @@ func (r *Reconciler) sync(ctx context.Context, md *clusterv1.MachineDeployment,
 // This may lead to stale reads of machine sets, thus incorrect deployment status.
 func (r *Reconciler) getAllMachineSetsAndSyncRevision(ctx context.Context, md *clusterv1.MachineDeployment, msList []*clusterv1.MachineSet, createIfNotExisted bool) (*clusterv1.MachineSet, []*clusterv1.MachineSet, error) {
 	reconciliationTime := metav1.Now()
-	allOldMSs := mdutil.FindOldMachineSets(md, msList, &reconciliationTime)
+	allOldMSs, err := mdutil.FindOldMachineSets(md, msList, &reconciliationTime)
+	if err != nil {
+		return nil, nil, err
+	}
 
 	// Get new machine set with the updated revision number
 	newMS, err := r.getNewMachineSet(ctx, md, msList, allOldMSs, createIfNotExisted, &reconciliationTime)
@@ -101,7 +104,10 @@ func (r *Reconciler) getNewMachineSet(ctx context.Context, md *clusterv1.Machine
 	// If we don't find a matching MachineSet, we need a rollout and thus create a new MachineSet.
 	// Note: The in-place mutable fields can be just updated inline, because they do not affect the actual machines
 	// themselves (i.e. the infrastructure and the software running on the Machines not the Machine object).
-	matchingMS := mdutil.FindNewMachineSet(md, msList, reconciliationTime)
+	matchingMS, createReason, err := mdutil.FindNewMachineSet(md, msList, reconciliationTime)
+	if err != nil {
+		return nil, err
+	}
 
 	// If there is a MachineSet that matches the intent of the MachineDeployment, update the MachineSet
 	// to propagate all in-place mutable fields from MachineDeployment to the MachineSet.
@@ -121,7 +127,7 @@ func (r *Reconciler) getNewMachineSet(ctx context.Context, md *clusterv1.Machine
 	}
 
 	// Create a new MachineSet and wait until the new MachineSet exists in the cache.
-	newMS, err := r.createMachineSetAndWait(ctx, md, oldMSs)
+	newMS, err := r.createMachineSetAndWait(ctx, md, oldMSs, createReason)
 	if err != nil {
 		return nil, err
 	}
@@ -154,7 +160,7 @@ func (r *Reconciler) updateMachineSet(ctx context.Context, deployment *clusterv1
 
 // createMachineSetAndWait creates a new MachineSet with the desired intent of the MachineDeployment.
 // It waits for the cache to be updated with the newly created MachineSet.
-func (r *Reconciler) createMachineSetAndWait(ctx context.Context, deployment *clusterv1.MachineDeployment, oldMSs []*clusterv1.MachineSet) (*clusterv1.MachineSet, error) {
+func (r *Reconciler) createMachineSetAndWait(ctx context.Context, deployment *clusterv1.MachineDeployment, oldMSs []*clusterv1.MachineSet, createReason string) (*clusterv1.MachineSet, error) {
 	log := ctrl.LoggerFrom(ctx)
 
 	// Compute the desired MachineSet.
@@ -163,12 +169,17 @@ func (r *Reconciler) createMachineSetAndWait(ctx context.Context, deployment *cl
 		return nil, errors.Wrap(err, "failed to create new MachineSet")
 	}
 
+	log = log.WithValues("MachineSet", klog.KObj(newMS))
+	ctx = ctrl.LoggerInto(ctx, log)
+
+	log.Info(fmt.Sprintf("Creating new MachineSet, %s", createReason))
+
 	// Create the MachineSet.
 	if err := ssa.Patch(ctx, r.Client, machineDeploymentManagerName, newMS); err != nil {
 		r.recorder.Eventf(deployment, corev1.EventTypeWarning, "FailedCreate", "Failed to create MachineSet %s: %v", klog.KObj(newMS), err)
 		return nil, errors.Wrapf(err, "failed to create new MachineSet %s", klog.KObj(newMS))
 	}
-	log.V(4).Info("Created new MachineSet", "MachineSet", klog.KObj(newMS))
+	log.V(4).Info("Created new MachineSet")
 	r.recorder.Eventf(deployment, corev1.EventTypeNormal, "SuccessfulCreate", "Created MachineSet %s", klog.KObj(newMS))
 
 	// Keep trying to get the MachineSet. This will force the cache to update and prevent any future reconciliation of

internal/controllers/machinedeployment/mdutil/util.go

@@ -23,11 +23,11 @@ import (
 	"sort"
 	"strconv"
 	"strings"
+	"time"
 
 	"github.com/go-logr/logr"
 	"github.com/pkg/errors"
 	corev1 "k8s.io/api/core/v1"
-	apiequality "k8s.io/apimachinery/pkg/api/equality"
 	"k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
@@ -37,6 +37,7 @@ 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,11 +372,11 @@ func getMachineSetFraction(ms clusterv1.MachineSet, md clusterv1.MachineDeployme
 
 // 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) bool {
+func EqualMachineTemplate(template1, template2 *clusterv1.MachineTemplateSpec) (equal bool, diff string, err error) {
 	t1Copy := MachineTemplateDeepCopyRolloutFields(template1)
 	t2Copy := MachineTemplateDeepCopyRolloutFields(template2)
 
-	return apiequality.Semantic.DeepEqual(t1Copy, t2Copy)
+	return compare.Diff(t1Copy, t2Copy)
 }
 
 // MachineTemplateDeepCopyRolloutFields copies a MachineTemplateSpec
@@ -415,45 +416,75 @@ func MachineTemplateDeepCopyRolloutFields(template *clusterv1.MachineTemplateSpe
 // 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
 // MS with the most replicas so that there is minimum machine churn.
-func FindNewMachineSet(deployment *clusterv1.MachineDeployment, msList []*clusterv1.MachineSet, reconciliationTime *metav1.Time) *clusterv1.MachineSet {
+func FindNewMachineSet(deployment *clusterv1.MachineDeployment, msList []*clusterv1.MachineSet, reconciliationTime *metav1.Time) (*clusterv1.MachineSet, string, error) {
+	if len(msList) == 0 {
+		return nil, "no MachineSets exist for the MachineDeployment", nil
+	}
+
+	// In rare cases, such as after cluster upgrades, Deployment may end up with
+	// having more than one new MachineSets that have the same template,
+	// see https://github.com/kubernetes/kubernetes/issues/40415
+	// We deterministically choose the oldest new MachineSet with matching template hash.
 	sort.Sort(MachineSetsByDecreasingReplicas(msList))
-	for i := range msList {
-		if EqualMachineTemplate(&msList[i].Spec.Template, &deployment.Spec.Template) &&
-			!shouldRolloutAfter(msList[i], reconciliationTime, deployment.Spec.RolloutAfter) {
-			// In rare cases, such as after cluster upgrades, Deployment may end up with
-			// having more than one new MachineSets that have the same template,
-			// see https://github.com/kubernetes/kubernetes/issues/40415
-			// We deterministically choose the oldest new MachineSet with matching template hash.
-			return msList[i]
+
+	var matchingMachineSets []*clusterv1.MachineSet
+	var diffs []string
+	for _, ms := range msList {
+		ms := ms
+
+		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 {
+			matchingMachineSets = append(matchingMachineSets, ms)
+		} else {
+			diffs = append(diffs, fmt.Sprintf("MachineSet %s: diff: %s", ms.Name, diff))
 		}
 	}
-	// new MachineSet does not exist.
-	return nil
-}
 
-func shouldRolloutAfter(ms *clusterv1.MachineSet, reconciliationTime *metav1.Time, rolloutAfter *metav1.Time) bool {
-	if ms == nil {
-		return false
+	if len(matchingMachineSets) == 0 {
+		return nil, fmt.Sprintf("couldn't find MachineSet matching MachineDeployment spec template: %s", strings.Join(diffs, ",")), nil
 	}
-	if reconciliationTime == nil || rolloutAfter == nil {
-		return false
+
+	// If RolloutAfter is not set, pick the first matching MachineSet.
+	if deployment.Spec.RolloutAfter == nil {
+		return matchingMachineSets[0], "", nil
+	}
+
+	// If reconciliation time is before RolloutAfter, pick the first matching MachineSet.
+	if reconciliationTime.Before(deployment.Spec.RolloutAfter) {
+		return matchingMachineSets[0], "", nil
+	}
+
+	// Pick the first matching MachineSet that has been created after RolloutAfter.
+	for _, ms := range matchingMachineSets {
+		ms := ms
+		if ms.CreationTimestamp.After(deployment.Spec.RolloutAfter.Time) {
+			return ms, "", nil
+		}
 	}
-	return ms.CreationTimestamp.Before(rolloutAfter) && rolloutAfter.Before(reconciliationTime)
+
+	// If no matching MachineSet was created after RolloutAfter, trigger creation of a new MachineSet.
+	return nil, fmt.Sprintf("RolloutAfter on MachineDeployment set to %s, no MachineSet has been created afterwards", deployment.Spec.RolloutAfter.Format(time.RFC3339)), nil
 }
 
 // FindOldMachineSets returns the old machine sets targeted by the given Deployment, within the given slice of MSes.
 // Returns a list of machine sets which contains all old machine sets.
-func FindOldMachineSets(deployment *clusterv1.MachineDeployment, msList []*clusterv1.MachineSet, reconciliationTime *metav1.Time) []*clusterv1.MachineSet {
+func FindOldMachineSets(deployment *clusterv1.MachineDeployment, msList []*clusterv1.MachineSet, reconciliationTime *metav1.Time) ([]*clusterv1.MachineSet, error) {
 	allMSs := make([]*clusterv1.MachineSet, 0, len(msList))
-	newMS := FindNewMachineSet(deployment, msList, reconciliationTime)
+	newMS, _, err := FindNewMachineSet(deployment, msList, reconciliationTime)
+	if err != nil {
+		return nil, err
+	}
 	for _, ms := range msList {
 		// Filter out new machine set
 		if newMS != nil && ms.UID == newMS.UID {
 			continue
 		}
 		allMSs = append(allMSs, ms)
 	}
-	return allMSs
+	return allMSs, nil
 }
 
 // GetReplicaCountForMachineSets returns the sum of Replicas of the given machine sets.