[Bug Fix] Node timeout should use default value if set to zero. (flyt…

…eorg#121)

* [Bug Fix] Node timeout should use default value if set to zero.

* .

* feedback

* cr feedback

flytepropeller/pkg/controller/nodes/executor.go

@@ -214,17 +214,17 @@ func (c *nodeExecutor) execute(ctx context.Context, h handler.Node, nCtx *nodeEx
 	// check for timeout for non-terminal phases
 	if !phase.GetPhase().IsTerminal() {
 		activeDeadline := c.defaultActiveDeadline
-		if nCtx.Node().GetActiveDeadline() != nil {
+		if nCtx.Node().GetActiveDeadline() != nil && *nCtx.Node().GetActiveDeadline() > 0 {
 			activeDeadline = *nCtx.Node().GetActiveDeadline()
 		}
 		if c.isTimeoutExpired(nodeStatus.GetQueuedAt(), activeDeadline) {
 			logger.Errorf(ctx, "Node has timed out; timeout configured: %v", activeDeadline)
-			return handler.PhaseInfoTimedOut(nil, "active deadline elapsed"), nil
+			return handler.PhaseInfoTimedOut(nil, fmt.Sprintf("task active timeout [%s] expired", activeDeadline.String())), nil
 		}
 
 		// Execution timeout is a retry-able error
 		executionDeadline := c.defaultExecutionDeadline
-		if nCtx.Node().GetExecutionDeadline() != nil {
+		if nCtx.Node().GetExecutionDeadline() != nil && *nCtx.Node().GetExecutionDeadline() > 0 {
 			executionDeadline = *nCtx.Node().GetExecutionDeadline()
 		}
 		if c.isTimeoutExpired(nodeStatus.GetLastAttemptStartedAt(), executionDeadline) {

flytepropeller/pkg/controller/nodes/executor_test.go

@@ -1284,6 +1284,7 @@ func Test_nodeExecutor_timeout(t *testing.T) {
 		executionDeadline time.Duration
 		retries           int
 		err               error
+		expectedReason    string
 	}{
 		{
 			name:              "timeout",
@@ -1293,6 +1294,16 @@ func Test_nodeExecutor_timeout(t *testing.T) {
 			executionDeadline: time.Second * 5,
 			err:               nil,
 		},
+		{
+			name:              "default_execution_timeout",
+			phaseInfo:         handler.PhaseInfoRunning(nil),
+			expectedPhase:     handler.EPhaseRetryableFailure,
+			activeDeadline:    time.Second * 50,
+			executionDeadline: 0,
+			retries:           2,
+			err:               nil,
+			expectedReason:    "task execution timeout [1s] expired",
+		},
 		{
 			name:              "retryable-failure",
 			phaseInfo:         handler.PhaseInfoRunning(nil),
@@ -1348,7 +1359,7 @@ func Test_nodeExecutor_timeout(t *testing.T) {
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			c := &nodeExecutor{}
+			c := &nodeExecutor{defaultActiveDeadline: time.Second, defaultExecutionDeadline: time.Second}
 			handlerReturn := func() (handler.Transition, error) {
 				return handler.DoTransition(handler.TransitionTypeEphemeral, tt.phaseInfo), tt.err
 			}
@@ -1384,6 +1395,9 @@ func Test_nodeExecutor_timeout(t *testing.T) {
 				assert.NoError(t, err)
 			}
 			assert.Equal(t, tt.expectedPhase.String(), phaseInfo.GetPhase().String())
+			if tt.expectedReason != "" {
+				assert.Equal(t, tt.expectedReason, phaseInfo.GetReason())
+			}
 		})
 	}
 }

flytepropeller/pkg/controller/nodes/handler/transition_info.go

@@ -148,7 +148,7 @@ func phaseInfoFailed(p EPhase, err *core.ExecutionError, info *ExecutionInfo) Ph
 			Message: "Unknown error message",
 		}
 	}
-	return phaseInfo(p, err, info, "")
+	return phaseInfo(p, err, info, err.Message)
 }
 
 func PhaseInfoFailure(code, reason string, info *ExecutionInfo) PhaseInfo {