Update tolerated flavors integration test #179

denkensk · 2022-04-06T08:34:23Z

Signed-off-by: Alex Wang wangqingcan1990@gmail.com

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

alculquicondor · 2022-04-06T14:40:22Z

Add to description that this is now possible after #135, with it being the default.

alculquicondor · 2022-04-06T14:42:04Z

Also, should we have a similar test where we ensure that a smaller later workload is not scheduled when using StrictFIFO.
Although it might be flaky before #164

alculquicondor · 2022-04-06T14:44:17Z

test/integration/scheduler/scheduler_test.go

@@ -189,7 +190,7 @@ var _ = ginkgo.Describe("Scheduler", func() {

 	ginkgo.It("Should schedule jobs on tolerated flavors", func() {


specify the queueing strategy

alculquicondor · 2022-04-06T18:16:53Z

test/integration/scheduler/scheduler_test.go

@@ -103,6 +103,7 @@ var _ = ginkgo.Describe("Scheduler", func() {
 		prodBEClusterQ = testing.MakeClusterQueue("prod-be-cq").


MakeClusterQueue uses StrictFIFO by default. We should make it match the API default.

+1, and we should have a test for StrictFIFO testing that a workload continues to be blocked if another fronting it is inadmissible.

Also, should we have a similar test where we ensure that a smaller later workload is not scheduled when using StrictFIFO. Although it might be flaky before #164

why will it be flaky?

I was thinking that we could end up scheduling the second workload while we are asynchronously doing the API update.
However, since we assume the first (blocking) workload, I guess that shouldn't be possible.

MakeClusterQueue uses StrictFIFO by default. We should make it match the API default.

Added.

ahg-g · 2022-04-07T11:51:16Z

test/integration/scheduler/scheduler_test.go

+		ginkgo.By("checking a second job without toleration doesn't start")
+		job2 := testing.MakeJob("on-demand-job2", ns.Name).Queue(prodBEQueue.Name).Request(corev1.ResourceCPU, "5").Obj()
+		gomega.Expect(k8sClient.Create(ctx, job2)).Should(gomega.Succeed())
+		createdJob2 := &batchv1.Job{}


you can put this inside the Consistently clause since it is not used later.

You could use QueuedWorkload (instead of Job) and use this

kueue/test/integration/framework/framework.go

Lines 151 to 168 in 425187b

func ExpectWorkloadsToBePending(ctx context.Context, k8sClient client.Client, wls ...*kueue.QueuedWorkload) {

gomega.EventuallyWithOffset(1, func() int {

pending := 0

var updatedWorkload kueue.QueuedWorkload

for _, wl := range wls {

gomega.ExpectWithOffset(1, k8sClient.Get(ctx, client.ObjectKeyFromObject(wl), &updatedWorkload)).To(gomega.Succeed())

idx := workload.FindConditionIndex(&updatedWorkload.Status, kueue.QueuedWorkloadAdmitted)

if idx == -1 {

continue

}

cond := updatedWorkload.Status.Conditions[idx]

if cond.Status == corev1.ConditionFalse && cond.Reason == "Pending" && wl.Spec.Admission == nil {

pending++

}

}

return pending

}).Should(gomega.Equal(len(wls)), "Not enough workloads are pending")

}

The advantage is that it uses Eventually instead of Consistently, using the QW status.

Can we please move forward with this PR? I think we can keep this test as is without changing to workload, we can do the migration for the whole file separately.

Also change to use Workload (instead of Job) in this test case and use ExpectWorkloadsToBePending here.

we can do the migration for the whole file separately.

+1 We can use workload in new added test case. And migration the old tests for the whole file in a separate pr.

test/integration/scheduler/scheduler_test.go

alculquicondor · 2022-05-03T13:29:49Z

test/integration/scheduler/scheduler_test.go

+		wlCopy := &kueue.Workload{}
+		gomega.Eventually(func() bool {
+			gomega.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(wl1), wlCopy)).To(gomega.Succeed())
+			return wlCopy.Spec.Admission != nil


Suggested change

return wlCopy.Spec.Admission != nil

return wlCopy.Spec.Admission

Hi Aldo Did you suggest to change it like this?

expectAdmission := testing.MakeAdmission(prodClusterQ.Name).Flavor(corev1.ResourceCPU,onDemandFlavor.Name).Obj() gomega.Eventually(func() kueue.Admission { gomega.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(wl1), wlCopy)).To(gomega.Succeed()) return wlCopy.Spec.Admission or *wlCopy.Spec.Admission }, framework.Timeout, framework.Interval).Should(gomega.Equal(expectAdmission))

wlCopy.Spec.Admission is a point of kueue.Admission. We still need to check if it's nil before getting value of it. And we'd better not continuity to check the value of Admission, we just need to check it when the first time it's not nil.

Oops. My recommendation is to return *kueue.Admission.

gomega.Equal and pkg/util/testing.Equal should be able to handle it.

alculquicondor · 2022-05-03T13:30:00Z

test/integration/scheduler/scheduler_test.go

-		}, framework.Timeout, framework.Interval).Should(gomega.Equal(pointer.Bool(false)))
-		gomega.Expect(createdJob3.Spec.Template.Spec.NodeSelector[instanceKey]).Should(gomega.Equal(spotTaintedFlavor.Name))
+		wlCopy := &kueue.Workload{}
+		gomega.Eventually(func() bool {


Suggested change

gomega.Eventually(func() bool {

gomega.Eventually(func() kueue.Admission {

alculquicondor · 2022-05-03T13:30:27Z

test/integration/scheduler/scheduler_test.go

+		wl3 := testing.MakeWorkload("on-demand-wl3", ns.Name).Queue(prodQueue.Name).Toleration(spotToleration).Request(corev1.ResourceCPU, "5").Obj()
+		gomega.Expect(k8sClient.Create(ctx, wl3)).Should(gomega.Succeed())
+
+		gomega.Eventually(func() bool {


alculquicondor · 2022-05-06T15:55:55Z

test/integration/scheduler/scheduler_test.go

+		gomega.Eventually(func() *kueue.Admission {
+			gomega.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(wl1), wlCopy)).To(gomega.Succeed())
+			return wlCopy.Spec.Admission
+		}, framework.Timeout, framework.Interval).Should(gomega.Equal(expectAdmission))


Suggested change

}, framework.Timeout, framework.Interval).Should(gomega.Equal(expectAdmission))

}, framework.Timeout, framework.Interval).Should(testing.Equal(expectAdmission))

or testingutil, depending on the alias. That reports using cmp.Diff

Thanks.
Updated.

Signed-off-by: Alex Wang <wangqingcan1990@gmail.com>

alculquicondor

Great!

/approve
/lgtm

k8s-ci-robot · 2022-05-06T16:10:35Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alculquicondor, denkensk

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:

~~OWNERS~~ [alculquicondor]

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 kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Apr 6, 2022

k8s-ci-robot requested review from alculquicondor and ArangoGutierrez April 6, 2022 08:34

k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Apr 6, 2022

alculquicondor reviewed Apr 6, 2022

View reviewed changes

ahg-g reviewed Apr 7, 2022

View reviewed changes

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

denkensk mentioned this pull request Apr 21, 2022

Change make cluster queue in wrappers to bestfifo #227

Merged

denkensk force-pushed the intest branch from 2d50fd5 to 732aac4 Compare May 1, 2022 03:47

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels May 1, 2022

alculquicondor reviewed May 2, 2022

View reviewed changes

test/integration/scheduler/scheduler_test.go Outdated Show resolved Hide resolved

test/integration/scheduler/scheduler_test.go Outdated Show resolved Hide resolved

test/integration/scheduler/scheduler_test.go Show resolved Hide resolved

denkensk force-pushed the intest branch from 732aac4 to 8cb0e0d Compare May 3, 2022 00:51

denkensk requested review from ahg-g and alculquicondor May 3, 2022 00:54

alculquicondor reviewed May 3, 2022

View reviewed changes

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

denkensk force-pushed the intest branch from 8cb0e0d to 7b898a2 Compare May 5, 2022 13:08

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

denkensk requested a review from alculquicondor May 6, 2022 13:59

alculquicondor reviewed May 6, 2022

View reviewed changes

update tolerated flavors integration test

98f1b7e

Signed-off-by: Alex Wang <wangqingcan1990@gmail.com>

denkensk force-pushed the intest branch from 7b898a2 to 98f1b7e Compare May 6, 2022 16:05

alculquicondor reviewed May 6, 2022

View reviewed changes

k8s-ci-robot assigned alculquicondor May 6, 2022

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

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

k8s-ci-robot merged commit f2d362d into kubernetes-sigs:main May 6, 2022

denkensk deleted the intest branch May 7, 2022 00:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update tolerated flavors integration test #179

Update tolerated flavors integration test #179

denkensk commented Apr 6, 2022

alculquicondor commented Apr 6, 2022

alculquicondor commented Apr 6, 2022

alculquicondor Apr 6, 2022

alculquicondor Apr 6, 2022

ahg-g Apr 7, 2022

ahg-g Apr 7, 2022

alculquicondor Apr 7, 2022

denkensk May 1, 2022

ahg-g Apr 7, 2022

alculquicondor Apr 7, 2022 •

edited

Loading

ahg-g Apr 19, 2022

denkensk May 1, 2022

alculquicondor May 3, 2022

denkensk May 4, 2022

alculquicondor May 4, 2022

alculquicondor May 3, 2022

alculquicondor May 3, 2022

alculquicondor May 6, 2022

denkensk May 6, 2022

alculquicondor left a comment

k8s-ci-robot commented May 6, 2022

		@@ -189,7 +190,7 @@ var _ = ginkgo.Describe("Scheduler", func() {

		ginkgo.It("Should schedule jobs on tolerated flavors", func() {

		@@ -103,6 +103,7 @@ var _ = ginkgo.Describe("Scheduler", func() {
		prodBEClusterQ = testing.MakeClusterQueue("prod-be-cq").

	func ExpectWorkloadsToBePending(ctx context.Context, k8sClient client.Client, wls ...*kueue.QueuedWorkload) {
	gomega.EventuallyWithOffset(1, func() int {
	pending := 0
	var updatedWorkload kueue.QueuedWorkload
	for _, wl := range wls {
	gomega.ExpectWithOffset(1, k8sClient.Get(ctx, client.ObjectKeyFromObject(wl), &updatedWorkload)).To(gomega.Succeed())
	idx := workload.FindConditionIndex(&updatedWorkload.Status, kueue.QueuedWorkloadAdmitted)
	if idx == -1 {
	continue
	}
	cond := updatedWorkload.Status.Conditions[idx]
	if cond.Status == corev1.ConditionFalse && cond.Reason == "Pending" && wl.Spec.Admission == nil {
	pending++
	}
	}
	return pending
	}).Should(gomega.Equal(len(wls)), "Not enough workloads are pending")
	}

	return wlCopy.Spec.Admission != nil
	return wlCopy.Spec.Admission

	gomega.Eventually(func() bool {
	gomega.Eventually(func() kueue.Admission {

	}, framework.Timeout, framework.Interval).Should(gomega.Equal(expectAdmission))
	}, framework.Timeout, framework.Interval).Should(testing.Equal(expectAdmission))

Update tolerated flavors integration test #179

Update tolerated flavors integration test #179

Conversation

denkensk commented Apr 6, 2022

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:

alculquicondor commented Apr 6, 2022

alculquicondor commented Apr 6, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alculquicondor Apr 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alculquicondor left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented May 6, 2022

alculquicondor Apr 7, 2022 •

edited

Loading