Add testcase to validate the reason of `FitError` #93770

chendave · 2020-08-07T08:18:20Z

The error message should be rolled up into one line summary.

Signed-off-by: Dave Chen dave.chen@arm.com

What type of PR is this?

Add one of the following kinds:

/kind cleanup

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #

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

chendave · 2020-08-07T08:18:44Z

/sig scheduling

alculquicondor

Thanks

alculquicondor · 2020-08-07T13:49:42Z

pkg/scheduler/core/generic_scheduler_test.go

+func TestFitErrorNoSpammy(t *testing.T) {
+	tests := []struct {
+		name           string
+		nodeNum1       int


perhaps errorNodes and falseNodes?

Both FakeFilter and FalseFilter used here will return unschedulable, errorNodes might not fit here.

alculquicondor · 2020-08-07T13:50:27Z

pkg/scheduler/core/generic_scheduler_test.go

+				node := &v1.Node{ObjectMeta: metav1.ObjectMeta{Name: uid, UID: types.UID(uid)}}
+				nodes = append(nodes, node)
+				cache.AddNode(node)
+				failedNodeReturnCodeMap[uid] = framework.Unschedulable


What was the map for? shouldn't this be framework.Error?

it's mimic Unschedulable status and feasibleNodes is 0, so that FitError could be returned.

if it's not unschedulable status, it will not return FitError ,

kubernetes/pkg/scheduler/core/generic_scheduler.go

Lines 111 to 122 in 4f93175

if err != nil {

return result, err

}

trace.Step("Computing predicates done")

if len(feasibleNodes) == 0 {

return result, &framework.FitError{

Pod: pod,

NumAllNodes: g.nodeInfoSnapshot.NumNodes(),

Diagnosis: diagnosis,

}

}

and then the error message won't roll up to one line by below method,
func (f *FitError) Error()

pkg/scheduler/core/generic_scheduler_test.go

chendave

@alculquicondor thanks for your great suggestion! either replied or addressed.

chendave · 2020-08-08T05:43:57Z

pkg/scheduler/core/generic_scheduler_test.go

+				node := &v1.Node{ObjectMeta: metav1.ObjectMeta{Name: uid, UID: types.UID(uid)}}
+				nodes = append(nodes, node)
+				cache.AddNode(node)
+				failedNodeReturnCodeMap[uid] = framework.Unschedulable


it's mimic Unschedulable status and feasibleNodes is 0, so that FitError could be returned.

pkg/scheduler/core/generic_scheduler_test.go

chendave · 2020-08-08T05:47:02Z

pkg/scheduler/core/generic_scheduler_test.go

+func TestFitErrorNoSpammy(t *testing.T) {
+	tests := []struct {
+		name           string
+		nodeNum1       int


Both FakeFilter and FalseFilter used here will return unschedulable, errorNodes might not fit here.

chendave · 2020-08-09T06:48:54Z

/retest

fejta-bot · 2020-11-07T08:42:13Z

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

chendave · 2020-11-09T01:22:26Z

/remove-lifecycle stale

fejta-bot · 2021-02-07T01:34:25Z

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-contributor-experience at kubernetes/community.
/lifecycle stale

The error message should be rolled up into one line summary. Signed-off-by: Dave Chen <dave.chen@arm.com>

chendave · 2021-03-04T02:58:03Z

/remove-lifecycle stale

k8s-ci-robot · 2021-03-04T03:07:43Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: chendave
To complete the pull request process, please assign huang-wei after the PR has been reviewed.
You can assign the PR to them by writing /assign @huang-wei in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

pkg/scheduler/OWNERS

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

chendave · 2021-03-04T03:07:55Z

rebased.

@alculquicondor @ahg-g Does this one still have the chance to go?

alculquicondor

Sorry, I forgot about this PR.

So we are trying to test that the string representation of FitError is summarized. Why don't we directly test FitError.Error in the framework package? Or you can test recordSchedulingFailure in scheduler package sends the correct event message (I'm not sure if this is easy to test at unit level). TestGenericScheduler already tests that we return the correct FitError.

If your intent is to have a more E2E test, then the appropriate thing to do is to do it in the integration tests, where you can query the events and validate that message.

alculquicondor · 2021-03-04T14:21:19Z

pkg/scheduler/core/generic_scheduler_test.go

+					"FakeFilter",
+					st.NewFakeFilterPlugin(failedNodeReturnCodeMap),
+				),
+				st.RegisterFilterPlugin("FalseFilter", st.NewFalseFilterPlugin),


do we need this extra filter?

yes, we need this filter.

I want to test the case that the pod cannot pass the filter with different reasons, pls see my this testcase,
cannot pass the filters with two different reasons

chendave · 2021-03-05T04:18:01Z

Why don't we directly test FitError.Error in the framework package?

Test in framework workable as well, but the whole testcase need to rewrite.

alculquicondor · 2021-03-05T14:22:46Z

Just test the Error method

chendave · 2021-03-05T14:39:34Z

Just test the Error method

Got you, will come back for this later.

fejta-bot · 2021-06-03T15:06:57Z

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-contributor-experience at kubernetes/community.
/lifecycle stale

chendave · 2021-06-23T05:16:07Z

this is indeed a trivial change, no need to waste reviewer's time one this. so close.

/close

k8s-ci-robot · 2021-06-23T05:16:13Z

@chendave: Closed this PR.

In response to this:

this is indeed a trivial change, no need to waste reviewer's time one this. so close.

/close

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 added sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Aug 7, 2020

k8s-ci-robot requested review from ahg-g and alculquicondor August 7, 2020 08:19

chendave force-pushed the fiterror_message branch 2 times, most recently from 0902b54 to 40a2970 Compare August 7, 2020 09:32

alculquicondor reviewed Aug 7, 2020

View reviewed changes

chendave force-pushed the fiterror_message branch from 40a2970 to 66e892e Compare August 8, 2020 05:50

chendave commented Aug 8, 2020

View reviewed changes

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Nov 7, 2020

k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Nov 9, 2020

k8s-ci-robot added lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Feb 7, 2021

Add testcase to validate the reason of FitError

e26664c

The error message should be rolled up into one line summary. Signed-off-by: Dave Chen <dave.chen@arm.com>

k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Mar 4, 2021

chendave force-pushed the fiterror_message branch from 66e892e to e26664c Compare March 4, 2021 03:06

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

alculquicondor reviewed Mar 4, 2021

View reviewed changes

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jun 3, 2021

k8s-ci-robot closed this Jun 23, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add testcase to validate the reason of `FitError` #93770

Add testcase to validate the reason of `FitError` #93770

chendave commented Aug 7, 2020

chendave commented Aug 7, 2020

alculquicondor left a comment

alculquicondor Aug 7, 2020

chendave Aug 8, 2020

alculquicondor Aug 7, 2020

chendave Aug 8, 2020

chendave Mar 4, 2021

chendave left a comment

chendave Aug 8, 2020

chendave Aug 8, 2020

chendave commented Aug 9, 2020

fejta-bot commented Nov 7, 2020

chendave commented Nov 9, 2020

fejta-bot commented Feb 7, 2021

chendave commented Mar 4, 2021

k8s-ci-robot commented Mar 4, 2021

chendave commented Mar 4, 2021

alculquicondor left a comment

alculquicondor Mar 4, 2021

chendave Mar 5, 2021

chendave commented Mar 5, 2021 •

edited

Loading

alculquicondor commented Mar 5, 2021

chendave commented Mar 5, 2021

fejta-bot commented Jun 3, 2021

chendave commented Jun 23, 2021

k8s-ci-robot commented Jun 23, 2021

	if err != nil {
	return result, err
	}
	trace.Step("Computing predicates done")

	if len(feasibleNodes) == 0 {
	return result, &framework.FitError{
	Pod: pod,
	NumAllNodes: g.nodeInfoSnapshot.NumNodes(),
	Diagnosis: diagnosis,
	}
	}

Add testcase to validate the reason of FitError #93770

Add testcase to validate the reason of FitError #93770

Conversation

chendave commented Aug 7, 2020

chendave commented Aug 7, 2020

alculquicondor left a 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

chendave left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chendave commented Aug 9, 2020

fejta-bot commented Nov 7, 2020

chendave commented Nov 9, 2020

fejta-bot commented Feb 7, 2021

chendave commented Mar 4, 2021

k8s-ci-robot commented Mar 4, 2021

chendave commented Mar 4, 2021

alculquicondor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chendave commented Mar 5, 2021 • edited Loading

alculquicondor commented Mar 5, 2021

chendave commented Mar 5, 2021

fejta-bot commented Jun 3, 2021

chendave commented Jun 23, 2021

k8s-ci-robot commented Jun 23, 2021

Add testcase to validate the reason of `FitError` #93770

Add testcase to validate the reason of `FitError` #93770

chendave commented Mar 5, 2021 •

edited

Loading