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.

Sign up for GitHub

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

Jump to bottom

[forge tests] also colocate vfns and vns for the pfn tests #9137

Merged
merged 5 commits into from
Jul 28, 2023
Merged

[forge tests] also colocate vfns and vns for the pfn tests #9137

merged 5 commits into from
Jul 28, 2023

Conversation

bchocho
Copy link
Contributor

@bchocho bchocho commented Jul 18, 2023 •
edited
Loading

Description

For the PFN tests, also colocate VFNs and VNs, and add the PFNs as a single colocated group.

Test Plan

Ensure the PFNs have the correct index (larger than the VFNs), via forge.

@bchocho bchocho changed the title Brian/pfn vfn vn [forge tests] also group vfns and vns for the pfn tests Jul 18, 2023
@bchocho bchocho changed the title [forge tests] also group vfns and vns for the pfn tests [forge tests] also colocate vfns and vns for the pfn tests Jul 18, 2023
@bchocho bchocho marked this pull request as ready for review July 18, 2023 00:32
@bchocho bchocho requested a review from JoshLind July 18, 2023 00:32
JoshLind
JoshLind reviewed Jul 18, 2023
View reviewed changes
Copy link
Contributor

@JoshLind JoshLind 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! Only concern is reducing the number of PFNs for all tests 🤔

testsuite/forge-cli/src/main.rs Outdated
@@ -1899,7 +1899,7 @@ fn pfn_const_tps(
.with_initial_validator_count(NonZeroUsize::new(7).unwrap())
.with_initial_fullnode_count(7)
.with_emit_job(EmitJobRequest::default().mode(EmitJobMode::ConstTps { tps: 100 }))
.add_network_test(PFNPerformance::new(add_cpu_chaos, add_network_emulation))
.add_network_test(PFNPerformance::new(2, add_cpu_chaos, add_network_emulation))
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we reducing this? Do you require it for more accurate mempool experiments? If so, could we make this configurable? It's helpful on the state sync side because it allows us to test more cases.

@bchocho bchocho requested a review from a team as a code owner July 18, 2023 18:25
Base automatically changed from brian/vfn-vn to main July 18, 2023 21:48
@bchocho bchocho force-pushed the brian/pfn-vfn-vn branch from 608e07c to 7786549 Compare July 19, 2023 06:20
@bchocho bchocho added the CICD:run-e2e-tests when this label is present github actions will run all land-blocking e2e tests from the PR label Jul 19, 2023
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@bchocho bchocho removed the CICD:run-e2e-tests when this label is present github actions will run all land-blocking e2e tests from the PR label Jul 20, 2023
@bchocho bchocho force-pushed the brian/pfn-vfn-vn branch from 7786549 to 712aa1e Compare July 20, 2023 20:57
@bchocho bchocho force-pushed the brian/pfn-vfn-vn branch from 712aa1e to f237f14 Compare July 27, 2023 19:53
@bchocho bchocho requested a review from JoshLind July 27, 2023 19:53
@bchocho bchocho requested a review from ibalajiarun July 27, 2023 20:28
ibalajiarun
ibalajiarun reviewed Jul 27, 2023
View reviewed changes
testsuite/testcases/src/public_fullnode_performance.rs
let validator_peer_ids = swarm.validators().map(|v| v.peer_id()).collect::<Vec<_>>();
let fullnode_peer_ids = swarm.full_nodes().map(|v| v.peer_id()).collect::<Vec<_>>();
let (vfn_peer_ids, pfn_peer_ids) =
fullnode_peer_ids.split_at(fullnode_peer_ids.len() - self.num_pfns as usize);
Copy link
Contributor

Choose a reason for hiding this comment

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

Discussed offline with Brian: all the PFNs have index set to 0, so this split will not work.

@bchocho bchocho force-pushed the brian/pfn-vfn-vn branch from 7d199d1 to 3a974b7 Compare July 27, 2023 22:19
@bchocho bchocho added the CICD:run-e2e-tests when this label is present github actions will run all land-blocking e2e tests from the PR label Jul 27, 2023
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@bchocho bchocho requested a review from ibalajiarun July 27, 2023 23:39
ibalajiarun
ibalajiarun approved these changes Jul 27, 2023
View reviewed changes
Copy link
Contributor

@ibalajiarun ibalajiarun left a comment

Choose a reason for hiding this comment

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

LGTM

@bchocho bchocho removed the CICD:run-e2e-tests when this label is present github actions will run all land-blocking e2e tests from the PR label Jul 27, 2023
@bchocho bchocho enabled auto-merge (squash) July 27, 2023 23:45
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@github-actions
Copy link
Contributor

✅ Forge suite compat success on aptos-node-v1.5.1 ==> 8a2e754549ac0542c41574c20fd6a542a4cbc1d8

Compatibility test results for aptos-node-v1.5.1 ==> 8a2e754549ac0542c41574c20fd6a542a4cbc1d8 (PR)
1. Check liveness of validators at old version: aptos-node-v1.5.1
compatibility::simple-validator-upgrade::liveness-check : committed: 4801 txn/s, latency: 6717 ms, (p50: 6600 ms, p90: 9500 ms, p99: 10500 ms), latency samples: 177640
2. Upgrading first Validator to new version: 8a2e754549ac0542c41574c20fd6a542a4cbc1d8
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 1775 txn/s, latency: 15758 ms, (p50: 19100 ms, p90: 21900 ms, p99: 22600 ms), latency samples: 92320
3. Upgrading rest of first batch to new version: 8a2e754549ac0542c41574c20fd6a542a4cbc1d8
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 1772 txn/s, latency: 16317 ms, (p50: 19000 ms, p90: 22300 ms, p99: 22600 ms), latency samples: 92160
4. upgrading second batch to new version: 8a2e754549ac0542c41574c20fd6a542a4cbc1d8
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 3227 txn/s, latency: 9470 ms, (p50: 10300 ms, p90: 12600 ms, p99: 13600 ms), latency samples: 138800
5. check swarm health
Compatibility test for aptos-node-v1.5.1 ==> 8a2e754549ac0542c41574c20fd6a542a4cbc1d8 passed
Test Ok

@github-actions
Copy link
Contributor

✅ Forge suite realistic_env_max_load success on 8a2e754549ac0542c41574c20fd6a542a4cbc1d8

two traffics test: inner traffic : committed: 6268 txn/s, latency: 6240 ms, (p50: 6000 ms, p90: 8100 ms, p99: 11700 ms), latency samples: 2720560
two traffics test : committed: 100 txn/s, latency: 3037 ms, (p50: 3000 ms, p90: 3700 ms, p99: 4100 ms), latency samples: 1860
Max round gap was 1 [limit 4] at version 1136306. Max no progress secs was 3.936949 [limit 10] at version 1136306.
Test Ok

@github-actions
Copy link
Contributor

✅ Forge suite framework_upgrade success on aptos-node-v1.5.1 ==> 8a2e754549ac0542c41574c20fd6a542a4cbc1d8

Compatibility test results for aptos-node-v1.5.1 ==> 8a2e754549ac0542c41574c20fd6a542a4cbc1d8 (PR)
Upgrade the nodes to version: 8a2e754549ac0542c41574c20fd6a542a4cbc1d8
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 4081 txn/s, latency: 7617 ms, (p50: 7000 ms, p90: 10800 ms, p99: 22900 ms), latency samples: 163240
5. check swarm health
Compatibility test for aptos-node-v1.5.1 ==> 8a2e754549ac0542c41574c20fd6a542a4cbc1d8 passed
Test Ok

@bchocho bchocho merged commit b9e1b28 into main Jul 28, 2023
@bchocho bchocho deleted the brian/pfn-vfn-vn branch July 28, 2023 00:25
gedigi pushed a commit that referenced this pull request Aug 2, 2023
@bchocho @gedigi
[forge tests] also colocate vfns and vns for the pfn tests (#9137)
b729945 
### Description

For the PFN tests, also colocate VFNs and VNs, and add the PFNs as a single colocated group.

### Test Plan

Ensure the PFNs have the correct index (larger than the VFNs), via forge.
xbtmatt pushed a commit that referenced this pull request Aug 13, 2023
@bchocho @xbtmatt
[forge tests] also colocate vfns and vns for the pfn tests (#9137)
fcec61d 
### Description

For the PFN tests, also colocate VFNs and VNs, and add the PFNs as a single colocated group.

### Test Plan

Ensure the PFNs have the correct index (larger than the VFNs), via forge.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JoshLind JoshLind JoshLind approved these changes

@ibalajiarun ibalajiarun ibalajiarun approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@bchocho @ibalajiarun @JoshLind