feat: Optimize UnitedDeployment replicas settings

Signed-off-by: ricky <yricky509@gmail.com>

apis/apps/defaults/v1alpha1.go

@@ -201,9 +201,6 @@ func SetDefaultsBroadcastJob(obj *v1alpha1.BroadcastJob, injectTemplateDefaults
 
 // SetDefaults_UnitedDeployment set default values for UnitedDeployment.
 func SetDefaultsUnitedDeployment(obj *v1alpha1.UnitedDeployment, injectTemplateDefaults bool) {
-	if obj.Spec.Replicas == nil {
-		obj.Spec.Replicas = utilpointer.Int32Ptr(1)
-	}
 	if obj.Spec.RevisionHistoryLimit == nil {
 		obj.Spec.RevisionHistoryLimit = utilpointer.Int32Ptr(10)
 	}

pkg/controller/uniteddeployment/allocator.go

@@ -59,10 +59,16 @@ func (n subsetInfos) Swap(i, j int) {
 // new replicas indicated from UnitedDeployment.Spec.Topology.Subsets.
 func GetAllocatedReplicas(nameToSubset *map[string]*Subset, ud *appsv1alpha1.UnitedDeployment) (*map[string]int32, error) {
 	subsetInfos := getSubsetInfos(nameToSubset, ud)
-	specifiedReplicas := getSpecifiedSubsetReplicas(ud)
 
+	var expectedReplicas int32 = -1
+	if ud.Spec.Replicas != nil {
+		expectedReplicas = *ud.Spec.Replicas
+	}
+
+	specifiedReplicas := getSpecifiedSubsetReplicas(expectedReplicas, ud)
+	klog.V(4).Infof("UnitedDeployment %s/%s specifiedReplicas: %v", ud.Namespace, ud.Name, specifiedReplicas)
 	// call SortToAllocator to sort all subset by subset.Replicas in order of increment
-	return subsetInfos.SortToAllocator().AllocateReplicas(*ud.Spec.Replicas, specifiedReplicas)
+	return subsetInfos.SortToAllocator().AllocateReplicas(expectedReplicas, specifiedReplicas)
 }
 
 func (n subsetInfos) SortToAllocator() *replicasAllocator {
@@ -84,27 +90,29 @@ func (s *replicasAllocator) validateReplicas(replicas int32, subsetReplicasLimit
 		specifiedReplicas += replicas
 	}
 
-	if specifiedReplicas > replicas {
-		return fmt.Errorf("specified subsets' replica (%d) is greater than UnitedDeployment replica (%d)",
-			specifiedReplicas, replicas)
-	} else if specifiedReplicas < replicas {
-		specifiedCount := 0
-		for _, subset := range *s.subsets {
-			if _, exist := (*subsetReplicasLimits)[subset.SubsetName]; exist {
-				specifiedCount++
-			}
+	specifiedCount := 0
+	for _, subset := range *s.subsets {
+		if _, exist := (*subsetReplicasLimits)[subset.SubsetName]; exist {
+			specifiedCount++
 		}
-
-		if specifiedCount == len(*s.subsets) {
+	}
+	klog.V(4).Infof("specifiedCount: %d, len(*s.subsets): %d", specifiedCount, len(*s.subsets))
+	if replicas != -1 {
+		if specifiedReplicas > replicas {
+			return fmt.Errorf("specified subsets' replica (%d) is greater than UnitedDeployment replica (%d)",
+				specifiedReplicas, replicas)
+		} else if specifiedReplicas < replicas && specifiedCount == len(*s.subsets) {
 			return fmt.Errorf("specified subsets' replica (%d) is less than UnitedDeployment replica (%d)",
 				specifiedReplicas, replicas)
 		}
+	} else if specifiedCount != len(*s.subsets) {
+		return fmt.Errorf("all subsets must be specified when UnitedDeployment replica is unspecified")
 	}
 
 	return nil
 }
 
-func getSpecifiedSubsetReplicas(ud *appsv1alpha1.UnitedDeployment) *map[string]int32 {
+func getSpecifiedSubsetReplicas(replicas int32, ud *appsv1alpha1.UnitedDeployment) *map[string]int32 {
 	replicaLimits := map[string]int32{}
 	if ud.Spec.Topology.Subsets == nil {
 		return &replicaLimits
@@ -115,7 +123,7 @@ func getSpecifiedSubsetReplicas(ud *appsv1alpha1.UnitedDeployment) *map[string]i
 			continue
 		}
 
-		if specifiedReplicas, err := ParseSubsetReplicas(*ud.Spec.Replicas, *subsetDef.Replicas); err == nil {
+		if specifiedReplicas, err := ParseSubsetReplicas(replicas, *subsetDef.Replicas); err == nil {
 			replicaLimits[subsetDef.Name] = specifiedReplicas
 		} else {
 			klog.Warningf("Fail to consider the replicas of subset %s when parsing replicaLimits during managing replicas of UnitedDeployment %s/%s: %s",

pkg/controller/uniteddeployment/allocator_test.go

@@ -167,6 +167,23 @@ func TestSpecifyValidReplicas(t *testing.T) {
 	if " t1 -> 1; t2 -> 2; t3 -> 3; t4 -> 4;" != allocator.String() {
 		t.Fatalf("unexpected %s", allocator)
 	}
+
+	infos = subsetInfos{
+		createSubset("t1", 4),
+		createSubset("t2", 2),
+		createSubset("t3", 2),
+		createSubset("t4", 2),
+	}
+	allocator = infos.SortToAllocator()
+	allocator.AllocateReplicas(-1, &map[string]int32{
+		"t1": 1,
+		"t2": 2,
+		"t3": 3,
+		"t4": 4,
+	})
+	if " t1 -> 1; t2 -> 2; t3 -> 3; t4 -> 4;" != allocator.String() {
+		t.Fatalf("unexpected %s", allocator)
+	}
 }
 
 func TestSpecifyInvalidReplicas(t *testing.T) {

pkg/controller/uniteddeployment/uniteddeployment_controller_utils.go

@@ -41,6 +41,10 @@ func ParseSubsetReplicas(udReplicas int32, subsetReplicas intstr.IntOrString) (i
 		return subsetReplicas.IntVal, nil
 	}
 
+	if udReplicas < 0 {
+		return 0, fmt.Errorf("subsetReplicas (%v) should not be string when unitedDeployment replicas is empty", subsetReplicas.StrVal)
+	}
+
 	strVal := subsetReplicas.StrVal
 	if !strings.HasSuffix(strVal, "%") {
 		return 0, fmt.Errorf("subset replicas (%s) only support integer value or percentage value with a suffix '%%'", strVal)

pkg/webhook/uniteddeployment/validating/uniteddeployment_validation.go

@@ -43,7 +43,9 @@ import (
 func validateUnitedDeploymentSpec(spec *appsv1alpha1.UnitedDeploymentSpec, fldPath *field.Path) field.ErrorList {
 	allErrs := field.ErrorList{}
 
-	allErrs = append(allErrs, apivalidation.ValidateNonnegativeField(int64(*spec.Replicas), fldPath.Child("replicas"))...)
+	if spec.Replicas != nil {
+		allErrs = append(allErrs, apivalidation.ValidateNonnegativeField(int64(*spec.Replicas), fldPath.Child("replicas"))...)
+	}
 	if spec.Selector == nil {
 		allErrs = append(allErrs, field.Required(fldPath.Child("selector"), ""))
 	} else {
@@ -61,12 +63,13 @@ func validateUnitedDeploymentSpec(spec *appsv1alpha1.UnitedDeploymentSpec, fldPa
 	}
 
 	var sumReplicas int32
-	var expectedReplicas int32 = 1
+	var expectedReplicas int32 = -1
 	if spec.Replicas != nil {
 		expectedReplicas = *spec.Replicas
 	}
-	subSetNames := sets.String{}
 	count := 0
+	subSetNames := sets.String{}
+
 	for i, subset := range spec.Topology.Subsets {
 		if len(subset.Name) == 0 {
 			allErrs = append(allErrs, field.Required(fldPath.Child("topology", "subsets").Index(i).Child("name"), ""))
@@ -115,13 +118,18 @@ func validateUnitedDeploymentSpec(spec *appsv1alpha1.UnitedDeploymentSpec, fldPa
 		}
 	}
 
-	// sum of subset replicas may be less than uniteddployment replicas
-	if sumReplicas > expectedReplicas {
-		allErrs = append(allErrs, field.Invalid(fldPath.Child("topology", "subsets"), sumReplicas, fmt.Sprintf("sum of indicated subset replicas %d should not be greater than UnitedDeployment replicas %d", sumReplicas, expectedReplicas)))
-	}
+	if expectedReplicas != -1 {
+		// sum of subset replicas may be less than uniteddployment replicas
+		if sumReplicas > expectedReplicas {
+			allErrs = append(allErrs, field.Invalid(fldPath.Child("topology", "subsets"), sumReplicas, fmt.Sprintf("sum of indicated subset replicas %d should not be greater than UnitedDeployment replicas %d", sumReplicas, expectedReplicas)))
+		}
 
-	if count > 0 && count == len(spec.Topology.Subsets) && sumReplicas != expectedReplicas {
-		allErrs = append(allErrs, field.Invalid(fldPath.Child("topology", "subsets"), sumReplicas, fmt.Sprintf("if replicas of all subsets are provided, the sum of indicated subset replicas %d should equal UnitedDeployment replicas %d", sumReplicas, expectedReplicas)))
+		if count > 0 && count == len(spec.Topology.Subsets) && sumReplicas != expectedReplicas {
+			allErrs = append(allErrs, field.Invalid(fldPath.Child("topology", "subsets"), sumReplicas, fmt.Sprintf("if replicas of all subsets are provided, the sum of indicated subset replicas %d should equal UnitedDeployment replicas %d", sumReplicas, expectedReplicas)))
+		}
+	} else if count != len(spec.Topology.Subsets) {
+		// validate all of subsets replicas are not nil
+		allErrs = append(allErrs, field.Invalid(fldPath.Child("topology", "subsets"), sumReplicas, fmt.Sprintf("if UnitedDeployment replicas is not provided, replicas of all subsets should be provided")))
 	}
 
 	if spec.UpdateStrategy.ManualUpdate != nil {
@@ -146,7 +154,9 @@ func validateUnitedDeployment(unitedDeployment *appsv1alpha1.UnitedDeployment) f
 func ValidateUnitedDeploymentUpdate(unitedDeployment, oldUnitedDeployment *appsv1alpha1.UnitedDeployment) field.ErrorList {
 	allErrs := apivalidation.ValidateObjectMetaUpdate(&unitedDeployment.ObjectMeta, &oldUnitedDeployment.ObjectMeta, field.NewPath("metadata"))
 	allErrs = append(allErrs, validateUnitedDeploymentSpecUpdate(&unitedDeployment.Spec, &oldUnitedDeployment.Spec, field.NewPath("spec"))...)
-	allErrs = append(allErrs, apivalidation.ValidateNonnegativeField(int64(*unitedDeployment.Spec.Replicas), field.NewPath("spec", "replicas"))...)
+	if unitedDeployment.Spec.Replicas != nil {
+		allErrs = append(allErrs, apivalidation.ValidateNonnegativeField(int64(*unitedDeployment.Spec.Replicas), field.NewPath("spec", "replicas"))...)
+	}
 	return allErrs
 }
 

pkg/webhook/uniteddeployment/validating/uniteddeployment_validation_test.go

@@ -51,6 +51,34 @@ func TestValidateUnitedDeployment(t *testing.T) {
 	replicas3 := intstr.FromString("71%")
 	replicas4 := intstr.FromString("29%")
 	successCases := []appsv1alpha1.UnitedDeployment{
+		{
+			ObjectMeta: metav1.ObjectMeta{Name: "abc", Namespace: metav1.NamespaceDefault},
+			Spec: appsv1alpha1.UnitedDeploymentSpec{
+				Selector: &metav1.LabelSelector{MatchLabels: validLabels},
+				Template: appsv1alpha1.SubsetTemplate{
+					StatefulSetTemplate: &appsv1alpha1.StatefulSetTemplateSpec{
+						ObjectMeta: metav1.ObjectMeta{
+							Labels: validLabels,
+						},
+						Spec: apps.StatefulSetSpec{
+							Template: validPodTemplate.Template,
+						},
+					},
+				},
+				Topology: appsv1alpha1.Topology{
+					Subsets: []appsv1alpha1.Subset{
+						{
+							Name:     "subset1",
+							Replicas: &replicas1,
+						},
+						{
+							Name:     "subset2",
+							Replicas: &replicas1,
+						},
+					},
+				},
+			},
+		},
 		{
 			ObjectMeta: metav1.ObjectMeta{Name: "abc", Namespace: metav1.NamespaceDefault},
 			Spec: appsv1alpha1.UnitedDeploymentSpec{
@@ -424,6 +452,34 @@ func TestValidateUnitedDeployment(t *testing.T) {
 				},
 			},
 		},
+		"subset replicas type is percent when spec replicas not set": {
+			ObjectMeta: metav1.ObjectMeta{Name: "abc", Namespace: metav1.NamespaceDefault},
+			Spec: appsv1alpha1.UnitedDeploymentSpec{
+				Selector: &metav1.LabelSelector{MatchLabels: validLabels},
+				Template: appsv1alpha1.SubsetTemplate{
+					StatefulSetTemplate: &appsv1alpha1.StatefulSetTemplateSpec{
+						ObjectMeta: metav1.ObjectMeta{
+							Labels: validLabels,
+						},
+						Spec: apps.StatefulSetSpec{
+							Template: validPodTemplate.Template,
+						},
+					},
+				},
+				Topology: appsv1alpha1.Topology{
+					Subsets: []appsv1alpha1.Subset{
+						{
+							Name:     "subset1",
+							Replicas: &replicas2,
+						},
+						{
+							Name:     "subset2",
+							Replicas: &replicas1,
+						},
+					},
+				},
+			},
+		},
 		"partition not exist": {
 			ObjectMeta: metav1.ObjectMeta{Name: "abc", Namespace: metav1.NamespaceDefault},
 			Spec: appsv1alpha1.UnitedDeploymentSpec{
@@ -583,6 +639,7 @@ func TestValidateUnitedDeployment(t *testing.T) {
 					field != "spec.topology.subsets" &&
 					field != "spec.topology.subsets[0]" &&
 					field != "spec.topology.subsets[0].name" &&
+					field != "spec.topology.subsets[0].replicas" &&
 					field != "spec.updateStrategy.partitions" &&
 					field != "spec.topology.subsets[0].nodeSelectorTerm.matchExpressions[0].values" {
 					t.Errorf("%s: missing prefix for: %v", k, errs[i])
@@ -974,10 +1031,6 @@ func TestValidateUnitedDeploymentUpdate(t *testing.T) {
 }
 
 func setTestDefault(obj *appsv1alpha1.UnitedDeployment) {
-	if obj.Spec.Replicas == nil {
-		obj.Spec.Replicas = new(int32)
-		*obj.Spec.Replicas = 1
-	}
 	if obj.Spec.RevisionHistoryLimit == nil {
 		obj.Spec.RevisionHistoryLimit = new(int32)
 		*obj.Spec.RevisionHistoryLimit = 10