Bug 1916554: failing if rsync client pods are stuck #958

JaydipGabani · 2021-02-16T19:03:58Z

Failing the migration if Rsync client pods are stuck in "container creating" phase for more than 10 mins.

github-actions · 2021-02-16T19:04:16Z

valid bug 1916554

alaypatel07 · 2021-02-16T21:37:41Z

I was out of the office today. My gut feeling is that this bug has manifested in many forms, we need to create a generic solution and get some unit tests in. I will take a detailed look tomorrow morning

alaypatel07 · 2021-02-17T19:25:27Z

pkg/controller/directvolumemigrationprogress/directvolumemigrationprogress_controller.go

+	case !containerStatus.Ready && containerStatus.State.Waiting != nil && containerStatus.State.Waiting.Reason == ContainerCreating:
+		// if pod is not in running state after 10 mins of its creation, assume the pod is in failed state
+		if time.Now().Sub(pod.CreationTimestamp.Time) > 10*time.Minute {
+			pvProgress.Status.PodPhase = kapi.PodFailed


do we really want this?

Ideally, we should raise a warning right? that says that a pod is in creating the state since 10 mins?

+1 to rasing warning

dymurray · 2021-02-19T21:20:40Z

pkg/controller/directvolumemigrationprogress/directvolumemigrationprogress_controller.go

+			pvProgress.Status.PodPhase = kapi.PodPending
+			pvProgress.Status.LogMessage = fmt.Sprintf("Pod %s/%s is stuck in ContainerCreating for more than 10 mins", pod.Name, pod.Namespace)
+			pvProgress.Status.ContainerElapsedTime = &metav1.Duration{Duration: time.Now().Sub(pod.CreationTimestamp.Time).Round(time.Second)}
+			//pvProgress.Status.SetCondition(migapi.Condition{


Any reason we are keeping this commented out?

Forgot to delete it

dymurray · 2021-02-19T21:20:51Z

pkg/controller/directvolumemigration/task.go

@@ -452,6 +452,15 @@ func (t *Task) Run() error {
 				return liberr.Wrap(err)
 			}
 		} else {
+			//if pending {
+			//	t.Owner.Status.SetCondition(migapi.Condition{


Why is this commented out?

Not needed removing

dymurray · 2021-02-19T21:22:56Z

pkg/controller/directvolumemigration/rsync.go

+					LastObservedProgressPercent: dvmp.Status.LastObservedProgressPercent,
+					LastObservedTransferRate:    dvmp.Status.LastObservedTransferRate,
+				})
+				pendingMessage += dvmp.Status.LogMessage + " "


So this is grabbing the logs from the status field and setting it as a warning message in the status condition? is that right?

Yes! Since there could be multiple pods (each for one dvmp), that might be in the pending state for DVM, to me aggregating logMessage from each such dvmp and raising one warning in DVM makes more sense.

Okay, my only concern is wouldn't this mean if I have multiple pods that are pending it will look like one long log message but will really be multiple pods logs?

Could we separate the logs with a line indicating its a new pod? Or am I missing something?

good catch! starting new log from new line. There is only one logMessage per pod for this situation, so starting each from new line would make it less confusing.

dymurray · 2021-02-19T21:23:30Z

pkg/controller/directvolumemigration/rsync.go

@@ -1166,11 +1166,11 @@ func getMD5Hash(s string) string {
 	return hex.EncodeToString(hash[:])
 }

-func (t *Task) haveRsyncClientPodsCompletedOrFailed() (bool, bool, error) {
+func (t *Task) haveRsyncClientPodsCompletedOrFailed() (bool, bool, bool, error) {


Consider renaming function. This is now returning 3 booleans so name doesn't make much sense any longer

+1, updated the name

dymurray

LGTM

alaypatel07 · 2021-02-23T17:12:11Z

pkg/controller/directvolumemigrationprogress/directvolumemigrationprogress_controller.go

@@ -276,6 +284,13 @@ func (r *ReconcileDirectVolumeMigrationProgress) reportContainerStatus(pvProgres
 		exitCode := containerStatus.LastTerminationState.Terminated.ExitCode
 		pvProgress.Status.ExitCode = &exitCode
 		pvProgress.Status.ContainerElapsedTime = &metav1.Duration{Duration: containerStatus.LastTerminationState.Terminated.FinishedAt.Sub(containerStatus.LastTerminationState.Terminated.StartedAt.Time).Round(time.Second)}
+	case !containerStatus.Ready && pod.Status.Phase == kapi.PodPending && containerStatus.State.Waiting != nil && containerStatus.State.Waiting.Reason == ContainerCreating:
+		// if pod is not in running state after 10 mins of its creation, raise a
+		if time.Now().Sub(pod.CreationTimestamp.Time) > TimeLimit {


NIT: do we want time.Now().UTC().Sub(pod.CreationTimestamp.Time.UTC()) instead of time.Now().Sub(pod.CreationTimestamp.Time) just to be sure the timezone is common?

alaypatel07 · 2021-02-23T17:21:54Z

pkg/controller/directvolumemigration/rsync.go

+			Status:   migapi.True,
+			Reason:   "PodStuckInContainerCreating",
+			Category: migapi.Warn,
+			Message:  pendingMessage,


I wonder if there is some amount of redundancy here?

Reason: PodStuckInContainerCreating message: fmt.Sprintf("Pod %s/%s is stuck in ContainerCreating for more than 10 mins\n", pod.Namespace, pod.Name)

I expect just the list of name of the pods in the message or something like 9/15 pods are stuck, something more general, less repetitive

a message with Name/namespace of each pod that is in the pending state is more user friendly I think

alaypatel07 · 2021-02-23T18:44:28Z

pkg/controller/directvolumemigration/rsync.go

+					LastObservedProgressPercent: dvmp.Status.LastObservedProgressPercent,
+					LastObservedTransferRate:    dvmp.Status.LastObservedTransferRate,
+				})
+				pendingPods = append(pendingPods, strings.Split(dvmp.Status.LogMessage, " ")[1])


Failing to understand why this is not as simple as obj.Name and object.Namespace? What am I missing?

Ahh, missed that obj has these two pieces of info.

alaypatel07 · 2021-02-23T18:45:03Z

pkg/controller/directvolumemigrationprogress/directvolumemigrationprogress_controller.go

+		// if pod is not in running state after 10 mins of its creation, raise a
+		if time.Now().UTC().Sub(pod.CreationTimestamp.Time.UTC()) > TimeLimit {
+			pvProgress.Status.PodPhase = kapi.PodPending
+			pvProgress.Status.LogMessage = fmt.Sprintf("Pod %s/%s is stuck in ContainerCreating for more than 10 mins", pod.Namespace, pod.Name)


with the above do we really need this?

We don't, but there is no harm in keeping this information in dvmp as well.

alaypatel07 · 2021-02-23T23:40:31Z

pkg/controller/directvolumemigration/task.go

 		if err != nil {
 			return liberr.Wrap(err)
 		}
-		if completed {
+		if completed && !pending {


why do you need pending here? if the pods are in a pending state doesn't that automatically mean they are not completed?

Yip You are on the right track. removed the additional variable since it is not neeed

…ner for more than 10 mins adding support to warn user if client pods are in pending state using a variable to define time limit adding warning condition in dvm removing comments and renaming function adding new line and following naming conventions scrapping repeatetive stmts removing not needed variables making the message more generalized

…ner for more than 10 mins (migtools#958) adding support to warn user if client pods are in pending state using a variable to define time limit adding warning condition in dvm removing comments and renaming function adding new line and following naming conventions scrapping repeatetive stmts removing not needed variables making the message more generalized (cherry picked from commit c1162b5)

…ner for more than 10 mins (#958) (#972) adding support to warn user if client pods are in pending state using a variable to define time limit adding warning condition in dvm removing comments and renaming function adding new line and following naming conventions scrapping repeatetive stmts removing not needed variables making the message more generalized (cherry picked from commit c1162b5)

JaydipGabani requested review from alaypatel07 and dymurray February 16, 2021 19:04

alaypatel07 reviewed Feb 17, 2021

View reviewed changes

JaydipGabani requested a review from alaypatel07 February 18, 2021 19:28

JaydipGabani force-pushed the bz-1916554 branch from fb974ed to 95f430f Compare February 19, 2021 15:35

dymurray reviewed Feb 19, 2021

View reviewed changes

JaydipGabani force-pushed the bz-1916554 branch from 95f430f to 62714a6 Compare February 20, 2021 00:03

JaydipGabani requested a review from dymurray February 20, 2021 00:15

JaydipGabani force-pushed the bz-1916554 branch from 62714a6 to 343a37f Compare February 22, 2021 17:45

dymurray approved these changes Feb 22, 2021

View reviewed changes

alaypatel07 reviewed Feb 23, 2021

View reviewed changes

JaydipGabani force-pushed the bz-1916554 branch from 7543d48 to 0d98d4e Compare February 23, 2021 18:31

alaypatel07 reviewed Feb 23, 2021

View reviewed changes

JaydipGabani force-pushed the bz-1916554 branch from 0d98d4e to 04ea2b4 Compare February 23, 2021 18:51

JaydipGabani requested review from alaypatel07 and dymurray February 23, 2021 20:24

alaypatel07 reviewed Feb 23, 2021

View reviewed changes

JaydipGabani requested a review from alaypatel07 February 24, 2021 15:15

JaydipGabani force-pushed the bz-1916554 branch from ae4abc5 to d0b0aee Compare February 24, 2021 16:36

JaydipGabani force-pushed the bz-1916554 branch from d0b0aee to 9f01aa4 Compare February 24, 2021 18:30

dymurray approved these changes Feb 24, 2021

View reviewed changes

JaydipGabani merged commit c1162b5 into migtools:master Feb 24, 2021

JaydipGabani mentioned this pull request Feb 24, 2021

cherry-pick fix for bz-1916554 #972

Merged

Bug 1916554: failing if rsync client pods are stuck #958

Bug 1916554: failing if rsync client pods are stuck #958

JaydipGabani commented Feb 16, 2021

github-actions bot commented Feb 16, 2021

alaypatel07 commented Feb 16, 2021 •

edited

alaypatel07 Feb 17, 2021

JaydipGabani Feb 18, 2021

dymurray Feb 19, 2021

JaydipGabani Feb 19, 2021

dymurray Feb 19, 2021

JaydipGabani Feb 19, 2021

dymurray Feb 19, 2021

JaydipGabani Feb 20, 2021

dymurray Feb 22, 2021

JaydipGabani Feb 22, 2021

dymurray Feb 19, 2021

JaydipGabani Feb 20, 2021

dymurray left a comment

alaypatel07 Feb 23, 2021

JaydipGabani Feb 23, 2021

alaypatel07 Feb 23, 2021

JaydipGabani Feb 23, 2021

alaypatel07 Feb 23, 2021

JaydipGabani Feb 23, 2021

alaypatel07 Feb 23, 2021

JaydipGabani Feb 23, 2021

alaypatel07 Feb 23, 2021

JaydipGabani Feb 24, 2021

Bug 1916554: failing if rsync client pods are stuck #958

Bug 1916554: failing if rsync client pods are stuck #958

Conversation

JaydipGabani commented Feb 16, 2021

github-actions bot commented Feb 16, 2021

alaypatel07 commented Feb 16, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dymurray left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alaypatel07 commented Feb 16, 2021 •

edited