Inconsistent implementation about when the validation of job's spec failed

[HeGaoYuan]

In below codes, when validate the job's spec failed, the process is different. The MPIJob will return an err, so the MPIJob will not continue to creating corresponding pods/services, it will try again after some time. The PytorchJob/TFJob will just print an error log then continue, but it maybe cause unexpected results in the future.

I think we need to discuss what exactly we should do when we validate job's spec failed then we apply it to all Jobs. In my opinion, it should not continue after validating job's spec failed, and we not only to print error log, but also need to record a warning event so that users can know why their Job is blocking through kubectl describe XXJob.

Referring to point4 of #1703

training-operator/pkg/controller.v1/mpi/mpijob_controller.go

Lines 135 to 138 in 82af677

	if err = kubeflowv1.ValidateV1MpiJobSpec(&mpijob.Spec); err != nil {
	logger.Info(err.Error(), "MPIJob failed validation", req.NamespacedName.String())
	return ctrl.Result{}, err
	}

training-operator/pkg/controller.v1/pytorch/pytorchjob_controller.go

Lines 133 to 135 in 82af677

	if err = kubeflowv1.ValidateV1PyTorchJobSpec(&pytorchjob.Spec); err != nil {
	logger.Info(err.Error(), "PyTorchJob failed validation", req.NamespacedName.String())
	}

training-operator/pkg/controller.v1/tensorflow/tfjob_controller.go

Lines 158 to 160 in 82af677

	if err = kubeflowv1.ValidateV1TFJobSpec(&tfjob.Spec); err != nil {
	logger.Info(err.Error(), "TFJob failed validation", req.NamespacedName.String())
	}

[johnugeorge]

Thanks for reporting.

Yes. we should not continue if validation fails. Also, recording a warning event is a great idea. Can you fix this?

[johnugeorge]

/cc @gaocegege @terrytangyuan

[terrytangyuan]

Yes, I think MPI controller is doing it correctly.

[johnugeorge]

@terrytangyuan Since error is returned when Validation fails in MPI, reconcile function will be called again. Ref: #1705 (comment)

[tenzen-y]

Maybe this issue does not complete.
/reopen

[google-oss-prow]

@tenzen-y: Reopened this issue.

In response to this:

It looks like not to complete this issue.
/reopen

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.

[github-actions]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[tenzen-y]

/lifecycle frozen