Add proxy support to GPU validation role#25
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds configurable HTTP, HTTPS, and no-proxy defaults, wires proxy setup into GPU validation, injects proxy variables into the vLLM service and Podman, and disables proxy use and TLS certificate validation for local health and metrics checks. ChangesProxy Configuration Support
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Setup as setup.yaml
participant ProxyTask as proxy.yaml
participant System as System files and DNF
participant Podman as Podman service
participant Vllm as vLLM service
participant Checks as health and metrics requests
Setup->>ProxyTask: include when HTTP proxy is set
ProxyTask->>System: write proxy variables and DNF proxy
ProxyTask->>Podman: install proxy drop-in and reload systemd
Vllm->>Podman: pass proxy environment variables
Checks->>Checks: poll endpoints without proxy and certificate validation
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
gpu-validation/templates/vllm-serve.service.j2 (1)
24-26: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueInconsistent quoting vs. HF token env var.
Line 21 quotes the HF token value with
| quote, but the proxy env values here are not quoted. Minor inconsistency; low risk since URLs rarely contain shell-significant characters, but worth aligning for consistency/defensiveness.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@gpu-validation/templates/vllm-serve.service.j2` around lines 24 - 26, The proxy environment variables in the vllm-serve.service.j2 template are inconsistently rendered compared with the HF token value. Update the HTTP_PROXY, HTTPS_PROXY, and NO_PROXY env entries in the service template to use the same quoting approach as the HF token env var for consistency and safer shell rendering, keeping the existing variable names and defaults intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@gpu-validation/tasks/proxy.yaml`:
- Around line 8-13: The proxy environment rendering is using
gpu_validation_https_proxy directly, which can produce the literal string "None"
when that value is unset. Update the proxy template so the HTTPS variables in
the proxy block (including the matching lower-case entries) fall back to
gpu_validation_http_proxy the same way vllm-serve.service.j2 does, and apply the
same fix wherever this proxy block is duplicated.
- Around line 21-35: After the drop-in is created in the proxy task, add an
explicit systemd daemon reload so podman.service picks up the new environment
immediately. Update the tasks in proxy.yaml around the Configure container
engine proxy and Set container engine proxy environment steps to trigger a
reload using the appropriate systemd/ansible action, and keep the change
localized to the existing podman.service.d and http-proxy.conf setup.
In `@gpu-validation/templates/vllm-serve.service.j2`:
- Around line 8-12: The vllm-serve systemd unit template currently writes proxy
environment values into a world-readable file, which can expose embedded
credentials. Update the rendering in
gpu_validation/tasks/model_download_and_serve.yaml and the vllm-serve.service.j2
template to use tighter permissions and ownership (for example, a
non-world-readable mode owned by cloud-user), or otherwise ensure
HTTP_PROXY/HTTPS_PROXY values cannot contain credentials; keep the existing
Environment entries but make the generated unit file private enough to protect
them.
- Line 10: The vllm-serve.service.j2 template is using the wrong fallback
behavior for HTTPS_PROXY, so when gpu_validation_https_proxy is empty it can
still resolve to None instead of reusing the HTTP proxy. Update both HTTPS proxy
environment entries in gpu_validation/templates/vllm-serve.service.j2 to use
default(gpu_validation_http_proxy, true) so gpu_validation_http_proxy is used
whenever gpu_validation_https_proxy is unset or empty.
---
Nitpick comments:
In `@gpu-validation/templates/vllm-serve.service.j2`:
- Around line 24-26: The proxy environment variables in the
vllm-serve.service.j2 template are inconsistently rendered compared with the HF
token value. Update the HTTP_PROXY, HTTPS_PROXY, and NO_PROXY env entries in the
service template to use the same quoting approach as the HF token env var for
consistency and safer shell rendering, keeping the existing variable names and
defaults intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 9e72d700-90dc-450c-8069-dd90ec059592
📒 Files selected for processing (5)
gpu-validation/defaults/main.yamlgpu-validation/tasks/model_download_and_serve.yamlgpu-validation/tasks/proxy.yamlgpu-validation/tasks/setup.yamlgpu-validation/templates/vllm-serve.service.j2
bogdando
reviewed
Jul 10, 2026
Contributor
|
it failed when the proxy var unset: |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@gpu-validation/defaults/main.yaml`:
- Around line 75-79: Set both gpu_validation_http_proxy and
gpu_validation_https_proxy to explicit empty-string defaults instead of null in
the defaults configuration. Also update the consuming template’s default filters
to use the boolean true argument, ensuring explicitly provided null values are
converted to empty strings before length or truthiness checks.
- Around line 33-42: Resolve the conflict in the defaults file by removing all
Git conflict markers, preserving the gpu_validation_mem_page_size setting and
its comments, and deleting the duplicate gpu_validation_libnvidia_ml_version
definition from this block while retaining its later definition.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: c16cfc44-36de-41d1-906c-4c4443577bf5
📒 Files selected for processing (5)
gpu-validation/defaults/main.yamlgpu-validation/tasks/model_download_and_serve.yamlgpu-validation/tasks/proxy.yamlgpu-validation/tasks/setup.yamlgpu-validation/templates/vllm-serve.service.j2
🚧 Files skipped from review as they are similar to previous changes (4)
- gpu-validation/templates/vllm-serve.service.j2
- gpu-validation/tasks/model_download_and_serve.yaml
- gpu-validation/tasks/setup.yaml
- gpu-validation/tasks/proxy.yaml
MiguelCarpio
force-pushed
the
http_proxy
branch
2 times, most recently
from
July 14, 2026 10:57
0910622 to
e6525d3
Compare
- Add gpu_validation_http_proxy, gpu_validation_https_proxy, and gpu_validation_no_proxy variables for outbound connectivity - Add proxy.yaml task to configure /etc/environment, dnf, and podman - Add proxy env vars to vllm-serve.service.j2 (service and container) - Add use_proxy: false and validate_certs: false to vLLM health checks Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
bogdando
approved these changes
Jul 14, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
What does this PR do?
gpu_validation_http_proxy,gpu_validation_https_proxy, andgpu_validation_no_proxyvariables for outbound connectivityuse_proxy: falseandvalidate_certs: falseto vLLM health checksWhy do we need this PR?
When VMs cannot reach the internet directly, a Proxy VM serves as the outbound gateway. The Proxy VM must have internet access. To route traffic through it, the proxy must be configured at every layer inside the VM: /etc/environment, dnf.conf, podman systemd, and the vLLM container.
If the Proxy VM also lacks internet access but a workstation can reach it via SSH, the workstation can provide connectivity through an SSH reverse tunnel — forwarding its local Squid proxy to the Proxy VM. The Proxy VM then chains requests through the tunnel to the workstation's proxy, giving the entire environment (Proxy VM and all GPU VMs behind it) outbound internet access without any direct connectivity.
Related: RHOSSTRAT-1212