Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Test Synapse against Complement with workers. #12810

Merged
merged 12 commits into from
May 31, 2022
Merged

Test Synapse against Complement with workers. #12810

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
be69bd2
Run Complement with workers in CI
reivilibre May 20, 2022
322d22f
Newsfile
reivilibre May 20, 2022
a974ccb
Split workers' tests by regex into 4 different jobs for now
reivilibre May 23, 2022
a2ae63e
Merge branch 'develop' into rei/complement_workers_in_ci
reivilibre May 26, 2022
9b3cdeb
Restrict Complement workers tests to develop branch
reivilibre May 31, 2022
7f9870e
Cut out the regex rubbish
reivilibre May 31, 2022
d539339
Work around the lack of YAML anchors or other easy reuse primitives, …
reivilibre May 31, 2022
50a9264
Move checkout to a script
reivilibre May 31, 2022
06b1221
Flatten develop-only
reivilibre May 31, 2022
5f1efa4
Flatten normal (reversible later)
reivilibre May 31, 2022
b998416
Fix path to script
reivilibre May 31, 2022
58819e1
Update .ci/scripts/checkout_complement.sh
reivilibre May 31, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 21 additions & 1 deletion .github/workflows/tests.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -310,6 +310,26 @@ jobs:
needs: linting-done
runs-on: ubuntu-latest

strategy:
fail-fast: false
matrix:
include:
# GHA requires all matrix configurations to have at least one value. We don't want to set one here though.
- _: monolith
reivilibre marked this conversation as resolved.
Show resolved Hide resolved

# Test with workers
- workers: workers
regex: 'A-Ea-e'

- workers: workers
regex: 'F-Lf-l'

- workers: workers
regex: 'M-Qm-q'

- workers: workers
regex: 'R-Zr-z'
Copy link
Member

Choose a reason for hiding this comment

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

I have reservations over this is a useful thing to do.

We are constrained as an organisation as to the number of GHA jobs we can run concurrently (to 60, according to https://docs.github.com/en/actions/learn-github-actions/usage-limits-billing-and-administration#usage-limits). Splitting the tests up this way does nothing to reduce the total amount of GHA worker time used by our tests, but does increase the chances of us monopolising the workers.

An alternative might be to exclude worker-mode-complement from the tests-done job, so we don't need to wait for it. But that's fiddly too.

le sigh.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, fair enough, I didn't realise we had this limit (well, I assumed there must be a limit, but I didn't know how much it was or how concerned we were about it).
You're right that this only reduces the wall time, at the cost of increasing the total consumed time (since each image has the overheads of building the Complement images, etc.).

Perhaps it may make sense, for now, to:

  • restrict these worker tests to running on develop only(?) — gives us an indication if nothing else. Also prevents us having to 'wait' for it.
  • try and get some speed improvements landed (e.g. the forking launcher, which cuts it down to ~26 minutes which is only a few minutes longer than the next slowest thing.)

Whilst testing, I've been cheating a bit and making Complement start before the lints are done; this gives it a few minutes headstart and then it's not the slowest thing on the list. If we're interested that much in shaving some time off, it could be worth considering, though I'd rather get some real time shavings if possible.

Copy link
Member

Choose a reason for hiding this comment

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

Perhaps it may make sense, for now, to:

yeah, let's try what you suggest for now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Turns out you can't use a matrix in the if block of a job (YAML anchors are no good either), so I've duplicated it for now :/

Copy link
Contributor

Choose a reason for hiding this comment

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

Turns out you can't use a matrix in the if block of a job (YAML anchors are no good either), so I've duplicated it for now :/

Are you sure about this? We have examples of doing so, e.g.

synapse/.github/workflows/latest_deps.yml

Line 58 in 57fac2a

if: ${{ matrix.postgres-version }}

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, perhaps you mean the job-level if rather than the step-level if

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Turns out you can't use a matrix in the if block of a job (YAML anchors are no good either), so I've duplicated it for now :/

Are you sure about this? We have examples of doing so, e.g.

synapse/.github/workflows/latest_deps.yml

Line 58 in 57fac2a

if: ${{ matrix.postgres-version }}

Yeah, see https:/matrix-org/synapse/actions/runs/2414412187/workflow#L310

I don't think that example is using it at the same level with the same meaning (we want to skip a job, not a step)


steps:
# The path is set via a file given by $GITHUB_PATH. We need both Go 1.17 and GOPATH on the path to run Complement.
# See https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions#adding-a-system-path
Expand Down Expand Up @@ -356,7 +376,7 @@ jobs:

- run: |
set -o pipefail
COMPLEMENT_DIR=`pwd`/complement synapse/scripts-dev/complement.sh -json 2>&1 | gotestfmt
WORKERS=${{ matrix.workers && 1 }} COMPLEMENT_DIR=`pwd`/complement synapse/scripts-dev/complement.sh -json ${{ matrix.regex && format('-run Test[{0}]', matrix.regex) || '' }} 2>&1 | gotestfmt
shell: bash
name: Run Complement Tests

Expand Down
1 change: 1 addition & 0 deletions changelog.d/12810.misc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Test Synapse against Complement with workers.