Merge pull request #5297 from enxebre/md-internal-main

 ⚠️ Move mdutil into an internal package

api/v1alpha4/machinedeployment_types.go

@@ -53,6 +53,10 @@ const (
 	// is machinedeployment.spec.replicas + maxSurge. Used by the underlying machine sets to estimate their
 	// proportions in case the deployment has surge replicas.
 	MaxReplicasAnnotation = "machinedeployment.clusters.x-k8s.io/max-replicas"
+
+	// MachineDeploymentUniqueLabel is the label applied to Machines
+	// in a MachineDeployment containing the hash of the template.
+	MachineDeploymentUniqueLabel = "machine-template-hash"
 )
 
 // ANCHOR: MachineDeploymentSpec

api/v1beta1/machinedeployment_types.go

@@ -53,6 +53,10 @@ const (
 	// is machinedeployment.spec.replicas + maxSurge. Used by the underlying machine sets to estimate their
 	// proportions in case the deployment has surge replicas.
 	MaxReplicasAnnotation = "machinedeployment.clusters.x-k8s.io/max-replicas"
+
+	// MachineDeploymentUniqueLabel is the label applied to Machines
+	// in a MachineDeployment containing the hash of the template.
+	MachineDeploymentUniqueLabel = "machine-template-hash"
 )
 
 // ANCHOR: MachineDeploymentSpec

cmd/clusterctl/client/alpha/machinedeployment.go

@@ -17,13 +17,16 @@ limitations under the License.
 package alpha
 
 import (
+	"strconv"
+
 	"github.com/pkg/errors"
+	"k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/labels"
+	"k8s.io/apimachinery/pkg/runtime"
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
 	"sigs.k8s.io/cluster-api/cmd/clusterctl/client/cluster"
 	logf "sigs.k8s.io/cluster-api/cmd/clusterctl/log"
-	"sigs.k8s.io/cluster-api/controllers/mdutil"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 )
 
@@ -75,7 +78,7 @@ func findMachineDeploymentRevision(toRevision int64, allMSs []*clusterv1.Machine
 		previousRevision   = int64(-1)
 	)
 	for _, ms := range allMSs {
-		if v, err := mdutil.Revision(ms); err == nil {
+		if v, err := revision(ms); err == nil {
 			if toRevision == 0 {
 				if latestRevision < v {
 					// newest one we've seen so far
@@ -147,3 +150,15 @@ func getMachineSetsForDeployment(proxy cluster.Proxy, d *clusterv1.MachineDeploy
 
 	return filtered, nil
 }
+
+func revision(obj runtime.Object) (int64, error) {
+	acc, err := meta.Accessor(obj)
+	if err != nil {
+		return 0, err
+	}
+	v, ok := acc.GetAnnotations()[clusterv1.RevisionAnnotation]
+	if !ok {
+		return 0, nil
+	}
+	return strconv.ParseInt(v, 10, 64)
+}

cmd/clusterctl/client/alpha/rollout_rollbacker.go

@@ -22,7 +22,6 @@ import (
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
 	"sigs.k8s.io/cluster-api/cmd/clusterctl/client/cluster"
 	logf "sigs.k8s.io/cluster-api/cmd/clusterctl/log"
-	"sigs.k8s.io/cluster-api/controllers/mdutil"
 	"sigs.k8s.io/cluster-api/util/patch"
 )
 
@@ -73,7 +72,7 @@ func rollbackMachineDeployment(proxy cluster.Proxy, d *clusterv1.MachineDeployme
 	}
 	// Copy template into the machinedeployment (excluding the hash)
 	revMSTemplate := *msForRevision.Spec.Template.DeepCopy()
-	delete(revMSTemplate.Labels, mdutil.DefaultMachineDeploymentUniqueLabelKey)
+	delete(revMSTemplate.Labels, clusterv1.MachineDeploymentUniqueLabel)
 
 	d.Spec.Template = revMSTemplate
 	return patchHelper.Patch(ctx, d)

controllers/mdutil/doc.go → controllers/internal/mdutil/doc.go

@@ -14,5 +14,6 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */
 
-// Package mdutil implements MachineDeployment utilities.
+// Package mdutil implements MachineDeployment utilities
+// meant to be consumed internally by the controller.
 package mdutil

controllers/mdutil/util.go → controllers/internal/mdutil/util.go

@@ -38,33 +38,6 @@ import (
 	"sigs.k8s.io/cluster-api/util/conversion"
 )
 
-const (
-	// DefaultMachineDeploymentUniqueLabelKey is the label applied to Machines
-	// in a MachineDeployment containing the hash of the template.
-	DefaultMachineDeploymentUniqueLabelKey = "machine-template-hash"
-
-	// FailedMSCreateReason is added in a machine deployment when it cannot create a new machine set.
-	// Deprecated: This field will be removed in a next release.
-	FailedMSCreateReason = "MachineSetCreateError"
-
-	// FoundNewMSReason is added in a machine deployment when it adopts an existing machine set.
-	// Deprecated: This field will be removed in a next release.
-	FoundNewMSReason = "FoundNewMachineSet"
-
-	// PausedDeployReason is added in a deployment when it is paused. Lack of progress shouldn't be
-	// estimated once a deployment is paused.
-	// Deprecated: This field will be removed in a next release.
-	PausedDeployReason = "DeploymentPaused"
-
-	// MinimumReplicasAvailable is added in a deployment when it has its minimum replicas required available.
-	// Deprecated: This field will be removed in a next release.
-	MinimumReplicasAvailable = "MinimumReplicasAvailable"
-	// MinimumReplicasUnavailable is added in a deployment when it doesn't have the minimum required replicas
-	// available.
-	// Deprecated: This field will be removed in a next release.
-	MinimumReplicasUnavailable = "MinimumReplicasUnavailable"
-)
-
 // MachineSetsByCreationTimestamp sorts a list of MachineSet by creation timestamp, using their names as a tie breaker.
 type MachineSetsByCreationTimestamp []*clusterv1.MachineSet
 
@@ -299,9 +272,8 @@ func SetReplicasAnnotations(ms *clusterv1.MachineSet, desiredReplicas, maxReplic
 		ms.Annotations[clusterv1.DesiredReplicasAnnotation] = desiredString
 		updated = true
 	}
-	maxString := fmt.Sprintf("%d", maxReplicas)
-	if hasString := ms.Annotations[clusterv1.MaxReplicasAnnotation]; hasString != maxString {
-		ms.Annotations[clusterv1.MaxReplicasAnnotation] = maxString
+	if hasString := ms.Annotations[clusterv1.MaxReplicasAnnotation]; hasString != fmt.Sprintf("%d", maxReplicas) {
+		ms.Annotations[clusterv1.MaxReplicasAnnotation] = fmt.Sprintf("%d", maxReplicas)
 		updated = true
 	}
 	return updated
@@ -316,8 +288,7 @@ func ReplicasAnnotationsNeedUpdate(ms *clusterv1.MachineSet, desiredReplicas, ma
 	if hasString := ms.Annotations[clusterv1.DesiredReplicasAnnotation]; hasString != desiredString {
 		return true
 	}
-	maxString := fmt.Sprintf("%d", maxReplicas)
-	if hasString := ms.Annotations[clusterv1.MaxReplicasAnnotation]; hasString != maxString {
+	if hasString := ms.Annotations[clusterv1.MaxReplicasAnnotation]; hasString != fmt.Sprintf("%d", maxReplicas) {
 		return true
 	}
 	return false
@@ -403,8 +374,8 @@ func EqualMachineTemplate(template1, template2 *clusterv1.MachineTemplateSpec) b
 	// 1. The hash result would be different upon machineTemplateSpec API changes
 	//    (e.g. the addition of a new field will cause the hash code to change)
 	// 2. The deployment template won't have hash labels
-	delete(t1Copy.Labels, DefaultMachineDeploymentUniqueLabelKey)
-	delete(t2Copy.Labels, DefaultMachineDeploymentUniqueLabelKey)
+	delete(t1Copy.Labels, clusterv1.MachineDeploymentUniqueLabel)
+	delete(t2Copy.Labels, clusterv1.MachineDeploymentUniqueLabel)
 
 	// Remove the version part from the references APIVersion field,
 	// for more details see issue #2183 and #2140.
@@ -697,23 +668,6 @@ func CloneSelectorAndAddLabel(selector *metav1.LabelSelector, labelKey, labelVal
 	return newSelector
 }
 
-// DeepHashObject writes specified object to hash using the spew library
-// which follows pointers and prints actual values of the nested objects
-// ensuring the hash does not change when a pointer changes.
-// Deprecated: Please use controllers/mdutil SpewHashObject(hasher, objectToWrite).
-func DeepHashObject(hasher hash.Hash, objectToWrite interface{}) {
-	hasher.Reset()
-	printer := spew.ConfigState{
-		Indent:         " ",
-		SortKeys:       true,
-		DisableMethods: true,
-		SpewKeys:       true,
-	}
-	// We ignore the returned error because there is no way to return the error without
-	// breaking API compatibility. Please use SpewHashObject instead.
-	_, _ = printer.Fprintf(hasher, "%#v", objectToWrite)
-}
-
 // SpewHashObject writes specified object to hash using the spew library
 // which follows pointers and prints actual values of the nested objects
 // ensuring the hash does not change when a pointer changes.
@@ -732,18 +686,10 @@ func SpewHashObject(hasher hash.Hash, objectToWrite interface{}) error {
 	return nil
 }
 
-// ComputeHash computes the hash of a MachineTemplateSpec using the spew library.
-// Deprecated: Please use controllers/mdutil ComputeSpewHash(template).
-func ComputeHash(template *clusterv1.MachineTemplateSpec) uint32 {
-	machineTemplateSpecHasher := fnv.New32a()
-	DeepHashObject(machineTemplateSpecHasher, *template)
-	return machineTemplateSpecHasher.Sum32()
-}
-
 // ComputeSpewHash computes the hash of a MachineTemplateSpec using the spew library.
-func ComputeSpewHash(template *clusterv1.MachineTemplateSpec) (uint32, error) {
+func ComputeSpewHash(objectToWrite interface{}) (uint32, error) {
 	machineTemplateSpecHasher := fnv.New32a()
-	if err := SpewHashObject(machineTemplateSpecHasher, *template); err != nil {
+	if err := SpewHashObject(machineTemplateSpecHasher, objectToWrite); err != nil {
 		return 0, err
 	}
 	return machineTemplateSpecHasher.Sum32(), nil

controllers/mdutil/util_test.go → controllers/internal/mdutil/util_test.go

@@ -24,7 +24,6 @@ import (
 	"time"
 
 	. "github.com/onsi/gomega"
-
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
@@ -106,62 +105,62 @@ func TestEqualMachineTemplate(t *testing.T) {
 	}{
 		{
 			Name:     "Same spec, same labels",
-			Former:   generateMachineTemplateSpec(map[string]string{}, map[string]string{DefaultMachineDeploymentUniqueLabelKey: "value-1", "something": "else"}),
-			Latter:   generateMachineTemplateSpec(map[string]string{}, map[string]string{DefaultMachineDeploymentUniqueLabelKey: "value-1", "something": "else"}),
+			Former:   generateMachineTemplateSpec(map[string]string{}, map[string]string{clusterv1.MachineDeploymentUniqueLabel: "value-1", "something": "else"}),
+			Latter:   generateMachineTemplateSpec(map[string]string{}, map[string]string{clusterv1.MachineDeploymentUniqueLabel: "value-1", "something": "else"}),
 			Expected: true,
 		},
 		{
 			Name:     "Same spec, only machine-template-hash label value is different",
-			Former:   generateMachineTemplateSpec(map[string]string{}, map[string]string{DefaultMachineDeploymentUniqueLabelKey: "value-1", "something": "else"}),
-			Latter:   generateMachineTemplateSpec(map[string]string{}, map[string]string{DefaultMachineDeploymentUniqueLabelKey: "value-2", "something": "else"}),
+			Former:   generateMachineTemplateSpec(map[string]string{}, map[string]string{clusterv1.MachineDeploymentUniqueLabel: "value-1", "something": "else"}),
+			Latter:   generateMachineTemplateSpec(map[string]string{}, map[string]string{clusterv1.MachineDeploymentUniqueLabel: "value-2", "something": "else"}),
 			Expected: true,
 		},
 		{
 			Name:     "Same spec, the former doesn't have machine-template-hash label",
 			Former:   generateMachineTemplateSpec(map[string]string{}, map[string]string{"something": "else"}),
-			Latter:   generateMachineTemplateSpec(map[string]string{}, map[string]string{DefaultMachineDeploymentUniqueLabelKey: "value-2", "something": "else"}),
+			Latter:   generateMachineTemplateSpec(map[string]string{}, map[string]string{clusterv1.MachineDeploymentUniqueLabel: "value-2", "something": "else"}),
 			Expected: true,
 		},
 		{
 			Name:     "Same spec, the former doesn't have machine-template-hash label",
 			Former:   generateMachineTemplateSpec(map[string]string{}, map[string]string{"something": "else"}),
-			Latter:   generateMachineTemplateSpec(map[string]string{}, map[string]string{DefaultMachineDeploymentUniqueLabelKey: "value-2", "something": "else"}),
+			Latter:   generateMachineTemplateSpec(map[string]string{}, map[string]string{clusterv1.MachineDeploymentUniqueLabel: "value-2", "something": "else"}),
 			Expected: true,
 		},
 		{
 			Name:     "Same spec, the label is different, the former doesn't have machine-template-hash label, same number of labels",
 			Former:   generateMachineTemplateSpec(map[string]string{}, map[string]string{"something": "else"}),
-			Latter:   generateMachineTemplateSpec(map[string]string{}, map[string]string{DefaultMachineDeploymentUniqueLabelKey: "value-2"}),
+			Latter:   generateMachineTemplateSpec(map[string]string{}, map[string]string{clusterv1.MachineDeploymentUniqueLabel: "value-2"}),
 			Expected: false,
 		},
 		{
 			Name:     "Same spec, the label is different, the latter doesn't have machine-template-hash label, same number of labels",
-			Former:   generateMachineTemplateSpec(map[string]string{}, map[string]string{DefaultMachineDeploymentUniqueLabelKey: "value-1"}),
+			Former:   generateMachineTemplateSpec(map[string]string{}, map[string]string{clusterv1.MachineDeploymentUniqueLabel: "value-1"}),
 			Latter:   generateMachineTemplateSpec(map[string]string{}, map[string]string{"something": "else"}),
 			Expected: false,
 		},
 		{
 			Name:     "Same spec, the label is different, and the machine-template-hash label value is the same",
-			Former:   generateMachineTemplateSpec(map[string]string{}, map[string]string{DefaultMachineDeploymentUniqueLabelKey: "value-1"}),
-			Latter:   generateMachineTemplateSpec(map[string]string{}, map[string]string{DefaultMachineDeploymentUniqueLabelKey: "value-1", "something": "else"}),
+			Former:   generateMachineTemplateSpec(map[string]string{}, map[string]string{clusterv1.MachineDeploymentUniqueLabel: "value-1"}),
+			Latter:   generateMachineTemplateSpec(map[string]string{}, map[string]string{clusterv1.MachineDeploymentUniqueLabel: "value-1", "something": "else"}),
 			Expected: false,
 		},
 		{
 			Name:     "Different spec, same labels",
-			Former:   generateMachineTemplateSpec(map[string]string{"former": "value"}, map[string]string{DefaultMachineDeploymentUniqueLabelKey: "value-1", "something": "else"}),
-			Latter:   generateMachineTemplateSpec(map[string]string{"latter": "value"}, map[string]string{DefaultMachineDeploymentUniqueLabelKey: "value-1", "something": "else"}),
+			Former:   generateMachineTemplateSpec(map[string]string{"former": "value"}, map[string]string{clusterv1.MachineDeploymentUniqueLabel: "value-1", "something": "else"}),
+			Latter:   generateMachineTemplateSpec(map[string]string{"latter": "value"}, map[string]string{clusterv1.MachineDeploymentUniqueLabel: "value-1", "something": "else"}),
 			Expected: false,
 		},
 		{
 			Name:     "Different spec, different machine-template-hash label value",
-			Former:   generateMachineTemplateSpec(map[string]string{"x": ""}, map[string]string{DefaultMachineDeploymentUniqueLabelKey: "value-1", "something": "else"}),
-			Latter:   generateMachineTemplateSpec(map[string]string{"x": "1"}, map[string]string{DefaultMachineDeploymentUniqueLabelKey: "value-2", "something": "else"}),
+			Former:   generateMachineTemplateSpec(map[string]string{"x": ""}, map[string]string{clusterv1.MachineDeploymentUniqueLabel: "value-1", "something": "else"}),
+			Latter:   generateMachineTemplateSpec(map[string]string{"x": "1"}, map[string]string{clusterv1.MachineDeploymentUniqueLabel: "value-2", "something": "else"}),
 			Expected: false,
 		},
 		{
 			Name:     "Different spec, the former doesn't have machine-template-hash label",
 			Former:   generateMachineTemplateSpec(map[string]string{"x": ""}, map[string]string{"something": "else"}),
-			Latter:   generateMachineTemplateSpec(map[string]string{"x": "1"}, map[string]string{DefaultMachineDeploymentUniqueLabelKey: "value-2", "something": "else"}),
+			Latter:   generateMachineTemplateSpec(map[string]string{"x": "1"}, map[string]string{clusterv1.MachineDeploymentUniqueLabel: "value-2", "something": "else"}),
 			Expected: false,
 		},
 		{
@@ -273,11 +272,11 @@ func TestFindNewMachineSet(t *testing.T) {
 
 	deployment := generateDeployment("nginx")
 	newMS := generateMS(deployment)
-	newMS.Labels[DefaultMachineDeploymentUniqueLabelKey] = "hash"
+	newMS.Labels[clusterv1.MachineDeploymentUniqueLabel] = "hash"
 	newMS.CreationTimestamp = later
 
 	newMSDup := generateMS(deployment)
-	newMSDup.Labels[DefaultMachineDeploymentUniqueLabelKey] = "different-hash"
+	newMSDup.Labels[clusterv1.MachineDeploymentUniqueLabel] = "different-hash"
 	newMSDup.CreationTimestamp = now
 
 	oldDeployment := generateDeployment("nginx")
@@ -331,11 +330,11 @@ func TestFindOldMachineSets(t *testing.T) {
 	deployment := generateDeployment("nginx")
 	newMS := generateMS(deployment)
 	*(newMS.Spec.Replicas) = 1
-	newMS.Labels[DefaultMachineDeploymentUniqueLabelKey] = "hash"
+	newMS.Labels[clusterv1.MachineDeploymentUniqueLabel] = "hash"
 	newMS.CreationTimestamp = later
 
 	newMSDup := generateMS(deployment)
-	newMSDup.Labels[DefaultMachineDeploymentUniqueLabelKey] = "different-hash"
+	newMSDup.Labels[clusterv1.MachineDeploymentUniqueLabel] = "different-hash"
 	newMSDup.CreationTimestamp = now
 
 	oldDeployment := generateDeployment("nginx")

controllers/machinedeployment_rolling.go

@@ -23,7 +23,7 @@ import (
 	"github.com/pkg/errors"
 	"k8s.io/utils/integer"
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
-	"sigs.k8s.io/cluster-api/controllers/mdutil"
+	"sigs.k8s.io/cluster-api/controllers/internal/mdutil"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 )

controllers/machinedeployment_rolling_test.go

@@ -26,7 +26,7 @@ import (
 	"k8s.io/client-go/tools/record"
 	"k8s.io/utils/pointer"
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
-	"sigs.k8s.io/cluster-api/controllers/mdutil"
+	"sigs.k8s.io/cluster-api/controllers/internal/mdutil"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/client/fake"
 )

controllers/machinedeployment_rollout_ondelete.go

@@ -23,7 +23,7 @@ import (
 	"github.com/pkg/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
-	"sigs.k8s.io/cluster-api/controllers/mdutil"
+	"sigs.k8s.io/cluster-api/controllers/internal/mdutil"
 	"sigs.k8s.io/cluster-api/util/patch"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"

controllers/machinedeployment_sync.go

@@ -30,7 +30,7 @@ import (
 	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/client-go/util/retry"
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
-	"sigs.k8s.io/cluster-api/controllers/mdutil"
+	"sigs.k8s.io/cluster-api/controllers/internal/mdutil"
 	"sigs.k8s.io/cluster-api/feature"
 	"sigs.k8s.io/cluster-api/util"
 	"sigs.k8s.io/cluster-api/util/conditions"
@@ -147,11 +147,11 @@ func (r *MachineDeploymentReconciler) getNewMachineSet(ctx context.Context, d *c
 	}
 	machineTemplateSpecHash := fmt.Sprintf("%d", hash)
 	newMSTemplate.Labels = mdutil.CloneAndAddLabel(d.Spec.Template.Labels,
-		mdutil.DefaultMachineDeploymentUniqueLabelKey, machineTemplateSpecHash)
+		clusterv1.MachineDeploymentUniqueLabel, machineTemplateSpecHash)
 
 	// Add machineTemplateHash label to selector.
 	newMSSelector := mdutil.CloneSelectorAndAddLabel(&d.Spec.Selector,
-		mdutil.DefaultMachineDeploymentUniqueLabelKey, machineTemplateSpecHash)
+		clusterv1.MachineDeploymentUniqueLabel, machineTemplateSpecHash)
 
 	minReadySeconds := int32(0)
 	if d.Spec.MinReadySeconds != nil {

controllers/machinedeployment_sync_test.go

@@ -28,7 +28,7 @@ import (
 	"k8s.io/client-go/tools/record"
 	"k8s.io/utils/pointer"
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
-	"sigs.k8s.io/cluster-api/controllers/mdutil"
+	"sigs.k8s.io/cluster-api/controllers/internal/mdutil"
 	capierrors "sigs.k8s.io/cluster-api/errors"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/client/fake"

controllers/topology/internal/scope/state.go

@@ -19,7 +19,7 @@ package scope
 import (
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
-	"sigs.k8s.io/cluster-api/controllers/mdutil"
+	"sigs.k8s.io/cluster-api/controllers/internal/mdutil"
 )
 
 // ClusterState holds all the objects representing the state of a managed Cluster topology.