Fix HPA E2E CustomResourceDefinition test #112335

piotrnosek · 2022-09-08T22:29:46Z

What type of PR is this?

Fix for a failing e2e test

What this PR does / why we need it:

Major fix for running HPA E2E CustomResourceDefinition test. This specifically adds all necessary utilities around creating a CRD, creating an instance and making sure HPA is able to fetch number of Replicas from CRD's status via scale client.

Which issue(s) this PR fixes:

Fixes bug(s) introduced by #111865

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 · 2022-09-08T22:29:54Z

@piotrnosek: 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.

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/test-infra repository.

k8s-ci-robot · 2022-09-08T22:29:54Z

Hi @piotrnosek. 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/test-infra repository.

piotrnosek · 2022-09-08T22:35:57Z

/kind failing-test

piotrnosek · 2022-09-08T22:51:01Z

/assign liggitt
Jordan, would you find time to look at this? This touches some files outside of autoscaling OWNERS power.

piotrnosek · 2022-09-08T22:51:13Z

/assign mwielgus
/assign pbetkier

test/utils/delete_resources.go

test/e2e/framework/resource/runtimeobj.go

test/e2e/framework/resource/resources.go

test/e2e/framework/autoscaling/autoscaling_utils.go

piotrnosek

Thanks for all the comments - all should be resolved now.

test/e2e/framework/autoscaling/autoscaling_utils.go

test/utils/delete_resources.go

test/e2e/framework/resource/runtimeobj.go

test/e2e/framework/resource/resources.go

test/e2e/framework/autoscaling/autoscaling_utils.go

piotrnosek · 2022-09-15T14:52:37Z

did part of this merge in another PR? looks like it needs a rebase

I've just rebased the PR and resolved conflicts - some stuff in autoscaling_utils.go were refactored in another PR (#112252).

piotrnosek · 2022-09-15T15:17:49Z

@liggitt - many thanks for the comments. I've fixed some of those, but I'm not sure I understand what you mean by some others - can you elaborate on that a bit more?

piotrnosek · 2022-09-19T08:41:05Z

/retest-required

piotrnosek · 2022-09-20T12:27:44Z

/retest-required

liggitt · 2022-09-20T18:39:21Z

(drop the spurious files in the repo root in this PR)

liggitt

I pushed a commit up to https://github.com/liggitt/kubernetes/commits/fixcustomcrd on top of this PR's commit with an example of what I'm envisioning, in case that's helpful. The diff against this PR looks messy, but it's actually getting closer to the current state of master and adding a distinct parallel path for handling custom resources

test/e2e/framework/resource/resources.go

liggitt · 2022-09-20T18:41:42Z

test/e2e/framework/autoscaling/autoscaling_utils.go

+		gvr := schema.GroupVersionResource{Group: crdGroup, Version: crdVersion, Resource: crdNamePlural}
+		framework.ExpectNoError(e2eresource.DeleteResourceAndWaitForGCWithDynamicClient(rc.clientSet, rc.dynamicClient, rc.scaleClient, true, gvr, kind, rc.nsName, rc.name))
+		// Delete CRD kind.
+		err := rc.apiExtensionClient.ApiextensionsV1().CustomResourceDefinitions().Delete(context.TODO(), crdNamePlural+"."+crdGroup, metav1.DeleteOptions{})


this is deleting the entire CRD (the whole type), not just the custom resource instance ... doesn't that mean any test using this must be serial / cannot run concurrently with other tests using this CRD?

also, this delete request just sets deletionTimestamp on the CRD, it doesn't synchronously wait for the CRD and CR instances to be cleaned up

True, though we don't have any other tests using this CRD (and I'm not aware of any plans).

Right.
So maybe it's best to not delete the CRD (the whole type)? I believe it won't hurt us to leave it to just be deleted with the whole cluster deletion?

I decided to not delete the CRD type. Let me know if you think it's okay or we should have some solution which deletes CRD while still allowing to run concurrently / wait sync.

And regarding this comment:

I noted in the cleanup the bit that will disrupt other concurrent tests using the same CRD

Creating CRD should work fine - we have checks that if CRD already exists, we do not create it.

I decided to not delete the CRD type. Let me know if you think it's okay or we should have some solution which deletes CRD while still allowing to run concurrently / wait sync.

that's probably ok, at least for now

(the normal way to be non-disruptive is to generate a CRD with a unique name per test)

test/e2e/framework/autoscaling/autoscaling_utils.go

test/utils/delete_resources.go

test/e2e/framework/resource/resources.go

piotrnosek · 2022-09-20T21:51:34Z

Many thanks for all the comments and the example commit. I've refactored the code to include your proposed changes.

test/e2e/framework/autoscaling/autoscaling_utils.go

piotrnosek · 2022-09-22T06:53:04Z

/retest-required

liggitt · 2022-09-22T12:30:30Z

/lgtm
/approve

k8s-ci-robot · 2022-09-22T12:30:59Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, piotrnosek

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:

~~pkg/controller/podautoscaler/OWNERS~~ [liggitt]
~~test/OWNERS~~ [liggitt]

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

k8s-triage-robot · 2022-09-22T15:48:01Z

The Kubernetes project has merge-blocking tests that are currently too flaky to consistently pass.

This bot retests PRs for certain kubernetes repos according to the following rules:

The PR does have any do-not-merge/* labels
The PR does not have the needs-ok-to-test label
The PR is mergeable (does not have a needs-rebase label)
The PR is approved (has cncf-cla: yes, lgtm, approved labels)
The PR is failing tests required for merge

You can:

Review the full test history for this PR
Prevent this bot from retesting with /lgtm cancel or /hold
Help make our tests less flaky by following our Flaky Tests Guide

/retest

k8s-ci-robot requested review from bskiba and fejta September 8, 2022 22:29

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Sep 8, 2022

k8s-ci-robot added kind/failing-test Categorizes issue or PR as related to a consistently or frequently failing test. and removed do-not-merge/needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels Sep 8, 2022

piotrnosek force-pushed the fixcustomcrd branch from f001026 to 252f7f8 Compare September 8, 2022 22:50

k8s-ci-robot assigned liggitt Sep 8, 2022

k8s-ci-robot assigned mwielgus and pbetkier Sep 8, 2022

liggitt reviewed Sep 9, 2022

View reviewed changes

piotrnosek commented Sep 12, 2022

View reviewed changes

piotrnosek force-pushed the fixcustomcrd branch from 252f7f8 to 185cb08 Compare September 12, 2022 01:03

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 15, 2022

piotrnosek force-pushed the fixcustomcrd branch from 1315f67 to 1897b5c Compare September 15, 2022 15:15

piotrnosek requested a review from liggitt September 15, 2022 15:17

piotrnosek force-pushed the fixcustomcrd branch 3 times, most recently from b31c5b7 to 4e7a69f Compare September 18, 2022 22:58

liggitt reviewed Sep 20, 2022

View reviewed changes

piotrnosek force-pushed the fixcustomcrd branch from 4e7a69f to 65ade53 Compare September 20, 2022 21:50

piotrnosek requested a review from liggitt September 20, 2022 21:51

This comment was marked as duplicate.

Sign in to view

liggitt reviewed Sep 21, 2022

View reviewed changes

test/e2e/framework/autoscaling/autoscaling_utils.go Outdated Show resolved Hide resolved

liggitt reviewed Sep 21, 2022

View reviewed changes

test/e2e/framework/autoscaling/autoscaling_utils.go Outdated Show resolved Hide resolved

piotrnosek force-pushed the fixcustomcrd branch from 65ade53 to 1f19920 Compare September 21, 2022 21:41

piotrnosek requested a review from liggitt September 21, 2022 21:42

Fix HPA E2E CRD test

96ff1b1

piotrnosek force-pushed the fixcustomcrd branch from 1f19920 to 96ff1b1 Compare September 21, 2022 22:40

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

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 22, 2022

k8s-ci-robot merged commit 3a0dbe5 into kubernetes:master Sep 22, 2022

k8s-ci-robot added this to the v1.26 milestone Sep 22, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix HPA E2E CustomResourceDefinition test #112335

Fix HPA E2E CustomResourceDefinition test #112335

piotrnosek commented Sep 8, 2022 •

edited

k8s-ci-robot commented Sep 8, 2022

k8s-ci-robot commented Sep 8, 2022

piotrnosek commented Sep 8, 2022

piotrnosek commented Sep 8, 2022

piotrnosek commented Sep 8, 2022

piotrnosek left a comment •

edited

piotrnosek commented Sep 15, 2022

piotrnosek commented Sep 15, 2022

piotrnosek commented Sep 19, 2022

piotrnosek commented Sep 20, 2022

liggitt commented Sep 20, 2022

liggitt left a comment •

edited

liggitt Sep 20, 2022

liggitt Sep 20, 2022

piotrnosek Sep 20, 2022

piotrnosek Sep 20, 2022

piotrnosek Sep 20, 2022

liggitt Sep 21, 2022

liggitt Sep 21, 2022

piotrnosek commented Sep 20, 2022

This comment was marked as duplicate.

piotrnosek commented Sep 22, 2022

liggitt commented Sep 22, 2022

k8s-ci-robot commented Sep 22, 2022

k8s-triage-robot commented Sep 22, 2022

Fix HPA E2E CustomResourceDefinition test #112335

Fix HPA E2E CustomResourceDefinition test #112335

Conversation

piotrnosek commented Sep 8, 2022 • edited

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.:

k8s-ci-robot commented Sep 8, 2022

k8s-ci-robot commented Sep 8, 2022

piotrnosek commented Sep 8, 2022

piotrnosek commented Sep 8, 2022

piotrnosek commented Sep 8, 2022

piotrnosek left a comment • edited

Choose a reason for hiding this comment

piotrnosek commented Sep 15, 2022

piotrnosek commented Sep 15, 2022

piotrnosek commented Sep 19, 2022

piotrnosek commented Sep 20, 2022

liggitt commented Sep 20, 2022

liggitt left a comment • edited

Choose a reason for hiding this comment

liggitt Sep 20, 2022

Choose a reason for hiding this comment

liggitt Sep 20, 2022

Choose a reason for hiding this comment

piotrnosek Sep 20, 2022

Choose a reason for hiding this comment

piotrnosek Sep 20, 2022

Choose a reason for hiding this comment

piotrnosek Sep 20, 2022

Choose a reason for hiding this comment

liggitt Sep 21, 2022

Choose a reason for hiding this comment

liggitt Sep 21, 2022

Choose a reason for hiding this comment

piotrnosek commented Sep 20, 2022

This comment was marked as duplicate.

piotrnosek commented Sep 22, 2022

liggitt commented Sep 22, 2022

k8s-ci-robot commented Sep 22, 2022

k8s-triage-robot commented Sep 22, 2022

piotrnosek commented Sep 8, 2022 •

edited

piotrnosek left a comment •

edited

liggitt left a comment •

edited