[GitHub] [incubator-yunikorn-k8shim] yangwwei commented on a change in pull request #182: [WIP][YUNIKORN-366] Add rest API to update queue configuration

[GitBox <gi...@apache.org>]

yangwwei commented on a change in pull request #182:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/182#discussion_r477040798



##########
File path: pkg/cache/context.go
##########
@@ -765,3 +767,62 @@ func (ctx *Context) SchedulerNodeEventHandler() func(obj interface{}) {
 	// this is not required in some tests
 	return nil
 }
+
+func createConfigMapSelector() (labels.Selector, error) {
+	slt := labels.NewSelector()
+	appVals := [] string {"yunikorn"}
+	req, err := labels.NewRequirement(common.LabelApp, selection.Equals, appVals)
+	if err != nil {
+		return  nil, err
+	}
+	slt = slt.Add(*req)
+	return slt, nil
+}
+
+func findYKConfigMap(configMaps []*v1.ConfigMap) (*v1.ConfigMap, error) {
+	var ykconf *v1.ConfigMap
+	foundConf := false
+	if len(configMaps) == 0 {
+		return nil, fmt.Errorf("configmap with label app:yunikorn not found")
+	}
+	for _, c := range configMaps {
+		if c.Name == common.DefaultConfigMapName {
+			ykconf = c
+			foundConf = true
+			break
+		}
+	}
+	if !foundConf {
+		return nil, fmt.Errorf("configmap with name %s not found", common.DefaultConfigMapName)
+	}
+	return ykconf, nil
+}

Review comment:
       this can be simplified to:
   
   ```
   for _, c := range configMaps {
     if c.Name == common.DefaultConfigMapName {
       return c
     }
   }
   return nil, fmt.Errorf("configmap with name %s not found", common.DefaultConfigMapName)
   ```
   because range handles the case when the slice is nil

##########
File path: pkg/cache/context.go
##########
@@ -21,6 +21,8 @@ package cache
 import (
 	"encoding/json"
 	"fmt"
+	"k8s.io/apimachinery/pkg/labels"
+	"k8s.io/apimachinery/pkg/selection"
 	"strings"

Review comment:
       fix the import format, make sure this passes the lint checks

##########
File path: pkg/cache/context.go
##########
@@ -765,3 +767,62 @@ func (ctx *Context) SchedulerNodeEventHandler() func(obj interface{}) {
 	// this is not required in some tests
 	return nil
 }
+
+func createConfigMapSelector() (labels.Selector, error) {
+	slt := labels.NewSelector()
+	appVals := [] string {"yunikorn"}
+	req, err := labels.NewRequirement(common.LabelApp, selection.Equals, appVals)
+	if err != nil {
+		return  nil, err
+	}
+	slt = slt.Add(*req)
+	return slt, nil
+}

Review comment:
       Can you pls check if this can be simplified by
   
   ```
   labels.SelectorFromSet()
   ```
   
   if that can work, seems like we do not need a separate function for this.

##########
File path: pkg/cache/context.go
##########
@@ -765,3 +767,62 @@ func (ctx *Context) SchedulerNodeEventHandler() func(obj interface{}) {
 	// this is not required in some tests
 	return nil
 }
+
+func createConfigMapSelector() (labels.Selector, error) {
+	slt := labels.NewSelector()
+	appVals := [] string {"yunikorn"}
+	req, err := labels.NewRequirement(common.LabelApp, selection.Equals, appVals)
+	if err != nil {
+		return  nil, err
+	}
+	slt = slt.Add(*req)
+	return slt, nil
+}
+
+func findYKConfigMap(configMaps []*v1.ConfigMap) (*v1.ConfigMap, error) {
+	var ykconf *v1.ConfigMap
+	foundConf := false
+	if len(configMaps) == 0 {
+		return nil, fmt.Errorf("configmap with label app:yunikorn not found")
+	}
+	for _, c := range configMaps {
+		if c.Name == common.DefaultConfigMapName {
+			ykconf = c
+			foundConf = true
+			break
+		}
+	}
+	if !foundConf {
+		return nil, fmt.Errorf("configmap with name %s not found", common.DefaultConfigMapName)
+	}
+	return ykconf, nil
+}
+/*
+Save the configmap and returns the old one and an error if the process failed
+ */
+func (ctx *Context) SaveConfigmap(data string) (string, error) {
+	slt, err := createConfigMapSelector()
+	if err != nil {
+		return "", err
+	}
+
+	configMaps, err := ctx.apiProvider.GetAPIs().ConfigMapInformer.Lister().List(slt)
+	if err != nil {
+		return "", err
+	}
+	ykconf, err := findYKConfigMap(configMaps)
+	if err != nil {
+		return "", err
+	}
+
+	newConfData := map[string] string {"queues.yaml":strings.ReplaceAll(data, "\r\n", "\n")}

Review comment:
       not sure why the replaceAll is needed here. 
   Why we have `\r\n` in the fist place?

##########
File path: pkg/cache/context.go
##########
@@ -765,3 +767,62 @@ func (ctx *Context) SchedulerNodeEventHandler() func(obj interface{}) {
 	// this is not required in some tests
 	return nil
 }
+
+func createConfigMapSelector() (labels.Selector, error) {
+	slt := labels.NewSelector()
+	appVals := [] string {"yunikorn"}
+	req, err := labels.NewRequirement(common.LabelApp, selection.Equals, appVals)
+	if err != nil {
+		return  nil, err
+	}
+	slt = slt.Add(*req)
+	return slt, nil
+}
+
+func findYKConfigMap(configMaps []*v1.ConfigMap) (*v1.ConfigMap, error) {
+	var ykconf *v1.ConfigMap
+	foundConf := false
+	if len(configMaps) == 0 {
+		return nil, fmt.Errorf("configmap with label app:yunikorn not found")
+	}
+	for _, c := range configMaps {
+		if c.Name == common.DefaultConfigMapName {
+			ykconf = c
+			foundConf = true
+			break
+		}
+	}
+	if !foundConf {
+		return nil, fmt.Errorf("configmap with name %s not found", common.DefaultConfigMapName)
+	}
+	return ykconf, nil
+}
+/*
+Save the configmap and returns the old one and an error if the process failed
+ */
+func (ctx *Context) SaveConfigmap(data string) (string, error) {
+	slt, err := createConfigMapSelector()
+	if err != nil {
+		return "", err
+	}
+
+	configMaps, err := ctx.apiProvider.GetAPIs().ConfigMapInformer.Lister().List(slt)
+	if err != nil {
+		return "", err
+	}
+	ykconf, err := findYKConfigMap(configMaps)
+	if err != nil {
+		return "", err
+	}
+
+	newConfData := map[string] string {"queues.yaml":strings.ReplaceAll(data, "\r\n", "\n")}
+	newConf := ykconf.DeepCopy()
+	oldConfData := ykconf.Data["queues.yaml"]
+	newConf.Data = newConfData
+	_, err = ctx.apiProvider.GetAPIs().KubeClient.GetClientSet().CoreV1().ConfigMaps(ykconf.Namespace).Update(newConf)
+	if err != nil {
+		return "", err
+	}
+	log.Logger().Debug("ConfigMap updated successfully")

Review comment:
       this should be the `INFO` level




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org