fix: removing only networks started by hedera instead of global prune #686

arianejasuwienas · 2024-07-03T14:58:27Z

Description:
#685 (comment)

Related issue(s):

Fixes #685

Notes for reviewer:

Checklist

Documented (Code comments, README, etc.)
Tested (unit, integration, etc.)

Signed-off-by: Mariusz Jasuwienas <[email protected]>

test/unit/utils/SafeDockerNetworkRemover.spec.ts

Signed-off-by: Mariusz Jasuwienas <[email protected]>

isavov

Nice approach and a valuable contribution, thank you so much!
Just one small thing, this approach relies on the deterministic creation of networks. Currently if we start the local node and run
docker network ls you can see there's a default network being created - its name is dependent on the name of the docker-compose project, which might change if we don't set it explicitly when bringing up the project.
That network is created here because the explorer component doesn't have an explicit network set. If all containers have an explicitly set custom network the default one will not be created. I would suggest adding it to the mirror-node network.
For extra safety it would be nice if you can add a test that verifies each service has a network set as well.

isavov · 2024-07-04T11:41:04Z

src/utils/SafeDockerNetworkRemover.ts

+  }
+
+  /**
+   * Removes one single network by its name.


This is technically not true, as it will loop through all networks that match that name filter and remove them, so for example an argument of hedera would remove all 3 networks created here, and if in the future we have a network named hedera-mirror-node-2 for example that would be removed together with hedera-mirror-node when called with the hedera-mirror-node parameter

@isavov You are correct. I have updated the comment and added a remark to the README file regarding the functionality of our script. Currently, it will remove all networks with the “hedera-” prefix.

isavov · 2024-07-04T11:43:14Z

docker-compose-state-migration.yml

@@ -356,7 +356,7 @@ services:

 networks:
  network-node-bridge:
-    name: network-node-bridge
+    name: hedera-network-node-bridge


maybe for consistency's sake all networks should have a -bridge suffix, or none of them

@isavov
I chose the latter option since there is only one network with the “bridge” suffix and a few without it.

I have only updated the name used by Docker, not the Docker Compose level alias, as changing the alias seemed to be outside the current scope.

victor-yanev · 2024-07-04T11:50:51Z

src/constants.ts

+ * Names of all the networks created by Docker Compose in this repository.
+ * Make sure to keep them up to date to always be able to remove them all using this service.
+ */
+export const NETWORK_NAMES = [


QQ: can’t we get this dynamically? It’s a pain to have to update this constant every time we make modifications to the docker-compose configurations. Also, one might not know this and may introduce bugs if he forgets to update this.

Btw, why not use docker compose down?

@victor-yanev it's called, but this is an extra cleanup step. Alternatively to avoid the need for const NETWORK_NAMES we could rely that all networks we use and only them have the hedera- prefix and remove them that way.

we could rely that all networks we use and only them have the hedera- prefix and remove them that way.

Understood, it will now function as described.

… listed in the constant Signed-off-by: Mariusz Jasuwienas <[email protected]>

… as its usage seemed to be inconsistent Signed-off-by: Mariusz Jasuwienas <[email protected]>

isavov

Do we need to add that many different networks?

isavov · 2024-07-10T11:47:15Z

docker-compose.multinode.yml

+      - multinode-importer-network
+
+networks:
+  multinode-importer-network:


do we need this extra network?

isavov · 2024-07-10T11:48:18Z

docker-compose.multinode.evm.yml

+
+networks:
+  multinode-importer-evm-network:
+    name: hedera-multinode-importer-evm


do we need a different network here

I guess not, I've replaced the extra networks with

network_mode: none

declarations.

Signed-off-by: Mariusz Jasuwienas <[email protected]>

isavov

Starting with the latest changes gives me an error, removing the offending option for this service makes the next one behave the same way and so on

service record-sidecar-uploader declares mutually exclusive `network_mode` and `networks`: invalid compose project

isavov · 2024-07-15T15:32:29Z

docker-compose.evm.yml

@@ -26,6 +30,8 @@ services:
        "echo 'Deleting old sidecars files.';find /sidecar-files/ -type f -delete;echo 'This container is intentionally disabled by the EVM profile.';"
      ]
    command: []
+    network_mode: none


When starting local node I'm getting this error

service record-sidecar-uploader declares mutually exclusive `network_mode` and `networks`: invalid compose project

Signed-off-by: Mariusz Jasuwienas <[email protected]>

isavov · 2024-07-16T08:41:07Z

docker-compose.multinode.yml

@@ -15,6 +15,7 @@ services:
      start_period: 10s
    volumes:
      - "${APPLICATION_ROOT_PATH}/config.multinode.txt:/opt/hgcapp/services-hedera/HapiApp2.0/config.txt"
+    network_mode: none


@arianejasuwienas this will cause the same issue as with the evm profile, you can test by running with
--multinode flag

Strange, no network option was present for this service. However, I've changed it. Could you please check it once again? @isavov

@arianejasuwienas I don't see the changes, can you please double check if you've pushed all changes.
Regarding the no network being defined. The way the multinode mode is constructed is that docker-compose.multinode.yml is loaded on top of docker-compose.yml and overrides the values in there. Therefore the network defined in the base file (docker-compose.yml) are used. You can gain a deeper insight in how it all works by reviewing this function here https://github.com/hashgraph/hedera-local-node/blob/40357136639d9caeb3834404ed0d3b12ab37c16f/src/services/DockerService.ts#L327-L357 as all the logic around this is defined there.

No, you're right, I've missed it before . Both

npm run start

and

npm run start --multinode

seem to work correctly at this moment.

…ces defined in docker-compose.yaml Signed-off-by: Mariusz Jasuwienas <[email protected]>

isavov

Thanks for this contribution @arianejasuwienas
Looks good to me 🙏

fix: removing only networks started by hedera instead of global prune

7dce9a2

Signed-off-by: Mariusz Jasuwienas <[email protected]>

arianejasuwienas requested review from georgi-l95, isavov, Ivo-Yankov, natanasow, konstantinabl and victor-yanev as code owners July 3, 2024 14:58

arianejasuwienas mentioned this pull request Jul 3, 2024

Stopping the dockerized local node affects unrelated networks #685

Closed

isavov reviewed Jul 4, 2024

View reviewed changes

test/unit/utils/SafeDockerNetworkRemover.spec.ts Outdated Show resolved Hide resolved

fix: typo

728fc89

Signed-off-by: Mariusz Jasuwienas <[email protected]>

isavov suggested changes Jul 4, 2024

View reviewed changes

isavov reviewed Jul 4, 2024

View reviewed changes

victor-yanev reviewed Jul 4, 2024

View reviewed changes

fix: removing all the networks with hedera-prefix instead of the ones…

1c19719

… listed in the constant Signed-off-by: Mariusz Jasuwienas <[email protected]>

arianejasuwienas requested review from a team as code owners July 7, 2024 15:11

arianejasuwienas requested review from JeffreyDallas and jeromy-cannon July 7, 2024 15:11

fix: removing the bridge suffix from one of the bridge typed networks…

a77e477

… as its usage seemed to be inconsistent Signed-off-by: Mariusz Jasuwienas <[email protected]>

isavov reviewed Jul 10, 2024

View reviewed changes

fix: removing extra networks

851de02

Signed-off-by: Mariusz Jasuwienas <[email protected]>

isavov reviewed Jul 15, 2024

View reviewed changes

fix: ignoring network in evm yaml files

358a4b6

Signed-off-by: Mariusz Jasuwienas <[email protected]>

isavov reviewed Jul 16, 2024

View reviewed changes

arianejasuwienas added 2 commits July 16, 2024 16:27

fix: ignoring network in multinode yaml files since they excend servi…

4e8569b

…ces defined in docker-compose.yaml Signed-off-by: Mariusz Jasuwienas <[email protected]>

fix: ignoring network in multinode yaml files since they excend servi…

6cfd740

…ces defined in docker-compose.yaml Signed-off-by: Mariusz Jasuwienas <[email protected]>

isavov approved these changes Jul 16, 2024

View reviewed changes

georgi-l95 approved these changes Jul 17, 2024

View reviewed changes

georgi-l95 merged commit c2a0a10 into hiero-ledger:main Jul 17, 2024
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: removing only networks started by hedera instead of global prune #686

fix: removing only networks started by hedera instead of global prune #686

arianejasuwienas commented Jul 3, 2024

isavov left a comment

isavov Jul 4, 2024

arianejasuwienas Jul 7, 2024

isavov Jul 4, 2024

arianejasuwienas Jul 7, 2024

victor-yanev Jul 4, 2024

victor-yanev Jul 4, 2024

isavov Jul 4, 2024

arianejasuwienas Jul 7, 2024

isavov left a comment

isavov Jul 10, 2024

isavov Jul 10, 2024

arianejasuwienas Jul 15, 2024

isavov left a comment

isavov Jul 15, 2024

isavov Jul 16, 2024 •

edited

Loading

arianejasuwienas Jul 16, 2024

isavov Jul 16, 2024

arianejasuwienas Jul 16, 2024

isavov left a comment

fix: removing only networks started by hedera instead of global prune #686

fix: removing only networks started by hedera instead of global prune #686

Conversation

arianejasuwienas commented Jul 3, 2024

isavov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

isavov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

isavov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

isavov Jul 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

isavov left a comment

Choose a reason for hiding this comment

isavov Jul 16, 2024 •

edited

Loading