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

Speed Up Publish OSS Artifacts for Cloud Workflow #19696

Merged
merged 14 commits into from
Dec 14, 2022

Conversation

git-phu
Copy link
Contributor

@git-phu git-phu commented Nov 22, 2022

What

Attempt to speed up Publish OSS Artifacts for Cloud Workflow

How

  • Use gradle test instead of gradle build to skip unnecessary steps before publishing jars
  • Do not run the buildDockerImage tasks (since this will cause the workflow to build the same images twice).
    • Since we are using docker buildx, just run copyGeneratedTar and only have buildx actually build images.

Others:

  • enable remote gradle caching
  • fix workflow concurrency to work as intended
  • fix not stopping the runner at the end of the workflow

Tests

Successful run: https://github.com/airbytehq/airbyte/actions/runs/3520087241
gw test build scan: https://scans.gradle.com/s/vfs6gdoaq3ue4/timeline

Actual Speedup:
Not very definitive since this workflow seems to have high variance in run time (can take ~18 minutes or ~28 minutes, not sure what affects this) but it seems like going from gradle build -> gradle test saves us about ~3 minutes.

Also copying this from a comment below:

looks when switching from gradle build to gradle test we save some time, but the tests still take up a large amount of the total run time. From the build scan it looks like :airbyte-webapp:test (and its dependencies) are what make the test step take so long

We could also just try merging this in and seeing what run times look like after this change

when using inputs for concurrency expression, needs to be on job level
docker buildx will actually build the images
@git-phu git-phu temporarily deployed to more-secrets November 22, 2022 04:10 Inactive
@git-phu git-phu temporarily deployed to more-secrets November 22, 2022 04:10 Inactive
@git-phu git-phu changed the title Peter/faster oss publish Speed Up Publish OSS Artifacts for Cloud Workflow Nov 22, 2022
@git-phu
Copy link
Contributor Author

git-phu commented Nov 22, 2022

@davinchia looks when switching from gradle build to gradle test we save some time, but the tests still take up a large amount of the total run time. From the build scan it looks like :airbyte-webapp:test (and its dependencies) are what make the test step take so long

@git-phu git-phu temporarily deployed to more-secrets November 22, 2022 17:03 Inactive
@git-phu git-phu temporarily deployed to more-secrets November 22, 2022 17:03 Inactive
@git-phu git-phu marked this pull request as ready for review November 22, 2022 18:03
@git-phu git-phu requested review from davinchia and a team November 22, 2022 18:04
@@ -144,3 +157,29 @@ jobs:
- name: Cleanup Docker buildx
run: docker buildx rm oss-buildx
shell: bash

stop-runner:
Copy link
Contributor

Choose a reason for hiding this comment

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

oh did we previously not stop runners?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yup, seems like a bug that went unnoticed

Copy link
Contributor

Choose a reason for hiding this comment

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

yikes - good catch

@@ -10,6 +12,9 @@ on:
required: false
jobs:
find_valid_pat:
concurrency:
Copy link
Contributor

Choose a reason for hiding this comment

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

for my understanding: what is this for?

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 is so only one instance of this workflow can be actively running for a specific oss sha (so we don't attempt to push artifacts for the same sha from multiple concurrent workflows)

Copy link
Contributor

Choose a reason for hiding this comment

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

ah ok! can we also add a comment here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added comments

Copy link
Contributor

@davinchia davinchia left a comment

Choose a reason for hiding this comment

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

Looks good!

Comments are non-blocking. So feel free to merge after they are addressed.

I'm not sure I see where the gradle remote cache is enabled in the changeset. Can you point me to it for my knowledge?

@git-phu git-phu temporarily deployed to more-secrets November 22, 2022 19:08 Inactive
@git-phu git-phu temporarily deployed to more-secrets November 22, 2022 19:08 Inactive
@git-phu git-phu temporarily deployed to more-secrets November 23, 2022 01:15 Inactive
@git-phu git-phu temporarily deployed to more-secrets November 23, 2022 01:15 Inactive
- for both publish jobs we only want to allow one running workflow per SHA.
- for stopping the runner, use the find a PAT step (job was removed)
@git-phu git-phu temporarily deployed to more-secrets November 23, 2022 01:26 Inactive
@git-phu git-phu temporarily deployed to more-secrets November 23, 2022 01:26 Inactive
@git-phu git-phu temporarily deployed to more-secrets November 23, 2022 01:27 Inactive
group: ${{ github.workflow }}-${{ inputs.oss_ref || github.sha }}

env:
S3_BUILD_CACHE_ACCESS_KEY_ID: ${{ secrets.SELF_RUNNER_AWS_ACCESS_KEY_ID }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe a comment is nice here that says something along the lines of

//these environment variables are necessary for gradle cache task

@git-phu git-phu temporarily deployed to more-secrets December 14, 2022 22:27 — with GitHub Actions Inactive
@git-phu git-phu temporarily deployed to more-secrets December 14, 2022 22:27 — with GitHub Actions Inactive
@git-phu git-phu temporarily deployed to more-secrets December 14, 2022 22:28 — with GitHub Actions Inactive
@git-phu
Copy link
Contributor Author

git-phu commented Dec 14, 2022

forgot to merge this in after the code freeze but I'll do so now

One more verification that it's still working: https://github.com/airbytehq/airbyte/actions/runs/3699244652

@git-phu git-phu merged commit 863d8da into master Dec 14, 2022
@git-phu git-phu deleted the peter/faster-oss-publish branch December 14, 2022 23:07
mfsiega-airbyte added a commit that referenced this pull request Dec 15, 2022
mfsiega-airbyte added a commit that referenced this pull request Jan 25, 2023
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