CONSOLE-5293: Use console-downloads image instead of cli-artifacts#1158
CONSOLE-5293: Use console-downloads image instead of cli-artifacts#1158jhadvig wants to merge 1 commit into
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@jhadvig: This pull request references CONSOLE-5293 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📜 Recent review details🧰 Additional context used🔀 Multi-repo context openshift/consoleFindings for openshift/console:
Implication: The operator PR changes manifests to use an image tag "console-downloads", but this repository currently builds/labels an image based on origin-cli-artifacts/cli-artifacts (Dockerfile.downloads). CI (openshift/release) must build and publish a console-downloads image (likely from this Dockerfile or a modified build) so the operator's new DOWNLOADS_IMAGE value points to an existing image. README and image labels in this repo may need updating to reflect the new tag. WalkthroughThe pull request migrates the OpenShift console downloads service from a Python-based HTTP server (cli-artifacts image) to a compiled ChangesDownloads Service Migration
🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 Pre-merge checks | ✅ 12✅ Passed checks (12 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
the
approved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@bindata/assets/deployments/Untitled`:
- Line 1: Delete the stray asset file named "Untitled" that contains only the
token "readOnlyRootFilesystem" from the assets/deployments bundle; remove it
from the repository and any asset-generation lists so it won't be packaged into
bindata, and if the readOnlyRootFilesystem setting was intended for a real
deployment manifest, move that key into the proper deployment YAML (not the
"Untitled" stub).
🪄 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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 07e1b599-077b-438b-adb5-324ed8e45c96
📒 Files selected for processing (4)
bindata/assets/deployments/Untitledbindata/assets/deployments/downloads-deployment.yamlmanifests/07-operator-ibm-cloud-managed.yamlmanifests/07-operator.yaml
📜 Review details
🧰 Additional context used
📓 Path-based instructions (2)
manifests/**/*.yaml
⚙️ CodeRabbit configuration file
manifests/**/*.yaml: Review CVO manifests carefully.
These are deployed to clusters and changes have wide impact.
Files:
manifests/07-operator-ibm-cloud-managed.yamlmanifests/07-operator.yaml
bindata/assets/**/*.yaml
⚙️ CodeRabbit configuration file
bindata/assets/**/*.yaml: Review YAML assets for Kubernetes resource correctness.
Ensure proper annotations for cluster profiles.
Files:
bindata/assets/deployments/downloads-deployment.yaml
🔀 Multi-repo context openshift/console
[::openshift/console::] README.md — references the origin-cli-artifacts image and oc binaries from quay.io/repository/openshift/origin-cli-artifacts (match at README.md:443 and README.md:463).
[::openshift/console::] Dockerfile.downloads — builds FROM quay.io/openshift/origin-cli-artifacts:4.18 and labels image as "openshift,cli-artifacts" (matches at Dockerfile.downloads:1 and Dockerfile.downloads:36).
Note: manifests/ and bindata/ were not present in this checkout (shell reported "No such file or directory"), so I could not locate the exact deployment YAML files changed in the PR within this repository.
🔇 Additional comments (1)
bindata/assets/deployments/downloads-deployment.yaml (1)
51-54: LGTM!
| @@ -0,0 +1 @@ | |||
| readOnlyRootFilesystem No newline at end of file | |||
There was a problem hiding this comment.
Remove this stray file.
bindata/assets/deployments/Untitled is not valid deployment YAML and looks like an accidental editor artifact. Leaving it under the assets tree risks confusing future maintainers or being swept into asset generation by mistake.
🤖 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 `@bindata/assets/deployments/Untitled` at line 1, Delete the stray asset file
named "Untitled" that contains only the token "readOnlyRootFilesystem" from the
assets/deployments bundle; remove it from the repository and any
asset-generation lists so it won't be packaged into bindata, and if the
readOnlyRootFilesystem setting was intended for a real deployment manifest, move
that key into the proper deployment YAML (not the "Untitled" stub).
There was a problem hiding this comment.
@TheRealJon, glad we're on the same page! Once the bindata/assets/deployments/Untitled file is removed from the PR, feel free to let me know and I'll mark this resolved.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@bindata/assets/deployments/downloads-deployment.yaml`:
- Around line 51-54: The deployment currently runs the downloads binary
(command: /opt/downloads/downloads) with a readOnlyRootFilesystem but lacks a
writable /tmp, causing the Go server's os.MkdirTemp("", "artifacts") to fail;
update the Downloads Deployment to add a volumeMount for /tmp (mountPath: /tmp)
in the container spec and add a corresponding emptyDir volume (name like tmp or
downloads-tmp) in the pod spec so the container has a writable /tmp when
readOnlyRootFilesystem is true.
🪄 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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 023cf83e-cb7f-47c2-b8e2-1271d4209157
⛔ Files ignored due to path filters (1)
pkg/console/subresource/deployment/deployment_test.gois excluded by!**/*_test.go
📒 Files selected for processing (4)
bindata/assets/deployments/Untitledbindata/assets/deployments/downloads-deployment.yamlmanifests/07-operator-ibm-cloud-managed.yamlmanifests/07-operator.yaml
✅ Files skipped from review due to trivial changes (1)
- manifests/07-operator-ibm-cloud-managed.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- bindata/assets/deployments/Untitled
📜 Review details
🧰 Additional context used
📓 Path-based instructions (2)
manifests/**/*.yaml
⚙️ CodeRabbit configuration file
manifests/**/*.yaml: Review CVO manifests carefully.
These are deployed to clusters and changes have wide impact.
Files:
manifests/07-operator.yaml
bindata/assets/**/*.yaml
⚙️ CodeRabbit configuration file
bindata/assets/**/*.yaml: Review YAML assets for Kubernetes resource correctness.
Ensure proper annotations for cluster profiles.
Files:
bindata/assets/deployments/downloads-deployment.yaml
🔀 Multi-repo context openshift/console
[::openshift/console::] Dockerfile.downloads:1 — FROM quay.io/openshift/origin-cli-artifacts:4.18 (file shows image built-from still references origin-cli-artifacts).
[::openshift/console::] Dockerfile.downloads:36 — image label io.openshift.tags="openshift,cli-artifacts".
[::openshift/console::] README.md:443,463 — README documents that the server provides oc binaries from quay.io/repository/openshift/origin-cli-artifacts and notes origin-cli-artifacts usage for local builds.
[::openshift/console::] Dockerfile.downloads:20-21,28 — the repo already builds and bundles the Go downloads server binary (/opt/downloads/downloads) and defaultArtifactsConfig.yaml; Dockerfile.downloads CMD is "/opt/downloads/downloads --config-path=/opt/downloads/defaultArtifactsConfig.yaml".
[::openshift/console::] cmd/downloads/main.go & cmd/downloads/config/* — implementation of the Go downloads server and config generation present in-tree (server listens on configured port and serves the generated TempDir).
Summary: repository contains the Go downloads server and Dockerfile that builds an image from origin-cli-artifacts base; README also references origin-cli-artifacts. The PR switching operator manifests to use registry.svc.ci.openshift.org/openshift:console-downloads will require the console-downloads image to be produced/pushed (openshift/release) and reconciled with the existing Dockerfile.downloads / README references that still mention origin-cli-artifacts.
🔇 Additional comments (1)
manifests/07-operator.yaml (1)
72-73: LGTM!
| command: | ||
| - /opt/downloads/downloads | ||
| args: | ||
| - --config-path=/opt/downloads/defaultArtifactsConfig.yaml |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Local deployment YAML checks =="
rg -n 'readOnlyRootFilesystem|volumeMounts|emptyDir|/tmp' bindata/assets/deployments/downloads-deployment.yaml
echo
echo "== Linked openshift/console downloads server temp-dir checks =="
BASE="https://raw.githubusercontent.com/openshift/console/main"
for p in \
cmd/downloads/main.go \
cmd/downloads/config/config.go \
cmd/downloads/config/artifacts.go
do
echo "--- ${p} ---"
curl -fsSL "${BASE}/${p}" 2>/dev/null | rg -n 'TempDir|MkdirTemp|CreateTemp|os\.TempDir|ioutil\.TempDir' || true
doneRepository: openshift/console-operator
Length of output: 451
🏁 Script executed:
#!/bin/bash
# Get full deployment manifest to see all volumes and volumeMounts
cat -n bindata/assets/deployments/downloads-deployment.yamlRepository: openshift/console-operator
Length of output: 2991
🏁 Script executed:
#!/bin/bash
# Search for any tmp or temp references in the deployment
rg -n 'tmp|temp|TempDir|volumeMounts|volumes' bindata/assets/deployments/downloads-deployment.yaml -iRepository: openshift/console-operator
Length of output: 85
🏁 Script executed (no clone):
#!/bin/bash
# Check where TempDir is configured in openshift/console
BASE="https://raw.githubusercontent.com/openshift/console/main"
echo "=== cmd/downloads/config/config.go ==="
curl -fsSL "${BASE}/cmd/downloads/config/config.go" 2>/dev/null | head -100
echo
echo "=== Searching for TempDir assignment in main.go ==="
curl -fsSL "${BASE}/cmd/downloads/main.go" 2>/dev/null | head -80
Length of output: 1373
🏁 Script executed (no clone):
#!/bin/bash
# Get the full config.go to see how TempDir is initialized
BASE="https://raw.githubusercontent.com/openshift/console/main"
curl -fsSL "${BASE}/cmd/downloads/config/config.go" 2>/dev/null
Length of output: 14
🏁 Script executed:
#!/bin/bash
# Find config files related to downloads
find . -type f -name "*config*" | grep -i downloadRepository: openshift/console
Length of output: 104
🏁 Script executed:
#!/bin/bash
# Search for TempDir initialization pattern
rg -n 'TempDir\s*[:=]|ioutil\.TempDir|os\.MkdirTemp' --type goRepository: openshift/console
Length of output: 2443
🏁 Script executed (no clone):
#!/bin/bash
# Alternative: check if config exists at a different path or structure
curl -fsSL "https://api.github.com/repos/openshift/console/contents/cmd/downloads/config" 2>/dev/null | jq -r '.[] | "\(.name)"' || echo "API check failed"
Length of output: 79
🏁 Script executed:
#!/bin/bash
# Get the full downloads_config.go to see the TempDir initialization
cat -n cmd/downloads/config/downloads_config.go | sed -n '100,140p'Repository: openshift/console
Length of output: 1431
🏁 Script executed:
#!/bin/bash
# Check git history to see if /tmp volumes existed before
git log --oneline --all -- bindata/assets/deployments/downloads-deployment.yaml | head -5Repository: openshift/console-operator
Length of output: 460
🏁 Script executed:
#!/bin/bash
# Check if any recent commits removed volume mounts
git log -p -1 -- bindata/assets/deployments/downloads-deployment.yaml 2>/dev/null | head -100Repository: openshift/console-operator
Length of output: 3133
Add /tmp volumeMount and emptyDir volume — required for runtime temp directory creation.
The new Go downloads server calls os.MkdirTemp("", "artifacts") at startup (verified in openshift/console cmd/downloads/config/downloads_config.go:112), which creates a temporary directory in the system temp location (typically /tmp). With readOnlyRootFilesystem: true and no writable /tmp volume, pod startup will fail.
Previous commits (ea4062a, 29c40d9) added /tmp volumes when enabling read-only root filesystem. This change removed those volumes when switching to the Go server but did not account for the server's temp directory requirement.
Required patch
containers:
- resources:
requests:
cpu: 10m
memory: 50Mi
+ volumeMounts:
+ - name: tmp
+ mountPath: /tmp
readinessProbe: priorityClassName: system-cluster-critical
+ volumes:
+ - name: tmp
+ emptyDir: {}🤖 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 `@bindata/assets/deployments/downloads-deployment.yaml` around lines 51 - 54,
The deployment currently runs the downloads binary (command:
/opt/downloads/downloads) with a readOnlyRootFilesystem but lacks a writable
/tmp, causing the Go server's os.MkdirTemp("", "artifacts") to fail; update the
Downloads Deployment to add a volumeMount for /tmp (mountPath: /tmp) in the
container spec and add a corresponding emptyDir volume (name like tmp or
downloads-tmp) in the pod spec so the container has a writable /tmp when
readOnlyRootFilesystem is true.
|
/retest |
TheRealJon
left a comment
TheRealJon
left a comment
There was a problem hiding this comment.
Just the stray file to remove, other than that it looks good!
| @@ -0,0 +1 @@ | |||
| readOnlyRootFilesystem No newline at end of file | |||
Replace the cli-artifacts image with the new console-downloads image for the downloads deployment. The new image contains a Golang HTTP server with its own entrypoint, so the inline Python server script and associated command/args/volumes are removed from the deployment template.
|
@jhadvig: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jhadvig, TheRealJon The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
3 participants
Analysis / Root cause:
The downloads deployment currently uses the
cli-artifactsimage and runs an inline Python HTTP server script to serve CLI binaries. This approach is being replaced by a dedicatedconsole-downloadsimage(openshift/console Dockerfile.downloads) that contains a purpose-built Golang HTTP server with its own entrypoint, making the inline Python
script unnecessary.
Jira: https://redhat.atlassian.net/browse/CONSOLE-5293
Solution description:
manifests/07-operator.yamlandmanifests/07-operator-ibm-cloud-managed.yaml— Updated theDOWNLOADS_IMAGEenv var fromregistry.svc.ci.openshift.org/openshift:cli-artifactstoregistry.svc.ci.openshift.org/openshift:console-downloads.bindata/assets/deployments/downloads-deployment.yaml— Removed the inline Python HTTP server script (command,args), the/tmpvolumeMount, and thetmpemptyDirvolume. The newconsole-downloadsimage has its own entrypoint (/opt/downloads/downloads --config-path=/opt/downloads/defaultArtifactsConfig.yaml), so no custom command/args are needed.No Go code changes were required — the operator reads the deployment YAML from bindata and sets the image from the
DOWNLOADS_IMAGEenv var, which continues to work as before.Test setup:
The
console-downloadsimage must be present in the release payload. Ensure the image is built and available via the CI config in openshift/release.Test cases:
console-downloadsimagehttps://downloads-openshift-console.apps.<cluster>/serves CLI binaries correctlyocdownload links for all architectures (amd64, arm64, ppc64le, s390x) and platforms (linux, mac, windows) are functionalcli-artifactsNeeds to be tested with openshift/console#16455
Depends on: openshift/release#79355
Browser conformance:
Additional info:
This is a net deletion of ~200 lines of inline Python. The new Golang-based downloads server in the
console-downloadsimage provides the same functionality with better performance and maintainability.Summary by CodeRabbit