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

Increasing haproxy resources #4336

Merged
merged 2 commits into from Sep 28, 2022
Merged

Increasing haproxy resources #4336

merged 2 commits into from Sep 28, 2022
+353 −35

Conversation

Markuze
Copy link
Contributor

@Markuze Markuze commented Sep 19, 2022 •
edited

Description

Re-Enabling haproxy and adding more CPU+Mem to haproxy nodes.

Test Plan

  • Join AIT3 with haproxy node
  • run 100 validator forge with haproxy
  • manual testing with /testsuite/ddos/ tools.

This change is Reviewable

@Markuze Markuze added the CICD:run-e2e-tests when this label is present github actions will run all land-blocking e2e tests from the PR label Sep 19, 2022
@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
@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
@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
@Markuze
Copy link
Contributor Author

Markuze commented Sep 20, 2022

I'm somewhat surprised as already with TCP buffer values as low as 1MB HAproxy seems to keel over.

  • We should have an haproxy tag for GHA. @rustielin
  • Lets see if we can reenable haproxy.

@Markuze Markuze marked this pull request as ready for review September 20, 2022 12:20
@Markuze Markuze requested a review from a team as a code owner September 20, 2022 12:20
@Markuze
Copy link
Contributor Author

Markuze commented Sep 20, 2022

This is not actually ready for review, as we force haproxy in forge instead of doing something more graceful.

@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
perryjrandall
perryjrandall previously approved these changes Sep 20, 2022
View reviewed changes
Copy link
Contributor

@perryjrandall perryjrandall left a comment

Choose a reason for hiding this comment

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

Can you share how you're testing this?

terraform/helm/aptos-node/files/haproxy.cfg
@@ -10,7 +10,7 @@ global
nbthread 4

#4MB for client facing sndbuf/rcvbuf. -- 100Mb/s with 300 mili latency (e.g., us-asia)
tune.sndbuf.client {{ $.Values.haproxy.limits.validator.tcpBufSize }}
#tune.sndbuf.client {{ $.Values.haproxy.limits.validator.tcpBufSize }}
Copy link
Contributor

Choose a reason for hiding this comment

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

remove?

testsuite/forge.py Outdated
@@ -1199,8 +1199,8 @@ def create_forge_command(
forge_args.append("--reuse")
if forge_namespace_keep == "true":
forge_args.append("--keep")
if forge_enable_haproxy == "true":
forge_args.append("--enable-haproxy")
#if forge_enable_haproxy == "true":
Copy link
Contributor

Choose a reason for hiding this comment

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

remove this as well

@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
@github-actions

This comment has been minimized.

Sign in to view
perryjrandall
perryjrandall approved these changes Sep 27, 2022
View reviewed changes
Copy link
Contributor

@perryjrandall perryjrandall left a comment

Choose a reason for hiding this comment

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

Please add description / test plan otherwise LFG

rustielin
rustielin reviewed Sep 27, 2022
View reviewed changes
docker/compose/aptos-node/docker-compose.yaml
@@ -3,6 +3,29 @@

version: "3.8"
services:
haproxy:
image: haproxytech/haproxy-debian:2.2
Copy link
Contributor

Choose a reason for hiding this comment

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

why not this image instead? https://hub.docker.com/_/haproxy it's what we use for k8s

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This this is the image we have in aws and we used. 2.5 is the newest.

rustielin
rustielin reviewed Sep 27, 2022
View reviewed changes
testsuite/forge/src/backend/k8s/helm-values/aptos-node-default-values.yaml Outdated
# or removing it from the reference implementation entirely
haproxy:
nodeSelector:
eks.amazonaws.com/nodegroup: "validators"
Copy link
Contributor

Choose a reason for hiding this comment

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

we don't need this change anymore, now that the utilities nodepool has been updated to 8vcpu 32gb ram #4404

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed

rustielin
rustielin approved these changes Sep 27, 2022
View reviewed changes
Copy link
Contributor

@rustielin rustielin left a comment

Choose a reason for hiding this comment

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

lgtm, just few questions and nits above

@perryjrandall perryjrandall enabled auto-merge (rebase) September 27, 2022 22:22
@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
@Markuze Markuze dismissed stale reviews from davidiw and sherry-x September 28, 2022 14:40

Test running on 100 nodes

@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 843b204dce971d98449b82624f4f684c7a18b991 ==> 7eda68e1a338304eaeb3a8ca2a470d1a692bf4c0

Compatibility test results for 843b204dce971d98449b82624f4f684c7a18b991 ==> 7eda68e1a338304eaeb3a8ca2a470d1a692bf4c0 (PR)
1. Check liveness of validators at old version: 843b204dce971d98449b82624f4f684c7a18b991
compatibility::simple-validator-upgrade::liveness-check : 7496 TPS, 5125 ms latency, 7900 ms p99 latency,no expired txns
2. Upgrading first Validator to new version: 7eda68e1a338304eaeb3a8ca2a470d1a692bf4c0
compatibility::simple-validator-upgrade::single-validator-upgrade : 5308 TPS, 7166 ms latency, 9500 ms p99 latency,no expired txns
3. Upgrading rest of first batch to new version: 7eda68e1a338304eaeb3a8ca2a470d1a692bf4c0
compatibility::simple-validator-upgrade::half-validator-upgrade : 4892 TPS, 8012 ms latency, 10000 ms p99 latency,no expired txns
4. upgrading second batch to new version: 7eda68e1a338304eaeb3a8ca2a470d1a692bf4c0
compatibility::simple-validator-upgrade::rest-validator-upgrade : 7617 TPS, 4803 ms latency, 8000 ms p99 latency,no expired txns
5. check swarm health
Compatibility test for 843b204dce971d98449b82624f4f684c7a18b991 ==> 7eda68e1a338304eaeb3a8ca2a470d1a692bf4c0 passed
Test Ok

@github-actions
Copy link
Contributor

✅ Forge suite land_blocking success on 7eda68e1a338304eaeb3a8ca2a470d1a692bf4c0

performance benchmark with full nodes : 7436 TPS, 5326 ms latency, 8700 ms p99 latency,(!) expired 340 out of 3175580 txns
Test Ok

@perryjrandall perryjrandall merged commit 300206b into main Sep 28, 2022
@perryjrandall perryjrandall deleted the alexm/haproxy_juice branch September 28, 2022 15:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@perryjrandall perryjrandall perryjrandall approved these changes

@rustielin rustielin rustielin approved these changes

@zekun000 zekun000 zekun000 left review comments

@JoshLind JoshLind Awaiting requested review from JoshLind

@sitalkedia sitalkedia Awaiting requested review from sitalkedia

@davidiw davidiw Awaiting requested review from davidiw

@sherry-x sherry-x Awaiting requested review from sherry-x

Assignees

@Markuze Markuze

Labels
CICD:run-e2e-tests when this label is present github actions will run all land-blocking e2e tests from the PR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@Markuze @rustielin @davidiw @perryjrandall @zekun000 @sherry-x