Enable metrics in our upstream test suite by kannon92 · Pull Request #1511 · openshift/kueue-operator

kannon92 · 2026-02-19T16:15:21Z

https://issues.redhat.com/browse/OCPKUEUE-547

coderabbitai · 2026-02-19T16:15:42Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 4201f214-130f-48c1-94a0-b696ecaa6539

📥 Commits

Reviewing files that changed from the base of the PR and between 19a4b14 and c38e145.

📒 Files selected for processing (4)

test/e2e/bindata/assets/08_kueue_default.yaml
upstream/kueue/e2e-test-ocp.sh
upstream/kueue/patch/README.md
upstream/kueue/patch/test_util_e2e.patch

✅ Files skipped from review due to trivial changes (1)

test/e2e/bindata/assets/08_kueue_default.yaml

🚧 Files skipped from review as they are similar to previous changes (3)

upstream/kueue/patch/README.md
upstream/kueue/e2e-test-ocp.sh
upstream/kueue/patch/test_util_e2e.patch

Walkthrough

Adds feature:metrics to the singlecluster Ginkgo label-filter, sets Kueue spec.config.preemption.preemptionPolicy to FairSharing in test assets, documents a new e2e test_util patch, and updates e2e utilities to label created namespaces kueue.openshift.io/managed=true and extend metric polling timeouts.

Changes

Cohort / File(s)	Summary
E2E test script `upstream/kueue/e2e-test-ocp.sh`	`ginkgo_label_filter` `singlecluster` branch now inserts `feature:metrics` into the emitted `--label-filter` string.
Test assets `test/e2e/bindata/assets/08_kueue_default.yaml`	Added `spec.config.preemption.preemptionPolicy: FairSharing` to the `Kueue` resource.
Patch list `upstream/kueue/patch/README.md`	Documented new patch `test_util_e2e.patch` describing namespace-label and metrics-timeout adjustments.
E2E test util patch `upstream/kueue/patch/test_util_e2e.patch`, `upstream/kueue/test/util/.../e2e.go`	`CreateNamespaceFromObjectWithLog` now initializes `ns.Labels` when nil, sets `kueue.openshift.io/managed=true`, logs the namespace object and creation, and uses explicit client create with `gomega.Expect`; metric waiters replace `LongTimeout` with `VeryLongTimeout`.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and specifically describes the main change: enabling metrics in the upstream test suite, which directly aligns with the changeset modification to add feature:metrics to the Ginkgo test labels.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Stable And Deterministic Test Names	✅ Passed	The PR modifies only a shell script that configures Ginkgo test execution with label filters. No test name definitions (It, Describe, etc.) are added or modified.
Test Structure And Quality	✅ Passed	Pull request modifies only a shell script configuration file that selects pre-existing tests via Ginkgo label filters, not test code itself.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@upstream/kueue/patch/test_util_e2e.patch`:
- Around line 8-18: The CreateNamespaceFromObjectWithLog helper was changed to
forcibly add the managed label and directly call k8sClient.Create/assert, which
makes the helper non-neutral; revert it to its original neutral behavior by
removing the label mutation, removal of the direct k8sClient.Create() call and
the extra Expect(), and restore the original MustCreate(ctx, k8sClient, ns) call
and logging of ns.Name in CreateNamespaceFromObjectWithLog; if tests need an
opt-in managed namespace, add a new helper (e.g., CreateManagedNamespaceWithLog)
that sets ns.Labels["kueue.openshift.io/managed"]="true" and creates it
explicitly.

🪄 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: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 7538ad79-5f69-44aa-b588-249eb717fe85

📥 Commits

Reviewing files that changed from the base of the PR and between 4ea01ae and 60fcc26.

📒 Files selected for processing (4)

test/e2e/bindata/assets/08_kueue_default.yaml
upstream/kueue/e2e-test-ocp.sh
upstream/kueue/patch/README.md
upstream/kueue/patch/test_util_e2e.patch

✅ Files skipped from review due to trivial changes (3)

upstream/kueue/patch/README.md
test/e2e/bindata/assets/08_kueue_default.yaml
upstream/kueue/e2e-test-ocp.sh

kannon92 · 2026-04-10T23:36:40Z

/retest

kannon92 · 2026-04-12T14:26:31Z

/retest

MaysaMacedo · 2026-04-13T15:01:53Z

+ 		g.Expect(err).NotTo(gomega.HaveOccurred())
+ 		g.Expect(metricsOutput).Should(utiltesting.ContainMetrics(metrics))
+-	}, LongTimeout, Interval).Should(gomega.Succeed())
+	}, VeryLongTimeout, Interval).Should(gomega.Succeed())


Did you have a chance to check why metrics are taking longer to be available and not available?

Hmm. Maybe I can try removing this.

I noticed that sometimes the metrics were stuck with leader and haven't finished the leader election.

And it seems that they were running before Kueue was fully ready.

If you think this can help with avoiding races, I'm fine with it.

yea let's leave it for now.

MaysaMacedo

/lgtm

MaysaMacedo · 2026-04-13T17:36:14Z

 spec:
  managementState: Managed
  config:
+    preemption:


Just to keep track, this was needed because of this metric https://github.com/kubernetes-sigs/kueue/blob/f6cc162d758d5c2f2cf8c584dfd80b8e1e7749d7/test/e2e/singlecluster/metrics_test.go#L159C5-L159C63

openshift-ci · 2026-04-13T17:36:39Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kannon92, MaysaMacedo

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:

~~OWNERS~~ [MaysaMacedo,kannon92]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci · 2026-04-13T21:52:23Z

@kannon92: all tests passed!

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-ci Bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 19, 2026

openshift-ci Bot requested review from mrunalp and sohankunkerkar February 19, 2026 16:17

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

kannon92 force-pushed the enable-metrics-kueue branch 2 times, most recently from 8be2e2b to c7b3ed0 Compare April 6, 2026 20:35

kannon92 changed the title ~~WIP: Enable metrics in our upstream test suite~~ Enable metrics in our upstream test suite Apr 6, 2026

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 Apr 6, 2026

kannon92 force-pushed the enable-metrics-kueue branch 2 times, most recently from 2427b6a to 60fcc26 Compare April 8, 2026 00:33

coderabbitai Bot reviewed Apr 8, 2026

View reviewed changes

Comment thread upstream/kueue/patch/test_util_e2e.patch

kannon92 force-pushed the enable-metrics-kueue branch from 60fcc26 to 19a4b14 Compare April 9, 2026 23:52

Enable metrics in our upstream test suite

c38e145

kannon92 force-pushed the enable-metrics-kueue branch from 19a4b14 to c38e145 Compare April 10, 2026 21:15

MaysaMacedo reviewed Apr 13, 2026

View reviewed changes

MaysaMacedo approved these changes Apr 13, 2026

View reviewed changes

openshift-ci Bot assigned MaysaMacedo Apr 13, 2026

openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Apr 13, 2026

openshift-merge-bot Bot merged commit 1590e50 into openshift:main Apr 13, 2026
22 checks passed

Conversation

kannon92 commented Feb 19, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coderabbitai Bot commented Feb 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kannon92 commented Apr 10, 2026

Uh oh!

kannon92 commented Apr 12, 2026

Uh oh!

MaysaMacedo Apr 13, 2026

Choose a reason for hiding this comment

Uh oh!

kannon92 Apr 13, 2026

Choose a reason for hiding this comment

Uh oh!

MaysaMacedo Apr 13, 2026

Choose a reason for hiding this comment

Uh oh!

kannon92 Apr 13, 2026

Choose a reason for hiding this comment

Uh oh!

MaysaMacedo left a comment

Choose a reason for hiding this comment

Uh oh!

MaysaMacedo Apr 13, 2026

Choose a reason for hiding this comment

Uh oh!

openshift-ci Bot commented Apr 13, 2026

Uh oh!

openshift-ci Bot commented Apr 13, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

kannon92 commented Feb 19, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Feb 19, 2026 •

edited

Loading