WordPress / openverse

Openverse is a search engine for openly-licensed media. This monorepo includes all application code.
https://openverse.org
MIT License
240 stars 194 forks source link

Stop binding ports in the reservable range locally #200

Closed sarayourfriend closed 1 year ago

sarayourfriend commented 2 years ago

Problem

We currently bind ports in the reservable range, except for the API proxy in the frontend repository.

This means that our local development services can clash with system services (not to mention default ports of other projects that are also binding in the reserveable range; how many things do you know that run on 3000 or 8080 or 8000?)

For example SyncThing runs on port 8384 by default which is fine because it's a non-ephemeral system service.

Also there's always the possibility that this will happen again: https://developer.apple.com/forums/thread/682332

And there's nothing wrong with an OEM adding a system service in that range, the range is specifically reserved for such things. We're just lucky that nothing has conflicted with our choices (or the default values) thus far.

Description

There is actually a range of ports available for dynamic and ephemeral services: https://en.wikipedia.org/wiki/List_of_TCP_and_UDP_port_numbers#Dynamic,_private_or_ephemeral_ports

Let's switch our services to use those so that we avoid conflicting with other projects or, indeed, other running applications that could be critical to a developers flow or conflict with a new system service from an OEM or from the dev themselves.

It'd be nice if we just used a solid block of ports so that it's easier to remember where each service is running locally. For example we could do the following, going lowest in the stack to highest, with the main services grouped together and then auxiliary services (Databases, Redis, etc) grouped after those.

Alternatively, the other direction as Madison suggested:

Implementation

AetherUnbound commented 2 years ago

I actually think that the reverse might be best, start with frontend -> API -> Airflow. The higher the port, the "further back in the stack". As you mention, databases and other supported services could just be added on incrementally as needed. I like this idea, because I can never remember which port something is on and using an explicitly different port would allow me to bookmark that on localhost as well :slightly_smiling_face:

I think that having a monorepo (#192) would make this organization a lot easier, rather than having to cross reference which next port in 3 different repos is available for some new service.

sarayourfriend commented 2 years ago

I actually think that the reverse might be best, start with frontend -> API -> Airflow.

:+1: I'm not too worried about the specific order so whatever folks on the team most like is what I'd suggest :slightly_smiling_face: It makes no difference to me.

zackkrida commented 2 years ago

Also there's always the possibility that this will happen again: developer.apple.com/forums/thread/682332

And it has! @dhruvkb identified a new one:

macOS 12.3 adds a service /usr/libexec/remotepairingdeviced that listens on port 49152. Can’t run the Talkback proxy anymore now :sad-panda:

dhruvkb commented 2 years ago

@AetherUnbound we could reserve about 100 ports for each repo using the first two digits constant, third as a repo number and the last two as arbitrary ports.

501xx - catalog 502xx - API 503xx - frontend

sarayourfriend commented 2 years ago

I'm adding help wanted and good first issue to this issue because I keep running into this locally and it's quite frustrating! We bind redis to the host in the API on the actual redis port which clashes with any local running redis instances. It's a theoretically straightforward issue so I might pick it up myself in the next couple weeks unless someone else gets to it first. It would be a nice QOL improvement for me personally. I'm sure other folks will run into this as well.

Update: blocked on my other work for the time being so going to try this in the API.

sarayourfriend commented 2 years ago

It seems like there are some service interactions between the ingestion server and the workers (and maybe the API?) that I am not fully aware of. I've got a diff below of my start in the API but going to unassign myself from this.

The diff for API to use 602 port prefix
    diff --git a/.github/workflows/ci_cd.yml b/.github/workflows/ci_cd.yml
    index 5d5ded35..95218f67 100644
    --- a/.github/workflows/ci_cd.yml
    +++ b/.github/workflows/ci_cd.yml
    @@ -144,7 +144,7 @@ jobs:

          - name: Smoke test ReDoc site
            run: |
    -          curl --fail 'http://localhost:8000/v1/?format=openapi'
    +          curl --fail 'http://localhost:60200/v1/?format=openapi'

          - name: Run API tests
            run: just api-test
    diff --git a/api/README.md b/api/README.md
    index 74b6f721..79f8970a 100644
    --- a/api/README.md
    +++ b/api/README.md
    @@ -9,7 +9,7 @@ The API has two sets of documentation.
          ```bash
          just sphinx-live
          ```
    -    - visiting the `https://localhost:3000/` endpoint
    +    - visiting the `https://localhost:60207/` endpoint
      - contain more details on how to contribute to the API project

    - [Consumer docs](https://api.openverse.engineering/)
    @@ -19,5 +19,5 @@ The API has two sets of documentation.
          ```bash
          just up
          ```
    -    - visiting the `https://localhost:8000/` endpoint
    +    - visiting the `https://localhost:60200/` endpoint
      - contain more details about the API endpoints with usage examples
    diff --git a/api/catalog/settings.py b/api/catalog/settings.py
    index 4c915d5f..cffb599c 100644
    --- a/api/catalog/settings.py
    +++ b/api/catalog/settings.py
    @@ -63,7 +63,7 @@ if DEBUG:
    SHORT_URL_WHITELIST = {
        "api-dev.openverse.engineering",
        "api.openverse.engineering",
    -    "localhost:8000",
    +    "localhost:60200",
    }
    SHORT_URL_PATH_WHITELIST = ["/v1/list", "/v1/images/"]

    @@ -186,7 +186,7 @@ CACHES = {
    }

    # Produce CC-hosted thumbnails dynamically through a proxy.
    -THUMBNAIL_PROXY_URL = config("THUMBNAIL_PROXY_URL", default="http://localhost:8222")
    +THUMBNAIL_PROXY_URL = config("THUMBNAIL_PROXY_URL", default="http://localhost:60202")

    THUMBNAIL_WIDTH_PX = config("THUMBNAIL_WIDTH_PX", cast=int, default=600)
    THUMBNAIL_JPG_QUALITY = config("THUMBNAIL_JPG_QUALITY", cast=int, default=80)
    diff --git a/api/docs/guides/quickstart.md b/api/docs/guides/quickstart.md
    index d277d4cb..45449d2d 100644
    --- a/api/docs/guides/quickstart.md
    +++ b/api/docs/guides/quickstart.md
    @@ -30,7 +30,7 @@ Ensure that you have installed `mkcert` (and the corresponding NSS tools). You c
        just up
        ```

    -5. Point your browser to `http://localhost:8000`. You should be able to see the API documentation.
    +5. Point your browser to `http://localhost:60200`. You should be able to see the API documentation.
        ![API ReDoc](/_static/api_redoc.png)

    6. Load the sample data. This could take a couple of minutes.
    diff --git a/api/test/api_live_integration.py b/api/test/api_live_integration.py
    index c5b88235..6e78acdd 100644
    --- a/api/test/api_live_integration.py
    +++ b/api/test/api_live_integration.py
    @@ -19,9 +19,9 @@ designed. Run with the `pytest -s` command from this directory.
    """

    -API_URL = os.getenv("INTEGRATION_TEST_URL", "http://localhost:8000")
    +API_URL = os.getenv("INTEGRATION_TEST_URL", "http://localhost:60200")
    known_apis = {
    -    "http://localhost:8000": "LOCAL",
    +    "http://localhost:60200": "LOCAL",
        "https://api.openverse.engineering": "PRODUCTION",
        "https://api-dev.openverse.engineering": "TESTING",
    }
    diff --git a/api/test/auth_test.py b/api/test/auth_test.py
    index 37aac957..c1ae7207 100644
    --- a/api/test/auth_test.py
    +++ b/api/test/auth_test.py
    @@ -55,7 +55,7 @@ def test_auth_token_exchange(client, test_auth_tokens_registration):
    def test_auth_email_verification(client, test_auth_token_exchange):
        # This test needs to cheat by looking in the database, so it will be
        # skipped in non-local environments.
    -    if API_URL == "http://localhost:8000":
    +    if API_URL == "http://localhost:60200":
            verify = OAuth2Verification.objects.last()
            code = verify.code
            path = reverse("verify-email", args=[code])
    diff --git a/api/test/constants.py b/api/test/constants.py
    index 7254fe5a..99420667 100644
    --- a/api/test/constants.py
    +++ b/api/test/constants.py
    @@ -1,10 +1,10 @@
    import os

    -API_URL = os.getenv("INTEGRATION_TEST_URL", "http://localhost:8000")
    +API_URL = os.getenv("INTEGRATION_TEST_URL", "http://localhost:60200")

    KNOWN_ENVS = {
    -    "http://localhost:8000": "LOCAL",
    +    "http://localhost:60200": "LOCAL",
        "https://api.openverse.engineering": "PRODUCTION",
        "https://api-dev.openverse.engineering": "TESTING",
    }
    diff --git a/docker-compose.yml b/docker-compose.yml
    index beed92cc..60eb13d5 100644
    --- a/docker-compose.yml
    +++ b/docker-compose.yml
    @@ -3,7 +3,7 @@ services:
      db:
        image: postgres:13.2-alpine
        ports:
    -      - "5432:5432"
    +      - "60201:5432"
        env_file:
          - postgres/env.docker
        healthcheck:
    @@ -14,7 +14,7 @@ services:
      thumbnails:
        image: ghcr.io/wordpress/openverse-imaginary:latest
        ports:
    -      - "8222:8222"
    +      - "60202:8222"
        environment:
          PORT: 8222
          MALLOC_ARENA_MAX: 2
    @@ -23,7 +23,7 @@ services:
      upstream_db:
        image: postgres:13.2-alpine
        ports:
    -      - "5433:5432"
    +      - "60203:5432"
        volumes:
          - catalog-postgres:/var/lib/postgresql/data
          - ./sample_data:/sample_data
    @@ -35,12 +35,12 @@ services:
      cache:
        image: redis:4.0.10
        ports:
    -      - "6379:6379"
    +      - "60204:6379"

      es:
        image: docker.elastic.co/elasticsearch/elasticsearch:7.12.0
        ports:
    -      - "9200:9200"
    +      - "60205:9200"
        environment:
          # disable XPack
          # https://www.elastic.co/guide/en/elasticsearch/reference/5.3/docker.html#_security_note
    @@ -72,8 +72,8 @@ services:
        volumes:
          - ./api:/api
        ports:
    -      - "8000:8000"
    -      - "3000:3000"
    +      - "60200:8000"
    +      - "60207:3000"
        depends_on:
          - db
          - es
    @@ -88,7 +88,7 @@ services:
        image: openverse-ingestion_server
        command: gunicorn -c ./gunicorn.conf.py
        ports:
    -      - "8001:8001"
    +      - "60208:8001"
        depends_on:
          - db
          - es
    @@ -105,7 +105,7 @@ services:
        image: openverse-ingestion_server
        command: gunicorn -c ./gunicorn_worker.conf.py
        ports:
    -      - "8002:8002"
    +      - "60209:8002"
        depends_on:
          - db
          - es
    @@ -119,8 +119,8 @@ services:
      proxy:
        image: nginx:alpine
        ports:
    -      - "9080:9080"
    -      - "9443:9443"
    +      - "60210:9080"
    +      - "60211:9443"
        depends_on:
          - web
        volumes:
    diff --git a/ingestion_server/README.md b/ingestion_server/README.md
    index d59d0bac..f1ad2cea 100644
    --- a/ingestion_server/README.md
    +++ b/ingestion_server/README.md
    @@ -59,7 +59,7 @@ To make cURL requests to the server
    ```bash
    pipenv run \
      curl \
    -    --XPOST localhost:8001/task \
    +    --XPOST localhost:60208/task \
        -H "Content-Type: application/json" \
        -d '{"model": , "action": }'
    ```
    diff --git a/ingestion_server/test/test_constants.py b/ingestion_server/test/test_constants.py
    index fad5615a..a466972f 100644
    --- a/ingestion_server/test/test_constants.py
    +++ b/ingestion_server/test/test_constants.py
    @@ -3,6 +3,6 @@
    service_ports = {
        "upstream_db": 65433,  # from 5433
        "db": 65432,  # from 5432
    -    "es": 60200,  # from 9200
    +    "es": 6020,  # from 9200
        "ingestion_server": 60001,  # from 8001
    }
    diff --git a/justfile b/justfile
    index 24d31b26..b7afc09e 100644
    --- a/justfile
    +++ b/justfile
    @@ -107,14 +107,14 @@ cert:
        -curl -s -o /dev/null -w '%{http_code}' 'http://{{ es_host }}/_cluster/health?pretty'

    # Wait for Elasticsearch to be healthy
    -@wait-for-es es_host="localhost:9200":
    +@wait-for-es es_host="localhost:60205":
        just _loop \
        '"$(just es-health {{ es_host }})" != "200"' \
        "Waiting for Elasticsearch to be healthy..."

    # Check if the media is indexed in Elasticsearch
    @check-index index="image":
    -    -curl -sb -H "Accept:application/json" "http://localhost:9200/_cat/aliases/{{ index }}" | grep -c "{{ index }}-"
    +    -curl -sb -H "Accept:application/json" "http://localhost:60205/_cat/aliases/{{ index }}" | grep -c "{{ index }}-"

    # Wait for the media to be indexed in Elasticsearch
    @wait-for-index index="image":
    @@ -132,7 +132,7 @@ _ing-install:
        cd ingestion_server && pipenv install --dev

    # Perform the given action on the given model by invoking the ingestion-server API
    -_ing-api model action port="8001":
    +_ing-api model action port="60208":
        curl \
          -X POST \
          -H 'Content-Type: application/json' \
    @@ -144,7 +144,7 @@ _ing-api model action port="8001":
        -curl -s -o /dev/null -w '%{http_code}' 'http://{{ ing_host }}/'

    # Wait for the ingestion-server to be healthy
    -@wait-for-ing ing_host="localhost:8001":
    +@wait-for-ing ing_host="localhost:60208":
        just _loop \
        '"$(just ing-health {{ ing_host }})" != "200"' \
        "Waiting for the ingestion-server to be healthy..."
    @@ -172,7 +172,7 @@ _api-install:

    # Check the health of the API
    @web-health:
    -    -curl -s -o /dev/null -w '%{http_code}' 'http://localhost:8000/healthcheck'
    +    -curl -s -o /dev/null -w '%{http_code}' 'http://localhost:60200/healthcheck'

    # Wait for the API to be healthy
    @wait-for-web:
    @@ -201,7 +201,7 @@ dj-local +args:

    # Make a test cURL request to the API
    stats media="images":
    -    curl "http://localhost:8000/v1/{{ media }}/stats/"
    +    curl "http://localhost:60200/v1/{{ media }}/stats/"

    # Get Django shell with IPython
    ipython:
dhruvkb commented 2 years ago

Seems like binding to port 50292 in GitHub Actions is flaky. I've perceived an increase in CI failures (that go away on re-run) since WordPress/openverse-api#844 was merged.

sarayourfriend commented 2 years ago

@dhruvkb Do you have an example of the flaky runs? Are they failing specifically on binding to that port only, do you think if we just changed that one it could address the flakiness?

dhruvkb commented 2 years ago

It takes a little effort to go through the runs and find occurrences, because when they occurred I usually re-ran them and they passed. Nonetheless here are a couple of examples:

It's confusing why the error would be

Error starting userland proxy: listen tcp4 0.0.0.0:502xx: bind: address already in use

and if that is indeed the case, how it would go away on a re-run.

sarayourfriend commented 2 years ago

That is strange. Let's change it to another port and see if it helps... but maybe there's actually some kind of race condition we introduced at some point that is causing multiple of the same docker service to get spun up?

sarayourfriend commented 1 year ago

We haven't seen this flakiness anymore recently, I believe, so I'm going to close this issue along with #1035.