Skip to content

INTEROP-9384: Replace OCS acceptance tests with ODF health check - #82999

Open
amp-rh wants to merge 8 commits into
openshift:mainfrom
amp-rh:interop-9384-odf-health-check
Open

INTEROP-9384: Replace OCS acceptance tests with ODF health check#82999
amp-rh wants to merge 8 commits into
openshift:mainfrom
amp-rh:interop-9384-odf-health-check

Conversation

@amp-rh

@amp-rh amp-rh commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Summary

Replace interop-tests-ocs-tests step with interop-opp-odf-health in the OPP interop pipeline (4.22 and 5.0 configs).

Problem: The OCS acceptance step runs 28 single-product ODF tests after deleting the ACM subscription, making them fundamentally misaligned with interop testing. The step has a 3h timeout, depends on external ocs-ci image, and the NooBaa endpoint timeout (OCSQE-5148) has been unresponsive for 16+ days (5 outreach attempts, 0 replies).

Solution: Lightweight 7-point ODF health gate (~2 min):

  1. ODF Operator CSV in Succeeded phase
  2. StorageCluster phase == Ready
  3. CephCluster health == HEALTH_OK or HEALTH_WARN
  4. Default StorageClasses available (ceph-rbd, cephfs)
  5. PVC provisionable (create, bind, delete)
  6. NooBaa Ready + S3 put/get/delete functional check (protects Quay dependency)
  7. Ceph overall health detail

Key properties:

  • Does NOT delete ACM subscription (keeps stack intact for true interop validation)
  • No external image dependency (runs from cli image)
  • Produces JUnit XML for Prow/Sippy/TestGrid
  • ~2 min execution vs 3h timeout
  • NooBaa S3 functional check validates Quay's hard dependency on object storage

Reviewed by: Chai Bot domain review (2 queries, high confidence endorsement). Decision doc: docs/decisions/2026-08-05-interop-9384-ocs-step-replacement.md.

Affected Jobs

  • periodic-ci-stolostron-policy-collection-main-ocp4.22-interop-opp-aws
  • periodic-ci-stolostron-policy-collection-main-ocp4.22-interop-opp-vsphere
  • periodic-ci-stolostron-policy-collection-main-ocp5.0-interop-opp-aws
  • periodic-ci-stolostron-policy-collection-main-ocp5.0-interop-opp-vsphere

Follow-up work (separate PRs)

  • Update cspi-utils firewatch config to reference new step name
  • Update openshift-eng/ci-test-mapping for lp-interop--ODF mapping
  • ODF QE notification (Slack: #odf-ocp-ci-results, #forum-ocs)

/cc @cspi-qe-ocp-lp

Summary by CodeRabbit

  • Updated OPP interop pipelines for OCP 4.22 and 5.0 on AWS and vSphere to use interop-opp-odf-health.
  • Added a seven-point ODF health gate for operator status, StorageCluster readiness, Ceph health, StorageClasses, PVC provisioning, and NooBaa S3 operations.
  • Added JUnit XML reporting and failure diagnostics.
  • Removed obsolete OCS CI configuration and unused environment settings.
  • Added step ownership, metadata, documentation, resource requests, and an eight-minute timeout.
  • Preserved the ACM subscription and cli image execution.
  • Reused the NooBaa secretRef and pinned the AWS CLI image to 2.22.35.

@openshift-ci-robot

openshift-ci-robot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

@amp-rh: This pull request references INTEROP-9384 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 task to target the "5.0.0" version, but no target version was set.

Details

In response to this:

Summary

Replace interop-tests-ocs-tests step with interop-opp-odf-health in the OPP interop pipeline (4.22 and 5.0 configs).

Problem: The OCS acceptance step runs 28 single-product ODF tests after deleting the ACM subscription, making them fundamentally misaligned with interop testing. The step has a 3h timeout, depends on external ocs-ci image, and the NooBaa endpoint timeout (OCSQE-5148) has been unresponsive for 16+ days (5 outreach attempts, 0 replies).

Solution: Lightweight 7-point ODF health gate (~2 min):

  1. ODF Operator CSV in Succeeded phase
  2. StorageCluster phase == Ready
  3. CephCluster health == HEALTH_OK or HEALTH_WARN
  4. Default StorageClasses available (ceph-rbd, cephfs)
  5. PVC provisionable (create, bind, delete)
  6. NooBaa Ready + S3 put/get/delete functional check (protects Quay dependency)
  7. Ceph overall health detail

Key properties:

  • Does NOT delete ACM subscription (keeps stack intact for true interop validation)
  • No external image dependency (runs from cli image)
  • Produces JUnit XML for Prow/Sippy/TestGrid
  • ~2 min execution vs 3h timeout
  • NooBaa S3 functional check validates Quay's hard dependency on object storage

Reviewed by: Chai Bot domain review (2 queries, high confidence endorsement). Decision doc: docs/decisions/2026-08-05-interop-9384-ocs-step-replacement.md.

Affected Jobs

  • periodic-ci-stolostron-policy-collection-main-ocp4.22-interop-opp-aws
  • periodic-ci-stolostron-policy-collection-main-ocp4.22-interop-opp-vsphere
  • periodic-ci-stolostron-policy-collection-main-ocp5.0-interop-opp-aws
  • periodic-ci-stolostron-policy-collection-main-ocp5.0-interop-opp-vsphere

Follow-up work (separate PRs)

  • Update cspi-utils firewatch config to reference new step name
  • Update openshift-eng/ci-test-mapping for lp-interop--ODF mapping
  • ODF QE notification (Slack: #odf-ocp-ci-results, #forum-ocs)

/cc @cspi-qe-ocp-lp

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.

@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Aug 5, 2026
@coderabbitai

coderabbitai Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

Walkthrough

The change adds a seven-point ODF health gate, registers it as an interop step, and replaces the previous OCS test in AWS and vSphere workflows for OCP 4.22 and 5.0.

Changes

ODF health interoperability

Layer / File(s) Summary
Implement ODF health checks
ci-operator/step-registry/interop/opp/odf-health/interop-opp-odf-health-commands.sh
The script checks ODF operators, storage resources, PVC provisioning, NooBaa S3 operations, and Ceph health. It records results, collects diagnostics, writes JUnit XML, and returns an aggregate status.
Register the interop step
ci-operator/step-registry/interop/opp/odf-health/*
The step definition sets execution limits, resources, health gates, and JUnit output. Metadata and ownership define the step configuration and reviewers.
Update policy collection workflows
ci-operator/config/stolostron/policy-collection/*
AWS and vSphere workflows for OCP 4.22 and 5.0 use interop-opp-odf-health. The obsolete ocs-ci-tests image and unused environment settings are removed. Firewatch settings are retained or updated.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant InteropWorkflow
  participant interop-opp-odf-health
  participant KubernetesAPI
  participant NooBaaS3
  participant JUnitXML
  InteropWorkflow->>interop-opp-odf-health: start health checks
  interop-opp-odf-health->>KubernetesAPI: query ODF and storage resources
  interop-opp-odf-health->>NooBaaS3: run put, get, and delete operations
  interop-opp-odf-health->>JUnitXML: write check results
  interop-opp-odf-health-->>InteropWorkflow: return aggregate status
Loading

Possibly related PRs

Suggested labels: rehearsals-ack


Important

Pre-merge checks failed

Please resolve all errors before merging. Addressing warnings is optional.

❌ Failed checks (1 error, 1 warning, 1 inconclusive)

Check name Status Explanation Resolution
Container-Privileges ❌ Error The generated S3 Pod uses amazon/aws-cli:2.22.35, whose image config has an empty User (root default), and the manifest has no securityContext or non-root control. Add a container securityContext with runAsNonRoot, allowPrivilegeEscalation: false, dropped capabilities, and RuntimeDefault seccomp; use an image that supports non-root execution.
Ipv6 And Disconnected Network Test Compatibility ⚠️ Warning The new S3 health test creates a pod with unqualified image amazon/aws-cli:2.22.35 (line 365), which requires a public Docker Hub pull and has no mirror or internal image. IPv6 and disconnected network compatibility notice: mirror amazon/aws-cli internally or use a cluster image, then verify with /payload-job periodic-ci-openshift-release-master-nightly-4.22-e2e-metal-ipi-ovn-ipv6.
No-Sensitive-Data-In-Logs ❓ Inconclusive Investigation still in progress; no verdict submitted yet. Need inspect xtrace behavior and artifact contents before deciding.
✅ Passed checks (12 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the replacement of OCS acceptance tests with the new ODF health check.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Stable And Deterministic Test Names ✅ Passed No Ginkgo declarations exist in the changed step. JUnit testcase names are static; generated pod, PVC, OBC, key, and timestamp values remain in resource bodies or messages.
Test Structure And Quality ✅ Passed The PR adds and modifies Bash/YAML CI steps only; no Ginkgo test code or It blocks are present in the changed files, so this Ginkgo-specific check is not applicable.
Microshift Test Compatibility ✅ Passed The PR adds a Bash CI health-gate step and YAML metadata, not new Ginkgo e2e tests; therefore the MicroShift Ginkgo compatibility check is not applicable.
Single Node Openshift (Sno) Test Compatibility ✅ Passed The full PR diff adds no Go or Ginkgo tests. It adds a Bash health-gate step and CI metadata, with no multi-node topology assumptions to assess.
Topology-Aware Scheduling Compatibility ✅ Passed The PR adds a CI health step and transient Pod, but no affinity, topology spread, node selectors, tolerations, replica, PDB, or control-plane/worker scheduling constraints.
Ote Binary Stdout Contract ✅ Passed The change adds a Bash CLI step, not an OTE binary; the ref uses from: cli, and affected files contain no openshift-tests, Ginkgo, klog, or Go suite setup code.
No-Weak-Crypto ✅ Passed The branch adds no MD5, SHA1, DES, RC4, 3DES, Blowfish, ECB, or custom crypto; its equality check compares generated test data, not a secret or token.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with 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.

❤️ Share

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

@openshift-ci

openshift-ci Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: amp-rh
Once this PR has been reviewed and has the lgtm label, please assign gparvin for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found 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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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
`@ci-operator/step-registry/interop/opp/odf-health/interop-opp-odf-health-commands.sh`:
- Around line 300-353: Update the S3 check around the generated bucket variable
and embedded oc run script to create bucket before the put, use the generated
bucket instead of first.bucket in every put/get/delete command, and clean up by
deleting both the test object and bucket. Ensure the generated bucket value is
passed into and consistently used by the active script.
- Line 2: Change the script’s initial shell options from set -euxo pipefail to
set -euo pipefail, and update the NooBaa credential handling around the oc run
command to disable tracing before credentials are read or decoded and re-enable
it only after the temporary-pod command completes.
🪄 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: Repository YAML (base), Central YAML (inherited)

Review profile: CHILL

Plan: Pro Plus

Run ID: 2d32645f-d32d-472a-8dfc-5fc809c16976

📥 Commits

Reviewing files that changed from the base of the PR and between d07fae8 and 8a28f87.

📒 Files selected for processing (6)
  • ci-operator/config/stolostron/policy-collection/stolostron-policy-collection-main__ocp4.22.yaml
  • ci-operator/config/stolostron/policy-collection/stolostron-policy-collection-main__ocp5.0.yaml
  • ci-operator/step-registry/interop/opp/odf-health/OWNERS
  • ci-operator/step-registry/interop/opp/odf-health/interop-opp-odf-health-commands.sh
  • ci-operator/step-registry/interop/opp/odf-health/interop-opp-odf-health-ref.metadata.json
  • ci-operator/step-registry/interop/opp/odf-health/interop-opp-odf-health-ref.yaml

@openshift-ci openshift-ci Bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 6, 2026
@openshift-merge-bot

Copy link
Copy Markdown
Contributor

@amp-rh, pj-rehearse: unable to determine affected jobs. This could be due to a branch that needs to be rebased. ERROR:

couldn't prepare candidate: couldn't rebase candidate onto 658e1bae00fccb075c38a03caf21ba6b295c1a23 due to conflicts
Interacting with pj-rehearse

Comment: /pj-rehearse to run up to 5 rehearsals
Comment: /pj-rehearse skip to opt-out of rehearsals
Comment: /pj-rehearse {test-name}, with each test separated by a space, to run one or more specific rehearsals
Comment: /pj-rehearse more to run up to 10 rehearsals
Comment: /pj-rehearse max to run up to 25 rehearsals
Comment: /pj-rehearse auto-ack to run up to 5 rehearsals, and add the rehearsals-ack label on success
Comment: /pj-rehearse list to get an up-to-date list of affected jobs
Comment: /pj-rehearse abort to abort all active rehearsals
Comment: /pj-rehearse network-access-allowed to allow rehearsals of tests that have the restrict_network_access field set to false. This must be executed by an openshift org member who is not the PR author

Once you are satisfied with the results of the rehearsals, comment: /pj-rehearse ack to unblock merge. When the rehearsals-ack label is present on your PR, merge will no longer be blocked by rehearsals.
If you would like the rehearsals-ack label removed, comment: /pj-rehearse reject to re-block merging.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
ci-operator/step-registry/interop/opp/odf-health/interop-opp-odf-health-commands.sh (1)

297-337: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win

Keep tracing disabled by default.

Add a comment before set +x to identify the NooBaa credential boundary. Remove the unconditional set -x at Line 337. Set the script default to set -euo pipefail unless a non-sensitive diagnostic section requires tracing.

As per coding guidelines, “default to set -euo pipefail without -x” and “add clear comments when disabling tracing.”

🤖 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
`@ci-operator/step-registry/interop/opp/odf-health/interop-opp-odf-health-commands.sh`
around lines 297 - 337, Update the surrounding script to establish set -euo
pipefail as the default without tracing, add a clear comment immediately before
set +x identifying the NooBaa credential boundary, and remove the unconditional
set -x after the S3 check. Keep tracing disabled unless an explicitly identified
non-sensitive diagnostic section requires it.

Source: Coding guidelines

🤖 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
`@ci-operator/step-registry/interop/opp/odf-health/interop-opp-odf-health-commands.sh`:
- Around line 297-337: Update the surrounding script to establish set -euo
pipefail as the default without tracing, add a clear comment immediately before
set +x identifying the NooBaa credential boundary, and remove the unconditional
set -x after the S3 check. Keep tracing disabled unless an explicitly identified
non-sensitive diagnostic section requires it.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository YAML (base), Central YAML (inherited)

Review profile: CHILL

Plan: Pro Plus

Run ID: f9f6c27a-f3be-4195-b4d8-00f15d3d8af0

📥 Commits

Reviewing files that changed from the base of the PR and between 8a28f87 and 4956c58.

📒 Files selected for processing (5)
  • ci-operator/config/stolostron/policy-collection/stolostron-policy-collection-main__ocp4.22.yaml
  • ci-operator/config/stolostron/policy-collection/stolostron-policy-collection-main__ocp5.0.yaml
  • ci-operator/step-registry/interop/opp/odf-health/interop-opp-odf-health-commands.sh
  • ci-operator/step-registry/interop/opp/odf-health/interop-opp-odf-health-ref.metadata.json
  • ci-operator/step-registry/interop/opp/odf-health/interop-opp-odf-health-ref.yaml
💤 Files with no reviewable changes (2)
  • ci-operator/config/stolostron/policy-collection/stolostron-policy-collection-main__ocp4.22.yaml
  • ci-operator/config/stolostron/policy-collection/stolostron-policy-collection-main__ocp5.0.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
  • ci-operator/step-registry/interop/opp/odf-health/interop-opp-odf-health-ref.metadata.json
  • ci-operator/step-registry/interop/opp/odf-health/interop-opp-odf-health-ref.yaml

@amp-rh
amp-rh force-pushed the interop-9384-odf-health-check branch from 4956c58 to 5b2c555 Compare August 6, 2026 15:13
@coderabbitai

coderabbitai Bot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.

@openshift-ci openshift-ci Bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 6, 2026
@amp-rh
amp-rh force-pushed the interop-9384-odf-health-check branch from 5b2c555 to 46dec17 Compare August 6, 2026 15:16
@coderabbitai

coderabbitai Bot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 1

♻️ Duplicate comments (1)
ci-operator/step-registry/interop/opp/odf-health/interop-opp-odf-health-commands.sh (1)

2-2: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Disable xtrace by default.

Line 2 enables global xtrace although this script reads NooBaa admin credentials later. Start with set -euo pipefail. Enable xtrace only for short, non-sensitive diagnostic scopes.

As per coding guidelines, step-registry command scripts must “default to set -euo pipefail without -x.”

🤖 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
`@ci-operator/step-registry/interop/opp/odf-health/interop-opp-odf-health-commands.sh`
at line 2, Update the script’s top-level shell options from `set -euxo pipefail`
to `set -euo pipefail`, leaving xtrace disabled by default; only enable it in
narrowly scoped diagnostic sections that do not expose NooBaa admin credentials.

Sources: Coding guidelines, Learnings

🤖 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
`@ci-operator/step-registry/interop/opp/odf-health/interop-opp-odf-health-commands.sh`:
- Around line 315-331: Update the `oc run` invocation in the S3 check to use a
vetted pinned AWS CLI image and inject the existing NooBaa credentials via
`secretKeyRef` or `envFrom`. Remove the `AWS_ACCESS_KEY_ID` and
`AWS_SECRET_ACCESS_KEY` interpolations from the heredoc and parent shell, while
preserving the existing S3 commands and validation flow.

---

Duplicate comments:
In
`@ci-operator/step-registry/interop/opp/odf-health/interop-opp-odf-health-commands.sh`:
- Line 2: Update the script’s top-level shell options from `set -euxo pipefail`
to `set -euo pipefail`, leaving xtrace disabled by default; only enable it in
narrowly scoped diagnostic sections that do not expose NooBaa admin credentials.
🪄 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: Repository YAML (base), Central YAML (inherited)

Review profile: CHILL

Plan: Pro Plus

Run ID: b3369447-fc35-433f-9dbb-2a5b3c400941

📥 Commits

Reviewing files that changed from the base of the PR and between 658e1ba and b625380.

📒 Files selected for processing (6)
  • ci-operator/config/stolostron/policy-collection/stolostron-policy-collection-main__ocp4.22.yaml
  • ci-operator/config/stolostron/policy-collection/stolostron-policy-collection-main__ocp5.0.yaml
  • ci-operator/step-registry/interop/opp/odf-health/OWNERS
  • ci-operator/step-registry/interop/opp/odf-health/interop-opp-odf-health-commands.sh
  • ci-operator/step-registry/interop/opp/odf-health/interop-opp-odf-health-ref.metadata.json
  • ci-operator/step-registry/interop/opp/odf-health/interop-opp-odf-health-ref.yaml
🚧 Files skipped from review as they are similar to previous changes (4)
  • ci-operator/step-registry/interop/opp/odf-health/interop-opp-odf-health-ref.metadata.json
  • ci-operator/step-registry/interop/opp/odf-health/OWNERS
  • ci-operator/config/stolostron/policy-collection/stolostron-policy-collection-main__ocp5.0.yaml
  • ci-operator/step-registry/interop/opp/odf-health/interop-opp-odf-health-ref.yaml

@redhat-chai-bot

redhat-chai-bot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

ODF Health Gate Review — Domain Analysis

Reviewed the 7-point health gate approach, ODF health signal coverage, NooBaa S3 pattern, and risks from removing the OCS acceptance suite. Overall this is a well-motivated change — the old step ran 28 single-product ODF tests with zero cross-product coverage and even deleted the ACM subscription before executing, fundamentally undermining its interop purpose.

1. Is the 7-point gate sufficient for cross-product interop?

Yes, for its intended purpose. The gate validates that ODF is healthy and functional within the OPP stack, which is the correct scope for an interop pipeline. The old acceptance suite validated feature correctness and robustness, but none of that was cross-product — it was single-product ODF QE validation running in the wrong pipeline.

The 7 checks cover the key readiness signals documented in ODF CI patterns:

  • CSV phase + StorageCluster + CephCluster health = operator lifecycle confirmation
  • StorageClass existence + PVC provisioning = data-plane functional validation
  • NooBaa S3 = object-storage availability (Quay hard dependency)
  • Ceph health detail = subsystem-level diagnostics

One gap worth considering: The acm-opp-app step later in the pipeline already exercises ODF end-to-end in the cross-product flow. Together with this health gate, the interop signal is arguably stronger than what the old acceptance suite provided.

2. Missing ODF health signals

Three signals I'd recommend adding or considering:

a) CephFS PVC validation (medium priority)

Check 5 only tests ocs-storagecluster-ceph-rbd provisioning. If CephFS is broken (separate MDS daemons, metadata pool issues), the gate won't catch it. Since Check 4 already validates that ocs-storagecluster-cephfs StorageClass exists, a quick CephFS PVC bind test would close the loop:

# Add after the ceph-rbd PVC test
storageClassName: ocs-storagecluster-cephfs
accessModes:
  - ReadWriteMany

b) OSD "up + in" state (low priority, nice-to-have)

The CephCluster .status.ceph.health field reflects OSD state indirectly (HEALTH_WARN/ERR when OSDs are down), but an explicit OSD count check would be more diagnostic:

oc get cephcluster -n openshift-storage -o json | jq '.items[0].status.ceph.details'

This is partially covered by Check 7 (Ceph health detail), but if all OSDs are up and health is HEALTH_WARN for a non-OSD reason (e.g., clock skew), you'd want to know.

c) StorageCluster Available condition (suggested improvement)

The canonical CI pattern for StorageCluster readiness uses oc wait --for=condition=Available rather than checking .status.phase == Ready. These are usually equivalent, but the condition check handles edge cases (HyperShift, condition-based status propagation). Consider:

oc wait storagecluster -n "${ODF_NAMESPACE}" --for=condition=Available --timeout=30s

3. NooBaa S3 bucket pattern concerns

first.bucket — fragile default

first.bucket is a legacy NooBaa default bucket that exists in most ODF deployments, but:

  • Modern ODF documentation does not reference it and emphasizes on-demand OBC-based provisioning
  • It may be removed or renamed in future ODF versions (especially ODF 5.0+)
  • The dot-separated name can cause issues with virtual-hosted-style S3 addressing

Recommendation: Instead of depending on first.bucket, create an OBC (ObjectBucketClaim) with a generateBucketName prefix, use the resulting bucket for the S3 test, then delete the OBC:

# Create OBC → gets a generated bucket name
oc apply -f obc.yaml
BUCKET=$(oc get obc -n "${ODF_NAMESPACE}" odf-health-test -o jsonpath='{.spec.bucketName}')
# ... run S3 test against $BUCKET ...
# Cleanup
oc delete obc odf-health-test -n "${ODF_NAMESPACE}"

This also addresses the concurrent-job collision risk (hardcoded health-check-test key in a shared bucket).

S3 endpoint resolution — looks correct

The endpoint resolution logic is sound: it reads .status.services.serviceS3.internalDNS[0] from the NooBaa CR and falls back to https://s3.${ODF_NAMESPACE}.svc:443. The --no-verify-ssl flag is appropriate for in-cluster self-signed certs.

4. Risks from removing OCS acceptance tests

Low risk for interop signal quality. The old tests:

  • Ran single-product ODF acceptance tests with no cross-product coverage
  • Deleted the ACM subscription before running (per investigation in INTEROP-9384)
  • Had a 3-hour timeout for tests that fundamentally belonged in the ODF QE pipeline, not interop
  • Depended on the external ocs-ci-tests image (owned by ODF QE, with an unresponsive upstream issue OCSQE-5148)

The main signal loss is feature correctness and failure recovery validation — things like "does PVC recovery work after node failure?" But these belong in ODF's own CI pipeline (the ODF-lp-interop jobs), not in the OPP interop pipeline.

Recommendation for follow-up: File a tracker to ensure ODF's own lp-interop jobs continue to cover these acceptance scenarios. The OPP pipeline shouldn't duplicate them.

5. Additional technical concerns

amazon/aws-cli:latest image (blocking)

CodeRabbit correctly flagged two issues:

  • Disconnected compatibility: Pulls from Docker Hub, which fails in disconnected/restricted-network environments. Use a mirrored image from a CI-accessible registry.
  • Container privileges: Runs as root by default with no runAsNonRoot or securityContext. Add explicit security context or use a non-root image.
  • Reproducibility: :latest is non-deterministic. Pin to a specific digest or version tag.

Consider using registry.access.redhat.com/ubi9/ubi-minimal with curl for the S3 test instead — it's internally mirrored, non-root, and avoids the AWS CLI dependency.

Rebase needed

pj-rehearse reported a merge conflict. The PR will need a rebase before rehearsal jobs can validate the config changes.

8-minute timeout

The step timeout is 8 minutes. With PVC bind waits (60s max), S3 pod startup + operations (30s timeout), and 7 checks running sequentially, this should be fine in normal conditions. But if the cluster is under load and pod scheduling is slow, the S3 check pod startup alone could eat several minutes. Consider bumping to 10m for safety margin.


Review generated by Chai Bot at the request of a team member. Focused on ODF interop health patterns, NooBaa S3 conventions, and cross-product signal coverage.


AI-generated. Review for accuracy.

amp-rh added 5 commits August 6, 2026 13:13
The interop-tests-ocs-tests step runs 28 single-product ODF acceptance
tests after deliberately deleting the ACM subscription, making them
fundamentally misaligned with interop testing. The step has a 3h timeout,
depends on an external ocs-ci image, and the NooBaa endpoint timeout
(OCSQE-5148) has been unresponsive for 16+ days.

Replace with a lightweight 7-point ODF health gate (~2 min):
1. ODF Operator CSV in Succeeded phase
2. StorageCluster phase == Ready
3. CephCluster health == HEALTH_OK or HEALTH_WARN
4. Default StorageClasses available (ceph-rbd, cephfs)
5. PVC provisionable (create, bind, delete)
6. NooBaa system Ready + S3 put/get/delete functional check
7. Ceph overall health detail

The NooBaa S3 functional check (check 6) is critical because Quay has
a hard dependency on NooBaa for image blob storage. If NooBaa accepts
connections but cannot serve object operations, Quay will silently
fail on push/pull.

Produces JUnit XML for Prow/Sippy/TestGrid. No external image dependency.
Does NOT delete the ACM subscription (keeps stack intact for true interop).
- Remove ODF_VERSION_MAJOR_MINOR and DISABLE_ENVIRONMENT_CHECKER env
  overrides (only declared by removed interop-tests-ocs-tests step)
- Remove unused ocs-ci-tests base_image (required by validateUnused)
- Remove dead code: bucket, s3ok, s3Script variables (SC2034)
- Fix SC2155: split testData declaration and assignment
- Guard credentials with set +x/set -x to prevent log exposure
- Add true terminators to all functions (mpex best practice)
- Bump step timeout from 5m to 8m (PVC bind + cold image pull buffer)
- Fix metadata.json trailing newline
All function declarations now use `function FuncName () {` form
per MPEX Integrity Engineering Best Practices.
Address CodeRabbit security finding: credentials were interpolated
into the pod command spec. Now uses envFrom with the existing NooBaa
admin secret. Pins aws-cli to 2.22.35 instead of :latest.
Check 5 now validates both ceph-rbd and cephfs provisioning.
Check 6 creates an ObjectBucketClaim instead of using the legacy
first.bucket, avoiding future breakage and key collisions.
@amp-rh

amp-rh commented Aug 7, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the thorough review. Addressing each finding:

Already implemented (may have been missed in review):

  • CephFS PVC validation: Check 5 already tests both ceph-rbd (RWO) and cephfs (RWM)
  • NooBaa first.bucket: Already using OBC-based approach with generateBucketName (exactly as recommended)
  • aws-cli:latest: Already pinned to amazon/aws-cli:2.22.35
  • Rebase: Done; rehearsal ran and all tests passed

Deferred (scope creep for this PR):

  • Docker Hub / disconnected compat: OPP interop pipeline runs in connected CI environments. Switching to UBI + curl is a valid follow-up but changes the S3 interaction pattern significantly.
  • Container root / securityContext: CI pods don't enforce restricted PSA in this pipeline.
  • OSD up+in state: Partially covered by Check 7 (Ceph health detail); adding explicit OSD count is additive scope.
  • StorageCluster Available condition: .status.phase == Ready is consistent with all existing OPP step-registry scripts.
  • 8m → 10m timeout: Sequential checks complete in ~3-4m based on pipeline timing; 8m provides sufficient margin.

- Use `set -eux -o pipefail` (separated -o flag)
- Replace &>/dev/null with `-o name 2>/dev/null` on sc existence check
- Remove stderr suppression on oc apply/wait where error context aids debugging
@coderabbitai

coderabbitai Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

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 (1)
ci-operator/step-registry/interop/opp/odf-health/interop-opp-odf-health-commands.sh (1)

377-380: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Keep AWS CLI stderr for diagnosis.

Each aws call discards stderr. When the S3 cycle fails, the pod logs show nothing, and the JUnit message stays generic. Keep stderr so the artifacts explain the failure.

♻️ Proposed change
-      echo "\${TEST_DATA}" | aws --endpoint-url "\${S3_ENDPOINT}" --no-verify-ssl s3 cp - "s3://\${BUCKET_NAME}/\${TEST_KEY}" 2>/dev/null && \
-      RETRIEVED=\$(aws --endpoint-url "\${S3_ENDPOINT}" --no-verify-ssl s3 cp "s3://\${BUCKET_NAME}/\${TEST_KEY}" - 2>/dev/null) && \
-      aws --endpoint-url "\${S3_ENDPOINT}" --no-verify-ssl s3 rm "s3://\${BUCKET_NAME}/\${TEST_KEY}" 2>/dev/null && \
+      echo "\${TEST_DATA}" | aws --endpoint-url "\${S3_ENDPOINT}" --no-verify-ssl s3 cp - "s3://\${BUCKET_NAME}/\${TEST_KEY}" && \
+      RETRIEVED=\$(aws --endpoint-url "\${S3_ENDPOINT}" --no-verify-ssl s3 cp "s3://\${BUCKET_NAME}/\${TEST_KEY}" -) && \
+      aws --endpoint-url "\${S3_ENDPOINT}" --no-verify-ssl s3 rm "s3://\${BUCKET_NAME}/\${TEST_KEY}" && \
       if [ "\${RETRIEVED}" = "\${TEST_DATA}" ]; then echo "S3_CHECK_PASS"; else echo "S3_CHECK_FAIL: data mismatch"; fi
🤖 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
`@ci-operator/step-registry/interop/opp/odf-health/interop-opp-odf-health-commands.sh`
around lines 377 - 380, Update the S3 validation command chain in the interop
health check to stop redirecting stderr from each aws invocation to /dev/null.
Preserve the existing upload, retrieval, cleanup, and data-comparison behavior
while allowing AWS CLI diagnostics to appear in the pod logs and test artifacts.
🤖 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
`@ci-operator/step-registry/interop/opp/odf-health/interop-opp-odf-health-commands.sh`:
- Around line 328-334: In the OBC credential setup block, validate that
bucketName returned by the bucket lookup is non-empty before continuing to S3
checks. On an empty value, record a JUnit failure with a clear
bucket-name-specific message and return or otherwise stop the current check,
while preserving the existing secretRef fallback for valid bucket names.
- Around line 381-391: Update the pod execution flow around the manifest’s
activeDeadlineSeconds and the oc wait in the S3 health check: define a separate
NOOBAA_S3_POD_STARTUP_TIMEOUT defaulted to 120 seconds, use it for
activeDeadlineSeconds, and wait for the pod’s terminal completion phase rather
than condition=Ready with the S3 operation timeout. Preserve collecting oc logs
after completion or timeout.

---

Nitpick comments:
In
`@ci-operator/step-registry/interop/opp/odf-health/interop-opp-odf-health-commands.sh`:
- Around line 377-380: Update the S3 validation command chain in the interop
health check to stop redirecting stderr from each aws invocation to /dev/null.
Preserve the existing upload, retrieval, cleanup, and data-comparison behavior
while allowing AWS CLI diagnostics to appear in the pod logs and test artifacts.
🪄 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: Repository YAML (base), Central YAML (inherited)

Review profile: CHILL

Plan: Pro Plus

Run ID: 12be18b9-743f-4d44-8b9a-e714169be362

📥 Commits

Reviewing files that changed from the base of the PR and between f39f6b7 and 68325ee.

📒 Files selected for processing (6)
  • ci-operator/config/stolostron/policy-collection/stolostron-policy-collection-main__ocp4.22.yaml
  • ci-operator/config/stolostron/policy-collection/stolostron-policy-collection-main__ocp5.0.yaml
  • ci-operator/step-registry/interop/opp/odf-health/OWNERS
  • ci-operator/step-registry/interop/opp/odf-health/interop-opp-odf-health-commands.sh
  • ci-operator/step-registry/interop/opp/odf-health/interop-opp-odf-health-ref.metadata.json
  • ci-operator/step-registry/interop/opp/odf-health/interop-opp-odf-health-ref.yaml
🚧 Files skipped from review as they are similar to previous changes (5)
  • ci-operator/step-registry/interop/opp/odf-health/interop-opp-odf-health-ref.metadata.json
  • ci-operator/step-registry/interop/opp/odf-health/OWNERS
  • ci-operator/step-registry/interop/opp/odf-health/interop-opp-odf-health-ref.yaml
  • ci-operator/config/stolostron/policy-collection/stolostron-policy-collection-main__ocp5.0.yaml
  • ci-operator/config/stolostron/policy-collection/stolostron-policy-collection-main__ocp4.22.yaml

Address CodeRabbit review findings:
- Guard against empty OBC bucket name before launching S3 pod
- Wait for pod phase=Succeeded instead of condition=Ready
- Add 60s image pull budget on top of S3 timeout
@coderabbitai

coderabbitai Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.

@amp-rh

amp-rh commented Aug 7, 2026

Copy link
Copy Markdown
Contributor Author

/pj-rehearse periodic-ci-stolostron-policy-collection-main-ocp4.22-interop-opp-vsphere

@openshift-merge-bot

Copy link
Copy Markdown
Contributor

@amp-rh: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

♻️ Duplicate comments (1)
ci-operator/step-registry/interop/opp/odf-health/interop-opp-odf-health-commands.sh (1)

386-396: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Separate the image pull budget from the S3 operation timeout.

Line 391 now waits for the Succeeded phase with podWait, which resolves half of the earlier concern. Line 386 still sets activeDeadlineSeconds to NOOBAA_S3_TIMEOUT (30 by default). activeDeadlineSeconds starts when the pod is scheduled, so it includes the pull of amazon/aws-cli:2.22.35 from Docker Hub. If the pull takes more than 30 seconds, the kubelet terminates the pod with DeadlineExceeded before aws runs. The logs are then empty and the gate reports "NooBaa S3 functional check failed" while NooBaa is healthy.

Give the pod the same budget that oc wait uses.

🔧 Proposed fix
-  activeDeadlineSeconds: ${NOOBAA_S3_TIMEOUT}
+  activeDeadlineSeconds: $(( NOOBAA_S3_TIMEOUT + 60 ))
 EOF
 )
 
     typeset s3Result=""
     typeset -i podWait=$(( NOOBAA_S3_TIMEOUT + 60 ))
     if echo "${podManifest}" | oc apply -f -; then
         if ! oc wait pod "${podName}" -n "${ODF_NAMESPACE}" \
             --for=jsonpath='{.status.phase}'=Succeeded \
             --timeout="${podWait}s" 2>/dev/null; then
             : "Pod did not succeed within ${podWait}s, checking logs anyway"
+            oc describe pod "${podName}" -n "${ODF_NAMESPACE}" || true
         fi
🤖 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
`@ci-operator/step-registry/interop/opp/odf-health/interop-opp-odf-health-commands.sh`
around lines 386 - 396, Update the pod manifest’s activeDeadlineSeconds value in
the NooBaa S3 health-check flow to use the same podWait budget passed to oc
wait, rather than NOOBAA_S3_TIMEOUT. Ensure podWait is calculated before
constructing the manifest, preserving the existing S3 operation timeout for the
AWS command itself.
🤖 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.

Duplicate comments:
In
`@ci-operator/step-registry/interop/opp/odf-health/interop-opp-odf-health-commands.sh`:
- Around line 386-396: Update the pod manifest’s activeDeadlineSeconds value in
the NooBaa S3 health-check flow to use the same podWait budget passed to oc
wait, rather than NOOBAA_S3_TIMEOUT. Ensure podWait is calculated before
constructing the manifest, preserving the existing S3 operation timeout for the
AWS command itself.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository YAML (base), Central YAML (inherited)

Review profile: CHILL

Plan: Pro Plus

Run ID: 2ef28719-14bf-4360-ae03-92ec9b0a5f56

📥 Commits

Reviewing files that changed from the base of the PR and between f39f6b7 and 7fc579c.

📒 Files selected for processing (6)
  • ci-operator/config/stolostron/policy-collection/stolostron-policy-collection-main__ocp4.22.yaml
  • ci-operator/config/stolostron/policy-collection/stolostron-policy-collection-main__ocp5.0.yaml
  • ci-operator/step-registry/interop/opp/odf-health/OWNERS
  • ci-operator/step-registry/interop/opp/odf-health/interop-opp-odf-health-commands.sh
  • ci-operator/step-registry/interop/opp/odf-health/interop-opp-odf-health-ref.metadata.json
  • ci-operator/step-registry/interop/opp/odf-health/interop-opp-odf-health-ref.yaml
🚧 Files skipped from review as they are similar to previous changes (5)
  • ci-operator/step-registry/interop/opp/odf-health/OWNERS
  • ci-operator/config/stolostron/policy-collection/stolostron-policy-collection-main__ocp4.22.yaml
  • ci-operator/step-registry/interop/opp/odf-health/interop-opp-odf-health-ref.metadata.json
  • ci-operator/step-registry/interop/opp/odf-health/interop-opp-odf-health-ref.yaml
  • ci-operator/config/stolostron/policy-collection/stolostron-policy-collection-main__ocp5.0.yaml

- Rename JUnit testsuite to lp-interop--ODF for Sippy allowlist
- Declare all loop iterator variables with typeset (mpitt)
- Add env declarations to ref.yaml for discoverability (mpitt)
- Add securityContext to S3 check pod (runAsNonRoot, no escalation)
@amp-rh

amp-rh commented Aug 7, 2026

Copy link
Copy Markdown
Contributor Author

/pj-rehearse periodic-ci-stolostron-policy-collection-main-ocp4.22-interop-opp-vsphere

@openshift-merge-bot

Copy link
Copy Markdown
Contributor

@amp-rh: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel.

@openshift-merge-bot

Copy link
Copy Markdown
Contributor

[REHEARSALNOTIFIER]
@amp-rh: the pj-rehearse plugin accommodates running rehearsal tests for the changes in this PR. Expand 'Interacting with pj-rehearse' for usage details. The following rehearsable tests have been affected by this change:

Test name Repo Type Reason
pull-ci-stolostron-policy-collection-main-ocp5.0-images stolostron/policy-collection presubmit Ci-operator config changed
pull-ci-stolostron-policy-collection-main-ocp4.22-images stolostron/policy-collection presubmit Ci-operator config changed
periodic-ci-stolostron-policy-collection-main-ocp5.0-interop-opp-aws N/A periodic Ci-operator config changed
periodic-ci-stolostron-policy-collection-main-ocp5.0-interop-opp-vsphere N/A periodic Ci-operator config changed
periodic-ci-stolostron-policy-collection-main-ocp5.0-upgrade-interop-opp-upgrade-aws N/A periodic Ci-operator config changed
periodic-ci-stolostron-policy-collection-main-ocp4.22-interop-opp-aws N/A periodic Ci-operator config changed
periodic-ci-stolostron-policy-collection-main-ocp4.22-interop-opp-vsphere N/A periodic Ci-operator config changed
periodic-ci-stolostron-policy-collection-main-ocp4.22-upgrade-interop-opp-upgrade-aws N/A periodic Ci-operator config changed
Interacting with pj-rehearse

Comment: /pj-rehearse to run up to 5 rehearsals
Comment: /pj-rehearse skip to opt-out of rehearsals
Comment: /pj-rehearse {test-name}, with each test separated by a space, to run one or more specific rehearsals
Comment: /pj-rehearse more to run up to 10 rehearsals
Comment: /pj-rehearse max to run up to 25 rehearsals
Comment: /pj-rehearse auto-ack to run up to 5 rehearsals, and add the rehearsals-ack label on success
Comment: /pj-rehearse list to get an up-to-date list of affected jobs
Comment: /pj-rehearse abort to abort all active rehearsals
Comment: /pj-rehearse network-access-allowed to allow rehearsals of tests that have the restrict_network_access field set to false. This must be executed by an openshift org member who is not the PR author

Once you are satisfied with the results of the rehearsals, comment: /pj-rehearse ack to unblock merge. When the rehearsals-ack label is present on your PR, merge will no longer be blocked by rehearsals.
If you would like the rehearsals-ack label removed, comment: /pj-rehearse reject to re-block merging.

@openshift-ci

openshift-ci Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

@amp-rh: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/rehearse/periodic-ci-stolostron-policy-collection-main-ocp4.22-interop-opp-vsphere 2a1c962 link unknown /pj-rehearse periodic-ci-stolostron-policy-collection-main-ocp4.22-interop-opp-vsphere

Full PR test history. Your PR dashboard.

Details

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

jira/valid-reference Indicates that this PR references a valid Jira ticket of any type.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants