Uh oh!
There was an error while loading. Please reload this page.
✨Namespace and PVC probes - #2535
Conversation
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
Pull request overview
Adds readiness probing for additional Kubernetes resource types (Namespaces and PVCs) using CEL-based probes in the ClusterExtensionRevision reconciliation flow, and extends the e2e framework + scenarios to validate direct CER application and phase-blocking behavior when probes fail.
Changes:
- Add CEL readiness probes for Namespace (Active) and PersistentVolumeClaim (Bound) and wire them into the CER progress probe chain.
- Extend e2e step framework to support direct ClusterExtensionRevision application and scenario templating via
${CER_NAME}. - Add new e2e feature coverage for CER install success and probe-driven phase progression/halting (PVC scenarios), plus required RBAC.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| test/e2e/steps/testdata/rbac-template.yaml | Grants e2e SA permissions for PV/PVC resources needed by new scenarios. |
| test/e2e/steps/steps.go | Adds ${CER_NAME} templating and helper to fetch CER objects; expands step matching for “not installed”. |
| test/e2e/steps/hooks.go | Extends scenario context with CER name/state and updates cleanup behavior. |
| test/e2e/features/revision.feature | New e2e feature validating direct CER apply + probe gating for PVC phases. |
| test/e2e/README.md | Documents the new ${CER_NAME} scenario variable. |
| internal/operator-controller/controllers/clusterextensionrevision_controller.go | Initializes and registers CEL probes for Namespace/PVC readiness in CER reconcile options. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
d87dfaf to
6a9f311Compare6a9f311 to
ec5019eCompareThere was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@## main #2535 +/- ##
==========================================
- Coverage 68.62% 68.56% -0.07%
==========================================
Files 131 131 Lines 9288 9304 +16 ==========================================
+ Hits 6374 6379 +5 - Misses 2427 2436 +9 - Partials 487 489 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
493a1a7 to
1c7882bCompareThere was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
1c7882b to
1f0a99fCompare
pedjak
left a comment
There was a problem hiding this comment.
not sure if for this PR is necessary to expose direct creation of CERs, instead of going through CE and appropriate bundles.
Uh oh!
There was an error while loading. Please reload this page.
| And ServiceAccount "olm-sa" with needed permissions is available in ${TEST_NAMESPACE} | ||
| @BoxcutterRuntime | ||
| Scenario: Install simple revision |
There was a problem hiding this comment.
how is this scenario related to the logic introduced in PR? We have plenty of scenarios under install.feature that install exactly these resources.
There was a problem hiding this comment.
IMO it's an easy way to prove that CERs can be used to install content without a ClusterExtension owner. I thought this was necessary since we had discussed the possibility that users might (but probably won't) use the API directly - plus its runtime is only about 1.5 seconds. I can remove it if you think it's unnecessary.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| for _, r := range forDeletion { | ||
| if _, err := k8sClient("delete", r.kind, r.name, "--ignore-not-found=true"); err != nil { | ||
| logger.Info("Error deleting resource", "name", r.name, "namespace", sc.namespace, "stderr", stderrOutput(err)) | ||
| for _, r := range forDeletion { |
There was a problem hiding this comment.
how is this change related to the PR?
There was a problem hiding this comment.
Before the change, in my test runs, the after hook would only delete a single item in forDeletion (the clusterextension) and would leave remaining resources (namespaces and owner-less clusterextensionrevisions) on the cluster. This fixed that and also made the deletion run more quickly by executing them asynchronously.
dtfranz
commented
Mar 5, 2026
The follow-up for this is to add custom CEL probe to the CER API, which will necessitate these tests as we currently have no way of setting the custom probes in the bundle and carrying them into the CER. |
1f0a99f to
f24055aCompareThere was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| pvcBoundCEL = `self.status.phase == "Bound"` | ||
| pvcBoundProbe probing.GroupKindSelector | ||
| // deplStaefulSetProbe probes Deployment, StatefulSet objects. |
There was a problem hiding this comment.
The comment for deplStatefulSetProbe is misspelled/inaccurate (deplStaefulSetProbe). This makes the identifier hard to search for and is inconsistent with the actual variable name. Update the comment to match deplStatefulSetProbe (and correct “Stateful”).
| // deplStaefulSetProbe probes Deployment, StatefulSet objects. | |
| // deplStatefulSetProbe probes Deployment, StatefulSet objects. |
There was a problem hiding this comment.
I didn't change this line, I can keep it in mind for a future PR though
f24055a to
944188bCompareUh oh!
There was an error while loading. Please reload this page.
944188b to
d73a46dCompareThere was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| annotations: | ||
| olm.operatorframework.io/service-account-name: olm-sa | ||
| olm.operatorframework.io/service-account-namespace: ${TEST_NAMESPACE} |
There was a problem hiding this comment.
are these annotations required for reconciliation to work?
There was a problem hiding this comment.
yeah =/ this is a bit of a rough edge since we don't expose the service account configuration in the .spec. I think it's ok to leave it like this for now since we'll need to act on it once we have a decision on the OLM MVP and whether we want to keep the service account or not.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| volumeMode: Filesystem | ||
| local: | ||
| path: /tmp/persistent-volume |
There was a problem hiding this comment.
this works under kind, does it work under openshift?
There was a problem hiding this comment.
It does, I just tested it (aws). I can try a few other cluster types too just in case.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Adds readiness probing via CEL for namespaces and PVCs, to prevent subsequent phases from installing until their readiness checks have passed. Also adds e2e coverage via direct CER creation. Increased e2e timeout to 15m for experimental target. Signed-off-by: Daniel Franz <dfranz@redhat.com>
d73a46d to
be95970Compareperdasilva
commented
Mar 6, 2026
/approve |
rashmigottipati
left a comment
There was a problem hiding this comment.
Changes look great to me!
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 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 |
dtfranz
commented
Mar 9, 2026
/override codecov/patch |
@dtfranz: dtfranz unauthorized: /override is restricted to Repo administrators, approvers in top level OWNERS file, and the following github teams:. 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. |
perdasilva
commented
Mar 9, 2026
/override codecov/patch |
@perdasilva: Overrode contexts on behalf of perdasilva: codecov/patch 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. |
39ed482
into
operator-framework:mainUh oh!
There was an error while loading. Please reload this page.
Adds readiness probing via CEL for namespaces and PVCs, to prevent subsequent phases from installing until their readiness checks have passed. Also adds e2e coverage via direct CER creation.
Description
Reviewer Checklist