e2e: wrap DRA ResourceQuota Forbidden claim check in gomega.Eventually #128943

nojnhuh · 2024-11-22T17:43:39Z

What type of PR is this?

/kind bug
/kind flake

What this PR does / why we need it:

This PR de-flakes the DRA e2e that ensures a ResourceQuota correctly forbids a ResourceClaim that exceeds its defined count/resourceclaims.resource.k8s.io quota. #128410 describes that some flakes occurred where the ResourceClaim was admitted without error, likely due to the API server's cache not being up to date like #128899 simulates. This PR updates the check that newly ResourceClaims are rejected with the expected HTTP 403 message eventually, rather than always as soon as possible on the first try.

Which issue(s) this PR fixes:

Fixes #128410

Special notes for your reviewer:

Does this PR introduce a user-facing change?

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot · 2024-11-22T17:43:48Z

This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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.

nojnhuh · 2024-11-22T17:47:10Z

/cc @pohly

jackfrancis · 2024-11-22T19:28:32Z

/retest

test/e2e/dra/dra.go

jackfrancis

/lgtm

k8s-ci-robot · 2024-11-22T22:53:31Z

LGTM label has been added.

Details

Git tree hash: 013e96905fd4f88c77aff3f953e124e599ee8ff5

test/e2e/dra/dra.go

pohly · 2024-11-25T08:54:55Z

/test pull-kubernetes-unit

Because of flake (#128962).

pohly · 2024-11-25T08:56:21Z

test/e2e/dra/dra.go

+			// with an "already exists" error, so use a new name each time.
+			claim.GenerateName = "claim-1-"
+			claim.Name = ""
+			gomega.Eventually(ctx, func() error {


Suggested change

gomega.Eventually(ctx, func() error {

gomega.Eventually(ctx, func(ctx context.Context) error {

We should get a new context from gomega.Eventually because that honors the timeout set for gomega.Eventually.

Do we have the appetite to clean up the many instances of gomega.Eventually in the k/k codebase that are not using context properly? (there are a lot! :/)

Let me know if there are any SIGs in particular that have an appetite for this and I'd be happy to drive that work.

Do we have the appetite to clean up the many instances of gomega.Eventually in the k/k codebase that are not using context properly?

I don't, but I will point it out in new code when I spot it. OTOH, such a search/replace should be safe, so why not?

The biggest issue is that even if we clean up once, developers will keep adding this back unless we provide some linter which warns about this. Reviewers do not always spot this. Such a linter would be more work.

Let me know if there are any SIGs in particular that have an appetite for this

I'd say "writing better tests" falls under SIG Testing and I've been the TL there who did most of that work in the last years.

Let's use this PR as a reference, then after it merges I'll submit a PR that cleans up similar usage gaps in the codebase.

We should get a new context from gomega.Eventually because that honors the timeout set for gomega.Eventually.

Just pushed this change.

pohly

/lgtm
/approve

k8s-ci-robot · 2024-11-26T13:49:14Z

LGTM label has been added.

Details

Git tree hash: 698cd3862abc7c9a9c74b121793841aaf23e1fdc

k8s-ci-robot · 2024-11-26T13:49:22Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jackfrancis, nojnhuh, pohly

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:

~~test/e2e/dra/OWNERS~~ [pohly]

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 needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Nov 22, 2024

k8s-ci-robot requested review from bart0sh and klueska November 22, 2024 17:43

nojnhuh mentioned this pull request Nov 22, 2024

[WIP] Add integration test showing possible ResourceQuota race #128899

Closed

k8s-ci-robot requested a review from pohly November 22, 2024 17:47

jackfrancis reviewed Nov 22, 2024

View reviewed changes

test/e2e/dra/dra.go Outdated Show resolved Hide resolved

nojnhuh force-pushed the dra-e2e-quota-forbid branch 2 times, most recently from 003495f to 23877fc Compare November 22, 2024 21:06

jackfrancis approved these changes Nov 22, 2024

View reviewed changes

k8s-ci-robot assigned jackfrancis Nov 22, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 22, 2024

pohly reviewed Nov 23, 2024

View reviewed changes

test/e2e/dra/dra.go Outdated Show resolved Hide resolved

nojnhuh force-pushed the dra-e2e-quota-forbid branch from 23877fc to c6cfee8 Compare November 24, 2024 17:32

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 24, 2024

k8s-ci-robot requested a review from jackfrancis November 24, 2024 17:32

pohly reviewed Nov 25, 2024

View reviewed changes

e2e: wrap DRA ResourceQuota Forbidden claim check in gomega.Eventually

05ebf45

nojnhuh force-pushed the dra-e2e-quota-forbid branch from c6cfee8 to 05ebf45 Compare November 25, 2024 18:33

pohly approved these changes Nov 26, 2024

View reviewed changes

k8s-ci-robot assigned pohly Nov 26, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 26, 2024

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 26, 2024

k8s-ci-robot merged commit 95d4813 into kubernetes:master Dec 12, 2024
22 checks passed

k8s-ci-robot added this to the v1.33 milestone Dec 12, 2024

nojnhuh deleted the dra-e2e-quota-forbid branch December 12, 2024 05:29

tallclair mentioned this pull request Mar 24, 2025

Failure cluster [c199077e...]: Pod InPlace Resize Container pod-resize-resource-quota-test #128840

Closed

	gomega.Eventually(ctx, func() error {
	gomega.Eventually(ctx, func(ctx context.Context) error {

e2e: wrap DRA ResourceQuota Forbidden claim check in gomega.Eventually #128943

e2e: wrap DRA ResourceQuota Forbidden claim check in gomega.Eventually #128943

Uh oh!

Conversation

nojnhuh commented Nov 22, 2024

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

Uh oh!

k8s-ci-robot commented Nov 22, 2024

Uh oh!

nojnhuh commented Nov 22, 2024

Uh oh!

jackfrancis commented Nov 22, 2024

Uh oh!

Uh oh!

jackfrancis left a comment

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Nov 22, 2024

Uh oh!

Uh oh!

pohly commented Nov 25, 2024

Uh oh!

pohly Nov 25, 2024

Choose a reason for hiding this comment

Uh oh!

jackfrancis Nov 25, 2024

Choose a reason for hiding this comment

Uh oh!

pohly Nov 25, 2024

Choose a reason for hiding this comment

Uh oh!

jackfrancis Nov 25, 2024

Choose a reason for hiding this comment

Uh oh!

nojnhuh Nov 25, 2024

Choose a reason for hiding this comment

Uh oh!

pohly left a comment

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Nov 26, 2024

Uh oh!

k8s-ci-robot commented Nov 26, 2024

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants