Skip to content

Add reference to etcd and ose-cli rhel9 manifest list image#369

Merged
openshift-merge-bot[bot] merged 7 commits intoopendatahub-io:mainfrom
davidebianchi:feat/RHOAIENG-32222
Sep 2, 2025
Merged

Add reference to etcd and ose-cli rhel9 manifest list image#369
openshift-merge-bot[bot] merged 7 commits intoopendatahub-io:mainfrom
davidebianchi:feat/RHOAIENG-32222

Conversation

@davidebianchi
Copy link
Member

@davidebianchi davidebianchi commented Aug 19, 2025

Motivation

Update etcd and ose-cli images to follow guidelines

Modifications

Updated ectd and ose-cli images.

Result

Test the release with opendatahub-operator, and it release the correct images.

PR checklist

Checklist items below are applicable for development targeted to both fast and stable branches/tags

  • Unit tests pass locally
  • FVT tests pass locally
  • If the PR adds a new container image or updates the tag of an existing image (not build within cpaas), is the corresponding change made in live-builder and cpaas-midstream to add/update the image tag in the operator CSV? Link the PRs if applicable

Checklist items below are applicable for development targeted to both fast and stable branches/tags

  • Tested modelmesh serving deployment with odh-manifests and ran odh-manifests-e2e tests locally

Summary by CodeRabbit

  • New Features

    • Etcd and CLI images made configurable via deployment parameters and templated placeholders; initContainer image reference path adjusted for deployment resolution.
  • Chores

    • Updated default container images to RHEL9-based variants across configs and test manifests.
    • Minor formatting cleanups with no behavioral impact.
  • Documentation

    • Fixed broken cheatsheet link in the scripts README.

@coderabbitai
Copy link

coderabbitai bot commented Aug 19, 2025

Walkthrough

Added Kustomize vars ose-etcd and ose-cli to the ODH overlay and parameterized init/container image references to use those vars; appended image values to overlay params; updated multiple etcd and oauth-proxy image references across FVT and deployment templates; fixed a README link.

Changes

Cohort / File(s) Summary
ODH overlay vars & params
config/overlays/odh/kustomization.yaml, config/overlays/odh/params.env, config/overlays/odh/params.yaml, config/overlays/odh/quickstart.yaml
Added Kustomize vars ose-etcd and ose-cli sourced via fieldRef from ConfigMap mesh-parameters; appended ose-etcd and ose-cli image entries to params.env; changed varReference path in params.yaml to target spec/template/spec/initContainers[]/image; replaced static images in quickstart.yaml with $(ose-cli) (initContainer) and $(ose-etcd) (etcd container).
FVT manifests
config/dependencies/fvt.yaml, opendatahub/scripts/manifests/fvt/fvt.yaml
config/dependencies/fvt.yaml: etcd image reference changed from a pinned digest to the placeholder $(ose-etcd); opendatahub/scripts/manifests/fvt/fvt.yaml: etcd container image replaced with a different RHEL9 etcd image digest.
Internal deployment template
config/internal/base/deployment.yaml.tmpl
Swapped oauth-proxy container image to the RHEL9 variant (image name/digest changed); other container fields unchanged.
Controller testdata
controllers/testdata/servingruntime_controller.golden
Updated eight deployments in test data to use the RHEL9 ose-oauth-proxy image digest instead of the prior image digest; other fields unchanged.
Docs fix
opendatahub/scripts/README.md
Fixed link makefile_-cheatsheet.mdmakefile-cheatsheet.md.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant Env as params.env
  participant CM as ConfigMap(mesh-parameters)
  participant K as Kustomize (odh overlay)
  participant Q as Quickstart Deployment
  participant F as FVT manifests
  participant D as Internal Deployment Template

  Env->>K: provide default `ose-etcd` and `ose-cli`
  CM-->>K: fieldRef reads data.ose-etcd / data.ose-cli
  K->>K: create vars `ose-etcd`, `ose-cli`
  K->>Q: substitute `$(ose-cli)` -> initContainers[].image
  K->>Q: substitute `$(ose-etcd)` -> containers[].image
  K->>F: inject `$(ose-etcd)` into dependency fvt.yaml
  note right of D: oauth-proxy images replaced in template and testdata (no runtime control-flow change)
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

I nibbled at digests, swapped them for bright vars,
slipped ose-cli and ose-etcd into overlay jars.
Manifests updated, tests now hum a new tune,
README mended beneath the deploy-moon.
Off I hop—builds await at noon. 🥕🐇

Tip

🔌 Remote MCP (Model Context Protocol) integration is now available!

Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats.


📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between af270c0 and 8f75a2d.

📒 Files selected for processing (6)
  • config/dependencies/fvt.yaml (1 hunks)
  • config/internal/base/deployment.yaml.tmpl (1 hunks)
  • config/overlays/odh/kustomization.yaml (1 hunks)
  • config/overlays/odh/params.env (1 hunks)
  • config/overlays/odh/quickstart.yaml (3 hunks)
  • controllers/testdata/servingruntime_controller.golden (8 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
  • config/overlays/odh/kustomization.yaml
  • config/dependencies/fvt.yaml
  • config/overlays/odh/params.env
  • config/overlays/odh/quickstart.yaml
🔇 Additional comments (2)
config/internal/base/deployment.yaml.tmpl (1)

139-139: Ensure multi-arch compatibility for the rhel9 oauth-proxy digest

The deployment template at config/internal/base/deployment.yaml.tmpl:139 now pins to:

image: registry.redhat.io/openshift4/ose-oauth-proxy-rhel9@sha256:d3056b35d9a205b9f2c48d924f199c5ac23904eb18d526e4bff229e7c7181415

Please verify that this digest advertises all of your supported architectures (at minimum: amd64, arm64, s390x). If you have podman or skopeo + jq installed, re-run:

podman manifest inspect docker://registry.redhat.io/openshift4/ose-oauth-proxy-rhel9@sha256:d3056b35d9a205b9f2c48d924f199c5ac23904eb18d526e4bff229e7c7181415 \
  | jq -r '.manifests[].platform.architecture' | sort -u
# or
skopeo inspect --raw docker://registry.redhat.io/openshift4/ose-oauth-proxy-rhel9@sha256:d3056b35d9a205b9f2c48d924f199c5ac23904eb18d526e4bff229e7c7181415 \
  | jq -r '.manifests[].platform.architecture' | sort -u

If neither tool is available, you can fall back to the registry HTTP API + jq (after podman login registry.redhat.io to populate your Docker config):

#!/usr/bin/env bash
set -euo pipefail

REGISTRY=registry.redhat.io
REPO=openshift4/ose-oauth-proxy-rhel9
DIGEST=sha256:d3056b35d9a205b9f2c48d924f199c5ac23904eb18d526e4bff229e7c7181415

# Extract the Bearer token from your local Docker config
TOKEN=$(jq -r '.auths["'$REGISTRY'"].auth' ~/.docker/config.json | base64 -d | cut -d: -f2)

curl -sSL \
  -H "Accept: application/vnd.docker.distribution.manifest.list.v2+json" \
  -H "Authorization: Bearer $TOKEN" \
  "https://$REGISTRY/v2/$REPO/manifests/$DIGEST" \
  | jq -r '.manifests[].platform.architecture' \
  | sort -u

Expect at least:
• amd64
• arm64
• s390x

– if any are missing, please choose a different tag/digest or add the missing architectures.

controllers/testdata/servingruntime_controller.golden (1)

107-107: All oauth-proxy digests updated and no stale references found

Sanity check passed – there are no remaining non-rhel9 oauth-proxy digests, and all eight golden snapshots reference the new RHEL9 digest.

✨ Finishing Touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

CodeRabbit Commands (Invoked using PR/Issue comments)

Type @coderabbitai help to get the list of available commands.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Status, Documentation and Community

  • Visit our Status Page to check the current availability of CodeRabbit.
  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Signed-off-by: Davide Bianchi <10374360+davidebianchi@users.noreply.github.com>
Signed-off-by: Davide Bianchi <10374360+davidebianchi@users.noreply.github.com>
Signed-off-by: Davide Bianchi <10374360+davidebianchi@users.noreply.github.com>
Signed-off-by: Davide Bianchi <10374360+davidebianchi@users.noreply.github.com>
@davidebianchi davidebianchi changed the title feat(RHOAIENG-32222): add reference to etcd and ose-cli rhel9 manifest list image Add reference to etcd and ose-cli rhel9 manifest list image Aug 19, 2025
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🧹 Nitpick comments (3)
config/overlays/odh/params.yaml (1)

8-13: Good addition: initContainers image path now covered; consider future-proofing for other workload kinds

The added varReference for Deployment initContainers[]/image is correct. Optional hardening: include common kinds that might reference these images in the future (Pod, Job, CronJob, DaemonSet, StatefulSet), so placeholders won’t slip through if usage expands.

Apply this diff to extend coverage:

 varReference:
   - path: metadata/namespace
     kind: ServiceAccount
   - path: metadata/name
     kind: ClusterRoleBinding
   - path: subjects/namespace
     kind: RoleBinding
   - path: spec/template/spec/containers[]/image
     kind: Deployment
   - path: spec/template/spec/initContainers[]/image
     kind: Deployment
+  - path: spec/containers[]/image
+    kind: Pod
+  - path: spec/initContainers[]/image
+    kind: Pod
+  - path: spec/template/spec/containers[]/image
+    kind: Job
+  - path: spec/template/spec/initContainers[]/image
+    kind: Job
+  - path: spec/jobTemplate/spec/template/spec/containers[]/image
+    kind: CronJob
+  - path: spec/jobTemplate/spec/template/spec/initContainers[]/image
+    kind: CronJob
+  - path: spec/template/spec/containers[]/image
+    kind: DaemonSet
+  - path: spec/template/spec/initContainers[]/image
+    kind: DaemonSet
+  - path: spec/template/spec/containers[]/image
+    kind: StatefulSet
+  - path: spec/template/spec/initContainers[]/image
+    kind: StatefulSet
   - path: data
     kind: ConfigMap
config/dependencies/fvt.yaml (1)

46-53: Duplicate --data-dir argument passed to etcd

The command list includes --data-dir /tmp/etcd.data twice. It’s harmless (last one wins), but confusing.

Apply this diff to remove the duplicate:

         - etcd
-        - --data-dir # use data directory under /tmp for read/write access by non-root user on OpenShift
-        - /tmp/etcd.data
         - --listen-client-urls
         - http://0.0.0.0:2379
         - --advertise-client-urls
         - http://0.0.0.0:2379
         - "--data-dir"
         - /tmp/etcd.data
opendatahub/scripts/manifests/params.env (1)

8-9: Add ose-cli to keep scripts/manifests generation in sync

Quickstart now uses $(ose-cli) for the init container. If this params.env drives that overlay (or related generated manifests), also add the ose-cli entry here to avoid unresolved placeholders.

 odh-model-controller=quay.io/opendatahub/odh-model-controller:fast
 etcd=registry.redhat.io/openshift4/ose-etcd-rhel9@sha256:ea7545b79599f3868d442fdffdfe9b12a02a4b56ac155f02c0fac4720d475796
+ose-cli=registry.redhat.io/openshift4/ose-cli-rhel9@sha256:<digest>

Replace with the intended RHEL9 manifest list digest used elsewhere in the PR.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 7e8b60b and 7b5a1db.

📒 Files selected for processing (8)
  • config/dependencies/fvt.yaml (1 hunks)
  • config/overlays/odh/kustomization.yaml (1 hunks)
  • config/overlays/odh/params.env (1 hunks)
  • config/overlays/odh/params.yaml (1 hunks)
  • config/overlays/odh/quickstart.yaml (3 hunks)
  • opendatahub/scripts/README.md (1 hunks)
  • opendatahub/scripts/manifests/fvt/fvt.yaml (1 hunks)
  • opendatahub/scripts/manifests/params.env (1 hunks)
🔇 Additional comments (7)
opendatahub/scripts/README.md (1)

22-22: Fix looks good: corrected cheatsheet link

The updated link path is correct and removes the stray underscore.

config/overlays/odh/params.env (1)

5-6: All etcd and ose-cli placeholders are correctly wired and substituted

  • The mesh-parameters ConfigMap (via configMapGenerator) includes the new etcd and ose-cli keys from params.env.
  • In config/overlays/odh/kustomization.yaml, the vars section defines both etcd and ose-cli pointing to data.etcd and data.ose-cli respectively.
  • The varReference entries in config/overlays/odh/params.yaml cover:
    • spec/template/spec/containers[]/image
    • spec/template/spec/initContainers[]/image
    • data on ConfigMaps
  • All occurrences of $(etcd) and $(ose-cli) in:
    • config/overlays/odh/quickstart.yaml
    • config/dependencies/fvt.yaml
      are within those supported fields/kinds and will be substituted by Kustomize.

No further changes needed.

config/overlays/odh/kustomization.yaml (1)

58-71: Vars for etcd and ose-cli wired correctly

FieldRefs to mesh-parameters.data.{etcd,ose-cli} look good and match the new params.env entries.

config/dependencies/fvt.yaml (1)

56-56: Parametrizing etcd image looks good; confirm substitution occurs in this context

Using $(etcd) aligns with the new pattern. Please verify that the kustomize config that consumes this file defines the etcd var and includes a varReference for spec/template/spec/containers[]/image for kind=Deployment; otherwise this placeholder won’t resolve at build time.

config/overlays/odh/quickstart.yaml (3)

52-52: Init image parametrization is consistent

Switching to $(ose-cli) is aligned with the new variable-based approach. Ensure params.yaml (kustomize-config) includes varReference for spec/template/spec/initContainers[]/image so this resolves during build (your PR notes suggest you did this).


75-75: Whitespace-only tweak

No functional impact. OK to keep.


95-95: Main etcd image now parameterized

Looks good. Please confirm kustomize varReference still covers spec/template/spec/containers[]/image (for Deployment) in addition to initContainers[]/image; otherwise $(etcd) here may not be substituted after your recent params.yaml edit.

# image: quay.io/coreos/etcd:v3.5.4
# Tag -> registry.access.redhat.com/rhel7/etcd:3.2.32-34
image: registry.redhat.io/openshift4/ose-etcd@sha256:d3275cd886d13865937d225d8138db7f6b7bf59ac1a94d9fbe61e35286bee6ff
image: $(etcd)
Copy link

Choose a reason for hiding this comment

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

💡 Verification agent

🧩 Analysis chain

Switch to $(etcd) placeholder is fine; verify substitution is wired in scripts/manifests

Since this path isn’t under config/overlays/odh/, ensure the scripts/manifests kustomization (or templating step) will substitute $(etcd) using its own params.env/ConfigMap. Otherwise this will deploy with an unresolved literal string.

Apply this script to confirm substitution wiring:


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "Searching for a kustomization under opendatahub/scripts/manifests..."
fd -HI --glob kustomization.yaml opendatahub/scripts/manifests | sed 's/^/ - /' || true

echo
echo "Show any params.env in scripts/manifests:"
fd -HI --glob params.env opendatahub/scripts/manifests | sed 's/^/ - /' | while read -r f; do
  echo "---- $f"; sed -n '1,120p' "$f"; echo; done

echo
echo "Look for varReferences that would cover images in Deployments/Pods:"
fd -HI --glob params.yaml opendatahub/scripts/manifests | while read -r f; do
  echo "---- $f"; sed -n '1,200p' "$f"; echo; done

echo
echo "Confirm all placeholders in scripts/manifests:"
rg -nC2 -e '\$\((etcd|ose-cli)\)' opendatahub/scripts/manifests || true

Length of output: 2607


I’ll pull in the relevant kustomize config and params to confirm that $(etcd) is defined and wired correctly:


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "Kustomization files under scripts/manifests:"
fd -HI --glob kustomization.yaml opendatahub/scripts/manifests | sed 's/^/ - /'

echo
echo "Contents of fvt kustomization.yaml:"
sed -n '1,200p' opendatahub/scripts/manifests/fvt/kustomization.yaml

echo
echo "Contents of params.env:"
sed -n '1,200p' opendatahub/scripts/manifests/params.env

echo
echo "Vars & ConfigMapGenerator in fvt kustomization.yaml:"
rg -n -C3 '^(vars:|configMapGenerator)' opendatahub/scripts/manifests/fvt/kustomization.yaml || true

echo
echo "Placeholder occurrences for $(etcd):"
rg -nC2 '\$\((etcd)\)' opendatahub/scripts/manifests/fvt || true

Length of output: 2244


Wire the $(etcd) placeholder in fvt/kustomization.yaml
The current kustomization under scripts/manifests/fvt doesn’t load params.env nor declare a var for etcd, so image: $(etcd) will remain literal at deploy time.

Please update opendatahub/scripts/manifests/fvt/kustomization.yaml to include:

  • A ConfigMapGenerator that pulls in params.env
  • A vars entry that points etcd at the generated ConfigMap’s data.etcd field

For example:

 apiVersion: kustomize.config.k8s.io/v1beta1
 kind: Kustomization
 resources:
 - fvt.yaml
 patchesStrategicMerge:
 - remove_etcd_patch.yaml
 namespace: modelmesh-serving

+# load all key=values from params.env, including etcd
+configMapGenerator:
+  - name: etcd-params
+    env: ../params.env
+
+vars:
+  - name: etcd
+    objref:
+      kind: ConfigMap
+      name: etcd-params
+      apiVersion: v1
+    fieldref:
+      fieldpath: data.etcd

Ensure the relative path to params.env is correct and that kustomize version supports vars substitution. After this change, image: $(etcd) will be replaced with the SHA-pinned RHEL9 etcd image.

📝 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.

Suggested change
image: $(etcd)
apiVersion: kustomize.config.k8s.io/v1beta1
kind: Kustomization
resources:
- fvt.yaml
patchesStrategicMerge:
- remove_etcd_patch.yaml
namespace: modelmesh-serving
# load all key=values from params.env, including etcd
configMapGenerator:
- name: etcd-params
env: ../params.env
vars:
- name: etcd
objref:
kind: ConfigMap
name: etcd-params
apiVersion: v1
fieldref:
fieldpath: data.etcd
🤖 Prompt for AI Agents
In opendatahub/scripts/manifests/fvt/kustomization.yaml (near where
resources/patches are declared), add a ConfigMapGenerator that loads the
params.env file and a vars entry mapping the name "etcd" to the generated
ConfigMap's data.etcd field so the $(etcd) placeholder is substituted.
Specifically: add a configMapGenerator block that references the relative path
to params.env (ensure the path is correct from this kustomization file) and
gives the ConfigMap a stable name, then add a vars section with name: etcd,
objref pointing to that ConfigMap (kind: ConfigMap, name: <generated-name>) and
fieldref: data.etcd. Confirm your kustomize version supports vars substitution.

…tahub

Signed-off-by: Davide Bianchi <10374360+davidebianchi@users.noreply.github.com>
Copy link
Member

@grdryn grdryn left a comment

Choose a reason for hiding this comment

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

Looks like the right approach, thanks @davidebianchi! I'm not all that familiar with this project, so will try to get someone more knowledgeable to take a look. 🙂

I've left a couple of comments/thoughts inline below also.

# image: quay.io/coreos/etcd:v3.5.4
# Tag -> registry.access.redhat.com/rhel7/etcd:3.2.32-34
image: registry.redhat.io/openshift4/ose-etcd@sha256:d3275cd886d13865937d225d8138db7f6b7bf59ac1a94d9fbe61e35286bee6ff
image: registry.redhat.io/openshift4/ose-etcd-rhel9@sha256:ea7545b79599f3868d442fdffdfe9b12a02a4b56ac155f02c0fac4720d475796
Copy link
Member

Choose a reason for hiding this comment

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

I'm interested to know how this one is used, and why it can't be changed to $(etcd). I guess it's because it's not kustomized in the same way? I wonder if maybe it should be?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it is not kustomized in the same way. I could have added into the kustomization file the same override, but I noticed that there is a patch to delete the etcd configurations.

patchesStrategicMerge:
- remove_etcd_patch.yaml

So, I thought it's no longer useful. I’d like to understand whether it makes sense to handle this here as well, or if it’s not really needed.

Copy link
Member

Choose a reason for hiding this comment

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

this is only for test purposes, fvp on ci.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, thank you. What do you think could be the best way to manage it? Use the params.env as in config overlays, or keep as is?

Copy link
Member

Choose a reason for hiding this comment

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

Sounds like it should be fine to keep it as you have it here, if it's just for testing purposes.

Copy link
Member Author

Choose a reason for hiding this comment

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

I noticed that fvt github actions does not run, there is an error and are never triggered. Is it known? Was it intentional?

@davidebianchi
Copy link
Member Author

@brettmthompson can you please take a look?

@openshift-ci openshift-ci bot requested review from Jooho and spolti August 22, 2025 13:29
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (3)
config/overlays/odh/params.env (3)

5-6: Add a brief comment to document intent (operator will pin digests)

A short note helps future maintainers understand why tags (not digests) are used here.

Apply this diff:

-ose-etcd=registry.redhat.io/openshift4/ose-etcd-rhel9:v4.19
-ose-cli=registry.redhat.io/openshift4/ose-cli-rhel9:v4.19
+# Defaults use minor-stream tags; operator/overrides are expected to pin to digests.
+ose-etcd=registry.redhat.io/openshift4/ose-etcd-rhel9:v4.19
+ose-cli=registry.redhat.io/openshift4/ose-cli-rhel9:v4.19

5-6: Confirm pull access and mirroring strategy for registry.redhat.io

These images require valid pull credentials. Please verify:

  • Target clusters have the necessary pull-secret access to registry.redhat.io for workloads in this namespace.
  • For disconnected/air-gapped environments, operator overrides (or a mirror/ICSP) are in place to point to mirrored ose-etcd/ose-cli images.

Optionally note in the README/overlay docs that these params are defaults and may be overridden by operator-supplied, digest-pinned, mirrored images for disconnected clusters.


5-6: Confirm kustomize varReference coverage and hyphenated var support

I inspected config/overlays/odh/kustomization.yaml and found:

  • No varReference block is defined (only a single vars: entry for mesh-namespace at lines 22–27).
  • The only vars declared are for mesh-namespace; ose-etcd and ose-cli aren’t listed under vars:.
  • There is a ConfigMap with keys data.ose-etcd and data.ose-cli (lines 56–70), but without corresponding vars and varReference entries, kustomize won’t substitute $(ose-etcd) or $(ose-cli) in Deployment images.

Please verify:

  • Your kustomize version’s default varReference covers both
    • spec/template/spec/containers[]/image
    • spec/template/spec/initContainers[]/image
      under kind: Deployment, so you don’t need to explicitly declare them.
  • Hyphenated var names (ose-etcd, ose-cli) are supported by your kustomize version (some versions only allow [A-Za-z0-9_]+).

If either condition is not met, you should:

  1. Add vars for ose-etcd and ose-cli and broaden varReference:
    vars:
      - name: ose-etcd
        objref:
          kind: ConfigMap
          name: mesh-parameters
          apiVersion: v1
        fieldref:
          fieldpath: data.ose-etcd
      - name: ose-cli
        objref:
          kind: ConfigMap
          name: mesh-parameters
          apiVersion: v1
        fieldref:
          fieldpath: data.ose-cli
    
    varReference:
      - kind: Deployment
        path: spec/template/spec/containers[]/image
      - kind: Deployment
        path: spec/template/spec/initContainers[]/image
  2. Or rename vars to use underscores (e.g. ose_etcd, ose_cli) if hyphens aren’t supported.

To double-check no placeholders remain after build, run locally (expect no matches):

kustomize build config/overlays/odh \
  | rg -nP '\$\((ose-etcd|ose-cli)\)' || true
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between dcc4881 and af270c0.

📒 Files selected for processing (4)
  • config/dependencies/fvt.yaml (1 hunks)
  • config/overlays/odh/kustomization.yaml (1 hunks)
  • config/overlays/odh/params.env (1 hunks)
  • config/overlays/odh/quickstart.yaml (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
  • config/dependencies/fvt.yaml
  • config/overlays/odh/quickstart.yaml
  • config/overlays/odh/kustomization.yaml
⏰ 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). (1)
  • GitHub Check: Analyze (go)
🔇 Additional comments (1)
config/overlays/odh/params.env (1)

5-6: Tag-based defaults (v4.19) for ose-etcd/ose-cli — LGTM

Switching from digests to tags here matches the earlier review guidance and keeps the defaults aligned with the OCP 4.19 minor stream. It also cleanly supports operator-side digest pinning via overrides.

Signed-off-by: Davide Bianchi <10374360+davidebianchi@users.noreply.github.com>
odh-modelmesh=quay.io/opendatahub/modelmesh:fast
odh-modelmesh-controller=quay.io/opendatahub/modelmesh-controller:fast
ose-etcd=registry.redhat.io/openshift4/ose-etcd-rhel9:v4.19
ose-cli=registry.redhat.io/openshift4/ose-cli-rhel9:v4.19
Copy link
Member

@spolti spolti Aug 26, 2025

Choose a reason for hiding this comment

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

Any particular reason for adding it? (the cli one)

Copy link
Member Author

Choose a reason for hiding this comment

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

To follow the dependency management guidelines. It was in the same manifest, so I updated them together. Do you see any problems?

Copy link
Member

Choose a reason for hiding this comment

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

@spolti the idea here is that of RHOAIENG-25619, where the images can be kept consistent with the rest of RHOAI, rather than the old approach of every component specifying their own version of everything, and those all going stale and having 7 different versions of the same image across the different components.

Copy link
Member

Choose a reason for hiding this comment

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

Actually, there are oauth-proxy references in this repository also: https://github.com/search?q=repo%3Aopendatahub-io%2Fmodelmesh-serving+oauth-proxy&type=code

Should/could those also be updated here?

Copy link
Member

Choose a reason for hiding this comment

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

@grdryn I am confused now, will ose-cli replace ose ose-proxy? If not, why add ose-cli?

Copy link
Member

Choose a reason for hiding this comment

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

@spolti ose-cli is a different case than oauth-proxy. The idea is to keep all 3rd party images (or at least other images from RH registry) up-to-date, so ose-cli is being added because it's referenced in config/overlays/odh/quickstart.yaml on L52 (and you can see the reference is updated in this PR).

So maybe we should have a 3rd entry here for ose-oauth-proxy? Or I guess that would actually need to be passed to that deployment.yaml.tmpl from here 🤔 Maybe we should update the static reference once in this PR, and then consider parameterizing that and figuring out how to feed the image through in a potential follow-up change?

Copy link
Member

Choose a reason for hiding this comment

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

I see, thanks.

Yes, this template is processed at runtime, we could add the image there and later patch it accordingly.

Copy link
Member

Choose a reason for hiding this comment

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

Shall we already add the proxy as part of this PR?

Copy link
Member

Choose a reason for hiding this comment

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

I'm easy with it either being done in this PR or another one. Which would you prefer? Maybe we could get this one merged, then create a follow up one for that (or potentially 2 follow ups: one to just update the static image reference in place, then a second to have the image passed through, if that's even a reasonable thing to do)?

Copy link
Member Author

Choose a reason for hiding this comment

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

I updated the oauth-proxy static reference here. Passing the image through is something I'd do in a separate PR.

Signed-off-by: Davide Bianchi <10374360+davidebianchi@users.noreply.github.com>
Copy link
Member

@spolti spolti left a comment

Choose a reason for hiding this comment

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

@Jooho, do you want to take a look before merging?

@openshift-ci
Copy link

openshift-ci bot commented Aug 28, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: davidebianchi, spolti

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@grdryn
Copy link
Member

grdryn commented Sep 2, 2025

@spolti @Jooho bump! :)

@spolti
Copy link
Member

spolti commented Sep 2, 2025

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Sep 2, 2025
@openshift-merge-bot openshift-merge-bot bot merged commit 5114ac9 into opendatahub-io:main Sep 2, 2025
9 checks passed
@grdryn
Copy link
Member

grdryn commented Sep 3, 2025

@spolti is main the correct branch for this to get synced down to rhds? Looks like maybe it needs to be on a branch called release-0.12.0-rc0? 🤔 https://github.com/red-hat-data-services/rhods-devops-infra/blob/main/src/config/upstream-source-map.yaml#L33

Also I wonder if it'll need to be on a stable-2.x branch, if that's going to be used in this repo?

@grdryn
Copy link
Member

grdryn commented Sep 3, 2025

/cherrypick release-0.12.0-rc0
/cherrypick stable-2.x

^Not sure if I have the power to do that in this repo. Doing it optimistically in the hopes that it saves some time in case it needs to be done.

@openshift-cherrypick-robot

@grdryn: #369 failed to apply on top of branch "release-0.12.0-rc0":

Applying: feat(RHOAIENG-32222): add reference to etcd rhel9 manifest list image
Using index info to reconstruct a base tree...
M	config/dependencies/fvt.yaml
M	config/overlays/odh/params.env
M	opendatahub/scripts/manifests/fvt/fvt.yaml
Falling back to patching base and 3-way merge...
Auto-merging opendatahub/scripts/manifests/fvt/fvt.yaml
Auto-merging config/overlays/odh/params.env
CONFLICT (content): Merge conflict in config/overlays/odh/params.env
Auto-merging config/dependencies/fvt.yaml
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config advice.mergeConflict false"
Patch failed at 0001 feat(RHOAIENG-32222): add reference to etcd rhel9 manifest list image

Details

In response to this:

/cherrypick release-0.12.0-rc0
/cherrypick stable-2.x

^Not sure if I have the power to do that in this repo. Doing it optimistically in the hopes that it saves some time in case it needs to be done.

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.

@spolti
Copy link
Member

spolti commented Sep 3, 2025

@grdryn, sry for the delay, the branch which we sync with rhoai is release-0.12.0-rc0.
stable-2.x is a copy of release-0.12.0-rc0.

At this point, any change that needs to land on 2.25 should be cherry-picked to both branches, including main.

@spolti
Copy link
Member

spolti commented Sep 3, 2025

Do you guys need help cherry-picking it to the release branches?

grdryn pushed a commit to grdryn/modelmesh-serving that referenced this pull request Sep 3, 2025
…hub-io#369)

* feat(RHOAIENG-32222): add reference to etcd rhel9 manifest list image

Signed-off-by: Davide Bianchi <10374360+davidebianchi@users.noreply.github.com>

* feat(RHOAIENG-32222): add reference to ose-cli rhel9 manifest list image

Signed-off-by: Davide Bianchi <10374360+davidebianchi@users.noreply.github.com>

* feat(RHOAIENG-32222): rename etcd image

Signed-off-by: Davide Bianchi <10374360+davidebianchi@users.noreply.github.com>

* fix(RHOAIENG-32222): substitute vars in initContainers

Signed-off-by: Davide Bianchi <10374360+davidebianchi@users.noreply.github.com>

* feat(RHOAIENG-32222): set specific image to etcd deployment in opendatahub

Signed-off-by: Davide Bianchi <10374360+davidebianchi@users.noreply.github.com>

* feat: rename etcd to ose-etcd

Signed-off-by: Davide Bianchi <10374360+davidebianchi@users.noreply.github.com>

* feat: update static reference to oauth-proxy

Signed-off-by: Davide Bianchi <10374360+davidebianchi@users.noreply.github.com>

---------

Signed-off-by: Davide Bianchi <10374360+davidebianchi@users.noreply.github.com>
@grdryn
Copy link
Member

grdryn commented Sep 3, 2025

Do you guys need help cherry-picking it to the release branches?

@spolti I've done it now in #372 and #373. I was just hoping that I could be lazy and use Prow's /cherrypick command, but it failed because of a conflict.

@davidebianchi davidebianchi deleted the feat/RHOAIENG-32222 branch September 4, 2025 10:07
openshift-merge-bot bot pushed a commit that referenced this pull request Sep 4, 2025
)

* feat(RHOAIENG-32222): add reference to etcd rhel9 manifest list image



* feat(RHOAIENG-32222): add reference to ose-cli rhel9 manifest list image



* feat(RHOAIENG-32222): rename etcd image



* fix(RHOAIENG-32222): substitute vars in initContainers



* feat(RHOAIENG-32222): set specific image to etcd deployment in opendatahub



* feat: rename etcd to ose-etcd



* feat: update static reference to oauth-proxy



---------

Signed-off-by: Davide Bianchi <10374360+davidebianchi@users.noreply.github.com>
Co-authored-by: Davide Bianchi <10374360+davidebianchi@users.noreply.github.com>
openshift-merge-bot bot pushed a commit that referenced this pull request Sep 4, 2025
)

* feat(RHOAIENG-32222): add reference to etcd rhel9 manifest list image



* feat(RHOAIENG-32222): add reference to ose-cli rhel9 manifest list image



* feat(RHOAIENG-32222): rename etcd image



* fix(RHOAIENG-32222): substitute vars in initContainers



* feat(RHOAIENG-32222): set specific image to etcd deployment in opendatahub



* feat: rename etcd to ose-etcd



* feat: update static reference to oauth-proxy



---------

Signed-off-by: Davide Bianchi <10374360+davidebianchi@users.noreply.github.com>
Co-authored-by: Davide Bianchi <10374360+davidebianchi@users.noreply.github.com>
mholder6 pushed a commit to mholder6/modelmesh-serving that referenced this pull request Sep 25, 2025
…hub-io#369) (opendatahub-io#373)

* feat(RHOAIENG-32222): add reference to etcd rhel9 manifest list image

* feat(RHOAIENG-32222): add reference to ose-cli rhel9 manifest list image

* feat(RHOAIENG-32222): rename etcd image

* fix(RHOAIENG-32222): substitute vars in initContainers

* feat(RHOAIENG-32222): set specific image to etcd deployment in opendatahub

* feat: rename etcd to ose-etcd

* feat: update static reference to oauth-proxy

---------

Signed-off-by: Davide Bianchi <10374360+davidebianchi@users.noreply.github.com>
Co-authored-by: Davide Bianchi <10374360+davidebianchi@users.noreply.github.com>
Signed-off-by: Mariah Holder <marholde@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants