Update docker config to use docker networking instead of host networking #312

mainnet-pat · 2024-09-01T13:07:44Z

This allows us to update the docker version to latest and have no issues on macOS.

woodser · 2024-09-01T14:23:18Z

The CI tests are failing with errors: Error: Http response at 400 or 500 level, http status code: 0 https://github.com/haveno-dex/haveno-ts/pull/312/checks

woodser · 2024-09-01T20:38:52Z

I'm still getting errors on macOS with the latest docker:

  ● Can get the version (CI)

    Error: Http response at 400 or 500 level, http status code: 503

      528 |     for (const settledPromise of await Promise.allSettled(promises)) {
      529 |       if (settledPromise.status === "fulfilled") startupHavenods.push((settledPromise as PromiseFulfilledResult<HavenoClient>).value);
    > 530 |       else if (!err) err = new Error((settledPromise as PromiseRejectedResult).reason);
          |                            ^
      531 |     }
      532 |     if (err) throw err;
      533 |

      at Object.<anonymous> (src/HavenoClient.test.ts:530:28)

I'm only using docker to start the envoy service. I start the other services manually (monerod1, monerod2, funding-wallet-local, and the seed node).

But the only changes in the PR are for docker files, so I suppose something needs updated for macOS too.

woodser · 2024-09-01T20:47:01Z

Also the logs for the arbitrator, user1, and user2 are no longer collected with this PR as they are on master:

Suspecting that's because those were commented out in docker/docker-compose.yml.

mainnet-pat · 2024-09-02T05:46:43Z

That's the idea of this PR, to ditch starting all services manually with make in 4 terminals, now they are started with docker. Spawning of other processes is not scoped in this PR so the haveno compilation is still needed

mainnet-pat · 2024-09-02T05:47:55Z

What is the rationale for starting only envoy with docker. Why other services are needed there?

woodser · 2024-09-02T10:42:58Z

It’s helpful to have manual control to start, stop, modify, build, and delete the services individually throughout the development process, instead of using docker to start all services together, which can’t be easily managed individually. I found there was too much overhead for making small changes and rapidly iterating, so I do want to preserve the ability to run the services manually.

mainnet-pat · 2024-09-02T10:46:07Z

So you want to keep it as is? So I struggle to see what is actually needed?

I see pure docker setup is used for CI, but make scripts for localdev. What is the desired outcome of docker update?

mainnet-pat · 2024-09-02T10:46:48Z

I can introduce two docker files, for localdev and for CI

woodser · 2024-09-02T10:51:36Z

Updating docker makes the tests fail at least when the services are started manually (I didn't test when started with docker). So the goal is to make the tests work after updating docker, with the services started manually and with docker.

woodser · 2024-09-05T10:29:47Z

dist/HavenoClient.test.js

@@ -0,0 +1,4109 @@
+"use strict";


Please remove dist files from PR.

woodser · 2024-09-05T10:30:38Z

docker/docker-compose.yml

-    network_mode: "host"
-    depends_on:
-      - seed1
+  # arbitrator:


I saw that the log files were not collected in the CI tests for the arbitrator, user1, and user2. Guessing because these were commented out. Should be uncommented?

No, because these containers are currently unused. Tests are invoking arbitrator, user1 and user2 from haveno directory. To fully dockerize the project we'd need to invoke monerod and haveno processes with docker run.

If I'd uncomment these containers, there would be binding errors since docker and local haveno will be racing to bind to the same port. If I remove the port exposure from docker, then logs will be essentially empty (as in current master).

How can we get the logging for arbitrator, user1, and user2 to remain collected in the CI tests then? Those logs are sometimes needed for debugging.

On master, these logs are populated:

woodser · 2024-09-05T10:31:09Z

docker/docker-compose.yml

-      "--rpc-access-control-origins=http://localhost:8080",
-      "--wallet-dir=./.localnet",
+      "--rpc-access-control-origins='*'",
+      "--wallet-dir=./.localnet/funding_wallet",


Why move the funding wallet to this folder?

This is for docker to place the artifacts into proper local persistent directory

woodser · 2024-09-06T13:13:55Z

dist/HavenoClient.d.ts

@@ -1,591 +0,0 @@
-import type * as grpcWeb from "grpc-web";


No changes to dist files should be included with the PR.

woodser · 2024-09-06T13:14:54Z

docker/docker-compose.yml

-      "--add-exclusive-node=127.0.0.1:48080",
-      "--rpc-access-control-origins=http://localhost:8080",
+      "--add-exclusive-node=node1:48080",
+      "--rpc-access-control-origins='*'",


Any reason to open up the access control origins from http://localhost:8080? Thinking to keep it as is to be more representative of production.

woodser · 2024-09-06T13:15:10Z

docker/docker-compose.yml

    depends_on:
      - seed1
+    # ports:


Can these comments be removed, or they're documenting something?

mainnet-pat · 2024-09-06T17:00:17Z

Closing as incompleted.

Update docker config to use docker networking instead of host networking

9ed3386

mainnet-pat requested a review from woodser as a code owner September 1, 2024 13:07

mainnet-pat mentioned this pull request Sep 1, 2024

Update docker [2 XMR] #308

Closed

Update docker-compose.yaml

bed3636

mainnet-pat force-pushed the docker_update branch from 1a88d3b to 2673f6e Compare September 1, 2024 18:48

Fix persistent volumes

d08f671

mainnet-pat force-pushed the docker_update branch from 2673f6e to d08f671 Compare September 1, 2024 18:49

mainnet-pat requested a review from a team as a code owner September 5, 2024 10:23

woodser reviewed Sep 5, 2024

View reviewed changes

Update guide for local development

1bf6587

mainnet-pat force-pushed the docker_update branch from ecb0600 to 1bf6587 Compare September 5, 2024 11:07

mainnet-pat added 3 commits September 5, 2024 16:52

Merge branch 'master' into docker_update

7918dfa

Reenable user1, user2 and arbitrator containers

8625729

test

22ce272

woodser reviewed Sep 6, 2024

View reviewed changes

mainnet-pat closed this Sep 6, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update docker config to use docker networking instead of host networking #312

Update docker config to use docker networking instead of host networking #312

mainnet-pat commented Sep 1, 2024

woodser commented Sep 1, 2024

woodser commented Sep 1, 2024 •

edited

Loading

woodser commented Sep 1, 2024

mainnet-pat commented Sep 2, 2024 •

edited

Loading

mainnet-pat commented Sep 2, 2024

woodser commented Sep 2, 2024

mainnet-pat commented Sep 2, 2024 •

edited

Loading

mainnet-pat commented Sep 2, 2024

woodser commented Sep 2, 2024 •

edited

Loading

woodser Sep 5, 2024

mainnet-pat Sep 5, 2024

woodser Sep 5, 2024 •

edited

Loading

mainnet-pat Sep 5, 2024

woodser Sep 5, 2024

woodser Sep 5, 2024

woodser Sep 5, 2024

mainnet-pat Sep 5, 2024

woodser Sep 6, 2024

woodser Sep 6, 2024

woodser Sep 6, 2024

mainnet-pat commented Sep 6, 2024

Update docker config to use docker networking instead of host networking #312

Update docker config to use docker networking instead of host networking #312

Conversation

mainnet-pat commented Sep 1, 2024

woodser commented Sep 1, 2024

woodser commented Sep 1, 2024 • edited Loading

woodser commented Sep 1, 2024

mainnet-pat commented Sep 2, 2024 • edited Loading

mainnet-pat commented Sep 2, 2024

woodser commented Sep 2, 2024

mainnet-pat commented Sep 2, 2024 • edited Loading

mainnet-pat commented Sep 2, 2024

woodser commented Sep 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

woodser Sep 5, 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

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

mainnet-pat commented Sep 6, 2024

woodser commented Sep 1, 2024 •

edited

Loading

mainnet-pat commented Sep 2, 2024 •

edited

Loading

mainnet-pat commented Sep 2, 2024 •

edited

Loading

woodser commented Sep 2, 2024 •

edited

Loading

woodser Sep 5, 2024 •

edited

Loading