Add support for MachineDeployment to detect infrastructure spec changes

Signed-off-by: Vince Prignano <[email protected]>

pkg/controller/machinedeployment/sync.go

@@ -18,7 +18,6 @@ package machinedeployment
 
 import (
 	"context"
-	"fmt"
 	"reflect"
 	"sort"
 	"strconv"
@@ -72,7 +71,7 @@ func (r *ReconcileMachineDeployment) sync(d *clusterv1.MachineDeployment, msList
 // Note that currently the deployment controller is using caches to avoid querying the server for reads.
 // This may lead to stale reads of machine sets, thus incorrect deployment status.
 func (r *ReconcileMachineDeployment) getAllMachineSetsAndSyncRevision(d *clusterv1.MachineDeployment, msList []*clusterv1.MachineSet, machineMap map[types.UID]*clusterv1.MachineList, createIfNotExisted bool) (*clusterv1.MachineSet, []*clusterv1.MachineSet, error) {
-	_, allOldMSs := dutil.FindOldMachineSets(d, msList)
+	_, allOldMSs := dutil.FindOldMachineSets(r.Client, d, msList)
 
 	// Get new machine set with the updated revision number
 	newMS, err := r.getNewMachineSet(d, msList, allOldMSs, createIfNotExisted)
@@ -89,7 +88,7 @@ func (r *ReconcileMachineDeployment) getAllMachineSetsAndSyncRevision(d *cluster
 // 3. If there's no existing new MS and createIfNotExisted is true, create one with appropriate revision number (maxOldRevision + 1) and replicas.
 // Note that the machine-template-hash will be added to adopted MSes and machines.
 func (r *ReconcileMachineDeployment) getNewMachineSet(d *clusterv1.MachineDeployment, msList, oldMSs []*clusterv1.MachineSet, createIfNotExisted bool) (*clusterv1.MachineSet, error) {
-	existingNewMS := dutil.FindNewMachineSet(d, msList)
+	existingNewMS := dutil.FindNewMachineSet(r.Client, d, msList)
 
 	// Calculate the max revision number among all old MSes
 	maxOldRevision := dutil.MaxRevision(oldMSs)
@@ -124,9 +123,13 @@ func (r *ReconcileMachineDeployment) getNewMachineSet(d *clusterv1.MachineDeploy
 		return nil, nil
 	}
 
-	// new MachineSet does not exist, create one.
+	// New MachineSet does not exist, create one.
 	newMSTemplate := *d.Spec.Template.DeepCopy()
-	machineTemplateSpecHash := fmt.Sprintf("%d", dutil.ComputeHash(&newMSTemplate))
+	machineTemplateSpecHash, err := dutil.ComputeHash(r.Client, d.Namespace, &newMSTemplate)
+	if err != nil {
+		return nil, err
+	}
+
 	newMSTemplate.Labels = dutil.CloneAndAddLabel(d.Spec.Template.Labels,
 		dutil.DefaultMachineDeploymentUniqueLabelKey, machineTemplateSpecHash)
 

pkg/controller/machinedeployment/util/util.go

@@ -25,16 +25,20 @@ import (
 	"strings"
 
 	"github.com/davecgh/go-spew/spew"
+	"github.com/pkg/errors"
 	v1 "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/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/runtime"
 	intstrutil "k8s.io/apimachinery/pkg/util/intstr"
 	"k8s.io/klog"
 	"k8s.io/utils/integer"
 	"sigs.k8s.io/cluster-api/pkg/apis/cluster/common"
 	"sigs.k8s.io/cluster-api/pkg/apis/cluster/v1alpha2"
+	"sigs.k8s.io/cluster-api/pkg/controller/external"
+	"sigs.k8s.io/controller-runtime/pkg/client"
 )
 
 const (
@@ -406,16 +410,41 @@ func EqualIgnoreHash(template1, template2 *v1alpha2.MachineTemplateSpec) bool {
 	return apiequality.Semantic.DeepEqual(t1Copy, t2Copy)
 }
 
+func Equals(c client.Client, namespace string, template1, template2 *v1alpha2.MachineTemplateSpec) (bool, error) {
+	t1Copy := template1.DeepCopy()
+	t2Copy := template2.DeepCopy()
+	delete(t1Copy.Labels, DefaultMachineDeploymentUniqueLabelKey)
+	delete(t2Copy.Labels, DefaultMachineDeploymentUniqueLabelKey)
+	h1, err := ComputeHash(c, namespace, t1Copy)
+	if err != nil {
+		return false, err
+	}
+	h2, err := ComputeHash(c, namespace, t2Copy)
+	if err != nil {
+		return false, err
+	}
+	return h1 != "" && h1 == h2, nil
+}
+
 // FindNewMachineSet returns the new MS this given deployment targets (the one with the same machine template).
-func FindNewMachineSet(deployment *v1alpha2.MachineDeployment, msList []*v1alpha2.MachineSet) *v1alpha2.MachineSet {
+func FindNewMachineSet(c client.Client, d *v1alpha2.MachineDeployment, msList []*v1alpha2.MachineSet) *v1alpha2.MachineSet {
 	sort.Sort(MachineSetsByCreationTimestamp(msList))
 	for i := range msList {
-		if EqualIgnoreHash(&msList[i].Spec.Template, &deployment.Spec.Template) {
+
+		if ok, err := Equals(c, d.Namespace, &msList[i].Spec.Template, &d.Spec.Template); ok {
 			// 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]
+		} else if err != nil && EqualIgnoreHash(&msList[i].Spec.Template, &d.Spec.Template) {
+			// Fallback to old equal if there is an error.
+			klog.Warningf("Fallback hash occurred for Machine Deployment %q in namespace %q, got error: %v",
+				d.Name, d.Namespace, err)
+			return msList[i]
+		} else if err != nil {
+			klog.Errorf("Error calculating hash for Machine Deployment %q in namespace %q, got error: %v",
+				d.Name, d.Namespace, err)
 		}
 	}
 	// new MachineSet does not exist.
@@ -426,10 +455,10 @@ func FindNewMachineSet(deployment *v1alpha2.MachineDeployment, msList []*v1alpha
 // Returns two list of machine sets
 //  - the first contains all old machine sets with all non-zero replicas
 //  - the second contains all old machine sets
-func FindOldMachineSets(deployment *v1alpha2.MachineDeployment, msList []*v1alpha2.MachineSet) ([]*v1alpha2.MachineSet, []*v1alpha2.MachineSet) {
+func FindOldMachineSets(c client.Client, deployment *v1alpha2.MachineDeployment, msList []*v1alpha2.MachineSet) ([]*v1alpha2.MachineSet, []*v1alpha2.MachineSet) {
 	var requiredMSs []*v1alpha2.MachineSet
 	allMSs := make([]*v1alpha2.MachineSet, 0, len(msList))
-	newMS := FindNewMachineSet(deployment, msList)
+	newMS := FindNewMachineSet(c, deployment, msList)
 	for _, ms := range msList {
 		// Filter out new machine set
 		if newMS != nil && ms.UID == newMS.UID {
@@ -676,7 +705,6 @@ func CloneSelectorAndAddLabel(selector *metav1.LabelSelector, labelKey, labelVal
 // which follows pointers and prints actual values of the nested objects
 // ensuring the hash does not change when a pointer changes.
 func DeepHashObject(hasher hash.Hash, objectToWrite interface{}) {
-	hasher.Reset()
 	printer := spew.ConfigState{
 		Indent:         " ",
 		SortKeys:       true,
@@ -686,9 +714,23 @@ func DeepHashObject(hasher hash.Hash, objectToWrite interface{}) {
 	printer.Fprintf(hasher, "%#v", objectToWrite)
 }
 
-func ComputeHash(template *v1alpha2.MachineTemplateSpec) uint32 {
-	machineTemplateSpecHasher := fnv.New32a()
-	DeepHashObject(machineTemplateSpecHasher, *template)
+func ComputeHash(c client.Client, namespace string, template *v1alpha2.MachineTemplateSpec) (string, error) {
+	hasher := fnv.New32a()
+
+	// Compute hash of the template.
+	DeepHashObject(hasher, template)
+
+	// Get and compute hash of infrastructure config.
+	infraConfig, err := external.Get(c, &template.Spec.InfrastructureRef, namespace)
+	if err != nil {
+		return "", err
+	}
+	infraSpec, _, err := unstructured.NestedMap(infraConfig.Object, "spec")
+	if err != nil {
+		return "", errors.Wrap(err, "failed to retrieve spec from infrastructure provider object")
+	}
+	DeepHashObject(hasher, infraSpec)
 
-	return machineTemplateSpecHasher.Sum32()
+	// Return the hash.
+	return fmt.Sprintf("%d", hasher.Sum32()), nil
 }

pkg/controller/machinedeployment/util/util_test.go

@@ -25,16 +25,41 @@ import (
 	"testing"
 	"time"
 
+	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
+
 	"github.com/pkg/errors"
+	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/apimachinery/pkg/util/intstr"
 	"k8s.io/apiserver/pkg/storage/names"
+	"k8s.io/client-go/kubernetes/scheme"
 	core "k8s.io/client-go/testing"
+	"sigs.k8s.io/cluster-api/pkg/apis"
 	"sigs.k8s.io/cluster-api/pkg/apis/cluster/common"
 	"sigs.k8s.io/cluster-api/pkg/apis/cluster/v1alpha2"
 	"sigs.k8s.io/cluster-api/pkg/client/clientset_generated/clientset/fake"
+	fakeruntime "sigs.k8s.io/controller-runtime/pkg/client/fake"
+)
+
+func init() {
+	apis.AddToScheme(scheme.Scheme)
+}
+
+var (
+	infraResource = &unstructured.Unstructured{
+		Object: map[string]interface{}{
+			"kind":       "InfrastructureRef",
+			"apiVersion": "infrastructure.cluster.sigs.k8s.io/v1alpha1",
+			"metadata": map[string]interface{}{
+				"name":      "foo-template",
+				"namespace": "default",
+			},
+			"spec":   map[string]interface{}{},
+			"status": map[string]interface{}{},
+		},
+	}
 )
 
 func addListMSReactor(fakeClient *fake.Clientset, obj runtime.Object) *fake.Clientset {
@@ -151,7 +176,13 @@ func generateDeployment(image string) v1alpha2.MachineDeployment {
 				ObjectMeta: v1alpha2.ObjectMeta{
 					Labels: machineLabels,
 				},
-				Spec: v1alpha2.MachineSpec{},
+				Spec: v1alpha2.MachineSpec{
+					InfrastructureRef: corev1.ObjectReference{
+						Name:       infraResource.GetName(),
+						Kind:       infraResource.GetKind(),
+						APIVersion: infraResource.GetAPIVersion(),
+					},
+				},
 			},
 		},
 	}
@@ -164,7 +195,13 @@ func generateMachineTemplateSpec(name, nodeName string, annotations, labels map[
 			Annotations: annotations,
 			Labels:      labels,
 		},
-		Spec: v1alpha2.MachineSpec{},
+		Spec: v1alpha2.MachineSpec{
+			InfrastructureRef: corev1.ObjectReference{
+				Name:       infraResource.GetName(),
+				Kind:       infraResource.GetKind(),
+				APIVersion: infraResource.GetAPIVersion(),
+			},
+		},
 	}
 }
 
@@ -307,7 +344,12 @@ func TestFindNewMachineSet(t *testing.T) {
 
 	for _, test := range tests {
 		t.Run(test.Name, func(t *testing.T) {
-			if ms := FindNewMachineSet(&test.deployment, test.msList); !reflect.DeepEqual(ms, test.expected) {
+			items := []runtime.Object{infraResource}
+			for _, ms := range test.msList {
+				items = append(items, ms)
+			}
+			c := fakeruntime.NewFakeClient(items...)
+			if ms := FindNewMachineSet(c, &test.deployment, test.msList); !reflect.DeepEqual(ms, test.expected) {
 				t.Errorf("In test case %q, expected %#v, got %#v", test.Name, test.expected, ms)
 			}
 		})
@@ -375,7 +417,12 @@ func TestFindOldMachineSets(t *testing.T) {
 
 	for _, test := range tests {
 		t.Run(test.Name, func(t *testing.T) {
-			requireMS, allMS := FindOldMachineSets(&test.deployment, test.msList)
+			items := []runtime.Object{infraResource}
+			for _, ms := range test.msList {
+				items = append(items, ms)
+			}
+			c := fakeruntime.NewFakeClient(items...)
+			requireMS, allMS := FindOldMachineSets(c, &test.deployment, test.msList)
 			sort.Sort(MachineSetsByCreationTimestamp(allMS))
 			sort.Sort(MachineSetsByCreationTimestamp(test.expected))
 			if !reflect.DeepEqual(allMS, test.expected) {