feat: node starts without checking internal hostname #12112
Conversation
Signed-off-by: Edward Wertz <[email protected]>
edward-swirldslabs
requested review from
litt3,
rbair23,
mhess-swl,
nathanklick,
lpetrovic05,
cody-littley,
poulok and
kfa-aguda
Signed-off-by: Edward Wertz <[email protected]>
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
Node: Unit Test Results 2 266 files +1 1 errors 2 265 suites +1 3h 51m 16s ⏱️ + 1h 30m 54s 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. |
Signed-off-by: Edward Wertz <[email protected]>
|
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. |
.../swirlds-platform-core/src/main/java/com/swirlds/platform/crypto/EnhancedKeyStoreLoader.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Edward Wertz <[email protected]>
|
@edward-swirldslabs , this is confusing: Environment Variable: comma separated list for the nodesToRun environment variable. It looks like the code and expectation is that you only supply a single node ID, but your example here shows multiple. |
I have updated the PR text to clarify that ServicesMain can only start 1 node per process. Browser can start multiple nodes per process. |
Signed-off-by: Nathan Klick <[email protected]>
nathanklick
dismissed stale reviews from kimbor, timo0, and lpetrovic05
via
0fb9830
nathanklick
requested review from
andrewb1269hg
and removed request for
litt3 and
cody-littley
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferencesCodacy stopped sending the deprecated coverage status on June 5th, 2024. Learn more |
Signed-off-by: Nathan Klick <[email protected]>
nathanklick
added
Feature Enhancement
| @@ -100,6 +100,10 @@ if [[ "${JCP_OVERRIDDEN}" != true && "${JAVA_MAIN_CLASS}" == "com.swirlds.platfo | |||
| JAVA_CLASS_PATH="data/lib/*" | |||
| fi | |||
|
|
|||
| # Setup Consensus Node Arguments | |||
| CONSENSUS_NODE_ARGS="" | |||
| [[ -n "${CONSENSUS_NODE_ID}" && "${CONSENSUS_NODE_ID}" -ge 0 ]] && CONSENSUS_NODE_ARGS="-local ${CONSENSUS_NODE_ID}" | |||
There was a problem hiding this comment.
is CONSENSUS_NODE_ID is not specified or less than 0 should this be treated as an error condition and not startup?
There was a problem hiding this comment.
@dalvizu It ultimately will be at the NMT level. We could add a failure condition here; however, these images are also used for older software releases which do not have the -local option.
We made it optional here for backwards compatibility with NMT 1.2.9 and 0.58.x releases.
There was a problem hiding this comment.
Ultimately, the ServicesMain entrypoint should fail fast if the required nodeId is not specified.
@edward-swirldslabs Does the ServicesMain entrypoint fail if no environment variable and no -local switch is supplied?
There was a problem hiding this comment.
@nathanklick Yes. The ServicesMain call to ensureSingleNode makes sure there is no more and no less than 1 node being started.
If no -local switch is supplied, then we're relying on the environment variable to determine the node to start. If no environment variable is set, then the node will fail to start (from ServicesMain) because its singular identity was not specified.
There was a problem hiding this comment.
@dalvizu Does this satisfy your concerns?
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.
Browserstarts all nodes locally.ServicesMainlogs an error and exits.Browserstarts just the nodes specified locallyServicesMainexits with an error indicating exactly 1 node must be started.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.
Environment Variable: comma separated list for the
nodesToRunenvironment variable.!!!! This PR impacts all deployments of nodes, from JRS, to SOLO, to DevOps managed test and production networks.
I've removed
ServicesMainTest.javaas 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.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?
Sign Offs