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

Add New Node Flow fixes #1712

Merged
merged 19 commits into from
Dec 28, 2023
Merged

Add New Node Flow fixes #1712

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
29140ba
Add New Node Flow fixes
otherview Dec 20, 2023
4720e32
fixes
otherview Dec 20, 2023
bda3bcf
node_id
otherview Dec 20, 2023
60a45ee
update gas values
otherview Dec 22, 2023
e79e913
updating gas prices
otherview Dec 22, 2023
3792c75
expand ports
otherview Dec 22, 2023
7aaafdd
bump gas
otherview Dec 26, 2023
fdb8c05
bump gas
otherview Dec 26, 2023
57dcce5
add geth checks
otherview Dec 26, 2023
a58c598
sleep between first batches
otherview Dec 26, 2023
d12a540
nits
otherview Dec 26, 2023
30f5fd8
.
otherview Dec 26, 2023
3bde2ac
weird get tx count handling
otherview Dec 26, 2023
6f9c00e
add logs around port reuse
otherview Dec 26, 2023
5d1fd19
Merge branch 'pedro/fix_build' into pedro/add_new_node_fixes
otherview Dec 27, 2023
96003ff
fix lb + test
otherview Dec 27, 2023
b6677bf
adding more details to the deploy scheme
otherview Dec 27, 2023
cbde85f
comment out the load balancer change
otherview Dec 27, 2023
db662be
Merge remote-tracking branch 'origin/main' into pedro/add_new_node_fixes
otherview Dec 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
114 changes: 60 additions & 54 deletions .github/workflows/manual-deploy-testnet-validator.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,21 @@
# This script uses GitHub Environments for variables (vars) and secrets - these are configured on GitHub and
# the environments match the input.testnet_type options
#
# # # # README # # # #
#
# 1 - Fetch the Management, Msg Bus Contract addresses and L1 Start Hash from:
# curl -X POST erpc.dev-testnet.obscu.ro:80 -H 'Content-Type: application/json' \
# -d '{"jsonrpc":"2.0","method":"obscuro_config","params":[],"id":1}'
#
# 2 - Update the secrets in Github
# ADD_NEW_NODE_L1_WS_URL -> l1 WS url
# ADD_NEW_NODE_PRIVATE_KEY -> L1 and L2 Private Key
# Note: L1 PK Must have funds in it
#
# 3 - Trigger the flow and supply data from Step 1.
# Node Id: is the node number
# Node Account Address: is the Account Address of the ADD_NEW_NODE_PRIVATE_KEY
#

name: '[M] Deploy Testnet Validator'
run-name: '[M] Deploy Testnet Validator ( ${{ github.event.inputs.testnet_type }} )'
Expand All @@ -23,18 +38,14 @@ on:
required: true
default: 3
type: number
node_private_key:
description: 'Node Private Key'
node_id:
description: 'Node id'
required: true
type: string
node_account_address:
description: 'Node Account Address'
required: true
type: string
node_l1_ws_url:
description: 'Node L1 Connection String'
required: true
type: string
MGMT_CONTRACT_ADDR:
description: 'Management Contract Addr'
required: true
Comment on lines 38 to 51
Copy link

Choose a reason for hiding this comment

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

💡 NOTE
Codebase verification is a beta feature.

The removal of HOC_ERC20_ADDR and POC_ERC20_ADDR from manual-deploy-testnet-validator.yml is confirmed. However, these variables are still referenced in .github/workflows/manual-deploy-testnet-l2.yml. This could be an oversight or intentional, depending on whether these workflows are interconnected or independent.

  • HOC_ERC20_ADDR and POC_ERC20_ADDR are still used in .github/workflows/manual-deploy-testnet-l2.yml.
Analysis chain

Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [41-60]

The declarations for node_l1_ws_url, HOC_ERC20_ADDR, and POC_ERC20_ADDR have been removed. Confirm that there are no lingering references to these variables in the workflow.

Scripts Executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check for any remaining references to the removed variables.
rg --type yaml 'node_l1_ws_url' .github/workflows/
rg --type yaml 'HOC_ERC20_ADDR' .github/workflows/
rg --type yaml 'POC_ERC20_ADDR' .github/workflows/

Length of output: 394

Expand All @@ -47,14 +58,7 @@ on:
description: 'L1 Starting Hash'
required: true
type: string
HOC_ERC20_ADDR:
description: 'HOC ERC20 Contract Addr'
required: true
type: string
POC_ERC20_ADDR:
description: 'POC ERC20 Contract Addr'
required: true
type: string


jobs:
build:
Expand Down Expand Up @@ -115,34 +119,34 @@ jobs:
with:
creds: ${{ secrets.AZURE_CREDENTIALS }}

- name: 'Create VM for Obscuro node-${{ matrix.host_id }} on Azure'
- name: 'Create VM for Obscuro node-${{ github.event.inputs.node_id }} on Azure'
uses: azure/CLI@v1
with:
inlineScript: |
az vm create -g Testnet -n "${{ vars.AZURE_RESOURCE_PREFIX }}-${{ matrix.host_id }}-${{ GITHUB.RUN_NUMBER }}" \
az vm create -g Testnet -n "${{ vars.AZURE_RESOURCE_PREFIX }}-${{ github.event.inputs.node_id }}-${{ GITHUB.RUN_NUMBER }}" \
--admin-username obscurouser --admin-password "${{ secrets.OBSCURO_NODE_VM_PWD }}" \
--public-ip-address-dns-name "obscuronode-${{ matrix.host_id }}-${{ github.event.inputs.testnet_type }}-${{ GITHUB.RUN_NUMBER }}" \
--public-ip-address-dns-name "obscuronode-${{ github.event.inputs.node_id }}-${{ github.event.inputs.testnet_type }}-${{ GITHUB.RUN_NUMBER }}" \
--tags deploygroup=ObscuroNode-${{ github.event.inputs.testnet_type }}-${{ GITHUB.RUN_NUMBER }} ${{ vars.AZURE_DEPLOY_GROUP_L2 }}=true \
--vnet-name ${{ github.event.inputs.testnet_type }}-virtual-network --subnet ${{ github.event.inputs.testnet_type }}-sub-network \
--size Standard_DC8_v2 --storage-sku StandardSSD_LRS --image ObscuroConfUbuntu \
--public-ip-sku Standard --authentication-type password

- name: 'Open Obscuro node-${{ matrix.host_id }} ports on Azure'
- name: 'Open Obscuro node-${{ github.event.inputs.host_id }} ports on Azure'
uses: azure/CLI@v1
with:
inlineScript: |
az vm open-port -g Testnet -n "${{ vars.AZURE_RESOURCE_PREFIX }}-${{ matrix.host_id }}-${{ GITHUB.RUN_NUMBER }}" --port 80,81,6060,6061,10000
az vm open-port -g Testnet -n "${{ vars.AZURE_RESOURCE_PREFIX }}-${{ github.event.inputs.node_id }}-${{ GITHUB.RUN_NUMBER }}" --port 80,81,6060,6061,10000

# To overcome issues with critical VM resources being unavailable, we need to wait for the VM to be ready
- name: 'Allow time for VM initialization'
shell: bash
run: sleep 60
Comment on lines 141 to 143
Copy link

Choose a reason for hiding this comment

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

Using sleep 60 for VM initialization could lead to flaky behavior if the VM takes longer to initialize. Consider implementing a health check loop that confirms the VM is ready.

- sleep 60
+ while ! nc -z $VM_IP $VM_PORT; do   
+   sleep 1 # wait for 1 second before check again
+ done
Committable suggestion

❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
- name: 'Allow time for VM initialization'
shell: bash
run: sleep 60
- name: 'Allow time for VM initialization'
shell: bash
run: |
while ! nc -z $VM_IP $VM_PORT; do
sleep 1 # wait for 1 second before check again
done


- name: 'Start Obscuro node-${{ matrix.host_id }} on Azure'
- name: 'Start Obscuro node-${{ github.event.inputs.node_id }} on Azure'
uses: azure/CLI@v1
with:
inlineScript: |
az vm run-command invoke -g Testnet -n "${{ vars.AZURE_RESOURCE_PREFIX }}-${{ matrix.host_id }}-${{ GITHUB.RUN_NUMBER }}" \
az vm run-command invoke -g Testnet -n "${{ vars.AZURE_RESOURCE_PREFIX }}-${{ github.event.inputs.node_id }}-${{ GITHUB.RUN_NUMBER }}" \
--command-id RunShellScript \
--scripts 'mkdir -p /home/obscuro \
&& git clone --depth 1 -b ${{ env.BRANCH_NAME }} https://github.com/ten-protocol/go-ten.git /home/obscuro/go-obscuro \
Expand All @@ -166,13 +170,13 @@ jobs:
-node_type=validator \
-is_sgx_enabled=true \
-host_id=${{ github.event.inputs.node_account_address }} \
-l1_ws_url=${{ github.event.inputs.node_l1_ws_url }} \
-l1_ws_url=${{ secrets.ADD_NEW_NODE_L1_WS_URL }} \
-management_contract_addr=${{ github.event.inputs.MGMT_CONTRACT_ADDR }} \
-message_bus_contract_addr=${{ github.event.inputs.MSG_BUS_CONTRACT_ADDR }} \
-l1_start=${{ github.event.inputs.L1_START_HASH }} \
-private_key=${{ github.event.inputs.node_private_key }} \
-private_key=${{ secrets.ADD_NEW_NODE_PRIVATE_KEY }} \
-sequencer_id=${{ vars.ACCOUNT_ADDR_NODE_0 }} \
-host_public_p2p_addr=obscuronode-${{ matrix.host_id }}-${{ github.event.inputs.testnet_type }}-${{ GITHUB.RUN_NUMBER }}.uksouth.cloudapp.azure.com:10000 \
-host_public_p2p_addr=obscuronode-${{ github.event.inputs.node_id }}-${{ github.event.inputs.testnet_type }}-${{ GITHUB.RUN_NUMBER }}.uksouth.cloudapp.azure.com:10000 \
-host_p2p_port=10000 \
-enclave_docker_image=${{ vars.L2_ENCLAVE_DOCKER_BUILD_TAG }} \
-host_docker_image=${{ vars.L2_HOST_DOCKER_BUILD_TAG }} \
Expand All @@ -185,34 +189,37 @@ jobs:
start'


update-loadbalancer:
needs:
- build
- deploy
runs-on: ubuntu-latest
environment:
name: ${{ github.event.inputs.testnet_type }}
steps:
- uses: actions/checkout@v3

- name: 'Login via Azure CLI'
uses: azure/login@v1
with:
creds: ${{ secrets.AZURE_CREDENTIALS }}

- name: 'Remove existing backend nodes from the load balancer'
run: ./.github/workflows/runner-scripts/testnet-clear-loadbalancer.sh ${{ github.event.inputs.testnet_type }}

- name: 'Add load balancer address pool to the IP configuration'
uses: azure/CLI@v1
with:
inlineScript: |
az network nic ip-config address-pool add \
--address-pool ${{ github.event.inputs.testnet_type }}-backend-pool \
--ip-config-name ipconfig${{ vars.AZURE_RESOURCE_PREFIX }}-1-${{ GITHUB.RUN_NUMBER }} \
--nic-name ${{ vars.AZURE_RESOURCE_PREFIX }}-1-${{ GITHUB.RUN_NUMBER }}VMNic \
--resource-group Testnet \
--lb-name ${{ github.event.inputs.testnet_type }}-loadbalancer
#
# Load Balancer can't be updated until the L1 and L2 is bootstrapped
#
# update-loadbalancer:
# needs:
# - build
# - deploy
# runs-on: ubuntu-latest
# environment:
# name: ${{ github.event.inputs.testnet_type }}
# steps:
# - uses: actions/checkout@v3
#
# - name: 'Login via Azure CLI'
# uses: azure/login@v1
# with:
# creds: ${{ secrets.AZURE_CREDENTIALS }}
#
# - name: 'Remove existing backend nodes from the load balancer'
# run: ./.github/workflows/runner-scripts/testnet-clear-loadbalancer.sh ${{ github.event.inputs.testnet_type }}
#
# - name: 'Add load balancer address pool to the IP configuration'
# uses: azure/CLI@v1
# with:
# inlineScript: |
# az network nic ip-config address-pool add \
# --address-pool ${{ github.event.inputs.testnet_type }}-backend-pool \
# --ip-config-name ipconfig${{ vars.AZURE_RESOURCE_PREFIX }}-${{ github.event.inputs.node_id }}-${{ GITHUB.RUN_NUMBER }} \
# --nic-name ${{ vars.AZURE_RESOURCE_PREFIX }}-${{ github.event.inputs.node_id }}-${{ GITHUB.RUN_NUMBER }}VMNic \
# --resource-group Testnet \
# --lb-name ${{ github.event.inputs.testnet_type }}-loadbalancer

check-obscuro-is-healthy:
needs:
Expand All @@ -227,5 +234,4 @@ jobs:
- name: "Wait until obscuro node is healthy"
shell: bash
run: |
./.github/workflows/runner-scripts/wait-node-healthy.sh --host=obscuronode-0-${{ github.event.inputs.testnet_type }}-${{ GITHUB.RUN_NUMBER }}.uksouth.cloudapp.azure.com
./.github/workflows/runner-scripts/wait-node-healthy.sh --host=obscuronode-1-${{ github.event.inputs.testnet_type }}-${{ GITHUB.RUN_NUMBER }}.uksouth.cloudapp.azure.com
./.github/workflows/runner-scripts/wait-node-healthy.sh --host=obscuronode-${{ github.event.inputs.node_id }}-${{ github.event.inputs.testnet_type }}-${{ GITHUB.RUN_NUMBER }}.uksouth.cloudapp.azure.com
12 changes: 6 additions & 6 deletions .github/workflows/runner-scripts/testnet-clear-loadbalancer.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,12 +43,12 @@ if [[ -z "${ipconfig_id}" ]]; then
exit 0
Copy link

Choose a reason for hiding this comment

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

Consider using a non-zero exit status when the script exits due to not finding the nic_id or ipconfig_id. Exiting with a status of 0 usually indicates success, which might not be appropriate in this context.

fi

az network nic ip-config address-pool remove \
--address-pool ${pool} \
--ip-config-name "${ipconfig_id}" \
--nic-name "${nic_id}" \
--resource-group Testnet \
--lb-name ${lb} \
#az network nic ip-config address-pool remove \
# --address-pool ${pool} \
# --ip-config-name "${ipconfig_id}" \
# --nic-name "${nic_id}" \
# --resource-group Testnet \
# --lb-name ${lb} \
Comment on lines +46 to +51
Copy link

Choose a reason for hiding this comment

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

The block of code that removes an address pool from the network interface configuration has been commented out. If this is intentional to prevent the removal for now, ensure that there is a tracking mechanism (like a TODO or a ticket) to revisit this if the functionality needs to be restored in the future.



echo 'Load balancer removed successfully'
Copy link

Choose a reason for hiding this comment

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

The message 'Load balancer removed successfully' is misleading because the code that performs the removal is commented out. This message should be updated to reflect the current behavior of the script.

Loading