Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update Docker compose for development portal. #329

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

Conversation

SubOptimal
Copy link
Member

@SubOptimal SubOptimal commented Mar 15, 2023

All changes were successfully tested with

Docker version 23.0.1, build a5ee5b1

fixes #322

@SubOptimal SubOptimal marked this pull request as ready for review March 20, 2023 21:43
The values for the database, username and password were amended
to the new Keycloak development settings.
@SubOptimal SubOptimal requested a review from opatut March 21, 2023 21:25
The KeyCloak container depends on the started PostgreSQL database
instance. Without the explicit dependency to its healthy state,
the KeyCloak container might be started before the database can be
used.
docker-compose.yaml Outdated Show resolved Hide resolved
@@ -36,7 +41,6 @@ services:
- ./api/migrations:/opt/obs/api/migrations
- ./api/alembic.ini:/opt/obs/api/alembic.ini
depends_on:
- postgres
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@gluap gluap Mar 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor

@gluap gluap Apr 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@gluap gluap Apr 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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;

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

Copy link
Contributor

@gluap gluap Apr 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@@ -57,7 +61,6 @@ services:
- ./api/config.overrides.py:/opt/obs/api/config.overrides.py
- ./local/api-data:/data
depends_on:
- postgres
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

POSTGRES_DB: obs
POSTGRES_USER: keycloak
POSTGRES_PASSWORD: password
POSTGRES_DB: postgres
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link

sonarcloud bot commented Apr 17, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@SubOptimal
Copy link
Member Author

SubOptimal commented Apr 17, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update KeyCloak version
3 participants