Merge pull request #3351 from whitewindmills/pp-helper

add placement helper

pkg/apis/policy/v1alpha1/propagation_helper.go

@@ -18,3 +18,13 @@ func (p *PropagationPolicy) ExplicitPriority() int32 {
 func (p *ClusterPropagationPolicy) ExplicitPriority() int32 {
 	return p.Spec.ExplicitPriority()
 }
+
+// ReplicaSchedulingType returns the replica assignment strategy which is
+// "Duplicated" or "Divided". Returns "Duplicated" if the replica strategy is nil.
+func (p *Placement) ReplicaSchedulingType() ReplicaSchedulingType {
+	if p.ReplicaScheduling == nil {
+		return ReplicaSchedulingTypeDuplicated
+	}
+
+	return p.ReplicaScheduling.ReplicaSchedulingType
+}

pkg/apis/policy/v1alpha1/propagation_helper_test.go

@@ -13,11 +13,11 @@ func TestPropagationPolicy_ExplicitPriority(t *testing.T) {
 		expectedPriority int32
 	}{
 		{
-			name:             "no priority declared should defaults to zero",
+			name:             "expected to be zero in pp if no priority declared",
 			expectedPriority: 0,
 		},
 		{
-			name:             "no priority declared should defaults to zero",
+			name:             "expected to be declared priority in pp",
 			declaredPriority: pointer.Int32(20),
 			expectedPriority: 20,
 		},
@@ -41,11 +41,11 @@ func TestClusterPropagationPolicy_ExplicitPriority(t *testing.T) {
 		expectedPriority int32
 	}{
 		{
-			name:             "no priority declared should defaults to zero",
+			name:             "expected to be zero in cpp if no priority declared",
 			expectedPriority: 0,
 		},
 		{
-			name:             "no priority declared should defaults to zero",
+			name:             "expected to be declared priority in cpp",
 			declaredPriority: pointer.Int32(20),
 			expectedPriority: 20,
 		},
@@ -61,3 +61,34 @@ func TestClusterPropagationPolicy_ExplicitPriority(t *testing.T) {
 		})
 	}
 }
+
+func TestPlacement_ReplicaSchedulingType(t *testing.T) {
+	var tests = []struct {
+		name                          string
+		declaredReplicaSchedulingType ReplicaSchedulingType
+		expectedReplicaSchedulingType ReplicaSchedulingType
+	}{
+		{
+			name:                          "no replica scheduling strategy declared",
+			expectedReplicaSchedulingType: ReplicaSchedulingTypeDuplicated,
+		},
+		{
+			name:                          "replica scheduling strategy is 'Divided'",
+			declaredReplicaSchedulingType: ReplicaSchedulingTypeDivided,
+			expectedReplicaSchedulingType: ReplicaSchedulingTypeDivided,
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			p := &Placement{}
+			if test.declaredReplicaSchedulingType != "" {
+				p.ReplicaScheduling = &ReplicaSchedulingStrategy{ReplicaSchedulingType: test.declaredReplicaSchedulingType}
+			}
+			got := p.ReplicaSchedulingType()
+			if test.expectedReplicaSchedulingType != got {
+				t.Fatalf("Expected：%s, but got: %s", test.expectedReplicaSchedulingType, got)
+			}
+		})
+	}
+}

pkg/descheduler/core/filter.go

@@ -45,5 +45,5 @@ func validatePlacement(binding *workv1alpha2.ResourceBinding) bool {
 	if placement == nil {
 		return false
 	}
-	return helper.IsReplicaDynamicDivided(placement.ReplicaScheduling)
+	return helper.IsReplicaDynamicDivided(placement)
 }

pkg/util/helper/policy.go

@@ -103,11 +103,12 @@ func GenerateResourceSelectorForServiceImport(svcImport policyv1alpha1.ResourceS
 }
 
 // IsReplicaDynamicDivided checks if a PropagationPolicy schedules replicas as dynamic.
-func IsReplicaDynamicDivided(strategy *policyv1alpha1.ReplicaSchedulingStrategy) bool {
-	if strategy == nil || strategy.ReplicaSchedulingType != policyv1alpha1.ReplicaSchedulingTypeDivided {
+func IsReplicaDynamicDivided(placement *policyv1alpha1.Placement) bool {
+	if placement.ReplicaSchedulingType() != policyv1alpha1.ReplicaSchedulingTypeDivided {
 		return false
 	}
 
+	strategy := placement.ReplicaScheduling
 	switch strategy.ReplicaDivisionPreference {
 	case policyv1alpha1.ReplicaDivisionPreferenceWeighted:
 		return strategy.WeightPreference != nil && len(strategy.WeightPreference.DynamicWeight) != 0
@@ -132,9 +133,9 @@ func GetAppliedPlacement(annotations map[string]string) (*policyv1alpha1.Placeme
 }
 
 // SetReplicaDivisionPreferenceWeighted Set the default value of ReplicaDivisionPreference to Weighted
-func SetReplicaDivisionPreferenceWeighted(strategy *policyv1alpha1.ReplicaSchedulingStrategy) {
-	if strategy == nil || strategy.ReplicaSchedulingType != policyv1alpha1.ReplicaSchedulingTypeDivided || strategy.ReplicaDivisionPreference != "" {
+func SetReplicaDivisionPreferenceWeighted(placement *policyv1alpha1.Placement) {
+	if placement.ReplicaSchedulingType() != policyv1alpha1.ReplicaSchedulingTypeDivided || placement.ReplicaScheduling.ReplicaDivisionPreference != "" {
 		return
 	}
-	strategy.ReplicaDivisionPreference = policyv1alpha1.ReplicaDivisionPreferenceWeighted
+	placement.ReplicaScheduling.ReplicaDivisionPreference = policyv1alpha1.ReplicaDivisionPreferenceWeighted
 }

pkg/util/helper/policy_test.go

@@ -315,7 +315,7 @@ func TestIsReplicaDynamicDivided(t *testing.T) {
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			res := IsReplicaDynamicDivided(tt.strategy)
+			res := IsReplicaDynamicDivided(&policyv1alpha1.Placement{ReplicaScheduling: tt.strategy})
 			if res != tt.expected {
 				t.Errorf("expected %v, but got %v", tt.expected, res)
 			}
@@ -361,3 +361,42 @@ func TestGetAppliedPlacement(t *testing.T) {
 		})
 	}
 }
+
+func TestSetReplicaDivisionPreferenceWeighted(t *testing.T) {
+	tests := []struct {
+		name             string
+		strategy         *policyv1alpha1.ReplicaSchedulingStrategy
+		expectedWeighted bool
+	}{
+		{
+			name:             "no replica scheduling strategy declared",
+			expectedWeighted: false,
+		},
+		{
+			name: "specified aggregated division preference",
+			strategy: &policyv1alpha1.ReplicaSchedulingStrategy{
+				ReplicaSchedulingType:     policyv1alpha1.ReplicaSchedulingTypeDivided,
+				ReplicaDivisionPreference: policyv1alpha1.ReplicaDivisionPreferenceAggregated,
+			},
+			expectedWeighted: false,
+		},
+		{
+			name: "unspecified replica division preference",
+			strategy: &policyv1alpha1.ReplicaSchedulingStrategy{
+				ReplicaSchedulingType: policyv1alpha1.ReplicaSchedulingTypeDivided,
+			},
+			expectedWeighted: true,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			p := &policyv1alpha1.Placement{ReplicaScheduling: tt.strategy}
+			SetReplicaDivisionPreferenceWeighted(p)
+			if (p.ReplicaScheduling != nil &&
+				p.ReplicaScheduling.ReplicaDivisionPreference == policyv1alpha1.ReplicaDivisionPreferenceWeighted) != tt.expectedWeighted {
+				t.Errorf("expectedWeighted %v, but got %v", tt.expectedWeighted, !tt.expectedWeighted)
+			}
+		})
+	}
+}

pkg/webhook/clusterpropagationpolicy/mutating.go

@@ -57,7 +57,7 @@ func (a *MutatingAdmission) Handle(ctx context.Context, req admission.Request) a
 	}
 
 	// When ReplicaSchedulingType is Divided, set the default value of ReplicaDivisionPreference to Weighted.
-	helper.SetReplicaDivisionPreferenceWeighted(policy.Spec.Placement.ReplicaScheduling)
+	helper.SetReplicaDivisionPreferenceWeighted(&policy.Spec.Placement)
 
 	marshaledBytes, err := json.Marshal(policy)
 	if err != nil {

pkg/webhook/propagationpolicy/mutating.go

@@ -69,7 +69,7 @@ func (a *MutatingAdmission) Handle(ctx context.Context, req admission.Request) a
 	}
 
 	// When ReplicaSchedulingType is Divided, set the default value of ReplicaDivisionPreference to Weighted.
-	helper.SetReplicaDivisionPreferenceWeighted(policy.Spec.Placement.ReplicaScheduling)
+	helper.SetReplicaDivisionPreferenceWeighted(&policy.Spec.Placement)
 
 	marshaledBytes, err := json.Marshal(policy)
 	if err != nil {