Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Turn on test sharding for k8s and docker-build #1058

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

blampe
Copy link
Contributor

@blampe blampe commented Aug 21, 2024

This turns on test sharding for the docker-build and p-k providers.

The actual logic for how we discover tests and assign them is here, along with some test cases if there are correctness concerns.

Results for p-k:

Total billable time increased by 35m, which roughly lines up with the fixed setup time we use on the 5 additional shards.

In terms of onboarding, if a provider wants to use this they should:

  • Ensure a make install_sdks target exists, because each shard now needs all the SDKs.
  • Remove any build tags as part of updating the CI workflow (e.g. testing pulumi-kubernetes#3175). The test command on CI no longer uses build tags to shard, and you don't want tests excluded due to missing tags.
  • (Optional) Ensure tests have globally unique names. Running go run github.com/blampe/shard@latest --total 1 --index 0 will emit a warning for any tests with name collisions. Due to the way we're invoking go test -run (names...) packages..., it's possible for a test to get re-run on multiple shards if its name isn't unique, but this isn't the end of the world.

Refs #676

Comment on lines +500 to +501
run: 'for f in *.tar.gz; do tar -zxf "$f" -C "${f%.tar.gz}"; done',
"working-directory": "${{ github.workspace}}/sdk",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This relaxes the logic to unpack any SDK tarballs that happen to be present, not just a language-specific tarball.

Comment on lines +472 to +483
if (sharded) {
return ["nodejs", "go", "dotnet", "python", "java"].map((language) => {
return {
name: `Download ${language} SDK`,
uses: action.downloadArtifact,
with: {
name: `${language}-sdk.tar.gz`,
path: "${{ github.workspace}}/sdk/",
},
};
});
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Each shard now needs every SDK.

@@ -314,7 +314,7 @@ export function InstallPythonDeps(): Step {
export function InstallSDKDeps(): Step {
return {
name: "Install dependencies",
run: "make install_${{ matrix.language}}_sdk",
run: "make install_sdks",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is supported by our native providers, but I could also add a conditional here to be safe.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think neither pulumi-kubernetes nor pulumi-docker-build have a install_sdks entrypoint currently?

@@ -314,7 +314,7 @@ export function InstallPythonDeps(): Step {
export function InstallSDKDeps(): Step {
return {
name: "Install dependencies",
run: "make install_${{ matrix.language}}_sdk",
run: "make install_sdks",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think neither pulumi-kubernetes nor pulumi-docker-build have a install_sdks entrypoint currently?

name: "Run tests",
run: testCmd,
};
const shardCmd = `echo go test -v -count=1 -cover -timeout 2h -parallel 4${shortMode} "$(go run github.com/blampe/shard@b251cf8da6a83022628496125f285095772ebe86 --total \${{ strategy.job-total }} --index \${{ strategy.job-index }})" > test-command`;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we plan on movinggithub.com/blampe/shard to a Pulumi controlled repo?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps this should be an inline script so it's versioned per-provider via ci-mgmt? If not, then we should pin it.

Copy link
Member

@danielrbradley danielrbradley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Following up on this following the work on the generic provider template (#1140) and to clear this off my review backlog 😅 ...

I think we should hold off on adding the sharding here and instead focus on the generic template and moving the native providers to that.

Just to mirror the comments from elsewhere:

  • In order to get this live, we should create a version of the shard program in the pulumi organisation.
  • How are we planning to upgrade the version of the shard tool that we depend on?
  • In the implementation of the shard tool - is there any way to leverage Go's own test discovery process so we're sure the tool behaves consistently with running go test?
  • Related: I think there's potentially an issue here around when there's modules within modules.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants