[UI] Fix Trial Logs when Kubernetes Job Fails

[andreyvelich]

I fixed UI backend that should return proper value when Kubernetes Job Fails. When Job Fails, it spawns multiple Pods and our UI shows this: More than one master replica found.

To fix this, I follow this:

1. If one Succeeded or Running Pod exists, we print the logs.
2. Otherwise, print logs from one of Failed Pods.

/assign @tenzen-y @kimwnasptd @johnugeorge @apo-ger @d-gol

[google-oss-prow]

@andreyvelich: GitHub didn't allow me to assign the following users: d-gol.

Note that only kubeflow members with read permissions, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

I fixed UI backend that should return proper value when Kubernetes Job Fails. When Job Fails, it spawns multiple Pods and our UI shows this: More than one master replica found.

To fix this, I follow this:

1. If one Succeeded or Running Pod exists, we print the logs.
2. Otherwise, print logs from one of Failed pods.

/assign @tenzen-y @kimwnasptd @johnugeorge @apo-ger @d-gol

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[tenzen-y]

@andreyvelich Thank you for fixing this bug!

[tenzen-y]

Suggested change

	// Otherwise, return the first Failed Pod.
	// Otherwise, return the first Failed or Pending Pod.
	}

Is this included in Pending Pods too, right?
So, should we return a failed pod if there are pending pods and failed pods in podList?

[andreyvelich]

Is this included in Pending Pods too, right?

Yes, that's right. In case of Pending pod, UI shows the similar error with my change:

One of the Pod containers is is ContainerCreating stage

@tenzen-y Do you think we should process only Failed pod after we processed Succeeded and Running (like you mentioned above: So, should we return a failed pod if there are pending pods and failed pods in podList?

And then, if there are no more Failed pods, just return podList.Items[0].Name (Pending Pod) and UI should show the above error ?

[tenzen-y]

Do you think we should process only Failed pod after we processed Succeeded and Running ?

Yes, I do. I think users may prefer that show logs for a failed pod rather than a pending pod. wdyt?

Also in the current implementation, If there are pending pods and failed pods, maybe, the selected pods will differ each time we run clientset.CoreV1().Pods(trial.ObjectMeta.Namespace).List....

Hence, for either a pending or failed pod, we should guarantee which phase this function returns.

And then, if there are no more Failed pods, just return podList.Items[0].Name (Pending Pod) and UI should show the above error ?

Uhm. I'm not sure the above message is appropriate. However, I don't think that pending means ContainerCreating. For example, when using a custom scheduler to enforce pending pods based on priority or requested resources, it will take a bit much time until is scheduled into a Node and starts.

[andreyvelich]

logs for a failed pod rather than a pending pod. wdyt

That makes sense.

The selected pods will differ each time we run clientset.CoreV1().Pods(trial.ObjectMeta.Namespace).

From my testing however, the [0] element is always the last created Pod (despite that list in client-go doesn't guarantee ordering).
I guess, that is how caching works behind list operation.

However, I don't think that pending means ContainerCreating

That is right, it can be different message I just added this as an example. We can always log this message to the user (when Pod is the Pending state): Failed to get logs for this Trial. Pod is in the Pending state. WDYT @tenzen-y ?

[tenzen-y]

From my testing however, the [0] element is always the last created Pod (despite that list in client-go doesn't guarantee ordering).
I guess, that is how caching works behind list operation.

It's interesting. Thanks for sharing the result of the investigation!

FYI: I guess the informer cache might affect the result. And the informer cache generally is rotated in intervals. IIRC, an interval is 10 hours as a default.

Failed to get logs for this Trial. Pod is in the Pending state.

Looks great :)

[andreyvelich]

@tenzen-y @johnugeorge I've changed it.

[tenzen-y]

Thanks!
/lgtm
/approve

[google-oss-prow]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andreyvelich, tenzen-y

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• pkg/new-ui/v1beta1/OWNERS [andreyvelich,tenzen-y]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment