From c8fa229e66410fb8ac9c585f6306a462b3ae6763 Mon Sep 17 00:00:00 2001 From: Yu-Lin Chen Date: Fri, 30 Aug 2024 12:09:48 +0000 Subject: [PATCH] code review: adjust log message and emit pod event --- pkg/cache/task.go | 10 ++++++++-- pkg/common/constants/constants.go | 4 ++++ pkg/common/utils/utils.go | 22 ++++++++++++---------- pkg/common/utils/utils_test.go | 17 +++++++---------- 4 files changed, 31 insertions(+), 22 deletions(-) diff --git a/pkg/cache/task.go b/pkg/cache/task.go index 5b20cc0ac..e2d4c4e01 100644 --- a/pkg/cache/task.go +++ b/pkg/cache/task.go @@ -536,16 +536,22 @@ func (task *Task) sanityCheckBeforeScheduling() error { // After version 1.7.0, we should reject the task whose pod is unbound and has conflicting metadata. if !utils.PodAlreadyBound(task.pod) { if err := utils.CheckAppIdInPod(task.pod); err != nil { - log.Log(log.ShimCacheTask).Warn("The task has conflicting metadata will be rejected after version 1.7.0.", + log.Log(log.ShimCacheTask).Warn("Pod has inconsistent application metadata and may be rejected in a future YuniKorn release.", zap.String("appID", task.applicationID), zap.String("podName", task.pod.Name), zap.String("error", err.Error())) + + events.GetRecorder().Eventf(task.pod.DeepCopy(), + nil, v1.EventTypeWarning, "Scheduling", "Scheduling", fmt.Sprintf("Pod has inconsistent application metadata and may be rejected in a future YuniKorn release: %s", err.Error())) } if err := utils.CheckQueueNameInPod(task.pod); err != nil { - log.Log(log.ShimCacheTask).Warn("The task has conflicting metadata will be rejected after version 1.7.0.", + log.Log(log.ShimCacheTask).Warn("Pod has inconsistent queue metadata and may be rejected in a future YuniKorn release.", zap.String("appID", task.applicationID), zap.String("podName", task.pod.Name), zap.String("error", err.Error())) + + events.GetRecorder().Eventf(task.pod.DeepCopy(), + nil, v1.EventTypeWarning, "Scheduling", "Scheduling", fmt.Sprintf("Pod has inconsistent queue metadata and may be rejected in a future YuniKorn release: %s", err.Error())) } } return task.checkPodPVCs() diff --git a/pkg/common/constants/constants.go b/pkg/common/constants/constants.go index 8ddbc7eac..5a848b4af 100644 --- a/pkg/common/constants/constants.go +++ b/pkg/common/constants/constants.go @@ -26,6 +26,10 @@ import ( const True = "true" const False = "false" +// Kubernetes +const Label = "label" +const Annotation = "annotation" + // Cluster const DefaultNodeAttributeHostNameKey = "si.io/hostname" const DefaultNodeAttributeRackNameKey = "si.io/rackname" diff --git a/pkg/common/utils/utils.go b/pkg/common/utils/utils.go index 32e7ef7ea..f58c96e53 100644 --- a/pkg/common/utils/utils.go +++ b/pkg/common/utils/utils.go @@ -214,44 +214,46 @@ func GetApplicationIDFromPod(pod *v1.Pod) string { } func CheckAppIdInPod(pod *v1.Pod) error { - if err := ValidatePodLabelAnnotation(pod, constants.AppIdLabelKeys, constants.AppIdAnnotationKeys); err != nil { - return fmt.Errorf("pod has inconsistent application ID in labels and annotations. %w", err) - } - return nil + return ValidatePodLabelAnnotation(pod, constants.AppIdLabelKeys, constants.AppIdAnnotationKeys) } func CheckQueueNameInPod(pod *v1.Pod) error { - if err := ValidatePodLabelAnnotation(pod, constants.QueueLabelKeys, constants.QueueAnnotationKeys); err != nil { - return fmt.Errorf("pod has inconsistent queue name in labels and annotations. %w", err) - } - return nil + return ValidatePodLabelAnnotation(pod, constants.QueueLabelKeys, constants.QueueAnnotationKeys) } // return true if all non-empty values are same across all provided label/annotation func ValidatePodLabelAnnotation(pod *v1.Pod, labelKeys []string, annotationKeys []string) error { + var referenceKey string var referenceValue string + var referenceType string + checkingType := constants.Label for _, key := range labelKeys { value := GetPodLabelValue(pod, key) if value == "" { continue } if referenceValue == "" { + referenceKey = key referenceValue = value + referenceType = checkingType } else if referenceValue != value { - return fmt.Errorf("inconsistent values: %s, %s", referenceValue, value) + return fmt.Errorf("%s %s: \"%s\" doesn't match %s %s: \"%s\"", checkingType, key, value, referenceType, referenceKey, referenceValue) } } + checkingType = constants.Annotation for _, key := range annotationKeys { value := GetPodAnnotationValue(pod, key) if value == "" { continue } if referenceValue == "" { + referenceKey = key referenceValue = value + referenceType = checkingType } else if referenceValue != value { - return fmt.Errorf("inconsistent values: %s, %s", referenceValue, value) + return fmt.Errorf("%s %s: \"%s\" doesn't match %s %s: \"%s\"", checkingType, key, value, referenceType, referenceKey, referenceValue) } } diff --git a/pkg/common/utils/utils_test.go b/pkg/common/utils/utils_test.go index 0dbec3203..60dd1d482 100644 --- a/pkg/common/utils/utils_test.go +++ b/pkg/common/utils/utils_test.go @@ -725,7 +725,6 @@ func TestGetApplicationIDFromPod(t *testing.T) { } func TestCheckAppIdInPod(t *testing.T) { - mismatchError := errors.New("pod has inconsistent application ID") testCases := []struct { name string pod *v1.Pod @@ -757,7 +756,7 @@ func TestCheckAppIdInPod(t *testing.T) { }, }, }, - expected: mismatchError, + expected: errors.New("label spark-app-selector: \"app-456\" doesn't match label yunikorn.apache.org/app-id: \"app-123\""), }, { name: "inconsistent app ID between label and annotation", @@ -771,7 +770,7 @@ func TestCheckAppIdInPod(t *testing.T) { }, }, }, - expected: mismatchError, + expected: errors.New("annotation yunikorn.apache.org/app-id: \"app-456\" doesn't match label yunikorn.apache.org/app-id: \"app-123\""), }, } for _, tc := range testCases { @@ -787,7 +786,6 @@ func TestCheckAppIdInPod(t *testing.T) { } func TestCheckQueueNameInPod(t *testing.T) { - mismatchError := errors.New("pod has inconsistent queue name") testCases := []struct { name string pod *v1.Pod @@ -818,7 +816,7 @@ func TestCheckQueueNameInPod(t *testing.T) { }, }, }, - expected: mismatchError, + expected: errors.New("label queue: \"root.b\" doesn't match label yunikorn.apache.org/queue: \"root.a\""), }, { name: "inconsistent app ID between label and annotation", @@ -832,7 +830,7 @@ func TestCheckQueueNameInPod(t *testing.T) { }, }, }, - expected: mismatchError, + expected: errors.New("annotation yunikorn.apache.org/queue: \"root.b\" doesn't match label yunikorn.apache.org/queue: \"root.a\""), }, } for _, tc := range testCases { @@ -850,7 +848,6 @@ func TestCheckQueueNameInPod(t *testing.T) { func TestValidatePodLabelAnnotation(t *testing.T) { labelKeys := []string{"labelKey1", "labelKey2"} annotationKeys := []string{"annotationKey1", "annotationKey2"} - mismatchError := errors.New("inconsistent values: ") testCases := []struct { name string @@ -888,7 +885,7 @@ func TestValidatePodLabelAnnotation(t *testing.T) { }, }, }, - expected: mismatchError, + expected: errors.New("label labelKey2: \"value2\" doesn't match label labelKey1: \"value1\""), }, { name: "pod with inconsistent value between label and annotation", @@ -902,7 +899,7 @@ func TestValidatePodLabelAnnotation(t *testing.T) { }, }, }, - expected: mismatchError, + expected: errors.New("annotation annotationKey1: \"value2\" doesn't match label labelKey1: \"value1\""), }, { name: "pod with inconsistent value in annotations", @@ -914,7 +911,7 @@ func TestValidatePodLabelAnnotation(t *testing.T) { }, }, }, - expected: mismatchError, + expected: errors.New("annotation annotationKey2: \"value2\" doesn't match annotation annotationKey1: \"value1\""), }, }