From 98600a69f08d82c1cda20173a4e778d166a47357 Mon Sep 17 00:00:00 2001 From: Enxebre Date: Thu, 9 Sep 2021 12:50:07 +0200 Subject: [PATCH] Add MachineDeployment rolloutAfter support If the reconciliation time is after spec.rolloutAfter then a rollout should happen or has already happened. A new MachineSet is needed the first time the reconciliation time is after spec.rolloutAfter. Otherwise we pick the oldest with creation timestamp > lastRolloutAfter. If a new MachineSet is needed we include the rolloutAfter hash into the MachineSet name so when a new MachineSet is created the name does not clash with the one for the existing MachineSet with the same template and the rollout can be orchestrated as usual. --- api/v1alpha3/conversion.go | 8 + api/v1alpha3/zz_generated.conversion.go | 16 +- api/v1alpha4/conversion.go | 33 ++++- api/v1alpha4/zz_generated.conversion.go | 40 +++-- api/v1beta1/machinedeployment_types.go | 10 ++ api/v1beta1/zz_generated.deepcopy.go | 4 + .../cluster.x-k8s.io_machinedeployments.yaml | 8 + controllers/internal/mdutil/util.go | 69 ++++++++- controllers/internal/mdutil/util_test.go | 138 +++++++++++++++++- controllers/machinedeployment_sync.go | 57 ++++++-- 10 files changed, 343 insertions(+), 40 deletions(-) diff --git a/api/v1alpha3/conversion.go b/api/v1alpha3/conversion.go index d622a11c2b9f..3bb9a5100354 100644 --- a/api/v1alpha3/conversion.go +++ b/api/v1alpha3/conversion.go @@ -191,6 +191,10 @@ func (src *MachineDeployment) ConvertTo(dstRaw conversion.Hub) error { dst.Spec.Strategy.RollingUpdate.DeletePolicy = restored.Spec.Strategy.RollingUpdate.DeletePolicy } + if restored.Spec.RolloutAfter != nil { + dst.Spec.RolloutAfter = restored.Spec.RolloutAfter + } + dst.Status.Conditions = restored.Status.Conditions return nil } @@ -312,3 +316,7 @@ func Convert_v1alpha3_MachineStatus_To_v1beta1_MachineStatus(in *MachineStatus, // Status.version has been removed in v1beta1, thus requiring custom conversion function. the information will be dropped. return autoConvert_v1alpha3_MachineStatus_To_v1beta1_MachineStatus(in, out, s) } + +func Convert_v1beta1_MachineDeploymentSpec_To_v1alpha3_MachineDeploymentSpec(in *v1beta1.MachineDeploymentSpec, out *MachineDeploymentSpec, s apiconversion.Scope) error { + return autoConvert_v1beta1_MachineDeploymentSpec_To_v1alpha3_MachineDeploymentSpec(in, out, s) +} \ No newline at end of file diff --git a/api/v1alpha3/zz_generated.conversion.go b/api/v1alpha3/zz_generated.conversion.go index c391319bac9f..cb91cef575d8 100644 --- a/api/v1alpha3/zz_generated.conversion.go +++ b/api/v1alpha3/zz_generated.conversion.go @@ -159,11 +159,6 @@ func RegisterConversions(s *runtime.Scheme) error { }); err != nil { return err } - if err := s.AddGeneratedConversionFunc((*v1beta1.MachineDeploymentSpec)(nil), (*MachineDeploymentSpec)(nil), func(a, b interface{}, scope conversion.Scope) error { - return Convert_v1beta1_MachineDeploymentSpec_To_v1alpha3_MachineDeploymentSpec(a.(*v1beta1.MachineDeploymentSpec), b.(*MachineDeploymentSpec), scope) - }); err != nil { - return err - } if err := s.AddGeneratedConversionFunc((*MachineDeploymentStatus)(nil), (*v1beta1.MachineDeploymentStatus)(nil), func(a, b interface{}, scope conversion.Scope) error { return Convert_v1alpha3_MachineDeploymentStatus_To_v1beta1_MachineDeploymentStatus(a.(*MachineDeploymentStatus), b.(*v1beta1.MachineDeploymentStatus), scope) }); err != nil { @@ -334,6 +329,11 @@ func RegisterConversions(s *runtime.Scheme) error { }); err != nil { return err } + if err := s.AddConversionFunc((*v1beta1.MachineDeploymentSpec)(nil), (*MachineDeploymentSpec)(nil), func(a, b interface{}, scope conversion.Scope) error { + return Convert_v1beta1_MachineDeploymentSpec_To_v1alpha3_MachineDeploymentSpec(a.(*v1beta1.MachineDeploymentSpec), b.(*MachineDeploymentSpec), scope) + }); err != nil { + return err + } if err := s.AddConversionFunc((*v1beta1.MachineDeploymentStatus)(nil), (*MachineDeploymentStatus)(nil), func(a, b interface{}, scope conversion.Scope) error { return Convert_v1beta1_MachineDeploymentStatus_To_v1alpha3_MachineDeploymentStatus(a.(*v1beta1.MachineDeploymentStatus), b.(*MachineDeploymentStatus), scope) }); err != nil { @@ -770,6 +770,7 @@ func Convert_v1alpha3_MachineDeploymentSpec_To_v1beta1_MachineDeploymentSpec(in func autoConvert_v1beta1_MachineDeploymentSpec_To_v1alpha3_MachineDeploymentSpec(in *v1beta1.MachineDeploymentSpec, out *MachineDeploymentSpec, s conversion.Scope) error { out.ClusterName = in.ClusterName + // WARNING: in.RolloutAfter requires manual conversion: does not exist in peer-type out.Replicas = (*int32)(unsafe.Pointer(in.Replicas)) out.Selector = in.Selector if err := Convert_v1beta1_MachineTemplateSpec_To_v1alpha3_MachineTemplateSpec(&in.Template, &out.Template, s); err != nil { @@ -791,11 +792,6 @@ func autoConvert_v1beta1_MachineDeploymentSpec_To_v1alpha3_MachineDeploymentSpec return nil } -// Convert_v1beta1_MachineDeploymentSpec_To_v1alpha3_MachineDeploymentSpec is an autogenerated conversion function. -func Convert_v1beta1_MachineDeploymentSpec_To_v1alpha3_MachineDeploymentSpec(in *v1beta1.MachineDeploymentSpec, out *MachineDeploymentSpec, s conversion.Scope) error { - return autoConvert_v1beta1_MachineDeploymentSpec_To_v1alpha3_MachineDeploymentSpec(in, out, s) -} - func autoConvert_v1alpha3_MachineDeploymentStatus_To_v1beta1_MachineDeploymentStatus(in *MachineDeploymentStatus, out *v1beta1.MachineDeploymentStatus, s conversion.Scope) error { out.ObservedGeneration = in.ObservedGeneration out.Selector = in.Selector diff --git a/api/v1alpha4/conversion.go b/api/v1alpha4/conversion.go index f25d9d544f88..d6590d70c29f 100644 --- a/api/v1alpha4/conversion.go +++ b/api/v1alpha4/conversion.go @@ -19,6 +19,7 @@ package v1alpha4 import ( apiconversion "k8s.io/apimachinery/pkg/conversion" "sigs.k8s.io/cluster-api/api/v1beta1" + utilconversion "sigs.k8s.io/cluster-api/util/conversion" "sigs.k8s.io/controller-runtime/pkg/conversion" ) @@ -121,15 +122,39 @@ func (dst *MachineSetList) ConvertFrom(srcRaw conversion.Hub) error { func (src *MachineDeployment) ConvertTo(dstRaw conversion.Hub) error { dst := dstRaw.(*v1beta1.MachineDeployment) - return Convert_v1alpha4_MachineDeployment_To_v1beta1_MachineDeployment(src, dst, nil) + if err := Convert_v1alpha4_MachineDeployment_To_v1beta1_MachineDeployment(src, dst, nil); err != nil { + return err + } + + // Manually restore data. + restored := &v1beta1.MachineDeployment{} + if ok, err := utilconversion.UnmarshalData(src, restored); err != nil || !ok { + return err + } + + if restored.Spec.RolloutAfter != nil { + dst.Spec.RolloutAfter = restored.Spec.RolloutAfter + } + + return nil } func (dst *MachineDeployment) ConvertFrom(srcRaw conversion.Hub) error { src := srcRaw.(*v1beta1.MachineDeployment) - return Convert_v1beta1_MachineDeployment_To_v1alpha4_MachineDeployment(src, dst, nil) + if err := Convert_v1beta1_MachineDeployment_To_v1alpha4_MachineDeployment(src, dst, nil); err != nil { + return err + } + + // Preserve Hub data on down-conversion except for metadata + if err := utilconversion.MarshalData(src, dst); err != nil { + return err + } + + return nil } + func (src *MachineDeploymentList) ConvertTo(dstRaw conversion.Hub) error { dst := dstRaw.(*v1beta1.MachineDeploymentList) @@ -170,3 +195,7 @@ func Convert_v1alpha4_MachineStatus_To_v1beta1_MachineStatus(in *MachineStatus, // Status.version has been removed in v1beta1, thus requiring custom conversion function. the information will be dropped. return autoConvert_v1alpha4_MachineStatus_To_v1beta1_MachineStatus(in, out, s) } + +func Convert_v1beta1_MachineDeploymentSpec_To_v1alpha4_MachineDeploymentSpec(in *v1beta1.MachineDeploymentSpec, out *MachineDeploymentSpec, s apiconversion.Scope) error { + return autoConvert_v1beta1_MachineDeploymentSpec_To_v1alpha4_MachineDeploymentSpec(in, out, s) +} \ No newline at end of file diff --git a/api/v1alpha4/zz_generated.conversion.go b/api/v1alpha4/zz_generated.conversion.go index 1b138f920109..35b6442fff46 100644 --- a/api/v1alpha4/zz_generated.conversion.go +++ b/api/v1alpha4/zz_generated.conversion.go @@ -254,11 +254,6 @@ func RegisterConversions(s *runtime.Scheme) error { }); err != nil { return err } - if err := s.AddGeneratedConversionFunc((*v1beta1.MachineDeploymentSpec)(nil), (*MachineDeploymentSpec)(nil), func(a, b interface{}, scope conversion.Scope) error { - return Convert_v1beta1_MachineDeploymentSpec_To_v1alpha4_MachineDeploymentSpec(a.(*v1beta1.MachineDeploymentSpec), b.(*MachineDeploymentSpec), scope) - }); err != nil { - return err - } if err := s.AddGeneratedConversionFunc((*MachineDeploymentStatus)(nil), (*v1beta1.MachineDeploymentStatus)(nil), func(a, b interface{}, scope conversion.Scope) error { return Convert_v1alpha4_MachineDeploymentStatus_To_v1beta1_MachineDeploymentStatus(a.(*MachineDeploymentStatus), b.(*v1beta1.MachineDeploymentStatus), scope) }); err != nil { @@ -479,6 +474,11 @@ func RegisterConversions(s *runtime.Scheme) error { }); err != nil { return err } + if err := s.AddConversionFunc((*v1beta1.MachineDeploymentSpec)(nil), (*MachineDeploymentSpec)(nil), func(a, b interface{}, scope conversion.Scope) error { + return Convert_v1beta1_MachineDeploymentSpec_To_v1alpha4_MachineDeploymentSpec(a.(*v1beta1.MachineDeploymentSpec), b.(*MachineDeploymentSpec), scope) + }); err != nil { + return err + } return nil } @@ -1038,7 +1038,17 @@ func Convert_v1beta1_MachineDeploymentClassTemplate_To_v1alpha4_MachineDeploymen func autoConvert_v1alpha4_MachineDeploymentList_To_v1beta1_MachineDeploymentList(in *MachineDeploymentList, out *v1beta1.MachineDeploymentList, s conversion.Scope) error { out.ListMeta = in.ListMeta - out.Items = *(*[]v1beta1.MachineDeployment)(unsafe.Pointer(&in.Items)) + if in.Items != nil { + in, out := &in.Items, &out.Items + *out = make([]v1beta1.MachineDeployment, len(*in)) + for i := range *in { + if err := Convert_v1alpha4_MachineDeployment_To_v1beta1_MachineDeployment(&(*in)[i], &(*out)[i], s); err != nil { + return err + } + } + } else { + out.Items = nil + } return nil } @@ -1049,7 +1059,17 @@ func Convert_v1alpha4_MachineDeploymentList_To_v1beta1_MachineDeploymentList(in func autoConvert_v1beta1_MachineDeploymentList_To_v1alpha4_MachineDeploymentList(in *v1beta1.MachineDeploymentList, out *MachineDeploymentList, s conversion.Scope) error { out.ListMeta = in.ListMeta - out.Items = *(*[]MachineDeployment)(unsafe.Pointer(&in.Items)) + if in.Items != nil { + in, out := &in.Items, &out.Items + *out = make([]MachineDeployment, len(*in)) + for i := range *in { + if err := Convert_v1beta1_MachineDeployment_To_v1alpha4_MachineDeployment(&(*in)[i], &(*out)[i], s); err != nil { + return err + } + } + } else { + out.Items = nil + } return nil } @@ -1080,6 +1100,7 @@ func Convert_v1alpha4_MachineDeploymentSpec_To_v1beta1_MachineDeploymentSpec(in func autoConvert_v1beta1_MachineDeploymentSpec_To_v1alpha4_MachineDeploymentSpec(in *v1beta1.MachineDeploymentSpec, out *MachineDeploymentSpec, s conversion.Scope) error { out.ClusterName = in.ClusterName + // WARNING: in.RolloutAfter requires manual conversion: does not exist in peer-type out.Replicas = (*int32)(unsafe.Pointer(in.Replicas)) out.Selector = in.Selector if err := Convert_v1beta1_MachineTemplateSpec_To_v1alpha4_MachineTemplateSpec(&in.Template, &out.Template, s); err != nil { @@ -1093,11 +1114,6 @@ func autoConvert_v1beta1_MachineDeploymentSpec_To_v1alpha4_MachineDeploymentSpec return nil } -// Convert_v1beta1_MachineDeploymentSpec_To_v1alpha4_MachineDeploymentSpec is an autogenerated conversion function. -func Convert_v1beta1_MachineDeploymentSpec_To_v1alpha4_MachineDeploymentSpec(in *v1beta1.MachineDeploymentSpec, out *MachineDeploymentSpec, s conversion.Scope) error { - return autoConvert_v1beta1_MachineDeploymentSpec_To_v1alpha4_MachineDeploymentSpec(in, out, s) -} - func autoConvert_v1alpha4_MachineDeploymentStatus_To_v1beta1_MachineDeploymentStatus(in *MachineDeploymentStatus, out *v1beta1.MachineDeploymentStatus, s conversion.Scope) error { out.ObservedGeneration = in.ObservedGeneration out.Selector = in.Selector diff --git a/api/v1beta1/machinedeployment_types.go b/api/v1beta1/machinedeployment_types.go index 9d096d73c051..3ff5fce9f428 100644 --- a/api/v1beta1/machinedeployment_types.go +++ b/api/v1beta1/machinedeployment_types.go @@ -57,6 +57,9 @@ const ( // MachineDeploymentUniqueLabel is the label applied to Machines // in a MachineDeployment containing the hash of the template. MachineDeploymentUniqueLabel = "machine-template-hash" + + // LastRolloutAfterAnnotation is the last MachineDeployment.Spec.RolloutAfter that was met. + LastRolloutAfterAnnotation = "machinedeployment.clusters.x-k8s.io/lastRollout" ) // ANCHOR: MachineDeploymentSpec @@ -67,6 +70,13 @@ type MachineDeploymentSpec struct { // +kubebuilder:validation:MinLength=1 ClusterName string `json:"clusterName"` + // RolloutAfter performs a rollout of the MachineDeployment + // the first reconciliation loop where the given time is before now(). + // After that moment the field has no effect until it's updated with a new time. + // Any changes to other fields of the spec are not affected by this field and will be rolled out as normal. + // +optional + RolloutAfter *metav1.Time `json:"rolloutAfter,omitempty"` + // Number of desired machines. Defaults to 1. // This is a pointer to distinguish between explicit zero and not specified. // +optional diff --git a/api/v1beta1/zz_generated.deepcopy.go b/api/v1beta1/zz_generated.deepcopy.go index f8d0c1993c06..d9e597fd30f7 100644 --- a/api/v1beta1/zz_generated.deepcopy.go +++ b/api/v1beta1/zz_generated.deepcopy.go @@ -608,6 +608,10 @@ func (in *MachineDeploymentList) DeepCopyObject() runtime.Object { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *MachineDeploymentSpec) DeepCopyInto(out *MachineDeploymentSpec) { *out = *in + if in.RolloutAfter != nil { + in, out := &in.RolloutAfter, &out.RolloutAfter + *out = (*in).DeepCopy() + } if in.Replicas != nil { in, out := &in.Replicas, &out.Replicas *out = new(int32) diff --git a/config/crd/bases/cluster.x-k8s.io_machinedeployments.yaml b/config/crd/bases/cluster.x-k8s.io_machinedeployments.yaml index 0d1a21633ecc..f038f2612439 100644 --- a/config/crd/bases/cluster.x-k8s.io_machinedeployments.yaml +++ b/config/crd/bases/cluster.x-k8s.io_machinedeployments.yaml @@ -1051,6 +1051,14 @@ spec: Defaults to 1. format: int32 type: integer + rolloutAfter: + description: RolloutAfter performs a rollout of the MachineDeployment + the first reconciliation loop where the given time is before now(). + After that moment the field has no effect until it's updated with + a new time. Any changes to other fields of the spec are not affected + by this field and will be rolled out as normal. + format: date-time + type: string selector: description: Label selector for machines. Existing MachineSets whose machines are selected by this will be the ones affected by this diff --git a/controllers/internal/mdutil/util.go b/controllers/internal/mdutil/util.go index 05f710fbbe27..00dfb9cb790b 100644 --- a/controllers/internal/mdutil/util.go +++ b/controllers/internal/mdutil/util.go @@ -24,6 +24,7 @@ import ( "sort" "strconv" "strings" + "time" "github.com/davecgh/go-spew/spew" "github.com/go-logr/logr" @@ -392,6 +393,7 @@ func EqualMachineTemplate(template1, template2 *clusterv1.MachineTemplateSpec) b } // FindNewMachineSet returns the new MS this given deployment targets (the one with the same machine template). +// Deprecated: this does not for rolloutAfter. Use FindNewMachineSetWithRolloutAfter. func FindNewMachineSet(deployment *clusterv1.MachineDeployment, msList []*clusterv1.MachineSet) *clusterv1.MachineSet { sort.Sort(MachineSetsByCreationTimestamp(msList)) for i := range msList { @@ -403,14 +405,57 @@ func FindNewMachineSet(deployment *clusterv1.MachineDeployment, msList []*cluste return msList[i] } } - // new MachineSet does not exist. + // New MachineSet does not exist. return nil } +// FindNewMachineSetWithRolloutAfter is as FindNewMachineSet but it also accounts for RolloutAfter. +func FindNewMachineSetWithRolloutAfter(now *metav1.Time, deployment *clusterv1.MachineDeployment, msList []*clusterv1.MachineSet) (*clusterv1.MachineSet, error) { + // 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(MachineSetsByCreationTimestamp(msList)) + + if deployment.Spec.RolloutAfter != nil { + if now.After(deployment.Spec.RolloutAfter.Time) { + // TODO (alberto): inlining the format statement within the annotation assigment here + // does not set the value. Why? + lastRolloutAfter := deployment.Spec.RolloutAfter.Time.UTC().Format(time.RFC3339) + deployment.Annotations[clusterv1.LastRolloutAfterAnnotation] = lastRolloutAfter + } + } + + for i := range msList { + if EqualMachineTemplate(&msList[i].Spec.Template, &deployment.Spec.Template) { + // If a rolloutAfter should happen or has already happened + // we pick the oldest one with creationTimestamp after rolloutAfter. + if lastRolloutAfter, ok := deployment.Annotations[clusterv1.LastRolloutAfterAnnotation]; ok { + created := msList[i].CreationTimestamp + t, err := time.Parse(time.RFC3339, lastRolloutAfter) + if err != nil { + return nil, err + } + if created.After(t) { + return msList[i], nil + } + continue + } + + // If a rolloutAfter never took place just pick the oldest one. + return msList[i], nil + } + } + + // New MachineSet does not exist. + return nil, nil +} + // FindOldMachineSets returns the old machine sets targeted by the given Deployment, with the given slice of MSes. // 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 +// Deprecated: this does not for rolloutAfter. Use FindOldMachineSetsWithRolloutAfter. func FindOldMachineSets(deployment *clusterv1.MachineDeployment, msList []*clusterv1.MachineSet) ([]*clusterv1.MachineSet, []*clusterv1.MachineSet) { var requiredMSs []*clusterv1.MachineSet allMSs := make([]*clusterv1.MachineSet, 0, len(msList)) @@ -428,6 +473,28 @@ func FindOldMachineSets(deployment *clusterv1.MachineDeployment, msList []*clust return requiredMSs, allMSs } +// FindOldMachineSetsWithRolloutAfter is as FindOldMachineSets but also accounts for rolloutAfter. +func FindOldMachineSetsWithRolloutAfter(now *metav1.Time, deployment *clusterv1.MachineDeployment, msList []*clusterv1.MachineSet) ([]*clusterv1.MachineSet, []*clusterv1.MachineSet, error) { + var requiredMSs []*clusterv1.MachineSet + allMSs := make([]*clusterv1.MachineSet, 0, len(msList)) + newMS, err := FindNewMachineSetWithRolloutAfter(now, deployment, msList) + if err != nil { + return nil, nil, err + } + + for _, ms := range msList { + // Filter out new machine set + if newMS != nil && ms.UID == newMS.UID { + continue + } + allMSs = append(allMSs, ms) + if *(ms.Spec.Replicas) != 0 { + requiredMSs = append(requiredMSs, ms) + } + } + return requiredMSs, allMSs, nil +} + // GetReplicaCountForMachineSets returns the sum of Replicas of the given machine sets. func GetReplicaCountForMachineSets(machineSets []*clusterv1.MachineSet) int32 { totalReplicas := int32(0) diff --git a/controllers/internal/mdutil/util_test.go b/controllers/internal/mdutil/util_test.go index 8838724f3b8e..4a31630cf79d 100644 --- a/controllers/internal/mdutil/util_test.go +++ b/controllers/internal/mdutil/util_test.go @@ -33,6 +33,11 @@ import ( clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" ) +const ( + hashValue = "hash" + differentHashValue = "different-hash" +) + func newDControllerRef(d *clusterv1.MachineDeployment) *metav1.OwnerReference { isController := true return &metav1.OwnerReference{ @@ -272,11 +277,11 @@ func TestFindNewMachineSet(t *testing.T) { deployment := generateDeployment("nginx") newMS := generateMS(deployment) - newMS.Labels[clusterv1.MachineDeploymentUniqueLabel] = "hash" + newMS.Labels[clusterv1.MachineDeploymentUniqueLabel] = hashValue newMS.CreationTimestamp = later newMSDup := generateMS(deployment) - newMSDup.Labels[clusterv1.MachineDeploymentUniqueLabel] = "different-hash" + newMSDup.Labels[clusterv1.MachineDeploymentUniqueLabel] = differentHashValue newMSDup.CreationTimestamp = now oldDeployment := generateDeployment("nginx") @@ -322,6 +327,131 @@ func TestFindNewMachineSet(t *testing.T) { } } +func TestFindNewMachineSetWithRolloutAfter(t *testing.T) { + now := metav1.Now() + later := metav1.Time{Time: now.Add(time.Minute)} + + // Cases when a rolloutAfter never took place and is not required. + deployment := generateDeployment("nginx") + rolloutAfter := metav1.Time{Time: now.Add(1 * time.Hour)} + deployment.Spec.RolloutAfter = &rolloutAfter + newMS := generateMS(deployment) + newMS.Labels[clusterv1.MachineDeploymentUniqueLabel] = hashValue + newMS.CreationTimestamp = later + + newMSDup := generateMS(deployment) + newMSDup.Labels[clusterv1.MachineDeploymentUniqueLabel] = differentHashValue + newMSDup.CreationTimestamp = now + + oldDeployment := generateDeployment("nginx") + oldMS := generateMS(oldDeployment) + oldMS.Spec.Template.Annotations = map[string]string{ + "old": "true", + } + oldMS.Status.FullyLabeledReplicas = *(oldMS.Spec.Replicas) + + // Cases when rolloutAfter is required. + rolloutAfter = metav1.Time{Time: now.Add(-1 * time.Second)} + deploymentNeedRolloutAfter := generateDeployment("nginx") + deploymentNeedRolloutAfter.Spec.RolloutAfter = &rolloutAfter + machineSetBeforeRolloutAfter := generateMS(deployment) + machineSetBeforeRolloutAfter.Labels[clusterv1.MachineDeploymentUniqueLabel] = hashValue + machineSetBeforeRolloutAfter.CreationTimestamp = metav1.Time{ + Time: deploymentNeedRolloutAfter.Spec.RolloutAfter.Add(-2 * time.Minute), + } + + machineSetAfterRolloutAfter := generateMS(deployment) + machineSetAfterRolloutAfter.Labels[clusterv1.MachineDeploymentUniqueLabel] = hashValue + machineSetAfterRolloutAfter.CreationTimestamp = metav1.Time{ + Time: deploymentNeedRolloutAfter.Spec.RolloutAfter.Add(2 * time.Minute), + } + + oldestMachineSetAfterRolloutAfter := generateMS(deployment) + oldestMachineSetAfterRolloutAfter.Labels[clusterv1.MachineDeploymentUniqueLabel] = hashValue + oldestMachineSetAfterRolloutAfter.CreationTimestamp = metav1.Time{ + Time: deploymentNeedRolloutAfter.Spec.RolloutAfter.Add(1 * time.Minute), + } + + // Cases with when a previous rolloutAfter already took place and rolloutAfter is not required. + nextRolloutAfter := metav1.Time{Time: now.Add(1 * time.Hour)} + lastRolloutAfter := metav1.Time{Time: now.Add(-1 * time.Hour)} + deploymentRolloutAfterHappened := generateDeployment("nginx") + deploymentRolloutAfterHappened.Spec.RolloutAfter = &nextRolloutAfter + deploymentRolloutAfterHappened.Annotations[clusterv1.LastRolloutAfterAnnotation] = lastRolloutAfter.Format(time.RFC3339) + + machineSetBeforeLastRolloutAfter := generateMS(deployment) + machineSetBeforeLastRolloutAfter.Labels[clusterv1.MachineDeploymentUniqueLabel] = hashValue + machineSetBeforeLastRolloutAfter.CreationTimestamp = metav1.Time{ + Time: lastRolloutAfter.Add(-1 * time.Minute), + } + + machineSetAfterLastRolloutAfter := generateMS(deployment) + machineSetAfterLastRolloutAfter.Labels[clusterv1.MachineDeploymentUniqueLabel] = hashValue + machineSetAfterLastRolloutAfter.CreationTimestamp = metav1.Time{ + Time: lastRolloutAfter.Add(2 * time.Minute), + } + + oldestMachineSetAfterLastRolloutAfter := generateMS(deployment) + oldestMachineSetAfterLastRolloutAfter.Labels[clusterv1.MachineDeploymentUniqueLabel] = hashValue + oldestMachineSetAfterLastRolloutAfter.CreationTimestamp = metav1.Time{ + Time: lastRolloutAfter.Add(1 * time.Minute), + } + + tests := []struct { + Name string + deployment clusterv1.MachineDeployment + msList []*clusterv1.MachineSet + expected *clusterv1.MachineSet + }{ + { + Name: "When the templates match it should find the MachineSet even though the machine-template-hash value differs", + deployment: deployment, + msList: []*clusterv1.MachineSet{&newMS, &oldMS}, + expected: &newMS, + }, + { + Name: "When there are more than one MachineSet with the same template it should find the oldest", + deployment: deployment, + msList: []*clusterv1.MachineSet{&newMS, &oldMS, &newMSDup}, + expected: &newMSDup, + }, + { + Name: "When no MachineSets match template it should return nil", + deployment: deployment, + msList: []*clusterv1.MachineSet{&oldMS}, + expected: nil, + }, + { + Name: "When rolloutAfter is required and no MachineSets were created after it should return nil", + deployment: deploymentNeedRolloutAfter, + msList: []*clusterv1.MachineSet{&machineSetBeforeRolloutAfter}, + expected: nil, + }, + { + Name: "When rolloutAfter is required it should return the oldest created after rolloutAfter", + deployment: deploymentNeedRolloutAfter, + msList: []*clusterv1.MachineSet{&machineSetBeforeRolloutAfter, &machineSetAfterRolloutAfter, &oldestMachineSetAfterRolloutAfter}, + expected: &oldestMachineSetAfterRolloutAfter, + }, + { + Name: "When rolloutAfter is not required but another one took place in the past it should return oldest created after the last rolloutAfter", + deployment: deploymentRolloutAfterHappened, + msList: []*clusterv1.MachineSet{&machineSetBeforeLastRolloutAfter, &machineSetAfterLastRolloutAfter, &oldestMachineSetAfterLastRolloutAfter}, + expected: &oldestMachineSetAfterLastRolloutAfter, + }, + } + + for _, test := range tests { + t.Run(test.Name, func(t *testing.T) { + g := NewWithT(t) + + ms, err := FindNewMachineSetWithRolloutAfter(&now, &test.deployment, test.msList) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(ms).To(Equal(test.expected)) + }) + } +} + func TestFindOldMachineSets(t *testing.T) { now := metav1.Now() later := metav1.Time{Time: now.Add(time.Minute)} @@ -330,11 +460,11 @@ func TestFindOldMachineSets(t *testing.T) { deployment := generateDeployment("nginx") newMS := generateMS(deployment) *(newMS.Spec.Replicas) = 1 - newMS.Labels[clusterv1.MachineDeploymentUniqueLabel] = "hash" + newMS.Labels[clusterv1.MachineDeploymentUniqueLabel] = hashValue newMS.CreationTimestamp = later newMSDup := generateMS(deployment) - newMSDup.Labels[clusterv1.MachineDeploymentUniqueLabel] = "different-hash" + newMSDup.Labels[clusterv1.MachineDeploymentUniqueLabel] = differentHashValue newMSDup.CreationTimestamp = now oldDeployment := generateDeployment("nginx") diff --git a/controllers/machinedeployment_sync.go b/controllers/machinedeployment_sync.go index 9d3780a7946e..7f98e6bb2234 100644 --- a/controllers/machinedeployment_sync.go +++ b/controllers/machinedeployment_sync.go @@ -19,6 +19,7 @@ package controllers import ( "context" "fmt" + "hash/fnv" "sort" "strconv" @@ -75,10 +76,14 @@ func (r *MachineDeploymentReconciler) sync(ctx context.Context, d *clusterv1.Mac // 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 *MachineDeploymentReconciler) getAllMachineSetsAndSyncRevision(ctx context.Context, d *clusterv1.MachineDeployment, msList []*clusterv1.MachineSet, createIfNotExisted bool) (*clusterv1.MachineSet, []*clusterv1.MachineSet, error) { - _, allOldMSs := mdutil.FindOldMachineSets(d, msList) + now := metav1.Now() + _, allOldMSs, err := mdutil.FindOldMachineSetsWithRolloutAfter(&now, d, msList) + if err != nil { + return nil, nil, err + } // Get new machine set with the updated revision number - newMS, err := r.getNewMachineSet(ctx, d, msList, allOldMSs, createIfNotExisted) + newMS, err := r.getNewMachineSet(ctx, &now, d, msList, allOldMSs, createIfNotExisted) if err != nil { return nil, nil, err } @@ -91,10 +96,13 @@ func (r *MachineDeploymentReconciler) getAllMachineSetsAndSyncRevision(ctx conte // 2. If there's existing new MS, update its revision number if it's smaller than (maxOldRevision + 1), where maxOldRevision is the max revision number among all old MSes. // 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 *MachineDeploymentReconciler) getNewMachineSet(ctx context.Context, d *clusterv1.MachineDeployment, msList, oldMSs []*clusterv1.MachineSet, createIfNotExisted bool) (*clusterv1.MachineSet, error) { +func (r *MachineDeploymentReconciler) getNewMachineSet(ctx context.Context, now *metav1.Time, d *clusterv1.MachineDeployment, msList, oldMSs []*clusterv1.MachineSet, createIfNotExisted bool) (*clusterv1.MachineSet, error) { log := ctrl.LoggerFrom(ctx) - existingNewMS := mdutil.FindNewMachineSet(d, msList) + existingNewMS, err := mdutil.FindNewMachineSetWithRolloutAfter(now, d, msList) + if err != nil { + return nil, err + } // Calculate the max revision number among all old MSes maxOldRevision := mdutil.MaxRevision(oldMSs, log) @@ -139,19 +147,18 @@ func (r *MachineDeploymentReconciler) getNewMachineSet(ctx context.Context, d *c return nil, nil } - // new MachineSet does not exist, create one. + // New MachineSet does not exist, create one. newMSTemplate := *d.Spec.Template.DeepCopy() - hash, err := mdutil.ComputeSpewHash(&newMSTemplate) + machineSetName, machineDeploymentHash, err := generateMachineSetName(d, now) if err != nil { return nil, err } - machineTemplateSpecHash := fmt.Sprintf("%d", hash) - newMSTemplate.Labels = mdutil.CloneAndAddLabel(d.Spec.Template.Labels, - clusterv1.MachineDeploymentUniqueLabel, machineTemplateSpecHash) + + newMSTemplate.Labels = mdutil.CloneAndAddLabel(d.Spec.Template.Labels, clusterv1.MachineDeploymentUniqueLabel, machineDeploymentHash) // Add machineTemplateHash label to selector. newMSSelector := mdutil.CloneSelectorAndAddLabel(&d.Spec.Selector, - clusterv1.MachineDeploymentUniqueLabel, machineTemplateSpecHash) + clusterv1.MachineDeploymentUniqueLabel, machineDeploymentHash) minReadySeconds := int32(0) if d.Spec.MinReadySeconds != nil { @@ -162,7 +169,7 @@ func (r *MachineDeploymentReconciler) getNewMachineSet(ctx context.Context, d *c newMS := clusterv1.MachineSet{ ObjectMeta: metav1.ObjectMeta{ // Make the name deterministic, to ensure idempotence - Name: d.Name + "-" + apirand.SafeEncodeString(machineTemplateSpecHash), + Name: machineSetName, Namespace: d.Namespace, Labels: newMSTemplate.Labels, OwnerReferences: []metav1.OwnerReference{*metav1.NewControllerRef(d, machineDeploymentKind)}, @@ -252,6 +259,34 @@ func (r *MachineDeploymentReconciler) getNewMachineSet(ctx context.Context, d *c return createdMS, err } +func generateMachineSetName(d *clusterv1.MachineDeployment, now *metav1.Time) (string, string, error) { + template := *d.Spec.Template.DeepCopy() + hash, err := mdutil.ComputeSpewHash(&template) + if err != nil { + return "", "", err + } + machineDeploymentHash := fmt.Sprintf("%d", hash) + + if d.Spec.RolloutAfter != nil { + // If the reconciliation time is after spec.rolloutAfter then a rollout should happen or has already happened. + // We include the rolloutAfter hash into the MachineSet name so the first time that + // the reconciliation time is after spec.rolloutAfter then a new MachineSet is created with a name + // which does not clash with the one for the existing MachineSet with the same MachineDeployment template + // and the rollout is orchestrated as usual. + if now.After(d.Spec.RolloutAfter.Time) { + hasher := fnv.New32a() + if err := mdutil.SpewHashObject(hasher, d.Spec.RolloutAfter.Time); err != nil { + return "", "", err + } + rolloutAfterHash := hasher.Sum32() + machineDeploymentHash += fmt.Sprintf("%d", rolloutAfterHash) + } + } + machineSetName := d.Name + "-" + apirand.SafeEncodeString(machineDeploymentHash) + + return machineSetName, machineDeploymentHash, nil +} + // scale scales proportionally in order to mitigate risk. Otherwise, scaling up can increase the size // of the new machine set and scaling down can decrease the sizes of the old ones, both of which would // have the effect of hastening the rollout progress, which could produce a higher proportion of unavailable