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

test: minor version upgrade e2e testing #2797

Merged
merged 15 commits into from
Nov 8, 2023
Merged

Conversation

cmwaters
Copy link
Contributor

@cmwaters cmwaters commented Oct 31, 2023

Overview

Closes: #313

Ref: #2633

This PR introduces a test for checking compatibility and the running of upgrades of minor versions.

It reads all minor versions tagged in git. Each node begins on a random version.

Each node then individually performs upgrades, going down and spinning back up on the new randomly chosen version (can be downgrades as well)

Currently we only check that their is no app version mismatch and the network is able to continually build blocks

Checklist

  • New and updated code has appropriate documentation
  • New and updated code has new and/or updated testing
  • Required CI checks are passing
  • Visual proof for any user facing features like CLI or documentation updates
  • Linked issues closed with keywords

@cmwaters cmwaters self-assigned this Oct 31, 2023
Copy link
Collaborator

@rootulp rootulp left a comment

Choose a reason for hiding this comment

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

Great work! When I run this locally, I encounter:

$ make test-e2e
--> Running end to end tests
=== RUN   TestE2ESimple
    simple_test.go:34: Running simple e2e test version latest
    simple_test.go:37:
        	Error Trace:	/Users/rootulp/git/rootulp/celestia/celestia-app/test/e2e/simple_test.go:37
        	Error:      	Received unexpected error:
        	            	retrieving the Kubernetes config: stat /Users/rootulp/.kube/config: no such file or directory
        	Test:       	TestE2ESimple
--- FAIL: TestE2ESimple (0.00s)
=== RUN   TestMinorVersionCompatibility
    upgrade_test.go:27: skipping e2e test: E2E_VERSION not set
--- SKIP: TestMinorVersionCompatibility (0.00s)

What prerequisites do contributors need to perform prior to running make test-e2e locally? It looks like I need some kube config and I'm not sure why the the e2e test is getting skipped

Makefile Outdated Show resolved Hide resolved
test/e2e/simple_test.go Outdated Show resolved Hide resolved
test/e2e/upgrade_test.go Outdated Show resolved Hide resolved
test/e2e/upgrade_test.go Show resolved Hide resolved
test/e2e/versions_test.go Show resolved Hide resolved
@cmwaters
Copy link
Contributor Author

cmwaters commented Nov 1, 2023

What prerequisites do contributors need to perform prior to running make test-e2e locally? It looks like I need some kube config and I'm not sure why the the e2e test is getting skipped

Yes you need a .kube/config setup. You can ask samuel for it. Also you need the E2E environment variable to run the tests (I purposefully didn't want go test ./... to run these tests). It's best to just run make test-e2e

@rootulp
Copy link
Collaborator

rootulp commented Nov 1, 2023

  1. Can we add the .kube/config to this repo and document where maintainers should place it on their local machines? Doesn't strictly have to be in this PR.
  2. hmm I tried make test-e2e locally. I can retry after the env variable merging

@cmwaters
Copy link
Contributor Author

cmwaters commented Nov 1, 2023

Can we add the .kube/config to this repo and document where maintainers should place it on their local machines? Doesn't strictly have to be in this PR.

no because this shouldn't be public. Ideally we work out how to support locally running the tests but we don't have that at the moment

@celestia-bot celestia-bot requested a review from a team November 1, 2023 17:22
@codecov-commenter
Copy link

codecov-commenter commented Nov 1, 2023

Codecov Report

Merging #2797 (215a562) into main (8e5faa7) will increase coverage by 0.22%.
The diff coverage is 71.05%.

@@            Coverage Diff             @@
##             main    #2797      +/-   ##
==========================================
+ Coverage   19.44%   19.67%   +0.22%     
==========================================
  Files         143      144       +1     
  Lines       17320    17395      +75     
==========================================
+ Hits         3368     3422      +54     
- Misses      13633    13653      +20     
- Partials      319      320       +1     
Files Coverage Δ
test/e2e/node.go 0.00% <0.00%> (ø)
test/e2e/versions.go 76.05% <76.05%> (ø)

@rootulp
Copy link
Collaborator

rootulp commented Nov 2, 2023

no because this shouldn't be public.

[blocking] in this PR can we please indicate that make test-e2e isn't expected to work for external contributors.

Copy link
Member

@evan-forbes evan-forbes left a comment

Choose a reason for hiding this comment

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

had some questions, but its super dope we're getting to use knuu for compatibility tests

test/e2e/versions.go Outdated Show resolved Hide resolved
}
newVersion := versions.Random(r).String()
t.Log("Upgrading node", "node", i%numNodes, "version", newVersion)
err := testnet.Node(i % numNodes).Upgrade(newVersion)
Copy link
Member

Choose a reason for hiding this comment

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

does this function block until the node is back up?

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 actually don't know. It calls something in knuu (cc @smuu)

Copy link
Member

Choose a reason for hiding this comment

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

Yes the current implementation blocks until the node is back up

test/e2e/upgrade_test.go Outdated Show resolved Hide resolved
test/e2e/upgrade_test.go Outdated Show resolved Hide resolved
Comment on lines +75 to +79
// FIXME: skip the first node because we need them available to
// submit txs
if i%numNodes == 0 {
continue
}
Copy link
Member

Choose a reason for hiding this comment

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

is skipping the first node also the mechanism that is testing that the versions are compatible?

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 don't quite understand. We need the first node to always be up because we're constantly submitting transactions. If txsim can't submit a transaction it exits.

Copy link
Member

Choose a reason for hiding this comment

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

I see now that the test has been updated for each upgraded node to wait a few heights. I was curious what the mechanism was to ensure that different versions were running concurrently

test/e2e/upgrade_test.go Show resolved Hide resolved
test/e2e/upgrade_test.go Show resolved Hide resolved
Co-authored-by: Evan Forbes <42654277+evan-forbes@users.noreply.github.com>
@celestia-bot celestia-bot requested a review from a team November 2, 2023 16:08
@rootulp rootulp self-requested a review November 6, 2023 12:02
Copy link
Collaborator

@rootulp rootulp left a comment

Choose a reason for hiding this comment

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

When I run this locally, I observe:

  1. [not blocking] verbose logging from test e2e simple. Can we please redirect that logging so that the test results are easier to read?
  2. [blocking] TestMinorVersionCompatibility gets skipped. TBH not sure why b/c the Makefile looks correct but my shell doesn't ever have these values populated.
$ make test-e2e
--> Running end to end tests
=== RUN   TestE2ESimple
    simple_test.go:34: Running simple e2e test version latest
< verbose logging from test e2e simple> 
--- PASS: TestE2ESimple (318.32s)
=== RUN   TestMinorVersionCompatibility
    upgrade_test.go:29: skipping e2e test: E2E_VERSION not set
--- SKIP: TestMinorVersionCompatibility (0.00s)
=== RUN   TestVersionParsing
--- PASS: TestVersionParsing (0.00s)
=== RUN   TestFilterMajorVersions
--- PASS: TestFilterMajorVersions (0.00s)
=== RUN   TestOrder
--- PASS: TestOrder (0.00s)
=== RUN   TestOrderOfReleaseCandidates
--- PASS: TestOrderOfReleaseCandidates (0.00s)
PASS
ok  	github.com/celestiaorg/celestia-app/test/e2e	318.931s
$ echo $E2E_VERSIONS

$ echo $E2E_VERSION

Makefile Outdated Show resolved Hide resolved
test/e2e/upgrade_test.go Outdated Show resolved Hide resolved
@cmwaters
Copy link
Contributor Author

cmwaters commented Nov 6, 2023

[not blocking] verbose logging from test e2e simple. Can we please redirect that logging so that the test results are easier to read?

Yeah I can remove that. That was initially just to let me know it was working but should be fine to silence them now

[blocking] TestMinorVersionCompatibility gets skipped. TBH not sure why b/c the Makefile looks correct but my shell doesn't ever have these values populated.

Let me look into it

Co-authored-by: Rootul P <rootulp@gmail.com>
@celestia-bot celestia-bot requested a review from a team November 6, 2023 13:36
evan-forbes
evan-forbes previously approved these changes Nov 6, 2023
Copy link
Member

@evan-forbes evan-forbes left a comment

Choose a reason for hiding this comment

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

lgtm 👍

evan-forbes
evan-forbes previously approved these changes Nov 6, 2023
@cmwaters
Copy link
Contributor Author

cmwaters commented Nov 6, 2023

@rootulp do you want to try it now

Copy link
Collaborator

@rootulp rootulp left a comment

Choose a reason for hiding this comment

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

testing upgrade test now and it appears to be running 👍

Update: I quit after 10 mins b/c it didn't seem to be making progress

$ make test-e2e
--> Running end to end tests
=== RUN   TestE2ESimple
    simple_test.go:25:
--- SKIP: TestE2ESimple (0.00s)
=== RUN   TestMinorVersionCompatibility
versions v1.0.0	v1.1.0	v1.2.0	v1.3.0    upgrade_test.go:40: Running minor version compatibility test versions v1.0.0	v1.1.0	v1.2.0	v1.3.0
    upgrade_test.go:58: Starting node node 0 version v1.1.0
    upgrade_test.go:58: Starting node node 1 version v1.3.0
    upgrade_test.go:58: Starting node node 2 version v1.0.0
    upgrade_test.go:58: Starting node node 3 version v1.2.0








^Csignal: interrupt
FAIL	github.com/celestiaorg/celestia-app/test/e2e	585.399s
make: *** [test-e2e] Error 1

test/e2e/simple_test.go Outdated Show resolved Hide resolved
test/e2e/versions_test.go Outdated Show resolved Hide resolved
@celestia-bot celestia-bot requested a review from a team November 6, 2023 14:36
Copy link
Member

@smuu smuu left a comment

Choose a reason for hiding this comment

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

This looks great!

@celestia-bot celestia-bot requested a review from a team November 8, 2023 15:06
@cmwaters cmwaters merged commit 75f9393 into main Nov 8, 2023
29 checks passed
@cmwaters cmwaters deleted the cal/minor-upgrade-testing branch November 8, 2023 15:58
@rootulp rootulp mentioned this pull request Jan 8, 2024
4 tasks
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.

Version compatibility testing
5 participants