Add ODF custom catalog source support for dev/pre-release versions - #1262
Add ODF custom catalog source support for dev/pre-release versions#1262ebattat wants to merge 2 commits into
Conversation
📝 WalkthroughWalkthroughThe pipeline supplies an optional ODF catalog image. Templates create and select an ODF ChangesODF catalog configuration
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ebattat 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 |
![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
🧹 Nitpick comments (2)
jenkins/PerfCI/02_PerfCI_Operators_Deployment/Jenkinsfile (1)
12-12: 🩺 Stability & Availability | 🔵 TrivialVerify optional credential provisioning.
Line 12 makes the credential binding mandatory at pipeline startup. If
perfci_odf_catalog_imageis missing, Jenkins fails before the container starts. Theredhat-operatorsfallback is then unreachable. Confirm that every target Jenkins instance has this credential and supports an empty value.🤖 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 `@jenkins/PerfCI/02_PerfCI_Operators_Deployment/Jenkinsfile` at line 12, Update the ODF_CATALOG_IMAGE credential binding in the pipeline environment so missing perfci_odf_catalog_image does not fail Jenkins before the container starts, while preserving the redhat-operators fallback. Verify the credential configuration and supported empty-value behavior across all target Jenkins instances.benchmark_runner/common/ocp_resources/create_odf.py (1)
52-56: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winClose the rendered YAML file explicitly.
open(yaml_path).read()does not make file cleanup explicit. Use a context manager before checking the rendered content.As per path instructions,
benchmark_runner/**requires focus on Python best practices, error handling, and resource cleanup.Proposed fix
- if not open(yaml_path).read().strip(): + with open(yaml_path, encoding='utf-8') as yaml_file: + is_empty = not yaml_file.read().strip() + if is_empty: logger.info(f'Skipping empty template: {resource}') continue🤖 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 `@benchmark_runner/common/ocp_resources/create_odf.py` around lines 52 - 56, Update the rendered-template check in the resource generation method containing yaml_path to open the YAML file with a context manager, read its contents within that scope, and then preserve the existing empty-template skip and logging behavior.Source: Path instructions
🤖 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 `@benchmark_runner/common/ocp_resources/create_odf.py`:
- Around line 60-62: Update the wait_for_ocp_resource_create call in the ODF
creation flow to return the raw CatalogSource connection state instead of piping
it through grep -c READY. Since count_openshift_storage=True applies an
unrelated numeric comparison, remove or disable that option for this readiness
check and pass status='READY' so the wait validates the CatalogSource state
directly.
---
Nitpick comments:
In `@benchmark_runner/common/ocp_resources/create_odf.py`:
- Around line 52-56: Update the rendered-template check in the resource
generation method containing yaml_path to open the YAML file with a context
manager, read its contents within that scope, and then preserve the existing
empty-template skip and logging behavior.
In `@jenkins/PerfCI/02_PerfCI_Operators_Deployment/Jenkinsfile`:
- Line 12: Update the ODF_CATALOG_IMAGE credential binding in the pipeline
environment so missing perfci_odf_catalog_image does not fail Jenkins before the
container starts, while preserving the redhat-operators fallback. Verify the
credential configuration and supported empty-value behavior across all target
Jenkins instances.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 407f0021-c35c-4949-9b6a-b32a7e1e2c0a
📒 Files selected for processing (5)
benchmark_runner/common/ocp_resources/create_odf.pybenchmark_runner/common/ocp_resources/odf/template/00_catalog_source_template.yamlbenchmark_runner/common/ocp_resources/odf/template/07_subscription_template.yamlbenchmark_runner/main/environment_variables.pyjenkins/PerfCI/02_PerfCI_Operators_Deployment/Jenkinsfile
| self.wait_for_ocp_resource_create(operator='odf', | ||
| verify_cmd="oc get catalogsource odf-catalog-source -n openshift-marketplace -o jsonpath='{.status.connectionState.lastObservedState}' | grep -c READY || true", | ||
| count_openshift_storage=True) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Fix the CatalogSource readiness predicate.
grep -c READY returns 0 or 1. With count_openshift_storage=True, wait_for_ocp_resource_create compares that value with active_nodes * num_odf_disk. It does not check the CatalogSource connection state. Custom catalog deployments will normally time out before the Subscription is created.
Pass the raw connection state and use status='READY'.
Proposed fix
- verify_cmd="oc get catalogsource odf-catalog-source -n openshift-marketplace -o jsonpath='{.status.connectionState.lastObservedState}' | grep -c READY || true",
- count_openshift_storage=True)
+ verify_cmd="oc get catalogsource odf-catalog-source -n openshift-marketplace -o jsonpath='{.status.connectionState.lastObservedState}' || true",
+ status='READY')📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| self.wait_for_ocp_resource_create(operator='odf', | |
| verify_cmd="oc get catalogsource odf-catalog-source -n openshift-marketplace -o jsonpath='{.status.connectionState.lastObservedState}' | grep -c READY || true", | |
| count_openshift_storage=True) | |
| self.wait_for_ocp_resource_create(operator='odf', | |
| verify_cmd="oc get catalogsource odf-catalog-source -n openshift-marketplace -o jsonpath='{.status.connectionState.lastObservedState}' || true", | |
| status='READY') |
🤖 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 `@benchmark_runner/common/ocp_resources/create_odf.py` around lines 60 - 62,
Update the wait_for_ocp_resource_create call in the ODF creation flow to return
the raw CatalogSource connection state instead of piping it through grep -c
READY. Since count_openshift_storage=True applies an unrelated numeric
comparison, remove or disable that option for this readiness check and pass
status='READY' so the wait validates the CatalogSource state directly.
- Add ODF_CATALOG_IMAGE env var — when set, creates a custom CatalogSource (odf-catalog-source) pointing to the specified image before subscribing - Add 00_catalog_source_template.yaml to create the CatalogSource - Update 07_subscription_template.yaml: use odf-catalog-source when ODF_CATALOG_IMAGE is set, otherwise fall back to redhat-operators - Update create_odf.py to skip empty rendered templates and wait for catalog source to be ready before subscribing - Update Jenkinsfile to pass ODF_CATALOG_IMAGE to the container Usage for OCP 5.0 + ODF 4.23 dev build: ODF_VERSION=4.23 ODF_CATALOG_IMAGE=quay.io/rhceph-dev/ocs-registry:latest-stable-4.23 Assisted-by: Claude Code
c9a2d40 to
28649e1
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
benchmark_runner/common/ocp_resources/create_odf.py (2)
54-54: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win[Claude Code] Use a context manager for the rendered YAML file.
Line 54 opens the file without explicit cleanup. Under runtimes with delayed finalization, repeated resource processing can retain file descriptors. Use
with open(..., encoding='utf-8').Proposed fix
- if not open(yaml_path).read().strip(): - logger.info(f'Skipping empty template: {resource}') - continue + with open(yaml_path, encoding='utf-8') as rendered_yaml: + if not rendered_yaml.read().strip(): + logger.info(f'Skipping empty template: {resource}') + continueAs per path instructions, focus on Python best practices, error handling, and resource cleanup in
benchmark_runner/**.🤖 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 `@benchmark_runner/common/ocp_resources/create_odf.py` at line 54, Update the rendered YAML read in the surrounding resource-processing flow to use a context manager, opening yaml_path with UTF-8 encoding and reading it inside the with block. Preserve the existing empty-content check while ensuring the file is explicitly closed after reading.Source: Path instructions
52-54: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win[Claude Code] Use a context manager when reading
yaml_path.Replace
open(yaml_path).read().strip()with awith open(yaml_path) as file:block.🤖 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 `@benchmark_runner/common/ocp_resources/create_odf.py` around lines 52 - 54, Update the yaml_path reading logic in the resource-rendering flow to use a with open(yaml_path) as file context manager, then strip the file contents for the existing empty-template check. Preserve the current skip behavior for blank rendered templates.Source: Linters/SAST tools
🤖 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.
Nitpick comments:
In `@benchmark_runner/common/ocp_resources/create_odf.py`:
- Line 54: Update the rendered YAML read in the surrounding resource-processing
flow to use a context manager, opening yaml_path with UTF-8 encoding and reading
it inside the with block. Preserve the existing empty-content check while
ensuring the file is explicitly closed after reading.
- Around line 52-54: Update the yaml_path reading logic in the
resource-rendering flow to use a with open(yaml_path) as file context manager,
then strip the file contents for the existing empty-template check. Preserve the
current skip behavior for blank rendered templates.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 299a3099-ca94-400a-9e68-aa2e1f2524a1
📒 Files selected for processing (1)
benchmark_runner/common/ocp_resources/create_odf.py
…M storage class - create_odf.py: extract idms.yaml from catalog image before deploying resources and wait for MCP rollout when odf_catalog_image is set - create_ocp_resource.py: pass odf_catalog_image to CreateODF - hammerdb_mariadb_vm_template.yaml: use ocs-storagecluster-ceph-rbd-virtualization with ReadWriteMany for live migration support (required per KubeVirt docs) - Jenkinsfile: add ODF Registered stage to add quay.io/rhceph-dev pull secret credentials before operator deployment when ODF_CATALOG_IMAGE is set Assisted-by: Claude Code
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 (1)
jenkins/PerfCI/02_PerfCI_Operators_Deployment/Jenkinsfile (1)
151-164: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win[Claude Code] Pass registry authentication into the deployment container.
The
podman logincommand runs on the Jenkins host, butoc image extractruns inside the separatesudo podman runcontainer. The deployment command does not mount an auth file or setREGISTRY_AUTH_FILE. A private catalog image can fail before itsCatalogSourceis created.Create a temporary auth file with
podman login --authfile, mount it read-only, setREGISTRY_AUTH_FILEin the container, and remove the file after deployment.🤖 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 `@jenkins/PerfCI/02_PerfCI_Operators_Deployment/Jenkinsfile` around lines 151 - 164, Update the deployment flow around the sudo podman run invoking QUAY_BENCHMARK_RUNNER_REPOSITORY to create a temporary registry auth file using podman login --authfile, pass REGISTRY_AUTH_FILE into the container, and mount the auth file read-only so oc image extract can authenticate to private registries. Ensure the temporary auth file is removed after deployment completes, including failure paths.
🤖 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 `@benchmark_runner/common/ocp_resources/create_odf.py`:
- Around line 48-49: Remove the unused f-string prefix from the logger.info call
in the no-idms.yaml branch, while preserving the existing log message and
behavior.
- Around line 45-47: Update the `verify_cmd` passed by the ODF resource-creation
flow to select only the `master` and `worker` MachineConfigPools, and match
their updated statuses by exact lines rather than substring counts. Preserve the
expected status of two so `wait_for_ocp_resource_create` completes only when
both required pools report `True`.
In `@jenkins/PerfCI/02_PerfCI_Operators_Deployment/Jenkinsfile`:
- Around line 89-96: Update the shell block in the deployment stage to disable
tracing before handling credentials, replace the podman login password argument
with podman login --password-stdin, and create the temporary pull-secret files
with restrictive permissions. Add a shell trap that removes
global-pull-secret.json and its temporary file on every exit path, while
preserving the existing secret retrieval, merge, and oc set data flow.
---
Outside diff comments:
In `@jenkins/PerfCI/02_PerfCI_Operators_Deployment/Jenkinsfile`:
- Around line 151-164: Update the deployment flow around the sudo podman run
invoking QUAY_BENCHMARK_RUNNER_REPOSITORY to create a temporary registry auth
file using podman login --authfile, pass REGISTRY_AUTH_FILE into the container,
and mount the auth file read-only so oc image extract can authenticate to
private registries. Ensure the temporary auth file is removed after deployment
completes, including failure paths.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 16717968-5eed-4da8-b8c1-ddda21526b8f
📒 Files selected for processing (3)
benchmark_runner/common/ocp_resources/create_ocp_resource.pybenchmark_runner/common/ocp_resources/create_odf.pyjenkins/PerfCI/02_PerfCI_Operators_Deployment/Jenkinsfile
| self.wait_for_ocp_resource_create(operator='odf', | ||
| verify_cmd="oc get mcp -o jsonpath='{range .items[*]}{.status.conditions[?(@.type==\"Updated\")].status}{\"\\n\"}{end}' | grep -c True || true", | ||
| status=str(len(['master', 'worker']))) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
file=$(fd -t f 'create_odf\.py$' . | head -n 1)
printf '%s\n' "FILE=$file"
sed -n '1,110p' "$file"
printf '\n-- helper definitions and call sites --\n'
rg -n -C 5 'def wait_for_ocp_resource_create|wait_for_ocp_resource_create\(' benchmark_runnerRepository: redhat-performance/benchmark-runner
Length of output: 28659
🏁 Script executed:
#!/bin/bash
set -eu
helper=$(fd -t f 'create_ocp_resource_operations\.py$' . | head -n 1)
printf '%s\n' "FILE=$helper"
sed -n '1,115p' "$helper"
printf '\n-- focused command-behavior probe --\n'
python3 - <<'PY'
import re
samples = {
"all pools, required pools updated": "True\nTrue\nTrue\n",
"all pools, one required pool updated": "True\nFalse\nTrue\n",
"two required pools updated": "True\nTrue\n",
"count containing 2": "True\n" * 12,
}
for name, output in samples.items():
count_c = len(re.findall(r"True", output))
count_cx = sum(line == "True" for line in output.splitlines())
print(f"{name}: grep-c={count_c}, grep-cx={count_cx}, status-2-substring={str(2) in str(count_c)}")
PYRepository: redhat-performance/benchmark-runner
Length of output: 5542
[Claude Code] Wait only for the master and worker MachineConfigPools.
The command counts every pool, while wait_for_ocp_resource_create uses substring matching. Extra pools can cause a timeout, or a count such as 12 can satisfy status 2 before both required pools are updated. Query the two required pools and use exact-line matching.
🤖 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 `@benchmark_runner/common/ocp_resources/create_odf.py` around lines 45 - 47,
Update the `verify_cmd` passed by the ODF resource-creation flow to select only
the `master` and `worker` MachineConfigPools, and match their updated statuses
by exact lines rather than substring counts. Preserve the expected status of two
so `wait_for_ocp_resource_create` completes only when both required pools report
`True`.
Source: Path instructions
| else: | ||
| logger.info(f'No idms.yaml found in catalog image, skipping IDMS apply') |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
[Claude Code] Remove the unused f-string prefix.
Line 49 has no interpolation. Ruff reports F541 for this expression.
- logger.info(f'No idms.yaml found in catalog image, skipping IDMS apply')
+ logger.info('No idms.yaml found in catalog image, skipping IDMS apply')📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| else: | |
| logger.info(f'No idms.yaml found in catalog image, skipping IDMS apply') | |
| else: | |
| logger.info('No idms.yaml found in catalog image, skipping IDMS apply') |
🧰 Tools
🪛 Ruff (0.16.1)
[error] 49-49: f-string without any placeholders
Remove extraneous f prefix
(F541)
🤖 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 `@benchmark_runner/common/ocp_resources/create_odf.py` around lines 48 - 49,
Remove the unused f-string prefix from the logger.info call in the no-idms.yaml
branch, while preserving the existing log message and behavior.
Source: Linters/SAST tools
| sh ''' | ||
| oc get secret pull-secret -n openshift-config -o json | jq -r '.data.".dockerconfigjson"' | base64 -d > global-pull-secret.json | ||
| QUAY_AUTH=$(echo -n "${QUAY_USERNAME}:${QUAY_PASSWORD}" | base64 -w 0) | ||
| podman login quay.io -u $QUAY_USERNAME -p $QUAY_PASSWORD | ||
| jq --arg QUAY_AUTH "$QUAY_AUTH" '.auths += {"quay.io/rhceph-dev": {"auth":$QUAY_AUTH,"email":""}}' global-pull-secret.json > global-pull-secret.json.tmp | ||
| mv -f global-pull-secret.json.tmp global-pull-secret.json | ||
| oc set data secret/pull-secret -n openshift-config --from-file=.dockerconfigjson=global-pull-secret.json | ||
| rm -f global-pull-secret.json |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
[Claude Code] Prevent Quay credential exposure.
Line 92 passes QUAY_PASSWORD as a process argument. Line 91 creates an encoded form of the same credential. Lines 90-96 write the full global pull secret into the workspace and remove it only on the success path.
Disable shell tracing before secret operations. Use podman login --password-stdin. Create the temporary file with restrictive permissions. Remove it with a shell trap on every exit path. Jenkins recommends set +x because credential masking is best effort. (jenkins.io)
🤖 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 `@jenkins/PerfCI/02_PerfCI_Operators_Deployment/Jenkinsfile` around lines 89 -
96, Update the shell block in the deployment stage to disable tracing before
handling credentials, replace the podman login password argument with podman
login --password-stdin, and create the temporary pull-secret files with
restrictive permissions. Add a shell trap that removes global-pull-secret.json
and its temporary file on every exit path, while preserving the existing secret
retrieval, merge, and oc set data flow.
1 participant
Summary
ODF_CATALOG_IMAGEenv var to support custom/dev ODF catalog sourcesodf-catalog-sourceCatalogSource before subscribingProblem
On OCP 5.0, ODF 4.23 is not available in any standard catalog source. It's only available via a custom dev image:
quay.io/rhceph-dev/ocs-registry:latest-stable-4.23Changes
New:
00_catalog_source_template.yamlCreates
odf-catalog-sourceCatalogSource usingODF_CATALOG_IMAGE. Skipped (empty file) when not set.Updated:
07_subscription_template.yamlODF_CATALOG_IMAGEset →odf-catalog-source✓ODF_CATALOG_IMAGEempty →redhat-operators(OCP 4.22 works as before) ✓Updated:
create_odf.pyodf-catalog-sourceto beREADYbefore applying subscriptionUpdated:
JenkinsfileAdds
ODF_CATALOG_IMAGEJenkins credential and passes it to the container.Usage
For OCP 5.0 + ODF 4.23 dev build:
Leave
ODF_CATALOG_IMAGEempty to useredhat-operators(existing behavior for OCP 4.x).🤖 Assisted-by: Claude Code
Summary by CodeRabbit