Enrich PodSpecError to early fail Pod

microsoft · yqwang-ms · Jan 17, 2020 · Jan 16, 2020 · Jan 16, 2020 · 624002bf73197e437b1e47fdef554f22e171e32c
commit 624002bf73197e437b1e47fdef554f22e171e32c
diff --git a/pkg/apis/frameworkcontroller/v1/completion.go b/pkg/apis/frameworkcontroller/v1/completion.go
@@ -68,7 +68,7 @@ const (
 	CompletionCodeConfigMapCreationTimeout CompletionCode = -110
 	CompletionCodePodCreationTimeout       CompletionCode = -111
 	// -2XX: Permanent Error
-	CompletionCodePodSpecInvalid             CompletionCode = -200
+	CompletionCodePodSpecPermanentError      CompletionCode = -200
 	CompletionCodeStopFrameworkRequested     CompletionCode = -210
 	CompletionCodeFrameworkAttemptCompletion CompletionCode = -220
 	// -3XX: Unknown Error
@@ -163,8 +163,8 @@ func initCompletionCodeInfos() {
 				[]CompletionTypeAttribute{CompletionTypeAttributeTransient}},
 		},
 		{
-			Code:   CompletionCodePodSpecInvalid.Ptr(),
-			Phrase: "PodSpecInvalid",
+			Code:   CompletionCodePodSpecPermanentError.Ptr(),
+			Phrase: "PodSpecPermanentError",
 			Type: CompletionType{CompletionTypeNameFailed,
 				[]CompletionTypeAttribute{CompletionTypeAttributePermanent}},
 		},

diff --git a/pkg/apis/frameworkcontroller/v1/funcs.go b/pkg/apis/frameworkcontroller/v1/funcs.go
@@ -537,8 +537,7 @@ func (rp RetryPolicySpec) ShouldRetry(
 	ct := cs.Type
 
 	// 0. Built-in Always-on RetryPolicy
-	if cs.Code == CompletionCodePodSpecInvalid ||
-		cs.Code == CompletionCodeStopFrameworkRequested ||
+	if cs.Code == CompletionCodeStopFrameworkRequested ||
 		cs.Code == CompletionCodeFrameworkAttemptCompletion {
 		return RetryDecision{false, true, 0, fmt.Sprintf(
 			"CompletionCode is %v, %v", cs.Code, cs.Phrase)}

diff --git a/pkg/apis/frameworkcontroller/v1/types.go b/pkg/apis/frameworkcontroller/v1/types.go
@@ -134,8 +134,7 @@ const (
 //    complete a single Task in the TaskRole.
 //
 // Usage:
-// If the Pod Spec is invalid or
-// the ExecutionType is ExecutionStop or
+// If the ExecutionType is ExecutionStop or
 // the Task's FrameworkAttempt is completing,
 //   will not retry.
 //

diff --git a/pkg/controller/controller.go b/pkg/controller/controller.go
@@ -1146,7 +1146,7 @@ func (c *FrameworkController) createConfigMap(
 
 	remoteCM, createErr := c.kClient.CoreV1().ConfigMaps(f.Namespace).Create(cm)
 	if createErr != nil {
-		if apiErrors.IsConflict(createErr) {
+		if apiErrors.IsAlreadyExists(createErr) {
 			// Best effort to judge if conflict with a not controlled object.
 			localCM, getErr := c.cmLister.ConfigMaps(f.Namespace).Get(cm.Name)
 			if getErr == nil && !meta.IsControlledBy(localCM, f) {
@@ -1431,9 +1431,9 @@ func (c *FrameworkController) syncTaskState(
 		pod, err = c.createPod(f, cm, taskRoleName, taskIndex)
 		if err != nil {
 			apiErr := errorWrap.Cause(err)
-			if apiErrors.IsInvalid(apiErr) {
+			if internal.IsPodSpecPermanentError(apiErr) {
 				// Should be Framework Error instead of Platform Transient Error.
-				diag := fmt.Sprintf("%v", apiErr)
+				diag := fmt.Sprintf("Failed to create Pod: %v", common.ToJson(apiErr))
 				klog.Info(logPfx + diag)
 
 				// Ensure pod is deleted in remote to avoid managed pod leak after
@@ -1444,7 +1444,7 @@ func (c *FrameworkController) syncTaskState(
 				}
 
 				c.completeTaskAttempt(f, taskRoleName, taskIndex, true,
-					ci.CompletionCodePodSpecInvalid.
+					ci.CompletionCodePodSpecPermanentError.
 						NewTaskAttemptCompletionStatus(diag, nil))
 				return nil
 			} else {
@@ -1702,7 +1702,7 @@ func (c *FrameworkController) createPod(
 
 	remotePod, createErr := c.kClient.CoreV1().Pods(f.Namespace).Create(pod)
 	if createErr != nil {
-		if apiErrors.IsConflict(createErr) {
+		if apiErrors.IsAlreadyExists(createErr) {
 			// Best effort to judge if conflict with a not controlled object.
 			localPod, getErr := c.podLister.Pods(f.Namespace).Get(pod.Name)
 			if getErr == nil && !meta.IsControlledBy(localPod, cm) {

diff --git a/pkg/internal/utils.go b/pkg/internal/utils.go
@@ -38,6 +38,7 @@ import (
 	"k8s.io/client-go/tools/cache"
 	"k8s.io/klog"
 	"reflect"
+	"strings"
 	"time"
 )
 
@@ -221,3 +222,11 @@ func GetPodDeletionStartTime(pod *core.Pod) *meta.Time {
 	}
 	return common.PtrTime(meta.NewTime(pod.DeletionTimestamp.Add(-gracePeriod)))
 }
+
+func IsPodSpecPermanentError(apiErr error) bool {
+	return apiErrors.IsBadRequest(apiErr) ||
+		apiErrors.IsInvalid(apiErr) ||
+		apiErrors.IsRequestEntityTooLargeError(apiErr) ||
+		(apiErrors.IsForbidden(apiErr) &&
+			!strings.Contains(apiErr.Error(), "exceeded quota"))
+}