fix: issue#1261 config "overcommit-factor" less than 1 leads scheduler crash #1267

huone1 · 2021-01-26T08:35:55Z

kaihua give better advice： check OverCommitFactor in webhook validate。
so add webhook validate ，just for scheduler configmap；
currently， there are only some check for enqueue action， and add other actions or plugins ‘s check to it in future
Signed-off-by: huone1 huwanxing@huawei.com

shinytang6 · 2021-01-26T08:46:56Z

pkg/scheduler/actions/enqueue/enqueue.go

+	if factor < minOverCommitFactor {
+		factor = minOverCommitFactor
+	}
+


Could you add a unit test for that

I've added ut just now.

volcano-sh-bot · 2021-02-01T08:06:47Z

New changes are detected. LGTM label has been removed.

Signed-off-by: huone1 <huwanxing@huawei.com>

shinytang6 · 2021-02-05T10:46:46Z

pkg/scheduler/util.go

@@ -34,6 +34,7 @@ tiers:
 - plugins:
  - name: priority
  - name: gang
+  - name: conformance
 - plugins:
  - name: drf
  - name: predicates


binpack is also needed

I was attend to add binpack because it is not fit all scenarios， so it wasn't added.

The default cm under installer/volcano-development.yaml contains binpack, maybe we should remove binpack in that file?

shinytang6 · 2021-02-05T10:48:07Z

pkg/webhooks/admission/configmap/validate/admit_cm.go

+
+	"volcano.sh/volcano/pkg/scheduler/conf"
+	"volcano.sh/volcano/pkg/webhooks/router"
+)


dependency order needed to formated?

shinytang6 · 2021-02-05T10:51:49Z

pkg/webhooks/admission/configmap/validate/admit_cm.go

+
+/*
+allow config to create and update when
+1. configmap name must be "volcano-scheduler-configmap"


l think volcano-scheduler-configmap is not a necessity, it's defined by user

I check helm files ， you're right。
configmap name Transfer to admission webhook through ENV ， is it OK ？？

shinytang6 · 2021-02-05T11:00:05Z

pkg/webhooks/admission/configmap/validate/util.go

@@ -0,0 +1,43 @@
+package validate


The functions in this file should already exist in codebase？

yes ， but it is in scheduler folder， so I repeat some

volcano-sh-bot · 2021-02-05T11:00:25Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: huone1
To complete the pull request process, please assign hzxuzhonghu
You can assign the PR to them by writing /assign @hzxuzhonghu 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:

OWNERS

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

shinytang6 · 2021-02-06T04:09:33Z

pkg/webhooks/admission/configmap/validate/admit_cm.go

+	confKey := ""
+	confNum := 0
+	for key := range cm.Data {
+		if strings.HasSuffix(key, ".conf") {


also .conf is not a necessity

1，it watch all configmap, we must filter them, so it is necessary to adjust the configmap name
2。i think it is necessary to certain restrictions on the format of scheduler configmap data

Okay got it, I have no opinion on this change, but this introduces some name restrictions indeed.

k82cn · 2021-02-06T11:58:23Z

/hold

k82cn · 2021-02-06T11:59:36Z

prefer not to check scheduler's arguments in webhook.

huone1 · 2021-02-07T03:42:16Z

prefer not to check scheduler's arguments in webhook.

Another way ： scheduler process gets the scheduler's arguments to check them， and if one arguments is illegal，reset it to default val;

k82cn · 2021-02-07T06:03:46Z

Another way ： scheduler process gets the scheduler's arguments to check them， and if one arguments is illegal，reset it to default val;

That's better to me, please also record an event and log for that :)

huone1 · 2021-03-20T07:30:33Z

the question is resloved by #1324 , so closed it

volcano-sh-bot requested review from shinytang6 and william-wang January 26, 2021 08:35

volcano-sh-bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Jan 26, 2021

huone1 force-pushed the OverCommitFactor branch from 7f0943d to d957dae Compare January 26, 2021 08:39

huone1 changed the title ~~fix: issueconfig "overcommit-factor" less than 1 leads scheduler crash~~ fix: issue#1261 config "overcommit-factor" less than 1 leads scheduler crash Jan 26, 2021

huone1 mentioned this pull request Jan 26, 2021

config "overcommit-factor" less than 1 leads scheduler crash #1261

Closed

shinytang6 suggested changes Jan 26, 2021

View reviewed changes

volcano-sh-bot assigned shinytang6 Jan 26, 2021

huone1 force-pushed the OverCommitFactor branch from d957dae to 77fd010 Compare January 26, 2021 08:57

volcano-sh-bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jan 26, 2021

shinytang6 approved these changes Jan 26, 2021

View reviewed changes

volcano-sh-bot added lgtm Indicates that a PR is ready to be merged. and removed lgtm Indicates that a PR is ready to be merged. labels Jan 26, 2021

volcano-sh-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Feb 1, 2021

huone1 force-pushed the OverCommitFactor branch 4 times, most recently from b415434 to 00df8bd Compare February 2, 2021 08:12

check scheduler configmap by webhook

e5faa64

Signed-off-by: huone1 <huwanxing@huawei.com>

huone1 force-pushed the OverCommitFactor branch from 00df8bd to e5faa64 Compare February 2, 2021 08:16

shinytang6 suggested changes Feb 5, 2021

View reviewed changes

shinytang6 reviewed Feb 6, 2021

View reviewed changes

volcano-sh-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 6, 2021

huone1 closed this Mar 20, 2021

huone1 deleted the OverCommitFactor branch March 20, 2021 07:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: issue#1261 config "overcommit-factor" less than 1 leads scheduler crash #1267

fix: issue#1261 config "overcommit-factor" less than 1 leads scheduler crash #1267

huone1 commented Jan 26, 2021 •

edited

Loading

shinytang6 Jan 26, 2021

huone1 Jan 26, 2021

volcano-sh-bot commented Feb 1, 2021

shinytang6 Feb 5, 2021

huone1 Feb 5, 2021

shinytang6 Feb 6, 2021

shinytang6 Feb 5, 2021

shinytang6 Feb 5, 2021

huone1 Feb 5, 2021

shinytang6 Feb 5, 2021 •

edited

Loading

huone1 Feb 5, 2021

volcano-sh-bot commented Feb 5, 2021

shinytang6 Feb 6, 2021

huone1 Feb 6, 2021

shinytang6 Feb 6, 2021

k82cn commented Feb 6, 2021

k82cn commented Feb 6, 2021

huone1 commented Feb 7, 2021

k82cn commented Feb 7, 2021

huone1 commented Mar 20, 2021

fix: issue#1261 config "overcommit-factor" less than 1 leads scheduler crash #1267

fix: issue#1261 config "overcommit-factor" less than 1 leads scheduler crash #1267

Conversation

huone1 commented Jan 26, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

volcano-sh-bot commented Feb 1, 2021

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

shinytang6 Feb 5, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

volcano-sh-bot commented Feb 5, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k82cn commented Feb 6, 2021

k82cn commented Feb 6, 2021

huone1 commented Feb 7, 2021

k82cn commented Feb 7, 2021

huone1 commented Mar 20, 2021

huone1 commented Jan 26, 2021 •

edited

Loading

shinytang6 Feb 5, 2021 •

edited

Loading