RStudio: Replace supervisord and fcgi with httpd #2530
Closed
Conversation
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
WalkthroughReplaces fcgiwrap/supervisord CGI flow with Apache httpd across c9s and rhel9 Python 3.11 images, adds APP_ROOT-based paths, new httpd configs and CGI mappings, updates nginx templates to proxy /api* to httpd:8080, adjusts Dockerfiles, startup scripts, file copies and permissions, and removes supervisord configs. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
bot
added
the
review-requested
|
/hold This is for rhoai-3.0 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
rstudio/c9s-python-3.11/nginx/serverconf/proxy.conf.template (1)
11-20: /api/ still uses fcgiwrap; switch it to proxy httpd or it will 502.The PR removes fcgiwrap/supervisord. This block still forwards to the fcgiwrap socket and will break.
-location /api/ { - index probe.cgi; - fastcgi_index probe.cgi; - gzip off; - access_log off; - root /opt/app-root; - fastcgi_pass unix:/var/run/fcgiwrap.socket; - include /etc/nginx/fastcgi_params; - fastcgi_param SCRIPT_FILENAME /opt/app-root$fastcgi_script_name; -} +location /api/ { + # Ensure httpd serves CGI; httpd should have DirectoryIndex probe.cgi under /opt/app-root/api + proxy_pass http://localhost:8080; + proxy_set_header Host $host; + proxy_set_header X-Real-IP $remote_addr; + proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for; + proxy_set_header X-Forwarded-Proto $scheme; + gzip off; + access_log off; +}rstudio/rhel9-python-3.11/nginx/serverconf/proxy.conf.template_nbprefix (1)
21-30: /api/ still uses fcgiwrap; switch it to proxy httpd.Same issue as the c9s template; fcgiwrap is gone.
-location /api/ { - index probe.cgi; - fastcgi_index probe.cgi; - gzip off; - access_log off; - root /opt/app-root; - fastcgi_pass unix:/var/run/fcgiwrap.socket; - include /etc/nginx/fastcgi_params; - fastcgi_param SCRIPT_FILENAME /opt/app-root$fastcgi_script_name; -} +location /api/ { + proxy_pass http://localhost:8080; + proxy_set_header Host $host; + proxy_set_header X-Real-IP $remote_addr; + proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for; + proxy_set_header X-Forwarded-Proto $scheme; + gzip off; + access_log off; +}rstudio/c9s-python-3.11/nginx/serverconf/proxy.conf.template_nbprefix (1)
21-30: /api/ still uses fcgiwrap; switch it to proxy httpd.Mirror the fix from other templates.
-location /api/ { - index probe.cgi; - fastcgi_index probe.cgi; - gzip off; - access_log off; - root /opt/app-root; - fastcgi_pass unix:/var/run/fcgiwrap.socket; - include /etc/nginx/fastcgi_params; - fastcgi_param SCRIPT_FILENAME /opt/app-root$fastcgi_script_name; -} +location /api/ { + proxy_pass http://localhost:8080; + proxy_set_header Host $host; + proxy_set_header X-Real-IP $remote_addr; + proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for; + proxy_set_header X-Forwarded-Proto $scheme; + gzip off; + access_log off; +}
🧹 Nitpick comments (10)
rstudio/c9s-python-3.11/httpd/rstudio-cgi.conf (1)
5-9: Narrow CGI handler scope (avoid SetHandler on whole location).Setting
SetHandler cgi-scriptat the Location level executes any matched resource as CGI. Prefer restricting execution to .cgi files in the target directory.<LocationMatch "^/api/kernels/"> - SetHandler cgi-script Options +ExecCGI Require all granted </LocationMatch>rstudio/rhel9-python-3.11/run-rstudio.sh (1)
7-9: Don't mix “-D FOREGROUND” with backgrounding.
httpd -D FOREGROUND &is contradictory. Either run in foreground (PID 1) or background cleanly. Since you background it, drop-D FOREGROUND.- /usr/sbin/httpd -D FOREGROUND & + /usr/sbin/httpd &rstudio/c9s-python-3.11/run-rstudio.sh (1)
7-9: Don't mix “-D FOREGROUND” with backgrounding.Apply the same cleanup here.
- /usr/sbin/httpd -D FOREGROUND & + /usr/sbin/httpd &rstudio/c9s-python-3.11/httpd/httpd.conf (1)
30-35: Scope ExecCGI to API dirs and set DirectoryIndex.Reduce surface area and define default scripts.
-<Directory "/opt/app-root"> - AllowOverride None - Options +ExecCGI - Require all granted - AddHandler cgi-script .cgi -</Directory> +<Directory "/opt/app-root"> + AllowOverride None + Require all granted +</Directory> + +<Directory "/opt/app-root/api"> + Options +ExecCGI + AddHandler cgi-script .cgi + DirectoryIndex probe.cgi + Require all granted +</Directory> + +<Directory "/opt/app-root/api/kernels"> + Options +ExecCGI + AddHandler cgi-script .cgi + DirectoryIndex access.cgi + Require all granted +</Directory>rstudio/rhel9-python-3.11/httpd/rstudio-cgi.conf (1)
5-9: Narrow CGI handler scope (avoid SetHandler on whole location).Same hardening as c9s variant.
<LocationMatch "^/api/kernels/"> - SetHandler cgi-script Options +ExecCGI Require all granted </LocationMatch>rstudio/c9s-python-3.11/Dockerfile.cuda (1)
158-159: Remove duplicate httpd directory creation.The httpd directories are being created twice in the same RUN statement, which is redundant.
mkdir -p ${NGINX_PERL_MODULE_PATH} && \ - # Create httpd directories and set permissions - mkdir -p /var/log/httpd /var/run/httpd /etc/httpd/logs && \ chown -R 1001:0 ${NGINX_CONF_PATH} && \ chown -R 1001:0 ${NGINX_APP_ROOT}/etc && \ chown -R 1001:0 ${NGINX_CONTAINER_SCRIPTS_PATH}/nginx-start && \ chown -R 1001:0 /var/lib/nginx /var/log/nginx /run && \ - # Create httpd directories and set permissions - mkdir -p /var/log/httpd /var/run/httpd /etc/httpd/logs && \ + # Create httpd directories and set permissions + mkdir -p /var/log/httpd /var/run/httpd /etc/httpd/logs && \ chmod ug+rw ${NGINX_CONF_PATH} && \Also applies to: 163-164
rstudio/rhel9-python-3.11/Dockerfile.cuda (1)
298-299: Remove duplicate httpd directory creation.The httpd directories are being created twice in the same RUN statement, which is redundant and should be consolidated.
mkdir -p ${NGINX_PERL_MODULE_PATH} && \ - # Create httpd directories and set permissions - mkdir -p /var/log/httpd /var/run/httpd /etc/httpd/logs && \ chown -R 1001:0 ${NGINX_CONF_PATH} && \ chown -R 1001:0 ${NGINX_APP_ROOT}/etc && \ chown -R 1001:0 ${NGINX_CONTAINER_SCRIPTS_PATH}/nginx-start && \ chown -R 1001:0 /var/lib/nginx /var/log/nginx /run && \ - # Create httpd directories and set permissions - mkdir -p /var/log/httpd /var/run/httpd /etc/httpd/logs && \ + # Create httpd directories and set permissions + mkdir -p /var/log/httpd /var/run/httpd /etc/httpd/logs && \ chmod ug+rw ${NGINX_CONF_PATH} && \Also applies to: 304-305
rstudio/rhel9-python-3.11/Dockerfile.cpu (1)
168-169: Remove duplicate httpd directory creation.The httpd directories are being created twice in the same RUN statement, which is redundant.
mkdir -p ${NGINX_PERL_MODULE_PATH} && \ - # Create httpd directories and set permissions - mkdir -p /var/log/httpd /var/run/httpd /etc/httpd/logs && \ chown -R 1001:0 ${NGINX_CONF_PATH} && \ chown -R 1001:0 ${NGINX_APP_ROOT}/etc && \ chown -R 1001:0 ${NGINX_CONTAINER_SCRIPTS_PATH}/nginx-start && \ chown -R 1001:0 /var/lib/nginx /var/log/nginx /run && \ - # Create httpd directories and set permissions - mkdir -p /var/log/httpd /var/run/httpd /etc/httpd/logs && \ + # Create httpd directories and set permissions + mkdir -p /var/log/httpd /var/run/httpd /etc/httpd/logs && \ chmod ug+rw ${NGINX_CONF_PATH} && \Also applies to: 174-175
rstudio/c9s-python-3.11/Dockerfile.cpu (2)
108-109: **Fix environment variable self-reference.**Based on the static analysis error and Docker best practices, the ENV statement contains a self-reference whereAPP_ROOTis defined and immediately used within the same ENV statement. This violates Docker best practices as described by Docker documentation which states that ARG and ENV instructions should be handled carefully to avoid circular references and undefined behavior.-ENV APP_ROOT=/opt/app-root \ - NGINX_VERSION=1.24 \ +ENV APP_ROOT=/opt/app-root +ENV NGINX_VERSION=1.24 \ NGINX_SHORT_VER=124 \ NGINX_CONFIGURATION_PATH=${APP_ROOT}/etc/nginx.d \
157-158: Remove duplicate httpd directory creation.The httpd directories are being created twice in the same RUN statement, which is redundant.
mkdir -p ${NGINX_PERL_MODULE_PATH} && \ - # Create httpd directories and set permissions - mkdir -p /var/log/httpd /var/run/httpd /etc/httpd/logs && \ chown -R 1001:0 ${NGINX_CONF_PATH} && \ chown -R 1001:0 ${NGINX_APP_ROOT}/etc && \ chown -R 1001:0 ${NGINX_CONTAINER_SCRIPTS_PATH}/nginx-start && \ chown -R 1001:0 /var/lib/nginx /var/log/nginx /run && \ - # Create httpd directories and set permissions - mkdir -p /var/log/httpd /var/run/httpd /etc/httpd/logs && \ + # Create httpd directories and set permissions + mkdir -p /var/log/httpd /var/run/httpd /etc/httpd/logs && \ chmod ug+rw ${NGINX_CONF_PATH} && \Also applies to: 163-164
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
rstudio/c9s-python-3.11/Dockerfile.cpu(4 hunks)rstudio/c9s-python-3.11/Dockerfile.cuda(2 hunks)rstudio/c9s-python-3.11/httpd/httpd.conf(1 hunks)rstudio/c9s-python-3.11/httpd/rstudio-cgi.conf(1 hunks)rstudio/c9s-python-3.11/nginx/serverconf/proxy.conf.template(1 hunks)rstudio/c9s-python-3.11/nginx/serverconf/proxy.conf.template_nbprefix(1 hunks)rstudio/c9s-python-3.11/run-rstudio.sh(2 hunks)rstudio/c9s-python-3.11/supervisord/supervisord.conf(0 hunks)rstudio/rhel9-python-3.11/Dockerfile.cpu(2 hunks)rstudio/rhel9-python-3.11/Dockerfile.cuda(2 hunks)rstudio/rhel9-python-3.11/httpd/httpd.conf(1 hunks)rstudio/rhel9-python-3.11/httpd/rstudio-cgi.conf(1 hunks)rstudio/rhel9-python-3.11/nginx/serverconf/proxy.conf.template(1 hunks)rstudio/rhel9-python-3.11/nginx/serverconf/proxy.conf.template_nbprefix(1 hunks)rstudio/rhel9-python-3.11/run-rstudio.sh(2 hunks)rstudio/rhel9-python-3.11/supervisord/supervisord.conf(0 hunks)
💤 Files with no reviewable changes (2)
- rstudio/c9s-python-3.11/supervisord/supervisord.conf
- rstudio/rhel9-python-3.11/supervisord/supervisord.conf
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: jiridanek
PR: opendatahub-io/notebooks#1269
File: codeserver/ubi9-python-3.12/nginx/serverconf/proxy.conf.template:23-32
Timestamp: 2025-07-03T16:15:19.673Z
Learning: jiridanek requested GitHub issue creation for FastCGI configuration improvement in codeserver/ubi9-python-3.12/nginx/serverconf/proxy.conf.template during PR #1269 review, specifically for replacing hard-coded /opt/app-root path with $document_root variable to maintain DRY principle and improve maintainability. Issue #1311 was created with comprehensive architectural overview of FastCGI, NGINX, and supervisord integration for health checking and activity monitoring, designed for compatibility with JupyterHub idle culler and Kubeflow notebook controller culling systems.
📚 Learning: 2025-07-03T16:15:19.673Z
Learnt from: jiridanek
PR: opendatahub-io/notebooks#1269
File: codeserver/ubi9-python-3.12/nginx/serverconf/proxy.conf.template:23-32
Timestamp: 2025-07-03T16:15:19.673Z
Learning: jiridanek requested GitHub issue creation for FastCGI configuration improvement in codeserver/ubi9-python-3.12/nginx/serverconf/proxy.conf.template during PR #1269 review, specifically for replacing hard-coded /opt/app-root path with $document_root variable to maintain DRY principle and improve maintainability. Issue #1311 was created with comprehensive architectural overview of FastCGI, NGINX, and supervisord integration for health checking and activity monitoring, designed for compatibility with JupyterHub idle culler and Kubeflow notebook controller culling systems.
Applied to files:
rstudio/c9s-python-3.11/nginx/serverconf/proxy.conf.template_nbprefixrstudio/rhel9-python-3.11/nginx/serverconf/proxy.conf.template_nbprefixrstudio/c9s-python-3.11/httpd/httpd.confrstudio/c9s-python-3.11/Dockerfile.cpurstudio/c9s-python-3.11/nginx/serverconf/proxy.conf.templaterstudio/rhel9-python-3.11/nginx/serverconf/proxy.conf.template
📚 Learning: 2025-07-03T16:15:19.673Z
Learnt from: jiridanek
PR: opendatahub-io/notebooks#1269
File: codeserver/ubi9-python-3.12/nginx/serverconf/proxy.conf.template:23-32
Timestamp: 2025-07-03T16:15:19.673Z
Learning: jiridanek requested GitHub issue creation for FastCGI configuration improvement in codeserver/ubi9-python-3.12/nginx/serverconf/proxy.conf.template during PR #1269 review, specifically for replacing hard-coded /opt/app-root path with $document_root variable to maintain DRY principle and improve maintainability. The issue includes comprehensive architectural overview of FastCGI, NGINX, and supervisord integration for health checking and activity monitoring, designed for compatibility with JupyterHub idle culler and Kubeflow notebook controller culling systems.
Applied to files:
rstudio/c9s-python-3.11/nginx/serverconf/proxy.conf.template_nbprefixrstudio/rhel9-python-3.11/nginx/serverconf/proxy.conf.template_nbprefixrstudio/c9s-python-3.11/Dockerfile.cpurstudio/c9s-python-3.11/nginx/serverconf/proxy.conf.templaterstudio/rhel9-python-3.11/nginx/serverconf/proxy.conf.template
📚 Learning: 2025-07-04T10:41:13.061Z
Learnt from: grdryn
PR: opendatahub-io/notebooks#1320
File: rstudio/rhel9-python-3.11/Dockerfile.cuda:34-35
Timestamp: 2025-07-04T10:41:13.061Z
Learning: In the opendatahub-io/notebooks repository, when adapting NVIDIA CUDA Dockerfiles, the project intentionally maintains consistency with upstream NVIDIA patterns even when it might involve potential risks like empty variable expansions in package installation commands. This is considered acceptable because the containers only run on RHEL 9 with known yum/dnf behavior, and upstream consistency is prioritized over defensive coding practices.
Applied to files:
rstudio/c9s-python-3.11/Dockerfile.cudarstudio/c9s-python-3.11/Dockerfile.cpurstudio/rhel9-python-3.11/Dockerfile.cudarstudio/rhel9-python-3.11/Dockerfile.cpu
📚 Learning: 2025-09-05T12:10:50.856Z
Learnt from: jiridanek
PR: opendatahub-io/notebooks#2215
File: runtimes/datascience/ubi9-python-3.12/Dockerfile.cpu:0-0
Timestamp: 2025-09-05T12:10:50.856Z
Learning: jiridanek requested GitHub issue creation for Dockerfile environment variable refactoring during PR #2215 review. Issue #2311 was created addressing build-only variables (OPENBLAS_VERSION, ONNX_VERSION, GRPC_PYTHON_BUILD_SYSTEM_OPENSSL) being unnecessarily written to /etc/profile.d/ppc64le.sh in runtimes/datascience/ubi9-python-3.12/Dockerfile.cpu, causing variable duplication across stages, unreliable sourcing in non-login build contexts, and violation of DRY principles. The issue includes comprehensive problem description covering affected lines 30-37, detailed impact analysis of build reliability and maintenance overhead, three solution options with centralized ARG/ENV approach as recommended, clear acceptance criteria for version centralization and build-only variable cleanup, and specific implementation guidance with code examples, assigned to jiridanek, continuing the established pattern of systematic infrastructure improvements through detailed issue tracking.
Applied to files:
rstudio/c9s-python-3.11/Dockerfile.cpurstudio/rhel9-python-3.11/Dockerfile.cudarstudio/rhel9-python-3.11/Dockerfile.cpu
📚 Learning: 2025-09-05T12:10:50.856Z
Learnt from: jiridanek
PR: opendatahub-io/notebooks#2215
File: runtimes/datascience/ubi9-python-3.12/Dockerfile.cpu:0-0
Timestamp: 2025-09-05T12:10:50.856Z
Learning: jiridanek requested GitHub issue creation for Dockerfile environment variable refactoring during PR #2215 review. Issue addresses build-only variables (OPENBLAS_VERSION, ONNX_VERSION, GRPC_PYTHON_BUILD_SYSTEM_OPENSSL) being unnecessarily written to /etc/profile.d/ppc64le.sh in runtimes/datascience/ubi9-python-3.12/Dockerfile.cpu, causing variable duplication across stages, unreliable sourcing in non-login build contexts, and violation of DRY principles. The issue includes comprehensive problem description covering affected lines 30-37, detailed impact analysis of build reliability and maintenance overhead, three solution options with centralized ARG/ENV approach as recommended, clear acceptance criteria for version centralization and build-only variable cleanup, and specific implementation guidance with code examples, continuing the established pattern of systematic infrastructure improvements through detailed issue tracking.
Applied to files:
rstudio/c9s-python-3.11/Dockerfile.cpurstudio/rhel9-python-3.11/Dockerfile.cpu
📚 Learning: 2025-09-05T12:25:09.719Z
Learnt from: jiridanek
PR: opendatahub-io/notebooks#2227
File: codeserver/ubi9-python-3.12/Dockerfile.cpu:122-123
Timestamp: 2025-09-05T12:25:09.719Z
Learning: jiridanek requested GitHub issue creation for Docker multi-stage synchronization improvement in codeserver/ubi9-python-3.12/Dockerfile.cpu during PR #2227 review. The issue addresses sentinel file pattern using /tmp/control copied to /dev/null for stage coordination between rpm-base, whl-cache, and codeserver stages, proposing semantic improvements with descriptive file names, inline documentation, and elimination of /dev/null hack while maintaining multi-architecture build functionality for ppc64le support.
Applied to files:
rstudio/c9s-python-3.11/Dockerfile.cpurstudio/rhel9-python-3.11/Dockerfile.cpu
📚 Learning: 2025-08-12T08:40:55.286Z
Learnt from: jiridanek
PR: opendatahub-io/notebooks#1909
File: runtimes/pytorch+llmcompressor/ubi9-python-3.11/Dockerfile.cuda:11-15
Timestamp: 2025-08-12T08:40:55.286Z
Learning: jiridanek requested GitHub issue creation for redundant CUDA upgrade optimization during PR #1909 review. Analysis revealed all 14 CUDA Dockerfiles contain redundant `yum upgrade -y` commands in cuda-base stages that execute after base stages already performed comprehensive `dnf upgrade` via pre-upgrade blocks, causing unnecessary CI latency and build inefficiency. Issue includes complete scope analysis with specific line numbers, investigation framework requiring NVIDIA upstream documentation review, multiple solution options, comprehensive acceptance criteria covering systematic testing and performance measurement, and proper context linking to PR #1909 review comment.
Applied to files:
rstudio/rhel9-python-3.11/Dockerfile.cuda
📚 Learning: 2025-08-27T18:54:33.532Z
Learnt from: jiridanek
PR: opendatahub-io/notebooks#0
File: :0-0
Timestamp: 2025-08-27T18:54:33.532Z
Learning: jiridanek requested GitHub issue creation for comment typo fix during PR #2145 review, specifically fixing "hhttps" to "https" in rstudio/rhel9-python-3.11/Dockerfile.cuda line 139 NVIDIA CUDA Dockerfile reference. Issue #2164 was created with comprehensive problem description, clear before/after solution, and acceptance criteria, continuing the established pattern of systematic code quality improvements through detailed issue tracking for even minor documentation fixes.
Applied to files:
rstudio/rhel9-python-3.11/Dockerfile.cuda
🪛 GitHub Actions: Code static analysis
rstudio/c9s-python-3.11/Dockerfile.cpu
[error] 108-108: DL3044: Do not refer to an environment variable within the same ENV statement where it is defined.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: build (cuda-rstudio-rhel9-python-3.11, 3.11, linux/amd64, true) / build
- GitHub Check: build (rstudio-rhel9-python-3.11, 3.11, linux/amd64, true) / build
- GitHub Check: build (rstudio-c9s-python-3.11, 3.11, linux/amd64, false) / build
- GitHub Check: build (cuda-rstudio-c9s-python-3.11, 3.11, linux/amd64, false) / build
- GitHub Check: Red Hat Konflux / odh-workbench-rstudio-minimal-cuda-py311-c9s-on-pull-request
- GitHub Check: Red Hat Konflux / odh-workbench-rstudio-minimal-cpu-py311-c9s-on-pull-request
🔇 Additional comments (17)
rstudio/c9s-python-3.11/httpd/rstudio-cgi.conf (1)
12-12: ScriptAlias requires mod_alias — ensure it’s loaded.
ScriptAliaswon't work unlessalias_moduleis loaded in httpd.conf. AddLoadModule alias_module modules/mod_alias.so(see httpd.conf review).rstudio/rhel9-python-3.11/httpd/rstudio-cgi.conf (1)
12-12: ScriptAlias requires mod_alias — ensure it’s loaded.Confirm
alias_moduleis loaded in the active httpd.conf for this image.rstudio/rhel9-python-3.11/httpd/httpd.conf (1)
1-43: LGTM! Clean and secure CGI-enabled httpd configuration.The httpd configuration is well-structured with appropriate security defaults and minimal module loading. The configuration properly restricts access to the root directory and enables CGI execution only in the designated document root.
rstudio/c9s-python-3.11/Dockerfile.cuda (3)
122-122: Package set updated correctly for httpd-based CGI processing.The addition of
httpdto INSTALL_PKGS aligns with the architectural shift from supervisord/fcgiwrap to httpd-based CGI handling.
128-130: Confirmed: httpd.conf and rstudio-cgi.conf presentBoth rstudio/c9s-python-3.11/httpd/httpd.conf and rstudio/c9s-python-3.11/httpd/rstudio-cgi.conf exist — the Dockerfile COPY lines are valid.
135-138: Verified nginx config dirs and CGI present — no action neededserverconf/, httpconf/, and api/ exist; access.cgi found at rstudio/c9s-python-3.11/nginx/api/kernels/access.cgi.
rstudio/rhel9-python-3.11/Dockerfile.cuda (3)
262-262: Package set updated correctly for httpd-based CGI processing.The addition of
httpdto INSTALL_PKGS correctly supports the architectural shift from supervisord/fcgiwrap to httpd-based CGI handling.
268-270: HTTPD config files exist
Both files are present: rstudio/rhel9-python-3.11/httpd/httpd.conf and rstudio/rhel9-python-3.11/httpd/rstudio-cgi.conf
275-278: Verified nginx configuration directories and CGI script existFound rstudio/rhel9-python-3.11/nginx/{serverconf,httpconf,api} and access.cgi at rstudio/rhel9-python-3.11/nginx/api/kernels/access.cgi (also probe.cgi, httpconf/http.conf, serverconf/proxy.conf.template present).
rstudio/rhel9-python-3.11/nginx/serverconf/proxy.conf.template (2)
28-32: Excellent proxy configuration for httpd backend.The proxy configuration correctly forwards necessary headers for proper client identification and protocol handling. The
proxy_set_headerdirectives ensure that the upstream httpd service receives the original client information.
27-35: Verified — httpd listens on 8080 and startup script launches it.
httpd.conf contains "Listen 8080" (rstudio/rhel9-python-3.11/httpd/httpd.conf:3); run-rstudio.sh starts httpd (lines 7 and 9: "/usr/sbin/httpd -D FOREGROUND &").rstudio/rhel9-python-3.11/Dockerfile.cpu (3)
132-132: Package set updated correctly for httpd-based CGI processing.The addition of
httpdto INSTALL_PKGS properly supports the architectural transition from supervisord/fcgiwrap to httpd-based CGI handling.
145-148: Verified: nginx configuration directories and CGI script exist — no action required. rstudio/rhel9-python-3.11/nginx contains serverconf/, httpconf/, api/ and api/kernels/access.cgi.
138-140: HTTPD config files present — no action requiredFound rstudio/rhel9-python-3.11/httpd/httpd.conf and rstudio/rhel9-python-3.11/httpd/rstudio-cgi.conf; these source files exist for the Dockerfile COPYs.
rstudio/c9s-python-3.11/Dockerfile.cpu (3)
121-121: Package set updated correctly for httpd-based CGI processing.The addition of
httpdto INSTALL_PKGS properly supports the architectural transition from supervisord/fcgiwrap to httpd-based CGI handling.
127-129: Confirmed: httpd config files presentBoth rstudio/c9s-python-3.11/httpd/httpd.conf and rstudio/c9s-python-3.11/httpd/rstudio-cgi.conf exist and will be copied by the Dockerfile.
134-138: Verified nginx configuration directories and CGI script exist. serverconf/, httpconf/, and api/ are present; access.cgi is at rstudio/c9s-python-3.11/nginx/api/kernels/access.cgi.
jiridanek
approved these changes
Sep 23, 2025
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jiridanek The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
atheo89
force-pushed
the
RHAIENG-1071
branch
from
a425f02 to
ca7fcae
Compare
|
New changes are detected. LGTM label has been removed. |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
rstudio/rhel9-python-3.11/Dockerfile.cpu (1)
120-129: Fix DL3044: split ENV to avoid self-reference in the same instructionHadolint failure: “DL3044: Do not refer to an environment variable within the same ENV statement where it is defined.” Split APP_ROOT into its own ENV, then reference it.
Apply this diff:
-ENV APP_ROOT=/opt/app-root \ - NGINX_VERSION=1.24 \ - NGINX_SHORT_VER=124 \ - NGINX_CONFIGURATION_PATH=${APP_ROOT}/etc/nginx.d \ - NGINX_CONF_PATH=/etc/nginx/nginx.conf \ - NGINX_DEFAULT_CONF_PATH=${APP_ROOT}/etc/nginx.default.d \ - NGINX_CONTAINER_SCRIPTS_PATH=/usr/share/container-scripts/nginx \ - NGINX_APP_ROOT=${APP_ROOT} \ - NGINX_LOG_PATH=/var/log/nginx \ - NGINX_PERL_MODULE_PATH=${APP_ROOT}/etc/perl +ENV APP_ROOT=/opt/app-root +ENV NGINX_VERSION=1.24 \ + NGINX_SHORT_VER=124 \ + NGINX_CONFIGURATION_PATH=${APP_ROOT}/etc/nginx.d \ + NGINX_CONF_PATH=/etc/nginx/nginx.conf \ + NGINX_DEFAULT_CONF_PATH=${APP_ROOT}/etc/nginx.default.d \ + NGINX_CONTAINER_SCRIPTS_PATH=/usr/share/container-scripts/nginx \ + NGINX_APP_ROOT=${APP_ROOT} \ + NGINX_LOG_PATH=/var/log/nginx \ + NGINX_PERL_MODULE_PATH=${APP_ROOT}/etc/perlrstudio/c9s-python-3.11/Dockerfile.cpu (1)
108-118: Fix DL3044: split ENV to avoid self-referencing in one statement.Resolves the linter error.
-ENV APP_ROOT=/opt/app-root \ - NGINX_VERSION=1.24 \ - NGINX_SHORT_VER=124 \ - NGINX_CONFIGURATION_PATH=${APP_ROOT}/etc/nginx.d \ - NGINX_CONF_PATH=/etc/nginx/nginx.conf \ - NGINX_DEFAULT_CONF_PATH=${APP_ROOT}/etc/nginx.default.d \ - NGINX_CONTAINER_SCRIPTS_PATH=/usr/share/container-scripts/nginx \ - NGINX_APP_ROOT=${APP_ROOT} \ - NGINX_LOG_PATH=/var/log/nginx \ - NGINX_PERL_MODULE_PATH=${APP_ROOT}/etc/perl +ENV APP_ROOT=/opt/app-root +ENV NGINX_VERSION=1.24 \ + NGINX_SHORT_VER=124 \ + NGINX_CONFIGURATION_PATH=${APP_ROOT}/etc/nginx.d \ + NGINX_CONF_PATH=/etc/nginx/nginx.conf \ + NGINX_DEFAULT_CONF_PATH=${APP_ROOT}/etc/nginx.default.d \ + NGINX_CONTAINER_SCRIPTS_PATH=/usr/share/container-scripts/nginx \ + NGINX_APP_ROOT=${APP_ROOT} \ + NGINX_LOG_PATH=/var/log/nginx \ + NGINX_PERL_MODULE_PATH=${APP_ROOT}/etc/perlrstudio/rhel9-python-3.11/Dockerfile.cuda (1)
250-260: Fix DL3044: split ENV to avoid self-referencing.Matches CPU variant fix.
-ENV APP_ROOT=/opt/app-root \ - NGINX_VERSION=1.24 \ - NGINX_SHORT_VER=124 \ - NGINX_CONFIGURATION_PATH=${APP_ROOT}/etc/nginx.d \ - NGINX_CONF_PATH=/etc/nginx/nginx.conf \ - NGINX_DEFAULT_CONF_PATH=${APP_ROOT}/etc/nginx.default.d \ - NGINX_CONTAINER_SCRIPTS_PATH=/usr/share/container-scripts/nginx \ - NGINX_APP_ROOT=${APP_ROOT} \ - NGINX_LOG_PATH=/var/log/nginx \ - NGINX_PERL_MODULE_PATH=${APP_ROOT}/etc/perl +ENV APP_ROOT=/opt/app-root +ENV NGINX_VERSION=1.24 \ + NGINX_SHORT_VER=124 \ + NGINX_CONFIGURATION_PATH=${APP_ROOT}/etc/nginx.d \ + NGINX_CONF_PATH=/etc/nginx/nginx.conf \ + NGINX_DEFAULT_CONF_PATH=${APP_ROOT}/etc/nginx.default.d \ + NGINX_CONTAINER_SCRIPTS_PATH=/usr/share/container-scripts/nginx \ + NGINX_APP_ROOT=${APP_ROOT} \ + NGINX_LOG_PATH=/var/log/nginx \ + NGINX_PERL_MODULE_PATH=${APP_ROOT}/etc/perl
🧹 Nitpick comments (9)
rstudio/rhel9-python-3.11/Dockerfile.cpu (2)
169-177: Remove duplicate httpd directory creationThe httpd mkdir block appears twice; keep one to reduce layer bloat and noise.
Apply this diff:
mkdir -p ${NGINX_PERL_MODULE_PATH} && \ - # Create httpd directories and set permissions - mkdir -p /var/log/httpd /var/run/httpd /etc/httpd/logs && \ chown -R 1001:0 ${NGINX_CONF_PATH} && \
140-141: Consider leaving /etc/httpd conf files root-owned (0644) instead of 1001:0Not blocking, but reducing write permissions on config tightens posture. Current chown makes them user-writable if the process runs as UID 1001.
Potential change (optional):
- Drop
--chown=1001:0for httpd.conf and rstudio-cgi.conf and ensure mode 0644.rstudio/rhel9-python-3.11/httpd/rstudio-cgi.conf (2)
5-7: Use directory ScriptAlias + DirectoryIndex for clean mapping; also set default for “/api/”.Map “/api/kernels/” to the directory (not the file) and declare DirectoryIndex. Do the same for “/api/” so “/api/” resolves to probe.cgi.
-ScriptAlias /api/kernels/ /opt/app-root/api/kernels/access.cgi -ScriptAlias /api/probe /opt/app-root/api/probe.cgi -ScriptAlias /api /opt/app-root/api/probe.cgi +ScriptAlias /api/kernels/ /opt/app-root/api/kernels/ +ScriptAlias /api/probe /opt/app-root/api/probe.cgi +ScriptAlias /api/ /opt/app-root/api/ +ScriptAlias /api /opt/app-root/api/probe.cgi<Directory "/opt/app-root/api"> Options +ExecCGI AddHandler cgi-script .cgi + DirectoryIndex probe.cgi Require all granted </Directory>Also applies to: 17-21, 23-27
10-14: Optional: drop broad LocationMatch to reduce CGI surface.Explicit ScriptAlias + Directory blocks are sufficient and clearer.
-<LocationMatch "^/api/?"> - SetHandler cgi-script - Options +ExecCGI - Require all granted -</LocationMatch>rstudio/c9s-python-3.11/httpd/httpd.conf (1)
36-41: Limit ExecCGI/AddHandler to API dirs only.Avoid enabling CGI for the entire DocumentRoot.
<Directory "/opt/app-root"> AllowOverride None - Options +ExecCGI Require all granted - AddHandler cgi-script .cgi </Directory>rstudio/c9s-python-3.11/nginx/serverconf/proxy.conf.template (1)
11-19: Provide default for “/api” and “/api/” to hit probe CGI.Prevents 404 on bare path.
location /api/ { + # Default to probe for bare /api or /api/ + rewrite ^/api/?$ /api/probe break; proxy_pass http://localhost:8080; proxy_set_header Host $host; proxy_set_header X-Real-IP $remote_addr; proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for; proxy_set_header X-Forwarded-Proto $scheme; gzip off; access_log off; }rstudio/c9s-python-3.11/Dockerfile.cpu (1)
163-165: Remove duplicate httpd mkdir.It’s executed twice in the same RUN.
- # Create httpd directories and set permissions - mkdir -p /var/log/httpd /var/run/httpd /etc/httpd/logs && \rstudio/rhel9-python-3.11/Dockerfile.cuda (2)
300-307: Remove duplicate httpd mkdir.Same duplication as CPU Dockerfile.
- # Create httpd directories and set permissions - mkdir -p /var/log/httpd /var/run/httpd /etc/httpd/logs && \
176-177: Fix label typos: io.k9s → io.k8s.Follows Kubernetes label convention (used elsewhere in repo).
- io.k9s.display-name="RStudio Server image with python 3.11 based on Red Hat Enterprise Linux 9" \ - io.k9s.description="RStudio Server image with python 3.11 based on Red Hat Enterprise Linux 9" \ + io.k8s.display-name="RStudio Server image with python 3.11 based on Red Hat Enterprise Linux 9" \ + io.k8s.description="RStudio Server image with python 3.11 based on Red Hat Enterprise Linux 9" \
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
Makefile(2 hunks)rstudio/c9s-python-3.11/Dockerfile.cpu(4 hunks)rstudio/c9s-python-3.11/Dockerfile.cuda(3 hunks)rstudio/c9s-python-3.11/httpd/httpd.conf(1 hunks)rstudio/c9s-python-3.11/httpd/rstudio-cgi.conf(1 hunks)rstudio/c9s-python-3.11/nginx/serverconf/proxy.conf.template(2 hunks)rstudio/c9s-python-3.11/nginx/serverconf/proxy.conf.template_nbprefix(2 hunks)rstudio/c9s-python-3.11/run-rstudio.sh(2 hunks)rstudio/rhel9-python-3.11/Dockerfile.cpu(4 hunks)rstudio/rhel9-python-3.11/Dockerfile.cuda(3 hunks)rstudio/rhel9-python-3.11/httpd/httpd.conf(1 hunks)rstudio/rhel9-python-3.11/httpd/rstudio-cgi.conf(1 hunks)rstudio/rhel9-python-3.11/nginx/serverconf/proxy.conf.template(2 hunks)rstudio/rhel9-python-3.11/nginx/serverconf/proxy.conf.template_nbprefix(3 hunks)rstudio/rhel9-python-3.11/run-rstudio.sh(2 hunks)rstudio/rhel9-python-3.11/supervisord/supervisord.conf(0 hunks)
💤 Files with no reviewable changes (1)
- rstudio/rhel9-python-3.11/supervisord/supervisord.conf
🚧 Files skipped from review as they are similar to previous changes (8)
- rstudio/rhel9-python-3.11/nginx/serverconf/proxy.conf.template_nbprefix
- rstudio/c9s-python-3.11/httpd/rstudio-cgi.conf
- rstudio/c9s-python-3.11/Dockerfile.cuda
- rstudio/rhel9-python-3.11/httpd/httpd.conf
- rstudio/rhel9-python-3.11/nginx/serverconf/proxy.conf.template
- rstudio/c9s-python-3.11/nginx/serverconf/proxy.conf.template_nbprefix
- rstudio/c9s-python-3.11/run-rstudio.sh
- rstudio/rhel9-python-3.11/run-rstudio.sh
🧰 Additional context used
🧠 Learnings (16)
📓 Common learnings
Learnt from: jiridanek
PR: opendatahub-io/notebooks#1269
File: codeserver/ubi9-python-3.12/nginx/serverconf/proxy.conf.template:23-32
Timestamp: 2025-07-03T16:15:19.673Z
Learning: jiridanek requested GitHub issue creation for FastCGI configuration improvement in codeserver/ubi9-python-3.12/nginx/serverconf/proxy.conf.template during PR #1269 review, specifically for replacing hard-coded /opt/app-root path with $document_root variable to maintain DRY principle and improve maintainability. Issue #1311 was created with comprehensive architectural overview of FastCGI, NGINX, and supervisord integration for health checking and activity monitoring, designed for compatibility with JupyterHub idle culler and Kubeflow notebook controller culling systems.
Learnt from: jiridanek
PR: opendatahub-io/notebooks#1269
File: codeserver/ubi9-python-3.12/nginx/serverconf/proxy.conf.template:23-32
Timestamp: 2025-07-03T16:15:19.673Z
Learning: jiridanek requested GitHub issue creation for FastCGI configuration improvement in codeserver/ubi9-python-3.12/nginx/serverconf/proxy.conf.template during PR #1269 review, specifically for replacing hard-coded /opt/app-root path with $document_root variable to maintain DRY principle and improve maintainability. The issue includes comprehensive architectural overview of FastCGI, NGINX, and supervisord integration for health checking and activity monitoring, designed for compatibility with JupyterHub idle culler and Kubeflow notebook controller culling systems.
Learnt from: jiridanek
PR: opendatahub-io/notebooks#1269
File: codeserver/ubi9-python-3.12/nginx/serverconf/proxy.conf.template:23-32
Timestamp: 2025-07-03T16:15:19.673Z
Learning: jiridanek requested GitHub issue creation for FastCGI configuration improvement in codeserver/ubi9-python-3.12/nginx/serverconf/proxy.conf.template during PR #1269 review, specifically for replacing hard-coded /opt/app-root path with $document_root variable to maintain DRY principle and improve maintainability. The issue includes comprehensive architectural overview of FastCGI, NGINX, and supervisord integration for health checking and activity monitoring, designed for compatibility with JupyterHub idle culler and Kubeflow notebook controller culling systems.
📚 Learning: 2025-07-03T16:15:19.673Z
Learnt from: jiridanek
PR: opendatahub-io/notebooks#1269
File: codeserver/ubi9-python-3.12/nginx/serverconf/proxy.conf.template:23-32
Timestamp: 2025-07-03T16:15:19.673Z
Learning: jiridanek requested GitHub issue creation for FastCGI configuration improvement in codeserver/ubi9-python-3.12/nginx/serverconf/proxy.conf.template during PR #1269 review, specifically for replacing hard-coded /opt/app-root path with $document_root variable to maintain DRY principle and improve maintainability. Issue #1311 was created with comprehensive architectural overview of FastCGI, NGINX, and supervisord integration for health checking and activity monitoring, designed for compatibility with JupyterHub idle culler and Kubeflow notebook controller culling systems.
Applied to files:
rstudio/c9s-python-3.11/nginx/serverconf/proxy.conf.templaterstudio/rhel9-python-3.11/httpd/rstudio-cgi.confrstudio/c9s-python-3.11/Dockerfile.cpurstudio/rhel9-python-3.11/Dockerfile.cpurstudio/rhel9-python-3.11/Dockerfile.cuda
📚 Learning: 2025-07-03T16:15:19.673Z
Learnt from: jiridanek
PR: opendatahub-io/notebooks#1269
File: codeserver/ubi9-python-3.12/nginx/serverconf/proxy.conf.template:23-32
Timestamp: 2025-07-03T16:15:19.673Z
Learning: jiridanek requested GitHub issue creation for FastCGI configuration improvement in codeserver/ubi9-python-3.12/nginx/serverconf/proxy.conf.template during PR #1269 review, specifically for replacing hard-coded /opt/app-root path with $document_root variable to maintain DRY principle and improve maintainability. The issue includes comprehensive architectural overview of FastCGI, NGINX, and supervisord integration for health checking and activity monitoring, designed for compatibility with JupyterHub idle culler and Kubeflow notebook controller culling systems.
Applied to files:
rstudio/c9s-python-3.11/nginx/serverconf/proxy.conf.templaterstudio/rhel9-python-3.11/httpd/rstudio-cgi.confrstudio/c9s-python-3.11/Dockerfile.cpurstudio/rhel9-python-3.11/Dockerfile.cpu
📚 Learning: 2025-07-03T16:17:05.475Z
Learnt from: jiridanek
PR: opendatahub-io/notebooks#1269
File: codeserver/ubi9-python-3.12/nginx/api/kernels/access.cgi:6-6
Timestamp: 2025-07-03T16:17:05.475Z
Learning: jiridanek requested GitHub issue creation for CGI script health-check URL configurability and timeout improvement in codeserver/ubi9-python-3.12/nginx/api/kernels/access.cgi during PR #1269 review. The request follows the established pattern of systematic code quality improvements with comprehensive issue creation covering problem description, solution details, acceptance criteria, implementation guidance, and proper context linking.
Applied to files:
rstudio/c9s-python-3.11/nginx/serverconf/proxy.conf.templaterstudio/rhel9-python-3.11/httpd/rstudio-cgi.conf
📚 Learning: 2025-07-03T12:31:19.195Z
Learnt from: jiridanek
PR: opendatahub-io/notebooks#1269
File: codeserver/ubi9-python-3.12/nginx/api/kernels/access.cgi:6-6
Timestamp: 2025-07-03T12:31:19.195Z
Learning: jiridanek requested GitHub issue creation for CGI script error handling improvements in codeserver/ubi9-python-3.12/nginx/api/kernels/access.cgi during PR #1269 review. Issue #1286 was created with comprehensive problem description, acceptance criteria, implementation guidance, and proper context linking, continuing the established pattern of systematic code quality improvements.
Applied to files:
rstudio/c9s-python-3.11/nginx/serverconf/proxy.conf.templaterstudio/rhel9-python-3.11/httpd/rstudio-cgi.conf
📚 Learning: 2025-07-03T16:17:05.475Z
Learnt from: jiridanek
PR: opendatahub-io/notebooks#1269
File: codeserver/ubi9-python-3.12/nginx/api/kernels/access.cgi:6-6
Timestamp: 2025-07-03T16:17:05.475Z
Learning: jiridanek requested GitHub issue creation for CGI script health-check URL configurability and timeout improvement in codeserver/ubi9-python-3.12/nginx/api/kernels/access.cgi during PR #1269 review. Issue #1312 was successfully created with comprehensive problem description covering hard-coded URL limitations, timeout protection, error handling, acceptance criteria, implementation guidance with code examples, and proper context linking, continuing the established pattern of systematic code quality improvements.
Applied to files:
rstudio/c9s-python-3.11/nginx/serverconf/proxy.conf.template
📚 Learning: 2025-07-03T14:08:35.749Z
Learnt from: jiridanek
PR: opendatahub-io/notebooks#1269
File: codeserver/ubi9-python-3.12/nginx/api/kernels/access.cgi:1-5
Timestamp: 2025-07-03T14:08:35.749Z
Learning: jiridanek requested GitHub issue creation for CGI script strict mode improvements in codeserver/ubi9-python-3.12/nginx/api/kernels/access.cgi during PR #1269 review. Issue #1300 was created with comprehensive problem description covering silent failures, production risks, phased acceptance criteria for basic strict mode and enhanced error handling, implementation guidance with code examples, and proper context linking, continuing the established pattern of systematic code quality improvements.
Applied to files:
rstudio/c9s-python-3.11/nginx/serverconf/proxy.conf.template
📚 Learning: 2025-09-05T12:10:50.856Z
Learnt from: jiridanek
PR: opendatahub-io/notebooks#2215
File: runtimes/datascience/ubi9-python-3.12/Dockerfile.cpu:0-0
Timestamp: 2025-09-05T12:10:50.856Z
Learning: jiridanek requested GitHub issue creation for Dockerfile environment variable refactoring during PR #2215 review. Issue #2311 was created addressing build-only variables (OPENBLAS_VERSION, ONNX_VERSION, GRPC_PYTHON_BUILD_SYSTEM_OPENSSL) being unnecessarily written to /etc/profile.d/ppc64le.sh in runtimes/datascience/ubi9-python-3.12/Dockerfile.cpu, causing variable duplication across stages, unreliable sourcing in non-login build contexts, and violation of DRY principles. The issue includes comprehensive problem description covering affected lines 30-37, detailed impact analysis of build reliability and maintenance overhead, three solution options with centralized ARG/ENV approach as recommended, clear acceptance criteria for version centralization and build-only variable cleanup, and specific implementation guidance with code examples, assigned to jiridanek, continuing the established pattern of systematic infrastructure improvements through detailed issue tracking.
Applied to files:
rstudio/c9s-python-3.11/Dockerfile.cpurstudio/rhel9-python-3.11/Dockerfile.cpurstudio/rhel9-python-3.11/Dockerfile.cuda
📚 Learning: 2025-09-05T12:10:50.856Z
Learnt from: jiridanek
PR: opendatahub-io/notebooks#2215
File: runtimes/datascience/ubi9-python-3.12/Dockerfile.cpu:0-0
Timestamp: 2025-09-05T12:10:50.856Z
Learning: jiridanek requested GitHub issue creation for Dockerfile environment variable refactoring during PR #2215 review. Issue addresses build-only variables (OPENBLAS_VERSION, ONNX_VERSION, GRPC_PYTHON_BUILD_SYSTEM_OPENSSL) being unnecessarily written to /etc/profile.d/ppc64le.sh in runtimes/datascience/ubi9-python-3.12/Dockerfile.cpu, causing variable duplication across stages, unreliable sourcing in non-login build contexts, and violation of DRY principles. The issue includes comprehensive problem description covering affected lines 30-37, detailed impact analysis of build reliability and maintenance overhead, three solution options with centralized ARG/ENV approach as recommended, clear acceptance criteria for version centralization and build-only variable cleanup, and specific implementation guidance with code examples, continuing the established pattern of systematic infrastructure improvements through detailed issue tracking.
Applied to files:
rstudio/c9s-python-3.11/Dockerfile.cpurstudio/rhel9-python-3.11/Dockerfile.cpurstudio/rhel9-python-3.11/Dockerfile.cuda
📚 Learning: 2025-09-10T21:24:07.914Z
Learnt from: jiridanek
PR: opendatahub-io/notebooks#0
File: :0-0
Timestamp: 2025-09-10T21:24:07.914Z
Learning: jiridanek requested GitHub issue creation for Docker chown optimization in codeserver/ubi9-python-3.12/Dockerfile.cpu during PR #2356 review. Issue #2403 was created addressing performance impact of broad recursive chown on entire /opt/app-root directory (line 235), proposing three solution approaches: scoped chown targeting specific changed paths, root cause fix during file creation, and test modification for permission validation, with detailed benefits analysis covering layer size reduction and build time optimization, continuing the established pattern of systematic infrastructure improvements through detailed issue tracking.
Applied to files:
rstudio/c9s-python-3.11/Dockerfile.cpurstudio/rhel9-python-3.11/Dockerfile.cuda
📚 Learning: 2025-09-05T12:25:09.719Z
Learnt from: jiridanek
PR: opendatahub-io/notebooks#2227
File: codeserver/ubi9-python-3.12/Dockerfile.cpu:122-123
Timestamp: 2025-09-05T12:25:09.719Z
Learning: jiridanek requested GitHub issue creation for Docker multi-stage synchronization improvement in codeserver/ubi9-python-3.12/Dockerfile.cpu during PR #2227 review. The issue addresses sentinel file pattern using /tmp/control copied to /dev/null for stage coordination between rpm-base, whl-cache, and codeserver stages, proposing semantic improvements with descriptive file names, inline documentation, and elimination of /dev/null hack while maintaining multi-architecture build functionality for ppc64le support.
Applied to files:
rstudio/c9s-python-3.11/Dockerfile.cpurstudio/rhel9-python-3.11/Dockerfile.cpurstudio/rhel9-python-3.11/Dockerfile.cuda
📚 Learning: 2025-08-12T08:40:55.286Z
Learnt from: jiridanek
PR: opendatahub-io/notebooks#1909
File: runtimes/pytorch+llmcompressor/ubi9-python-3.11/Dockerfile.cuda:11-15
Timestamp: 2025-08-12T08:40:55.286Z
Learning: jiridanek requested GitHub issue creation for redundant CUDA upgrade optimization during PR #1909 review. Analysis revealed all 14 CUDA Dockerfiles contain redundant `yum upgrade -y` commands in cuda-base stages that execute after base stages already performed comprehensive `dnf upgrade` via pre-upgrade blocks, causing unnecessary CI latency and build inefficiency. Issue includes complete scope analysis with specific line numbers, investigation framework requiring NVIDIA upstream documentation review, multiple solution options, comprehensive acceptance criteria covering systematic testing and performance measurement, and proper context linking to PR #1909 review comment.
Applied to files:
rstudio/rhel9-python-3.11/Dockerfile.cuda
📚 Learning: 2025-07-04T10:41:13.061Z
Learnt from: grdryn
PR: opendatahub-io/notebooks#1320
File: rstudio/rhel9-python-3.11/Dockerfile.cuda:34-35
Timestamp: 2025-07-04T10:41:13.061Z
Learning: In the opendatahub-io/notebooks repository, when adapting NVIDIA CUDA Dockerfiles, the project intentionally maintains consistency with upstream NVIDIA patterns even when it might involve potential risks like empty variable expansions in package installation commands. This is considered acceptable because the containers only run on RHEL 9 with known yum/dnf behavior, and upstream consistency is prioritized over defensive coding practices.
Applied to files:
rstudio/rhel9-python-3.11/Dockerfile.cuda
📚 Learning: 2025-08-27T18:54:33.532Z
Learnt from: jiridanek
PR: opendatahub-io/notebooks#0
File: :0-0
Timestamp: 2025-08-27T18:54:33.532Z
Learning: jiridanek requested GitHub issue creation for comment typo fix during PR #2145 review, specifically fixing "hhttps" to "https" in rstudio/rhel9-python-3.11/Dockerfile.cuda line 139 NVIDIA CUDA Dockerfile reference. Issue #2164 was created with comprehensive problem description, clear before/after solution, and acceptance criteria, continuing the established pattern of systematic code quality improvements through detailed issue tracking for even minor documentation fixes.
Applied to files:
rstudio/rhel9-python-3.11/Dockerfile.cuda
📚 Learning: 2025-08-12T08:40:55.286Z
Learnt from: jiridanek
PR: opendatahub-io/notebooks#1909
File: runtimes/pytorch+llmcompressor/ubi9-python-3.11/Dockerfile.cuda:11-15
Timestamp: 2025-08-12T08:40:55.286Z
Learning: jiridanek requested GitHub issue creation for redundant CUDA upgrade optimization during PR #1909 review. Issue covers duplicate yum/dnf upgrade commands in cuda-base stages that execute after base stages already performed comprehensive upgrades, causing unnecessary CI latency and build inefficiency across multiple CUDA Dockerfiles. The solution requires investigating NVIDIA upstream documentation requirements before removing redundant upgrades, with systematic testing of all CUDA variants and performance measurement. Issue follows established pattern of comprehensive problem analysis, multiple solution options, detailed acceptance criteria, and proper context linking.
Applied to files:
rstudio/rhel9-python-3.11/Dockerfile.cuda
📚 Learning: 2025-08-27T18:58:27.554Z
Learnt from: jiridanek
PR: opendatahub-io/notebooks#0
File: :0-0
Timestamp: 2025-08-27T18:58:27.554Z
Learning: jiridanek requested GitHub issue creation for URL typo fix during PR #2145 review, identifying "hhttps" instead of "https" in rstudio/rhel9-python-3.11/Dockerfile.cuda line 139. Issue #2166 was created with specific line reference, before/after diff, and acceptance criteria for this minor but important documentation accuracy fix, demonstrating attention to detail in code review process.
Applied to files:
rstudio/rhel9-python-3.11/Dockerfile.cuda
🪛 GitHub Actions: Code static analysis
rstudio/c9s-python-3.11/Dockerfile.cpu
[error] 108-108: DL3044: Do not refer to an environment variable within the same ENV statement where it is defined.
rstudio/rhel9-python-3.11/Dockerfile.cpu
[error] 120-120: DL3044: Do not refer to an environment variable within the same ENV statement where it is defined.
rstudio/rhel9-python-3.11/Dockerfile.cuda
[error] 250-250: DL3044: Do not refer to an environment variable within the same ENV statement where it is defined.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: build (rstudio-rhel9-python-3.11, 3.11, linux/amd64, true) / build
- GitHub Check: build (cuda-rstudio-rhel9-python-3.11, 3.11, linux/amd64, true) / build
- GitHub Check: Red Hat Konflux / odh-workbench-rstudio-minimal-cpu-py311-c9s-on-pull-request
- GitHub Check: Red Hat Konflux / odh-workbench-rstudio-minimal-cuda-py311-c9s-on-pull-request
- GitHub Check: build (cuda-rstudio-c9s-python-3.11, 3.11, linux/amd64, false) / build
- GitHub Check: build (rstudio-c9s-python-3.11, 3.11, linux/amd64, false) / build
🔇 Additional comments (6)
rstudio/rhel9-python-3.11/Dockerfile.cpu (2)
133-134: LGTM: adding httpd alongside nginx and gating via module streamPackage set looks consistent with the migration to CGI via httpd.
139-141: Confirmed — httpd is started and nginx proxies to itrun-rstudio.sh (rstudio/.../run-rstudio.sh) launches Apache (/usr/sbin/httpd -D FOREGROUND &); httpd.conf (rstudio/.../httpd/httpd.conf) contains Listen 8080; nginx proxy templates (rstudio/.../nginx/serverconf/proxy.conf.template*) proxy /api and /api/kernels to http://localhost:8080.
rstudio/c9s-python-3.11/httpd/httpd.conf (2)
10-10: LGTM: alias_module is loaded.Enables ScriptAlias in included CGI config.
51-54: LGTM: conf.d includes are enabled.Ensures rstudio-cgi.conf is loaded.
rstudio/rhel9-python-3.11/httpd/rstudio-cgi.conf (1)
23-27: Add DirectoryIndex so “/api/kernels/” serves access.cgi by default.Without it, requests to the directory may 404 instead of executing the CGI.
Apply:
<Directory "/opt/app-root/api/kernels"> Options +ExecCGI AddHandler cgi-script .cgi + DirectoryIndex access.cgi Require all granted </Directory>rstudio/c9s-python-3.11/nginx/serverconf/proxy.conf.template (1)
26-34: Map “/api/kernels/” to the CGI entrypoint.Add rewrite so directory URL executes access.cgi.
location /api/kernels/ { + # Route directory URL to the CGI entrypoint + rewrite ^/api/kernels/?$ /api/kernels/access.cgi break; proxy_pass http://localhost:8080; proxy_set_header Host $host; proxy_set_header X-Real-IP $remote_addr; proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for; proxy_set_header X-Forwarded-Proto $scheme; gzip off; access_log off; }
atheo89
force-pushed
the
RHAIENG-1071
branch
from
ca7fcae to
664aee1
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
rstudio/c9s-python-3.11/Dockerfile.cpu (1)
108-118: Fix DL3044: don’t reference a var in the same ENV instruction.Splitting APP_ROOT resolves the pipeline error.
-ENV APP_ROOT=/opt/app-root \ - NGINX_VERSION=1.24 \ +ENV APP_ROOT=/opt/app-root +ENV NGINX_VERSION=1.24 \ NGINX_SHORT_VER=124 \ NGINX_CONFIGURATION_PATH=${APP_ROOT}/etc/nginx.d \ NGINX_CONF_PATH=/etc/nginx/nginx.conf \ NGINX_DEFAULT_CONF_PATH=${APP_ROOT}/etc/nginx.default.d \ NGINX_CONTAINER_SCRIPTS_PATH=/usr/share/container-scripts/nginx \ NGINX_APP_ROOT=${APP_ROOT} \ NGINX_LOG_PATH=/var/log/nginx \ NGINX_PERL_MODULE_PATH=${APP_ROOT}/etc/perlrstudio/rhel9-python-3.11/Dockerfile.cpu (1)
120-130: Fix DL3044: don’t reference a var in the same ENV instruction.-ENV APP_ROOT=/opt/app-root \ - NGINX_VERSION=1.24 \ +ENV APP_ROOT=/opt/app-root +ENV NGINX_VERSION=1.24 \ NGINX_SHORT_VER=124 \ NGINX_CONFIGURATION_PATH=${APP_ROOT}/etc/nginx.d \ NGINX_CONF_PATH=/etc/nginx/nginx.conf \ NGINX_DEFAULT_CONF_PATH=${APP_ROOT}/etc/nginx.default.d \ NGINX_CONTAINER_SCRIPTS_PATH=/usr/share/container-scripts/nginx \ NGINX_APP_ROOT=${APP_ROOT} \ NGINX_LOG_PATH=/var/log/nginx \ NGINX_PERL_MODULE_PATH=${APP_ROOT}/etc/perlrstudio/c9s-python-3.11/Dockerfile.cuda (1)
110-120: Fix DL3044: split ENV to avoid same‑line var reference.-ENV APP_ROOT=/opt/app-root \ - NGINX_VERSION=1.24 \ +ENV APP_ROOT=/opt/app-root +ENV NGINX_VERSION=1.24 \ NGINX_SHORT_VER=124 \ NGINX_CONFIGURATION_PATH=${APP_ROOT}/etc/nginx.d \ NGINX_CONF_PATH=/etc/nginx/nginx.conf \ NGINX_DEFAULT_CONF_PATH=${APP_ROOT}/etc/nginx.default.d \ NGINX_CONTAINER_SCRIPTS_PATH=/usr/share/container-scripts/nginx \ NGINX_APP_ROOT=${APP_ROOT} \ NGINX_LOG_PATH=/var/log/nginx \ NGINX_PERL_MODULE_PATH=${APP_ROOT}/etc/perlrstudio/rhel9-python-3.11/Dockerfile.cuda (1)
250-260: Fix DL3044: split ENV to avoid same‑line var reference.-ENV APP_ROOT=/opt/app-root \ - NGINX_VERSION=1.24 \ +ENV APP_ROOT=/opt/app-root +ENV NGINX_VERSION=1.24 \ NGINX_SHORT_VER=124 \ NGINX_CONFIGURATION_PATH=${APP_ROOT}/etc/nginx.d \ NGINX_CONF_PATH=/etc/nginx/nginx.conf \ NGINX_DEFAULT_CONF_PATH=${APP_ROOT}/etc/nginx.default.d \ NGINX_CONTAINER_SCRIPTS_PATH=/usr/share/container-scripts/nginx \ NGINX_APP_ROOT=${APP_ROOT} \ NGINX_LOG_PATH=/var/log/nginx \ NGINX_PERL_MODULE_PATH=${APP_ROOT}/etc/perl
🧹 Nitpick comments (6)
rstudio/rhel9-python-3.11/run-rstudio.sh (1)
7-13: Capture NGINX PID and add a cleanup trap for graceful shutdown.Prevents orphaned httpd/nginx on container stop; minimal change.
-# Start nginx and httpd -run-nginx.sh & +# Start nginx and httpd +run-nginx.sh & +NGINX_PID=$! @@ /usr/sbin/httpd -D FOREGROUND & HTTPD_PID=$! sleep 2 + +# Graceful shutdown +cleanup() { + kill -TERM "$HTTPD_PID" 2>/dev/null || true + kill -TERM "$NGINX_PID" 2>/dev/null || true +} +trap cleanup EXIT TERM INTrstudio/rhel9-python-3.11/nginx/serverconf/proxy.conf.template (3)
11-19: Enable HTTP/1.1 and disable buffering for streamed responses.Helps with long‑polling/SSE‑like flows.
location /api/ { proxy_pass http://localhost:8080; + proxy_http_version 1.1; + proxy_buffering off; proxy_set_header Host $host; proxy_set_header X-Real-IP $remote_addr; proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for; proxy_set_header X-Forwarded-Proto $scheme; gzip off; access_log off; }
26-34: Mirror HTTP/1.1 + no buffering for kernels endpoint.location /api/kernels/ { proxy_pass http://localhost:8080; + proxy_http_version 1.1; + proxy_buffering off; proxy_set_header Host $host; proxy_set_header X-Real-IP $remote_addr; proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for; proxy_set_header X-Forwarded-Proto $scheme; gzip off; access_log off; }
36-44: Apply same proxy options to probe.location = /api/probe { proxy_pass http://localhost:8080; + proxy_http_version 1.1; + proxy_buffering off; proxy_set_header Host $host; proxy_set_header X-Real-IP $remote_addr; proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for; proxy_set_header X-Forwarded-Proto $scheme; gzip off; access_log off; }rstudio/rhel9-python-3.11/Dockerfile.cpu (1)
46-47: Typo in labels: io.k9s. should be io.k8s.*.*Align with standard Kubernetes metadata keys.
- io.k9s.display-name="RStudio Server image with python 3.11 based on Red Hat Enterprise Linux 9" \ - io.k9s.description="RStudio Server image with python 3.11 based on Red Hat Enterprise Linux 9" \ + io.k8s.display-name="RStudio Server image with python 3.11 based on Red Hat Enterprise Linux 9" \ + io.k8s.description="RStudio Server image with python 3.11 based on Red Hat Enterprise Linux 9" \rstudio/rhel9-python-3.11/Dockerfile.cuda (1)
176-177: Typo in labels: io.k9s. should be io.k8s.*.*- io.k9s.display-name="RStudio Server image with python 3.11 based on Red Hat Enterprise Linux 9" \ - io.k9s.description="RStudio Server image with python 3.11 based on Red Hat Enterprise Linux 9" \ + io.k8s.display-name="RStudio Server image with python 3.11 based on Red Hat Enterprise Linux 9" \ + io.k8s.description="RStudio Server image with python 3.11 based on Red Hat Enterprise Linux 9" \
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
rstudio/c9s-python-3.11/Dockerfile.cpu(4 hunks)rstudio/c9s-python-3.11/Dockerfile.cuda(3 hunks)rstudio/c9s-python-3.11/httpd/httpd.conf(1 hunks)rstudio/c9s-python-3.11/httpd/rstudio-cgi.conf(1 hunks)rstudio/c9s-python-3.11/nginx/serverconf/proxy.conf.template(2 hunks)rstudio/c9s-python-3.11/nginx/serverconf/proxy.conf.template_nbprefix(2 hunks)rstudio/c9s-python-3.11/run-rstudio.sh(2 hunks)rstudio/rhel9-python-3.11/Dockerfile.cpu(4 hunks)rstudio/rhel9-python-3.11/Dockerfile.cuda(3 hunks)rstudio/rhel9-python-3.11/httpd/httpd.conf(1 hunks)rstudio/rhel9-python-3.11/httpd/rstudio-cgi.conf(1 hunks)rstudio/rhel9-python-3.11/nginx/serverconf/proxy.conf.template(2 hunks)rstudio/rhel9-python-3.11/nginx/serverconf/proxy.conf.template_nbprefix(3 hunks)rstudio/rhel9-python-3.11/run-rstudio.sh(2 hunks)rstudio/rhel9-python-3.11/supervisord/supervisord.conf(0 hunks)
💤 Files with no reviewable changes (1)
- rstudio/rhel9-python-3.11/supervisord/supervisord.conf
🚧 Files skipped from review as they are similar to previous changes (8)
- rstudio/rhel9-python-3.11/httpd/httpd.conf
- rstudio/c9s-python-3.11/httpd/httpd.conf
- rstudio/rhel9-python-3.11/nginx/serverconf/proxy.conf.template_nbprefix
- rstudio/c9s-python-3.11/nginx/serverconf/proxy.conf.template
- rstudio/c9s-python-3.11/nginx/serverconf/proxy.conf.template_nbprefix
- rstudio/c9s-python-3.11/httpd/rstudio-cgi.conf
- rstudio/rhel9-python-3.11/httpd/rstudio-cgi.conf
- rstudio/c9s-python-3.11/run-rstudio.sh
🧰 Additional context used
🧠 Learnings (13)
📓 Common learnings
Learnt from: jiridanek
PR: opendatahub-io/notebooks#1269
File: codeserver/ubi9-python-3.12/nginx/serverconf/proxy.conf.template:23-32
Timestamp: 2025-07-03T16:15:19.673Z
Learning: jiridanek requested GitHub issue creation for FastCGI configuration improvement in codeserver/ubi9-python-3.12/nginx/serverconf/proxy.conf.template during PR #1269 review, specifically for replacing hard-coded /opt/app-root path with $document_root variable to maintain DRY principle and improve maintainability. Issue #1311 was created with comprehensive architectural overview of FastCGI, NGINX, and supervisord integration for health checking and activity monitoring, designed for compatibility with JupyterHub idle culler and Kubeflow notebook controller culling systems.
Learnt from: jiridanek
PR: opendatahub-io/notebooks#1269
File: codeserver/ubi9-python-3.12/nginx/serverconf/proxy.conf.template:23-32
Timestamp: 2025-07-03T16:15:19.673Z
Learning: jiridanek requested GitHub issue creation for FastCGI configuration improvement in codeserver/ubi9-python-3.12/nginx/serverconf/proxy.conf.template during PR #1269 review, specifically for replacing hard-coded /opt/app-root path with $document_root variable to maintain DRY principle and improve maintainability. The issue includes comprehensive architectural overview of FastCGI, NGINX, and supervisord integration for health checking and activity monitoring, designed for compatibility with JupyterHub idle culler and Kubeflow notebook controller culling systems.
Learnt from: jiridanek
PR: opendatahub-io/notebooks#1269
File: codeserver/ubi9-python-3.12/nginx/serverconf/proxy.conf.template:23-32
Timestamp: 2025-07-03T16:15:19.673Z
Learning: jiridanek requested GitHub issue creation for FastCGI configuration improvement in codeserver/ubi9-python-3.12/nginx/serverconf/proxy.conf.template during PR #1269 review, specifically for replacing hard-coded /opt/app-root path with $document_root variable to maintain DRY principle and improve maintainability. The issue includes comprehensive architectural overview of FastCGI, NGINX, and supervisord integration for health checking and activity monitoring, designed for compatibility with JupyterHub idle culler and Kubeflow notebook controller culling systems.
📚 Learning: 2025-09-05T12:10:50.856Z
Learnt from: jiridanek
PR: opendatahub-io/notebooks#2215
File: runtimes/datascience/ubi9-python-3.12/Dockerfile.cpu:0-0
Timestamp: 2025-09-05T12:10:50.856Z
Learning: jiridanek requested GitHub issue creation for Dockerfile environment variable refactoring during PR #2215 review. Issue #2311 was created addressing build-only variables (OPENBLAS_VERSION, ONNX_VERSION, GRPC_PYTHON_BUILD_SYSTEM_OPENSSL) being unnecessarily written to /etc/profile.d/ppc64le.sh in runtimes/datascience/ubi9-python-3.12/Dockerfile.cpu, causing variable duplication across stages, unreliable sourcing in non-login build contexts, and violation of DRY principles. The issue includes comprehensive problem description covering affected lines 30-37, detailed impact analysis of build reliability and maintenance overhead, three solution options with centralized ARG/ENV approach as recommended, clear acceptance criteria for version centralization and build-only variable cleanup, and specific implementation guidance with code examples, assigned to jiridanek, continuing the established pattern of systematic infrastructure improvements through detailed issue tracking.
Applied to files:
rstudio/rhel9-python-3.11/Dockerfile.cpurstudio/c9s-python-3.11/Dockerfile.cudarstudio/c9s-python-3.11/Dockerfile.cpurstudio/rhel9-python-3.11/Dockerfile.cuda
📚 Learning: 2025-09-05T12:10:50.856Z
Learnt from: jiridanek
PR: opendatahub-io/notebooks#2215
File: runtimes/datascience/ubi9-python-3.12/Dockerfile.cpu:0-0
Timestamp: 2025-09-05T12:10:50.856Z
Learning: jiridanek requested GitHub issue creation for Dockerfile environment variable refactoring during PR #2215 review. Issue addresses build-only variables (OPENBLAS_VERSION, ONNX_VERSION, GRPC_PYTHON_BUILD_SYSTEM_OPENSSL) being unnecessarily written to /etc/profile.d/ppc64le.sh in runtimes/datascience/ubi9-python-3.12/Dockerfile.cpu, causing variable duplication across stages, unreliable sourcing in non-login build contexts, and violation of DRY principles. The issue includes comprehensive problem description covering affected lines 30-37, detailed impact analysis of build reliability and maintenance overhead, three solution options with centralized ARG/ENV approach as recommended, clear acceptance criteria for version centralization and build-only variable cleanup, and specific implementation guidance with code examples, continuing the established pattern of systematic infrastructure improvements through detailed issue tracking.
Applied to files:
rstudio/rhel9-python-3.11/Dockerfile.cpurstudio/c9s-python-3.11/Dockerfile.cudarstudio/c9s-python-3.11/Dockerfile.cpurstudio/rhel9-python-3.11/Dockerfile.cuda
📚 Learning: 2025-07-03T16:15:19.673Z
Learnt from: jiridanek
PR: opendatahub-io/notebooks#1269
File: codeserver/ubi9-python-3.12/nginx/serverconf/proxy.conf.template:23-32
Timestamp: 2025-07-03T16:15:19.673Z
Learning: jiridanek requested GitHub issue creation for FastCGI configuration improvement in codeserver/ubi9-python-3.12/nginx/serverconf/proxy.conf.template during PR #1269 review, specifically for replacing hard-coded /opt/app-root path with $document_root variable to maintain DRY principle and improve maintainability. Issue #1311 was created with comprehensive architectural overview of FastCGI, NGINX, and supervisord integration for health checking and activity monitoring, designed for compatibility with JupyterHub idle culler and Kubeflow notebook controller culling systems.
Applied to files:
rstudio/rhel9-python-3.11/Dockerfile.cpurstudio/c9s-python-3.11/Dockerfile.cudarstudio/c9s-python-3.11/Dockerfile.cpurstudio/rhel9-python-3.11/nginx/serverconf/proxy.conf.templaterstudio/rhel9-python-3.11/Dockerfile.cuda
📚 Learning: 2025-07-03T16:15:19.673Z
Learnt from: jiridanek
PR: opendatahub-io/notebooks#1269
File: codeserver/ubi9-python-3.12/nginx/serverconf/proxy.conf.template:23-32
Timestamp: 2025-07-03T16:15:19.673Z
Learning: jiridanek requested GitHub issue creation for FastCGI configuration improvement in codeserver/ubi9-python-3.12/nginx/serverconf/proxy.conf.template during PR #1269 review, specifically for replacing hard-coded /opt/app-root path with $document_root variable to maintain DRY principle and improve maintainability. The issue includes comprehensive architectural overview of FastCGI, NGINX, and supervisord integration for health checking and activity monitoring, designed for compatibility with JupyterHub idle culler and Kubeflow notebook controller culling systems.
Applied to files:
rstudio/rhel9-python-3.11/Dockerfile.cpurstudio/c9s-python-3.11/Dockerfile.cudarstudio/c9s-python-3.11/Dockerfile.cpurstudio/rhel9-python-3.11/nginx/serverconf/proxy.conf.template
📚 Learning: 2025-09-05T12:25:09.719Z
Learnt from: jiridanek
PR: opendatahub-io/notebooks#2227
File: codeserver/ubi9-python-3.12/Dockerfile.cpu:122-123
Timestamp: 2025-09-05T12:25:09.719Z
Learning: jiridanek requested GitHub issue creation for Docker multi-stage synchronization improvement in codeserver/ubi9-python-3.12/Dockerfile.cpu during PR #2227 review. The issue addresses sentinel file pattern using /tmp/control copied to /dev/null for stage coordination between rpm-base, whl-cache, and codeserver stages, proposing semantic improvements with descriptive file names, inline documentation, and elimination of /dev/null hack while maintaining multi-architecture build functionality for ppc64le support.
Applied to files:
rstudio/rhel9-python-3.11/Dockerfile.cpurstudio/c9s-python-3.11/Dockerfile.cpurstudio/rhel9-python-3.11/Dockerfile.cuda
📚 Learning: 2025-07-04T10:41:13.061Z
Learnt from: grdryn
PR: opendatahub-io/notebooks#1320
File: rstudio/rhel9-python-3.11/Dockerfile.cuda:34-35
Timestamp: 2025-07-04T10:41:13.061Z
Learning: In the opendatahub-io/notebooks repository, when adapting NVIDIA CUDA Dockerfiles, the project intentionally maintains consistency with upstream NVIDIA patterns even when it might involve potential risks like empty variable expansions in package installation commands. This is considered acceptable because the containers only run on RHEL 9 with known yum/dnf behavior, and upstream consistency is prioritized over defensive coding practices.
Applied to files:
rstudio/c9s-python-3.11/Dockerfile.cudarstudio/rhel9-python-3.11/Dockerfile.cuda
📚 Learning: 2025-09-10T21:24:07.914Z
Learnt from: jiridanek
PR: opendatahub-io/notebooks#0
File: :0-0
Timestamp: 2025-09-10T21:24:07.914Z
Learning: jiridanek requested GitHub issue creation for Docker chown optimization in codeserver/ubi9-python-3.12/Dockerfile.cpu during PR #2356 review. Issue #2403 was created addressing performance impact of broad recursive chown on entire /opt/app-root directory (line 235), proposing three solution approaches: scoped chown targeting specific changed paths, root cause fix during file creation, and test modification for permission validation, with detailed benefits analysis covering layer size reduction and build time optimization, continuing the established pattern of systematic infrastructure improvements through detailed issue tracking.
Applied to files:
rstudio/c9s-python-3.11/Dockerfile.cpu
📚 Learning: 2025-08-12T08:40:55.286Z
Learnt from: jiridanek
PR: opendatahub-io/notebooks#1909
File: runtimes/pytorch+llmcompressor/ubi9-python-3.11/Dockerfile.cuda:11-15
Timestamp: 2025-08-12T08:40:55.286Z
Learning: jiridanek requested GitHub issue creation for redundant CUDA upgrade optimization during PR #1909 review. Analysis revealed all 14 CUDA Dockerfiles contain redundant `yum upgrade -y` commands in cuda-base stages that execute after base stages already performed comprehensive `dnf upgrade` via pre-upgrade blocks, causing unnecessary CI latency and build inefficiency. Issue includes complete scope analysis with specific line numbers, investigation framework requiring NVIDIA upstream documentation review, multiple solution options, comprehensive acceptance criteria covering systematic testing and performance measurement, and proper context linking to PR #1909 review comment.
Applied to files:
rstudio/rhel9-python-3.11/Dockerfile.cuda
📚 Learning: 2025-08-27T18:54:33.532Z
Learnt from: jiridanek
PR: opendatahub-io/notebooks#0
File: :0-0
Timestamp: 2025-08-27T18:54:33.532Z
Learning: jiridanek requested GitHub issue creation for comment typo fix during PR #2145 review, specifically fixing "hhttps" to "https" in rstudio/rhel9-python-3.11/Dockerfile.cuda line 139 NVIDIA CUDA Dockerfile reference. Issue #2164 was created with comprehensive problem description, clear before/after solution, and acceptance criteria, continuing the established pattern of systematic code quality improvements through detailed issue tracking for even minor documentation fixes.
Applied to files:
rstudio/rhel9-python-3.11/Dockerfile.cuda
📚 Learning: 2025-08-12T08:40:55.286Z
Learnt from: jiridanek
PR: opendatahub-io/notebooks#1909
File: runtimes/pytorch+llmcompressor/ubi9-python-3.11/Dockerfile.cuda:11-15
Timestamp: 2025-08-12T08:40:55.286Z
Learning: jiridanek requested GitHub issue creation for redundant CUDA upgrade optimization during PR #1909 review. Issue covers duplicate yum/dnf upgrade commands in cuda-base stages that execute after base stages already performed comprehensive upgrades, causing unnecessary CI latency and build inefficiency across multiple CUDA Dockerfiles. The solution requires investigating NVIDIA upstream documentation requirements before removing redundant upgrades, with systematic testing of all CUDA variants and performance measurement. Issue follows established pattern of comprehensive problem analysis, multiple solution options, detailed acceptance criteria, and proper context linking.
Applied to files:
rstudio/rhel9-python-3.11/Dockerfile.cuda
📚 Learning: 2025-08-27T18:58:27.554Z
Learnt from: jiridanek
PR: opendatahub-io/notebooks#0
File: :0-0
Timestamp: 2025-08-27T18:58:27.554Z
Learning: jiridanek requested GitHub issue creation for URL typo fix during PR #2145 review, identifying "hhttps" instead of "https" in rstudio/rhel9-python-3.11/Dockerfile.cuda line 139. Issue #2166 was created with specific line reference, before/after diff, and acceptance criteria for this minor but important documentation accuracy fix, demonstrating attention to detail in code review process.
Applied to files:
rstudio/rhel9-python-3.11/Dockerfile.cuda
📚 Learning: 2025-08-27T07:37:13.713Z
Learnt from: jiridanek
PR: opendatahub-io/notebooks#2114
File: Makefile:22-24
Timestamp: 2025-08-27T07:37:13.713Z
Learning: In the opendatahub-io/notebooks repository, when transitioning from Python 3.11 to 3.12 as the default RELEASE_PYTHON_VERSION, RStudio images (both c9s and rhel9 variants) should retain Python 3.11 support, requiring the piplock-renewal.yaml workflow matrix to include both Python versions ["3.11", "3.12"].
Applied to files:
rstudio/rhel9-python-3.11/Dockerfile.cuda
🪛 GitHub Actions: Code static analysis
rstudio/c9s-python-3.11/Dockerfile.cpu
[error] 108-108: DL3044 Do not refer to an environment variable within the same ENV statement where it is defined.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: build (cuda-rstudio-rhel9-python-3.11, 3.11, linux/amd64, true) / build
- GitHub Check: build (rstudio-rhel9-python-3.11, 3.11, linux/amd64, true) / build
- GitHub Check: Red Hat Konflux / odh-workbench-rstudio-minimal-cuda-py311-c9s-on-pull-request
- GitHub Check: Red Hat Konflux / odh-workbench-rstudio-minimal-cpu-py311-c9s-on-pull-request
- GitHub Check: build (cuda-rstudio-c9s-python-3.11, 3.11, linux/amd64, false) / build
- GitHub Check: build (rstudio-c9s-python-3.11, 3.11, linux/amd64, false) / build
- GitHub Check: build (jupyter-trustyai-ubi9-python-3.12, 3.12, linux/amd64, false) / build
🔇 Additional comments (1)
rstudio/rhel9-python-3.11/run-rstudio.sh (1)
16-24: Verified: httpd listens on 8080 and will not attempt UID/GID switching when started as a non-root OpenShift UIDBoth configs contain "Listen 8080" and "User default"/"Group root" — mod_unixd only switches UID/GID when started as root, so running under an arbitrary OpenShift UID leaves the process UID unchanged.
Locations: rstudio/rhel9-python-3.11/httpd/httpd.conf:3,17-18; rstudio/c9s-python-3.11/httpd/httpd.conf:3,17-18
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
rstudio/rhel9-python-3.11/Dockerfile.cpu (1)
169-185: Prefer APP_ROOT and avoid broad chown on /run.
- Use ${APP_ROOT} instead of hard-coded /opt/app-root.
- Narrow chown/chmod to /run/nginx instead of entire /run to reduce blast radius.
Apply this diff within the RUN chain:
@@ - mkdir -p ${NGINX_PERL_MODULE_PATH} && \ + mkdir -p ${NGINX_PERL_MODULE_PATH} && \ + mkdir -p /run/nginx && \ @@ - chown -R 1001:0 /var/lib/nginx /var/log/nginx /run && \ + chown -R 1001:0 /var/lib/nginx /var/log/nginx /run/nginx && \ @@ - chmod -R ug+rwX /var/lib/nginx /var/log/nginx /run && \ + chmod -R ug+rwX /var/lib/nginx /var/log/nginx /run/nginx && \ @@ - chmod +x /opt/app-root/api/kernels/access.cgi && \ - chmod +x /opt/app-root/api/probe.cgi && \ - chown -R 1001:0 /opt/app-root/api && \ + chmod +x ${APP_ROOT}/api/kernels/access.cgi && \ + chmod +x ${APP_ROOT}/api/probe.cgi && \ + chown -R 1001:0 ${APP_ROOT}/api && \rstudio/rhel9-python-3.11/Dockerfile.cuda (1)
299-315: Use APP_ROOT and scope /run permissions.Mirror the CPU variant improvements: avoid chown/chmod of entire /run; use APP_ROOT for API paths.
@@ - mkdir -p ${NGINX_PERL_MODULE_PATH} && \ + mkdir -p ${NGINX_PERL_MODULE_PATH} && \ + mkdir -p /run/nginx && \ @@ - chown -R 1001:0 /var/lib/nginx /var/log/nginx /run && \ + chown -R 1001:0 /var/lib/nginx /var/log/nginx /run/nginx && \ @@ - chmod -R ug+rwX /var/lib/nginx /var/log/nginx /run && \ + chmod -R ug+rwX /var/lib/nginx /var/log/nginx /run/nginx && \ @@ - chmod +x /opt/app-root/api/kernels/access.cgi && \ - chmod +x /opt/app-root/api/probe.cgi && \ - chown -R 1001:0 /opt/app-root/api && \ + chmod +x ${APP_ROOT}/api/kernels/access.cgi && \ + chmod +x ${APP_ROOT}/api/probe.cgi && \ + chown -R 1001:0 ${APP_ROOT}/api && \rstudio/c9s-python-3.11/Dockerfile.cpu (1)
157-173: Scope /run permissions and use APP_ROOT for API paths.Avoid broad /run ownership; prefer ${APP_ROOT} over hard-coded paths.
@@ - mkdir -p ${NGINX_PERL_MODULE_PATH} && \ + mkdir -p ${NGINX_PERL_MODULE_PATH} && \ + mkdir -p /run/nginx && \ @@ - chown -R 1001:0 /var/lib/nginx /var/log/nginx /run && \ + chown -R 1001:0 /var/lib/nginx /var/log/nginx /run/nginx && \ @@ - chmod -R ug+rwX /var/lib/nginx /var/log/nginx /run && \ + chmod -R ug+rwX /var/lib/nginx /var/log/nginx /run/nginx && \ @@ - chmod +x /opt/app-root/api/kernels/access.cgi && \ - chmod +x /opt/app-root/api/probe.cgi && \ - chown -R 1001:0 /opt/app-root/api && \ + chmod +x ${APP_ROOT}/api/kernels/access.cgi && \ + chmod +x ${APP_ROOT}/api/probe.cgi && \ + chown -R 1001:0 ${APP_ROOT}/api && \rstudio/c9s-python-3.11/Dockerfile.cuda (1)
159-175: Scope /run permissions and use APP_ROOT.Mirror the same refinements as other variants.
@@ - mkdir -p ${NGINX_PERL_MODULE_PATH} && \ + mkdir -p ${NGINX_PERL_MODULE_PATH} && \ + mkdir -p /run/nginx && \ @@ - chown -R 1001:0 /var/lib/nginx /var/log/nginx /run && \ + chown -R 1001:0 /var/lib/nginx /var/log/nginx /run/nginx && \ @@ - chmod -R ug+rwX /var/lib/nginx /var/log/nginx /run && \ + chmod -R ug+rwX /var/lib/nginx /var/log/nginx /run/nginx && \ @@ - chmod +x /opt/app-root/api/kernels/access.cgi && \ - chmod +x /opt/app-root/api/probe.cgi && \ - chown -R 1001:0 /opt/app-root/api && \ + chmod +x ${APP_ROOT}/api/kernels/access.cgi && \ + chmod +x ${APP_ROOT}/api/probe.cgi && \ + chown -R 1001:0 ${APP_ROOT}/api && \
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
-
rstudio/c9s-python-3.11/Dockerfile.cpu(4 hunks) -
rstudio/c9s-python-3.11/Dockerfile.cuda(3 hunks) -
rstudio/rhel9-python-3.11/Dockerfile.cpu(4 hunks) -
rstudio/rhel9-python-3.11/Dockerfile.cuda(3 hunks)
🧰 Additional context used
🧠 Learnings (11)
📓 Common learnings
Learnt from: jiridanek
PR: opendatahub-io/notebooks#1269
File: codeserver/ubi9-python-3.12/nginx/serverconf/proxy.conf.template:23-32
Timestamp: 2025-07-03T16:15:19.673Z
Learning: jiridanek requested GitHub issue creation for FastCGI configuration improvement in codeserver/ubi9-python-3.12/nginx/serverconf/proxy.conf.template during PR #1269 review, specifically for replacing hard-coded /opt/app-root path with $document_root variable to maintain DRY principle and improve maintainability. Issue #1311 was created with comprehensive architectural overview of FastCGI, NGINX, and supervisord integration for health checking and activity monitoring, designed for compatibility with JupyterHub idle culler and Kubeflow notebook controller culling systems.
Learnt from: jiridanek
PR: opendatahub-io/notebooks#1269
File: codeserver/ubi9-python-3.12/nginx/serverconf/proxy.conf.template:23-32
Timestamp: 2025-07-03T16:15:19.673Z
Learning: jiridanek requested GitHub issue creation for FastCGI configuration improvement in codeserver/ubi9-python-3.12/nginx/serverconf/proxy.conf.template during PR #1269 review, specifically for replacing hard-coded /opt/app-root path with $document_root variable to maintain DRY principle and improve maintainability. The issue includes comprehensive architectural overview of FastCGI, NGINX, and supervisord integration for health checking and activity monitoring, designed for compatibility with JupyterHub idle culler and Kubeflow notebook controller culling systems.
Learnt from: jiridanek
PR: opendatahub-io/notebooks#1269
File: codeserver/ubi9-python-3.12/nginx/serverconf/proxy.conf.template:23-32
Timestamp: 2025-07-03T16:15:19.673Z
Learning: jiridanek requested GitHub issue creation for FastCGI configuration improvement in codeserver/ubi9-python-3.12/nginx/serverconf/proxy.conf.template during PR #1269 review, specifically for replacing hard-coded /opt/app-root path with $document_root variable to maintain DRY principle and improve maintainability. The issue includes comprehensive architectural overview of FastCGI, NGINX, and supervisord integration for health checking and activity monitoring, designed for compatibility with JupyterHub idle culler and Kubeflow notebook controller culling systems.
Learnt from: jiridanek
PR: opendatahub-io/notebooks#0
File: :0-0
Timestamp: 2025-08-05T17:24:08.616Z
Learning: jiridanek requested PR review for #1521 covering s390x architecture support improvements, demonstrating continued focus on systematic multi-architecture compatibility enhancements in the opendatahub-io/notebooks repository through clean implementation with centralized configuration, proper CI integration, and architecture-aware testing patterns.
📚 Learning: 2025-07-04T10:41:13.061Z
Learnt from: grdryn
PR: opendatahub-io/notebooks#1320
File: rstudio/rhel9-python-3.11/Dockerfile.cuda:34-35
Timestamp: 2025-07-04T10:41:13.061Z
Learning: In the opendatahub-io/notebooks repository, when adapting NVIDIA CUDA Dockerfiles, the project intentionally maintains consistency with upstream NVIDIA patterns even when it might involve potential risks like empty variable expansions in package installation commands. This is considered acceptable because the containers only run on RHEL 9 with known yum/dnf behavior, and upstream consistency is prioritized over defensive coding practices.
Applied to files:
rstudio/c9s-python-3.11/Dockerfile.cudarstudio/rhel9-python-3.11/Dockerfile.cuda
📚 Learning: 2025-09-05T12:10:50.856Z
Learnt from: jiridanek
PR: opendatahub-io/notebooks#2215
File: runtimes/datascience/ubi9-python-3.12/Dockerfile.cpu:0-0
Timestamp: 2025-09-05T12:10:50.856Z
Learning: jiridanek requested GitHub issue creation for Dockerfile environment variable refactoring during PR #2215 review. Issue #2311 was created addressing build-only variables (OPENBLAS_VERSION, ONNX_VERSION, GRPC_PYTHON_BUILD_SYSTEM_OPENSSL) being unnecessarily written to /etc/profile.d/ppc64le.sh in runtimes/datascience/ubi9-python-3.12/Dockerfile.cpu, causing variable duplication across stages, unreliable sourcing in non-login build contexts, and violation of DRY principles. The issue includes comprehensive problem description covering affected lines 30-37, detailed impact analysis of build reliability and maintenance overhead, three solution options with centralized ARG/ENV approach as recommended, clear acceptance criteria for version centralization and build-only variable cleanup, and specific implementation guidance with code examples, assigned to jiridanek, continuing the established pattern of systematic infrastructure improvements through detailed issue tracking.
Applied to files:
rstudio/c9s-python-3.11/Dockerfile.cudarstudio/rhel9-python-3.11/Dockerfile.cpurstudio/c9s-python-3.11/Dockerfile.cpurstudio/rhel9-python-3.11/Dockerfile.cuda
📚 Learning: 2025-09-05T12:10:50.856Z
Learnt from: jiridanek
PR: opendatahub-io/notebooks#2215
File: runtimes/datascience/ubi9-python-3.12/Dockerfile.cpu:0-0
Timestamp: 2025-09-05T12:10:50.856Z
Learning: jiridanek requested GitHub issue creation for Dockerfile environment variable refactoring during PR #2215 review. Issue addresses build-only variables (OPENBLAS_VERSION, ONNX_VERSION, GRPC_PYTHON_BUILD_SYSTEM_OPENSSL) being unnecessarily written to /etc/profile.d/ppc64le.sh in runtimes/datascience/ubi9-python-3.12/Dockerfile.cpu, causing variable duplication across stages, unreliable sourcing in non-login build contexts, and violation of DRY principles. The issue includes comprehensive problem description covering affected lines 30-37, detailed impact analysis of build reliability and maintenance overhead, three solution options with centralized ARG/ENV approach as recommended, clear acceptance criteria for version centralization and build-only variable cleanup, and specific implementation guidance with code examples, continuing the established pattern of systematic infrastructure improvements through detailed issue tracking.
Applied to files:
rstudio/c9s-python-3.11/Dockerfile.cudarstudio/rhel9-python-3.11/Dockerfile.cpurstudio/c9s-python-3.11/Dockerfile.cpurstudio/rhel9-python-3.11/Dockerfile.cuda
📚 Learning: 2025-07-03T16:15:19.673Z
Learnt from: jiridanek
PR: opendatahub-io/notebooks#1269
File: codeserver/ubi9-python-3.12/nginx/serverconf/proxy.conf.template:23-32
Timestamp: 2025-07-03T16:15:19.673Z
Learning: jiridanek requested GitHub issue creation for FastCGI configuration improvement in codeserver/ubi9-python-3.12/nginx/serverconf/proxy.conf.template during PR #1269 review, specifically for replacing hard-coded /opt/app-root path with $document_root variable to maintain DRY principle and improve maintainability. Issue #1311 was created with comprehensive architectural overview of FastCGI, NGINX, and supervisord integration for health checking and activity monitoring, designed for compatibility with JupyterHub idle culler and Kubeflow notebook controller culling systems.
Applied to files:
rstudio/c9s-python-3.11/Dockerfile.cudarstudio/rhel9-python-3.11/Dockerfile.cpurstudio/c9s-python-3.11/Dockerfile.cpurstudio/rhel9-python-3.11/Dockerfile.cuda
📚 Learning: 2025-07-03T16:15:19.673Z
Learnt from: jiridanek
PR: opendatahub-io/notebooks#1269
File: codeserver/ubi9-python-3.12/nginx/serverconf/proxy.conf.template:23-32
Timestamp: 2025-07-03T16:15:19.673Z
Learning: jiridanek requested GitHub issue creation for FastCGI configuration improvement in codeserver/ubi9-python-3.12/nginx/serverconf/proxy.conf.template during PR #1269 review, specifically for replacing hard-coded /opt/app-root path with $document_root variable to maintain DRY principle and improve maintainability. The issue includes comprehensive architectural overview of FastCGI, NGINX, and supervisord integration for health checking and activity monitoring, designed for compatibility with JupyterHub idle culler and Kubeflow notebook controller culling systems.
Applied to files:
rstudio/c9s-python-3.11/Dockerfile.cudarstudio/rhel9-python-3.11/Dockerfile.cpurstudio/c9s-python-3.11/Dockerfile.cpurstudio/rhel9-python-3.11/Dockerfile.cuda
📚 Learning: 2025-09-05T12:25:09.719Z
Learnt from: jiridanek
PR: opendatahub-io/notebooks#2227
File: codeserver/ubi9-python-3.12/Dockerfile.cpu:122-123
Timestamp: 2025-09-05T12:25:09.719Z
Learning: jiridanek requested GitHub issue creation for Docker multi-stage synchronization improvement in codeserver/ubi9-python-3.12/Dockerfile.cpu during PR #2227 review. The issue addresses sentinel file pattern using /tmp/control copied to /dev/null for stage coordination between rpm-base, whl-cache, and codeserver stages, proposing semantic improvements with descriptive file names, inline documentation, and elimination of /dev/null hack while maintaining multi-architecture build functionality for ppc64le support.
Applied to files:
rstudio/rhel9-python-3.11/Dockerfile.cpurstudio/rhel9-python-3.11/Dockerfile.cuda
📚 Learning: 2025-08-12T08:40:55.286Z
Learnt from: jiridanek
PR: opendatahub-io/notebooks#1909
File: runtimes/pytorch+llmcompressor/ubi9-python-3.11/Dockerfile.cuda:11-15
Timestamp: 2025-08-12T08:40:55.286Z
Learning: jiridanek requested GitHub issue creation for redundant CUDA upgrade optimization during PR #1909 review. Analysis revealed all 14 CUDA Dockerfiles contain redundant `yum upgrade -y` commands in cuda-base stages that execute after base stages already performed comprehensive `dnf upgrade` via pre-upgrade blocks, causing unnecessary CI latency and build inefficiency. Issue includes complete scope analysis with specific line numbers, investigation framework requiring NVIDIA upstream documentation review, multiple solution options, comprehensive acceptance criteria covering systematic testing and performance measurement, and proper context linking to PR #1909 review comment.
Applied to files:
rstudio/rhel9-python-3.11/Dockerfile.cuda
📚 Learning: 2025-08-27T18:54:33.532Z
Learnt from: jiridanek
PR: opendatahub-io/notebooks#0
File: :0-0
Timestamp: 2025-08-27T18:54:33.532Z
Learning: jiridanek requested GitHub issue creation for comment typo fix during PR #2145 review, specifically fixing "hhttps" to "https" in rstudio/rhel9-python-3.11/Dockerfile.cuda line 139 NVIDIA CUDA Dockerfile reference. Issue #2164 was created with comprehensive problem description, clear before/after solution, and acceptance criteria, continuing the established pattern of systematic code quality improvements through detailed issue tracking for even minor documentation fixes.
Applied to files:
rstudio/rhel9-python-3.11/Dockerfile.cuda
📚 Learning: 2025-08-12T08:40:55.286Z
Learnt from: jiridanek
PR: opendatahub-io/notebooks#1909
File: runtimes/pytorch+llmcompressor/ubi9-python-3.11/Dockerfile.cuda:11-15
Timestamp: 2025-08-12T08:40:55.286Z
Learning: jiridanek requested GitHub issue creation for redundant CUDA upgrade optimization during PR #1909 review. Issue covers duplicate yum/dnf upgrade commands in cuda-base stages that execute after base stages already performed comprehensive upgrades, causing unnecessary CI latency and build inefficiency across multiple CUDA Dockerfiles. The solution requires investigating NVIDIA upstream documentation requirements before removing redundant upgrades, with systematic testing of all CUDA variants and performance measurement. Issue follows established pattern of comprehensive problem analysis, multiple solution options, detailed acceptance criteria, and proper context linking.
Applied to files:
rstudio/rhel9-python-3.11/Dockerfile.cuda
📚 Learning: 2025-08-27T18:58:27.554Z
Learnt from: jiridanek
PR: opendatahub-io/notebooks#0
File: :0-0
Timestamp: 2025-08-27T18:58:27.554Z
Learning: jiridanek requested GitHub issue creation for URL typo fix during PR #2145 review, identifying "hhttps" instead of "https" in rstudio/rhel9-python-3.11/Dockerfile.cuda line 139. Issue #2166 was created with specific line reference, before/after diff, and acceptance criteria for this minor but important documentation accuracy fix, demonstrating attention to detail in code review process.
Applied to files:
rstudio/rhel9-python-3.11/Dockerfile.cuda
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: build (cuda-rstudio-rhel9-python-3.11, 3.11, linux/amd64, true) / build
- GitHub Check: build (rstudio-rhel9-python-3.11, 3.11, linux/amd64, true) / build
- GitHub Check: Red Hat Konflux / odh-workbench-rstudio-minimal-cuda-py311-c9s-on-pull-request
- GitHub Check: Red Hat Konflux / odh-workbench-rstudio-minimal-cpu-py311-c9s-on-pull-request
- GitHub Check: build (cuda-rstudio-c9s-python-3.11, 3.11, linux/amd64, false) / build
- GitHub Check: build (rstudio-c9s-python-3.11, 3.11, linux/amd64, false) / build
- GitHub Check: build (jupyter-trustyai-ubi9-python-3.12, 3.12, linux/amd64, false) / build
- GitHub Check: code-static-analysis
🔇 Additional comments (16)
rstudio/rhel9-python-3.11/Dockerfile.cpu (4)
119-121: APP_ROOT centralization looks good.Using APP_ROOT to normalize nginx paths improves maintainability across variants.
147-149: NGINX config path realignment LGTM.Copies to ${APP_ROOT}/etc/nginx.{d,default.d} match the sed include strategy.
133-137: CGI module already enabled — no action required. httpd.conf contains "LoadModule cgi_module modules/mod_cgi.so" and rstudio-cgi.conf defines ScriptAlias + AddHandler; the Dockerfile copies these into /etc/httpd (rstudio/rhel9-python-3.11/httpd/httpd.conf, rstudio/rhel9-python-3.11/httpd/rstudio-cgi.conf).
139-141: Confirmed: httpd is configured for unprivileged runtime (8080, /var/run/httpd, /var/log/httpd).Listen 8080 is set in rstudio/rhel9-python-3.11/httpd/httpd.conf; PidFile is /var/run/httpd/httpd.pid; ErrorLog is /var/log/httpd/error_log. Dockerfile (rstudio/rhel9-python-3.11/Dockerfile.cpu) creates and chowns /var/log/httpd, /var/run/httpd and /etc/httpd/logs to 1001:0 and sets ug+rwX, so logs and runtime dirs are writable by UID 1001.
rstudio/rhel9-python-3.11/Dockerfile.cuda (4)
250-251: APP_ROOT centralization looks good.Consistent with other images; keeps nginx paths DRY.
277-279: NGINX config copies LGTM.Destination matches APP_ROOT-based include pattern.
263-267: Confirmed: mod_cgi is enabled and CGI handlers configured.httpd/httpd.conf contains "LoadModule cgi_module modules/mod_cgi.so" and rstudio-cgi.conf defines ScriptAlias and AddHandler cgi-script (rstudio/rhel9-python-3.11/httpd/httpd.conf, rstudio/rhel9-python-3.11/httpd/rstudio-cgi.conf).
270-271: HTTPD rootless settings verified — OKhttpd.conf sets Listen 8080 and PidFile /var/run/httpd/httpd.pid and logs to /var/log/httpd/error_log; Dockerfile creates /var/log/httpd and /var/run/httpd and chowns/chmods them to 1001:0 (see rstudio/rhel9-python-3.11/httpd/httpd.conf lines 3,21,44 and rstudio/rhel9-python-3.11/Dockerfile.cuda lines 300,305,310).
rstudio/c9s-python-3.11/Dockerfile.cpu (4)
108-111: APP_ROOT env and path normalization LGTM.Brings consistency with rhel9 variants.
135-138: NGINX config path updates LGTM.Matches APP_ROOT include scheme.
121-126: Confirmed: mod_cgi is enabled. httpd.conf contains "LoadModule cgi_module modules/mod_cgi.so" and rstudio-cgi.conf defines ScriptAlias and AddHandler for .cgi (e.g. rstudio/c9s-python-3.11/httpd/httpd.conf and rstudio/c9s-python-3.11/httpd/rstudio-cgi.conf).
127-130: httpd configured for OpenShift: listens on 8080 and pid/log paths are writable. httpd.conf has Listen 8080, PidFile /var/run/httpd/httpd.pid and ErrorLog /var/log/httpd/error_log; the Dockerfile creates /var/run/httpd and /var/log/httpd, chowns them to 1001:0 and applies ug+rwX so they are writable for OpenShift random UID with GID=0.rstudio/c9s-python-3.11/Dockerfile.cuda (4)
110-112: APP_ROOT env adoption LGTM.Consistent pathing across images.
137-140: NGINX config path updates LGTM.Matches the include strategy used elsewhere.
130-131: Verify httpd rootless settings — Listen port and writable pid/log paths
- Check ${RSTUDIO_SOURCE_CODE}/httpd/httpd.conf and ${RSTUDIO_SOURCE_CODE}/httpd/rstudio-cgi.conf (the files copied into /etc/httpd/*) contain "Listen 8080" or another non‑privileged port.
- Ensure PidFile, ErrorLog and CustomLog point to directories/files writable by UID 1001 (or create + chown those dirs in the Dockerfile); avoid default /var/run or /var/log unless ownership is adjusted.
123-128: CGI module loading confirmedhttpd.conf contains "LoadModule cgi_module modules/mod_cgi.so" and rstudio-cgi.conf configures ScriptAlias/AddHandler; Dockerfiles copy these into /etc/httpd (e.g. rstudio/c9s-python-3.11/Dockerfile.cuda:130–131).
|
@atheo89: The following tests failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
Closing this in favor: #2541 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
RStudio: Replace supervisord and fcgi with httpd
Related to: https://issues.redhat.com/browse/RHAIENG-1071
Description
RStudio: Replace supervisord and fcgi with httpd
How Has This Been Tested?
GHAs:
c9s builds: https://github.com/opendatahub-io/notebooks/actions/runs/17945923496/job/51032235225?pr=2530
rhel builds: https://github.com/opendatahub-io/notebooks/actions/runs/17945923399/job/51032277263?pr=2530
image builds (these images are going to expire in 5days):
rstudio-cuda:
quay.io/opendatahub/odh-workbench-rstudio-minimal-cuda-py311-c9s:on-pr-ca7fcae92e7e546f50b5857dea969a91fea8675drstudio-cpu:
quay.io/opendatahub/odh-workbench-rstudio-minimal-cpu-py311-c9s:on-pr-ca7fcae92e7e546f50b5857dea969a91fea8675drstudio logs from ocp cluster:
konflux-cpu-rstudio-0-konflux-cpu-rstudio.log
Self checklist (all need to be checked):
make test(gmakeon macOS) before asking for reviewDockerfile.konfluxfiles should be done inodh/notebooksand automatically synced torhds/notebooks. For Konflux-specific changes, modifyDockerfile.konfluxfiles directly inrhds/notebooksas these require special attention in the downstream repository and flow to the upcoming RHOAI release.Merge criteria:
Summary by CodeRabbit
Refactor
Chores