[GitHub] [incubator-yunikorn-k8shim] wilfred-s commented on a change in pull request #239: [YUNIKORN-562] Support add labels/annotations to taskGroup

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

wilfred-s commented on a change in pull request #239:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/239#discussion_r592152660



##########
File path: go.sum
##########
@@ -829,6 +829,7 @@ golang.org/x/xerrors v0.0.0-20190717185122-a985d3407aa7/go.mod h1:I/5z698sn9Ka8T
 golang.org/x/xerrors v0.0.0-20191011141410-1b5146add898/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0=
 golang.org/x/xerrors v0.0.0-20191204190536-9bdfabe68543 h1:E7g+9GITq07hpfrRu66IVDexMakfv52eLZ2CXBWiKr4=
 golang.org/x/xerrors v0.0.0-20191204190536-9bdfabe68543/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0=
+golang.org/x/xerrors v0.0.0-20200804184101-5ec99f83aff1 h1:go1bK/D/BFZV2I8cIQd1NKEZ+0owSTG1fDTci4IqFcE=

Review comment:
       We should not update the go.sum in this change.

##########
File path: pkg/cache/placeholder.go
##########
@@ -73,6 +73,22 @@ func newPlaceholder(placeholderName string, app *Application, taskGroup v1alpha1
 	}
 }
 
+// merge two string maps
+// if the same key defined in the first and second maps
+// the value will be set by the second map
+func mergeMaps(first, second map[string]string) map[string]string {
+	// add default labels first

Review comment:
       This comment does not make sense: we're walking over a list of keys that could be anything, nothing to do with a label. Same for the comment below referencing taskgroup

##########
File path: pkg/cache/placeholder.go
##########
@@ -40,14 +40,14 @@ func newPlaceholder(placeholderName string, app *Application, taskGroup v1alpha1
 		ObjectMeta: metav1.ObjectMeta{
 			Name:      placeholderName,
 			Namespace: app.tags[constants.AppTagNamespace],
-			Labels: map[string]string{
+			Labels: mergeMaps(map[string]string{
 				constants.LabelApplicationID: app.GetApplicationID(),
 				constants.LabelQueueName:     app.GetQueue(),
-			},
-			Annotations: map[string]string{
+			}, taskGroup.Labels),

Review comment:
       The labels we set should not be overwritten by what the end user provides. We need it to be set to the correct app etc without allowing an override

##########
File path: pkg/cache/placeholder.go
##########
@@ -40,14 +40,14 @@ func newPlaceholder(placeholderName string, app *Application, taskGroup v1alpha1
 		ObjectMeta: metav1.ObjectMeta{
 			Name:      placeholderName,
 			Namespace: app.tags[constants.AppTagNamespace],
-			Labels: map[string]string{
+			Labels: mergeMaps(map[string]string{
 				constants.LabelApplicationID: app.GetApplicationID(),
 				constants.LabelQueueName:     app.GetQueue(),
-			},
-			Annotations: map[string]string{
+			}, taskGroup.Labels),
+			Annotations: mergeMaps(map[string]string{
 				constants.AnnotationPlaceholderFlag: "true",
 				constants.AnnotationTaskGroupName:   taskGroup.Name,
-			},
+			}, taskGroup.Annotations),

Review comment:
       I am not sure if we should override these values with the values from the annotations. We really need to make sure that these are set as we need and not allow overriding.

##########
File path: pkg/cache/placeholder_test.go
##########
@@ -68,6 +68,82 @@ func TestNewPlaceholder(t *testing.T) {
 	assert.Equal(t, holder.String(), "appID: app01, taskGroup: test-group-1, podName: test/ph-name")
 }
 
+func TestNewPlaceholderWithLabelsAndAnnotations(t *testing.T) {
+	const (
+		appID     = "app01"
+		queue     = "root.default"
+		namespace = "test"
+	)
+	mockedSchedulerAPI := newMockSchedulerAPI()
+	app := NewApplication(appID, queue,
+		"bob", map[string]string{constants.AppTagNamespace: namespace}, mockedSchedulerAPI)
+	app.setTaskGroups([]v1alpha1.TaskGroup{
+		{
+			Name:      "test-group-1",
+			MinMember: 10,
+			MinResource: map[string]resource.Quantity{
+				"cpu":    resource.MustParse("500m"),
+				"memory": resource.MustParse("1024M"),
+			},
+			Labels: map[string]string{
+				"labelKey0" : "labelKeyValue0",
+				"labelKey1" : "labelKeyValue1",
+			},
+			Annotations: map[string]string{
+				"annotationKey0" : "annotationValue0",
+				"annotationKey1" : "annotationValue1",
+				"annotationKey2" : "annotationValue2",
+			},
+		},
+	})
+
+	holder := newPlaceholder("ph-name", app, app.taskGroups[0])
+	assert.Equal(t, len(holder.pod.Labels), 4)
+	assert.Equal(t, len(holder.pod.Annotations), 5)
+	assert.Equal(t, holder.pod.Labels["labelKey0"], "labelKeyValue0")
+	assert.Equal(t, holder.pod.Labels["labelKey1"], "labelKeyValue1")
+	assert.Equal(t, holder.pod.Annotations["annotationKey0"], "annotationValue0")
+	assert.Equal(t, holder.pod.Annotations["annotationKey1"], "annotationValue1")
+	assert.Equal(t, holder.pod.Annotations["annotationKey2"], "annotationValue2")
+}
+
+func TestMergeMaps(t *testing.T) {
+	result := mergeMaps(nil, nil)
+	assert.Assert(t, result != nil)
+	assert.Equal(t, len(result), 0)

Review comment:
       two nil maps as input should return a nil map as output.

##########
File path: pkg/cache/placeholder.go
##########
@@ -73,6 +73,22 @@ func newPlaceholder(placeholderName string, app *Application, taskGroup v1alpha1
 	}
 }
 
+// merge two string maps
+// if the same key defined in the first and second maps
+// the value will be set by the second map

Review comment:
       This function should be defined as part of the `common.utils`. Easier for re-use.




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