chore: parallelize integ tests across regions #10269

rix0rrr · 2020-09-09T13:42:23Z

When AWS_REGIONS=region1,region2,region3 is set, the CLI integration
tests are going to parallelize themselves across those regions.

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

When `AWS_REGIONS=region1,region2,region3` is set, the CLI integration tests are going to parallelize themselves across those regions.

nija-at

Not looked into the PR itself yet, but can we make it very clear in the logs which region the stack is going to (if not already). I'd rather not clickity-click across 10 different regions to find the stack.

nija-at · 2020-09-09T13:45:37Z

packages/aws-cdk/CONTRIBUTING.md

@@ -42,6 +42,13 @@ To run regression tests in the source tree:
 $ test/integ/test-cli-regression-against-current-code.sh [-t '...']
 ```

+Integ tests can run in parallel across multiple regions. Set the `AWS_REGIONS`


Can you add a bit more about what the striping strategy will be, or how tests get assigned to regions?

Also state which region will be picked by default.

Also state which region will be picked by default.

All of them?

Why do you care? If you care, then you shouldn't be passing all of them.

Also state which region will be picked by default.

All of them?

Why do you care? If you care, then you shouldn't be passing all of them.

Not what I meant. Which region will be picked if AWS_REGIONS is not specified.

In relation, It might be good for debugging if each test prints out the region its running in

nija-at · 2020-09-09T13:59:40Z

packages/aws-cdk/test/integ/cli/cdk-helpers.ts

+    } finally {
+      this.regionLease.dispose();
    }


A bit hard to follow since the lease is taken in a different scope, (prepareAppFixture() I think).

Keep the take() and dispose() calls in the same block so it's easier to reason about.

That's fine, but I'm going to have to change everything to use higher-order functions. Not sure that's going to make things more readable, but it's definitely fun for me! :)

nija-at · 2020-09-09T14:05:28Z

packages/aws-cdk/test/integ/cli/cdk-helpers.ts

+ * forever before the user notices a simple misconfiguration.
+ *
+ * We can't check for the presence of environment variables since credentials could come from
+ * anywyere, so do simple account retrieval.


Suggested change

* anywyere, so do simple account retrieval.

* anywhere, so do simple account retrieval.

nija-at · 2020-09-09T14:06:31Z

packages/aws-cdk/test/integ/cli/resource-pool.ts

+ * If there are multiple consumers waiting for a resource, consumers are serviced
+ * in FIFO order for most fairness.
+ */
+export class ResourcePool<A> {


Unit tests for the leasing logic. A regression here (now or in the future) would be hard to catch/debug, esp one that works partially causing the logic to skew but not fail.

nija-at · 2020-09-09T14:08:47Z

packages/aws-cdk/CONTRIBUTING.md

@@ -42,6 +42,13 @@ To run regression tests in the source tree:
 $ test/integ/test-cli-regression-against-current-code.sh [-t '...']
 ```

+Integ tests can run in parallel across multiple regions. Set the `AWS_REGIONS`


Also state which region will be picked by default.

All of them?

Why do you care? If you care, then you shouldn't be passing all of them.

Not what I meant. Which region will be picked if AWS_REGIONS is not specified.

packages/aws-cdk/CONTRIBUTING.md

iliapolo · 2020-09-09T14:26:02Z

packages/aws-cdk/test/integ/cli/resource-pool.ts

+    if (resources.length === 0) {
+      throw new Error('Must have at least one resource in the pool');
+    }
+    this.resources = [...resources];


Dedup maybe? To avoid any funky unexpected repititions

Not sure what you mean here. I'm making a copy so we don't have accidental sharing of arrays.

I mean that AWS_REGIONS might be configured as ['us-east-1', 'us-east-1', 'us-east-1'], effectively causing three tests to run in parallel on us-east-1.

Hm. Seems that's the caller's responsibility. Maybe they want to weight certain regions differently? I could see that...

Agree as far as this generic pool is concerned.

But if we want to make sure tests don't run in parallel in the same region then I would add the deduplication on the caller side when you read the AWS_REGIONS env variable. Maybe even throw if len(array) != len(set).

I actually don't mind not deduping, just mentioning it because there is an edge case here that might break the intended functionality.

Addressed

iliapolo · 2020-09-10T10:10:00Z

packages/aws-cdk/test/integ/cli/cdk-helpers.ts

+ */
+async function ensureBootstrapped(fixture: TestFixture) {
+  // Old-style bootstrap stack with default name
+  if (await fixture.aws.stackStatus('CDKToolkit') === undefined) {


Are you sure this is necessary? just running bootstrap will no-op if no changes are detected, and if there are changes, shouldn't we bootstrap them?

I guess, just cutting down on the duration (since that's the whole goal of this). It's still pretty expensive to do a no-op bootstrap.

Ok, i'm still wondering about the need to re-bootstrap when changes are introduced. But thats for a later discussion.

nija-at · 2020-09-10T12:12:38Z

packages/aws-cdk/test/integ/cli/cdk-helpers.ts

+    const regionLease = await REGION_POOL.take();
+
+    context.output.write(` Stack prefix:   ${stackNamePrefix}\n`);
+    context.output.write(` Test directory: ${integTestDir}\n`);
+    context.output.write(` Region:         ${regionLease.value}\n`);
+
+    await cloneDirectory(path.join(__dirname, 'app'), integTestDir, context.output);
+    const fixture = new TestFixture(
+      integTestDir,
+      stackNamePrefix,
+      context.output,
+      regionLease,
+      context.aws);


Why take the lease here if it's going to be released within the dispose() method? As mentioned previously, would be nice to see take() and dispose() in the same block/scope, making it easier to reason about.

Would it make more sense to take the least as part of the TextFixture constructor?

Oh wait. You are correct that i left a bug.

Would it make more sense to take the least as part of the TextFixture constructor?

Can't. It's async.

nija-at · 2020-09-10T12:16:02Z

packages/aws-cdk/test/integ/cli/cdk-helpers.ts

+/**
+ * Higher order function to execute a block with a CDK app fixture
+ *
+ * Requires an AWS client to be passed in.
+ *
+ * For backwards compatibility with existing tests (so we don't have to change
+ * too much) the inner block is expecte to take a `TestFixture` object.
+ */
+export function withCdkApp<A extends TestContext & AwsContext>(block: (context: TestFixture) => Promise<void>) {


Very minor: Not just "with cdk app" but also "with region-based leases".
Could maybe split that into its own higher order function chain-link, or move that within withDefaultFixture().

Well this wrapper doesn't ADD a region based lease, it adds a cdk fixture.

It does REQUIRE a region lease to do that, but that's not what the name means.

The idea being that these wrappers should be freely composeable to add more setup and teardown.

…nteg-tests

mergify · 2020-09-10T13:13:47Z

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

iliapolo · 2020-09-10T13:23:59Z

@rix0rrr Added do-not-merge since it looks like the discussions are over yet?

aws-cdk-automation · 2020-09-10T14:00:04Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 9d70478
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2020-09-10T14:01:08Z

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

chore: parallelize integ tests across regions

543659c

When `AWS_REGIONS=region1,region2,region3` is set, the CLI integration tests are going to parallelize themselves across those regions.

rix0rrr requested review from iliapolo and nija-at September 9, 2020 13:42

rix0rrr self-assigned this Sep 9, 2020

mergify bot added the contribution/core This is a PR that came from AWS. label Sep 9, 2020

rix0rrr added the pr/do-not-merge This PR should not be merged at this time. label Sep 9, 2020

nija-at reviewed Sep 9, 2020

View reviewed changes

nija-at suggested changes Sep 9, 2020

View reviewed changes

rix0rrr added 2 commits September 9, 2020 15:51

Explain maximum parallelism

41e1838

Fix broken test, and add auto-bootstrap

ace047c

nija-at previously requested changes Sep 9, 2020

View reviewed changes

iliapolo reviewed Sep 9, 2020

View reviewed changes

rix0rrr added 4 commits September 9, 2020 16:28

Remove some more global state

347e38a

Update tests

b2ffbab

Unit tests for resource-pool.

19c325c

Switch to composing higher-order functions to manage lease lifetime

3f21c82

iliapolo reviewed Sep 10, 2020

View reviewed changes

nija-at approved these changes Sep 10, 2020

View reviewed changes

rix0rrr added 4 commits September 10, 2020 14:26

Merge remote-tracking branch 'origin/master' into huijbers/parallel-i…

473d680

…nteg-tests

Don't double-dispose leases

023ec1c

Oops get rid of double lease

4f9ddac

Gotta call that function

0b37008

rix0rrr removed the pr/do-not-merge This PR should not be merged at this time. label Sep 10, 2020

Merge branch 'master' into huijbers/parallel-integ-tests

471c048

iliapolo added the pr/do-not-merge This PR should not be merged at this time. label Sep 10, 2020

iliapolo approved these changes Sep 10, 2020

View reviewed changes

Be properly PROPERLY correct about the parallelism.

9d70478

rix0rrr removed the pr/do-not-merge This PR should not be merged at this time. label Sep 10, 2020

mergify bot merged commit 238d963 into master Sep 10, 2020

mergify bot deleted the huijbers/parallel-integ-tests branch September 10, 2020 14:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: parallelize integ tests across regions #10269

chore: parallelize integ tests across regions #10269

rix0rrr commented Sep 9, 2020

nija-at left a comment

nija-at Sep 9, 2020

nija-at Sep 9, 2020

rix0rrr Sep 9, 2020 •

edited

Loading

nija-at Sep 9, 2020

iliapolo Sep 10, 2020 •

edited

Loading

nija-at Sep 9, 2020

rix0rrr Sep 10, 2020

nija-at Sep 9, 2020

nija-at Sep 9, 2020

nija-at Sep 9, 2020

iliapolo Sep 9, 2020

rix0rrr Sep 10, 2020

iliapolo Sep 10, 2020

rix0rrr Sep 10, 2020

iliapolo Sep 10, 2020

iliapolo Sep 10, 2020

rix0rrr Sep 10, 2020

iliapolo Sep 10, 2020

nija-at Sep 10, 2020

rix0rrr Sep 10, 2020

rix0rrr Sep 10, 2020

nija-at Sep 10, 2020

rix0rrr Sep 10, 2020

rix0rrr Sep 10, 2020

mergify bot commented Sep 10, 2020

iliapolo commented Sep 10, 2020

aws-cdk-automation commented Sep 10, 2020

mergify bot commented Sep 10, 2020

	* anywyere, so do simple account retrieval.
	* anywhere, so do simple account retrieval.

chore: parallelize integ tests across regions #10269

chore: parallelize integ tests across regions #10269

Conversation

rix0rrr commented Sep 9, 2020

nija-at left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rix0rrr Sep 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iliapolo Sep 10, 2020 • edited Loading

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

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

mergify bot commented Sep 10, 2020

iliapolo commented Sep 10, 2020

aws-cdk-automation commented Sep 10, 2020

AWS CodeBuild CI Report

mergify bot commented Sep 10, 2020

rix0rrr Sep 9, 2020 •

edited

Loading

iliapolo Sep 10, 2020 •

edited

Loading