From 87cef2ca736ca67c95697e731e4bb3d3a099cf74 Mon Sep 17 00:00:00 2001 From: Andrew Sy Kim Date: Sun, 4 Jul 2021 22:25:36 -0400 Subject: [PATCH 1/4] test/integration/quota: deflake TestQuotaLimitService by collapsing test cases and adding a short delay for resource quota to propagate Signed-off-by: Andrew Sy Kim --- test/integration/quota/quota_test.go | 107 ++++++++++++++------------- 1 file changed, 56 insertions(+), 51 deletions(-) diff --git a/test/integration/quota/quota_test.go b/test/integration/quota/quota_test.go index dec7a6faa2f..7408ade4e5e 100644 --- a/test/integration/quota/quota_test.go +++ b/test/integration/quota/quota_test.go @@ -378,11 +378,7 @@ func TestQuotaLimitedResourceDenial(t *testing.T) { func TestQuotaLimitService(t *testing.T) { defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.ServiceLBNodePortControl, true)() - type testCase struct { - description string - svc *v1.Service - success bool - } + // Set up an API server h := &framework.APIServerHolder{Initialized: make(chan struct{})} s := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { @@ -477,56 +473,65 @@ func TestQuotaLimitService(t *testing.T) { }, }, } + waitForQuota(t, quota, clientset) - tests := []testCase{ - { - description: "node port service should be created successfully", - svc: newService("np-svc", v1.ServiceTypeNodePort, true), - success: true, - }, - { - description: "first LB type service that allocates node port should be created successfully", - svc: newService("lb-svc-withnp1", v1.ServiceTypeLoadBalancer, true), - success: true, - }, - { - description: "second LB type service that allocates node port creation should fail as node port quota is exceeded", - svc: newService("lb-svc-withnp2", v1.ServiceTypeLoadBalancer, true), - success: false, - }, - { - description: "first LB type service that doesn't allocates node port should be created successfully", - svc: newService("lb-svc-wonp1", v1.ServiceTypeLoadBalancer, false), - success: true, - }, - { - description: "second LB type service that doesn't allocates node port creation should fail as loadbalancer quota is exceeded", - svc: newService("lb-svc-wonp2", v1.ServiceTypeLoadBalancer, false), - success: false, - }, - { - description: "forth service creation should be successful", - svc: newService("clusterip-svc1", v1.ServiceTypeClusterIP, false), - success: true, - }, - { - description: "fifth service creation should fail as service quota is exceeded", - svc: newService("clusterip-svc2", v1.ServiceTypeClusterIP, false), - success: false, - }, + // Creating the first node port service should succeed + nodePortService := newService("np-svc", v1.ServiceTypeNodePort, true) + _, err = clientset.CoreV1().Services(ns.Name).Create(context.TODO(), nodePortService, metav1.CreateOptions{}) + if err != nil { + t.Errorf("creating first node port Service should not have returned error: %v", err) } - for _, test := range tests { - t.Log(test.description) - _, err := clientset.CoreV1().Services(ns.Name).Create(context.TODO(), test.svc, metav1.CreateOptions{}) - if (err == nil) != test.success { - if err != nil { - t.Fatalf("Error creating test service: %v, svc: %+v", err, test.svc) - } else { - t.Fatalf("Expect service creation to fail, but service %s is created", test.svc.Name) - } - } + // Creating the first loadbalancer service should succeed + lbServiceWithNodePort1 := newService("lb-svc-withnp1", v1.ServiceTypeLoadBalancer, true) + _, err = clientset.CoreV1().Services(ns.Name).Create(context.TODO(), lbServiceWithNodePort1, metav1.CreateOptions{}) + if err != nil { + t.Errorf("creating first loadbalancer Service should not have returned error: %v", err) + } + + // add a delay for resource quota changes to propagate + time.Sleep(1 * time.Second) + + // Creating another loadbalancer Service using node ports should fail because node prot quota is exceeded + lbServiceWithNodePort2 := newService("lb-svc-withnp2", v1.ServiceTypeLoadBalancer, true) + _, err = clientset.CoreV1().Services(ns.Name).Create(context.TODO(), lbServiceWithNodePort2, metav1.CreateOptions{}) + if err == nil { + t.Errorf("creating another loadbalancer Service with node ports should return error due to resource quota limits but got none") + } + + // Creating a loadbalancer Service without node ports should succeed + lbServiceWithoutNodePort1 := newService("lb-svc-wonp1", v1.ServiceTypeLoadBalancer, false) + _, err = clientset.CoreV1().Services(ns.Name).Create(context.TODO(), lbServiceWithoutNodePort1, metav1.CreateOptions{}) + if err != nil { + t.Errorf("creating another loadbalancer Service without node ports should not have returned error: %v", err) + } + + // add a delay for resource quota changes to propagate + time.Sleep(1 * time.Second) + + // Creating another loadbalancer Service without node ports should fail because loadbalancer quota is exceeded + lbServiceWithoutNodePort2 := newService("lb-svc-wonp2", v1.ServiceTypeLoadBalancer, false) + _, err = clientset.CoreV1().Services(ns.Name).Create(context.TODO(), lbServiceWithoutNodePort2, metav1.CreateOptions{}) + if err == nil { + t.Errorf("creating another loadbalancer Service without node ports should return error due to resource quota limits but got none") + } + + // Creating a ClusterIP Service should succeed + clusterIPService1 := newService("clusterip-svc1", v1.ServiceTypeClusterIP, false) + _, err = clientset.CoreV1().Services(ns.Name).Create(context.TODO(), clusterIPService1, metav1.CreateOptions{}) + if err != nil { + t.Errorf("creating a cluster IP Service should not have returned error: %v", err) + } + + // add a delay for resource quota changes to propagate + time.Sleep(1 * time.Second) + + // Creating a ClusterIP Service should fail because Service quota has been exceeded. + clusterIPService2 := newService("clusterip-svc2", v1.ServiceTypeClusterIP, false) + _, err = clientset.CoreV1().Services(ns.Name).Create(context.TODO(), clusterIPService2, metav1.CreateOptions{}) + if err == nil { + t.Errorf("creating another cluster IP Service should have returned error due to resource quota limits but got none") } } From 54bc1babe165e16f40a47eda5c21f6fd3da966ed Mon Sep 17 00:00:00 2001 From: Andrew Sy Kim Date: Sun, 4 Jul 2021 22:46:30 -0400 Subject: [PATCH 2/4] test/integration/quota: update TestQuotaLimitService to explicitly check for Forbidden status when quota limit is exceeded Signed-off-by: Andrew Sy Kim --- test/integration/quota/quota_test.go | 56 +++++++++++++++++++++++----- 1 file changed, 47 insertions(+), 9 deletions(-) diff --git a/test/integration/quota/quota_test.go b/test/integration/quota/quota_test.go index 7408ade4e5e..2feeab87508 100644 --- a/test/integration/quota/quota_test.go +++ b/test/integration/quota/quota_test.go @@ -18,6 +18,7 @@ package quota import ( "context" + "errors" "fmt" "net/http" "net/http/httptest" @@ -25,6 +26,7 @@ import ( "time" v1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/fields" @@ -495,9 +497,21 @@ func TestQuotaLimitService(t *testing.T) { // Creating another loadbalancer Service using node ports should fail because node prot quota is exceeded lbServiceWithNodePort2 := newService("lb-svc-withnp2", v1.ServiceTypeLoadBalancer, true) - _, err = clientset.CoreV1().Services(ns.Name).Create(context.TODO(), lbServiceWithNodePort2, metav1.CreateOptions{}) - if err == nil { - t.Errorf("creating another loadbalancer Service with node ports should return error due to resource quota limits but got none") + err = wait.PollImmediate(2*time.Second, 30*time.Second, func() (bool, error) { + _, err = clientset.CoreV1().Services(ns.Name).Create(context.TODO(), lbServiceWithNodePort2, metav1.CreateOptions{}) + if apierrors.IsForbidden(err) { + return true, nil + } + + if err == nil { + return false, errors.New("creating Service should have returned error but got nil") + } + + return false, nil + + }) + if err != nil { + t.Errorf("creating another loadbalancer Service with node ports should return Forbidden due to resource quota limits but got: %v", err) } // Creating a loadbalancer Service without node ports should succeed @@ -512,9 +526,21 @@ func TestQuotaLimitService(t *testing.T) { // Creating another loadbalancer Service without node ports should fail because loadbalancer quota is exceeded lbServiceWithoutNodePort2 := newService("lb-svc-wonp2", v1.ServiceTypeLoadBalancer, false) - _, err = clientset.CoreV1().Services(ns.Name).Create(context.TODO(), lbServiceWithoutNodePort2, metav1.CreateOptions{}) - if err == nil { - t.Errorf("creating another loadbalancer Service without node ports should return error due to resource quota limits but got none") + err = wait.PollImmediate(2*time.Second, 30*time.Second, func() (bool, error) { + _, err = clientset.CoreV1().Services(ns.Name).Create(context.TODO(), lbServiceWithoutNodePort2, metav1.CreateOptions{}) + if apierrors.IsForbidden(err) { + return true, nil + } + + if err == nil { + return false, errors.New("creating Service should have returned error but got nil") + } + + return false, nil + + }) + if err != nil { + t.Errorf("creating another loadbalancer Service without node ports should return Forbidden due to resource quota limits but got: %v", err) } // Creating a ClusterIP Service should succeed @@ -529,9 +555,21 @@ func TestQuotaLimitService(t *testing.T) { // Creating a ClusterIP Service should fail because Service quota has been exceeded. clusterIPService2 := newService("clusterip-svc2", v1.ServiceTypeClusterIP, false) - _, err = clientset.CoreV1().Services(ns.Name).Create(context.TODO(), clusterIPService2, metav1.CreateOptions{}) - if err == nil { - t.Errorf("creating another cluster IP Service should have returned error due to resource quota limits but got none") + err = wait.PollImmediate(2*time.Second, 30*time.Second, func() (bool, error) { + _, err = clientset.CoreV1().Services(ns.Name).Create(context.TODO(), clusterIPService2, metav1.CreateOptions{}) + if apierrors.IsForbidden(err) { + return true, nil + } + + if err == nil { + return false, errors.New("creating Service should have returned error but got nil") + } + + return false, nil + + }) + if err != nil { + t.Errorf("creating another clsuter IP Service should return Forbidden due to resource quota limits but got: %v", err) } } From caf42fde4308556b92debc870aa4e6cb5f78b73b Mon Sep 17 00:00:00 2001 From: Andrew Sy Kim Date: Mon, 5 Jul 2021 12:49:29 -0400 Subject: [PATCH 3/4] test/integration/quota: refactor Service forbidden check into helper function Signed-off-by: Andrew Sy Kim --- test/integration/quota/quota_test.go | 47 +++++++--------------------- 1 file changed, 11 insertions(+), 36 deletions(-) diff --git a/test/integration/quota/quota_test.go b/test/integration/quota/quota_test.go index 2feeab87508..30fdcf7dbe5 100644 --- a/test/integration/quota/quota_test.go +++ b/test/integration/quota/quota_test.go @@ -497,22 +497,7 @@ func TestQuotaLimitService(t *testing.T) { // Creating another loadbalancer Service using node ports should fail because node prot quota is exceeded lbServiceWithNodePort2 := newService("lb-svc-withnp2", v1.ServiceTypeLoadBalancer, true) - err = wait.PollImmediate(2*time.Second, 30*time.Second, func() (bool, error) { - _, err = clientset.CoreV1().Services(ns.Name).Create(context.TODO(), lbServiceWithNodePort2, metav1.CreateOptions{}) - if apierrors.IsForbidden(err) { - return true, nil - } - - if err == nil { - return false, errors.New("creating Service should have returned error but got nil") - } - - return false, nil - - }) - if err != nil { - t.Errorf("creating another loadbalancer Service with node ports should return Forbidden due to resource quota limits but got: %v", err) - } + testServiceForbidden(clientset, ns.Name, lbServiceWithNodePort2, t) // Creating a loadbalancer Service without node ports should succeed lbServiceWithoutNodePort1 := newService("lb-svc-wonp1", v1.ServiceTypeLoadBalancer, false) @@ -526,22 +511,7 @@ func TestQuotaLimitService(t *testing.T) { // Creating another loadbalancer Service without node ports should fail because loadbalancer quota is exceeded lbServiceWithoutNodePort2 := newService("lb-svc-wonp2", v1.ServiceTypeLoadBalancer, false) - err = wait.PollImmediate(2*time.Second, 30*time.Second, func() (bool, error) { - _, err = clientset.CoreV1().Services(ns.Name).Create(context.TODO(), lbServiceWithoutNodePort2, metav1.CreateOptions{}) - if apierrors.IsForbidden(err) { - return true, nil - } - - if err == nil { - return false, errors.New("creating Service should have returned error but got nil") - } - - return false, nil - - }) - if err != nil { - t.Errorf("creating another loadbalancer Service without node ports should return Forbidden due to resource quota limits but got: %v", err) - } + testServiceForbidden(clientset, ns.Name, lbServiceWithoutNodePort2, t) // Creating a ClusterIP Service should succeed clusterIPService1 := newService("clusterip-svc1", v1.ServiceTypeClusterIP, false) @@ -555,8 +525,13 @@ func TestQuotaLimitService(t *testing.T) { // Creating a ClusterIP Service should fail because Service quota has been exceeded. clusterIPService2 := newService("clusterip-svc2", v1.ServiceTypeClusterIP, false) - err = wait.PollImmediate(2*time.Second, 30*time.Second, func() (bool, error) { - _, err = clientset.CoreV1().Services(ns.Name).Create(context.TODO(), clusterIPService2, metav1.CreateOptions{}) + testServiceForbidden(clientset, ns.Name, clusterIPService2, t) +} + +// testServiceForbidden attempts to create a Service expecting 403 Forbidden due to resource quota limits being exceeded. +func testServiceForbidden(clientset clientset.Interface, namespace string, service *v1.Service, t *testing.T) { + pollErr := wait.PollImmediate(2*time.Second, 30*time.Second, func() (bool, error) { + _, err := clientset.CoreV1().Services(namespace).Create(context.TODO(), service, metav1.CreateOptions{}) if apierrors.IsForbidden(err) { return true, nil } @@ -568,8 +543,8 @@ func TestQuotaLimitService(t *testing.T) { return false, nil }) - if err != nil { - t.Errorf("creating another clsuter IP Service should return Forbidden due to resource quota limits but got: %v", err) + if pollErr != nil { + t.Errorf("creating Service should return Forbidden due to resource quota limits but got: %v", pollErr) } } From edbaf9d5d364059f34cb048d3820d321f91ddae3 Mon Sep 17 00:00:00 2001 From: Andrew Sy Kim Date: Tue, 6 Jul 2021 07:01:40 -0400 Subject: [PATCH 4/4] test/integration/quota: poll for ResourceQuota used status in TestQuotaLimitService Signed-off-by: Andrew Sy Kim Co-authored-by: Antonio Ojea --- test/integration/quota/quota_test.go | 64 +++++++++++++++++++++++++--- 1 file changed, 58 insertions(+), 6 deletions(-) diff --git a/test/integration/quota/quota_test.go b/test/integration/quota/quota_test.go index 30fdcf7dbe5..720ae6c341e 100644 --- a/test/integration/quota/quota_test.go +++ b/test/integration/quota/quota_test.go @@ -53,6 +53,10 @@ import ( "k8s.io/kubernetes/test/integration/framework" ) +const ( + resourceQuotaTimeout = 10 * time.Second +) + // 1.2 code gets: // quota_test.go:95: Took 4.218619579s to scale up without quota // quota_test.go:199: unexpected error: timed out waiting for the condition, ended with 342 pods (1 minute) @@ -188,6 +192,39 @@ func waitForQuota(t *testing.T, quota *v1.ResourceQuota, clientset *clientset.Cl } } +// waitForUsedResourceQuota polls a ResourceQuota status for an expected used value +func waitForUsedResourceQuota(t *testing.T, c clientset.Interface, ns, quotaName string, used v1.ResourceList) { + err := wait.Poll(1*time.Second, resourceQuotaTimeout, func() (bool, error) { + resourceQuota, err := c.CoreV1().ResourceQuotas(ns).Get(context.TODO(), quotaName, metav1.GetOptions{}) + if err != nil { + return false, err + } + + // used may not yet be calculated + if resourceQuota.Status.Used == nil { + return false, nil + } + + // verify that the quota shows the expected used resource values + for k, v := range used { + actualValue, found := resourceQuota.Status.Used[k] + if !found { + t.Logf("resource %s was not found in ResourceQuota status", k) + return false, nil + } + + if !actualValue.Equal(v) { + t.Logf("resource %s, expected %s, actual %s", k, v.String(), actualValue.String()) + return false, nil + } + } + return true, nil + }) + if err != nil { + t.Errorf("error waiting or ResourceQuota status: %v", err) + } +} + func scale(t *testing.T, namespace string, clientset *clientset.Clientset) { target := int32(100) rc := &v1.ReplicationController{ @@ -492,8 +529,13 @@ func TestQuotaLimitService(t *testing.T) { t.Errorf("creating first loadbalancer Service should not have returned error: %v", err) } - // add a delay for resource quota changes to propagate - time.Sleep(1 * time.Second) + // wait for ResourceQuota status to be updated before proceeding, otherwise the test will race with resource quota controller + expectedQuotaUsed := v1.ResourceList{ + v1.ResourceServices: resource.MustParse("2"), + v1.ResourceServicesNodePorts: resource.MustParse("2"), + v1.ResourceServicesLoadBalancers: resource.MustParse("1"), + } + waitForUsedResourceQuota(t, clientset, quota.Namespace, quota.Name, expectedQuotaUsed) // Creating another loadbalancer Service using node ports should fail because node prot quota is exceeded lbServiceWithNodePort2 := newService("lb-svc-withnp2", v1.ServiceTypeLoadBalancer, true) @@ -506,8 +548,13 @@ func TestQuotaLimitService(t *testing.T) { t.Errorf("creating another loadbalancer Service without node ports should not have returned error: %v", err) } - // add a delay for resource quota changes to propagate - time.Sleep(1 * time.Second) + // wait for ResourceQuota status to be updated before proceeding, otherwise the test will race with resource quota controller + expectedQuotaUsed = v1.ResourceList{ + v1.ResourceServices: resource.MustParse("3"), + v1.ResourceServicesNodePorts: resource.MustParse("2"), + v1.ResourceServicesLoadBalancers: resource.MustParse("2"), + } + waitForUsedResourceQuota(t, clientset, quota.Namespace, quota.Name, expectedQuotaUsed) // Creating another loadbalancer Service without node ports should fail because loadbalancer quota is exceeded lbServiceWithoutNodePort2 := newService("lb-svc-wonp2", v1.ServiceTypeLoadBalancer, false) @@ -520,8 +567,13 @@ func TestQuotaLimitService(t *testing.T) { t.Errorf("creating a cluster IP Service should not have returned error: %v", err) } - // add a delay for resource quota changes to propagate - time.Sleep(1 * time.Second) + // wait for ResourceQuota status to be updated before proceeding, otherwise the test will race with resource quota controller + expectedQuotaUsed = v1.ResourceList{ + v1.ResourceServices: resource.MustParse("4"), + v1.ResourceServicesNodePorts: resource.MustParse("2"), + v1.ResourceServicesLoadBalancers: resource.MustParse("2"), + } + waitForUsedResourceQuota(t, clientset, quota.Namespace, quota.Name, expectedQuotaUsed) // Creating a ClusterIP Service should fail because Service quota has been exceeded. clusterIPService2 := newService("clusterip-svc2", v1.ServiceTypeClusterIP, false)