From 4702043f649a71962354f0bdd7c9e3cfdfeb342d Mon Sep 17 00:00:00 2001 From: ricky Date: Fri, 7 Apr 2023 21:40:42 +0800 Subject: [PATCH] feat: Optimize UnitedDeployment replicas settings Signed-off-by: ricky --- apis/apps/defaults/v1alpha1.go | 3 - pkg/controller/uniteddeployment/allocator.go | 38 +++++++----- .../uniteddeployment/allocator_test.go | 17 ++++++ .../uniteddeployment_controller_utils.go | 4 ++ .../validating/uniteddeployment_validation.go | 30 ++++++--- .../uniteddeployment_validation_test.go | 61 +++++++++++++++++-- 6 files changed, 121 insertions(+), 32 deletions(-) diff --git a/apis/apps/defaults/v1alpha1.go b/apis/apps/defaults/v1alpha1.go index 5b81d265fd..912d42faf1 100644 --- a/apis/apps/defaults/v1alpha1.go +++ b/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) } diff --git a/pkg/controller/uniteddeployment/allocator.go b/pkg/controller/uniteddeployment/allocator.go index 629f17c44e..0c0abea2d9 100644 --- a/pkg/controller/uniteddeployment/allocator.go +++ b/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", diff --git a/pkg/controller/uniteddeployment/allocator_test.go b/pkg/controller/uniteddeployment/allocator_test.go index 5997c19d5c..d1c2ba0abd 100644 --- a/pkg/controller/uniteddeployment/allocator_test.go +++ b/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) { diff --git a/pkg/controller/uniteddeployment/uniteddeployment_controller_utils.go b/pkg/controller/uniteddeployment/uniteddeployment_controller_utils.go index 6b08a646d0..e26dea22fc 100644 --- a/pkg/controller/uniteddeployment/uniteddeployment_controller_utils.go +++ b/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) diff --git a/pkg/webhook/uniteddeployment/validating/uniteddeployment_validation.go b/pkg/webhook/uniteddeployment/validating/uniteddeployment_validation.go index a114cc1c35..2de34fd4a0 100644 --- a/pkg/webhook/uniteddeployment/validating/uniteddeployment_validation.go +++ b/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, "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 } diff --git a/pkg/webhook/uniteddeployment/validating/uniteddeployment_validation_test.go b/pkg/webhook/uniteddeployment/validating/uniteddeployment_validation_test.go index 5e7a9e1333..d93e4c8d1f 100644 --- a/pkg/webhook/uniteddeployment/validating/uniteddeployment_validation_test.go +++ b/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