[CI][Testing] Add BuildJet CI runners for unit tests, integration tests; cache Docker images #4906

gomisha · 2023-10-30T21:30:07Z

This PR encompasses many features / improvements to CI and flaky tests in order to

reduce CI run times (it now takes about 15 minutes for a clean CI run)
reduce test flakiness in CI so that it's less necessary to re-run CI jobs in order to merge PRs

Major Features

added managed CI runners from BuildJet to allow running tests on larger hardware; granularity is at the job level so different jobs have the choice of running different custom runners or default runners
caching Docker images used for integration tests in new CI job (Docker Build) on a powerful BuildJet runner and all the integration test jobs load the Docker images from cache - this eliminates the need for each integration test job to build Docker images; this job takes about 5 minutes to build and cache all Docker images

Custom CI Runners

replaced default GitHub runners with custom BuildJet runners for 13 jobs (6 unit tests, 2 module unit tests, Docker Build job, 4 integration tests)

Unit Tests Improvements

added ability to specify non-default CI runners to any unit test job package or sub-package with colon notation (e.g. engine/execution:buildjet-4vcpu-ubuntu-2204, network/test:buildjet-16vcpu-ubuntu-2204)
added ability to specify sub-sub packages (any nesting level) of a parent package to be tested in a separate job, while still being able to test all the remaining packages of the parent package in a separate job
- engine job and all it's sub-packages are good examples of how this works:
- engine/access engine/collection engine/common engine/consensus engine/execution/ingestion:buildjet-8vcpu-ubuntu-2204 engine/execution/computation engine/execution engine/verification engine:buildjet-4vcpu-ubuntu-2204
extraced state and storage packages out of others into separate jobs since others job was becoming too big

Unit Tests Splits (28 CI jobs)

split up engine unit tests into multiple smaller jobs (some with custom runners) because the single engine job was very large and flaky:
- engine/access engine/collection engine/common engine/consensus engine/execution/ingestion:buildjet-8vcpu-ubuntu-2204 engine/execution/computation engine/execution engine/verification engine:buildjet-4vcpu-ubuntu-2204
network tests further split out into network/alsp network/test/cohort1:buildjet-16vcpu-ubuntu-2204 network/test/cohort2:buildjet-4vcpu-ubuntu-2204 network/p2p/connection network/p2p/p2pnode:buildjet-4vcpu-ubuntu-2204 network/p2p/scoring network/p2p network
module job split up into module/dkg, module

Integration Tests (15 CI jobs)

updated integration test jobs to allow specifying custom runner
split up access integration tests (integration/tests/access/access_api_test.go) into smaller tests because the job was very large and flaky

Quarantined Flaky Tests

The following tests were quarantined because they kept failing / being flaky even with increased CI runners:

~~TestEpochJoinAndLeaveVN (integration/tests/epochs/cohort2/epoch_join_and_leave_vn_test.go)~~ - waiting for [Flaky Test] Modifies block rate in VN test to address sealing lagging finalization #4975 to be merged so can unquarantine this test
TestSealingAndVerificationPassThrough (integration/tests/bft/framework/passthrough_test.go)
TestUnicastRateLimit_Messages (network/test/cohort1/network_test.go)

Unquarantined Tests

The following tests were removed from quarantine because now we can run them on larger runners and they don't fail in CI. They were running without problems locally but used to fail in CI:

TestMeshNetTestSuite (network/test/cohort1/meshengine_test.go)
TestEchoEngineTestSuite (network/test/cohort2/echoengine_test.go)
network/p2p/p2pnode/resourceManager_test.go increased to full load test - previously this test had to operate at about 10% load capacity in order to pass in CI but now it's running on a larger runner and can run at full load

Other

max retries for unit and integration tests were increased from 3 to 5 and the timeout was increased from 25 to 35 minutes to allow more attempts for CI to run before having to manually re-try
removed localnet-test CI job as it's not relevant anymore and it was taking a long time to run

ref: https://github.com/dapperlabs/flow-go/issues/6894

codecov-commenter · 2023-11-01T21:37:10Z

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (6f601e4) 55.92% compared to head (d9d1b42) 56.11%.
Report is 42 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4906      +/-   ##
==========================================
+ Coverage   55.92%   56.11%   +0.19%     
==========================================
  Files         963      965       +2     
  Lines       89617    89689      +72     
==========================================
+ Hits        50115    50331     +216     
+ Misses      35752    35606     -146     
- Partials     3750     3752       +2

Flag	Coverage Δ
unittests	`56.11% <96.66%> (+0.19%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
network/test/cohort2/echoengine.go	`79.71% <ø> (ø)`
utils/test_matrix/test_matrix.go	`64.70% <96.66%> (+7.56%)`	⬆️

... and 21 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…ation tests)

sideninja

Awesome

sideninja · 2023-11-09T17:22:37Z

Do we know what is the new runtime approx with this runners?

gomisha · 2023-11-09T17:35:06Z

Do we know what is the new runtime approx with this runners?

Yes - usually about 15 minutes but sometimes takes longer (if needed to retry some jobs) up to about 25 minutes.

ramtinms · 2023-11-09T17:41:36Z

integration/tests/epochs/cohort2/epoch_join_and_leave_vn_test.go

+	unittest.SkipUnless(s.T(), unittest.TEST_TODO, "flaky")
 	s.RunTestEpochJoinAndLeave(flow.RoleVerification, s.AssertNetworkHealthyAfterVNChange)


I think I saw @jordanschalm fixing this test in another PR.

Yes, I will remove this from quarantine once his PR is merged.

#4975

jordanschalm

Awesome, great work Misha.

jordanschalm · 2023-11-09T21:21:22Z

utils/test_matrix/test_matrix.go

@@ -72,8 +80,10 @@ func generateTestMatrix(targetPackages map[string][]string, otherPackages []stri

 // listTargetPackages returns a map-list of target packages to run as separate CI jobs, based on a list of target package prefixes.
 // It also returns a list of the "seen" packages that can then be used to extract the remaining packages to run (in a separate CI job).
-func listTargetPackages(targetPackagePrefixes []string, allFlowPackages []string) (map[string][]string, map[string]string) {
+func listTargetPackages(targetPackagePrefixes []string, allFlowPackages []string) (targets, map[string]string) {
+	//func listTargetPackages(targetPackagePrefixes []string, allFlowPackages []string) (map[string][]string, map[string]string) {


Suggested change

//func listTargetPackages(targetPackagePrefixes []string, allFlowPackages []string) (map[string][]string, map[string]string) {

jordanschalm · 2023-11-09T21:24:01Z

integration/tests/epochs/cohort2/epoch_join_and_leave_vn_test.go

@@ -34,5 +36,6 @@ func (s *EpochJoinAndLeaveVNSuite) SetupTest() {
 // TestEpochJoinAndLeaveVN should update verification nodes and assert healthy network conditions
 // after the epoch transition completes. See health check function for details.
 func (s *EpochJoinAndLeaveVNSuite) TestEpochJoinAndLeaveVN() {
+	unittest.SkipUnless(s.T(), unittest.TEST_TODO, "flaky")


Suggested change

unittest.SkipUnless(s.T(), unittest.TEST_TODO, "flaky")

can remove after merging with master :)

Unquarantined! Thanks for the fix!

b60901b

jordanschalm · 2023-11-09T21:25:44Z

.github/workflows/flaky-test-monitor.yml

+#  push:
+#    paths:
+#      - '.github/workflows/flaky-test-monitor.yml'
+#      - '.github/workflows/ci.yml'


Should this be un-commented or removed?

Thanks. Flaky Test Monitor CI workflow needs to have a major overhaul now that the main CI workflow uses custom runners and Docker caching. I will address that in a separate PR.

6761410

#4975

…ow/flow-go into misha/6894-buildjet-ci-test

[CI][Testing] Add BuildJet CI runners for unit tests, integration tests; cache Docker images

gomisha added 7 commits October 30, 2023 16:25

unit tests, integration tests using buildjet-4vcpu runner

bc5e7ef

listTargetPackages() returns map of CI runners

fb69048

test for custom runners

291e71d

generateTestMatrix() stores CI runner

e7a7f7e

more testing with custom CI runners

371e4b3

CI test - 2 unit tests running buildjet runners

00117a4

increase engine unit tests to 8 vCPUs

6a25c05

gomisha added 22 commits November 2, 2023 09:34

resource manager full load test (8 vCPUs)

0a3854b

network/test tests unquarantined, increased runners

983e488

Merge branch 'master' into misha/6894-buildjet-ci-test

0b422d5

increased to 16 vCPUs

6048198

increased engine tests to 16 vCPUs

eb81e79

insecure module uses buildjet 4 vcpu runner

d496e7f

bft framework integration test using buildjet

d320b6b

engine tests split up to 6 jobs with default runners

620d6a1

added back all integration tests

daeac5f

upgraded runners for some integration tests, engine/execution

98c5197

dummy commit to kick off CI

1259067

split up TestScriptExecutionAndGetAccounts into 3 tests

959bf17

updated runners for Epoch Cohort2, Access Integration Tests

027513c

AN integration tests split up into 3 cohorts (skip unit, other integr…

e6a5ef3

…ation tests)

docker build using GitHub cache

10c9edd

remove saving Docker image locally

c589487

cache v3, saving local tar before caching, 8vcpu

5998a0d

add 11 docker images to docker-images.tar, 16 vcpu

9f86a31

docker save multiline fix; skip localnet-test

ba1b00f

docker build increase to 32 vcpu

544b877

buildjet cache instead of GitHub cache

99a9b9e

lint fix

0e4d303

gomisha added 2 commits November 9, 2023 10:52

module job increased to 4 vcpu

47e71f9

Merge branch 'master' into misha/6894-buildjet-ci-test

9f889cc

gomisha marked this pull request as ready for review November 9, 2023 17:07

gomisha requested a review from yhassanzadeh13 as a code owner November 9, 2023 17:07

gomisha requested review from jordanschalm, sideninja, yhassanzadeh13, ramtinms, kc1116 and tarakby and removed request for yhassanzadeh13 November 9, 2023 17:13

sideninja approved these changes Nov 9, 2023

View reviewed changes

ramtinms reviewed Nov 9, 2023

View reviewed changes

ramtinms approved these changes Nov 9, 2023

View reviewed changes

yhassanzadeh13 approved these changes Nov 9, 2023

View reviewed changes

jordanschalm approved these changes Nov 9, 2023

View reviewed changes

gomisha added 5 commits November 9, 2023 16:36

unquarantined epoch flaky test after it was fixed

b60901b

#4975

unlink flaky test monitor workflow from running when ci changes

6761410

Merge branch 'misha/6894-buildjet-ci-test' of https://github.com/onfl…

131490c

…ow/flow-go into misha/6894-buildjet-ci-test

clean up

8d6dfe4

lint fix

d9d1b42

gomisha added this pull request to the merge queue Nov 10, 2023

Merged via the queue into master with commit 72e9efd Nov 10, 2023
54 checks passed

gomisha deleted the misha/6894-buildjet-ci-test branch November 10, 2023 14:53

gomisha mentioned this pull request Nov 14, 2023

[Testing] Fix Flaky Test Monitor CI workflow and Grafana dashboards #5014

Closed

2 tasks

peterargue pushed a commit that referenced this pull request Nov 14, 2023

Merge pull request #4906 from onflow/misha/6894-buildjet-ci-test

7ea0227

[CI][Testing] Add BuildJet CI runners for unit tests, integration tests; cache Docker images

peterargue mentioned this pull request Nov 14, 2023

[CI][Testing] Add BuildJet CI runners for unit tests, integration tests; cache Docker images - backport #5015

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CI][Testing] Add BuildJet CI runners for unit tests, integration tests; cache Docker images #4906

[CI][Testing] Add BuildJet CI runners for unit tests, integration tests; cache Docker images #4906

gomisha commented Oct 30, 2023 •

edited

Loading

codecov-commenter commented Nov 1, 2023 •

edited

Loading

sideninja left a comment

sideninja commented Nov 9, 2023

gomisha commented Nov 9, 2023

ramtinms Nov 9, 2023

gomisha Nov 9, 2023

jordanschalm left a comment

jordanschalm Nov 9, 2023

gomisha Nov 9, 2023

jordanschalm Nov 9, 2023

gomisha Nov 9, 2023

jordanschalm Nov 9, 2023

gomisha Nov 9, 2023

		unittest.SkipUnless(s.T(), unittest.TEST_TODO, "flaky")
		s.RunTestEpochJoinAndLeave(flow.RoleVerification, s.AssertNetworkHealthyAfterVNChange)

[CI][Testing] Add BuildJet CI runners for unit tests, integration tests; cache Docker images #4906

[CI][Testing] Add BuildJet CI runners for unit tests, integration tests; cache Docker images #4906

Conversation

gomisha commented Oct 30, 2023 • edited Loading

Major Features

Custom CI Runners

Unit Tests Improvements

Unit Tests Splits (28 CI jobs)

Integration Tests (15 CI jobs)

Quarantined Flaky Tests

Unquarantined Tests

Other

codecov-commenter commented Nov 1, 2023 • edited Loading

Codecov Report

sideninja left a comment

Choose a reason for hiding this comment

sideninja commented Nov 9, 2023

gomisha commented Nov 9, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jordanschalm 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

gomisha commented Oct 30, 2023 •

edited

Loading

codecov-commenter commented Nov 1, 2023 •

edited

Loading