diff --git a/api/v1alpha3/conversion.go b/api/v1alpha3/conversion.go index 62f251e2e113..314da3016ccc 100644 --- a/api/v1alpha3/conversion.go +++ b/api/v1alpha3/conversion.go @@ -137,9 +137,9 @@ func (src *MachineDeployment) ConvertTo(dstRaw conversion.Hub) error { dst.Spec.Strategy.RollingUpdate = &v1alpha4.MachineRollingUpdateDeployment{} } dst.Spec.Strategy.RollingUpdate.DeletePolicy = restored.Spec.Strategy.RollingUpdate.DeletePolicy - } + dst.Status.Conditions = restored.Status.Conditions return nil } @@ -158,6 +158,11 @@ func (dst *MachineDeployment) ConvertFrom(srcRaw conversion.Hub) error { return nil } +// 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 +func Convert_v1alpha4_MachineDeploymentStatus_To_v1alpha3_MachineDeploymentStatus(in *v1alpha4.MachineDeploymentStatus, out *MachineDeploymentStatus, s apiconversion.Scope) error { + return autoConvert_v1alpha4_MachineDeploymentStatus_To_v1alpha3_MachineDeploymentStatus(in, out, s) +} + func (src *MachineDeploymentList) ConvertTo(dstRaw conversion.Hub) error { dst := dstRaw.(*v1alpha4.MachineDeploymentList) diff --git a/api/v1alpha3/zz_generated.conversion.go b/api/v1alpha3/zz_generated.conversion.go index 5ea7422da402..e68a7e45950b 100644 --- a/api/v1alpha3/zz_generated.conversion.go +++ b/api/v1alpha3/zz_generated.conversion.go @@ -174,11 +174,6 @@ func RegisterConversions(s *runtime.Scheme) error { }); err != nil { return err } - if err := s.AddGeneratedConversionFunc((*v1alpha4.MachineDeploymentStatus)(nil), (*MachineDeploymentStatus)(nil), func(a, b interface{}, scope conversion.Scope) error { - return Convert_v1alpha4_MachineDeploymentStatus_To_v1alpha3_MachineDeploymentStatus(a.(*v1alpha4.MachineDeploymentStatus), b.(*MachineDeploymentStatus), scope) - }); err != nil { - return err - } if err := s.AddGeneratedConversionFunc((*MachineDeploymentStrategy)(nil), (*v1alpha4.MachineDeploymentStrategy)(nil), func(a, b interface{}, scope conversion.Scope) error { return Convert_v1alpha3_MachineDeploymentStrategy_To_v1alpha4_MachineDeploymentStrategy(a.(*MachineDeploymentStrategy), b.(*v1alpha4.MachineDeploymentStrategy), scope) }); err != nil { @@ -349,6 +344,11 @@ func RegisterConversions(s *runtime.Scheme) error { }); err != nil { return err } + if err := s.AddConversionFunc((*v1alpha4.MachineDeploymentStatus)(nil), (*MachineDeploymentStatus)(nil), func(a, b interface{}, scope conversion.Scope) error { + return Convert_v1alpha4_MachineDeploymentStatus_To_v1alpha3_MachineDeploymentStatus(a.(*v1alpha4.MachineDeploymentStatus), b.(*MachineDeploymentStatus), scope) + }); err != nil { + return err + } if err := s.AddConversionFunc((*v1alpha4.MachineHealthCheckSpec)(nil), (*MachineHealthCheckSpec)(nil), func(a, b interface{}, scope conversion.Scope) error { return Convert_v1alpha4_MachineHealthCheckSpec_To_v1alpha3_MachineHealthCheckSpec(a.(*v1alpha4.MachineHealthCheckSpec), b.(*MachineHealthCheckSpec), scope) }); err != nil { @@ -826,14 +826,10 @@ func autoConvert_v1alpha4_MachineDeploymentStatus_To_v1alpha3_MachineDeploymentS out.AvailableReplicas = in.AvailableReplicas out.UnavailableReplicas = in.UnavailableReplicas out.Phase = in.Phase + // WARNING: in.Conditions requires manual conversion: does not exist in peer-type return nil } -// Convert_v1alpha4_MachineDeploymentStatus_To_v1alpha3_MachineDeploymentStatus is an autogenerated conversion function. -func Convert_v1alpha4_MachineDeploymentStatus_To_v1alpha3_MachineDeploymentStatus(in *v1alpha4.MachineDeploymentStatus, out *MachineDeploymentStatus, s conversion.Scope) error { - return autoConvert_v1alpha4_MachineDeploymentStatus_To_v1alpha3_MachineDeploymentStatus(in, out, s) -} - func autoConvert_v1alpha3_MachineDeploymentStrategy_To_v1alpha4_MachineDeploymentStrategy(in *MachineDeploymentStrategy, out *v1alpha4.MachineDeploymentStrategy, s conversion.Scope) error { out.Type = v1alpha4.MachineDeploymentStrategyType(in.Type) if in.RollingUpdate != nil { diff --git a/api/v1alpha4/condition_consts.go b/api/v1alpha4/condition_consts.go index 55e610cb280d..bfd3db452f2f 100644 --- a/api/v1alpha4/condition_consts.go +++ b/api/v1alpha4/condition_consts.go @@ -201,3 +201,14 @@ const ( // from making any further remediations. TooManyUnhealthyReason = "TooManyUnhealthy" ) + +// Conditions and condition Reasons for MachineDeployments + +const ( + // MachineDeploymentAvailableCondition means the MachineDeployment is available, that is, at least the minimum available + // machines required (i.e. Spec.Replicas-MaxUnavailable when MachineDeploymentStrategyType = RollingUpdate) are up and running for at least minReadySeconds. + MachineDeploymentAvailableCondition ConditionType = "Available" + + // WaitingForAvailableMachinesReason (Severity=Warning) reflects the fact that the required minimum number of machines for a machinedeployment are not available. + WaitingForAvailableMachinesReason = "WaitingForAvailableMachines" +) diff --git a/api/v1alpha4/machinedeployment_types.go b/api/v1alpha4/machinedeployment_types.go index ada96c5397c8..8ea7696c7182 100644 --- a/api/v1alpha4/machinedeployment_types.go +++ b/api/v1alpha4/machinedeployment_types.go @@ -211,6 +211,10 @@ type MachineDeploymentStatus struct { // Phase represents the current phase of a MachineDeployment (ScalingUp, ScalingDown, Running, Failed, or Unknown). // +optional Phase string `json:"phase,omitempty"` + + // Conditions defines current service state of the MachineDeployment. + // +optional + Conditions Conditions `json:"conditions,omitempty"` } // ANCHOR_END: MachineDeploymentStatus @@ -287,3 +291,13 @@ type MachineDeploymentList struct { func init() { SchemeBuilder.Register(&MachineDeployment{}, &MachineDeploymentList{}) } + +// GetConditions returns the set of conditions for the machinedeployment. +func (m *MachineDeployment) GetConditions() Conditions { + return m.Status.Conditions +} + +// SetConditions updates the set of conditions on the machinedeployment. +func (m *MachineDeployment) SetConditions(conditions Conditions) { + m.Status.Conditions = conditions +} diff --git a/api/v1alpha4/zz_generated.deepcopy.go b/api/v1alpha4/zz_generated.deepcopy.go index eeaa36285509..dd1dd8e06906 100644 --- a/api/v1alpha4/zz_generated.deepcopy.go +++ b/api/v1alpha4/zz_generated.deepcopy.go @@ -374,7 +374,7 @@ func (in *MachineDeployment) DeepCopyInto(out *MachineDeployment) { out.TypeMeta = in.TypeMeta in.ObjectMeta.DeepCopyInto(&out.ObjectMeta) in.Spec.DeepCopyInto(&out.Spec) - out.Status = in.Status + in.Status.DeepCopyInto(&out.Status) } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new MachineDeployment. @@ -472,6 +472,13 @@ func (in *MachineDeploymentSpec) DeepCopy() *MachineDeploymentSpec { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *MachineDeploymentStatus) DeepCopyInto(out *MachineDeploymentStatus) { *out = *in + if in.Conditions != nil { + in, out := &in.Conditions, &out.Conditions + *out = make(Conditions, len(*in)) + for i := range *in { + (*in)[i].DeepCopyInto(&(*out)[i]) + } + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new MachineDeploymentStatus. diff --git a/config/crd/bases/cluster.x-k8s.io_machinedeployments.yaml b/config/crd/bases/cluster.x-k8s.io_machinedeployments.yaml index bd780492233d..b6be71ade91b 100644 --- a/config/crd/bases/cluster.x-k8s.io_machinedeployments.yaml +++ b/config/crd/bases/cluster.x-k8s.io_machinedeployments.yaml @@ -864,6 +864,50 @@ spec: minReadySeconds) targeted by this deployment. format: int32 type: integer + conditions: + description: Conditions defines current service state of the MachineDeployment. + items: + description: Condition defines an observation of a Cluster API resource + operational state. + properties: + lastTransitionTime: + description: Last time the condition transitioned from one status + to another. This should be when the underlying condition changed. + If that is not known, then using the time when the API field + changed is acceptable. + format: date-time + type: string + message: + description: A human readable message indicating details about + the transition. This field may be empty. + type: string + reason: + description: The reason for the condition's last transition + in CamelCase. The specific API may choose whether or not this + field is considered a guaranteed API. This field may not be + empty. + type: string + severity: + description: Severity provides an explicit classification of + Reason code, so the users or machines can immediately understand + the current situation and act accordingly. The Severity field + MUST be set only when Status=False. + type: string + status: + description: Status of the condition, one of True, False, Unknown. + type: string + type: + description: Type of condition in CamelCase or in foo.example.com/CamelCase. + Many .condition.type values are consistent across resources + like Available, but because arbitrary conditions can be useful + (see .node.status.conditions), the ability to deconflict is + important. + type: string + required: + - status + - type + type: object + type: array observedGeneration: description: The generation observed by the deployment controller. format: int64 diff --git a/controllers/machinedeployment_controller.go b/controllers/machinedeployment_controller.go index e32f5c19bfe1..f17334dc2365 100644 --- a/controllers/machinedeployment_controller.go +++ b/controllers/machinedeployment_controller.go @@ -31,6 +31,7 @@ import ( clusterv1 "sigs.k8s.io/cluster-api/api/v1alpha4" "sigs.k8s.io/cluster-api/util" "sigs.k8s.io/cluster-api/util/annotations" + "sigs.k8s.io/cluster-api/util/conditions" "sigs.k8s.io/cluster-api/util/patch" "sigs.k8s.io/cluster-api/util/predicates" ctrl "sigs.k8s.io/controller-runtime" @@ -129,7 +130,12 @@ func (r *MachineDeploymentReconciler) Reconcile(ctx context.Context, req ctrl.Re defer func() { // Always attempt to patch the object and status after each reconciliation. - if err := patchHelper.Patch(ctx, deployment); err != nil { + // Patch ObservedGeneration only if the reconciliation completed successfully + patchOpts := []patch.Option{} + if reterr == nil { + patchOpts = append(patchOpts, patch.WithStatusObservedGeneration{}) + } + if err := patchMachineDeployment(ctx, patchHelper, deployment, patchOpts...); err != nil { reterr = kerrors.NewAggregate([]error{reterr, err}) } }() @@ -148,6 +154,24 @@ func (r *MachineDeploymentReconciler) Reconcile(ctx context.Context, req ctrl.Re return result, err } +func patchMachineDeployment(ctx context.Context, patchHelper *patch.Helper, d *clusterv1.MachineDeployment, options ...patch.Option) error { + // Always update the readyCondition by summarizing the state of other conditions. + conditions.SetSummary(d, + conditions.WithConditions( + clusterv1.MachineDeploymentAvailableCondition, + ), + ) + + // Patch the object, ignoring conflicts on the conditions owned by this controller. + options = append(options, + patch.WithOwnedConditions{Conditions: []clusterv1.ConditionType{ + clusterv1.ReadyCondition, + clusterv1.MachineDeploymentAvailableCondition, + }}, + ) + return patchHelper.Patch(ctx, d, options...) +} + func (r *MachineDeploymentReconciler) reconcile(ctx context.Context, cluster *clusterv1.Cluster, d *clusterv1.MachineDeployment) (ctrl.Result, error) { log := ctrl.LoggerFrom(ctx) log.V(4).Info("Reconcile MachineDeployment") diff --git a/controllers/machinedeployment_controller_test.go b/controllers/machinedeployment_controller_test.go index 3e4b1118fdc5..c1abea630123 100644 --- a/controllers/machinedeployment_controller_test.go +++ b/controllers/machinedeployment_controller_test.go @@ -33,6 +33,7 @@ import ( clusterv1 "sigs.k8s.io/cluster-api/api/v1alpha4" "sigs.k8s.io/cluster-api/controllers/external" "sigs.k8s.io/cluster-api/util" + "sigs.k8s.io/cluster-api/util/conditions" ) var _ reconcile.Reconciler = &MachineDeploymentReconciler{} @@ -389,6 +390,13 @@ func TestMachineDeploymentReconciler(t *testing.T) { return len(machineSets.Items) }, timeout*5).Should(BeEquivalentTo(0)) + t.Log("Verifying MachineDeployment has correct Conditions") + g.Eventually(func() bool { + key := client.ObjectKey{Name: deployment.Name, Namespace: deployment.Namespace} + g.Expect(testEnv.Get(ctx, key, deployment)).To(Succeed()) + return conditions.IsTrue(deployment, clusterv1.MachineDeploymentAvailableCondition) + }, timeout).Should(BeTrue()) + // Validate that the controller set the cluster name label in selector. g.Expect(deployment.Status.Selector).To(ContainSubstring(testCluster.Name)) }) diff --git a/controllers/machinedeployment_sync.go b/controllers/machinedeployment_sync.go index 9efa003d0349..83693c0a94dd 100644 --- a/controllers/machinedeployment_sync.go +++ b/controllers/machinedeployment_sync.go @@ -32,6 +32,7 @@ import ( clusterv1 "sigs.k8s.io/cluster-api/api/v1alpha4" "sigs.k8s.io/cluster-api/controllers/mdutil" "sigs.k8s.io/cluster-api/util" + "sigs.k8s.io/cluster-api/util/conditions" "sigs.k8s.io/cluster-api/util/patch" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" @@ -353,6 +354,16 @@ func (r *MachineDeploymentReconciler) scale(ctx context.Context, deployment *clu // syncDeploymentStatus checks if the status is up-to-date and sync it if necessary. func (r *MachineDeploymentReconciler) syncDeploymentStatus(allMSs []*clusterv1.MachineSet, newMS *clusterv1.MachineSet, d *clusterv1.MachineDeployment) error { d.Status = calculateStatus(allMSs, newMS, d) + + // minReplicasNeeded will be equal to d.Spec.Replicas when the strategy is not RollingUpdateMachineDeploymentStrategyType. + minReplicasNeeded := *(d.Spec.Replicas) - mdutil.MaxUnavailable(*d) + + if d.Status.AvailableReplicas >= minReplicasNeeded { + // NOTE: The structure of calculateStatus() does not allow us to update the machinedeployment directly, we can only update the status obj it returns. Ideally, we should change calculateStatus() --> updateStatus() to be consistent with the rest of the code base, until then, we update conditions here. + conditions.MarkTrue(d, clusterv1.MachineDeploymentAvailableCondition) + } else { + conditions.MarkFalse(d, clusterv1.MachineDeploymentAvailableCondition, clusterv1.WaitingForAvailableMachinesReason, clusterv1.ConditionSeverityWarning, "Minimum availability requires %d replicas, current %d available", minReplicasNeeded, d.Status.AvailableReplicas) + } return nil } @@ -380,6 +391,7 @@ func calculateStatus(allMSs []*clusterv1.MachineSet, newMS *clusterv1.MachineSet ReadyReplicas: mdutil.GetReadyReplicaCountForMachineSets(allMSs), AvailableReplicas: availableReplicas, UnavailableReplicas: unavailableReplicas, + Conditions: deployment.Status.Conditions, } if *deployment.Spec.Replicas == status.ReadyReplicas { diff --git a/controllers/machinedeployment_sync_test.go b/controllers/machinedeployment_sync_test.go index f2890b34f089..7f97e355d1ed 100644 --- a/controllers/machinedeployment_sync_test.go +++ b/controllers/machinedeployment_sync_test.go @@ -23,6 +23,7 @@ import ( . "github.com/onsi/gomega" "github.com/pkg/errors" + corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/client-go/tools/record" "k8s.io/utils/pointer" @@ -394,3 +395,100 @@ func TestScaleMachineSet(t *testing.T) { }) } } + +func newTestMachineDeployment(pds *int32, replicas, statusReplicas, updatedReplicas, availableReplicas int32, conditions clusterv1.Conditions) *clusterv1.MachineDeployment { + d := &clusterv1.MachineDeployment{ + ObjectMeta: metav1.ObjectMeta{ + Name: "progress-test", + }, + Spec: clusterv1.MachineDeploymentSpec{ + ProgressDeadlineSeconds: pds, + Replicas: &replicas, + Strategy: &clusterv1.MachineDeploymentStrategy{ + Type: clusterv1.RollingUpdateMachineDeploymentStrategyType, + RollingUpdate: &clusterv1.MachineRollingUpdateDeployment{ + MaxUnavailable: intOrStrPtr(0), + MaxSurge: intOrStrPtr(1), + DeletePolicy: pointer.StringPtr("Oldest"), + }, + }, + }, + Status: clusterv1.MachineDeploymentStatus{ + Replicas: statusReplicas, + UpdatedReplicas: updatedReplicas, + AvailableReplicas: availableReplicas, + Conditions: conditions, + }, + } + return d +} + +// helper to create MS with given availableReplicas. +func newTestMachinesetWithReplicas(name string, specReplicas, statusReplicas, availableReplicas int32) *clusterv1.MachineSet { + return &clusterv1.MachineSet{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + CreationTimestamp: metav1.Time{}, + Namespace: metav1.NamespaceDefault, + }, + Spec: clusterv1.MachineSetSpec{ + Replicas: pointer.Int32Ptr(specReplicas), + }, + Status: clusterv1.MachineSetStatus{ + AvailableReplicas: availableReplicas, + Replicas: statusReplicas, + }, + } +} + +func TestSyncDeploymentStatus(t *testing.T) { + pds := int32(60) + tests := []struct { + name string + d *clusterv1.MachineDeployment + oldMachineSets []*clusterv1.MachineSet + newMachineSet *clusterv1.MachineSet + expectedConditions []*clusterv1.Condition + }{ + { + name: "Deployment not available: MachineDeploymentAvailableCondition should exist and be false", + d: newTestMachineDeployment(&pds, 3, 2, 2, 2, clusterv1.Conditions{}), + oldMachineSets: []*clusterv1.MachineSet{}, + newMachineSet: newTestMachinesetWithReplicas("foo", 3, 2, 2), + expectedConditions: []*clusterv1.Condition{ + { + Type: clusterv1.MachineDeploymentAvailableCondition, + Status: corev1.ConditionFalse, + Severity: clusterv1.ConditionSeverityWarning, + Reason: clusterv1.WaitingForAvailableMachinesReason, + }, + }, + }, + { + name: "Deployment Available: MachineDeploymentAvailableCondition should exist and be true", + d: newTestMachineDeployment(&pds, 3, 3, 3, 3, clusterv1.Conditions{}), + oldMachineSets: []*clusterv1.MachineSet{}, + newMachineSet: newTestMachinesetWithReplicas("foo", 3, 3, 3), + expectedConditions: []*clusterv1.Condition{ + { + Type: clusterv1.MachineDeploymentAvailableCondition, + Status: corev1.ConditionTrue, + }, + }, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + g := NewWithT(t) + r := &MachineDeploymentReconciler{ + Client: fake.NewClientBuilder().Build(), + recorder: record.NewFakeRecorder(32), + } + allMachineSets := append(test.oldMachineSets, test.newMachineSet) + err := r.syncDeploymentStatus(allMachineSets, test.newMachineSet, test.d) + g.Expect(err).ToNot(HaveOccurred()) + assertConditions(t, test.d, test.expectedConditions...) + }) + } +}