Add network observability e2e tests - #31342
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
|
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:
WalkthroughAdds a new Ginkgo e2e suite for NetObserv that checks single-node absence cases and, on supported clusters, validates readiness, monitoring resources, logs, and metric exposure. ChangesNetObserv observability test suite
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 13 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (13 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: 3
🤖 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 `@test/extended/networking/network_observability.go`:
- Around line 41-55: The SNO absence checks in the namespace lookups and
FlowCollector CRD check are too broad because they treat any error as success.
Update the assertions around the namespace Get calls and the
oc.AsAdmin().WithoutNamespace().Run("get").Args("crd",
"flowcollectors.flows.netobserv.io") path to only pass when the error is
specifically NotFound, using apierrors.IsNotFound(err), and fail for all other
errors so the test only confirms the resources are truly absent.
- Around line 58-63: Skip the health spec on single-node clusters by adding the
same IsSingleNode guard used in the earlier NetObserv test before the checks in
the flow data health case. In the test that starts with g.It("should have all
components healthy and producing flow data"), call g.Skip when IsSingleNode
returns true so the namespace and FlowCollector assertions are never run on SNO.
Use the existing IsSingleNode helper and the health spec block to locate the
change.
- Around line 277-290: The Prometheus check in the network observability test
only verifies that a time series exists, not that it contains a non-zero sample.
Update the parsing logic in the Prometheus response handling to extract the
returned sample value from the result payload, then assert that the value is
greater than zero instead of only checking len(result.Data.Result). Keep the
change localized to the promResult handling and the Should(o.BeTrue()) predicate
so the test proves flow ingestion actually produced data.
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: ba6bd409-331b-4935-b969-c585e33c4f4c
📒 Files selected for processing (1)
test/extended/networking/network_observability.go
openshift-ci
Bot
added
the
ready-for-human-review
|
/test verify |
|
Scheduling required tests: |
|
Scheduling required tests: |
|
/retest |
|
/test e2e-aws-ovn-techpreview |
|
/test e2e-aws-ovn-single-node-techpreview |
|
/test e2e-gcp-ovn-techpreview |
|
Risk analysis has seen new tests most likely introduced by this PR. New Test Risks for sha: 70db7d9
New tests seen in this PR at sha: 70db7d9
|
|
/testwith e2e-gcp-ovn-techpreview openshift/cluster-network-operator#2925 |
|
/testwith openshift/cluster-network-operator/master/e2e-gcp-ovn-techpreview openshift/cluster-network-operator#2925 |
|
@arkadeepsen: This PR was included in a payload test run from openshift/cluster-network-operator#2925
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/b9884250-7559-11f1-8ec6-ae97ed4df99f-0 |
|
Scheduling required tests: |
|
/testwith openshift/cluster-network-operator/master/e2e-gcp-ovn-techpreview openshift/cluster-network-operator#2925 |
1 similar comment
|
/testwith openshift/cluster-network-operator/master/e2e-gcp-ovn-techpreview openshift/cluster-network-operator#2925 |
|
@stleerh: This PR was included in a payload test run from openshift/cluster-network-operator#2925
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/2a68d680-7959-11f1-974c-e4475b8eb2aa-0 |
|
Risk analysis has seen new tests most likely introduced by this PR. New Test Risks for sha: 6fcbf23
New tests seen in this PR at sha: 6fcbf23
|
|
Risk analysis has seen new tests most likely introduced by this PR. New Test Risks for sha: 6fcbf23
New tests seen in this PR at sha: 6fcbf23
|
|
/test e2e-gcp-ovn-techpreview-serial-1of2 |
|
@arkadeepsen @kyrtapz |
|
/test e2e-gcp-ovn-techpreview |
|
Risk analysis has seen new tests most likely introduced by this PR. New Test Risks for sha: 6fcbf23
New tests seen in this PR at sha: 6fcbf23
|
1 similar comment
|
Risk analysis has seen new tests most likely introduced by this PR. New Test Risks for sha: 6fcbf23
New tests seen in this PR at sha: 6fcbf23
|
|
/test e2e-gcp-ovn-techpreview I'll give it one more try just to be clear that the tests are not impacting other tests |
|
@danwinship PTAL |
|
Risk analysis has seen new tests most likely introduced by this PR. New Test Risks for sha: 6fcbf23
New tests seen in this PR at sha: 6fcbf23
|
|
@arkadeepsen: This PR was included in a payload test run from openshift/cluster-network-operator#3115
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/e6312c40-9584-11f1-94bc-26cf251f4a4b-0 |
|
@arkadeepsen: This PR was included in a payload test run from openshift/cluster-network-operator#3115
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/4ac5b180-960c-11f1-82db-52870a3dfce1-0 |
|
/test ? |
|
/test openshift/origin/main/e2e-gcp-ovn-techpreview openshift/cluster-network-operator#3115 |
|
/testwith openshift/origin/main/e2e-gcp-ovn-techpreview openshift/cluster-network-operator#3115 |
|
/testwith openshift/origin/main/e2e-gcp-ovn-techpreview openshift/cluster-network-operator#3120 |
2 similar comments
|
/testwith openshift/origin/main/e2e-gcp-ovn-techpreview openshift/cluster-network-operator#3120 |
|
/testwith openshift/origin/main/e2e-gcp-ovn-techpreview openshift/cluster-network-operator#3120 |
|
New changes are detected. LGTM label has been removed. |
|
/testwith openshift/origin/main/e2e-gcp-ovn-techpreview openshift/cluster-network-operator#3120 |
|
Scheduling required tests: |
|
@kapjain-rh: 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. |
|
/testwith openshift/origin/main/e2e-gcp-ovn-techpreview openshift/cluster-network-operator#3120 |
2 similar comments
|
/testwith openshift/origin/main/e2e-gcp-ovn-techpreview openshift/cluster-network-operator#3120 |
|
/testwith openshift/origin/main/e2e-gcp-ovn-techpreview openshift/cluster-network-operator#3120 |
|
/testwith openshift/origin/main/e2e-gcp-ovn-techpreview openshift/cluster-network-operator#3115 |
danwinship
left a comment
danwinship
left a comment
There was a problem hiding this comment.
Please squash the commits
| Status string `json:"status"` | ||
| } | ||
|
|
||
| var _ = g.Describe("[sig-network][OCPFeatureGate:NetworkObservabilityInstall][Feature:NetObserv]", func() { |
There was a problem hiding this comment.
In some contexts the labels with []s get stripped out, so you should have some reference to "Network Observability" outside of []s. Otherwise, eg, the first test would show up as just "should not be installed on signle node clusters" and it's not clear what it's talking about.
| var _ = g.Describe("[sig-network][OCPFeatureGate:NetworkObservabilityInstall][Feature:NetObserv]", func() { | ||
| oc := exutil.NewCLIWithoutNamespace("netobserv-e2e") | ||
|
|
||
| g.It("should not be installed on single node clusters", func(ctx context.Context) { |
There was a problem hiding this comment.
We support NO on SNO, right? It's just not supposed to be automatically installed? Theoretically, in the future, someone might need to create an e2e job for SNO with NO manually installed, and then this test would fail.
So, this should just be asserting "CNO doesn't install NO on SNO clusters", not that NO is never installed on SNO. Just check the NetworkObservabilityDeployed condition on the network operator config.
| g.It("should have FLP pods running", func(ctx context.Context) { | ||
| o.Eventually(func() bool { | ||
| flpPods, err := oc.AdminKubeClient().CoreV1().Pods(netobservNamespace).List(ctx, metav1.ListOptions{ | ||
| LabelSelector: "app=flowlogs-pipeline", |
There was a problem hiding this comment.
These tests are assuming a lot of details about how Network Observability works... is there always going to be a pod with app=flowlogs-pipeline? These tests are supposed to be testing that "the network observability operator has been deployed"; they shouldn't be asserting about the specific details of what the network observability operator does; that's something for NOO's own tests.
If we want to assert that network observability is actually working, then we should assert against the documented customer-visible behaviors of the feature (ie, that the network is being observed, in some way).
| o.Eventually(func() bool { | ||
| pods, err := client.CoreV1().Pods(netobservOperatorNamespace).List(ctx, metav1.ListOptions{}) | ||
| if err != nil { | ||
| return false |
There was a problem hiding this comment.
Does the netobservOperatorNamespace get deleted when disabling observability? If so, then this is racy; you should return true if the error is "not found"
| }, 5*time.Minute, 10*time.Second).Should(o.BeTrue(), | ||
| "operator pods should terminate after uninstall") | ||
|
|
||
| g.By("waiting for FLP pods to terminate") |
There was a problem hiding this comment.
Again, this is assuming too much about the behavior of a component one level away from the component we're testing.
We expect that after NO is disabled, there should be no pods of any sort running in netobservNamespace, right? You could just check that. (Also, same question as above about the namespace itself being deleted.)
| "FLP pods should be Running in %s", netobservNamespace) | ||
| } | ||
|
|
||
| var _ = g.Describe("[sig-network][OCPFeatureGate:NetworkObservabilityInstall][Feature:NetObserv][Serial]", g.Ordered, func() { |
There was a problem hiding this comment.
should be [Disruptive] too
| hasAccess, err := hasNetworkConfigWriteAccess(oc) | ||
| o.Expect(err).NotTo(o.HaveOccurred()) | ||
| if !hasAccess { | ||
| g.Skip("The test is not permitted to modify the cluster/network.config.openshift.io resource") |
There was a problem hiding this comment.
What is this for?
This seems like it would just cause bugs in the e2e job config to not get noticed...
5 participants
Summary
PrometheusRules), and end-to-end flow data pipeline from FLP metrics to Prometheus ingestion
Test plan
Summary by CodeRabbit
Summary