minor errors in pod.go

[xfate123]

just some minor errors I found when I studied the code

[kubeflow-bot]

This change is 

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign richardsliu
You can assign the PR to them by writing /assign @richardsliu in a comment when ready.

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

Details

Needs approval from an approver in each of these files:

• OWNERS

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

[terrytangyuan]

This change is unnecessary

[gaocegege]

It is created by gofmt I think, maybe we can keep it.

[terrytangyuan]

I think this was intended and correct but we probably need to improve the log message here

[Jeffwan]

The original logic here is to provide flexibility to define your own retryable exit code. This should not be changed.

Since kubernetes doesn't have restart method, That's the reason we delete the pod, and wait for next reconcile loop to create a new one. It's asynchronous so restart here may be confused to users.

[merlintang]

good catch up

[Jeffwan]

@xfate123

Please check logics here.

common/pkg/controller.v1/common/job_controller.go

Lines 321 to 336 in ac0c6e1

	func (jc *JobController) resolveControllerRef(namespace string, controllerRef *metav1.OwnerReference) metav1.Object {
	// We can't look up by UID, so look up by Name and then verify UID.
	// Don't even try to look up by Name if it's the wrong Kind.
	if controllerRef.Kind != jc.Controller.GetAPIGroupVersionKind().Kind {
	return nil
	}
	job, err := jc.Controller.GetJobFromInformerCache(namespace, controllerRef.Name)
	if err != nil {
	return nil
	}
	if job.GetUID() != controllerRef.UID {
	// The controller we found with this Name is not the same one that the
	// ControllerRef points to.
	return nil
	}
	return job

If we can not find the job, we will end reconcile loop and directly return. The only thing matters is if we want to have some meaningful log.

There're several reason job == nil here

1. Kind doesn't match
2. GroupNameLabel doesn't match
3. Job doesn't exist. In this case, the pod is an orphan pod.

the 3rd one is the only case we want to persist the log. Does it make sense?

[xfate123]

@Jeffwan
Thank you so much Jeff. Much clearer after your patient explanation.
But I am still a little confused
I think the three cases should be:
1.Kind doesn't match
2.Name unmatched
3. Name matched but UID unmatched
My understanding is that third case cannot prove this pod is an orphan pod. It can only prove that the controllerRef point s to a different job.
Thank you again. Really appreciate your help
Thank you

[Jeffwan]

@xfate123 You are right. I didn't follow logic in resolveControllerRef. UID mismatch is one of the cases. We check GroupNameLabel again in caller side which I think is unnecessary. We use fixed value kubeflow.org for most of the operators.

My understanding is that third case cannot prove this pod is an orphan pod. It can only prove that the controllerRef point s to a different job.

I may not explain this clearly. you are right. It could be an orphan pod (job has been deleted - case 2) or a pod point to different job (UID mismatch - case 3). case 1 seems match the criterion as well.

[xfate123]

@Jeffwan Thank you so much for your clear explanation

[Jeffwan]

The original logic here is to provide flexibility to define your own retryable exit code. This should not be changed.

Since kubernetes doesn't have restart method, That's the reason we delete the pod, and wait for next reconcile loop to create a new one. It's asynchronous so restart here may be confused to users.

[Jeffwan]

@xfate123

Please check logics here.

common/pkg/controller.v1/common/job_controller.go

Lines 321 to 336 in ac0c6e1

	func (jc *JobController) resolveControllerRef(namespace string, controllerRef *metav1.OwnerReference) metav1.Object {
	// We can't look up by UID, so look up by Name and then verify UID.
	// Don't even try to look up by Name if it's the wrong Kind.
	if controllerRef.Kind != jc.Controller.GetAPIGroupVersionKind().Kind {
	return nil
	}
	job, err := jc.Controller.GetJobFromInformerCache(namespace, controllerRef.Name)
	if err != nil {
	return nil
	}
	if job.GetUID() != controllerRef.UID {
	// The controller we found with this Name is not the same one that the
	// ControllerRef points to.
	return nil
	}
	return job

If we can not find the job, we will end reconcile loop and directly return. The only thing matters is if we want to have some meaningful log.

There're several reason job == nil here

1. Kind doesn't match
2. GroupNameLabel doesn't match
3. Job doesn't exist. In this case, the pod is an orphan pod.

the 3rd one is the only case we want to persist the log. Does it make sense?