KEP-3695: Add updates around e2e tests #5430

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Open

guptaNswati wants to merge 2 commits into kubernetes:master from guptaNswati:KEP-3695-update-e2e

+17 −1

Contributor

guptaNswati commented Jun 20, 2025

One-line PR description: Update KEP 3695 to address comments in PR kep-3695-beta update #5346
Issue link: DRA: Extend PodResources to include resources from Dynamic Resource Allocation #3695

/wg device-management
/assign @ffromani
/assign @soltysh
/assign @klueska


          Add updates on e2e test as discussed in PR#5346

37c682e

Signed-off-by: Swati Gupta <swatig@nvidia.com>

k8s-ci-robot assigned ffromani and klueska

k8s-ci-robot added the wg/device-management label

k8s-ci-robot assigned soltysh

k8s-ci-robot added the cncf-cla: yes label

github-project-automation bot added this to Dynamic Resource Allocation

github-project-automation bot moved this to 🆕 New in Dynamic Resource Allocation

k8s-ci-robot added kind/kep sig/node labels

k8s-ci-robot requested review from dchen1107 and derekwaynecarr

June 20, 2025 20:37

k8s-ci-robot added the needs-ok-to-test label

Contributor

k8s-ci-robot commented Jun 20, 2025

Hi @guptaNswati. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

k8s-ci-robot added the size/S label

guptaNswati mentioned this pull request

kep-3695-beta update #5346

Merged

lmktfy commented Jun 22, 2025

/retitle KEP-3695: Add updates around e2e tests

k8s-ci-robot changed the title ~~Add updates on e2e test as discussed in PR#5346~~ KEP-3695: Add updates around e2e tests

Contributor

ffromani commented Jun 23, 2025

/ok-to-test

k8s-ci-robot added ok-to-test and removed needs-ok-to-test labels

ffromani reviewed

View reviewed changes

Contributor

ffromani left a comment

thanks for the followup!

keps/sig-node/3695-pod-resources-for-dra/README.md

Comment on lines +270 to +273

+              Other improvements are addressed in:
+              - https://github.com/kubernetes/kubernetes/pull/132028
+              - https://github.com/kubernetes/kubernetes/pull/132345

Contributor

ffromani Jun 23, 2025

I think, but please doublecheck, is better not to link PRs in the KEP but rather describe the goal these PR implement.
The List fix I mentioned previously is meant to illustrate

the very case we missed
the problem with API underspecification - which we kinda have for Get.

So I'd just remove lines 270-272

Contributor Author

guptaNswati Jul 7, 2025

Lets do that once these PRs are merged as @soltysh suggested.

keps/sig-node/3695-pod-resources-for-dra/README.md Outdated

Comment on lines 350 to 352

+              This e2e test (https://github.com/kubernetes/kubernetes/pull/116846) will demonstrate that when the feature gate is disabled, the API returns the appropriate error code. The explicit on/off tests are scattered across the existing tests:
+              - https://github.com/kubernetes/kubernetes/blob/v1.34.0-alpha.1/test/e2e_node/podresources_test.go#L977
+              - https://github.com/kubernetes/kubernetes/blob/v1.34.0-alpha.1/test/e2e_node/podresources_test.go#L1066

Contributor

ffromani Jun 23, 2025

...and we should rectify that and have this test clear and explicit

soltysh reviewed

View reviewed changes

Contributor

soltysh left a comment

A few nits, but overall this is good - thank you!

keps/sig-node/3695-pod-resources-for-dra/README.md

@@ @@ -265,6 +265,12 @@ These cases will be added in the existing e2e tests: @@
                 - Get API work with DRA and device plugin.
                 - List API work with DRA and Device plugin.
+              [Get](https://github.com/kubernetes/kubernetes/pull/116846): [sig-node-kubelet](https://testgrid.k8s.io/sig-node-kubelet?include-filter-by-regex=PodResources), [triage](https://storage.googleapis.com/k8s-triage/index.html?test=PodResources)

Contributor

soltysh Jun 23, 2025

The PR link should be replaced with actual code links to https://github.com/kubernetes/kubernetes/blob/master/test/e2e_node/podresources_test.go

keps/sig-node/3695-pod-resources-for-dra/README.md

+              Other improvements are addressed in:
+              - https://github.com/kubernetes/kubernetes/pull/132028
+              - https://github.com/kubernetes/kubernetes/pull/132345

Contributor

soltysh Jun 23, 2025

Those are fine, since those are no-merged PRs, so when updating this the next time you'll be able to replace these with appropriate links and descriptions like above.

keps/sig-node/3695-pod-resources-for-dra/README.md Outdated

               ###### Are there any tests for feature enablement/disablement?
-              e2e test will demonstrate that when the feature gate is disabled, the API returns the appropriate error code. (https://github.com/kubernetes/kubernetes/pull/116846)
+              This e2e test (https://github.com/kubernetes/kubernetes/pull/116846) will demonstrate that when the feature gate is disabled, the API returns the appropriate error code. The explicit on/off tests are scattered across the existing tests:

Contributor

soltysh Jun 23, 2025

Again, replace the PR link with code pointers to https://github.com/kubernetes/kubernetes/blob/master/test/e2e_node/podresources_test.go

pohly moved this from 🆕 New to 👀 In review in Dynamic Resource Allocation

guptaNswati mentioned this pull request

DRA: Extend PodResources to include resources from Dynamic Resource Allocation #3695

Open

8 tasks

Contributor

klueska commented Jul 14, 2025

@mrunalp can we get an approval on this update PR? It is a follow-up to what was merged before the KEP freeze.


          Add code links

59c48b3

guptaNswati force-pushed the KEP-3695-update-e2e branch from 2ab1e07 to 59c48b3 Compare

July 15, 2025 21:26

ffromani reviewed

View reviewed changes

keps/sig-node/3695-pod-resources-for-dra/README.md

@@ @@ -282,6 +288,13 @@ These cases will be added in the existing e2e tests: @@
               - [ ] Allowing time for feedback (1 year).
               - [ ] Risks have been addressed.
+              - [ ] Add explicit feature enablement/disablement tests, before the feature is turned on by default.
+              - [ ] Additional test cases are needed to verify Get() behavior in diverse scenarios

Contributor

ffromani Jul 21, 2025

where these addressed?

Contributor Author

guptaNswati Jul 21, 2025

this is the feature gate one: kubernetes/kubernetes#132827

ffromani reviewed

View reviewed changes

keps/sig-node/3695-pod-resources-for-dra/README.md

@@ @@ -282,6 +288,13 @@ These cases will be added in the existing e2e tests: @@
               - [ ] Allowing time for feedback (1 year).
               - [ ] Risks have been addressed.
+              - [ ] Add explicit feature enablement/disablement tests, before the feature is turned on by default.
+              - [ ] Additional test cases are needed to verify Get() behavior in diverse scenarios
+                - Pods with multiple containers.

Contributor

ffromani Jul 21, 2025

tests missing

Contributor

ffromani Jul 21, 2025

can be left out/last. I'm working on a PR to add it

Contributor Author

guptaNswati Jul 21, 2025

My understanding was to wait for this to be merged before adding other tests kubernetes/kubernetes#132028

keps/sig-node/3695-pod-resources-for-dra/README.md

+              - [ ] Add explicit feature enablement/disablement tests, before the feature is turned on by default.
+              - [ ] Additional test cases are needed to verify Get() behavior in diverse scenarios
+                - Pods with multiple containers.
+                - Pods that do not use any exclusive resources.

Contributor

ffromani Jul 21, 2025

ok, test already present. Stretch goal: add more coverage

keps/sig-node/3695-pod-resources-for-dra/README.md

+              - [ ] Additional test cases are needed to verify Get() behavior in diverse scenarios
+                - Pods with multiple containers.
+                - Pods that do not use any exclusive resources.
+                - Comparison of List() and Get() on returned pods to validate consistency.

Contributor

ffromani Jul 21, 2025

test missing

keps/sig-node/3695-pod-resources-for-dra/README.md

+                - Pods with multiple containers.
+                - Pods that do not use any exclusive resources.
+                - Comparison of List() and Get() on returned pods to validate consistency.
+                - Pod exists but container name is invalid

Contributor

ffromani Jul 21, 2025

test missing

Contributor

ffromani Jul 22, 2025

correction: turns out we have an existing test that roughly covers this use case

keps/sig-node/3695-pod-resources-for-dra/README.md

+                - Pods that do not use any exclusive resources.
+                - Comparison of List() and Get() on returned pods to validate consistency.
+                - Pod exists but container name is invalid
+                - Get() is called on terminated pods to validate appropriate error handling.

Contributor

ffromani Jul 21, 2025

test missing

mrunalp approved these changes

View reviewed changes

Contributor

k8s-ci-robot commented Jul 22, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: guptaNswati, mrunalp

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~keps/sig-node/OWNERS~~ [mrunalp]

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

k8s-ci-robot added the approved label

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

Labels

approved cncf-cla: yes kind/kep ok-to-test sig/node size/S wg/device-management