Merge pull request #6998 from yaroslava-serdiuk/multiple-provreqs

Do not fail if multiple ProvReqs are injected
kubernetes · Jul 4, 2024 · 02521ed · 02521ed
2 parents bb5d064 + 1953031
commit 02521ed
Show file tree

Hide file tree

Showing 4 changed files with 60 additions and 62 deletions.
diff --git a/cluster-autoscaler/provisioningrequest/besteffortatomic/provisioning_class.go b/cluster-autoscaler/provisioningrequest/besteffortatomic/provisioning_class.go
@@ -81,13 +81,13 @@ func (o *bestEffortAtomicProvClass) Provision(
 	if len(unschedulablePods) == 0 {
 		return &status.ScaleUpStatus{Result: status.ScaleUpNotTried}, nil
 	}
-	pr, err := provreqclient.ProvisioningRequestForPods(o.client, unschedulablePods)
-	if err != nil {
-		return status.UpdateScaleUpError(&status.ScaleUpStatus{}, errors.NewAutoscalerError(errors.InternalError, err.Error()))
-	}
-	if pr.Spec.ProvisioningClassName != v1beta1.ProvisioningClassBestEffortAtomicScaleUp {
+	prs := provreqclient.ProvisioningRequestsForPods(o.client, unschedulablePods)
+	prs = provreqclient.FilterOutProvisioningClass(prs, v1beta1.ProvisioningClassBestEffortAtomicScaleUp)
+	if len(prs) == 0 {
 		return &status.ScaleUpStatus{Result: status.ScaleUpNotTried}, nil
 	}
+	// Pick 1 ProvisioningRequest.
+	pr := prs[0]
 
 	o.context.ClusterSnapshot.Fork()
 	defer o.context.ClusterSnapshot.Revert()

diff --git a/cluster-autoscaler/provisioningrequest/checkcapacity/provisioningclass.go b/cluster-autoscaler/provisioningrequest/checkcapacity/provisioningclass.go
@@ -73,14 +73,14 @@ func (o *checkCapacityProvClass) Provision(
 	if len(unschedulablePods) == 0 {
 		return &status.ScaleUpStatus{Result: status.ScaleUpNotTried}, nil
 	}
-	pr, err := provreqclient.ProvisioningRequestForPods(o.client, unschedulablePods)
-	if err != nil {
-		return status.UpdateScaleUpError(&status.ScaleUpStatus{}, errors.NewAutoscalerError(errors.InternalError, err.Error()))
-	}
-	if pr.Spec.ProvisioningClassName != v1beta1.ProvisioningClassCheckCapacity {
+
+	prs := provreqclient.ProvisioningRequestsForPods(o.client, unschedulablePods)
+	prs = provreqclient.FilterOutProvisioningClass(prs, v1beta1.ProvisioningClassCheckCapacity)
+	if len(prs) == 0 {
 		return &status.ScaleUpStatus{Result: status.ScaleUpNotTried}, nil
 	}
-
+	// Pick 1 ProvisioningRequest.
+	pr := prs[0]
 	o.context.ClusterSnapshot.Fork()
 	defer o.context.ClusterSnapshot.Revert()
 

diff --git a/cluster-autoscaler/provisioningrequest/provreqclient/client.go b/cluster-autoscaler/provisioningrequest/provreqclient/client.go
@@ -178,30 +178,31 @@ func newPodTemplatesLister(client *kubernetes.Clientset, stopChannel <-chan stru
 	return podTemplLister, nil
 }
 
-// ProvisioningRequestForPods check that all pods belong to one ProvisioningRequest and return it.
-func ProvisioningRequestForPods(client *ProvisioningRequestClient, unschedulablePods []*apiv1.Pod) (*provreqwrapper.ProvisioningRequest, error) {
+// ProvisioningRequestsForPods check that all pods belong to one ProvisioningRequest and return it.
+func ProvisioningRequestsForPods(client *ProvisioningRequestClient, unschedulablePods []*apiv1.Pod) []*provreqwrapper.ProvisioningRequest {
+	prMap := make(map[string]*provreqwrapper.ProvisioningRequest)
+	prList := []*provreqwrapper.ProvisioningRequest{}
 	if len(unschedulablePods) == 0 {
-		return nil, fmt.Errorf("empty unschedulablePods list")
-	}
-	if unschedulablePods[0].OwnerReferences == nil || len(unschedulablePods[0].OwnerReferences) == 0 {
-		return nil, fmt.Errorf("pod %s has no OwnerReference", unschedulablePods[0].Name)
-	}
-	provReq, err := client.ProvisioningRequest(unschedulablePods[0].Namespace, unschedulablePods[0].OwnerReferences[0].Name)
-	if err != nil {
-		return nil, fmt.Errorf("failed retrive ProvisioningRequest from unscheduled pods, err: %v", err)
+		return prList
 	}
 	for _, pod := range unschedulablePods {
-		if pod.Namespace != unschedulablePods[0].Namespace {
-			return nil, fmt.Errorf("pods %s and %s are from different namespaces", pod.Name, unschedulablePods[0].Name)
-		}
 		if pod.OwnerReferences == nil || len(pod.OwnerReferences) == 0 {
-			return nil, fmt.Errorf("pod %s has no OwnerReference", pod.Name)
+			klog.Errorf("pod %s has no OwnerReference", pod.Name)
+			continue
+		}
+		provReq, err := client.ProvisioningRequest(pod.Namespace, pod.OwnerReferences[0].Name)
+		if err != nil {
+			klog.Errorf("failed to retrieve ProvisioningRequest from unschedulable pod, err: %v", err)
+			continue
 		}
-		if pod.OwnerReferences[0].Name != unschedulablePods[0].OwnerReferences[0].Name {
-			return nil, fmt.Errorf("pods %s and %s have different OwnerReference", pod.Name, unschedulablePods[0].Name)
+		if _, found := prMap[provReq.Name]; !found {
+			prMap[provReq.Name] = provReq
 		}
 	}
-	return provReq, nil
+	for _, pr := range prMap {
+		prList = append(prList, pr)
+	}
+	return prList
 }
 
 // DeleteProvisioningRequest deletes the given ProvisioningRequest CR using the ProvisioningRequestInterface and returns an error in case of failure.
@@ -216,3 +217,14 @@ func (c *ProvisioningRequestClient) DeleteProvisioningRequest(pr *v1beta1.Provis
 	klog.V(4).Infof("Deleted ProvisioningRequest %s/%s", pr.Namespace, pr.Name)
 	return nil
 }
+
+// FilterOutProvisioningClass filters out ProvReqs that belongs to certain Provisioning Class
+func FilterOutProvisioningClass(prList []*provreqwrapper.ProvisioningRequest, class string) []*provreqwrapper.ProvisioningRequest {
+	newPrList := []*provreqwrapper.ProvisioningRequest{}
+	for _, pr := range prList {
+		if pr.Spec.ProvisioningClassName == class {
+			newPrList = append(newPrList, pr)
+		}
+	}
+	return newPrList
+}
diff --git a/cluster-autoscaler/provisioningrequest/provreqclient/client_test.go b/cluster-autoscaler/provisioningrequest/provreqclient/client_test.go
@@ -49,63 +49,49 @@ func TestFetchPodTemplates(t *testing.T) {
 	}
 }
 
-func TestProvisioningRequestForPods(t *testing.T) {
+func TestProvisioningRequestsForPods(t *testing.T) {
 	checkCapacityProvReq := provreqwrapper.BuildTestProvisioningRequest("ns", "check-capacity", "1m", "100", "", int32(100), false, time.Now(), v1beta1.ProvisioningClassCheckCapacity)
 	customProvReq := provreqwrapper.BuildTestProvisioningRequest("ns", "custom", "1m", "100", "", int32(100), false, time.Now(), "custom")
 	checkCapacityPods, _ := pods.PodsForProvisioningRequest(checkCapacityProvReq)
 	customProvReqPods, _ := pods.PodsForProvisioningRequest(customProvReq)
 	regularPod := BuildTestPod("p1", 600, 100)
 	client := NewFakeProvisioningRequestClient(context.Background(), t, checkCapacityProvReq, customProvReq)
 	testCases := []struct {
-		name      string
-		pods      []*apiv1.Pod
-		className string
-		err       bool
-		pr        *provreqwrapper.ProvisioningRequest
+		name string
+		pods []*apiv1.Pod
+		err  bool
+		prs  []*provreqwrapper.ProvisioningRequest
 	}{
 		{
-			name:      "no pods",
-			pods:      []*apiv1.Pod{},
-			className: "some-class",
-			err:       true,
+			name: "no pods",
+			pods: []*apiv1.Pod{},
 		},
 		{
-			name:      "pods from one Provisioning Class",
-			pods:      checkCapacityPods,
-			className: v1beta1.ProvisioningClassCheckCapacity,
-			pr:        checkCapacityProvReq,
+			name: "pods from one Provisioning Class",
+			pods: checkCapacityPods,
+			prs:  []*provreqwrapper.ProvisioningRequest{checkCapacityProvReq},
 		},
 		{
-			name:      "pods from different Provisioning Classes",
-			pods:      append(checkCapacityPods, customProvReqPods...),
-			className: v1beta1.ProvisioningClassCheckCapacity,
-			err:       true,
+			name: "pods from different Provisioning Classes",
+			pods: append(checkCapacityPods, customProvReqPods...),
+			prs:  []*provreqwrapper.ProvisioningRequest{checkCapacityProvReq, customProvReq},
 		},
 		{
-			name:      "regular pod",
-			pods:      []*apiv1.Pod{regularPod},
-			className: v1beta1.ProvisioningClassCheckCapacity,
-			err:       true,
+			name: "regular pod",
+			pods: []*apiv1.Pod{regularPod},
 		},
 		{
-			name:      "provreq pods and regular pod",
-			pods:      append(checkCapacityPods, regularPod),
-			className: v1beta1.ProvisioningClassCheckCapacity,
-			err:       true,
+			name: "provreq pods and regular pod",
+			pods: append(checkCapacityPods, regularPod),
+			prs:  []*provreqwrapper.ProvisioningRequest{checkCapacityProvReq},
 		},
 	}
 	for _, tc := range testCases {
 		tc := tc
 		t.Run(tc.name, func(t *testing.T) {
 			t.Parallel()
-			pr, err := ProvisioningRequestForPods(client, tc.pods)
-			if tc.err {
-				assert.Error(t, err)
-			} else {
-				assert.NoError(t, err)
-				assert.Equal(t, pr, tc.pr)
-				assert.Equal(t, pr.Spec.ProvisioningClassName, tc.className)
-			}
+			got := ProvisioningRequestsForPods(client, tc.pods)
+			assert.ElementsMatch(t, got, tc.prs)
 		})
 	}
 }