Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 39 additions & 5 deletions Dockerfile
Original file line number Diff line number Diff line change
Expand Up @@ -119,8 +119,8 @@ RUN rm -rf openms-build
# Prepare and run streamlit app.
FROM compile-openms AS run-app

# Install Redis server for job queue
RUN apt-get update && apt-get install -y --no-install-recommends redis-server \
# Install Redis server for job queue and nginx for load balancing
RUN apt-get update && apt-get install -y --no-install-recommends redis-server nginx \
&& rm -rf /var/lib/apt/lists/*

# Create Redis data directory
Expand Down Expand Up @@ -154,6 +154,10 @@ RUN echo "0 3 * * * /root/miniforge3/envs/streamlit-env/bin/python /app/clean-up
ENV RQ_WORKER_COUNT=1
ENV REDIS_URL=redis://localhost:6379/0

# Number of Streamlit server instances for load balancing (default: 1 = no load balancer)
# Set to >1 to enable nginx load balancer with multiple Streamlit instances
ENV STREAMLIT_SERVER_COUNT=1

# create entrypoint script to start cron, Redis, RQ workers, and Streamlit
RUN echo -e '#!/bin/bash\n\
set -e\n\
Expand All @@ -180,9 +184,39 @@ for i in $(seq 1 $WORKER_COUNT); do\n\
rq worker openms-workflows --url $REDIS_URL --name worker-$i &\n\
done\n\
\n\
# Start Streamlit (foreground - main process)\n\
echo "Starting Streamlit app..."\n\
exec streamlit run app.py\n\
# Load balancer setup\n\
SERVER_COUNT=${STREAMLIT_SERVER_COUNT:-1}\n\
\n\
if [ "$SERVER_COUNT" -gt 1 ]; then\n\
echo "Starting $SERVER_COUNT Streamlit instances with nginx load balancer..."\n\
\n\
# Generate nginx upstream block\n\
UPSTREAM_SERVERS=""\n\
BASE_PORT=8510\n\
for i in $(seq 0 $((SERVER_COUNT - 1))); do\n\
PORT=$((BASE_PORT + i))\n\
UPSTREAM_SERVERS="${UPSTREAM_SERVERS} server 127.0.0.1:${PORT};\\n"\n\
done\n\
\n\
# Write nginx config\n\
mkdir -p /etc/nginx\n\
echo -e "worker_processes auto;\\npid /run/nginx.pid;\\n\\nevents {\\n worker_connections 1024;\\n}\\n\\nhttp {\\n client_max_body_size 0;\\n\\n map \\$cookie_stroute \\$route_key {\\n \\x22\\x22 \\$request_id;\\n default \\$cookie_stroute;\\n }\\n\\n upstream streamlit_backend {\\n hash \\$route_key consistent;\\n${UPSTREAM_SERVERS} }\\n\\n map \\$http_upgrade \\$connection_upgrade {\\n default upgrade;\\n \\x27\\x27 close;\\n }\\n\\n server {\\n listen 8501;\\n\\n location / {\\n proxy_pass http://streamlit_backend;\\n proxy_http_version 1.1;\\n proxy_set_header Upgrade \\$http_upgrade;\\n proxy_set_header Connection \\$connection_upgrade;\\n proxy_set_header Host \\$host;\\n proxy_set_header X-Real-IP \\$remote_addr;\\n proxy_set_header X-Forwarded-For \\$proxy_add_x_forwarded_for;\\n proxy_set_header X-Forwarded-Proto \\$scheme;\\n proxy_read_timeout 86400;\\n proxy_send_timeout 86400;\\n proxy_buffering off;\\n add_header Set-Cookie \\x22stroute=\\$route_key; Path=/; HttpOnly; SameSite=Lax\\x22 always;\\n }\\n }\\n}" > /etc/nginx/nginx.conf\n\
\n\
# Start Streamlit instances on internal ports (localhost only)\n\
for i in $(seq 0 $((SERVER_COUNT - 1))); do\n\
PORT=$((BASE_PORT + i))\n\
echo "Starting Streamlit instance on port $PORT..."\n\
streamlit run app.py --server.port $PORT --server.address 127.0.0.1 &\n\
done\n\
\n\
sleep 2\n\
echo "Starting nginx load balancer on port 8501..."\n\
exec /usr/sbin/nginx -g "daemon off;"\n\
else\n\
# Single instance mode (default) - run Streamlit directly on port 8501\n\
echo "Starting Streamlit app..."\n\
exec streamlit run app.py\n\
fi\n\
Comment on lines 187 to 219
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Compounded unsupervised-process problem: Redis + RQ workers + Streamlit instances + nginx.

In multi-instance mode, this container runs at least 4+ background daemons (Redis, N RQ workers, M Streamlit instances) with nginx as PID 1. None of the background processes have supervision or restart capability, and nginx won't reap zombie children.

This is the same issue flagged in Dockerfile_simple but amplified here. A process supervisor like supervisord would be strongly recommended for this container, or at minimum tini as the init process.

🤖 Prompt for AI Agents
In `@Dockerfile` around lines 187 - 218, The Dockerfile spawns multiple background
services (Redis, RQ workers, multiple Streamlit instances created from the
SERVER_COUNT loop, and nginx started with exec nginx -g "daemon off;") without a
proper init/supervisor to reap zombies or restart failed services; replace the
current ad-hoc backgrounding with a real process supervisor (e.g., supervisord)
or at minimum use tini as PID 1: add tini as the ENTRYPOINT and
install/configure supervisord to run and manage redis, the RQ worker processes,
each streamlit instance (those started in the for loop using streamlit run
app.py --server.port $PORT --server.address 127.0.0.1), and nginx (instead of
exec nginx -g "daemon off;"), or create a supervisord config that defines
programs for redis, rqworker, streamlit instances (parametrized by
SERVER_COUNT), and nginx so they are supervised, can restart, and are correctly
reaped.

' > /app/entrypoint.sh
# make the script executable
RUN chmod +x /app/entrypoint.sh
Expand Down
51 changes: 46 additions & 5 deletions Dockerfile_simple
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ USER root

RUN apt-get -y update
# note: streamlit in docker needs libgtk2.0-dev (see https://yugdamor.medium.com/importerror-libgthread-2-0-so-0-cannot-open-shared-object-file-no-such-file-or-directory-895b94a7827b)
RUN apt-get install -y --no-install-recommends --no-install-suggests wget ca-certificates libgtk2.0-dev curl jq cron
RUN apt-get install -y --no-install-recommends --no-install-suggests wget ca-certificates libgtk2.0-dev curl jq cron nginx
RUN update-ca-certificates

# Install Github CLI
Expand Down Expand Up @@ -84,11 +84,52 @@ COPY clean-up-workspaces.py /app/clean-up-workspaces.py
# add cron job to the crontab
RUN echo "0 3 * * * /root/miniforge3/envs/streamlit-env/bin/python /app/clean-up-workspaces.py >> /app/clean-up-workspaces.log 2>&1" | crontab -

# Number of Streamlit server instances for load balancing (default: 1 = no load balancer)
# Set to >1 to enable nginx load balancer with multiple Streamlit instances
ENV STREAMLIT_SERVER_COUNT=1

# create entrypoint script to start cron service and launch streamlit app
RUN echo "#!/bin/bash" > /app/entrypoint.sh
RUN echo "source /root/miniforge3/bin/activate streamlit-env" >> /app/entrypoint.sh && \
echo "service cron start" >> /app/entrypoint.sh && \
echo "streamlit run app.py" >> /app/entrypoint.sh
RUN echo -e '#!/bin/bash\n\
set -e\n\
source /root/miniforge3/bin/activate streamlit-env\n\
\n\
# Start cron for workspace cleanup\n\
service cron start\n\
\n\
# Load balancer setup\n\
SERVER_COUNT=${STREAMLIT_SERVER_COUNT:-1}\n\
\n\
if [ "$SERVER_COUNT" -gt 1 ]; then\n\
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Non-numeric STREAMLIT_SERVER_COUNT will crash the entrypoint with an unhelpful error.

The [ "$SERVER_COUNT" -gt 1 ] comparison will fail with a confusing error if the env var is set to a non-integer value (e.g., "auto" or ""). Consider adding input validation early in the script.

🛠️ Proposed validation
# Validate SERVER_COUNT is a positive integer
if ! [[ "$SERVER_COUNT" =~ ^[0-9]+$ ]] || [ "$SERVER_COUNT" -lt 1 ]; then
    echo "ERROR: STREAMLIT_SERVER_COUNT must be a positive integer, got: '$SERVER_COUNT'" >&2
    exit 1
fi
🤖 Prompt for AI Agents
In `@Dockerfile_simple` at line 102, Validate the
STREAMLIT_SERVER_COUNT/SERVER_COUNT env var before using the numeric comparison
in the entrypoint: check that "$SERVER_COUNT" matches a positive-integer regex
and is >=1, and if not print a clear error to stderr and exit non‑zero; then you
can safely use the existing conditional that contains [ "$SERVER_COUNT" -gt 1 ].
Ensure you update the variable name references (STREAMLIT_SERVER_COUNT ->
SERVER_COUNT) consistently and place the validation before the line with [
"$SERVER_COUNT" -gt 1 ].

echo "Starting $SERVER_COUNT Streamlit instances with nginx load balancer..."\n\
\n\
# Generate nginx upstream block\n\
UPSTREAM_SERVERS=""\n\
BASE_PORT=8510\n\
for i in $(seq 0 $((SERVER_COUNT - 1))); do\n\
PORT=$((BASE_PORT + i))\n\
UPSTREAM_SERVERS="${UPSTREAM_SERVERS} server 127.0.0.1:${PORT};\\n"\n\
done\n\
\n\
# Write nginx config\n\
mkdir -p /etc/nginx\n\
echo -e "worker_processes auto;\\npid /run/nginx.pid;\\n\\nevents {\\n worker_connections 1024;\\n}\\n\\nhttp {\\n client_max_body_size 0;\\n\\n map \\$cookie_stroute \\$route_key {\\n \\x22\\x22 \\$request_id;\\n default \\$cookie_stroute;\\n }\\n\\n upstream streamlit_backend {\\n hash \\$route_key consistent;\\n${UPSTREAM_SERVERS} }\\n\\n map \\$http_upgrade \\$connection_upgrade {\\n default upgrade;\\n \\x27\\x27 close;\\n }\\n\\n server {\\n listen 8501;\\n\\n location / {\\n proxy_pass http://streamlit_backend;\\n proxy_http_version 1.1;\\n proxy_set_header Upgrade \\$http_upgrade;\\n proxy_set_header Connection \\$connection_upgrade;\\n proxy_set_header Host \\$host;\\n proxy_set_header X-Real-IP \\$remote_addr;\\n proxy_set_header X-Forwarded-For \\$proxy_add_x_forwarded_for;\\n proxy_set_header X-Forwarded-Proto \\$scheme;\\n proxy_read_timeout 86400;\\n proxy_send_timeout 86400;\\n proxy_buffering off;\\n add_header Set-Cookie \\x22stroute=\\$route_key; Path=/; HttpOnly; SameSite=Lax\\x22 always;\\n }\\n }\\n}" > /etc/nginx/nginx.conf\n\
\n\
# Start Streamlit instances on internal ports (localhost only)\n\
for i in $(seq 0 $((SERVER_COUNT - 1))); do\n\
PORT=$((BASE_PORT + i))\n\
echo "Starting Streamlit instance on port $PORT..."\n\
streamlit run app.py --server.port $PORT --server.address 127.0.0.1 &\n\
done\n\
\n\
sleep 2\n\
echo "Starting nginx load balancer on port 8501..."\n\
exec /usr/sbin/nginx -g "daemon off;"\n\
else\n\
# Single instance mode (default) - run Streamlit directly on port 8501\n\
echo "Starting Streamlit app..."\n\
exec streamlit run app.py\n\
fi\n\
' > /app/entrypoint.sh
Comment on lines 92 to 132
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion | 🟠 Major

Massive inline script is fragile and hard to maintain — consider an external file.

The entrypoint script is generated via a single echo -e with deeply nested escaping (especially the nginx config on line 114). This makes the script nearly impossible to read, debug, or modify safely. A single misplaced \n or \\ will silently produce a broken script or nginx config at runtime, with no build-time feedback.

Consider using a separate entrypoint_simple.sh file and COPYing it into the image (which is what the PR description says was done, but the actual code still uses inline generation).

🛠️ Proposed approach

Create a file entrypoint_simple.sh alongside the Dockerfile with the script contents, then in the Dockerfile:

-RUN echo -e '#!/bin/bash\n\
-set -e\n\
-...long inline script...
-' > /app/entrypoint.sh
+COPY entrypoint_simple.sh /app/entrypoint.sh

This gives you:

  • Syntax highlighting and linting in your editor/CI
  • Shell linting via shellcheck
  • Readable nginx config generation
  • Easier diffs in future PRs
🤖 Prompt for AI Agents
In `@Dockerfile_simple` around lines 92 - 131, The Dockerfile currently builds a
fragile inline entrypoint by echoing a large heredoc into /app/entrypoint.sh
(see the block creating /app/entrypoint.sh which references
STREAMLIT_SERVER_COUNT, UPSTREAM_SERVERS, BASE_PORT and the nginx config);
replace this by moving the entire script to a new file named
entrypoint_simple.sh in the repo, COPY that file into the image in the
Dockerfile, chmod +x it, and change the Dockerfile to use that script as the
container entrypoint (avoid the echo -e generation and inline nginx config),
keeping the same runtime logic (cron start, multi-instance loop, nginx exec or
single exec streamlit) so symbols like STREAMLIT_SERVER_COUNT, BASE_PORT, and
the nginx upstream/template remain unchanged.

# make the script executable
RUN chmod +x /app/entrypoint.sh

Expand Down
5 changes: 4 additions & 1 deletion docker-compose.yml
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,9 @@ services:
- 8501:8501
volumes:
- workspaces-streamlit-template:/workspaces-streamlit-template
command: streamlit run openms-streamlit-template/app.py
environment:
# Number of Streamlit server instances (default: 1 = no load balancer).
# Set to >1 to enable nginx load balancing across multiple Streamlit instances.
- STREAMLIT_SERVER_COUNT=1
volumes:
workspaces-streamlit-template:
Loading