test!: adds the ability to set or unset bbr in e2e tests #3817

staheri14 · 2024-08-23T17:35:15Z

Closes #3815 by disabling BBR in e2e tests.

staheri14 · 2024-08-23T22:31:29Z

Both E2ESimple and MajorUpgradeToV2 tests work fine with this new change, however, MinorVersionCompatibility still fails, and I am investigating it. Feel free to comment if you see an immediate fix.

Can confirm that all e2e tests work fine.

test/e2e/testnet/node.go

…e2e-tests

…sing testnet fields

staheri14 · 2024-08-28T22:11:42Z

UPDATE [This issue is resolved, please skip the comment]

After extensive debugging on this PR, I identified the reason behind the failure of the MinorVersionCompatibility test. The issue is unrelated to the changes in this PR. Instead, it stems from certain versions of celestia-app not being available on ghcr.io (or failing to be pulled).

Context and Reproducibility:

To reproduce the issue, run the MinorVersionCompatibility test from this branch. You will notice that the second validator, node 1, running version 1.33.0 fails:

--> Running end to end tests
go run ./test/e2e MinorVersionCompatibility
test-e2e2024/08/28 15:03:42 === RUN MinorVersionCompatibility
test-e2e2024/08/28 15:03:42 Running minor version compatibility test versions v1.0.0    v1.1.0  v1.10.0 v1.10.1 v1.11.0 v1.12.0 v1.13.0 v1.14.0 v1.2.0  v1.20.0 v1.21.0 v1.22.0 v1.23.0 v1.24.0 v1.25.0 v1.26.0 v1.27.0 v1.28.0 v1.29.0 v1.3.0  v1.30.0 v1.31.0 v1.32.0 v1.33.0 v1.34.0 v1.4.0  v1.5.0  v1.6.0  v1.7.0  v1.8.0  v1.9.0  v2.0.0  v2.0.0-rc0      v2.0.0-rc1      v2.0.0-rc2      v2.0.0-rc3      v2.0.0-rc4      v2.1.0v2.1.1  v2.1.2  v2.1.2-rc0      v2.1.2-rc1
INFO[0000] Initializing knuu with scope: runMinorVersionCompatibility_20240828_150342 
INFO[0000] The .env file does not exist, continuing without loading environment variables. 
INFO[2024-08-28T15:03:42-07:00]knuu/knuu.go:264 LOG_LEVEL: info                              
test-e2e2024/08/28 15:04:18 Starting node node 0 version v1.12.0
test-e2e2024/08/28 15:04:18 Starting node node 1 version v1.33.0
test-e2e2024/08/28 15:04:18 Starting node node 2 version v2.0.0-rc0
test-e2e2024/08/28 15:04:18 Starting node node 3 version v2.0.0-rc4
test-e2e2024/08/28 15:04:18 Creating txsim
{"level":"info","name":"txsim","directory":"/var/folders/gl/8f7g64t91rxc6hsz_gj0mqfh0000gn/T/txsim","time":"2024-08-28T15:04:20-07:00","message":"txsim keyring directory created"}
{"level":"info","name":"txsim","pk":"PubKeySecp256k1{038C900FC60525F5365DDD2AC8803C9AEC1E7D76FF5797A2730A7ED8C394461C01}","time":"2024-08-28T15:04:20-07:00","message":"txsim account created and added to genesis"}
{"level":"info","name":"txsim","image":"ghcr.io/celestiaorg/txsim:pr-3541","time":"2024-08-28T15:04:20-07:00","message":"setting image for tx client"}
test-e2e2024/08/28 15:04:20 Setting up testnet
{"level":"info","name":"val0","directory":"/var/folders/gl/8f7g64t91rxc6hsz_gj0mqfh0000gn/T/val0","time":"2024-08-28T15:04:20-07:00","message":"Creating validator's config and data directories"}
{"level":"info","name":"val1","directory":"/var/folders/gl/8f7g64t91rxc6hsz_gj0mqfh0000gn/T/val1","time":"2024-08-28T15:04:20-07:00","message":"Creating validator's config and data directories"}
{"level":"info","name":"val2","directory":"/var/folders/gl/8f7g64t91rxc6hsz_gj0mqfh0000gn/T/val2","time":"2024-08-28T15:04:20-07:00","message":"Creating validator's config and data directories"}
{"level":"info","name":"val3","directory":"/var/folders/gl/8f7g64t91rxc6hsz_gj0mqfh0000gn/T/val3","time":"2024-08-28T15:04:20-07:00","message":"Creating validator's config and data directories"}
test-e2e2024/08/28 15:04:20 Starting testnet
{"level":"info","time":"2024-08-28T15:04:26-07:00","message":"forwarding ports for genesis nodes"}
{"level":"info","name":"val0","version":"v1.12.0","time":"2024-08-28T15:04:34-07:00","message":"started and ports forwarded"}
{"level":"error","error":"timeout while waiting for instance 'val1-ccd8d54e' to be running","name":"val1","version":"v1.33.0","time":"2024-08-28T15:05:34-07:00","message":"failed to start and forward ports"}
2024/08/28 15:05:34 Failed to start testnet: node val1 failed to start: timeout while waiting for instance 'val1-ccd8d54e' to be running

The logs of the validator indicate pod initialization failed which may not provide much insight, but using Lens revealed more details about the cause: Error: ImagePullBackOff meaning that the image for that version seems unavailable.

You can also reproduce the issue by running E2ESimple with the version set to v1.33.0 and will get the similar results, i.e., failure in pulling the image.

You may change the seed in the MinorVersionCompatibility test, for example, to 45. This change results in different versions being assigned to the validators, but some versions, such as v1.34.0, still fail to be pulled.

…e2e-tests

staheri14 · 2024-09-09T22:24:36Z

I ran the following tests once more after merging the main branch into this PR and can confirm they all work fine:

E2ESimple
MajorUpgradeToV2
MinorVersionCompatibility
TwoNodeSimple
TwoNodeBigBlock8MBLatency

evan-forbes

Not blocking, but I think we can simply always use the no bbr flag here

ninabarbakadze · 2024-09-10T13:21:32Z

Not blocking, but I think we can simply always use the no bbr flag here

Agreed! Might just be easier to simplify.

staheri14 · 2024-09-10T15:54:55Z

test/e2e/minor_version_compatibility.go

-		testnet.NoError("failed to create genesis node", testNet.CreateGenesisNode(v, 10000000, 0, testnet.DefaultResources))
+		testnet.NoError("failed to create genesis node",
+			testNet.CreateGenesisNode(v, 10000000, 0,
+				testnet.DefaultResources, false))


Not blocking, but I think we can simply always use the no bbr flag here

We cannot default to true i.e., disabling bbr since in the versions used in MinorVersionCompatibility, this flag is not present yet hence causing failure in the test. cc: @evan-forbes @ninabarbakadze

To elaborate on my previous comment: my perspective is that the bbr flag must be set according to the version of the celestia-app used for the validators. Since some versions support this flag while others do not, it is being made a parameter in the CreateGenesisNode function. This allows the caller, based on the version specified (the first parameter), to determine whether to enable or disable bbr.

…e2e-tests

staheri14 · 2024-09-11T17:45:56Z

The linter complaint is not caused by this PR.

staheri14 added 2 commits August 23, 2024 10:34

adds the ability to set or unset bbr in e2e tests

a55f1f4

resolves linter issues

5268516

staheri14 self-assigned this Aug 23, 2024

staheri14 added bug Something isn't working testing items that are strictly related to adding or extending test coverage labels Aug 23, 2024

staheri14 added 2 commits August 23, 2024 12:37

pass bbr flag for the versions that support it

0492a82

checks against the latest version as well

f47262b

removes print statements

c9e738f

evan-forbes previously approved these changes Aug 25, 2024

View reviewed changes

test/e2e/testnet/node.go Show resolved Hide resolved

staheri14 added 6 commits August 26, 2024 11:39

does not compare with the bbr version

64328e3

Merge remote-tracking branch 'origin/main' into sanaz/disable-bbr-in-…

6b9e4f1

…e2e-tests

adds a parameter to set bbr in the node creation methods instead of u…

9265070

…sing testnet fields

replaces equalOrHigher with isBBRCompatible

5eab0b8

disables BBR for e2e tests

fc8adfc

disables bbr on upgrade if applicable

ac5975d

staheri14 dismissed evan-forbes’s stale review via ac5975d August 28, 2024 20:04

staheri14 added 10 commits August 28, 2024 13:08

logs state of nodes while forwarding ports

ed8cd89

adds bbr option to the benchmark tests

a92ee08

removes unused enableBBR parameter from testnet.New

64cf5ec

deletes enableBBR as argument in e2e tests initialization

7f2e46f

removes an old comment

95822b7

renames enableBBR to disableBBR

25ab5a6

brings back original number of nodes

20c82d5

disables bbr in E2ESimple

366d55a

makes disableBBR parameter ineffective in the Node's Upgrade

46c5799

fixes left-over of refactoring

9769d08

staheri14 added 2 commits August 28, 2024 15:21

resolves some of the linter issues

9256ec6

adds bbr to the chain id

e1eca7b

evan-forbes previously approved these changes Aug 30, 2024

View reviewed changes

fixes a bug

7e83039

staheri14 dismissed evan-forbes’s stale review via 7e83039 August 31, 2024 01:17

celestia-bot requested a review from a team August 31, 2024 01:17

staheri14 added 2 commits August 30, 2024 18:17

removes fmt statement

92f5a66

Merge remote-tracking branch 'origin/main' into sanaz/disable-bbr-in-…

00ce90f

…e2e-tests

staheri14 requested a review from evan-forbes September 9, 2024 22:29

evan-forbes previously approved these changes Sep 10, 2024

View reviewed changes

staheri14 commented Sep 10, 2024

View reviewed changes

Merge remote-tracking branch 'origin/main' into sanaz/disable-bbr-in-…

b4687e7

…e2e-tests

celestia-bot requested a review from a team September 10, 2024 18:45

fixes some linter issues

a382bdd

staheri14 dismissed evan-forbes’s stale review via a382bdd September 10, 2024 19:05

staheri14 requested a review from liamsi as a code owner September 10, 2024 19:05

staheri14 removed the request for review from liamsi September 10, 2024 19:06

Merge branch 'main' into sanaz/disable-bbr-in-e2e-tests

50dff70

staheri14 enabled auto-merge (squash) September 11, 2024 16:56

staheri14 requested a review from evan-forbes September 11, 2024 16:56

Merge branch 'main' into sanaz/disable-bbr-in-e2e-tests

e70939a

evan-forbes approved these changes Sep 11, 2024

View reviewed changes

rootulp approved these changes Sep 11, 2024

View reviewed changes

staheri14 merged commit 2c53ddb into main Sep 11, 2024
31 of 32 checks passed

staheri14 deleted the sanaz/disable-bbr-in-e2e-tests branch September 11, 2024 19:21

This was referenced Sep 12, 2024

refactor(e2e): migrate the tests to the new knuu #3873

Merged

feat: support v3 app #3870

Merged

coderabbitai bot mentioned this pull request Oct 8, 2024

chore: pass knuu as an argument to the testnet.New #3952

Merged

coderabbitai bot mentioned this pull request Oct 23, 2024

docs: fix bbr notes #4002

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test!: adds the ability to set or unset bbr in e2e tests #3817

test!: adds the ability to set or unset bbr in e2e tests #3817

staheri14 commented Aug 23, 2024 •

edited

Loading

staheri14 commented Aug 23, 2024 •

edited

Loading

staheri14 commented Aug 28, 2024 •

edited

Loading

staheri14 commented Sep 9, 2024

evan-forbes left a comment

ninabarbakadze commented Sep 10, 2024

staheri14 Sep 10, 2024

staheri14 Sep 11, 2024

staheri14 commented Sep 11, 2024 •

edited

Loading

test!: adds the ability to set or unset bbr in e2e tests #3817

test!: adds the ability to set or unset bbr in e2e tests #3817

Conversation

staheri14 commented Aug 23, 2024 • edited Loading

staheri14 commented Aug 23, 2024 • edited Loading

staheri14 commented Aug 28, 2024 • edited Loading

staheri14 commented Sep 9, 2024

evan-forbes left a comment

Choose a reason for hiding this comment

ninabarbakadze commented Sep 10, 2024

staheri14 Sep 10, 2024

Choose a reason for hiding this comment

staheri14 Sep 11, 2024

Choose a reason for hiding this comment

staheri14 commented Sep 11, 2024 • edited Loading

staheri14 commented Aug 23, 2024 •

edited

Loading

staheri14 commented Aug 23, 2024 •

edited

Loading

staheri14 commented Aug 28, 2024 •

edited

Loading

staheri14 commented Sep 11, 2024 •

edited

Loading