-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
[Github Actions] Add new workflows for fullnode syncing. #6352
Conversation
2437188
to
8916a3c
Compare
b2fa21d
to
5bf673b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome work! Generally LGTM, but some nits on the structure of the composite action, as well as to split out the main action to a separate script
@@ -0,0 1,85 @@ | |||
# This action runs a public fullnode using a specified branch ($BRANCH), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I would make the environment variables BRANCH
, NETWORK
, BOOTSTRAPPING_MODE
, etc into action inputs, which makes it slightly more modular and clearer to the user what's required. Also, it makes it easier later on to move all these actions to a separate repo! Here's an example (from me moving out the replay-verify job) : https://github.com/aptos-labs/actions/blob/0a510a8f270c9b5093c3e79043f4613dbb07aeb3/replay-verify/action.yml
15986bc
to
158f45c
Compare
158f45c
to
052a4bc
Compare
GIT_SHA: | ||
required: false | ||
type: string | ||
description: The git SHA1 to test. If not specified, Forge will check the latest commits on the current branch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this input is not actually used -- can you remove it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copy/pasta magic that I thought might be used. Removed from them all. 😄
Thanks @rustielin! Made a few tweaks:
|
# Clone the aptos-networks repository at the same place as aptos-core | ||
current_working_directory = os.getcwd() | ||
os.chdir("..") | ||
subprocess.run(["git", "clone", "https://github.com/aptos-labs/aptos-networks"]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: instead of git clone, you can prob just get it from the raw paths here: e.g. https://raw.githubusercontent.com/aptos-labs/aptos-networks/main/mainnet/waypoint.txt
That way, you can get around having to do git magic and chdir ..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! So much simpler 😄
def ping_rest_api_index_page(rest_endpoint, exit_if_none): | ||
"""Pings and returns the index page from a REST API endpoint""" | ||
# Ping the REST API index page | ||
process = subprocess.Popen(["curl", rest_endpoint], stdout=subprocess.PIPE, stderr=subprocess.PIPE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instead of curl
, probably use https://pypi.org/project/requests/, or at least do curl -s
Looks like job output gets noisy with
b''
Found output on stderr for ping_rest_api_index_page: b' % Total % Received % Xferd Average Speed Time Time Time Current\n Dload Upload Total Spent Left Speed\n\r 0 0 0 0 0 0 0 0 --:--:-- --:--:-- --:--:-- 0\r100 221 100 221 0 0 215k 0 --:--:-- --:--:-- --:--:-- 215k\n'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, thanks! Just gone for curl -s
for now 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works as intended and LGTM! Just a few nits, but stamping to unblock. Let's get this landed as soon as possible!
94035ae
to
58cbbb1
Compare
58cbbb1
to
3c3ee99
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is incredible. I saw the max time is 5 hours. How long does the sync take today? In future could this be the basis of a regression test?
3c3ee99
to
6243dad
Compare
They are currently a mixture, e.g., 25 mins to 2 hours. I think in the future, we could make the shortest test land blocking if we wanted to, e.g., devnet fast sync :) But, I'd like these to stabilize first. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
✅ Forge suite
|
✅ Forge suite
|
Description
This PR adds nightly Github action workflows to verify fullnode health. Specifically, we add the following workflows:
main
branch, connect it todevnet
and verify it can sync using execution syncing.main
branch, connect it totestnet
and verify it can sync using fast syncing.main
branch, connect it tomainnet
and verify it can sync using output syncing.main
branch, connect it tomainnet
and verify it can sync using fast syncing.main
branch, connect it todevnet
and verify it can sync using intelligent syncing mode.The workflows will spawn and monitor the node and wait for it to sync to a target version (where the target is the latest version of the blockchain plus some delta). If this fails, the workflow will fail.
We'll likely add more of these tests in the future, but let's see how these do.
Test Plan
Manual verification.