feat: make dev.up wait until services are actually running - #229
Draft
pwnage101 wants to merge 1 commit into
Draft
feat: make dev.up wait until services are actually running#229pwnage101 wants to merge 1 commit into
pwnage101 wants to merge 1 commit into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR improves make dev.up reliability by having Docker Compose wait for containers to become healthy (not just “started”), and by normalizing service definitions so healthchecks and devserver commands are defined consistently across services.
Changes:
- Update
dev.uptargets to rundocker compose up -d --waitso the command only returns success once services are healthy. - Introduce
common.ymlbase services (backend-app,microfrontend) and refactor many services toextendsthem, standardizing devserver loops and healthchecks. - Convert several
depends_onentries to condition-based mappings to align with Compose readiness semantics.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
Makefile |
Adds --wait to docker compose up in dev.up targets so startup waits for healthy containers. |
docker-compose.yml |
Refactors many services to inherit common devserver/healthcheck behavior and updates depends_on conditions. |
common.yml |
Adds reusable base service definitions for backend apps and microfrontends, including healthcheck defaults. |
microfrontend.yml |
Removes the old microfrontend base configuration in favor of common.yml. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| extends: | ||
| file: common.yml | ||
| service: backend-app | ||
| # Sleep as a part of start up to give elasticsearch enough time to start up. |
Comment on lines
842
to
845
| @@ -792,9 +844,9 @@ services: | |||
| aliases: | |||
| - edx.devstack.enterprise-catalog | |||
Comment on lines
361
to
+363
| environment: | ||
| DEVSERVER_CMD: "python manage.py runserver 0.0.0.0:18120" | ||
| HEALTHCHECK_TARGET: "http://localhost:18120/heartbeat/" |
Comment on lines
596
to
+598
| environment: | ||
| DEVSERVER_CMD: "python manage.py runserver 0.0.0.0:19001" | ||
| HEALTHCHECK_TARGET: "http://localhost:19001/health/" |
| eval "$$DEVSERVER_CMD" | ||
| sleep 2 | ||
| done | ||
| # Configure a healthcheck to allow `make dev.up.<servce>` to give accurate feedback about |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (4)
common.yml:38
- Typo in comment:
dev.up.<servce>should bedev.up.<service>.
# Configure a healthcheck to allow `make dev.up.<servce>` to give accurate feedback about
Makefile:272
docker compose up --waitrequires a recent Docker Compose; older installs will error with an unknown flag. Since there’s no compose version/feature check here, the Make target will fail with a confusing error on those systems. Consider adding a quick feature check with a clear upgrade message before callingup --wait.
docker compose up -d --wait $$(echo $* | tr + " ")
common.yml:47
healthcheck.start_intervalis not supported by all Docker Engine / Docker Compose versions; if it's unsupported,docker composewill fail to parse this file and block all devstack startup. Consider removing it (or documenting/enforcing a minimum Docker version) and using a smallerintervalinstead.
interval: 10s
timeout: 10s
retries: 20
start_period: 240s
start_interval: 5s # poll fast during grace period so a healthy boot flips status quickly
Makefile:257
docker compose up --waitrequires a recent Docker Compose; older installs will error with an unknown flag. Since there’s no compose version/feature check here, the Make target will fail with a confusing error on those systems. Consider adding a quick feature check with a clear upgrade message before callingup --wait.
This issue also appears on line 272 of the same file.
docker compose up -d --wait --no-deps $$(echo $* | tr + " ")
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (5)
common.yml:38
- Typo in the comment:
servce→service.
# Configure a healthcheck to allow `make dev.up.<servce>` to give accurate feedback about
common.yml:47
healthcheck.start_intervalis not supported by older Docker Engine / Compose combinations (the repo docs currently claim Docker 19.03+ is sufficient). Using this key can makedocker composefail to parse the file or silently ignore the intent on older installs. Consider removing it (falling back tointerval) or updating the documented minimum Docker/Compose version.
start_period: 240s
start_interval: 5s # poll fast during grace period so a healthy boot flips status quickly
common.yml:30
DEVSERVER_PRECHECKruns viaevalbut its exit status is currently ignored, so a failing precheck (e.g., failingpip install/bundle install/sourcecommand) will still fall through into the devserver loop and can mask the real failure. The precheck should fail fast like theDEVSERVER_CHECKblock does.
This issue also appears on line 46 of the same file.
if [ -n "$$DEVSERVER_PRECHECK" ]; then eval "$$DEVSERVER_PRECHECK"; fi
provision-lms.sh:15
- Only
provision-lms.shwas updated to usedocker compose up --wait, but many otherprovision-*.shscripts still use plaindocker compose up -d ...(e.g., provision-discovery.sh, provision-ecommerce.sh, provision-insights.sh). If the goal is to avoid "all green" when services aren’t actually healthy, those scripts will still have the old behavior unless they’re updated too (or this script should document why it’s the exception).
docker compose up -d --wait $app
Makefile:272
docker compose up --waitis a relatively new Compose feature; on older Compose plugin versions this will fail with an "unknown flag: --wait" error. Since the docs only mention a minimum Docker version (and don’t pin Compose), consider adding a Makefile preflight check with a clearer error message and/or updating the documented minimum Docker Compose plugin version.
dev.up.%: dev.check-memory ## Bring up services and their dependencies.
docker compose up -d --wait $$(echo $* | tr + " ")
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 22 out of 22 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (19)
common.yml:38
- Typo in comment:
servce→service.
# Configure a healthcheck to allow `make dev.up.<servce>` to give accurate feedback about
Makefile:272
docker compose up --waitrequires a Docker Compose version that supports this flag. Without an explicit check, failures can be confusing and look like generic Compose errors. Adding a small guard makes the UX clearer.
docker compose up -d --wait $$(echo $* | tr + " ")
provision-forum.sh:5
forumdepends on services like mongo/memcached/elasticsearch/opensearch (docker-compose.yml:458-466). Using--no-depsprevents them from being started during provisioning, soforummay not boot and this script can fail.
docker compose up -d --wait --no-deps forum
provision-registrar.sh:10
registrardepends on services like redis/memcached/lms (docker-compose.yml:617-629). Using--no-depshere means those won't be started by provisioning, causing registrar to fail to boot and subsequentexecsteps to fail.
docker compose up -d --wait --no-deps $name
provision-xqueue.sh:6
--no-depsdisables bringing up xqueue dependencies. While xqueue currently depends on mysql80, relying on callers to have already started deps makes this script less robust duringmake dev.provisionand inconsistent with other provisioning scripts.
docker compose up -d --wait --no-deps xqueue
provision-insights.sh:10
insightsdepends on mysql80/memcached (and others). Using--no-depsprevents those from starting during provisioning, so the container may not boot and laterdocker compose execcommands can fail.
docker compose up -d --wait --no-deps insights
provision-analyticsapi.sh:10
analyticsapidepends on mysql80 and elasticsearch (docker-compose.yml:588-595). Using--no-depshere prevents those from being started duringmake dev.provision, making the service likely to fail to boot and causing subsequentexecsteps to fail.
docker compose up -d --wait --no-deps ${name}
provision-enterprise-access.sh:8
--no-depsprevents starting dependencies for bothenterprise-accessandlms(mysql80, redis, memcached, etc.). Inmake dev.provision,provision.shdoesn't start most of these, so this can leave the containers unhealthy and break the following provisioning steps.
docker compose up -d --wait --no-deps $name lms
provision-credentials.sh:13
--no-depsprevents startingcredentialsdependencies (e.g., lms/memcached).provision.shdoes not start these, somake dev.provisioncan fail whendocker compose execruns against a container that never successfully started.
docker compose up -d --wait --no-deps $name
provision-notes.sh:12
--no-depsprevents starting Notes dependencies (e.g., mysql80/elasticsearch). Sinceprovision.shonly guarantees mysql80/mongo are up, this can cause Notes to fail to start during provisioning.
docker compose up -d --wait --no-deps $name
provision-enterprise-subsidy.sh:11
- Starting
lmsandenterprise-subsidywith--no-depsprevents required dependencies (mysql80, memcached, redis, etc.) from being started during provisioning. This can cause the services to fail to boot and laterexec/LMS shell steps to fail.
docker compose up -d --wait --no-deps lms
docker compose up -d --wait --no-deps ${name}
provision-license-manager.sh:12
- Starting
license-managerandlmswith--no-depsprevents their required dependencies (mysql80, redis, memcached, etc.) from being started during provisioning, which can break subsequentexecand LMS shell steps.
docker compose up -d --wait --no-deps $name
docker compose up -d --wait --no-deps lms
provision-discovery.sh:8
- Starting
lms,cms, andecommercewith--no-depsprevents their dependencies from being started. This script immediately provisions Discovery and then calls into those services; if they aren't already fully up, this can fail intermittently.
docker compose up -d --wait --no-deps lms
docker compose up -d --wait --no-deps cms
docker compose up -d --wait --no-deps ecommerce
provision-coursegraph.sh:16
- Starting
coursegraphandcmswith--no-depsprevents CMS dependencies (mysql80, elasticsearch, etc.) from being started if they aren't already up, making this script brittle when run viamake dev.provisionor standalone.
docker compose up -d --wait --no-deps coursegraph cms
provision-enterprise-catalog.sh:4
enterprise-catalogdepends on services like mysql80 and memcached (docker-compose.yml:835-842). Using--no-depsprevents those dependencies from starting duringmake dev.provision, so the service may not boot and the subsequent provisioning commands can fail.
docker compose up -d --wait --no-deps $name
provision-edx-exams.sh:11
- Starting
lmsandedx-examswith--no-depsprevents required dependencies from being started during provisioning. This can leave either service unhealthy and cause the later migration/shell steps to fail.
docker compose up -d --wait --no-deps lms
docker compose up -d --wait --no-deps ${name}
provision-designer.sh:5
- Using
--no-depshere prevents starting required dependencies fordesignerandlmsduring provisioning (e.g., mysql80, memcached, etc.). Since this script later runs migrations and creates OAuth clients via LMS, provisioning can fail if dependencies aren't started.
docker compose up -d --wait --no-deps $name --build
docker compose up -d --wait --no-deps lms
common.yml:47
start_intervalis not supported by docker-compose v1 and can breakdocker composeparsing on older environments. Since this repo still has documentation referring to docker-compose 1.x (scripts/README.txt:25-29), consider removing this key for broader compatibility.
start_interval: 5s # poll fast during grace period so a healthy boot flips status quickly
Makefile:257
docker compose up --waitrequires a sufficiently recent Docker Compose. If a developer still has older tooling (or onlydocker-compose), this target will fail with an unhelpful error. Consider adding a quick feature check to emit a clear upgrade message.
This issue also appears on line 272 of the same file.
docker compose up -d --wait --no-deps $$(echo $* | tr + " ")
| # Bring edxapp containers online | ||
| for app in "${apps[@]}"; do | ||
| docker compose up -d $app | ||
| docker compose up -d --no-deps $app |
Comment on lines
+30
to
+34
| if [ -n "$$DEVSERVER_PRECHECK" ]; then eval "$$DEVSERVER_PRECHECK"; fi | ||
| if [ -n "$$DEVSERVER_CHECK" ]; then | ||
| eval "$$DEVSERVER_CHECK" || { echo "pre-flight check failed; giving up." >&2; exit 1; } | ||
| fi | ||
| while true; do |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 22 out of 22 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (4)
common.yml:40
- Typo in the usage comment:
dev.up.<servce>should bedev.up.<service>.
# Configure a healthcheck to allow `make dev.up.<servce>` to give accurate feedback about
# whether the service actually started successfully and is serving traffic.
# Set HEALTHCHECK_TARGET to the full URL curl should hit, e.g. "http://localhost:18000/heartbeat"
check:116
- The enterprise-access check block won’t run when users pass
enterprise-accessbecauseshould_checkis called withenterprise_access(underscore), and the health URL is pointing at the ecommerce port (18130) instead of enterprise-access (18270). This makes the check give false negatives/positives.
if should_check enterprise_access; then
echo "Checking enterprise-access health:"
run_check enterprise_access_heartbeat enterprise-access \
"curl --fail -L http://localhost:18130/health/"
fi
diff:5
- This looks like an accidentally committed patch/diff artifact. Keeping a file named
diffcontainingdiff --git ...output will confuse future readers and tooling; it should be removed from the repo/PR.
diff --git a/Makefile b/Makefile
index e78636a..c99172b 100644
--- a/Makefile
+++ b/Makefile
@@ -292,6 +292,9 @@ dev.restart-container: ## Restart all service containers.
check:12
- Comment typo: “Exists 0” should be “Exits 0”.
# Exists 0 if successful; non-zero otherwise.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 22 out of 22 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (20)
common.yml:40
- Typo in the comment:
dev.up.<servce>should bedev.up.<service>.
# Configure a healthcheck to allow `make dev.up.<servce>` to give accurate feedback about
# whether the service actually started successfully and is serving traffic.
# Set HEALTHCHECK_TARGET to the full URL curl should hit, e.g. "http://localhost:18000/heartbeat"
provision-xqueue.sh:6
--no-depsprevents Compose from starting xqueue’s dependencies (e.g., mysql80) during provisioning. Since provision.sh only brings up mysql80/mongo, this can cause--waitto block/fail when xqueue can’t start cleanly. For provisioning, it’s safer to keep--waitbut drop--no-depsso dependencies come up automatically.
# Bring up XQueue, we don't need the consumer for provisioning
docker compose up -d --wait --no-deps xqueue
provision-registrar.sh:10
--no-depsprevents registrar’s dependencies (mysql80, redis, etc.) from being started during provisioning. provision.sh does not start these, sodocker compose up --wait --no-depscan hang/fail. Drop--no-depshere so Compose will bring up dependencies as needed.
docker compose up -d --wait --no-deps $name
provision-notes.sh:12
--no-depsprevents the notes service’s dependencies (mysql80, elasticsearch, etc.) from being started during provisioning, but provision.sh doesn’t start them. With--wait, this can block/fail. Drop--no-depshere so dependencies are started automatically.
docker compose up -d --wait --no-deps $name
provision-lms.sh:16
--no-depsprevents LMS/CMS dependencies (memcached, elasticsearch, mongo, etc.) from being started during provisioning, but provision.sh only starts mysql80/mongo. With--wait, this can block/fail when runserver can’t come up healthy. Drop--no-depsso Compose starts required dependencies.
for app in "${apps[@]}"; do
docker compose up -d --wait --no-deps $app
done
provision-license-manager.sh:12
--no-depsprevents license-manager (and lms) dependencies from being started during provisioning. Since provision.sh doesn’t bring up redis/memcached/etc.,--wait --no-depscan hang/fail. Drop--no-depsso dependencies are started automatically.
docker compose up -d --wait --no-deps $name
docker compose up -d --wait --no-deps lms
provision-insights.sh:10
--no-depsprevents insights’ dependencies (mysql80, analyticsapi, memcached, etc.) from being started during provisioning, but provision.sh doesn’t start them. With--wait, this can block/fail. Drop--no-depsso Compose starts required dependencies.
docker compose up -d --wait --no-deps insights
provision-forum.sh:5
--no-depsprevents forum’s dependencies (mongo, memcached, search services, etc.) from being started during provisioning, but provision.sh doesn’t start them. With--wait, this can block/fail. Drop--no-depsso Compose brings up dependencies.
docker compose up -d --wait --no-deps forum
provision-enterprise-subsidy.sh:11
--no-depsprevents enterprise-subsidy (and lms) dependencies from being started during provisioning, but provision.sh doesn’t start most of them (memcached, redis, etc.). With--wait, this can block/fail. Drop--no-depsso dependencies are started automatically.
docker compose up -d --wait --no-deps lms
docker compose up -d --wait --no-deps ${name}
provision-enterprise-catalog.sh:4
--no-depsprevents enterprise-catalog’s dependencies (mysql80, memcached, redis, etc.) from being started during provisioning, but provision.sh doesn’t start them. With--wait, this can block/fail. Drop--no-depshere.
docker compose up -d --wait --no-deps $name
provision-enterprise-access.sh:8
--no-depsprevents enterprise-access and lms dependencies from being started during provisioning. provision.sh only brings up mysql80/mongo, so--wait --no-depscan hang/fail. Drop--no-depsso Compose starts required dependencies.
docker compose up -d --wait --no-deps $name lms
provision-edx-exams.sh:11
--no-depsprevents edx-exams (and lms) dependencies from being started during provisioning, but provision.sh doesn’t start most of them. With--wait, this can block/fail. Drop--no-depsso dependencies are started automatically.
docker compose up -d --wait --no-deps lms
docker compose up -d --wait --no-deps ${name}
provision-discovery.sh:8
- These services are started with
--no-deps, which prevents their dependencies from being brought up during provisioning. Since provision.sh does not start redis/memcached/elasticsearch/etc.,--wait --no-depscan hang/fail when these apps can’t become healthy. Drop--no-depshere so dependencies start automatically.
docker compose up -d --wait --no-deps lms
docker compose up -d --wait --no-deps cms
docker compose up -d --wait --no-deps ecommerce
provision-designer.sh:5
--no-depsprevents designer (and lms) dependencies from being started during provisioning. provision.sh only starts mysql80/mongo, so--wait --no-depscan hang/fail. Drop--no-depsso Compose starts dependencies.
docker compose up -d --wait --no-deps $name --build
docker compose up -d --wait --no-deps lms
provision-credentials.sh:13
--no-depsprevents credentials’ dependencies (mysql80, memcached, lms, etc.) from being started during provisioning, but provision.sh doesn’t start most of them. With--wait, this can block/fail. Drop--no-depshere.
docker compose up -d --wait --no-deps $name
provision-coursegraph.sh:16
--no-depsprevents coursegraph/cms dependencies from being started during provisioning. provision.sh doesn’t start most CMS deps (memcached, mongo, search, etc.), so--wait --no-depscan hang/fail. Drop--no-depsso dependencies are started automatically.
docker compose up -d --wait --no-deps coursegraph cms
provision-analyticsapi.sh:10
--no-depsprevents analyticsapi’s dependencies (mysql80, elasticsearch, etc.) from being started during provisioning, but provision.sh doesn’t start them. With--wait, this can block/fail. Drop--no-depshere.
docker compose up -d --wait --no-deps ${name}
check:4
- This new
checkscript is currently unused: the Makefile still runsbash ./check.sh $*(Makefile:335). Having bothcheckandcheck.shwith the same content is confusing; either (a) remove this file, or (b) complete the rename by updating the Makefile and deletingcheck.sh.
#!/usr/bin/env bash
# Run checks for the provided service(s).
# To specify multiple services, separate them with spaces or plus signs (+).
# To specify all services, just pass in "all".
check:115
- The enterprise-access check is currently incorrect:
should_checkusesenterprise_access(underscore), so it won’t match the actual service nameenterprise-access, and it probes port 18130 (ecommerce) instead of 18270.
if should_check enterprise_access; then
echo "Checking enterprise-access health:"
run_check enterprise_access_heartbeat enterprise-access \
"curl --fail -L http://localhost:18130/health/"
diff:5
- This file appears to be an accidentally committed patch/dump (it contains a nested
diff --git ...rather than code used by devstack). It should be removed from the repository to avoid confusion and accidental use.
diff --git a/Makefile b/Makefile
index e78636a..c99172b 100644
--- a/Makefile
+++ b/Makefile
@@ -292,6 +292,9 @@ dev.restart-container: ## Restart all service containers.
pwnage101
force-pushed
the
pwnage101/enhance-startup-status-reporting
branch
2 times, most recently
from
July 31, 2026 17:55
a62444b to
aeb55ed
Compare
All too often I'll run dev.up and get all green, but eventually discover that half the services did not actually start for one reason or another. The main issue isn't having to fix them, it's not knowing which ones failed and/or not knowing that any failed at all. Poor UX. Changes in this commit: Now, dev.up will not only report when each container has *started*, but also it will wait for the services to become *healthy* and exit fatally if any service fails to become healthy (most commonly due to missing imports). Other bug fixes: - course-discovery has been broken in devstack for several months because it was still pointing at the upstream repo. I Fixed both the repo URL and image name to point to the edx fork. - make dev.check.enterprise-access actually checks the correct port now. This commit also just does a lot to make everything a bit more normalized and DRY across the board: - Consolidated common configs into common.yml. - Removed several absolute paths when relative ones were sufficient. - Removed dead code which attempted to source legacy <service>_env files which have been empty for years. - Migrated everything away from deprecated depends_on syntax lacking an explicit condition. - Removed dead references to historic analytics containers (hadoop, vertica, etc.) - Simplified check.sh to remove redundant health check commands (it reads docker-reported health now).
pwnage101
force-pushed
the
pwnage101/enhance-startup-status-reporting
branch
from
July 31, 2026 18:14
aeb55ed to
1b944a9
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
All too often I'll run dev.up and get all green, but eventually discover that half the services did not actually start for one reason or another. The main issue isn't having to fix them, it's not knowing which ones failed and/or not knowing that any failed at all. Terrible UX.
This change makes it so that dev.up will not only report when each service has started, but also wait for them to become healthy, and also clearly state when a service fails to become healthy (e.g. missing imports).
This PR also just does a lot to make everything a bit more normalized and DRY across the board.
<service>_envfiles which have been empty for years.depends_onsyntax lacking an explicit condition.