diff --git a/pkg/propertychecker/azure/checker.go b/pkg/propertychecker/azure/checker.go index d2e94e9be..c031beee6 100644 --- a/pkg/propertychecker/azure/checker.go +++ b/pkg/propertychecker/azure/checker.go @@ -27,6 +27,11 @@ const ( // The value is constrained to a reasonable upper bound of 200 for most production workloads. // Upper bound is enforced after adjusting for operator semantics. maxVMInstanceCapacity = 200 + + // minVMInstanceCapacity defines the minimum allowed VM instance capacity for SKU capacity requirements. + // The Azure vmSizeRecommender API requires capacity values greater than 0, so minimum is set to 1. + // Lower bound is enforced after adjusting for operator semantics. + minVMInstanceCapacity = 1 ) // PropertyChecker provides Azure-specific property validation for member clusters. @@ -48,8 +53,8 @@ func NewPropertyChecker(vmSizeRecommenderClient compute.AttributeBasedVMSizeReco // CheckIfMeetSKUCapacityRequirement validates whether a member cluster can meet the specified // SKU capacity requirement. It extracts the required SKU and capacity from the property selector -// requirement and checks to determine if the cluster's Azure subscription -// and location can provision the requested VM instances. +// requirement and checks whether the cluster's Azure subscription and location can provision +// the requested VM instances. // // The cluster must have both Azure location and subscription ID labels configured. // Returns true if the SKU capacity requirement can be met, false otherwise. @@ -75,14 +80,14 @@ func (s *PropertyChecker) CheckIfMeetSKUCapacityRequirement( } // Request VM size recommendations to validate SKU availability and capacity. - // The capacity is checked by ensuring the current allocatable capacity is greater than the requested capacity. + // The capacity is checked by ensuring the current allocatable capacity is greater than or equal to the requested capacity. klog.V(2).Infof("Checking SKU %s with capacity %d in cluster %s", sku, capacity, cluster.Name) request := &computev1.GenerateAttributeBasedRecommendationsRequest{ SubscriptionId: subID, Location: location, RegularPriorityProfile: &computev1.RegularPriorityProfile{ CapacityUnitType: computev1.CapacityUnitType_CAPACITY_UNIT_TYPE_VM_INSTANCE_COUNT, - TargetCapacity: capacity, // CurrentAllocatableCapacity > RequestedCapacity + TargetCapacity: capacity, // CurrentAllocatableCapacity >= RequestedCapacity }, ResourceProperties: &computev1.ResourceProperties{ VmAttributes: &computev1.VMAttributes{ @@ -116,9 +121,8 @@ func (s *PropertyChecker) CheckIfMeetSKUCapacityRequirement( // extractCapacityRequirements extracts the capacity value from a PropertySelectorRequirement. // This function is specifically designed for Azure SKU capacity properties that follow the pattern: // "kubernetes.azure.com/vm-sizes/{sku}/capacity" -// Returns the capacity if the requirement is valid; -// The capacity will be updated based on the configured operator as VMSizeRecommender API -// checks if the current allocatableCapacity > the requested capacity. +// Returns the capacity value adjusted for the operator semantics, as the VMSizeRecommender API +// checks whether current allocatable capacity is greater than or equal to the requested capacity. func extractCapacityRequirements(req placementv1beta1.PropertySelectorRequirement) (uint32, error) { if req.Operator != placementv1beta1.PropertySelectorGreaterThan && req.Operator != placementv1beta1.PropertySelectorGreaterThanOrEqualTo { return 0, fmt.Errorf("unsupported operator %q for SKU capacity property, only GreaterThan (Gt) and GreaterThanOrEqualTo (Ge) are supported", req.Operator) @@ -149,18 +153,24 @@ func validateCapacity(value string, operator placementv1beta1.PropertySelectorOp capacity := uint32(valueUint) // capacity is >= 0 since it's parsed as uint. - if operator == placementv1beta1.PropertySelectorGreaterThanOrEqualTo && capacity > 0 { - capacity -= 1 + // Adjust capacity for VMSizeRecommender API semantics. + // The API requires requested capacity >= 1. + // If operator is GreaterThan (Gt) , increment the requested value by 1. + // Otherwise, use the requested value as-is. + // These adjustments ensure the API interprets the request correctly + if operator == placementv1beta1.PropertySelectorGreaterThan { + capacity += 1 } // Validate against maximum allowed capacity (exceed maxVMInstanceCapacity). - if capacity >= maxVMInstanceCapacity { + if capacity > maxVMInstanceCapacity { return 0, fmt.Errorf("capacity value %d exceeds maximum allowed value of %d", capacity, maxVMInstanceCapacity) } - // A capacity of zero is only valid for GreaterThan operator. - if capacity == 0 && operator != placementv1beta1.PropertySelectorGreaterThan { - return 0, fmt.Errorf("capacity value cannot be zero for operator %q", operator) + // Ensure capacity meets minimum requirements (minVMInstanceCapacity) after operator adjustment. + // The VMSizeRecommender API requires capacity > 0. + if capacity < minVMInstanceCapacity { + return 0, fmt.Errorf("capacity value %d is below minimum allowed value of %d", capacity, minVMInstanceCapacity) } return capacity, nil diff --git a/pkg/propertychecker/azure/checker_test.go b/pkg/propertychecker/azure/checker_test.go index dfe80ddb4..749ff4ff9 100644 --- a/pkg/propertychecker/azure/checker_test.go +++ b/pkg/propertychecker/azure/checker_test.go @@ -33,14 +33,14 @@ func TestValidateCapacity(t *testing.T) { name: "valid capacity value for GreaterThan operator", value: "10", operator: placementv1beta1.PropertySelectorGreaterThan, - wantValue: 10, + wantValue: 11, wantError: false, }, { name: "valid capacity value for GreaterThanOrEqualTo operator", value: "50", operator: placementv1beta1.PropertySelectorGreaterThanOrEqualTo, - wantValue: 49, + wantValue: 50, wantError: false, }, { @@ -72,11 +72,11 @@ func TestValidateCapacity(t *testing.T) { errorSubstring: "invalid capacity value", }, { - name: "unsupported operator for capacity of zero", + name: "invalid value of zero for capacity", value: "0", operator: placementv1beta1.PropertySelectorGreaterThanOrEqualTo, wantError: true, - errorSubstring: "capacity value cannot be zero for operator", + errorSubstring: "is below minimum allowed value", }, { name: "capacity equal to max limit with GreaterThan operator", @@ -85,18 +85,11 @@ func TestValidateCapacity(t *testing.T) { wantError: true, errorSubstring: "exceeds maximum allowed value", }, - { - name: "supported operator with capacity of zero", - value: "0", - operator: placementv1beta1.PropertySelectorGreaterThan, - wantValue: 0, - wantError: false, - }, { name: "capacity equal to max limit with supported operator", value: "200", operator: placementv1beta1.PropertySelectorGreaterThanOrEqualTo, - wantValue: 199, + wantValue: 200, wantError: false, }, { @@ -107,12 +100,19 @@ func TestValidateCapacity(t *testing.T) { errorSubstring: "exceeds maximum allowed value", }, { - name: "capacity at minimum limit", - value: "1", + name: "capacity at minimum limit (Gt)", + value: "0", operator: placementv1beta1.PropertySelectorGreaterThan, wantValue: 1, wantError: false, }, + { + name: "capacity at minimum limit (Ge)", + value: "1", + operator: placementv1beta1.PropertySelectorGreaterThanOrEqualTo, + wantValue: 1, + wantError: false, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -158,7 +158,7 @@ func TestExtractCapacityRequirements(t *testing.T) { Values: []string{"4"}, }, wantSKU: "Standard_D4s_v3", - wantCapacityValue: 4, + wantCapacityValue: 5, wantError: false, }, { @@ -172,6 +172,17 @@ func TestExtractCapacityRequirements(t *testing.T) { wantError: true, errorSubstring: "exceeds maximum allowed value of 200", }, + { + name: "invalid Azure SKU capacity property below min limit (Ge)", + req: placementv1beta1.PropertySelectorRequirement{ + Name: fmt.Sprintf(azure.CapacityPerSKUPropertyTmpl, "Standard_B2ms"), + Operator: placementv1beta1.PropertySelectorGreaterThanOrEqualTo, + Values: []string{"0"}, + }, + wantSKU: "Standard_B2ms", + wantError: true, + errorSubstring: "is below minimum allowed value", + }, { name: "invalid Azure SKU capacity property with decimal", req: placementv1beta1.PropertySelectorRequirement{ @@ -281,7 +292,7 @@ func TestExtractCapacityRequirements(t *testing.T) { } func TestCheckIfMeetSKUCapacityRequirement(t *testing.T) { - // Prepare test data + // Prepare test data. validSKU := "Standard_D2s_v3" validPropertySelectorRequirement := placementv1beta1.PropertySelectorRequirement{ Name: fmt.Sprintf(azure.CapacityPerSKUPropertyTmpl, validSKU), @@ -312,7 +323,7 @@ func TestCheckIfMeetSKUCapacityRequirement(t *testing.T) { name: "valid capacity request", cluster: cluster, sku: validSKU, - targetCapacity: 3, + targetCapacity: 4, req: validPropertySelectorRequirement, mockStatusCode: http.StatusOK, wantAvailable: true, @@ -322,7 +333,7 @@ func TestCheckIfMeetSKUCapacityRequirement(t *testing.T) { name: "unavailable SKU request", cluster: cluster, sku: "Standard_D2s_v4", - targetCapacity: 1, + targetCapacity: 2, req: placementv1beta1.PropertySelectorRequirement{ Name: fmt.Sprintf(azure.CapacityPerSKUPropertyTmpl, "Standard_D2s_v4"), Operator: placementv1beta1.PropertySelectorGreaterThanOrEqualTo, @@ -381,7 +392,7 @@ func TestCheckIfMeetSKUCapacityRequirement(t *testing.T) { name: "Azure API returns error", cluster: cluster, sku: validSKU, - targetCapacity: 3, + targetCapacity: 4, req: validPropertySelectorRequirement, mockStatusCode: http.StatusInternalServerError, wantError: true, @@ -402,7 +413,7 @@ func TestCheckIfMeetSKUCapacityRequirement(t *testing.T) { errorSubstring: "unsupported operator \"Eq\" for SKU capacity property, only GreaterThan (Gt) and GreaterThanOrEqualTo (Ge) are supported", }, { - name: "unsupported operator in requirement", + name: "too low value in requirement", cluster: cluster, sku: validSKU, targetCapacity: 0, @@ -411,15 +422,14 @@ func TestCheckIfMeetSKUCapacityRequirement(t *testing.T) { Operator: placementv1beta1.PropertySelectorGreaterThanOrEqualTo, Values: []string{"0"}, }, - mockStatusCode: http.StatusOK, wantError: true, - errorSubstring: "capacity value cannot be zero for operator", + errorSubstring: "is below minimum allowed value", }, { name: "cases-insensitive request - available SKU", cluster: cluster, sku: "STANDARD_D2S_V3", - targetCapacity: 1, + targetCapacity: 2, req: placementv1beta1.PropertySelectorRequirement{ Name: fmt.Sprintf(azure.CapacityPerSKUPropertyTmpl, "STANDARD_D2S_V3"), Operator: placementv1beta1.PropertySelectorGreaterThanOrEqualTo, diff --git a/pkg/scheduler/framework/plugins/clusteraffinity/filtering_test.go b/pkg/scheduler/framework/plugins/clusteraffinity/filtering_test.go index 6a68a1240..a45885e20 100644 --- a/pkg/scheduler/framework/plugins/clusteraffinity/filtering_test.go +++ b/pkg/scheduler/framework/plugins/clusteraffinity/filtering_test.go @@ -831,7 +831,7 @@ func TestFilter_PropertyChecker(t *testing.T) { }, }, vmSize: "Standard_D2s_v3", - targetCapacity: 1, + targetCapacity: 2, }, { name: "single cluster capacity based term, not matched", @@ -878,7 +878,7 @@ func TestFilter_PropertyChecker(t *testing.T) { }, }, vmSize: "Standard_B2ms", - targetCapacity: 3, + targetCapacity: 4, wantStatus: framework.NewNonErrorStatus(framework.ClusterUnschedulable, p.Name(), "cluster does not match with any of the required cluster affinity terms"), }, } diff --git a/pkg/scheduler/framework/plugins/clusteraffinity/types_azure_test.go b/pkg/scheduler/framework/plugins/clusteraffinity/types_azure_test.go index 6dceda911..165c81c29 100644 --- a/pkg/scheduler/framework/plugins/clusteraffinity/types_azure_test.go +++ b/pkg/scheduler/framework/plugins/clusteraffinity/types_azure_test.go @@ -124,7 +124,7 @@ func TestMatchPropertiesInPropertyChecker(t *testing.T) { Values: []string{"2"}, }, vmSize: "Standard_D2s_v3", - targetCapacity: 2, + targetCapacity: 3, wantHandled: true, wantAvailable: true, }, @@ -137,7 +137,7 @@ func TestMatchPropertiesInPropertyChecker(t *testing.T) { Values: []string{"2"}, }, vmSize: "NonExistentSKU", - targetCapacity: 1, + targetCapacity: 2, wantHandled: true, wantAvailable: false, }, diff --git a/pkg/scheduler/framework/plugins/clusteraffinity/types_test.go b/pkg/scheduler/framework/plugins/clusteraffinity/types_test.go index 648f34b44..879aa0267 100644 --- a/pkg/scheduler/framework/plugins/clusteraffinity/types_test.go +++ b/pkg/scheduler/framework/plugins/clusteraffinity/types_test.go @@ -1356,7 +1356,23 @@ func TestClusterRequirementMatches_WithPropertyChecker(t *testing.T) { }, cluster: cluster, sku: "Standard_D2s_v3", - targetCapacity: 0, + targetCapacity: 1, + want: true, + }, + { + name: "op >=, matched (min limit)", + matchExpression: []placementv1beta1.PropertySelectorRequirement{ + { + Name: validPropertyName, + Operator: placementv1beta1.PropertySelectorGreaterThanOrEqualTo, + Values: []string{ + "1", + }, + }, + }, + cluster: cluster, + sku: "Standard_D2s_v3", + targetCapacity: 1, want: true, }, { @@ -1372,7 +1388,7 @@ func TestClusterRequirementMatches_WithPropertyChecker(t *testing.T) { }, cluster: cluster, sku: "Standard_D4s_v3", - targetCapacity: 8, + targetCapacity: 9, }, { name: "op >=, matched (max limit)", @@ -1387,7 +1403,7 @@ func TestClusterRequirementMatches_WithPropertyChecker(t *testing.T) { }, cluster: cluster, sku: "Standard_D2s_v3", - targetCapacity: 199, + targetCapacity: 200, want: true, }, { @@ -1403,7 +1419,7 @@ func TestClusterRequirementMatches_WithPropertyChecker(t *testing.T) { }, cluster: cluster, sku: "Standard_D4s_v3", - targetCapacity: 79, + targetCapacity: 80, }, }