Skip to content
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.

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

Move PodPriorityResolution e2e to integration #80824

Merged

Conversation

@damemi
Copy link
Contributor

commented Jul 31, 2019

What type of PR is this?
/kind cleanup

What this PR does / why we need it:
Following #80821, this moves the e2e test checking pod priority resolution to integration to free up e2e resources

NONE

/sig scheduling

@damemi

This comment has been minimized.

Copy link
Contributor Author

commented Jul 31, 2019

@damemi damemi force-pushed the damemi:preemption-e2e-to-integration branch 2 times, most recently from 1c31c2e to 35a372a Aug 1, 2019
@alculquicondor

This comment has been minimized.

Copy link
Member

commented Aug 2, 2019

It looks like this is flaky?

/hold

@damemi

This comment has been minimized.

Copy link
Contributor Author

commented Aug 5, 2019

/retest

@alculquicondor

This comment has been minimized.

Copy link
Member

commented Aug 6, 2019

Even if the test passes after retrying, I would be opposed to merge this. We shouldn't be introducing a test that is flaky.

@damemi

This comment has been minimized.

Copy link
Contributor Author

commented Aug 6, 2019

@alculquicondor yeah, I just wanted to see if I could get a better idea of the flake after retrying. Do you have any suggestions to improve it? Since this is currently an e2e test, I'm not sure where the flakiness is introduced in moving it. We would like to move some of these e2e's to integration in this manner, so if I can learn how that should be done without introducing flakes it would be very helpful

@alculquicondor

This comment has been minimized.

Copy link
Member

commented Aug 6, 2019

Looking at other tests, it looks like you have to create a node before running the pod.
You should be able to run integration tests locally fairly easy. Add src/k8s.io/kubernetes/third_party/etcd to your PATH

@damemi damemi force-pushed the damemi:preemption-e2e-to-integration branch from 35a372a to a21098a Aug 7, 2019
@k8s-ci-robot k8s-ci-robot added size/L and removed size/M labels Aug 7, 2019
@damemi damemi force-pushed the damemi:preemption-e2e-to-integration branch from a21098a to 4fafd76 Aug 7, 2019
@damemi damemi force-pushed the damemi:preemption-e2e-to-integration branch from 4fafd76 to 7830622 Aug 26, 2019
@damemi damemi force-pushed the damemi:preemption-e2e-to-integration branch from 7830622 to 9e8b58d Aug 26, 2019
@k8s-ci-robot k8s-ci-robot requested a review from bsalamat Aug 29, 2019
@damemi damemi force-pushed the damemi:preemption-e2e-to-integration branch from 30a077a to af44f08 Aug 29, 2019
@damemi

This comment has been minimized.

Copy link
Contributor Author

commented Aug 29, 2019

/test pull-kubernetes-conformance-kind-ipv6

@damemi

This comment has been minimized.

Copy link
Contributor Author

commented Aug 30, 2019

/retest

@damemi damemi force-pushed the damemi:preemption-e2e-to-integration branch from af44f08 to c6bacc7 Sep 11, 2019
@@ -221,43 +221,6 @@ var _ = SIGDescribe("SchedulerPreemption [Serial]", func() {
})
})

var _ = SIGDescribe("PodPriorityResolution [Serial]", func() {

This comment has been minimized.

Copy link
@Huang-Wei

Huang-Wei Sep 12, 2019

Member

May I know why you want to remove this e2e test? Due to portability?

This comment has been minimized.

Copy link
@damemi

damemi Sep 12, 2019

Author Contributor

The goal was to try to lower e2e load, for example in OpenShift these tests get flaky with a lot of other resources running so @ravisantoshgudimetla wanted to move as many e2e to integration as we could (with a few exceptions)

@damemi

This comment has been minimized.

Copy link
Contributor Author

commented Sep 12, 2019

/retest

@damemi

This comment has been minimized.

Copy link
Contributor Author

commented Sep 18, 2019

Bumping, this is green in CI so wondering if there is any more feedback to it?

test/integration/scheduler/preemption_test.go Outdated Show resolved Hide resolved
test/e2e/scheduling/preemption.go Show resolved Hide resolved
test/integration/scheduler/preemption_test.go Outdated Show resolved Hide resolved
Namespace: metav1.NamespaceSystem,
PriorityClassName: scheduling.SystemClusterCritical,
}),
},

This comment has been minimized.

Copy link
@alculquicondor

alculquicondor Sep 18, 2019

Member

Add a negative case? one for an unknown priority, and expect the scheduler to fail?

This comment has been minimized.

Copy link
@damemi

damemi Sep 19, 2019

Author Contributor

Similar to my above reply, we're not setting integer values, we are setting a string and checking that it resolves to an integer, so I can't add a negative value check but I can add one for an unknown string and check that it resolves to nothing if you want. What do you think?

This comment has been minimized.

Copy link
@alculquicondor

alculquicondor Sep 19, 2019

Member

Sorry, I didn't mean a negative number, just a negative case, i.e. a case that should be rejected.

This comment has been minimized.

Copy link
@damemi

damemi Sep 19, 2019

Author Contributor

Ah, I see then I misunderstood. I added a case for an invalid priority that should be rejected

@damemi damemi force-pushed the damemi:preemption-e2e-to-integration branch 5 times, most recently from 821a39a to 118547b Sep 19, 2019
@damemi damemi force-pushed the damemi:preemption-e2e-to-integration branch from 118547b to ca18b48 Sep 20, 2019
@damemi

This comment has been minimized.

Copy link
Contributor Author

commented Sep 20, 2019

/retest

@damemi

This comment has been minimized.

Copy link
Contributor Author

commented Sep 20, 2019

@alculquicondor updated with your feedback, please let me know if there are any other changes you'd like

@alculquicondor

This comment has been minimized.

Copy link
Member

commented Sep 20, 2019

/lgtm

@damemi

This comment has been minimized.

Copy link
Contributor Author

commented Sep 20, 2019

/hold cancel

Copy link
Contributor

left a comment

/approve

Thanks for the PR @damemi

@deads2k

This comment has been minimized.

Copy link
Contributor

commented Sep 20, 2019

/approve

the admission update for integration test

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Sep 20, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: damemi, deads2k, ravisantoshgudimetla

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:

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 merged commit c7619bd into kubernetes:master Sep 20, 2019
25 checks passed
25 checks passed
cla/linuxfoundation damemi authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-image-test Skipped.
pull-kubernetes-conformance-kind-ipv6 Job succeeded.
Details
pull-kubernetes-cross Skipped.
pull-kubernetes-dependencies Job succeeded.
Details
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-alpha-features Skipped.
pull-kubernetes-e2e-gce-csi-serial Skipped.
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gce-iscsi Skipped.
pull-kubernetes-e2e-gce-iscsi-serial Skipped.
pull-kubernetes-e2e-gce-storage-slow Skipped.
pull-kubernetes-godeps Skipped.
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped.
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-node-e2e-containerd Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Skipped.
tide In merge pool.
Details
@k8s-ci-robot k8s-ci-robot added this to the v1.17 milestone Sep 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.