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

cmd/webhook-manager/main.go

@@ -30,6 +30,7 @@ import (
 	"volcano.sh/volcano/cmd/webhook-manager/app"
 	"volcano.sh/volcano/cmd/webhook-manager/app/options"
 
+	_ "volcano.sh/volcano/pkg/webhooks/admission/configmap/validate"
 	_ "volcano.sh/volcano/pkg/webhooks/admission/jobs/mutate"
 	_ "volcano.sh/volcano/pkg/webhooks/admission/jobs/validate"
 	_ "volcano.sh/volcano/pkg/webhooks/admission/pods"

pkg/scheduler/util.go

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

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

@@ -0,0 +1,143 @@
+package validate
+
+import (
+	"fmt"
+
+	yaml "gopkg.in/yaml.v2"
+
+	"k8s.io/api/admission/v1beta1"
+	whv1beta1 "k8s.io/api/admissionregistration/v1beta1"
+	v1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/klog"
+	"strings"
+	"volcano.sh/volcano/pkg/webhooks/schema"
+	"volcano.sh/volcano/pkg/webhooks/util"
+
+	"volcano.sh/volcano/pkg/scheduler/conf"
+	"volcano.sh/volcano/pkg/webhooks/router"
+)
+
+const (
+	configmapName          = "volcano-scheduler-configmap"
+	enqueueName            = "enqueue"
+	overCommitFactor       = "overcommit-factor"
+	overCommitFactorMinVal = 1.0
+)
+
+func init() {
+	router.RegisterAdmission(service)
+}
+
+var service = &router.AdmissionService{
+	Path: "/configmaps/validate",
+	Func: AdmitConfigmaps,
+
+	Config: config,
+
+	ValidatingConfig: &whv1beta1.ValidatingWebhookConfiguration{
+		Webhooks: []whv1beta1.ValidatingWebhook{{
+			Name: "validateconfigmap.volcano.sh",
+			Rules: []whv1beta1.RuleWithOperations{
+				{
+					Operations: []whv1beta1.OperationType{whv1beta1.Create, whv1beta1.Update},
+					Rule: whv1beta1.Rule{
+						APIGroups:   []string{""},
+						APIVersions: []string{"v1"},
+						Resources:   []string{"configmaps"},
+					},
+				},
+			},
+		}},
+	},
+}
+
+var config = &router.AdmissionServiceConfig{}
+
+// AdmitConfigmaps is to admit configMap for volcano scheduler and return response.
+func AdmitConfigmaps(ar v1beta1.AdmissionReview) *v1beta1.AdmissionResponse {
+	klog.V(3).Infof("admitting configmap -- %s", ar.Request.Operation)
+
+	cm, err := schema.DecodeConfigmap(ar.Request.Object, ar.Request.Resource)
+	if err != nil {
+		return util.ToAdmissionResponse(err)
+	}
+
+	var msg string
+	reviewResponse := v1beta1.AdmissionResponse{}
+	reviewResponse.Allowed = true
+
+	switch ar.Request.Operation {
+	case v1beta1.Create, v1beta1.Update:
+		msg = validateConfigmap(cm, &reviewResponse)
+	default:
+		err := fmt.Errorf("expect operation to be 'CREATE' or 'UPDATE'")
+		return util.ToAdmissionResponse(err)
+	}
+
+	if !reviewResponse.Allowed {
+		reviewResponse.Result = &metav1.Status{Message: strings.TrimSpace(msg)}
+	}
+	return &reviewResponse
+}
+
+/*
+allow config to create and update when
+1. configmap name must be "volcano-scheduler-configmap"
+2. config data must be only one
+3. enqueue argument "overcommit-factor" must be greater than 1 if existed
+*/
+func validateConfigmap(cm *v1.ConfigMap, reviewResponse *v1beta1.AdmissionResponse) string {
+	msg := ""
+	if cm.Name != configmapName {
+		return msg
+	}
+
+	confKey := ""
+	confNum := 0
+	for key := range cm.Data {
+		if strings.HasSuffix(key, ".conf") {
+			confNum++
+			confKey = key
+		}
+	}
+
+	if confNum != 1 {
+		reviewResponse.Allowed = false
+		return fmt.Errorf("%s data format is err, config file = %d ", configmapName, confNum).Error()
+	}
+
+	data := cm.Data[confKey]
+	schedulerConf := &conf.SchedulerConfiguration{}
+	buf := make([]byte, len(data))
+	copy(buf, data)
+
+	if err := yaml.Unmarshal(buf, schedulerConf); err != nil {
+		reviewResponse.Allowed = false
+		return fmt.Errorf("%s data %s Unmarshal failed", configmapName, confKey).Error()
+	}
+
+	err := enqueueConfCheck(schedulerConf.Configurations)
+	if err != nil {
+		reviewResponse.Allowed = false
+		return err.Error()
+	}
+
+	return msg
+}
+
+// check enqueue action's legality
+func enqueueConfCheck(configurations []conf.Configuration) error {
+	actionArg := GetArgOfActionFromConf(configurations, enqueueName)
+	if actionArg == nil {
+		return nil
+	}
+
+	var factor float64
+	actionArg.GetFloat64(&factor, overCommitFactor)
+	if factor < overCommitFactorMinVal {
+		return fmt.Errorf("enqueue argument %s must be more than %v", overCommitFactor, overCommitFactorMinVal)
+	}
+
+	return nil
+}

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

@@ -0,0 +1,184 @@
+package validate
+
+import (
+	"k8s.io/api/admission/v1beta1"
+	v1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"strings"
+	"testing"
+)
+
+func TestValidatePod(t *testing.T) {
+	testCases := []struct {
+		Name           string
+		ConfigMap      v1.ConfigMap
+		ExpectErr      bool
+		reviewResponse v1beta1.AdmissionResponse
+		ret            string
+	}{
+		{
+			Name: "check whether it is volcano config",
+			ConfigMap: v1.ConfigMap{
+				TypeMeta: metav1.TypeMeta{
+					APIVersion: "v1",
+					Kind:       "ConfigMap",
+				},
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "test-err-name.conf",
+				},
+				Data: map[string]string{
+					"default-scheduler.conf": `
+                        actions: "enqueue, allocate, backfill"
+                        tiers:
+                        - plugins:
+                          - name: priority
+                          - name: gang
+                          - name: conformance
+                        - plugins:
+                          - name: drf
+                          - name: predicates
+                          - name: proportion
+                          - name: nodeorder
+                        `,
+				},
+			},
+			reviewResponse: v1beta1.AdmissionResponse{Allowed: true},
+			ret:            "",
+			ExpectErr:      false,
+		},
+		{
+			Name: "check whether it exists more one config data",
+			ConfigMap: v1.ConfigMap{
+				TypeMeta: metav1.TypeMeta{
+					APIVersion: "v1",
+					Kind:       "ConfigMap",
+				},
+				ObjectMeta: metav1.ObjectMeta{
+					Name: configmapName,
+				},
+				Data: map[string]string{
+					"default-scheduler.conf": `
+                        actions: "enqueue, allocate, backfill"
+                        tiers:
+                        - plugins:
+                          - name: priority
+                          - name: gang
+                          - name: conformance
+                        - plugins:
+                          - name: drf
+                          - name: predicates
+                          - name: proportion
+                          - name: nodeorder
+                    `,
+					"ief-scheduler.conf": `
+                        actions: "enqueue, allocate, backfill"
+                        tiers:
+                        - plugins:
+                          - name: priority
+                          - name: gang
+                          - name: conformance
+                        - plugins:
+                          - name: drf
+                          - name: predicates
+                          - name: proportion
+                          - name: nodeorder
+                    `,
+				},
+			},
+			reviewResponse: v1beta1.AdmissionResponse{Allowed: false},
+			ret:            "data format is err",
+			ExpectErr:      true,
+		},
+		{
+			Name: "check whether it is volcano config",
+			ConfigMap: v1.ConfigMap{
+				TypeMeta: metav1.TypeMeta{
+					APIVersion: "v1",
+					Kind:       "ConfigMap",
+				},
+				ObjectMeta: metav1.ObjectMeta{
+					Name: configmapName,
+				},
+				Data: map[string]string{
+					"default-scheduler.conf": `
+                        actions: "enqueue, allocate, backfill"
+                        configurations:
+                        - name: enqueue
+                          arguments:
+                            overcommit-factor: 1.2
+                        tiers:
+                        - plugins:
+                          - name: priority
+                          - name: gang
+                          - name: conformance
+                        - plugins:
+                          - name: drf
+                          - name: predicates
+                          - name: proportion
+                          - name: nodeorder
+                        `,
+				},
+			},
+			reviewResponse: v1beta1.AdmissionResponse{Allowed: true},
+			ret:            "",
+			ExpectErr:      false,
+		},
+		{
+			Name: "check whether it is volcano config",
+			ConfigMap: v1.ConfigMap{
+				TypeMeta: metav1.TypeMeta{
+					APIVersion: "v1",
+					Kind:       "ConfigMap",
+				},
+				ObjectMeta: metav1.ObjectMeta{
+					Name: configmapName,
+				},
+				Data: map[string]string{
+					"default-scheduler.conf": `
+                        actions: "enqueue, allocate, backfill"
+                        configurations:
+                        - name: enqueue
+                          arguments:
+                            overcommit-factor: 0.8
+                        tiers:
+                        - plugins:
+                          - name: priority
+                          - name: gang
+                          - name: conformance
+                        - plugins:
+                          - name: drf
+                          - name: predicates
+                          - name: proportion
+                          - name: nodeorder
+                        `,
+				},
+			},
+			reviewResponse: v1beta1.AdmissionResponse{Allowed: false},
+			ret:            "must be more than",
+			ExpectErr:      true,
+		},
+	}
+
+	for _, testCase := range testCases {
+		ret := validateConfigmap(&testCase.ConfigMap, &testCase.reviewResponse)
+		if testCase.ExpectErr == true && ret == "" {
+			t.Errorf("%s: test case Expect error msg :%s, but got nil.", testCase.Name, testCase.ret)
+		}
+
+		if testCase.ExpectErr == true && testCase.reviewResponse.Allowed != false {
+			t.Errorf("%s: test case Expect Allowed as false but got true.", testCase.Name)
+		}
+
+		if testCase.ExpectErr == true && !strings.Contains(ret, testCase.ret) {
+			t.Errorf("%s: test case Expect error msg :%s, but got diff error %v", testCase.Name, testCase.ret, ret)
+		}
+
+		if testCase.ExpectErr == false && ret != "" {
+			t.Errorf("%s: test case Expect no error, but got error %v", testCase.Name, ret)
+		}
+
+		if testCase.ExpectErr == false && testCase.reviewResponse.Allowed != true {
+			t.Errorf("%s: test case Expect Allowed as true but got false. %v", testCase.Name, testCase.reviewResponse)
+		}
+	}
+}

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

@@ -0,0 +1,43 @@
+package validate
+
+import (
+	"strconv"
+
+	"k8s.io/klog"
+
+	"volcano.sh/volcano/pkg/scheduler/conf"
+)
+
+// Arguments map
+type Arguments map[string]string
+
+// GetArgOfActionFromConf return argument of action reading from configuration of schedule
+func GetArgOfActionFromConf(configurations []conf.Configuration, actionName string) Arguments {
+	for _, c := range configurations {
+		if c.Name == actionName {
+			return c.Arguments
+		}
+	}
+
+	return nil
+}
+
+// GetFloat64 get the float64 value from string
+func (a Arguments) GetFloat64(ptr *float64, key string) {
+	if ptr == nil {
+		return
+	}
+
+	argv, ok := a[key]
+	if !ok || len(argv) == 0 {
+		return
+	}
+
+	value, err := strconv.ParseFloat(argv, 64)
+	if err != nil {
+		klog.Warningf("Could not parse argument: %s for key %s, with err %v", argv, key, err)
+		return
+	}
+
+	*ptr = value
+}