From 08eb825ec0c6e776a3a8f44ed7790ef70e4da42e Mon Sep 17 00:00:00 2001
From: Yuqi Wang <yqwang@microsoft.com>
Date: Thu, 14 Feb 2019 18:34:09 +0800
Subject: [PATCH] Fix TaskComplete may transition to TaskAttemptCompleted

---
 pkg/apis/frameworkcontroller/v1/types.go | 16 +++++++++++-----
 pkg/controller/controller.go             | 15 ++++++++++++---
 2 files changed, 23 insertions(+), 8 deletions(-)

diff --git a/pkg/apis/frameworkcontroller/v1/types.go b/pkg/apis/frameworkcontroller/v1/types.go
index 9931e2a9..bd71c8a2 100644
--- a/pkg/apis/frameworkcontroller/v1/types.go
+++ b/pkg/apis/frameworkcontroller/v1/types.go
@@ -439,14 +439,17 @@ const (
 	FrameworkAttemptDeleting FrameworkState = "AttemptDeleting"
 
 	// ConfigMap does not exist and
-	// has been creation requested and is not expected to exist.
+	// has been creation requested and is not expected to exist and
+	// current attempt is not the last attempt or to be determined.
 	// [AttemptFinalState]
 	// [AssociatedState]
 	// -> FrameworkAttemptCreationPending
 	// -> FrameworkCompleted
 	FrameworkAttemptCompleted FrameworkState = "AttemptCompleted"
 
-	// Last FrameworkAttempt is completed.
+	// ConfigMap does not exist and
+	// has been creation requested and is not expected to exist and
+	// current attempt is the last attempt.
 	// [FinalState]
 	// [AssociatedState]
 	FrameworkCompleted FrameworkState = "Completed"
@@ -480,8 +483,8 @@ const (
 	// has not been deletion requested and
 	// is PodPending or PodUnknown afterwards.
 	// [AssociatedState]
-	// -> TaskAttemptDeletionPending
 	// -> TaskAttemptRunning
+	// -> TaskAttemptDeletionPending
 	// -> TaskAttemptDeleting
 	// -> TaskAttemptCompleted
 	TaskAttemptPreparing TaskState = "AttemptPreparing"
@@ -517,14 +520,17 @@ const (
 	TaskAttemptDeleting TaskState = "AttemptDeleting"
 
 	// Pod does not exist and
-	// has been creation requested and is not expected to exist.
+	// has been creation requested and is not expected to exist and
+	// current attempt is not the last attempt or to be determined.
 	// [AttemptFinalState]
 	// [AssociatedState]
 	// -> TaskAttemptCreationPending
 	// -> TaskCompleted
 	TaskAttemptCompleted TaskState = "AttemptCompleted"
 
-	// Last TaskAttempt is completed.
+	// Pod does not exist and
+	// has been creation requested and is not expected to exist and
+	// current attempt is the last attempt.
 	// [FinalState]
 	// [AssociatedState]
 	TaskCompleted TaskState = "Completed"
diff --git a/pkg/controller/controller.go b/pkg/controller/controller.go
index 26ec73d9..1a49ac59 100644
--- a/pkg/controller/controller.go
+++ b/pkg/controller/controller.go
@@ -991,6 +991,17 @@ func (c *FrameworkController) syncTaskState(
 	taskRoleStatus := f.TaskRoleStatus(taskRoleName)
 	taskStatus := f.TaskStatus(taskRoleName, taskIndex)
 
+	if taskStatus.State == ci.TaskCompleted {
+		// The TaskCompleted should not trigger FrameworkAttemptDeletionPending, so
+		// it is safe to skip the attemptToCompleteFrameworkAttempt.
+		// Otherwise, given it is impossible that the TaskCompleted is persisted
+		// but the FrameworkAttemptDeletionPending is not persisted, the TaskCompleted
+		// should have already triggered and persisted FrameworkAttemptDeletionPending
+		// in previous sync, so current sync should have already been skipped but not.
+		log.Infof(logPfx + "Skipped: Task is already completed")
+		return false, nil
+	}
+
 	// Get the ground truth readonly pod
 	pod, err := c.getOrCleanupPod(f, cm, taskRoleName, taskIndex)
 	if err != nil {
@@ -1156,7 +1167,7 @@ func (c *FrameworkController) syncTaskState(
 		}
 	}
 	// At this point, taskStatus.State must be in:
-	// {TaskCompleted, TaskAttemptCreationPending, TaskAttemptCompleted}
+	// {TaskAttemptCreationPending, TaskAttemptCompleted}
 
 	if taskStatus.State == ci.TaskAttemptCompleted {
 		// attemptToRetryTask
@@ -1207,8 +1218,6 @@ func (c *FrameworkController) syncTaskState(
 	// At this point, taskStatus.State must be in:
 	// {TaskCompleted, TaskAttemptCreationPending}
 
-	// Totally reconstruct actions triggered by TaskCompleted in case these actions
-	// are missed due to FrameworkController restart.
 	if taskStatus.State == ci.TaskCompleted {
 		// attemptToCompleteFrameworkAttempt
 		completionPolicy := taskRoleSpec.FrameworkAttemptCompletionPolicy