From b94dc5568a48e25eae637ad54e598777175f982b Mon Sep 17 00:00:00 2001 From: Enxebre Date: Thu, 9 Sep 2021 12:50:07 +0200 Subject: [PATCH] Implement MachineDeployment rolloutAfter support If the reconciliation time is after spec.rolloutAfter then a rollout should happen or has already happened. A new MachineSet will be created at the first time the reconciliation time is after spec.rolloutAfter. Otherwise the oldest with creation timestamp > lastRolloutAfter annotation is picked. If a new MachineSet is required due to reconciliation time > spec.rolloutAfter the rolloutAfter time is added for creating the hash of the MachineSet name. When a new MachineSet is created the name does not clash with the existing MachineSet having the same template and the rollout can be orchestrated as usual. Co-authored-by: Enxebre --- api/v1alpha3/conversion.go | 8 + api/v1alpha3/zz_generated.conversion.go | 16 +- api/v1alpha4/conversion.go | 8 + api/v1alpha4/zz_generated.conversion.go | 16 +- api/v1beta1/machinedeployment_types.go | 8 + api/v1beta1/machineset_types.go | 4 + api/v1beta1/zz_generated.deepcopy.go | 4 + api/v1beta1/zz_generated.openapi.go | 8 +- .../cluster.x-k8s.io_machinedeployments.yaml | 7 + .../machinedeployment_sync.go | 70 ++++++-- .../machinedeployment/mdutil/util.go | 92 ++++++++-- .../machinedeployment/mdutil/util_test.go | 160 +++++++++++++++--- 12 files changed, 327 insertions(+), 74 deletions(-) diff --git a/api/v1alpha3/conversion.go b/api/v1alpha3/conversion.go index 4b3c5dd99e9f..e632cffcbbae 100644 --- a/api/v1alpha3/conversion.go +++ b/api/v1alpha3/conversion.go @@ -195,6 +195,10 @@ func (src *MachineDeployment) ConvertTo(dstRaw conversion.Hub) error { } dst.Spec.Template.Spec.NodeDeletionTimeout = restored.Spec.Template.Spec.NodeDeletionTimeout + if restored.Spec.RolloutAfter != nil { + dst.Spec.RolloutAfter = restored.Spec.RolloutAfter + } + dst.Status.Conditions = restored.Status.Conditions return nil } @@ -312,6 +316,10 @@ func Convert_v1beta1_MachineSpec_To_v1alpha3_MachineSpec(in *clusterv1.MachineSp return autoConvert_v1beta1_MachineSpec_To_v1alpha3_MachineSpec(in, out, s) } +func Convert_v1beta1_MachineDeploymentSpec_To_v1alpha3_MachineDeploymentSpec(in *clusterv1.MachineDeploymentSpec, out *MachineDeploymentSpec, s apiconversion.Scope) error { + return autoConvert_v1beta1_MachineDeploymentSpec_To_v1alpha3_MachineDeploymentSpec(in, out, s) +} + func Convert_v1beta1_MachineDeploymentStatus_To_v1alpha3_MachineDeploymentStatus(in *clusterv1.MachineDeploymentStatus, out *MachineDeploymentStatus, s apiconversion.Scope) error { // Status.Conditions was introduced in v1alpha4, thus requiring a custom conversion function; the values is going to be preserved in an annotation thus allowing roundtrip without loosing informations return autoConvert_v1beta1_MachineDeploymentStatus_To_v1alpha3_MachineDeploymentStatus(in, out, s) diff --git a/api/v1alpha3/zz_generated.conversion.go b/api/v1alpha3/zz_generated.conversion.go index 9661e6cc3f18..1fa7311979d2 100644 --- a/api/v1alpha3/zz_generated.conversion.go +++ b/api/v1alpha3/zz_generated.conversion.go @@ -160,11 +160,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 { @@ -330,6 +325,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 { @@ -771,6 +771,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 { @@ -792,11 +793,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 ed788a78e183..f54bc4571fb4 100644 --- a/api/v1alpha4/conversion.go +++ b/api/v1alpha4/conversion.go @@ -243,6 +243,10 @@ func (src *MachineDeployment) ConvertTo(dstRaw conversion.Hub) error { } dst.Spec.Template.Spec.NodeDeletionTimeout = restored.Spec.Template.Spec.NodeDeletionTimeout + if restored.Spec.RolloutAfter != nil { + dst.Spec.RolloutAfter = restored.Spec.RolloutAfter + } + return nil } @@ -308,6 +312,10 @@ func Convert_v1beta1_MachineSpec_To_v1alpha4_MachineSpec(in *clusterv1.MachineSp return autoConvert_v1beta1_MachineSpec_To_v1alpha4_MachineSpec(in, out, s) } +func Convert_v1beta1_MachineDeploymentSpec_To_v1alpha4_MachineDeploymentSpec(in *clusterv1.MachineDeploymentSpec, out *MachineDeploymentSpec, s apiconversion.Scope) error { + return autoConvert_v1beta1_MachineDeploymentSpec_To_v1alpha4_MachineDeploymentSpec(in, out, s) +} + func Convert_v1beta1_Topology_To_v1alpha4_Topology(in *clusterv1.Topology, out *Topology, s apiconversion.Scope) error { // spec.topology.variables has been added with v1beta1. return autoConvert_v1beta1_Topology_To_v1alpha4_Topology(in, out, s) diff --git a/api/v1alpha4/zz_generated.conversion.go b/api/v1alpha4/zz_generated.conversion.go index b065ce6bb082..f3c47dd41320 100644 --- a/api/v1alpha4/zz_generated.conversion.go +++ b/api/v1alpha4/zz_generated.conversion.go @@ -235,11 +235,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 { @@ -465,6 +460,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 + } if err := s.AddConversionFunc((*v1beta1.MachineDeploymentTopology)(nil), (*MachineDeploymentTopology)(nil), func(a, b interface{}, scope conversion.Scope) error { return Convert_v1beta1_MachineDeploymentTopology_To_v1alpha4_MachineDeploymentTopology(a.(*v1beta1.MachineDeploymentTopology), b.(*MachineDeploymentTopology), scope) }); err != nil { @@ -1143,6 +1143,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 { @@ -1156,11 +1157,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 f42ca7947e3a..d56c3f39936a 100644 --- a/api/v1beta1/machinedeployment_types.go +++ b/api/v1beta1/machinedeployment_types.go @@ -67,6 +67,14 @@ type MachineDeploymentSpec struct { // +kubebuilder:validation:MinLength=1 ClusterName string `json:"clusterName"` + // RolloutAfter is a field to indicate a rollout should be performed + // after the specified time even if no changes have been made to the + // MachineDeployment. + // 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/machineset_types.go b/api/v1beta1/machineset_types.go index 1ca4e85d816c..d4d77fc5b232 100644 --- a/api/v1beta1/machineset_types.go +++ b/api/v1beta1/machineset_types.go @@ -29,6 +29,10 @@ const ( // MachineSetTopologyFinalizer is the finalizer used by the topology MachineDeployment controller to // clean up referenced template resources if necessary when a MachineSet is being deleted. MachineSetTopologyFinalizer = "machineset.topology.cluster.x-k8s.io" + + // MachineSetLastRolloutAfterAnnotation gets set during creation of a MachineSet to the value of + // MachineDeployment.Spec.RolloutAfter, if it is not nil. + MachineSetLastRolloutAfterAnnotation = "machineset.clusters.x-k8s.io/lastRollout" ) // ANCHOR: MachineSetSpec diff --git a/api/v1beta1/zz_generated.deepcopy.go b/api/v1beta1/zz_generated.deepcopy.go index 2c7ce3814dbd..a65f2d60ac68 100644 --- a/api/v1beta1/zz_generated.deepcopy.go +++ b/api/v1beta1/zz_generated.deepcopy.go @@ -867,6 +867,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/api/v1beta1/zz_generated.openapi.go b/api/v1beta1/zz_generated.openapi.go index f4fb3053952a..7f65b91ae9bd 100644 --- a/api/v1beta1/zz_generated.openapi.go +++ b/api/v1beta1/zz_generated.openapi.go @@ -1445,6 +1445,12 @@ func schema_sigsk8sio_cluster_api_api_v1beta1_MachineDeploymentSpec(ref common.R Format: "", }, }, + "rolloutAfter": { + SchemaProps: spec.SchemaProps{ + Description: "RolloutAfter is a field to indicate a rollout should be performed after the specified time even if no changes have been made to the MachineDeployment. Any changes to other fields of the spec are not affected by this field and will be rolled out as normal.", + Ref: ref("k8s.io/apimachinery/pkg/apis/meta/v1.Time"), + }, + }, "replicas": { SchemaProps: spec.SchemaProps{ Description: "Number of desired machines. Defaults to 1. This is a pointer to distinguish between explicit zero and not specified.", @@ -1505,7 +1511,7 @@ func schema_sigsk8sio_cluster_api_api_v1beta1_MachineDeploymentSpec(ref common.R }, }, Dependencies: []string{ - "k8s.io/apimachinery/pkg/apis/meta/v1.LabelSelector", "sigs.k8s.io/cluster-api/api/v1beta1.MachineDeploymentStrategy", "sigs.k8s.io/cluster-api/api/v1beta1.MachineTemplateSpec"}, + "k8s.io/apimachinery/pkg/apis/meta/v1.LabelSelector", "k8s.io/apimachinery/pkg/apis/meta/v1.Time", "sigs.k8s.io/cluster-api/api/v1beta1.MachineDeploymentStrategy", "sigs.k8s.io/cluster-api/api/v1beta1.MachineTemplateSpec"}, } } diff --git a/config/crd/bases/cluster.x-k8s.io_machinedeployments.yaml b/config/crd/bases/cluster.x-k8s.io_machinedeployments.yaml index ad98ac79ff5e..b8fe2584e57c 100644 --- a/config/crd/bases/cluster.x-k8s.io_machinedeployments.yaml +++ b/config/crd/bases/cluster.x-k8s.io_machinedeployments.yaml @@ -1065,6 +1065,13 @@ spec: Defaults to 1. format: int32 type: integer + rolloutAfter: + description: RolloutAfter is a field to indicate a rollout should + be performed after the specified time even if no changes have been + made to the MachineDeployment. 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/internal/controllers/machinedeployment/machinedeployment_sync.go b/internal/controllers/machinedeployment/machinedeployment_sync.go index 241f27c7fd08..b5fed4f90774 100644 --- a/internal/controllers/machinedeployment/machinedeployment_sync.go +++ b/internal/controllers/machinedeployment/machinedeployment_sync.go @@ -19,6 +19,7 @@ package machinedeployment import ( "context" "fmt" + "hash/fnv" "sort" "strconv" @@ -77,10 +78,15 @@ func (r *Reconciler) sync(ctx context.Context, d *clusterv1.MachineDeployment, m // 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 *Reconciler) 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.FindOldMachineSets(&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 } @@ -93,10 +99,13 @@ func (r *Reconciler) getAllMachineSetsAndSyncRevision(ctx context.Context, d *cl // 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 *Reconciler) getNewMachineSet(ctx context.Context, d *clusterv1.MachineDeployment, msList, oldMSs []*clusterv1.MachineSet, createIfNotExisted bool) (*clusterv1.MachineSet, error) { +func (r *Reconciler) 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.FindNewMachineSet(now, d, msList) + if err != nil { + return nil, err + } // Calculate the max revision number among all old MSes maxOldRevision := mdutil.MaxRevision(oldMSs, log) @@ -116,7 +125,7 @@ func (r *Reconciler) getNewMachineSet(ctx context.Context, d *clusterv1.MachineD } // Set existing new machine set's annotation - annotationsUpdated := mdutil.SetNewMachineSetAnnotations(d, msCopy, newRevision, true, log) + annotationsUpdated := mdutil.SetNewMachineSetAnnotations(now, d, msCopy, newRevision, true, log) minReadySecondsNeedsUpdate := msCopy.Spec.MinReadySeconds != *d.Spec.MinReadySeconds deletePolicyNeedsUpdate := d.Spec.Strategy.RollingUpdate.DeletePolicy != nil && msCopy.Spec.DeletePolicy != *d.Spec.Strategy.RollingUpdate.DeletePolicy @@ -141,19 +150,18 @@ func (r *Reconciler) getNewMachineSet(ctx context.Context, d *clusterv1.MachineD 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 { @@ -164,7 +172,7 @@ func (r *Reconciler) getNewMachineSet(ctx context.Context, d *clusterv1.MachineD 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)}, @@ -207,7 +215,7 @@ func (r *Reconciler) getNewMachineSet(ctx context.Context, d *clusterv1.MachineD *(newMS.Spec.Replicas) = newReplicasCount // Set new machine set's annotation - mdutil.SetNewMachineSetAnnotations(d, &newMS, newRevision, false, log) + mdutil.SetNewMachineSetAnnotations(now, d, &newMS, newRevision, false, log) // Create the new MachineSet. If it already exists, then we need to check for possible // hash collisions. If there is any other error, we need to report it in the status of // the Deployment. @@ -254,6 +262,42 @@ func (r *Reconciler) getNewMachineSet(ctx context.Context, d *clusterv1.MachineD 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() + + templateAndRolloutAfter := struct { + Template *clusterv1.MachineTemplateSpec + RolloutAfter *metav1.Time + }{ + Template: &template, + RolloutAfter: d.Spec.RolloutAfter, + } + if err := mdutil.SpewHashObject(hasher, templateAndRolloutAfter); 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 diff --git a/internal/controllers/machinedeployment/mdutil/util.go b/internal/controllers/machinedeployment/mdutil/util.go index 8fddb9ba5b63..04e9d4cb0444 100644 --- a/internal/controllers/machinedeployment/mdutil/util.go +++ b/internal/controllers/machinedeployment/mdutil/util.go @@ -24,6 +24,7 @@ import ( "sort" "strconv" "strings" + "time" "github.com/davecgh/go-spew/spew" "github.com/go-logr/logr" @@ -52,6 +53,45 @@ func (o MachineSetsByCreationTimestamp) Less(i, j int) bool { return o[i].CreationTimestamp.Before(&o[j].CreationTimestamp) } +// MachineSetsByRolloutAfterAnnotationAndCreationTimestamp sorts a list of MachineSet by the following criteria +// * rolloutAfter annotation value descending +// * creationTimestamp ascending +// * using their names as a tie breaker. +type MachineSetsByRolloutAfterAnnotationAndCreationTimestamp []*clusterv1.MachineSet + +func (o MachineSetsByRolloutAfterAnnotationAndCreationTimestamp) Len() int { return len(o) } +func (o MachineSetsByRolloutAfterAnnotationAndCreationTimestamp) Swap(i, j int) { + o[i], o[j] = o[j], o[i] +} +func (o MachineSetsByRolloutAfterAnnotationAndCreationTimestamp) Less(i, j int) bool { + iAnnotation, iHasAnnotation := o[i].Annotations[clusterv1.MachineSetLastRolloutAfterAnnotation] + jAnnotation, jHasAnnotation := o[j].Annotations[clusterv1.MachineSetLastRolloutAfterAnnotation] + switch { + case iHasAnnotation && jHasAnnotation: + iTime, err := time.Parse(time.RFC3339, iAnnotation) + if err != nil { + return false + } + jTime, err := time.Parse(time.RFC3339, jAnnotation) + if err != nil { + return true + } + if iTime.After(jTime) { + return true + } + case iHasAnnotation && !jHasAnnotation: + return true + case jHasAnnotation && !iHasAnnotation: + return false + } + + // fallback to comparing CreationTimestamp + if o[i].CreationTimestamp.Equal(&o[j].CreationTimestamp) { + return o[i].Name < o[j].Name + } + return o[i].CreationTimestamp.Before(&o[j].CreationTimestamp) +} + // MachineSetsBySizeOlder sorts a list of MachineSet by size in descending order, using their creation timestamp or name as a tie breaker. // By using the creation timestamp, this sorts from old to new machine sets. type MachineSetsBySizeOlder []*clusterv1.MachineSet @@ -186,7 +226,7 @@ func getIntFromAnnotation(ms *clusterv1.MachineSet, annotationKey string, logger // SetNewMachineSetAnnotations sets new machine set's annotations appropriately by updating its revision and // copying required deployment annotations to it; it returns true if machine set's annotation is changed. -func SetNewMachineSetAnnotations(deployment *clusterv1.MachineDeployment, newMS *clusterv1.MachineSet, newRevision string, exists bool, logger logr.Logger) bool { +func SetNewMachineSetAnnotations(now *metav1.Time, deployment *clusterv1.MachineDeployment, newMS *clusterv1.MachineSet, newRevision string, exists bool, logger logr.Logger) bool { logger = logger.WithValues("MachineSet", klog.KObj(newMS)) // First, copy deployment's annotations (except for apply and revision annotations) @@ -236,6 +276,12 @@ func SetNewMachineSetAnnotations(deployment *clusterv1.MachineDeployment, newMS if !exists && SetReplicasAnnotations(newMS, *(deployment.Spec.Replicas), *(deployment.Spec.Replicas)+MaxSurge(*deployment)) { annotationChanged = true } + // If the new MachineSet is about to be created, we need to add the rolloutAfter annotation if it is set at the deployment. + // The annotation is later on used to determine the current MachineSet in FindNewMachineSet. + if !exists && deployment.Spec.RolloutAfter != nil && now.After(deployment.Spec.RolloutAfter.Time) { + newMS.Annotations[clusterv1.MachineSetLastRolloutAfterAnnotation] = deployment.Spec.RolloutAfter.UTC().Format(time.RFC3339) + annotationChanged = true + } return annotationChanged } @@ -395,29 +441,49 @@ func EqualMachineTemplate(template1, template2 *clusterv1.MachineTemplateSpec) b } // FindNewMachineSet returns the new MS this given deployment targets (the one with the same machine template). -func FindNewMachineSet(deployment *clusterv1.MachineDeployment, msList []*clusterv1.MachineSet) *clusterv1.MachineSet { - sort.Sort(MachineSetsByCreationTimestamp(msList)) +func FindNewMachineSet(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 + // Besides only considering MachineSets which have an equivalent MachineTemplateSpec, we choose the MachineSet + // which has the most recent RolloutAfter annotation set (if any) or as second criteria is the oldest one. + sort.Sort(MachineSetsByRolloutAfterAnnotationAndCreationTimestamp(msList)) + + // We deterministically choose the MachineSet which has the newest RolloutAfter annotation or for equal values + // is the newest one. for i := range msList { if EqualMachineTemplate(&msList[i].Spec.Template, &deployment.Spec.Template) { - // 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] + // If RolloutAfter is set and is in the past, only return a MachineSet which got created after RolloutAfter. + // Otherwise this fallbacks to returning nil which results in a new MachineSet and Rollout. + if deployment.Spec.RolloutAfter != nil && now.After(deployment.Spec.RolloutAfter.Time) { + created := msList[i].CreationTimestamp + if created.Before(deployment.Spec.RolloutAfter) { + continue + } + return msList[i], nil + } + + // If a rolloutAfter never took place or is some time in the future just pick the oldest one. + return msList[i], nil } } - // new MachineSet does not exist. - return 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 -func FindOldMachineSets(deployment *clusterv1.MachineDeployment, msList []*clusterv1.MachineSet) ([]*clusterv1.MachineSet, []*clusterv1.MachineSet) { +func FindOldMachineSets(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 := FindNewMachineSet(deployment, msList) + newMS, err := FindNewMachineSet(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 { @@ -428,7 +494,7 @@ func FindOldMachineSets(deployment *clusterv1.MachineDeployment, msList []*clust requiredMSs = append(requiredMSs, ms) } } - return requiredMSs, allMSs + return requiredMSs, allMSs, nil } // GetReplicaCountForMachineSets returns the sum of Replicas of the given machine sets. diff --git a/internal/controllers/machinedeployment/mdutil/util_test.go b/internal/controllers/machinedeployment/mdutil/util_test.go index 1b508d3951a8..01af60d2cf4d 100644 --- a/internal/controllers/machinedeployment/mdutil/util_test.go +++ b/internal/controllers/machinedeployment/mdutil/util_test.go @@ -34,6 +34,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{ @@ -269,23 +274,25 @@ func TestEqualMachineTemplate(t *testing.T) { func TestFindNewMachineSet(t *testing.T) { now := metav1.Now() - later := metav1.Time{Time: now.Add(time.Minute)} deployment := generateDeployment("nginx") - newMS := generateMS(deployment) - newMS.Labels[clusterv1.MachineDeploymentUniqueLabel] = "hash" - newMS.CreationTimestamp = later - newMSDup := generateMS(deployment) - newMSDup.Labels[clusterv1.MachineDeploymentUniqueLabel] = "different-hash" - newMSDup.CreationTimestamp = now + // For cases when a rolloutAfter is scheduled but not yet took place. + deploymentRolloutAfterFuture := *deployment.DeepCopy() + deploymentRolloutAfterFuture.Spec.RolloutAfter = &metav1.Time{Time: now.Add(1 * time.Minute)} - oldDeployment := generateDeployment("nginx") - oldMS := generateMS(oldDeployment) - oldMS.Spec.Template.Annotations = map[string]string{ - "old": "true", - } - oldMS.Status.FullyLabeledReplicas = *(oldMS.Spec.Replicas) + // For cases when a rolloutAfter should happen now + deploymentRolloutAfterNow := *deployment.DeepCopy() + deploymentRolloutAfterNow.Spec.RolloutAfter = &metav1.Time{Time: now.Add(-1 * time.Second)} + + // For cases when a rolloutAfter should happen now or has happened in the past + deploymentRolloutAfter4mAgo := *deployment.DeepCopy() + deploymentRolloutAfter4mAgo.Spec.RolloutAfter = &metav1.Time{Time: now.Add(-4 * time.Minute)} + + msBuilder := newMachineSetBuilder(deployment) + + notEqualMachineSpec := msBuilder.obj.Spec + notEqualMachineSpec.Template.Annotations = map[string]string{"this": "does-not-match"} tests := []struct { Name string @@ -294,22 +301,79 @@ func TestFindNewMachineSet(t *testing.T) { expected *clusterv1.MachineSet }{ { - Name: "Get new MachineSet with the same template as Deployment spec but different machine-template-hash value", + Name: "No RolloutAfter / Return nil because there is no existing MachineSet which has an equivalent template", deployment: deployment, - msList: []*clusterv1.MachineSet{&newMS, &oldMS}, - expected: &newMS, + msList: []*clusterv1.MachineSet{ + msBuilder.WithSpec(notEqualMachineSpec).Build(), + }, + expected: nil, }, { - Name: "Get the oldest new MachineSet when there are more than one MachineSet with the same template", + Name: "No RolloutAfter / Return oldest MachineSet which has an equal MachineTemplate when RolloutAfter is not set and no rollout annotations are set on MachineSets", deployment: deployment, - msList: []*clusterv1.MachineSet{&newMS, &oldMS, &newMSDup}, - expected: &newMSDup, + msList: []*clusterv1.MachineSet{ + msBuilder.WithSpec(notEqualMachineSpec).Build(), + msBuilder.WithCreationTimestamp(now.Add(-1 * time.Minute)).Build(), + msBuilder.WithCreationTimestamp(now.Add(-5 * time.Minute)).Build(), + }, + expected: msBuilder.WithCreationTimestamp(now.Add(-5 * time.Minute)).Build(), + }, + { + Name: "Future RolloutAfter / Return oldest MachineSet which has an equal MachineTemplate when RolloutAfter is not set and no rollout annotations are set on MachineSets", + deployment: deploymentRolloutAfterFuture, + msList: []*clusterv1.MachineSet{ + msBuilder.WithSpec(notEqualMachineSpec).Build(), + msBuilder.WithCreationTimestamp(now.Add(-1 * time.Minute)).Build(), + msBuilder.WithCreationTimestamp(now.Add(-4 * time.Minute)).Build(), + msBuilder.WithCreationTimestamp(now.Add(-5 * time.Minute)).Build(), + }, + expected: msBuilder.WithCreationTimestamp(now.Add(-5 * time.Minute)).Build(), + }, + { + Name: "Future RolloutAfter / Return MachineSet with oldest rollout annotation which has an equal MachineTemplate when future RolloutAfter is set", + deployment: deploymentRolloutAfterFuture, + msList: []*clusterv1.MachineSet{ + msBuilder.WithSpec(notEqualMachineSpec).Build(), + msBuilder.WithCreationTimestamp(now.Add(-1 * time.Minute)).WithLastRolloutAfterAnnotation(now.Add(-4 * time.Minute)).Build(), + msBuilder.WithCreationTimestamp(now.Add(-4 * time.Minute)).WithLastRolloutAfterAnnotation(now.Add(-4 * time.Minute)).Build(), + msBuilder.WithCreationTimestamp(now.Add(-5 * time.Minute)).Build(), + }, + expected: msBuilder.WithCreationTimestamp(now.Add(-4 * time.Minute)).WithLastRolloutAfterAnnotation(now.Add(-4 * time.Minute)).Build(), }, { - Name: "Get nil new MachineSet", + Name: "Past RolloutAfter, but not set anymore / Return MachineSet with newest rollout annotation which has an equal MachineTemplate when RolloutAfter is not set", deployment: deployment, - msList: []*clusterv1.MachineSet{&oldMS}, - expected: nil, + msList: []*clusterv1.MachineSet{ + msBuilder.WithSpec(notEqualMachineSpec).Build(), + msBuilder.WithCreationTimestamp(now.Add(-1 * time.Minute)).WithLastRolloutAfterAnnotation(now.Add(-4 * time.Minute)).Build(), + msBuilder.WithCreationTimestamp(now.Add(-5 * time.Minute)).WithLastRolloutAfterAnnotation(now.Add(-5 * time.Minute)).Build(), + msBuilder.WithCreationTimestamp(now.Add(-4 * time.Minute)).WithLastRolloutAfterAnnotation(now.Add(-4 * time.Minute)).Build(), + msBuilder.WithCreationTimestamp(now.Add(-5 * time.Minute)).Build(), + }, + expected: msBuilder.WithCreationTimestamp(now.Add(-4 * time.Minute)).WithLastRolloutAfterAnnotation(now.Add(-4 * time.Minute)).Build(), + }, + { + Name: "Past RolloutAfter / Return MachineSet with newest rollout annotation which has an equal MachineTemplate", + deployment: deploymentRolloutAfter4mAgo, + msList: []*clusterv1.MachineSet{ + msBuilder.WithSpec(notEqualMachineSpec).Build(), + msBuilder.WithCreationTimestamp(now.Add(-1 * time.Minute)).WithLastRolloutAfterAnnotation(now.Add(-4 * time.Minute)).Build(), + msBuilder.WithCreationTimestamp(now.Add(-5 * time.Minute)).WithLastRolloutAfterAnnotation(now.Add(-5 * time.Minute)).Build(), + msBuilder.WithCreationTimestamp(now.Add(-4 * time.Minute)).WithLastRolloutAfterAnnotation(now.Add(-4 * time.Minute)).Build(), + msBuilder.WithCreationTimestamp(now.Add(-5 * time.Minute)).Build(), + }, + expected: msBuilder.WithCreationTimestamp(now.Add(-4 * time.Minute)).WithLastRolloutAfterAnnotation(now.Add(-4 * time.Minute)).Build(), + }, + { + Name: "RolloutAfter now / Return nil because there is no existing MachineSet which created after RolloutAfter and has an equivalent template", + deployment: deploymentRolloutAfterNow, + msList: []*clusterv1.MachineSet{ + msBuilder.WithSpec(notEqualMachineSpec).Build(), + msBuilder.WithCreationTimestamp(now.Add(-1 * time.Minute)).WithLastRolloutAfterAnnotation(now.Add(-4 * time.Minute)).Build(), + msBuilder.WithCreationTimestamp(now.Add(-4 * time.Minute)).WithLastRolloutAfterAnnotation(now.Add(-4 * time.Minute)).Build(), + msBuilder.WithCreationTimestamp(now.Add(-5 * time.Minute)).Build(), + }, + expected: nil, }, } @@ -317,7 +381,8 @@ func TestFindNewMachineSet(t *testing.T) { t.Run(test.Name, func(t *testing.T) { g := NewWithT(t) - ms := FindNewMachineSet(&test.deployment, test.msList) + ms, err := FindNewMachineSet(&now, &test.deployment, test.msList) + g.Expect(err).ToNot(HaveOccurred()) g.Expect(ms).To(Equal(test.expected)) }) } @@ -331,11 +396,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") @@ -401,7 +466,8 @@ func TestFindOldMachineSets(t *testing.T) { t.Run(test.Name, func(t *testing.T) { g := NewWithT(t) - requireMS, allMS := FindOldMachineSets(&test.deployment, test.msList) + requireMS, allMS, err := FindOldMachineSets(&now, &test.deployment, test.msList) + g.Expect(err).ToNot(HaveOccurred()) g.Expect(allMS).To(ConsistOf(test.expected)) // MSs are getting filtered correctly by ms.spec.replicas g.Expect(requireMS).To(ConsistOf(test.expectedRequire)) @@ -720,6 +786,8 @@ func TestAnnotationUtils(t *testing.T) { tDeployment.Annotations[clusterv1.RevisionAnnotation] = "999" logger := klogr.New() + now := metav1.Now() + // Test Case 1: Check if anotations are copied properly from deployment to MS t.Run("SetNewMachineSetAnnotations", func(t *testing.T) { g := NewWithT(t) @@ -727,7 +795,7 @@ func TestAnnotationUtils(t *testing.T) { // Try to set the increment revision from 1 through 20 for i := 0; i < 20; i++ { nextRevision := fmt.Sprintf("%d", i+1) - SetNewMachineSetAnnotations(&tDeployment, &tMS, nextRevision, true, logger) + SetNewMachineSetAnnotations(&now, &tDeployment, &tMS, nextRevision, true, logger) // Now the MachineSets Revision Annotation should be i+1 g.Expect(tMS.Annotations).To(HaveKeyWithValue(clusterv1.RevisionAnnotation, nextRevision)) } @@ -826,3 +894,41 @@ func TestReplicasAnnotationsNeedUpdate(t *testing.T) { }) } } + +type machineSetBuilder struct { + obj *clusterv1.MachineSet +} + +func newMachineSetBuilder(md clusterv1.MachineDeployment) *machineSetBuilder { + ms := generateMS(md) + return &machineSetBuilder{obj: &ms} +} + +func (b *machineSetBuilder) Build() *clusterv1.MachineSet { + return b.obj.DeepCopy() +} + +func (b *machineSetBuilder) WithAnnotation(k, v string) *machineSetBuilder { + newB := b.obj.DeepCopy() + if newB.Annotations == nil { + newB.Annotations = map[string]string{} + } + newB.Annotations[k] = v + return &machineSetBuilder{newB} +} + +func (b *machineSetBuilder) WithLastRolloutAfterAnnotation(t time.Time) *machineSetBuilder { + return b.WithAnnotation(clusterv1.MachineSetLastRolloutAfterAnnotation, t.UTC().Format(time.RFC3339)) +} + +func (b *machineSetBuilder) WithSpec(spec clusterv1.MachineSetSpec) *machineSetBuilder { + newB := b.obj.DeepCopy() + newB.Spec = spec + return &machineSetBuilder{newB} +} + +func (b *machineSetBuilder) WithCreationTimestamp(t time.Time) *machineSetBuilder { + newB := b.obj.DeepCopy() + newB.ObjectMeta.CreationTimestamp = metav1.NewTime(t) + return &machineSetBuilder{newB} +}