Changed VPC defaults to be multiple blocks to account for larger testing clusters #175

njtran · 2020-10-07T23:55:58Z

Issue #, if available:

Description of changes:

Expanded VPC default CIDR range in order to support more pods for larger scale tests
- Previous VPC defaults had one /19 CIDR Block, allowing for 8k pods. Added multiple blocks of max VPC Block size (/16).
- Changed VPCs from 192 space to 10 space.
Subnets are by default same CIDR range as VPC Blocks, but can be changed with environment variables
- Public Subnets are /16 blocks by default
- Private Subnets are /17 blocks by default

Testing

Tested by bringing up a cluster to a larger size than before to see if VPC was able to handle more IPs than before the change. VPC was brought up successfully, and more IPs were successfully utilized.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

gyuho · 2020-10-13T18:30:44Z

eks/cluster-loader/clusterloader2/addon.go

-	v1 "k8s.io/api/batch/v1"
-	"k8s.io/apimachinery/pkg/api/errors"
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/client-go/util/retry"


I would like to separate std imports from 3rd party imports. Did we run ./hack/fmt.sh?

Your editor should automate ths. Talk to me if you need help setting this up.

gyuho

PublicSubnetCIDR* and VPCBlock* are the same by default. Can we just use VPC block for subnet CIDR ranges?

gyuho · 2020-10-13T18:31:16Z

eks/cluster-loader/clusterloader2/addon.go

-	ctx, cancel := context.WithTimeout(context.Background(), 2*time.Hour)
-	defer cancel()
-	job := &v1.Job{}
-	for job.Status.Succeeded < 1 {


Can you explain why we are getting rid of this? It just waits for at least one job is successful...

We were going to use it for when we put certain tests into Prow, but found that this didn't properly handle the timeouts. Rather than trying to figure out the timeouts, I decided to get rid of it as we didn't need it, and may not need it later.

didn't properly handle the timeouts

Then, we can decrease the timeouts and just log in warning level? Do you run clusterloader2 for more than 2 hours?

Depending on the size of the test, yes it can run for up to two hours. But when scale tests get too large, API calls time out, and the testing for that takes a very long time, so rather than having a partially working feature, I just wanted to remove it.

Depending on the size of the test, yes it can run for up to two hours. But when scale tests get too large, API calls time out, and the testing for that takes a very long time, so rather than having a partially working feature, I just wanted to remove it.

I understand that. Still, this breaks other small-scale tests where 2-hour is enough. Our bug bash job relies on this checks to make sure 5 rounds of clusterloader runs complete successfully.

We can add another field to eksconfig to opt out of this.

Actually, let's skip this (delete). I was confused with other wati func.

gyuho · 2020-10-13T18:32:33Z

eks/cluster/vpc.go

+  VPCBlock1:
+    Value: !Ref VPCBlock1
+    Description: Primary VPC Block
+


Remove this?

Are you referring to the newline or the VPCBlock1?

Empty new line

gyuho · 2020-10-13T18:34:22Z

eks/cluster/vpc.go

+
+  VPCBlock2:
+    Value: !Ref VPCBlock2
+    Description: Primary VPC Block


s/Primary/Secondary/?

Good catch.

gyuho · 2020-10-13T19:50:41Z

Let's document in CHANGELOG, we are now adding default VPC blocks with the same range as subnet CIDRs. And highlight default blocks are expanded to support large scale workloads.

ellistarn

Thanks for the cleanup. A couple comments

ellistarn · 2020-10-13T22:08:57Z

eks/cluster-loader/clusterloader2/addon.go

-	v1 "k8s.io/api/batch/v1"
-	"k8s.io/apimachinery/pkg/api/errors"
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/client-go/util/retry"


Your editor should automate ths. Talk to me if you need help setting this up.

ellistarn · 2020-10-13T22:11:45Z

eks/cluster-loader/clusterloader2/addon.go

 	}{
 		ClusterLoaderSpec: c.Config.Spec.ClusterLoader,
 		ConfigMapData:     configMapData,
 		TestArgs:          c.buildArgs(),
+		InstanceTypes:     c.Config.Spec.ClusterLoader.InstanceTypes,


IMO instance types shouldn't be used to select. Perhaps we can use a node selector?

If you look at https://github.com/njtran/aws-k8s-tester/blob/vpcsubnets/eks/cluster-loader/clusterloader2/cluster-loader.gotmpl#L67 it's using a Node Selector so that it can specify which instances the clusterloader2 pod (not the test pods) can be scheduled onto. I will add a comment to make that clearer.

ellistarn · 2020-10-13T22:12:16Z

eks/cluster/vpc.go

-			ts.cfg.EKSConfig.Parameters.VPCCIDR = v
+			ts.cfg.EKSConfig.Parameters.VPCBlock1 = v
+		case "VPCBlock2":
+			// to avoid additional CFN describe stack call


Don't copy this comment 4 times.

…ing clusters and adjusted other defaults and functionalities for automated cl2 scale testing

gyuho · 2020-10-14T04:58:45Z

eks/cluster-loader/clusterloader2/addon.go

@@ -135,7 +117,6 @@ func (c *ClusterLoader) buildArgs() string {
 		"--provider=eks",
 		"--testconfig=/etc/config/config.yaml",
 		"--run-from-cluster",
-		"--report-dir=/var/reports/cluster-loader",


Why do we remove this?

Most of what I removed is what I've added in the past. This, along with the other removals, is part of the automated CL2 tests, where we don't need it because it adds extra functionality that isn't being used. We aren't using report-dir because we aren't keeping track of the CL2 test results, just making sure it's being completed.

njtran force-pushed the vpcsubnets branch from 56eb955 to bdbc7d2 Compare October 13, 2020 06:19

gyuho reviewed Oct 13, 2020

View reviewed changes

gyuho suggested changes Oct 13, 2020

View reviewed changes

ellistarn reviewed Oct 13, 2020

View reviewed changes

Changed VPC defaults to be multiple blocks to account for larger test…

202540c

…ing clusters and adjusted other defaults and functionalities for automated cl2 scale testing

njtran force-pushed the vpcsubnets branch from bdbc7d2 to 202540c Compare October 13, 2020 23:15

gyuho reviewed Oct 14, 2020

View reviewed changes

gyuho approved these changes Oct 14, 2020

View reviewed changes

njtran merged commit 46a8448 into aws:master Oct 14, 2020

njtran deleted the vpcsubnets branch October 18, 2020 23:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Changed VPC defaults to be multiple blocks to account for larger testing clusters #175

Changed VPC defaults to be multiple blocks to account for larger testing clusters #175

njtran commented Oct 7, 2020 •

edited

Loading

gyuho Oct 13, 2020

njtran Oct 13, 2020

ellistarn Oct 13, 2020

gyuho left a comment

gyuho Oct 13, 2020

njtran Oct 13, 2020

gyuho Oct 13, 2020

njtran Oct 13, 2020

gyuho Oct 14, 2020

gyuho Oct 14, 2020

gyuho Oct 13, 2020

njtran Oct 13, 2020

gyuho Oct 13, 2020

gyuho Oct 13, 2020

njtran Oct 13, 2020

gyuho commented Oct 13, 2020

ellistarn left a comment

ellistarn Oct 13, 2020

ellistarn Oct 13, 2020

njtran Oct 13, 2020

ellistarn Oct 13, 2020

njtran Oct 13, 2020

gyuho Oct 14, 2020

njtran Oct 14, 2020

Changed VPC defaults to be multiple blocks to account for larger testing clusters #175

Changed VPC defaults to be multiple blocks to account for larger testing clusters #175

Conversation

njtran commented Oct 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gyuho left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gyuho commented Oct 13, 2020

ellistarn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

njtran commented Oct 7, 2020 •

edited

Loading