diff --git a/pkg/controller/machinedeployment/sync.go b/pkg/controller/machinedeployment/sync.go index 720968315e01..29cb0af16b40 100644 --- a/pkg/controller/machinedeployment/sync.go +++ b/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) diff --git a/pkg/controller/machinedeployment/util/util.go b/pkg/controller/machinedeployment/util/util.go index e22573ee0e40..0450cc547dc0 100644 --- a/pkg/controller/machinedeployment/util/util.go +++ b/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 } diff --git a/pkg/controller/machinedeployment/util/util_test.go b/pkg/controller/machinedeployment/util/util_test.go index e9420dde787b..ee270f9e0e2f 100644 --- a/pkg/controller/machinedeployment/util/util_test.go +++ b/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) {