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:
[email protected]
