Skip to content

Commit 4079ef3

Browse files
committed
feat: dev.up now waits until services are actually healthy
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).
1 parent 4487f94 commit 4079ef3

16 files changed

Lines changed: 399 additions & 321 deletions

.github/workflows/provisioning-tests.yml

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -73,7 +73,10 @@ jobs:
7373
run: make dev.provision.${{matrix.services}}
7474

7575
- name: "Bring up services"
76-
run: make dev.up.${{matrix.services}}
76+
# Use the without-wait variant: this job only provisions a subset of
77+
# services, so their un-provisioned transitive dependencies can never
78+
# become healthy.
79+
run: make dev.up.without-wait.${{matrix.services}}
7780

7881
- name: "Wait for services to become ready"
7982
run: |

Makefile

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,8 @@
5555
dev.shell.cms_watcher dev.shell.xqueue dev.shell.xqueue_consumer \
5656
dev.static dev.static.lms dev.static.cms dev.stats dev.status \
5757
dev.stop dev.up dev.up.attach dev.up.shell \
58-
dev.up.without-deps dev.up.without-deps.shell dev.up.with-programs \
58+
dev.up.without-deps dev.up.without-deps.shell dev.up.without-wait \
59+
dev.up.with-programs \
5960
dev.up.with-watchers dev.validate docs \
6061
help requirements impl-dev.clone.https impl-dev.clone.ssh impl-dev.provision \
6162
impl-dev.pull impl-dev.pull.without-deps impl-dev.up impl-dev.up.attach \
@@ -254,22 +255,27 @@ dev.up.with-watchers.%: ## Bring up services and their dependencies + asset watc
254255
dev.up.without-deps: _expects-service-list.dev.up.without-deps
255256

256257
dev.up.without-deps.%: dev.check-memory ## Bring up services by themselves.
257-
docker compose up -d --no-deps $$(echo $* | tr + " ")
258+
docker compose up -d --wait --no-deps $$(echo $* | tr + " ")
258259

259260
dev.up.without-deps.shell: _expects-service.dev.up.without-deps.shell
260261

261262
dev.up.without-deps.shell.%: ## Bring up a service by itself + shell into it.
262263
make dev.up.without-deps.$*
263264
make dev.shell.$*
264265

266+
dev.up.without-wait: _expects-service-list.dev.up.without-wait
267+
268+
dev.up.without-wait.%: dev.check-memory ## Bring up services and their dependencies without waiting for them to become healthy.
269+
docker compose up -d $$(echo $* | tr + " ")
270+
265271
dev.up:
266272
@scripts/make_warn_default_large.sh "$@"
267273

268274
dev.up.large-and-slow: dev.up.$(DEFAULT_SERVICES) ## Bring up default services.
269275
@echo # at least one statement so that dev.up.% doesn't run too
270276

271277
dev.up.%: dev.check-memory ## Bring up services and their dependencies.
272-
docker compose up -d $$(echo $* | tr + " ")
278+
docker compose up -d --wait $$(echo $* | tr + " ")
273279
ifeq ($(ALWAYS_CACHE_PROGRAMS),true)
274280
make dev.cache-programs
275281
endif
@@ -552,9 +558,6 @@ validate-lms-volume: ## Validate that changes to the local workspace are reflect
552558
docker compose exec -T lms ls /edx/app/edxapp/edx-platform/testfile
553559
rm $(DEVSTACK_WORKSPACE)/edx-platform/testfile
554560

555-
hadoop-application-logs-%: ## View hadoop logs by application Id.
556-
docker compose exec nodemanager yarn logs -applicationId $*
557-
558561
create-test-course: ## Provisions cms, and ecommerce with course(s) in test-course.json.
559562
bash ./course-generator/create-courses.sh --cms --ecommerce course-generator/test-course.json
560563

check.sh

Lines changed: 67 additions & 139 deletions
Original file line numberDiff line numberDiff line change
@@ -1,15 +1,15 @@
11
#!/usr/bin/env bash
2-
# Run checks for the provided service(s).
2+
# Run health checks for the provided service(s).
33
# To specify multiple services, separate them with spaces or plus signs (+).
4-
# To specify all services, just pass in "all".
4+
# To specify all running services, just pass in "all".
55
#
66
# Examples:
77
# ./check.sh lms
88
# ./check.sh lms+forum
99
# ./check.sh lms+forum discovery
1010
# ./check.sh all
1111
#
12-
# Exists 0 if successful; non-zero otherwise.
12+
# Exits 0 if successful; non-zero otherwise.
1313
#
1414
# Fails if no services specified.
1515
#
@@ -18,25 +18,11 @@
1818

1919
set -eu -o pipefail
2020

21-
# Grab all arguments into one string, replacing plus signs with spaces.
22-
# Pad on either side with spaces so that the regex in `should_check` works correctly.
23-
services=" ${*//+/ } "
24-
2521
# Which checks succeeded and failed.
2622
succeeded=""
2723
failed=""
2824

29-
# Returns whether service in first arg should be checked.
30-
should_check() {
31-
local service="$1"
32-
if [[ "$services" == *" all "* ]] || [[ "$services" == *" $service "* ]]; then
33-
return 0 # Note that '0' means 'success' (i.e., true) in bash.
34-
else
35-
return 1
36-
fi
37-
}
38-
39-
# Runs a check named $1 on service $2 using the command $3.
25+
# Runs a check named $1 on service $2 using the host-side command $3.
4026
run_check() {
4127
local check_name="$1"
4228
local service="$2"
@@ -53,133 +39,75 @@ run_check() {
5339
echo # Newline
5440
}
5541

56-
mysql_run_check() {
57-
container_name="$1"
58-
mysql_probe="SELECT EXISTS(SELECT 1 FROM mysql.user WHERE user = 'root')"
59-
# The use of `--protocol tcp` forces MySQL to connect over TCP rather than
60-
# via a UNIX socket. This is needed because when MySQL starts for the first
61-
# time in a new container, it starts a "temporary server" that runs for a
62-
# few seconds and then shuts down before the "real" server starts up. The
63-
# temporary server does not listen on the TCP port, but if the mysql
64-
# command is not told which server to use, it will first try the UNIX
65-
# socket and only after that will it try the default TCP port.
66-
#
67-
# By specifying that mysql should use TCP, we won't get an early false
68-
# positive "ready" response while the temporary server is running.
69-
run_check "${container_name}_query" "$container_name" \
70-
"docker compose exec -T $(printf %q "$container_name") mysql --protocol tcp -uroot -se $(printf %q "$mysql_probe")"
42+
# Print a service container's Docker healthcheck status, one of:
43+
# healthy | unhealthy | starting | none | missing
44+
# "none" means the container exists but declares no healthcheck; "missing"
45+
# means no container is running for the service.
46+
container_health() {
47+
local service="$1" cid
48+
cid="$(docker compose ps -q "$service" 2>/dev/null || true)"
49+
if [[ -z "$cid" ]]; then
50+
echo "missing"
51+
return
52+
fi
53+
docker inspect \
54+
--format '{{if .State.Health}}{{.State.Health.Status}}{{else}}none{{end}}' \
55+
"$cid" 2>/dev/null || echo "missing"
7156
}
7257

73-
if should_check mysql80; then
74-
echo "Checking MySQL 8.0 query endpoint:"
75-
mysql_run_check mysql80
76-
fi
77-
78-
if should_check mongo; then
79-
echo "Checking MongoDB status:"
80-
run_check mongo_status mongo \
81-
"docker compose exec -T mongo mongo --eval \"db.serverStatus()\""
82-
fi
83-
84-
if should_check registrar; then
85-
echo "Checking Registrar heartbeat:"
86-
run_check registrar_heartbeat registrar \
87-
"curl --fail -L http://localhost:18734/health"
88-
fi
89-
90-
if should_check lms; then
91-
echo "Checking LMS heartbeat:"
92-
run_check lms_heartbeat lms \
93-
"curl --fail -L http://localhost:18000/heartbeat"
94-
95-
echo "Validating LMS volume:"
96-
run_check lms_volume lms \
97-
"make validate-lms-volume"
98-
fi
99-
100-
if should_check cms; then
101-
echo "Checking CMS heartbeat:"
102-
run_check cms_heartbeat cms \
103-
"curl --fail -L http://localhost:18010/heartbeat"
104-
fi
105-
106-
if should_check ecommerce; then
107-
echo "Checking ecommerce health:"
108-
run_check ecommerce_heartbeat ecommerce \
109-
"curl --fail -L http://localhost:18130/health/"
110-
fi
111-
112-
if should_check enterprise_access; then
113-
echo "Checking enterprise-access health:"
114-
run_check enterprise_access_heartbeat enterprise-access \
115-
"curl --fail -L http://localhost:18130/health/"
116-
fi
117-
118-
if should_check enterprise-subsidy; then
119-
echo "Checking enterprise_subsidy health:"
120-
run_check enterprise-subsidy_heartbeat enterprise-subsidy \
121-
"curl --fail -L http://localhost:18280/health/"
122-
fi
123-
124-
if should_check discovery; then
125-
echo "Checking discovery health:"
126-
run_check discovery_heartbeat discovery \
127-
"curl --fail -L http://localhost:18381/health/"
128-
fi
129-
130-
if should_check forum; then
131-
echo "Checking forum heartbeat:"
132-
run_check forum_heartbeat forum \
133-
"curl --fail -L http://localhost:44567/heartbeat"
134-
fi
135-
136-
if should_check edx_notes_api; then
137-
echo "Checking edx_notes_api heartbeat:"
138-
run_check edx_notes_api_heartbeat edx_notes_api \
139-
"curl --fail -L http://localhost:18120/heartbeat"
140-
fi
141-
142-
if should_check designer; then
143-
echo "Checking designer health:"
144-
run_check designer_heartbeat designer \
145-
"curl --fail -L http://localhost:18808/health/"
146-
fi
147-
148-
if should_check credentials; then
149-
echo "Checking credentials heartbeat:"
150-
run_check credentials_heartbeat credentials \
151-
"curl --fail -L http://localhost:18150/health"
152-
fi
153-
154-
if should_check xqueue; then
155-
echo "Checking xqueue status:"
156-
run_check xqueue_heartbeat xqueue \
157-
"curl --fail -L http://localhost:18040/xqueue/status"
158-
fi
159-
160-
if should_check insights; then
161-
echo "Running Analytics Dashboard Devstack tests: "
162-
run_check insights_heartbeat insights \
163-
"curl --fail -L http://localhost:18110/health/"
164-
fi
58+
# Default check: pass iff the service's container reports itself healthy.
59+
# Services with no healthcheck (and no extra check) simply don't contribute a
60+
# check, matching the old behavior for unrecognized services.
61+
run_health_check() {
62+
local service="$1"
63+
local status
64+
status="$(container_health "$service")"
65+
case "$status" in
66+
healthy)
67+
echo "Checking $service: healthy"
68+
succeeded="$succeeded ${service}_health"
69+
echo
70+
;;
71+
starting|unhealthy)
72+
echo "Checking $service: $status"
73+
docker compose logs --tail 500 "$service"
74+
failed="$failed ${service}_health"
75+
echo
76+
;;
77+
none|missing)
78+
# No container healthcheck to consult; rely on any extra checks.
79+
:
80+
;;
81+
esac
82+
}
16583

166-
if should_check analyticsapi; then
167-
echo "Running Analytics Data API Devstack tests: "
168-
run_check analyticsapi_heartbeat analyticsapi \
169-
"curl --fail -L http://localhost:19001/health/"
170-
fi
84+
# Extra/override checks for services that need more than their container
85+
# healthcheck can express. Keep this SMALL -- it is the only place service
86+
# names should be enumerated.
87+
run_extra_checks() {
88+
local service="$1"
89+
case "$service" in
90+
lms)
91+
echo "Validating LMS volume:"
92+
run_check lms_volume lms "make validate-lms-volume"
93+
;;
94+
esac
95+
}
17196

172-
if should_check license-manager; then
173-
echo "Running License Manager Devstack tests: "
174-
run_check license_manager_heartbeat license-manager \
175-
"curl --fail -L http://localhost:18170/health/"
97+
# Expand the requested services into a plain, space-separated list. "all"
98+
# means every service with a running container (word-splitting is safe here
99+
# because compose service names contain no whitespace).
100+
requested=" ${*//+/ } "
101+
if [[ "$requested" == *" all "* ]]; then
102+
service_list="$(docker compose ps --services)"
103+
else
104+
service_list="${*//+/ }"
176105
fi
177106

178-
if should_check edx-exams; then
179-
echo "Running edX Exam Devstack tests: "
180-
run_check edx-exams_heartbeat edx-exams \
181-
"curl --fail -L http://localhost:18740/health/"
182-
fi
107+
for service in $service_list; do
108+
run_health_check "$service"
109+
run_extra_checks "$service"
110+
done
183111

184112
echo "Successful checks:${succeeded:- NONE}"
185113
echo "Failed checks:${failed:- NONE}"

common.yml

Lines changed: 87 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,87 @@
1+
# Abstract base services, inherited by concrete services via `extends`.
2+
services:
3+
4+
# Traffic-serving backend app (Django/IDA) services.
5+
backend-app:
6+
stdin_open: true # Allows `make dev.attach.<service>` to work correctly.
7+
tty: true
8+
# Concrete services may set these extra environment variables:
9+
#
10+
# - DEVSERVER_SETUP allows some services to run extra commands at the very beginning.
11+
# This is rarely needed.
12+
# - DEVSERVER_CMD runs inside an infinite loop to allow `make dev.restart-devserver.<service>`
13+
# to work correctly.
14+
#
15+
# Examples:
16+
#
17+
# environment:
18+
# DEVSERVER_SETUP: "source /edx/app/foo/foo_env" # optional
19+
# DEVSERVER_CMD: "python manage.py runserver 0.0.0.0:18000"
20+
command:
21+
- bash
22+
- -c
23+
- |
24+
if [ -n "$$DEVSERVER_SETUP" ]; then eval "$$DEVSERVER_SETUP"; fi
25+
while true; do
26+
eval "$$DEVSERVER_CMD"
27+
sleep 2
28+
done
29+
# Configure a healthcheck to allow `make dev.up.<service>` to give accurate feedback
30+
# about whether the service actually started successfully and is serving traffic.
31+
#
32+
# Set HEALTHCHECK_TARGET to the full URL curl should hit, e.g.:
33+
#
34+
# environment:
35+
# HEALTHCHECK_TARGET: "http://localhost:18150/health/"
36+
#
37+
# Timings are sized so the worst case from container start to "give up"
38+
# (marked unhealthy) is ~5 minutes: start_period + retries*interval = 300s = 5min.
39+
healthcheck:
40+
test: ["CMD-SHELL", "curl -fsS $$HEALTHCHECK_TARGET || exit 1"]
41+
interval: 10s
42+
timeout: 10s
43+
retries: 6
44+
start_period: 240s
45+
start_interval: 5s # poll fast during grace period so a healthy boot flips status quickly
46+
47+
# All microfrontends.
48+
microfrontend:
49+
# Use `npm ci` rather than `npm install` for a few reasons:
50+
#
51+
# - Repeatability: Respect the currently checked out package
52+
# versions rather than upgrading when package.json and
53+
# package-lock.json don't match. (Two people using this at
54+
# different times on the same commit should get the same
55+
# results.)
56+
# - Immutability: Don't change the repo's working directory
57+
# unexpectedly when there's a lock mismatch.
58+
#
59+
# Fail fast if package install fails to avoid mysterious
60+
# errors later.
61+
command:
62+
- bash
63+
- -c
64+
- |
65+
npm ci || exit 1
66+
if [ -n "$${PARAGON_BRAND_PACKAGE}" ]; then
67+
npx paragon install-theme "$${PARAGON_BRAND_PACKAGE}" || exit 1
68+
fi
69+
while true; do
70+
npm start
71+
sleep 2
72+
done
73+
stdin_open: true
74+
tty: true
75+
image: node:18
76+
environment:
77+
- NODE_ENV=development
78+
# MFEs take longer to startup: a cold `npm ci` + `npm start` is empirically
79+
# observed to reach healthy in about 5 minutes, so we shouldn't give up
80+
# until at least ~10 minutes. start_period + retries*interval = 600s = 10min.
81+
healthcheck:
82+
test: ["CMD-SHELL", "curl -fsS $$HEALTHCHECK_TARGET || exit 1"]
83+
interval: 10s
84+
timeout: 10s
85+
retries: 6
86+
start_period: 540s
87+
start_interval: 5s # poll fast during grace period so a healthy boot flips status quickly

0 commit comments

Comments
 (0)