Bug 1731263: Re-enable preemption test with fixes by damemi · Pull Request #23645 · openshift/origin

damemi · 2019-08-21T17:01:06Z

Rebased changes from #23050

openshift-ci-robot · 2019-08-21T17:01:10Z

@damemi: This pull request references Bugzilla bug 1731263, which is valid. The bug has been updated to refer to the pull request using the external bug tracker.

Details

In response to this:

Bug 1731263: Re-enable preemption test with fixes

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.

soltysh · 2019-08-22T19:27:12Z

/retest
/assign

soltysh · 2019-08-23T09:45:46Z

-			milliCPU = milliCPU * 10 / 100
-			memAllocatable, found := currentNode.Status.Allocatable["memory"]
+			// Just to be tolerant use 0.6 of resources available on the node
+			milliCPU = int64(float64(milliCPU-currentMemUsage) * float64(0.6))


@damemi it looks like this ends up being a negative number in the tests: Pod \"pod0-sched-preemption-medium-priority\" is invalid: spec.containers[0].resources.requests[cpu]: Invalid value: \"-912259020m\": must be greater than or equal to 0 maybe you'd need similar checks for utilization like in the other test?

damemi · 2019-08-26T19:46:53Z

/retest

damemi · 2019-08-27T13:07:54Z

/retest

damemi · 2019-08-27T16:21:38Z

I think checking cpu is eliminating all 3 nodes... so maybe an error in the calculation there

damemi · 2019-08-28T17:03:32Z

/retest

damemi · 2019-08-28T20:29:51Z

@soltysh this is green now, think we can merge it?

soltysh

/lgtm
/approve

openshift-ci-robot · 2019-08-29T11:01:22Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: damemi, soltysh

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:

~~OWNERS~~ [soltysh]

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

…#23645) This fixes a bug introduced in openshift#23645 by initializing the pod array with a length of 0 and a capacity of 4

damemi · 2019-09-04T21:25:23Z

+				continue
+			}
 			podRes[v1.ResourceCPU] = *resource.NewMilliQuantity(int64(milliCPU), resource.DecimalSI)
-			podRes[v1.ResourceMemory] = *resource.NewQuantity(int64(memory), resource.BinarySI)


I'm noticing this now and it seems weird, @ravisantoshgudimetla do you know why we wouldn't want to set the resourceMemory, especially after we calculated it above? Or could this have been an error?

damemi · 2019-09-04T22:09:19Z

+				continue
+			}
 			podRes[v1.ResourceCPU] = *resource.NewMilliQuantity(int64(milliCPU), resource.DecimalSI)
-			podRes[v1.ResourceMemory] = *resource.NewQuantity(int64(memory), resource.BinarySI)


@ravisantoshgudimetla same question here

Bug 1748150: Modify scheduler preemption tests (fixup #23645)

…s#23645) This fixes a bug introduced in openshift/origin#23645 by initializing the pod array with a length of 0 and a capacity of 4 Origin-commit: 5690efe3eeea7a54f5a30ae49cc37b0083fddcb5

sttts · 2019-09-10T10:36:32Z

@@ -90,45 +90,67 @@ var _ = SIGDescribe("SchedulerPreemption [Serial]", func() {
 		var podRes v1.ResourceList


can we please start using sensible commit messages? This one says nothing.

This test was marked as flaky but has had a number of fixes applied to it (pr openshift#23645 and openshift#23719) tht have adjusted the node utilization calculations and structure of the test to hopefully make it more stable. In order to address the comments in the BZ for this test's flakiness (https://bugzilla.redhat.com/show_bug.cgi?id=1731263#c9) we should remove the [flaky] tag to re-enable the tests.

…-enable it This test was marked as flaky but has had a number of fixes applied to it (pr openshift#23645 and openshift#23719) tht have adjusted the node utilization calculations and structure of the test to hopefully make it more stable. In order to address the comments in the BZ for this test's flakiness (https://bugzilla.redhat.com/show_bug.cgi?id=1731263#c9) we should remove the [flaky] tag to re-enable the tests.

openshift-ci-robot added the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label Aug 21, 2019

openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 21, 2019

openshift-ci-robot requested review from bparees and soltysh August 21, 2019 17:01

damemi mentioned this pull request Aug 21, 2019

Bug 1731263: Re-enable preemption test with fixes #23050

Closed

openshift-ci-robot assigned soltysh Aug 22, 2019

soltysh suggested changes Aug 23, 2019

View reviewed changes

damemi force-pushed the ravig-fix-tests branch from 0cd7a5c to a2dab5d Compare August 26, 2019 17:08

UPSTREAM: <carry>: Modify scheduler preemption tests

95ecd27

damemi force-pushed the ravig-fix-tests branch from a2dab5d to 95ecd27 Compare August 28, 2019 14:24

soltysh approved these changes Aug 29, 2019

View reviewed changes

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 29, 2019

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 29, 2019

openshift-merge-robot merged commit b2f2313 into openshift:master Aug 29, 2019

damemi added a commit to damemi/origin that referenced this pull request Sep 3, 2019

UPSTREAM: <carry>: Modify scheduler preemption tests (fixup openshift…

c5d9998

…#23645) This fixes a bug introduced in openshift#23645 by initializing the pod array with a length of 0 and a capacity of 4

damemi mentioned this pull request Sep 3, 2019

Bug 1748150: Modify scheduler preemption tests (fixup #23645) #23719

Merged

damemi added a commit to damemi/origin that referenced this pull request Sep 3, 2019

UPSTREAM: <carry>: Modify scheduler preemption tests (fixup openshift…

aa5068f

…#23645) This fixes a bug introduced in openshift#23645 by initializing the pod array with a length of 0 and a capacity of 4

damemi added a commit to damemi/origin that referenced this pull request Sep 4, 2019

UPSTREAM: <carry>: Modify scheduler preemption tests (fixup openshift…

5690efe

…#23645) This fixes a bug introduced in openshift#23645 by initializing the pod array with a length of 0 and a capacity of 4

damemi commented Sep 4, 2019

View reviewed changes

damemi mentioned this pull request Sep 4, 2019

Fix preemption race conditions on heavily utilized nodes for e2e tests kubernetes/kubernetes#82350

Merged

openshift-merge-robot added a commit that referenced this pull request Sep 5, 2019

Merge pull request #23719 from damemi/fixup-23645

ab0a4e4

Bug 1748150: Modify scheduler preemption tests (fixup #23645)

sttts reviewed Sep 10, 2019

View reviewed changes

damemi mentioned this pull request Sep 10, 2019

Bug 1731263: Reenable basic pod preemption e2es #23758

Closed

damemi mentioned this pull request Apr 13, 2020

Bug 1731263: Disable failing preemption e2es #24868

Merged

		@@ -90,45 +90,67 @@ var _ = SIGDescribe("SchedulerPreemption [Serial]", func() {
		var podRes v1.ResourceList

Conversation

damemi commented Aug 21, 2019

Uh oh!

openshift-ci-robot commented Aug 21, 2019

Uh oh!

soltysh commented Aug 22, 2019

Uh oh!

soltysh Aug 23, 2019

Choose a reason for hiding this comment

Uh oh!

damemi commented Aug 26, 2019

Uh oh!

damemi commented Aug 27, 2019

Uh oh!

damemi commented Aug 27, 2019

Uh oh!

damemi commented Aug 28, 2019

Uh oh!

damemi commented Aug 28, 2019

Uh oh!

soltysh left a comment

Choose a reason for hiding this comment

Uh oh!

openshift-ci-robot commented Aug 29, 2019

Uh oh!

damemi Sep 4, 2019

Choose a reason for hiding this comment

Uh oh!

damemi Sep 4, 2019

Choose a reason for hiding this comment

Uh oh!

sttts Sep 10, 2019

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants