Install NOO using OLMv0 instead of OLMv1 - #3115
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
Summary by CodeRabbit
WalkthroughThe NetObserv operator installation now uses an OLM v0 ChangesNetObserv OLM installation
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant ObservabilityController
participant OpenShiftOLM
participant NetObservOperator
ObservabilityController->>OpenShiftOLM: Apply OLM v0 Subscription
OpenShiftOLM->>NetObservOperator: Install stable-channel operator
ObservabilityController->>OpenShiftOLM: Check CSV installation status
OpenShiftOLM-->>ObservabilityController: Return CSV phase
ObservabilityController->>NetObservOperator: Apply FlowCollector manifest
Suggested reviewers: Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (5 errors, 1 warning)
✅ Passed checks (18 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.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/controller/observability/observability_controller.go (1)
118-145: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftAdd e2e coverage and a PR
Testingsection for the OLMv0 migration.This change alters user-facing installation behavior: the operator now installs through an OLMv0 Subscription, and the FlowCollector namespace is no longer created by CNO. The unit test file only received a comment rename, and the PR description has no
TestingorHow to verify itsection.Add or update tests under
test/e2e/for the OLMv0 install path, and describe the CI lanes, platform coverage, and results in the PR description.As per coding guidelines: "For new or modified Go code under
pkg/that changes user-facing behavior or fixes a bug, add or modify corresponding files undertest/e2e/, and include a PRTestingorHow to verify itsection describing CI lanes, platform coverage, and test results."🤖 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 `@pkg/controller/observability/observability_controller.go` around lines 118 - 145, Add or update end-to-end coverage under test/e2e for the OLMv0 installation path, including validation that the FlowCollector namespace is handled correctly without CNO creating it. Update the PR description with a Testing or How to verify it section documenting CI lanes, platform coverage, and test results.Source: Coding guidelines
🧹 Nitpick comments (1)
pkg/controller/observability/observability_controller.go (1)
128-130: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe
ceExistsbranch is now unreachable for OLMv0 installs.
ceExistsis set fromcheckOLMv1Installation. With the OLMv0 Subscription this staysfalse, so the in-progress path at Line 129 never runs, andinstallNetObservOperatorre-applies the Subscription on every requeue. The apply is idempotent, so the behavior is correct, but the "Applied OLM v0 Subscription" message at Line 124 repeats on each cycle and the comment at Line 128 describes a path that OLMv0 does not use.Consider gating the apply on Subscription existence and reserving the in-progress log for the actual OLMv0 progressing state.
🤖 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 `@pkg/controller/observability/observability_controller.go` around lines 128 - 130, Update the installation flow around installNetObservOperator and ceExists so OLMv0 applies the Subscription only when it does not already exist, avoiding repeated apply logs on requeues. Reserve the “installation in progress” branch and its comment for the actual OLMv1 ClusterExtension state, while preserving requeue behavior for both installation paths.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@bindata/observability/07-observability-operator.yaml`:
- Around line 6-22: Add NetworkPolicy resources for the netobserv-operator
namespace alongside the existing OperatorGroup and Subscription, following the
established operator-namespace pattern to explicitly isolate both ingress and
egress traffic. Ensure the policies target the netobserv-operator namespace and
preserve the current OLM resource configuration.
- Around line 12-22: Update the netobserv-operator Subscription handling to
avoid indefinitely retrying when the configured CatalogSource cannot be
resolved. Add bounded unresolved-attempt tracking and surface a degraded or
informative condition explaining that source/sourceNamespace may be unavailable,
while preserving normal installation when the catalog resolves.
In `@pkg/controller/observability/observability_controller.go`:
- Around line 116-134: Update checkOLMv0Installation and
isNetObservOperatorInstalled so a present FlowCollector CRD with an OLMv0 CSV
still installing is reported as installation in progress, not an identification
error. Propagate that progressing state through Reconcile, preserving the
existing requeue path and avoiding the DeploymentFailed condition until
installation actually fails.
- Around line 447-448: Update the OLMv0 CSV detection condition in
isNetObservOperatorInstalled to use the netobserv-operator prefix, matching
names such as netobserv-operator.v1.11.4-community, and correct the
observability spelling in the adjacent comment.
---
Outside diff comments:
In `@pkg/controller/observability/observability_controller.go`:
- Around line 118-145: Add or update end-to-end coverage under test/e2e for the
OLMv0 installation path, including validation that the FlowCollector namespace
is handled correctly without CNO creating it. Update the PR description with a
Testing or How to verify it section documenting CI lanes, platform coverage, and
test results.
---
Nitpick comments:
In `@pkg/controller/observability/observability_controller.go`:
- Around line 128-130: Update the installation flow around
installNetObservOperator and ceExists so OLMv0 applies the Subscription only
when it does not already exist, avoiding repeated apply logs on requeues.
Reserve the “installation in progress” branch and its comment for the actual
OLMv1 ClusterExtension state, while preserving requeue behavior for both
installation paths.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: ea7101a0-3ff1-44cc-a9db-567ca707b0d8
📒 Files selected for processing (3)
bindata/observability/07-observability-operator.yamlpkg/controller/observability/observability_controller.gopkg/controller/observability/observability_controller_test.go
| apiVersion: operators.coreos.com/v1alpha1 | ||
| kind: Subscription | ||
| metadata: | ||
| name: netobserv-operator | ||
| spec: | ||
| namespace: netobserv-operator | ||
| serviceAccount: | ||
| name: netobserv-operator-installer | ||
| source: | ||
| sourceType: Catalog | ||
| catalog: | ||
| packageName: netobserv-operator | ||
| channels: [stable] | ||
| spec: | ||
| channel: stable | ||
| installPlanApproval: Automatic | ||
| name: netobserv-operator | ||
| source: redhat-operators | ||
| sourceNamespace: openshift-marketplace |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Hardcoded catalog source can break disconnected and custom-catalog clusters.
source: redhat-operators and sourceNamespace: openshift-marketplace assume the default Red Hat catalog is present and enabled. Clusters that disable the default sources or mirror catalogs under a different CatalogSource name will never resolve this Subscription. The reconcile loop then requeues indefinitely without a terminal condition.
Consider surfacing a degraded or informative condition after a bounded number of unresolved attempts, so the cluster admin sees why NetObserv never installs.
🤖 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 `@bindata/observability/07-observability-operator.yaml` around lines 12 - 22,
Update the netobserv-operator Subscription handling to avoid indefinitely
retrying when the configured CatalogSource cannot be resolved. Add bounded
unresolved-attempt tracking and surface a degraded or informative condition
explaining that source/sourceNamespace may be unavailable, while preserving
normal installation when the catalog resolves.
|
/payload-job-with-prs periodic-ci-openshift-release-main-ci-5.0-e2e-aws-ovn-techpreview openshift/origin#31342 /payload-job periodic-ci-openshift-release-main-ci-5.0-e2e-aws-ovn-techpreview @stleerh I am trigerring these payload tests. We need to ensure these are working fine with the e2e tests being added for netobserv operator as well as it is not breaking the existing payload jobs. In the meanwile, can you please fix the unit test? cc @danwinship |
|
@arkadeepsen: An error was encountered. No known errors were detected, please see the full error message for details. Full error message.
unable to get additional pr info from string: /payload-job: string: /payload-job doesn't match expected format: org/repo#number
Please contact an administrator to resolve this issue. |
|
/payload-job periodic-ci-openshift-release-main-ci-5.0-e2e-aws-ovn-techpreview |
|
@arkadeepsen: trigger 4 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/ddb1ba30-9584-11f1-9efc-efe47012d50e-0 |
|
/payload-job-with-prs periodic-ci-openshift-release-main-ci-5.0-e2e-aws-ovn-techpreview openshift/origin#31342 |
|
@arkadeepsen: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/e6312c40-9584-11f1-94bc-26cf251f4a4b-0 |
|
/approve |
openshift-ci
Bot
added
the
approved
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/controller/observability/observability_controller.go (1)
107-154: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftAdd payload E2E coverage for the OLMv0 installation path.
This change modifies installation timeout, Subscription progression, and FlowCollector application behavior. Add
test/e2e/coverage for the tech-preview payload lanes, including serial variants, and document CI lanes, platform coverage, and results in the PR testing section.As per coding guidelines,
pkg/**/*.gochanges that alter user-facing behavior require correspondingtest/e2e/changes and PR testing details.🤖 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 `@pkg/controller/observability/observability_controller.go` around lines 107 - 154, Add test/e2e coverage for the tech-preview payload lanes covering OLMv0 installation, including serial variants and the changed installation-timeout, Subscription progression, and FlowCollector application behavior. Document the CI lanes, platform coverage, and test results in the PR testing section, aligning with the modified reconciliation flow around isNetObservOperatorInstalled and applyManifest.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.
Inline comments:
In `@pkg/controller/observability/observability_controller_test.go`:
- Around line 765-766: Update createTestCSV and the fixture setup around the
status fields to handle every SetNestedField error, including both calls setting
status.reason and status.message and the call at line 153. Propagate or fail the
test on errors instead of discarding them, while preserving the existing fixture
values.
In `@pkg/controller/observability/observability_controller.go`:
- Line 110: Update all calls to setNetworkObservabilityCondition in the
reconciliation flow, including the DeploymentTimedOut path and the branches at
the other referenced locations, so their returned errors are handled rather than
assigned to _. Return a contextual error or requeue when a condition update
fails, ensuring reconciliation does not continue or silently succeed until the
condition is persisted.
- Around line 205-219: Change ReconcileObservability.hasInstallTimedOut to
return (bool, error), propagate the Network CR client.Get error instead of
treating it as not timed out, and update Reconcile to handle the returned error
and retry the status check. Preserve the existing timeout evaluation for
successful reads and ensure all callers handle both return values.
---
Outside diff comments:
In `@pkg/controller/observability/observability_controller.go`:
- Around line 107-154: Add test/e2e coverage for the tech-preview payload lanes
covering OLMv0 installation, including serial variants and the changed
installation-timeout, Subscription progression, and FlowCollector application
behavior. Document the CI lanes, platform coverage, and test results in the PR
testing section, aligning with the modified reconciliation flow around
isNetObservOperatorInstalled and applyManifest.
🪄 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: 62f35059-cebc-4549-9ab1-be21a95be6aa
📒 Files selected for processing (2)
pkg/controller/observability/observability_controller.gopkg/controller/observability/observability_controller_test.go
| _ = unstructured.SetNestedField(csv.Object, "InstallCheckFailed", "status", "reason") | ||
| _ = unstructured.SetNestedField(csv.Object, "install timeout", "status", "message") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 2 'func SetNestedField' -g '*.go' .
rg -n -C 2 'SetNestedField\(csv\.Object' pkg/controller/observability/observability_controller_test.goRepository: openshift/cluster-network-operator
Length of output: 1492
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '235,285p' vendor/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured/helpers.go
sed -n '120,165p' pkg/controller/observability/observability_controller_test.go
sed -n '735,780p' pkg/controller/observability/observability_controller_test.go
rg -n '_ = unstructured\.SetNestedField|SetNestedField\(' pkg/controller/observability/observability_controller_test.goRepository: openshift/cluster-network-operator
Length of output: 5341
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 2 'createTestCSV\(' pkg/controller/observability/observability_controller_test.go
rg -n -C 3 'require\.(NoError|Error)|assert\.(NoError|Error)|t\.Helper\(\)|func createTestCSV' pkg/controller/observability/observability_controller_test.go | head -160
python3 - <<'PY'
def set_nested_field(obj, value, *fields):
m = obj
for field in fields[:-1]:
if field in m:
if isinstance(m[field], dict):
m = m[field]
else:
return f"{field} is not a map"
else:
m[field] = {}
m = m[field]
m[fields[-1]] = value
return None
csv = {"status": {}}
for value, field in [("InstallCheckFailed", "reason"), ("install timeout", "message")]:
print(f"status.{field}: {set_nested_field(csv, value, 'status', field)}")
print(csv)
PYRepository: openshift/cluster-network-operator
Length of output: 1993
Handle all SetNestedField errors in the test fixtures.
createTestCSV creates status as a map, so these calls currently cannot fail. Handle the ignored errors at lines 153, 765, and 766 to keep future fixture changes from producing misleading assertions.
🤖 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 `@pkg/controller/observability/observability_controller_test.go` around lines
765 - 766, Update createTestCSV and the fixture setup around the status fields
to handle every SetNestedField error, including both calls setting status.reason
and status.message and the call at line 153. Propagate or fail the test on
errors instead of discarding them, while preserving the existing fixture values.
Source: Path instructions
| // Check if installation has been failing for too long | ||
| if r.hasInstallTimedOut(ctx) { | ||
| klog.Warning("Network Observability installation timed out, giving up") | ||
| _ = r.setNetworkObservabilityCondition(ctx, operatorv1.ConditionFalse, "DeploymentTimedOut", |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Propagate condition-update errors.
Lines 110, 132, and 138 discard errors from setNetworkObservabilityCondition. If the update at Line 110 fails, reconciliation stops without persisting DeploymentTimedOut. Return a contextual error or requeue until the condition update succeeds.
As per path instructions, **/*.go must “Never ignore error returns.”
Also applies to: 132-132, 138-138
🤖 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 `@pkg/controller/observability/observability_controller.go` at line 110, Update
all calls to setNetworkObservabilityCondition in the reconciliation flow,
including the DeploymentTimedOut path and the branches at the other referenced
locations, so their returned errors are handled rather than assigned to _.
Return a contextual error or requeue when a condition update fails, ensuring
reconciliation does not continue or silently succeed until the condition is
persisted.
Source: Path instructions
| func (r *ReconcileObservability) hasInstallTimedOut(ctx context.Context) bool { | ||
| network := &operatorv1.Network{} | ||
| if err := r.client.Get(ctx, types.NamespacedName{Name: NetworkCRName}, network); err != nil { | ||
| return false | ||
| } | ||
|
|
||
| condition := operatorv1helpers.FindOperatorCondition(network.Status.Conditions, NetworkObservabilityDeployed) | ||
| if condition == nil || condition.Status != operatorv1.ConditionFalse { | ||
| return false | ||
| } | ||
| if condition.Reason == "DeploymentTimedOut" { | ||
| return true | ||
| } | ||
|
|
||
| return time.Since(condition.LastTransitionTime.Time) > installTimeout |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Return the Network CR read error.
Lines 207-209 treat a failed Get as “not timed out.” Change hasInstallTimedOut to return an error and let Reconcile retry the status check. This prevents an API failure from bypassing timeout-state evaluation.
As per path instructions, **/*.go must “Never ignore error returns.”
🤖 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 `@pkg/controller/observability/observability_controller.go` around lines 205 -
219, Change ReconcileObservability.hasInstallTimedOut to return (bool, error),
propagate the Network CR client.Get error instead of treating it as not timed
out, and update Reconcile to handle the returned error and retry the status
check. Preserve the existing timeout evaluation for successful reads and ensure
all callers handle both return values.
Source: Path instructions
When merging, can't it just do a "Squash and merge" so it's automatic? |
|
/payload-job periodic-ci-openshift-release-main-ci-5.0-e2e-aws-ovn-techpreview |
|
@arkadeepsen: trigger 4 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/417a8b00-960c-11f1-9470-fdf97521680d-0 |
|
/payload-job-with-prs periodic-ci-openshift-release-main-ci-5.0-e2e-aws-ovn-techpreview openshift/origin#31342 |
|
@arkadeepsen: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/4ac5b180-960c-11f1-82db-52870a3dfce1-0 |
|
/test ? |
|
/testwith openshift/cluster-network-operator/master/e2e-gcp-ovn-techpreview openshift/origin#31342 |
| olmv1Installed, olmv1CEExists, olmv1Err := r.checkOLMv1Installation(ctx) | ||
| if olmv1Err != nil { | ||
| // Installation error from OLMv1 | ||
| return false, olmv1CEExists, fmt.Errorf("OLMv1 installation error: %w", olmv1Err) | ||
| } | ||
|
|
||
| // Check OLMv0 (ClusterServiceVersion/Subscription) installation status | ||
| olmv0Installed, olmv0Err := r.checkOLMv0Installation(ctx) | ||
| if olmv0Err != nil && !errors.IsNotFound(olmv0Err) { | ||
| // Installation error from OLMv0 | ||
| return false, olmv1CEExists, fmt.Errorf("OLMv0 installation error: %w", olmv0Err) | ||
| } |
There was a problem hiding this comment.
@stleerh didn't you say you wanted to remove those checks, to only rely on CRD?
There was a problem hiding this comment.
There was a long discussion regarding this in the previous PR and this was the approach finalized: #2925 (comment)
There was a problem hiding this comment.
Yes, I want to make this change and now that I've done the investigation, I would like to move forward on this. The idea is that if the FlowCollector CRD exists, there must have been an attempt to install Network Observability. Therefore, it should not try to enable it anymore. In general, it should not try to repair or complete a partial install. This keeps it simple, and it's easy to understand and explain. It basically boils down to:
- If FlowCollector CRD exists, exit.
- Otherwise, install NOO.
- If FlowCollector CR doesn't exist, create it.
Of course, it has to meet all of the prerequisites first. This is the current state. They are:
- If feature gate is disabled, exit.
- If set to NoAction, exit.
- If NOO was already deployed before, exit.
- If SNO and not set to InstallAndEnable, exit.
- If timed out, exit.
There was a problem hiding this comment.
So, if the CRD is not present will it be assumed that NOO is not installed? If that's the assumption, I think the github comment thread from that I shared above needs another look. There are various scenarios in which the CRD may not be there, and attempting to install NOO may fail. Probably the checks can be simplified as mentioned by you, but the whole thing can only rely on the presence of CRD alone.
Another thing to keep in mind is that the behavior should not deviate from the what has been documented in the API, since the user will expect the documented behavior.
There was a problem hiding this comment.
Let's take the discussion separate from this PR. We must go back to OLMv0 because of the lack of Console support for OLMv1. This works as it stands right now. I'll go ahead and squash the commits.
|
LGTM |
|
- Install NOO using OLMv0 instead of OLMv1, since there's a lack of Console integration with OLMv1. - Remove the code to create the namespace for FlowCollector, since it's created automatically - Fix CSV name for OLMv0 - Add two new test cases for OLMv0 - Add a NOO installation tiemout so if it fails, it won't retry forever.
6cb4cda to
5cef6d7
Compare
|
/pipeline required |
|
Scheduling required tests: Scheduling tests matching the |
|
/retest |
|
It is failing on two unrelated tests. If there are no more open issues, can we get this merged? |
|
/retest |
|
@stleerh: The following tests failed, say
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. |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: arkadeepsen, danwinship, stleerh 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 |
|
Tests from second stage were triggered manually. Pipeline can be controlled only manually, until HEAD changes. Use command to trigger second stage. |
4 participants
Uh oh!
There was an error while loading. Please reload this page.