Update Docker compose for development portal.

[SubOptimal]

All changes were successfully tested with

Docker version 23.0.1, build a5ee5b1

fixes #322

[gluap]

I don't think we can remove this dependency - api depends on postgres even if keycloak were not needed. Depending on keycloak should make sure that the start order is right even with the dependency.

[SubOptimal]

Before the api container depended on the postgres and keycloak containers. But only in the way that it started after the both containers where started (from the Docker point of view). There was no guarantee about the state of the applications in the containers.

Now the keycloak container depend on the postgres container with the condition: service_healthy. This ensures the keycloak container will only be started after the healthchecck of the postgres container is passed. Without this dependency it is not possible to ensure that the KeyCloak server will not try to access the Postgres database before it is ready to accept connections. If the healthcheck of postgres container fails, the keycloak container is not started.

As the containers api and worker depend on the container keycloak to be started, it is ensured that the Postgres database is ready to accept connections.

Is there a scenario to run the containers postgres and api without the keycloak container? In that case, the dependency on container keycloak, on container api should be removed.

[gluap]

well, for one thing worker can work just fine without a keycloak running. For api saying that it also depends on postgres is more semantic: It does not only need access to keycloak but also direct access to postgres. That second is the reason I was suggesting to keep the dependency. Furthermore when running commands like upgrade.py or osm2pgsql.sh (or its decendants) api can actually run without keycloak running (we shouldn't make it keycloak independent because of that, though).

If I understand you correctly, the best way to ensure that api and worker await all their dependencies would be to also have them use the

      postgres:
        condition: service_healthy

condition that is used on keycloak now. By the way: I learned about that from this PR, thank you, that is indeed useful.

PS: if you say "hey I did enough for this pr already, leave me be", that's fine too. This is fine-tuning and putting the change in without this fine tuning is better than leaving it out in any case.

[SubOptimal]

I will have a look on this. Probably we need to add a health_check also for the keycloak container.

[SubOptimal]

I did some further investigation.

• the api depends on successful access of /auth/realms/obs-dev/.well-known/openid-configuration
• which isn't possible before the initial KeyCloak setup was done

This implies that even by defining a dependency path as api -> keycloak -> postgres a docker compose up api would not guarantee a proper start of the api container.

I will take the opportunity to implement a script for the initial setup (which is currently a manual action).

It will still not provide a stripped down dependency path as api -> postgres. As api relies on the accessibility of a KeyCloak URL (/auth/realms/obs-dev/.well-known/openid-configuration).

My suggestion would be to define only explicit dependencies in the compose file, e.g. api -> keycloak (here it needs to be discovered if started or healthy status is required) and keycloak -> postgres. Then a docker compose up api (after initial setup was done) will ensure that the api is fully functional (e.g. KeyCloak and PostgreSQL are started and usable by api).

[gluap]

Wie oben im PS schon gesagt: Gern ohne weitere Änderungen an docker-compose.yaml mergen, funktional macht der Change genau was er soll, nämlich dafür sorgen dass api erst startet wenn alle dependencies verfügbar sind. Einzig die dependency auf postgres wird nicht explizit gemacht. Ich bin aber nicht sicher ob ich verstanden habe was du sagen willst und du verstanden hast was ich sagen will und deshalb: Falls du Lust hast, mit mir rauszufinden wo wir uns missverstehen gehts hier weiter:

Genau die explizite Abhängigkeit, die du erwähnst von api auf postgres gibt es aus meiner Sicht. Ich habe dieses Verständnis von Dependency zugrundelegt, das glaube ich bei docker gemeint ist:

container a hängt für seine Lauffähigkeit von einer Schnittstelle ab, die container b anbietet

Wir sind uns einig: api redet per rest-api mit keycloak (um user zu authentifizieren)
deshalb:

api hängt von der REST-Schnittstelle (auf port 8080) von keycloak ab
➡️ api depends on keycloak

keycloak hängt von der psql Schnittstelle auf port 5432 von postgres ab
➡️ keycloak depends on postgres. (und der neue health check hier ist prima!)

Zusätzlich (meine interpretation von "explizit"):
(vollkommen unabhängig von keycloak, es legt daten direkt in postgres ohne umweg über keycloak)
api hängt von der psql Schnittstelle auf port 5432 von postgres ab auf port 5432 von postgres ab
➡️ api depends on postgres

ich würde das übersetzen als

api:
...
  depends_on:
     - postgres # because api reads tables directly via psql
     - keycloak  # because api needs user and auth info from keycloac

Was macht compose, wenn man das so konfiguriert? Nichts anderes, api muss per Konvention warten, bis jede seiner dependencies verfügbar ist - und da keycloak nach postgres verfügbar ist, wird api durch die Änderung nicht früher starten. Aber für denjenigen der den compose file liest ist klar: api hängt nicht nur implizit wegen keycloak von postgres ab, sondern auch explizit wegen der datenbankverbindung.

PS: als Mermaid:

graph TD;
    keycloak-->|psql|postgres;
    api-->|REST|keycloak;
    api-->|psql|postgres;
    worker-->|psql|postgres;

Loading

[boldt]

Eine Dependency in docker-compose sorgt nur dafür, dass ein Container vor dem anderen gestartet ist. D.h. man kann Ketten, wie von @gluap abbilden.

Eine Dependency sorgt nicht dafür, dass der Service dann auch wirklich bereit ist. Es kann also theoretisch sein (praktisch kommt es auch vor), dass ein Service auf eine Datenbank zugreifen will, obwohl dieser Service tatsächlich noch am Starten und somit noch nicht bereit ist.

[gluap]

@boldt seit compose v2 kann man auch auf service healthy und nicht nur auf "gestartet" dependen - das ist das coole, was SubOptimal mit seinem healthcheck möglich gemacht hat.

[SubOptimal]

@gluap Danke. Nun habe ich die Abhängigkeiten auch verstanden. War davon ausgegangen, das api ohne KeyCloak nicht funktionieren würde.

Im KeyCloak Image gibt es OOTB keine einfache Möglichkeit einen Health-Check zu implementieren, mögliche Binaries (z. B. curl, wget, nc, ...) und Paket-Management sind nicht im Image enthalten.

Die Abhängigkeiten können so abgebildet werden

graph TD;
    keycloak-->|service_healthy|postgres;
    api-->|service_started|keycloak;
    api-->|service_healthy|postgres;
    worker-->|service_healthy|postgres;
    frontend-->|service_started|api;

Loading

Der Status bezieht sich auf den Container, von dem ein anderer abhängt.

[gluap]

Dein Graph bildet aus meiner sicht genau die Abhängigkeiten ab!
In Api habe ich iirc einen retry eingebaut, als ich mal über das Problem mit dem Keycloak gestolpert bin, wenn ich mich nicht irre versucht api es einfach so lange, bis keycloak antwortet.

[gluap]

I don't think we can remove this dependency - api depends on postgres even if keycloak were not needed. Depending on keycloak should make sure that the start order is right even with the dependency.

[gluap]

I like the separation between the keycloak and postgres DB. Had to go through some trouble to separate the two when making my test setup more production like and this is exactly what would have made that easier!

This may mess with peoples pre-existing dev setup though if they started from default. But probably almost everyone with a dev setup will be able to resolve the issue themself.

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

[SubOptimal]

The Docker compose file and the README is updated for the development setup.

For the automatic initialization of the development setup we need to decide if it should be part of this PR (then I will commit the files to this PR) or if it should go with its own PR.

edit: This pull request should only be merged after #321 as both have changed some of the same files.