Skip to content

MON-4634: Update collection profiles e2e tests - #31477

Open
simonpasquier wants to merge 6 commits into
openshift:mainfrom
simonpasquier:update-collection-profiles-e2e-tests
Open

MON-4634: Update collection profiles e2e tests#31477
simonpasquier wants to merge 6 commits into
openshift:mainfrom
simonpasquier:update-collection-profiles-e2e-tests

Conversation

@simonpasquier

@simonpasquier simonpasquier commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Summary by CodeRabbit

  • Tests
    • Expanded Prometheus collection profile coverage across full, minimal, empty, and default configurations.
    • Added validation for discovering supported profiles from managed monitors and preserving existing configuration settings.
    • Improved checks for monitor selection and implementation coverage across supported profiles.
    • Added verification that temporary configuration is removed correctly, including scenarios with and without pre-existing settings.

@openshift-merge-bot

Copy link
Copy Markdown
Contributor

Pipeline controller notification
This repo is configured to use the pipeline controller. Second-stage tests will be triggered either automatically or after lgtm label is added, depending on the repository configuration. The pipeline controller will automatically detect which contexts are required and will utilize /test Prow commands to trigger the second stage.

For optional jobs, comment /test ? to see a list of all defined jobs. To trigger manually all jobs from second stage use /pipeline required command.

This repository is configured in: automatic mode

@openshift-ci openshift-ci Bot added ready-for-human-review Indicates a PR has been reviewed by automated tools and is ready for human review do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Aug 3, 2026
@openshift-ci
openshift-ci Bot requested review from machine424 and rexagod August 3, 2026 15:13
@openshift-ci

openshift-ci Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: simonpasquier

The full list of commands accepted by this bot can be found here.

The pull request process is described 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

@openshift-ci openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 3, 2026
@coderabbitai

coderabbitai Bot commented Aug 3, 2026

Copy link
Copy Markdown

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

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

Review profile: CHILL

Plan: Enterprise

Run ID: 88bb0eeb-1b3c-4c40-b4b3-7dc4ea90dd81

📥 Commits

Reviewing files that changed from the base of the PR and between 4ccfabd and 32cff6d.

📒 Files selected for processing (1)
  • test/extended/prometheus/collection_profiles.go
🚧 Files skipped from review as they are similar to previous changes (1)
  • test/extended/prometheus/collection_profiles.go

Walkthrough

Collection profile tests now discover supported profiles from managed ServiceMonitor labels, preserve existing operator ConfigMap settings, use typed selectors and shared assertions, and verify cleanup when configuration did not previously exist.

Changes

Prometheus collection profile tests

Layer / File(s) Summary
Profile configuration and discovery
test/extended/prometheus/collection_profiles.go
Discovers supported profiles at runtime. Updates nested Prometheus configuration while preserving existing ConfigMap YAML.
ServiceMonitor selection and validation
test/extended/prometheus/collection_profiles.go
Uses typed selectors and shared helpers to validate managed, namespace-scoped, and cross-profile ServiceMonitors.
Profile scenarios and cleanup
test/extended/prometheus/collection_profiles.go
Updates default, heterogeneous, empty, and minimal profile scenarios. Verifies deletion when the test created the configuration.

Estimated code review effort: 3 (Moderate) | ~30 minutes

Suggested reviewers: machine424

🚥 Pre-merge checks | ✅ 13 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Test Structure And Quality ⚠️ Warning The test has six bare error assertions (lines 127, 137, 161, 189, 255, 266) and uses suite-wide BeforeAll/AfterAll instead of per-test lifecycle hooks. Add specific messages to every assertion, use BeforeEach/AfterEach or registered cleanup for configuration changes, and pass bounded contexts to direct cluster API calls.
Microshift Test Compatibility ⚠️ Warning The untagged Serial suite uses ServiceMonitors and Prometheus queries in openshift-monitoring; helpers resolve prometheus-k8s, and no MicroShift skip or guard exists. Add [Skipped:MicroShift] or an IsMicroShiftCluster guard. Because this is Serial, also run periodic-ci-openshift-microshift-release-4.22-periodics-e2e-aws-ovn-ocp-conformance-serial.
✅ Passed checks (13 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly identifies the update to collection profiles end-to-end tests and matches the pull request changes.
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 All Ginkgo Describe, Context, and It titles in the changed file are quoted static strings; none includes runtime names, namespaces, nodes, IPs, timestamps, UUIDs, or other generated values.
Single Node Openshift (Sno) Test Compatibility ✅ Passed The added and changed tests use ConfigMaps, ServiceMonitors, and Prometheus queries only; they contain no node, scheduling, replica, topology, failover, drain, or scaling assumptions.
Topology-Aware Scheduling Compatibility ✅ Passed The PR changes only a Prometheus test file. It adds no manifests, controllers, workloads, or scheduling constraints such as affinity, node selectors, tolerations, replicas, or PDBs.
Ote Binary Stdout Contract ✅ Passed The changed file has no fmt.Print, log, or klog writes; its three added diagnostics use g.GinkgoWriter, which the contract explicitly allows.
Ipv6 And Disconnected Network Test Compatibility ✅ Passed The changed test uses Kubernetes APIs and the cluster Prometheus client only; it contains no IPv4 literals, IP parsing, external URLs, public hosts, registry pulls, or external DNS/API calls.
No-Weak-Crypto ✅ Passed The PR changes one Go test file and adds no MD5, SHA1, DES, RC4, Blowfish, ECB, crypto imports, secret comparisons, or custom crypto logic.
Container-Privileges ✅ Passed The commit changes only a Go Prometheus test. It adds no Kubernetes manifest or privilege settings such as privileged, host namespaces, SYS_ADMIN, or allowPrivilegeEscalation.
No-Sensitive-Data-In-Logs ✅ Passed Explicit logs contain only collection profiles and ServiceMonitor namespace/name identifiers; no passwords, tokens, API keys, PII, session IDs, or customer data are logged.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

@simonpasquier

Copy link
Copy Markdown
Contributor Author

/uncc @rexagod

@openshift-ci
openshift-ci Bot removed the request for review from rexagod August 3, 2026 15:19
@simonpasquier

Copy link
Copy Markdown
Contributor Author

/test e2e-gcp-ovn

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
test/extended/prometheus/collection_profiles.go (1)

59-62: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Move collectionProfilesSupportedList into runner.

collectionProfilesSupportedList is mutable state populated in BeforeAll and read by both It blocks. Store it as runner.supportedCollectionProfiles to keep discovered test state with the existing lifecycle state and avoid package-level mutable 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 `@test/extended/prometheus/collection_profiles.go` around lines 59 - 62, Move
collectionProfilesSupportedList from package scope into the runner state as
runner.supportedCollectionProfiles. Update its runtime initialization in
BeforeAll and both It blocks to access the runner field, preserving the existing
discovery and test behavior while removing the package-level mutable variable.
🤖 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 `@test/extended/prometheus/collection_profiles.go`:
- Around line 59-62: Move collectionProfilesSupportedList from package scope
into the runner state as runner.supportedCollectionProfiles. Update its runtime
initialization in BeforeAll and both It blocks to access the runner field,
preserving the existing discovery and test behavior while removing the
package-level mutable variable.

ℹ️ Review info
⚙️ Run configuration

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

Review profile: CHILL

Plan: Enterprise

Run ID: 70a77c8f-a3de-4f78-800a-e8daed9fc1fa

📥 Commits

Reviewing files that changed from the base of the PR and between e3c4a6b and 3829229.

📒 Files selected for processing (1)
  • test/extended/prometheus/collection_profiles.go

@simonpasquier

Copy link
Copy Markdown
Contributor Author

/test e2e-aws-ovn-serial-1of2
/test e2e-aws-ovn-serial-2of2

@simonpasquier

Copy link
Copy Markdown
Contributor Author

/label tide/merge-method-squash

@openshift-ci openshift-ci Bot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Aug 4, 2026
@simonpasquier simonpasquier changed the title WIP: Update collection profiles e2e tests MON-4634: Update collection profiles e2e tests Aug 4, 2026
@openshift-ci-robot

openshift-ci-robot commented Aug 4, 2026

Copy link
Copy Markdown

@simonpasquier: This pull request references MON-4634 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.

Details

In response to this:

Summary by CodeRabbit

  • Tests
  • Expanded coverage for Prometheus collection profiles, including full, minimal, empty, and default configurations.
  • Added validation for profile discovery, configuration updates, preservation of existing settings, and cleanup behavior.
  • Improved assertions to verify monitor selection and configuration outcomes across supported profile scenarios.

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 4, 2026
@openshift-ci openshift-ci Bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 4, 2026
@simonpasquier

Copy link
Copy Markdown
Contributor Author

/test e2e-aws-ovn-serial-1of2
/test e2e-aws-ovn-serial-2of2

1 similar comment
@simonpasquier

Copy link
Copy Markdown
Contributor Author

/test e2e-aws-ovn-serial-1of2
/test e2e-aws-ovn-serial-2of2

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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 `@test/extended/prometheus/collection_profiles.go`:
- Around line 379-386: Update runner.getServiceMonitors to use its namespace
parameter when constructing the ServiceMonitors client instead of always using
operatorNamespaceName, so callers query the requested namespace while preserving
the existing label selector behavior.
🪄 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: Enterprise

Run ID: 1abf29ac-0255-4f96-9bb0-819b836be693

📥 Commits

Reviewing files that changed from the base of the PR and between 3829229 and 5a338e5.

📒 Files selected for processing (1)
  • test/extended/prometheus/collection_profiles.go

Comment thread test/extended/prometheus/collection_profiles.go
@openshift-trt

openshift-trt Bot commented Aug 4, 2026

Copy link
Copy Markdown

Risk analysis has seen new tests most likely introduced by this PR.
Please ensure that new tests meet guidelines for naming and stability.

New Test Risks for sha: 5a338e5

Job Name New Test Risk
pull-ci-openshift-origin-main-e2e-aws-ovn-serial-2of2 Medium - "[sig-instrumentation][OCPFeatureGate:MetricsCollectionProfiles][Serial] The collection profiles feature-set in a heterogeneous environment, should implement all collection profiles or none [Suite:openshift/conformance/serial]" is a new test, and was only seen in one job.
pull-ci-openshift-origin-main-e2e-aws-ovn-serial-2of2 Medium - "[sig-instrumentation][OCPFeatureGate:MetricsCollectionProfiles][Serial] The collection profiles feature-set initially, in a homogeneous default environment, should expose all metrics [Suite:openshift/conformance/serial]" is a new test, and was only seen in one job.

New tests seen in this PR at sha: 5a338e5

  • "[sig-instrumentation][OCPFeatureGate:MetricsCollectionProfiles][Serial] The collection profiles feature-set in a heterogeneous environment, should implement all collection profiles or none [Suite:openshift/conformance/serial]" [Total: 1, Pass: 1, Fail: 0, Flake: 0]
  • "[sig-instrumentation][OCPFeatureGate:MetricsCollectionProfiles][Serial] The collection profiles feature-set initially, in a homogeneous default environment, should expose all metrics [Suite:openshift/conformance/serial]" [Total: 1, Pass: 1, Fail: 0, Flake: 0]

Signed-off-by: Simon Pasquier <spasquie@redhat.com>
@simonpasquier
simonpasquier force-pushed the update-collection-profiles-e2e-tests branch from 5a338e5 to 32cff6d Compare August 5, 2026 07:56
@coderabbitai

coderabbitai Bot commented Aug 5, 2026

Copy link
Copy Markdown

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.

@simonpasquier

Copy link
Copy Markdown
Contributor Author

/test e2e-aws-ovn-serial-1of2
/test e2e-aws-ovn-serial-2of2

@openshift-ci

openshift-ci Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

@simonpasquier: The following tests 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/prow/e2e-gcp-ovn 3829229 link true /test e2e-gcp-ovn
ci/prow/e2e-aws-ovn-serial-2of2 32cff6d link true /test e2e-aws-ovn-serial-2of2

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.

@openshift-trt

openshift-trt Bot commented Aug 5, 2026

Copy link
Copy Markdown

Risk analysis has seen new tests most likely introduced by this PR.
Please ensure that new tests meet guidelines for naming and stability.

New Test Risks for sha: 32cff6d

Job Name New Test Risk
pull-ci-openshift-origin-main-e2e-aws-ovn-serial-2of2 Medium - "[sig-instrumentation][OCPFeatureGate:MetricsCollectionProfiles][Serial] The collection profiles feature-set in a heterogeneous environment, should implement all collection profiles or none [Suite:openshift/conformance/serial]" is a new test, and was only seen in one job.
pull-ci-openshift-origin-main-e2e-aws-ovn-serial-2of2 Medium - "[sig-instrumentation][OCPFeatureGate:MetricsCollectionProfiles][Serial] The collection profiles feature-set initially, in a homogeneous default environment, should expose all metrics [Suite:openshift/conformance/serial]" is a new test, and was only seen in one job.

New tests seen in this PR at sha: 32cff6d

  • "[sig-instrumentation][OCPFeatureGate:MetricsCollectionProfiles][Serial] The collection profiles feature-set in a heterogeneous environment, should implement all collection profiles or none [Suite:openshift/conformance/serial]" [Total: 1, Pass: 1, Fail: 0, Flake: 0]
  • "[sig-instrumentation][OCPFeatureGate:MetricsCollectionProfiles][Serial] The collection profiles feature-set initially, in a homogeneous default environment, should expose all metrics [Suite:openshift/conformance/serial]" [Total: 1, Pass: 1, Fail: 0, Flake: 0]

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

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. ready-for-human-review Indicates a PR has been reviewed by automated tools and is ready for human review tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants