Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion api/v1alpha3/conversion.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand All @@ -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)

Expand Down
16 changes: 6 additions & 10 deletions api/v1alpha3/zz_generated.conversion.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

11 changes: 11 additions & 0 deletions api/v1alpha4/condition_consts.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)
14 changes: 14 additions & 0 deletions api/v1alpha4/machinedeployment_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
}
9 changes: 8 additions & 1 deletion api/v1alpha4/zz_generated.deepcopy.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

44 changes: 44 additions & 0 deletions config/crd/bases/cluster.x-k8s.io_machinedeployments.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
26 changes: 25 additions & 1 deletion controllers/machinedeployment_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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{}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see the := approach used far more frequently in the capi code base. If you don't mind, I will leave it as is.

if reterr == nil {
patchOpts = append(patchOpts, patch.WithStatusObservedGeneration{})
}
if err := patchMachineDeployment(ctx, patchHelper, deployment, patchOpts...); err != nil {
reterr = kerrors.NewAggregate([]error{reterr, err})
}
}()
Expand All @@ -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")
Expand Down
8 changes: 8 additions & 0 deletions controllers/machinedeployment_controller_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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{}
Expand Down Expand Up @@ -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(env.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))
})
Expand Down
12 changes: 12 additions & 0 deletions controllers/machinedeployment_sync.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

may be add a comment here that minReplicasNeeded will be equal to d.Spec.Replicas when the strategy is not RollingUpdateMachineDeploymentStrategyType

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the only other strategy I see implemented today is OnDeleteMachineDeploymentStrategyType. Would the comment become stale if we implement other strategies?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

	// Rolling update config params. Present only if
	// MachineDeploymentStrategyType = RollingUpdate.
	// +optional
	RollingUpdate *MachineRollingUpdateDeployment `json:"rollingUpdate,omitempty"`

As today maxUnavailable and MaxSurge are only applicable when // MachineDeploymentStrategyType = RollingUpdate.

I think reinforcing this would make it easier for new folks to ramp up and avoid confusion.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. Added the comment.

// 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
}

Expand Down Expand Up @@ -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 {
Expand Down
98 changes: 98 additions & 0 deletions controllers/machinedeployment_sync_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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...)
})
}
}