Replace host ZK with container

[worxli]

Fixes #2013

---

This change is 

[lukedirtwalker]

Reviewed 15 of 15 files at r1.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @worxli)

---

docker.sh, line 97 at r1 (raw file):

    args+=" -u root"
    args+=" --add-host=docker0:$(./tools/docker-ip)"
    args+=" -e DOCKER0=$(./tools/docker-ip)"

Maybe we could run docker-ip just once and save it in a variable?

---

scion.sh, line 62 at r1 (raw file):

        local addr="127.0.0.1:2181"
        if is_running_in_docker; then
            addr="172.17.0.1:2182"

shouldn't this come from docker-ip ? below as well?

[worxli]

Reviewable status: 13 of 15 files reviewed, 2 unresolved discussions (waiting on @lukedirtwalker and @worxli)

---

docker.sh, line 97 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

Maybe we could run docker-ip just once and save it in a variable?

Done.

---

scion.sh, line 62 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

shouldn't this come from docker-ip ? below as well?

This one can't because docker0 doesn't exist in the container. But as a matter of fact there is an env var with the IP. And yes, for the next one it should be docker-ip.

[lukedirtwalker]

🏷️

Reviewed 2 of 2 files at r2.
Reviewable status: complete! all files reviewed, all discussions resolved

[lukedirtwalker]

Reviewable status: complete! all files reviewed, all discussions resolved

[kormat]

Reviewed 6 of 15 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @worxli)

---

README.md, line 88 at r2 (raw file):

   The default topology looks like [this](doc/fig/default_topo.png).

3. Run the infrastructure:

Don't change the numbers - markdown does auto-numbering.

[worxli]

Reviewable status: 7 of 18 files reviewed, 1 unresolved discussion (waiting on @kormat, @lukedirtwalker, and @worxli)

---

README.md, line 88 at r2 (raw file):

Previously, kormat (Stephen Shirley) wrote…

Don't change the numbers - markdown does auto-numbering.

Done.

[worxli]

 zookeeper:
    container_name: zookeeper
    image: zookeeper:latest
    environment:
      - ZOO_MAX_CLIENT_CNXNS=0
    tmpfs:
      - /datalog
    ports:
    - 2181:2181

Logs: docker logs zookeeper >& logs/zookeeper.log

Reviewable status: 7 of 18 files reviewed, 1 unresolved discussion (waiting on @kormat, @lukedirtwalker, and @worxli)

[worxli]

Reviewable status: 3 of 21 files reviewed, 2 unresolved discussions (waiting on @kormat, @lukedirtwalker, and @worxli)

---

.buildkite/clean_env.sh, line 4 at r4 (raw file):

cntrs="$(docker ps -aq)"
[ -n "$cntrs" ] && { echo "Remove leftover containers: $cntrs"; docker rm -f $cntrs; }

Don't echo.

[kormat]

Reviewed 1 of 15 files at r1, 2 of 13 files at r3, 5 of 17 files at r4, 1 of 1 files at r5.
Reviewable status: 11 of 19 files reviewed, 3 unresolved discussions (waiting on @lukedirtwalker and @worxli)

---

python/topology/zk.py, line 62 at r5 (raw file):

            ]
        }

Blank line.

---

tools/ci/integration_run, line 42 at r5 (raw file):

fi

./docker.sh exec "./tools/dc zk logs zookeeper >& logs/zookeeper.log"

>& - i've never seen that before. &> is preferred.

[kormat]

Reviewed 2 of 17 files at r4.
Reviewable status: 13 of 19 files reviewed, 3 unresolved discussions (waiting on @lukedirtwalker and @worxli)

[kormat]

Reviewable status: 13 of 19 files reviewed, 7 unresolved discussions (waiting on @lukedirtwalker and @worxli)

---

python/topology/common.py, line 128 at r5 (raw file):

def _gen_zk_entry(addr, port, in_docker, docker):

This seems like it more

---

python/topology/topo.py, line 289 at r5 (raw file):

            zk_conf = self.args.topo_config_dict["defaults"]["zookeepers"]
        if len(zk_conf) > 1:
            logging.critical("Only one zk instance is allowed!")

s/allowed/supported/

---

python/topology/zk.py, line 41 at r5 (raw file):

    def generate(self):
        if "zookeepers" not in self.args.config.get("defaults", {}):

This code is a ~duplicate of _gen_zk_entries in python/topology/topo.py

---

python/topology/zk.py, line 42 at r5 (raw file):

    def generate(self):
        if "zookeepers" not in self.args.config.get("defaults", {}):
            logging.critical("No zookeeper configured in the topology!")

It looks like defaults is therefore mandatory, so instead of checking to see if defaults exists in multiple places, enforce its presence once.

[kormat]

Reviewable status: 13 of 19 files reviewed, 7 unresolved discussions (waiting on @lukedirtwalker and @worxli)

---

python/topology/common.py, line 130 at r5 (raw file):

def _gen_zk_entry(addr, port, in_docker, docker):
    if in_docker:
        port = DEFAULT_DOCKER_ZK_PORT

Why is the supplied port ignored if this is in docker?

[kormat]

Reviewed 6 of 17 files at r4.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @worxli)

---

python/topology/common.py, line 139 at r5 (raw file):

            print('DOCKER0 env variable required! Exiting!')
            sys.exit(1)
    elif docker:

This logic chain looks convoluted, i'm not clear what you're trying to do here.

---

python/topology/net.py, line 35 at r5 (raw file):

DEFAULT_PRIV_NETWORK = "192.168.0.0/16"
DEFAULT_MININET_NETWORK = "100.64.0.0/10"
DEFAULT_SCN_DC_NETWORK = "172.40.0.0/16"

Using T-Mobile's IP allocations seems unwise :P

[worxli]

Reviewable status: 11 of 19 files reviewed, 8 unresolved discussions (waiting on @kormat)

---

python/topology/common.py, line 128 at r5 (raw file):

Previously, kormat (Stephen Shirley) wrote…

This seems like it more

?

---

python/topology/common.py, line 130 at r5 (raw file):

Previously, kormat (Stephen Shirley) wrote…

Why is the supplied port ignored if this is in docker?

It can't use the same port for in in-docker and on the host. This is making sure the in-docker version strictly uses a different port.

---

python/topology/common.py, line 139 at r5 (raw file):

Previously, kormat (Stephen Shirley) wrote…

This logic chain looks convoluted, i'm not clear what you're trying to do here.

Done.

---

python/topology/net.py, line 35 at r5 (raw file):

Previously, kormat (Stephen Shirley) wrote…

Using T-Mobile's IP allocations seems unwise :P

Hm. Thought there is more free space than /12.

---

python/topology/topo.py, line 289 at r5 (raw file):

Previously, kormat (Stephen Shirley) wrote…

s/allowed/supported/

Done.

---

python/topology/zk.py, line 41 at r5 (raw file):

Previously, kormat (Stephen Shirley) wrote…

This code is a ~duplicate of _gen_zk_entries in python/topology/topo.py

Done.

---

python/topology/zk.py, line 42 at r5 (raw file):

Previously, kormat (Stephen Shirley) wrote…

It looks like defaults is therefore mandatory, so instead of checking to see if defaults exists in multiple places, enforce its presence once.

Done.

---

python/topology/zk.py, line 62 at r5 (raw file):

Previously, kormat (Stephen Shirley) wrote…

Blank line.

Done.

---

tools/ci/integration_run, line 42 at r5 (raw file):

Previously, kormat (Stephen Shirley) wrote…

>& - i've never seen that before. &> is preferred.

Done.

[kormat]

Reviewed 5 of 8 files at r6.
Reviewable status: 16 of 19 files reviewed, 7 unresolved discussions (waiting on @kormat and @worxli)

---

python/topology/topo.py, line 303 at r6 (raw file):

        # If we're in-docker, we need to set the port to not conflict with something on the host
        if in_docker:
            port = DEFAULT_DOCKER_ZK_PORT

I'd rather increment the port than choose one which is completely unrelated to whatever was specified.

---

python/topology/topo.py, line 307 at r6 (raw file):

            port = DEFAULT_ZK_PORT

        # If in-docker, we need to know the DOCKER0 IP

Comments like these should be inside the if block, otherwise you end up with things like the comment for the elif obscuring the fact that there code before and after are connected.

---

python/topology/zk.py, line 42 at r5 (raw file):

so instead of checking to see if defaults exists in multiple places, enforce its presence once.

My point is that this should be done somewhere central. ConfigGenerator._read_defaults() looks like the right place.

[kormat]

Reviewed 2 of 8 files at r6.
Reviewable status: 18 of 19 files reviewed, 4 unresolved discussions (waiting on @kormat and @worxli)

[worxli]

Reviewable status: 16 of 19 files reviewed, 4 unresolved discussions (waiting on @kormat)

---

python/topology/topo.py, line 303 at r6 (raw file):

Previously, kormat (Stephen Shirley) wrote…

I'd rather increment the port than choose one which is completely unrelated to whatever was specified.

Done.

---

python/topology/topo.py, line 307 at r6 (raw file):

Previously, kormat (Stephen Shirley) wrote…

Comments like these should be inside the if block, otherwise you end up with things like the comment for the elif obscuring the fact that there code before and after are connected.

Done.

---

python/topology/zk.py, line 42 at r5 (raw file):

Previously, kormat (Stephen Shirley) wrote…

so instead of checking to see if defaults exists in multiple places, enforce its presence once.

My point is that this should be done somewhere central. ConfigGenerator._read_defaults() looks like the right place.

Is the mininet approach in there still needed?

[kormat]

Reviewed 1 of 8 files at r6, 3 of 3 files at r7.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @worxli)

---

docker.sh, line 91 at r7 (raw file):

    args+=" -v $SCION_MOUNT/htmlcov:/home/scion/go/src/github.com/scionproto/scion/python/htmlcov"
    args+=" -v /run/shm/dispatcher:/run/shm/dispatcher"
    args+=" -v /run/shm/sciond:/run/shm/sciond"

• why this change?
• why this change in this PR?

---

python/topology/net.py, line 35 at r5 (raw file):

Previously, worxli (Lukas Bischofberger) wrote…

Hm. Thought there is more free space than /12.

• we should have static IP network allocations that we use for X, Y and Z
• these should all be in 172.16/12, so reduce possible clashes with host networking
• and these should be configurable, so that if there is a clash with another docker-compose project, the user can adjust things
• and single services like zk/postgres/etc don't need their own network, they can use the default docker0 network.

---

python/topology/zk.py, line 42 at r5 (raw file):

Previously, worxli (Lukas Bischofberger) wrote…

Is the mininet approach in there still needed?

Mininet hasn't been used in a very long time, i'd be rather surprised if it did work, and tbh the dockerised setup mostly superceeds it. So, we should remove mininet support, but not in this PR. Can you file an issue for it, please?

[worxli]

Reviewable status: 17 of 20 files reviewed, 3 unresolved discussions (waiting on @kormat)

---

docker.sh, line 91 at r7 (raw file):

Previously, kormat (Stephen Shirley) wrote…

• why this change?
• why this change in this PR?

Otherwise a docker.sh run with a supervisor topology has the socekts bind-mounted to/from the host.

Why here? Because I wanted to test host and in-docker zookeeper running in parallel, with integration tests running in parallel. And the sockets conflicted.

---

python/topology/net.py, line 35 at r5 (raw file):

172.16

I think: done.

---

python/topology/zk.py, line 42 at r5 (raw file):

Previously, kormat (Stephen Shirley) wrote…

Mininet hasn't been used in a very long time, i'd be rather surprised if it did work, and tbh the dockerised setup mostly superceeds it. So, we should remove mininet support, but not in this PR. Can you file an issue for it, please?

Done.

[kormat]

Reviewed 3 of 3 files at r8.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @worxli)

---

python/topology/net.py, line 35 at r5 (raw file):

Previously, worxli (Lukas Bischofberger) wrote…

172.16

I think: done.

I wouldn't use 172.16.0.0, as that's a very common private network. Docker uses 172.17.0.0/16, so i'd start with, say, 172.20.0.0.

[worxli]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @kormat)

---

python/topology/net.py, line 35 at r5 (raw file):

Previously, kormat (Stephen Shirley) wrote…

I wouldn't use 172.16.0.0, as that's a very common private network. Docker uses 172.17.0.0/16, so i'd start with, say, 172.20.0.0.

Done.

[lukedirtwalker]

Reviewable status: 10 of 19 files reviewed, 2 unresolved discussions (waiting on @kormat, @lukedirtwalker, and @worxli)

---

python/topology/topo.py, line 302 at r9 (raw file):

            port = port + 1

        if in_docker:

As discussed please move this code block to a common function.

def docker_host(in_docker, docker, def_addr):
       if in_docker:
           # If in-docker we need to know the DOCKER0 IP
           addr = os.getenv('DOCKER0', None)
           if not addr:
               print('DOCKER0 env variable required! Exiting!')
               sys.exit(1)
       elif docker or not addr:
           # Using docker topology or there is no default addr,
           # we directly get the DOCKER0 IP
           addr = docker_ip()
       else:
           # Addr is specified in the topo file
           addr = str(ip_address(addr))
       return addr

so we can use this in other places as well.

[worxli]

Reviewable status: 10 of 19 files reviewed, 2 unresolved discussions (waiting on @kormat and @lukedirtwalker)

---

python/topology/topo.py, line 302 at r9 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

As discussed please move this code block to a common function.

def docker_host(in_docker, docker, def_addr):
       if in_docker:
           # If in-docker we need to know the DOCKER0 IP
           addr = os.getenv('DOCKER0', None)
           if not addr:
               print('DOCKER0 env variable required! Exiting!')
               sys.exit(1)
       elif docker or not addr:
           # Using docker topology or there is no default addr,
           # we directly get the DOCKER0 IP
           addr = docker_ip()
       else:
           # Addr is specified in the topo file
           addr = str(ip_address(addr))
       return addr

so we can use this in other places as well.

Done.

[lukedirtwalker]

Reviewed 2 of 3 files at r10.
Reviewable status: 12 of 19 files reviewed, 1 unresolved discussion (waiting on @kormat and @lukedirtwalker)

[kormat]

Reviewed 6 of 11 files at r9, 2 of 3 files at r10, 1 of 1 files at r11.
Reviewable status: complete! all files reviewed, all discussions resolved