You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@yunikorn.apache.org by GitBox <gi...@apache.org> on 2022/11/16 16:25:20 UTC
[GitHub] [yunikorn-k8shim] 0yukali0 commented on a diff in pull request #474: [Yunikorn-1353] refactor annotation and label processing
0yukali0 commented on code in PR #474:
URL: https://github.com/apache/yunikorn-k8shim/pull/474#discussion_r1024219300
##########
pkg/common/utils/utils.go:
##########
@@ -257,3 +259,109 @@ func GetUserFromPod(pod *v1.Pod) string {
return value
}
+
+func GetPodAnnotationValue(pod *v1.Pod, annotationKey string) string {
+ if value, ok := pod.Annotations[annotationKey]; ok {
+ return value
+ }
+ return ""
+}
+
+func GetNameSpaceAnnotationValue(namespace *v1.Namespace, annotationKey string) string {
+ if value, ok := namespace.Annotations[annotationKey]; ok {
+ return value
+ }
+ return ""
+}
+
+func GetPodLabelValue(pod *v1.Pod, labelKey string) string {
+ if value, ok := pod.Labels[labelKey]; ok {
+ return value
+ }
+ return ""
+}
+
+func GetTaskGroupFromPodSpec(pod *v1.Pod) string {
+ return GetPodAnnotationValue(pod, constants.AnnotationTaskGroupName)
+}
+
+func GetPlaceholderFlagFromPodSpec(pod *v1.Pod) bool {
+ if value := GetPodAnnotationValue(pod, constants.AnnotationPlaceholderFlag); value != "" {
+ if v, err := strconv.ParseBool(value); err == nil {
+ return v
+ }
+ }
+
+ if value := GetPodLabelValue(pod, constants.LabelPlaceholderFlag); value != "" {
+ if v, err := strconv.ParseBool(value); err == nil {
+ return v
+ }
+ }
+ return false
+}
+
+func GetTaskGroupsFromAnnotation(pod *v1.Pod) ([]v1alpha1.TaskGroup, error) {
+ taskGroupInfo := GetPodAnnotationValue(pod, constants.AnnotationTaskGroups)
+ if taskGroupInfo == "" {
+ return nil, nil
+ }
+
+ taskGroups := []v1alpha1.TaskGroup{}
+ err := json.Unmarshal([]byte(taskGroupInfo), &taskGroups)
+ if err != nil {
+ return nil, err
+ }
+ // json.Unmarchal won't return error if name or MinMember is empty, but will return error if MinResource is empty or error format.
+ for _, taskGroup := range taskGroups {
+ if taskGroup.Name == "" {
+ return nil, fmt.Errorf("can't get taskGroup Name from pod annotation, %s",
+ taskGroupInfo)
+ }
+ if taskGroup.MinMember == int32(0) {
+ return nil, fmt.Errorf("can't get taskGroup MinMember from pod annotation, %s",
+ taskGroupInfo)
Review Comment:
UT includes the nil MinResource, so I think we should add following code to handle this problem.
```
if taskGroup.MinResource == nil {
return nil, fmt.Errorf("can't get taskGroup MinResource from pod annotation, %s",
pod.Annotations[constants.AnnotationTaskGroups])
}
```
##########
pkg/common/utils/utils_test.go:
##########
@@ -696,3 +699,356 @@ func TestNeedRecovery(t *testing.T) {
})
}
}
+func TestGetTaskGroupFromPodSpec(t *testing.T) {
+ pod := &v1.Pod{
+ TypeMeta: metav1.TypeMeta{
+ Kind: "Pod",
+ APIVersion: "v1",
+ },
+ ObjectMeta: metav1.ObjectMeta{
+ Name: "pod-01",
+ UID: "UID-01",
+ Annotations: map[string]string{
+ constants.AnnotationTaskGroupName: "test-task-group",
+ },
+ },
+ }
+
+ assert.Equal(t, GetTaskGroupFromPodSpec(pod), "test-task-group")
+
+ pod = &v1.Pod{
+ TypeMeta: metav1.TypeMeta{
+ Kind: "Pod",
+ APIVersion: "v1",
+ },
+ ObjectMeta: metav1.ObjectMeta{
+ Name: "pod-01",
+ UID: "UID-01",
+ },
+ }
+
+ assert.Equal(t, GetTaskGroupFromPodSpec(pod), "")
+}
+
+// nolint: funlen
+func TestGetTaskGroupFromAnnotation(t *testing.T) {
+ // correct json
+ testGroup := `
+ [
+ {
+ "name": "test-group-1",
+ "minMember": 10,
+ "minResource": {
+ "cpu": 1,
+ "memory": "2Gi"
+ },
+ "nodeSelector": {
+ "test": "testnode",
+ "locate": "west"
+ },
+ "tolerations": [
+ {
+ "key": "key",
+ "operator": "Equal",
+ "value": "value",
+ "effect": "NoSchedule"
+ }
+ ]
+ },
+ {
+ "name": "test-group-2",
+ "minMember": 5,
+ "minResource": {
+ "cpu": 2,
+ "memory": "4Gi"
+ }
+ }
+ ]`
+ testGroup2 := `
+ [
+ {
+ "name": "test-group-3",
+ "minMember": 3,
+ "minResource": {
+ "cpu": 2,
+ "memory": "1Gi"
+ }
+ }
+ ]`
+ // Error json
+ testGroupErr := `
+ [
+ {
+ "name": "test-group-err-1",
+ "minMember": "ERR",
+ "minResource": {
+ "cpu": "ERR",
+ "memory": "ERR"
+ },
+ }
+ ]`
+ // without name
+ testGroupErr2 := `
+ [
+ {
+ "minMember": 3,
+ "minResource": {
+ "cpu": 2,
+ "memory": "1Gi"
+ }
+ }
+ ]`
+ // without minMember
+ testGroupErr3 := `
+ [
+ {
+ "name": "test-group-err-2",
+ "minResource": {
+ "cpu": 2,
+ "memory": "1Gi"
+ }
+ }
+ ]`
+ // withot minResource
+ testGroupErr4 := `
+ [
+ {
+ "name": "test-group-err-4",
+ "minMember": 3,
Review Comment:
This example is designed for nil minMenber.
So we should remove the comma, or this err will be classified to parsing error.
"minMember": 3, -> "minMember": 3
However, there is not an exception handling for empty minResource in the GetTaskGroupsFromAnnotation function.
##########
pkg/common/utils/utils_test.go:
##########
@@ -696,3 +699,356 @@ func TestNeedRecovery(t *testing.T) {
})
}
}
+func TestGetTaskGroupFromPodSpec(t *testing.T) {
+ pod := &v1.Pod{
+ TypeMeta: metav1.TypeMeta{
+ Kind: "Pod",
+ APIVersion: "v1",
+ },
+ ObjectMeta: metav1.ObjectMeta{
+ Name: "pod-01",
+ UID: "UID-01",
+ Annotations: map[string]string{
+ constants.AnnotationTaskGroupName: "test-task-group",
+ },
+ },
+ }
+
+ assert.Equal(t, GetTaskGroupFromPodSpec(pod), "test-task-group")
+
+ pod = &v1.Pod{
+ TypeMeta: metav1.TypeMeta{
+ Kind: "Pod",
+ APIVersion: "v1",
+ },
+ ObjectMeta: metav1.ObjectMeta{
+ Name: "pod-01",
+ UID: "UID-01",
+ },
+ }
+
+ assert.Equal(t, GetTaskGroupFromPodSpec(pod), "")
+}
+
+// nolint: funlen
+func TestGetTaskGroupFromAnnotation(t *testing.T) {
+ // correct json
+ testGroup := `
+ [
+ {
+ "name": "test-group-1",
+ "minMember": 10,
+ "minResource": {
+ "cpu": 1,
+ "memory": "2Gi"
+ },
+ "nodeSelector": {
+ "test": "testnode",
+ "locate": "west"
+ },
+ "tolerations": [
+ {
+ "key": "key",
+ "operator": "Equal",
+ "value": "value",
+ "effect": "NoSchedule"
+ }
+ ]
+ },
+ {
+ "name": "test-group-2",
+ "minMember": 5,
+ "minResource": {
+ "cpu": 2,
+ "memory": "4Gi"
+ }
+ }
+ ]`
+ testGroup2 := `
+ [
+ {
+ "name": "test-group-3",
+ "minMember": 3,
+ "minResource": {
+ "cpu": 2,
+ "memory": "1Gi"
+ }
+ }
+ ]`
+ // Error json
+ testGroupErr := `
+ [
+ {
+ "name": "test-group-err-1",
+ "minMember": "ERR",
+ "minResource": {
+ "cpu": "ERR",
+ "memory": "ERR"
+ },
+ }
+ ]`
+ // without name
+ testGroupErr2 := `
+ [
+ {
+ "minMember": 3,
+ "minResource": {
+ "cpu": 2,
+ "memory": "1Gi"
+ }
+ }
+ ]`
+ // without minMember
+ testGroupErr3 := `
+ [
+ {
+ "name": "test-group-err-2",
+ "minResource": {
+ "cpu": 2,
+ "memory": "1Gi"
+ }
+ }
+ ]`
+ // withot minResource
+ testGroupErr4 := `
+ [
+ {
+ "name": "test-group-err-4",
+ "minMember": 3,
+ }
+ ]`
+ // negative minMember
+ testGroupErr5 := `
+ [
+ {
+ "name": "test-group-err-5",
+ "minMember": -100,
Review Comment:
This example is designed for negative minMember, However this case is classified to parsing error when I tested
So we should remove the comma, or this err will be classified to parsing error.
"minMember": -100, to "minMember": -100
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org