Skip to content

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
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

KEP-3695: Add updates around e2e tests #5430

wants to merge 1 commit into from

Conversation

guptaNswati
Copy link
Contributor

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

@k8s-ci-robot k8s-ci-robot added the wg/device-management Categorizes an issue or PR as relevant to WG Device Management. label Jun 20, 2025
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 20, 2025
@k8s-ci-robot k8s-ci-robot added the kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory label Jun 20, 2025
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: guptaNswati
Once this PR has been reviewed and has the lgtm label, please assign derekwaynecarr for approval. For more information see the Code Review Process.

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

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

@k8s-ci-robot k8s-ci-robot added the sig/node Categorizes an issue or PR as relevant to SIG Node. label Jun 20, 2025
@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jun 20, 2025
@k8s-ci-robot
Copy link
Contributor

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 k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jun 20, 2025
@guptaNswati guptaNswati mentioned this pull request Jun 20, 2025
Merged
@lmktfy
Copy link

lmktfy commented Jun 22, 2025

/retitle KEP-3695: Add updates around e2e tests

@k8s-ci-robot k8s-ci-robot changed the title Add updates on e2e test as discussed in PR#5346 KEP-3695: Add updates around e2e tests Jun 22, 2025
@ffromani
Copy link
Contributor

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jun 23, 2025
ffromani
ffromani reviewed Jun 23, 2025
View reviewed changes
Copy link
Contributor

@ffromani ffromani left a comment

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

So I'd just remove lines 270-272

keps/sig-node/3695-pod-resources-for-dra/README.md
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
Copy link
Contributor

Choose a reason for hiding this comment

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

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

soltysh
soltysh reviewed Jun 23, 2025
View reviewed changes
Copy link
Contributor

@soltysh soltysh left a comment

Choose a reason for hiding this comment

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

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)
Copy link
Contributor

Choose a reason for hiding this comment

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

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
Copy link
Contributor

Choose a reason for hiding this comment

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

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
@@ -334,7 +347,9 @@ The API becomes available again. The API is stateless, so no recovery is needed,

###### 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:
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

@soltysh soltysh soltysh left review comments

@ffromani ffromani ffromani left review comments

@dchen1107 dchen1107 Awaiting requested review from dchen1107

@derekwaynecarr derekwaynecarr Awaiting requested review from derekwaynecarr

Assignees

@klueska klueska

@soltysh soltysh

@ffromani ffromani

Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory ok-to-test Indicates a non-member PR verified by an org member that is safe to test. sig/node Categorizes an issue or PR as relevant to SIG Node. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. wg/device-management Categorizes an issue or PR as relevant to WG Device Management.
Projects
Dynamic Resource Allocation
Status: 🆕 New
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@guptaNswati @k8s-ci-robot @lmktfy @ffromani @soltysh @klueska