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

feat: node starts without checking internal hostname #12112

Merged

andrewb1269hg merged 34 commits into main from 11751-determine-node-ids-to-start-without-network

Jan 8, 2025

Contributor

edward-swirldslabs commented Mar 13, 2024 •

edited

Loading

Must not merge until https://github.com/swirlds/swirlds-platform-regression/pull/4032 is merged

Description:
This PR removes the use of the internal ip address for determining which nodes to start. The nodes to start locally must be specified on the command line or through environment variable.

If no nodes are specified:
- Browser starts all nodes locally.
- ServicesMain logs an error and exits.
If multiple nodes are specified
- Browser starts just the nodes specified locally
- ServicesMain exits with an error indicating exactly 1 node must be started.
If specified, the commandline arguments take precedence over environment variables.

The methods for loading the cryptography now take parameters indicating which nodes will be started locally.

CLI: comma separated list at end of java command.

java <jar> -local 0,1,2

Environment Variable: comma separated list for the nodesToRun environment variable.

nodesToRun=0,1,2

!!!! This PR impacts all deployments of nodes, from JRS, to SOLO, to DevOps managed test and production networks.

To start a single node, provide a single node id through commandline or environment variable.

I've removed ServicesMainTest.java as it only tested the behavior of mismatched addresses. The logic has significantly changed. These unit tests were actually misbehaving. legacyConfigProperties was returning a null address book instead of an empty address book. All code paths are used in testing and production and will fail critically if the code paths are not working as expected. Maintaining complicated static mocked scenarios is not necessary.

HAPI tests specify nodes to run through the command line.
SOLO uses environment variables to configure nodes to run.
The Browser runs all nodes locally.
ServicesMain later ensures there is exactly 1 node running.

Related issue(s):

Fixes #11751

Testing

This was manually tested locally with the Browser and verified to work with both CLI and ENV methods. The same logic is at play in ServicesMain. The proof is when the code is executed in testing environments and it works.

Checklist

Possible Impacts?

NMT
JRS
Solo / FST
HAPI tests
Local Node Testing
Gradle Build
Perfnet Testing

Sign Offs

Services
Testing
Release Engineering
DevOps


          feat: node starts without using Netwoprk.isLocal

1c41076

Signed-off-by: Edward Wertz <edward@swirldslabs.com>

edward-swirldslabs added this to the v0.49 milestone

edward-swirldslabs self-assigned this

edward-swirldslabs requested a review from a team as a code owner

March 13, 2024 19:59

edward-swirldslabs requested a review from a team

March 13, 2024 19:59

edward-swirldslabs requested a review from a team as a code owner

March 13, 2024 19:59

edward-swirldslabs requested review from litt3, rbair23, mhess-swl, nathanklick, lpetrovic05, cody-littley, poulok and kfa-aguda

March 13, 2024 19:59


          missing newline

675ac38

Signed-off-by: Edward Wertz <edward@swirldslabs.com>

litt3 reviewed

View reviewed changes

platform-sdk/swirlds-platform-core/src/main/java/com/swirlds/platform/crypto/CryptoStatic.java Outdated Show resolved Hide resolved

platform-sdk/swirlds-platform-core/src/main/java/com/swirlds/platform/util/BootstrapUtils.java Outdated Show resolved Hide resolved

github-actions bot commented Mar 13, 2024 •

edited

Loading

Node: HAPI Test (Restart) Results

2 tests 2 ✔️ 5m 24s ⏱️
2 suites 0 💤
2 files 0 ❌

Results for commit a2a49cc.

♻️ This comment has been updated with latest results.

github-actions bot commented Mar 13, 2024 •

edited

Loading

Node: HAPI Test (Token) Results

190 tests 190 ✔️ 19m 7s ⏱️
  14 suites     0 💤
  14 files     0 ❌

Results for commit a2a49cc.

♻️ This comment has been updated with latest results.

github-actions bot commented Mar 13, 2024 •

edited

Loading

Node: HAPI Test (Crypto) Results

229 tests 229 ✔️ 28m 35s ⏱️
  23 suites     0 💤
  23 files     0 ❌

Results for commit a2a49cc.

♻️ This comment has been updated with latest results.

github-actions bot commented Mar 13, 2024 •

edited

Loading

Node: HAPI Test (Misc) Results

444 tests 434 ✔️ 38m 58s ⏱️
  77 suites   10 💤
  77 files     0 ❌

Results for commit a2a49cc.

♻️ This comment has been updated with latest results.

github-actions bot commented Mar 13, 2024 •

edited

Loading

Node: HAPI Test (Time Consuming) Results

21 tests 21 ✔️ 53m 32s ⏱️
  3 suites   0 💤
  3 files   0 ❌

Results for commit a2a49cc.

♻️ This comment has been updated with latest results.

github-actions bot commented Mar 13, 2024 •

edited

Loading

Node: HAPI Test (Smart Contract) Results

494 tests 491 ✔️ 1h 3m 10s ⏱️
  55 suites     3 💤
  55 files     0 ❌

Results for commit a2a49cc.

♻️ This comment has been updated with latest results.

github-actions bot commented Mar 13, 2024 •

edited

Loading

Node: Unit Test Results

    2 266 files +1           1 errors 2 265 suites +1 3h 51m 16s ⏱️ + 1h 30m 54s
111 102 tests +5 111 031 ✔️ +5 71 💤 ±0 0 ❌ ±0
119 544 runs +5 119 473 ✔️ +5 71 💤 ±0 0 ❌ ±0

For more details on these parsing errors, see this check.

Results for commit a2a49cc. ± Comparison against base commit 3d09dbf.

♻️ This comment has been updated with latest results.


          refactored use of nodesToStart

a2a49cc

Signed-off-by: Edward Wertz <edward@swirldslabs.com>

edward-swirldslabs requested a review from litt3

March 14, 2024 14:13

github-actions bot commented Mar 14, 2024

Node: HAPI Test (Node Death Reconnect) Results

2 tests 2 ✔️ 7m 28s ⏱️
2 suites 0 💤
2 files 0 ❌

Results for commit a2a49cc.

litt3 approved these changes

View reviewed changes

Contributor Author

edward-swirldslabs commented Mar 14, 2024 •

edited

Loading

Lots of work to do before this can be merged. This PR removes a capability without introducing the capability's replacement. JRS and other tools such as NMT will need to be re-worked to support either (1) command line identification of which node to start or (2) setting of an environment variable which indicates which node to start.

nathanklick reviewed

View reviewed changes

.../swirlds-platform-core/src/main/java/com/swirlds/platform/crypto/EnhancedKeyStoreLoader.java Outdated Show resolved Hide resolved

cody-littley approved these changes

View reviewed changes

Contributor

cody-littley left a comment

LGTM, although this should not be merged until all downstream dependents give the thumbs up.


          Updating log text

4ff5141

Signed-off-by: Edward Wertz <edward@swirldslabs.com>

edward-swirldslabs dismissed stale reviews from anthony-swirldslabs, imalygin, and alex-kuzmin-hg via

4ff5141

January 7, 2025 04:06

edward-swirldslabs requested review from imalygin, alex-kuzmin-hg, cody-littley, litt3, dalvizu and anthony-swirldslabs

January 7, 2025 04:08

anthony-swirldslabs previously approved these changes

View reviewed changes

Contributor

anthony-swirldslabs left a comment

LGTM. Thanks!


          Merge branch 'main' into 11751-determine-node-ids-to-start-without-ne…

…twork

# Conflicts:
#	hedera-node/hedera-app/src/main/java/com/hedera/node/app/info/DiskStartupNetworks.java
#	platform-sdk/swirlds-platform-core/src/main/java/com/swirlds/platform/crypto/EnhancedKeyStoreLoader.java

edward-swirldslabs dismissed anthony-swirldslabs’s stale review via

January 7, 2025 22:11

edward-swirldslabs requested a review from anthony-swirldslabs

January 7, 2025 22:11

anthony-swirldslabs previously approved these changes

View reviewed changes

nathanklick previously approved these changes

View reviewed changes

mhess-swl reviewed

View reviewed changes

Contributor

mhess-swl left a comment

Mostly looks good from services, aside from some deleted tests. There are also a couple of cosmetic changes suggested for posterity

hedera-node/hedera-app/src/main/java/com/hedera/node/app/ServicesMain.java Outdated Show resolved Hide resolved

hedera-node/hedera-app/src/main/java/com/hedera/node/app/info/DiskStartupNetworks.java Outdated Show resolved Hide resolved

hedera-node/hedera-app/src/test/java/com/hedera/node/app/ServicesMainTest.java Show resolved Hide resolved

tinker-michaelj previously approved these changes

View reviewed changes

Contributor

tinker-michaelj left a comment

I like @mhess-swl's suggested comments but otherwise LGTM, tyvm @edward-swirldslabs !


          Update hedera-node/hedera-app/src/main/java/com/hedera/node/app/Servi…

29ba5c2

…cesMain.java

Co-authored-by: Matt Hess <matt.hess@swirldslabs.com>
Signed-off-by: Edward Wertz <123979964+edward-swirldslabs@users.noreply.github.com>

edward-swirldslabs dismissed stale reviews from tinker-michaelj, nathanklick, and anthony-swirldslabs via

29ba5c2

January 8, 2025 02:26


          Update hedera-node/hedera-app/src/main/java/com/hedera/node/app/info/…

f9ea609

…DiskStartupNetworks.java

Co-authored-by: Matt Hess <matt.hess@swirldslabs.com>
Signed-off-by: Edward Wertz <123979964+edward-swirldslabs@users.noreply.github.com>

kfbr approved these changes

View reviewed changes

nathanklick approved these changes

View reviewed changes

tinker-michaelj approved these changes

View reviewed changes

Contributor

tinker-michaelj left a comment

LGTM, ty @edward-swirldslabs !

andrewb1269hg approved these changes

View reviewed changes

Contributor

andrewb1269hg left a comment

Review and approve files associated to devops-ci ownership.

andrewb1269hg merged commit 9934e12 into main

48 of 50 checks passed

andrewb1269hg deleted the 11751-determine-node-ids-to-start-without-network branch

January 8, 2025 03:34

edward-swirldslabs mentioned this pull request

Cherry pick #12112 to 0.58 #17270

Closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

mhess-swl mhess-swl left review comments

nathanklick nathanklick approved these changes

tinker-michaelj tinker-michaelj approved these changes

kfbr kfbr approved these changes

andrewb1269hg andrewb1269hg approved these changes

anthony-swirldslabs anthony-swirldslabs left review comments

poulok Awaiting requested review from poulok

rbair23 Awaiting requested review from rbair23

timfn-hg Awaiting requested review from timfn-hg

lpetrovic05 Awaiting requested review from lpetrovic05

timo0 Awaiting requested review from timo0

kimbor Awaiting requested review from kimbor

imalygin Awaiting requested review from imalygin

alex-kuzmin-hg Awaiting requested review from alex-kuzmin-hg

cody-littley Awaiting requested review from cody-littley

litt3 Awaiting requested review from litt3

dalvizu Awaiting requested review from dalvizu

Labels

Feature Enhancement P0