MD: improve replica defaulting for autoscaler

Signed-off-by: Stefan Büringer buringerst@vmware.com

api/v1beta1/machinedeployment_types.go

@@ -44,6 +44,10 @@ const (
 	// RevisionHistoryAnnotation maintains the history of all old revisions that a machine set has served for a machine deployment.
 	RevisionHistoryAnnotation = "machinedeployment.clusters.x-k8s.io/revision-history"
 
+	// DefaultReplicasAnnotation can be used to overwrite the default value of the MachineDeployment replicas field.
+	// If the annotation is set, its value will be used as default value.
+	DefaultReplicasAnnotation = "machinedeployment.cluster.x-k8s.io/default-replicas"
+
 	// DesiredReplicasAnnotation is the desired replicas for a machine deployment recorded as an annotation
 	// in its machine sets. Helps in separating scaling events from the rollout process and for
 	// determining if the new machine set for a deployment is really saturated.
@@ -67,10 +71,12 @@ type MachineDeploymentSpec struct {
 	// +kubebuilder:validation:MinLength=1
 	ClusterName string `json:"clusterName"`
 
-	// Number of desired machines. Defaults to 1.
+	// Number of desired machines.
 	// This is a pointer to distinguish between explicit zero and not specified.
+	//
+	// Defaults to: (in this order)
+	// TODO: describe logic based on code.
 	// +optional
-	// +kubebuilder:default=1
 	Replicas *int32 `json:"replicas,omitempty"`
 
 	// Label selector for machines. Existing MachineSets whose machines are

api/v1beta1/machinedeployment_webhook.go

@@ -17,9 +17,13 @@ limitations under the License.
 package v1beta1
 
 import (
+	"context"
 	"fmt"
+	"strconv"
 	"strings"
 
+	"github.com/pkg/errors"
+	v1 "k8s.io/api/admission/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/labels"
@@ -30,30 +34,75 @@ import (
 	"k8s.io/utils/pointer"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/webhook"
+	"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
 
 	"sigs.k8s.io/cluster-api/util/version"
 )
 
 func (m *MachineDeployment) SetupWebhookWithManager(mgr ctrl.Manager) error {
+	// This registers MachineDeployment as a validating and
+	// machineDeploymentDefaulter as a defaulting webhook.
 	return ctrl.NewWebhookManagedBy(mgr).
 		For(m).
+		WithDefaulter(MachineDeploymentDefaulter(mgr.GetScheme())).
 		Complete()
 }
 
 // +kubebuilder:webhook:verbs=create;update,path=/validate-cluster-x-k8s-io-v1beta1-machinedeployment,mutating=false,failurePolicy=fail,matchPolicy=Equivalent,groups=cluster.x-k8s.io,resources=machinedeployments,versions=v1beta1,name=validation.machinedeployment.cluster.x-k8s.io,sideEffects=None,admissionReviewVersions=v1;v1beta1
 // +kubebuilder:webhook:verbs=create;update,path=/mutate-cluster-x-k8s-io-v1beta1-machinedeployment,mutating=true,failurePolicy=fail,matchPolicy=Equivalent,groups=cluster.x-k8s.io,resources=machinedeployments,versions=v1beta1,name=default.machinedeployment.cluster.x-k8s.io,sideEffects=None,admissionReviewVersions=v1;v1beta1
 
-var _ webhook.Defaulter = &MachineDeployment{}
+var _ webhook.CustomDefaulter = &machineDeploymentDefaulter{}
 var _ webhook.Validator = &MachineDeployment{}
 
-// Default implements webhook.Defaulter so a webhook will be registered for the type.
-func (m *MachineDeployment) Default() {
+// MachineDeploymentDefaulter creates a new CustomDefaulter for MachineDeployments.
+func MachineDeploymentDefaulter(scheme *runtime.Scheme) webhook.CustomDefaulter {
+	// Note: The error return parameter is always nil and will be dropped with the next CR release.
+	decoder, _ := admission.NewDecoder(scheme)
+	return &machineDeploymentDefaulter{
+		decoder: decoder,
+	}
+}
+
+// machineDeploymentDefaulter implements a defaulting webhook for MachineDeployment.
+type machineDeploymentDefaulter struct {
+	decoder *admission.Decoder
+}
+
+// Default implements webhook.CustomDefaulter.
+func (webhook *machineDeploymentDefaulter) Default(ctx context.Context, obj runtime.Object) error {
+	m, ok := obj.(*MachineDeployment)
+	if !ok {
+		return apierrors.NewBadRequest(fmt.Sprintf("expected a MachineDeployment but got a %T", obj))
+	}
+
+	req, err := admission.RequestFromContext(ctx)
+	if err != nil {
+		return err
+	}
+
+	var oldMD *MachineDeployment
+	if req.Operation == v1.Update {
+		oldMD = &MachineDeployment{}
+		if err := webhook.decoder.DecodeRaw(req.OldObject, oldMD); err != nil {
+			return errors.Wrapf(err, "failed to decode oldObject to MachineDeployment")
+		}
+	}
+
 	PopulateDefaultsMachineDeployment(m)
+
+	replicas, err := calculateMachineDeploymentReplicas(oldMD, m)
+	if err != nil {
+		return err
+	}
+	m.Spec.Replicas = pointer.Int32(replicas)
+
 	// tolerate version strings without a "v" prefix: prepend it if it's not there
 	if m.Spec.Template.Spec.Version != nil && !strings.HasPrefix(*m.Spec.Template.Spec.Version, "v") {
 		normalizedVersion := "v" + *m.Spec.Template.Spec.Version
 		m.Spec.Template.Spec.Version = &normalizedVersion
 	}
+
+	return nil
 }
 
 // ValidateCreate implements webhook.Validator so a webhook will be registered for the type.
@@ -218,3 +267,68 @@ func PopulateDefaultsMachineDeployment(d *MachineDeployment) {
 	d.Spec.Selector.MatchLabels[ClusterNameLabel] = d.Spec.ClusterName
 	d.Spec.Template.Labels[ClusterNameLabel] = d.Spec.ClusterName
 }
+
+const (
+	autoscalerMinSize = "cluster.x-k8s.io/cluster-api-autoscaler-node-group-min-size"
+	autoscalerMaxSize = "cluster.x-k8s.io/cluster-api-autoscaler-node-group-max-size"
+)
+
+func calculateMachineDeploymentReplicas(oldMD *MachineDeployment, newMD *MachineDeployment) (int32, error) {
+	// If replicas is already set => Keep the current value.
+	if newMD.Spec.Replicas != nil {
+		return *newMD.Spec.Replicas, nil
+	}
+
+	// Default replica annotation is set => Use the annotation value.
+	if defaultReplicasString, ok := newMD.Annotations[DefaultReplicasAnnotation]; ok {
+		defaultReplicas, err := strconv.ParseInt(defaultReplicasString, 10, 32)
+		if err != nil {
+			return 0, errors.Wrapf(err, "could not parse the value of the %q annotation", DefaultReplicasAnnotation)
+		}
+		return int32(defaultReplicas), nil
+	}
+
+	// If both autoscaler annotations are set, use them to calculate the default value.
+	// FIXME: The adjustment to min/max only works when replicas is initially dropped, not later
+	// as then newMD.Spec.Replicas already has a value, the topology controller doesn't care abut
+	// the field anymore
+	minSizeString, hasMinSizeAnnotation := newMD.Annotations[autoscalerMinSize]
+	maxSizeString, hasMaxSizeAnnotation := newMD.Annotations[autoscalerMaxSize]
+	if hasMinSizeAnnotation && hasMaxSizeAnnotation {
+		minSize, err := strconv.ParseInt(minSizeString, 10, 32)
+		if err != nil {
+			return 0, errors.Wrapf(err, "could not parse the value of the %q annotation", autoscalerMinSize)
+		}
+		maxSize, err := strconv.ParseInt(maxSizeString, 10, 32)
+		if err != nil {
+			return 0, errors.Wrapf(err, "could not parse the value of the %q annotation", autoscalerMaxSize)
+		}
+
+		// If it's a new MachineDeployment => Use the min size.
+		// Note: This will result in a scale up to get into the range where autoscaler takes over.
+		if oldMD == nil {
+			return int32(minSize), nil
+		}
+
+		switch {
+		// If the old MachineDeployment doesn't have replicas set => Use the min size.
+		// Note: As defaulting always sets the replica field, this case should not be possible.
+		case oldMD.Spec.Replicas == nil:
+			return int32(minSize), nil
+		// If the old MachineDeployment replicas are lower than min size => Use the min size.
+		// Note: This will result in a scale up to get into the range where autoscaler takes over.
+		case *oldMD.Spec.Replicas < int32(minSize):
+			return int32(minSize), nil
+		// If the old MachineDeployment replicas are higher than max size => Use the max size.
+		// Note: This will result in a scale down to get into the range where autoscaler takes over.
+		case *oldMD.Spec.Replicas > int32(maxSize):
+			return int32(maxSize), nil
+		// If the old MachineDeployment replicas are between min and max size => Keep the current value.
+		default:
+			return *oldMD.Spec.Replicas, nil
+		}
+	}
+
+	// If neither the default nor the autoscaler annotations are set => Default to 1.
+	return 1, nil
+}

api/v1beta1/machinedeployment_webhook_test.go

@@ -17,14 +17,15 @@ limitations under the License.
 package v1beta1
 
 import (
+	"context"
 	"testing"
 
 	. "github.com/onsi/gomega"
+	admissionv1 "k8s.io/api/admission/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/intstr"
 	"k8s.io/utils/pointer"
-
-	utildefaulting "sigs.k8s.io/cluster-api/util/defaulting"
+	"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
 )
 
 func TestMachineDeploymentDefault(t *testing.T) {
@@ -41,8 +42,17 @@ func TestMachineDeploymentDefault(t *testing.T) {
 			},
 		},
 	}
-	t.Run("for MachineDeployment", utildefaulting.DefaultValidateTest(md))
-	md.Default()
+
+	scheme, _ := SchemeBuilder.Build()
+	defaulter := MachineDeploymentDefaulter(scheme)
+
+	// t.Run("for MachineDeployment", utildefaulting.DefaultValidateTest(md)) FIXME
+	ctx := admission.NewContextWithRequest(context.Background(), admission.Request{
+		AdmissionRequest: admissionv1.AdmissionRequest{
+			Operation: admissionv1.Create,
+		},
+	})
+	g.Expect(defaulter.Default(ctx, md)).To(Succeed())
 
 	g.Expect(md.Labels[ClusterNameLabel]).To(Equal(md.Spec.ClusterName))
 	g.Expect(md.Spec.MinReadySeconds).To(Equal(pointer.Int32(0)))
@@ -281,7 +291,7 @@ func TestMachineDeploymentVersionValidation(t *testing.T) {
 
 func TestMachineDeploymentWithSpec(t *testing.T) {
 	g := NewWithT(t)
-	md := MachineDeployment{
+	md := &MachineDeployment{
 		Spec: MachineDeploymentSpec{
 			ClusterName: "test-cluster",
 			Template: MachineTemplateSpec{
@@ -292,7 +302,16 @@ func TestMachineDeploymentWithSpec(t *testing.T) {
 		},
 	}
 
-	md.Default()
+	scheme, _ := SchemeBuilder.Build()
+	defaulter := MachineDeploymentDefaulter(scheme)
+
+	ctx := admission.NewContextWithRequest(context.Background(), admission.Request{
+		AdmissionRequest: admissionv1.AdmissionRequest{
+			Operation: admissionv1.Create,
+		},
+	})
+	g.Expect(defaulter.Default(ctx, md)).To(Succeed())
+
 	g.Expect(md.Spec.Selector.MatchLabels).To(HaveKeyWithValue(ClusterNameLabel, "test-cluster"))
 	g.Expect(md.Spec.Template.Labels).To(HaveKeyWithValue(ClusterNameLabel, "test-cluster"))
 }

internal/test/builder/builders.go

@@ -17,14 +17,17 @@ limitations under the License.
 package builder
 
 import (
+	"context"
 	"fmt"
 	"strings"
 
+	admissionv1 "k8s.io/api/admission/v1"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/util/intstr"
 	"sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
 
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
 	expv1 "sigs.k8s.io/cluster-api/exp/api/v1beta1"
@@ -1244,7 +1247,13 @@ func (m *MachineDeploymentBuilder) Build() *clusterv1.MachineDeployment {
 		}
 	}
 	if m.defaulter {
-		obj.Default()
+		scheme, _ := clusterv1.SchemeBuilder.Build()
+		ctx := admission.NewContextWithRequest(context.Background(), admission.Request{
+			AdmissionRequest: admissionv1.AdmissionRequest{
+				Operation: admissionv1.Create,
+			},
+		})
+		_ = clusterv1.MachineDeploymentDefaulter(scheme).Default(ctx, obj)
 	}
 	return obj
 }