Repository: kubeflow/training-operator (language: python)

Issue:
The default value for CleanPodPolicy is inconsistent.
In the comments of common, the default value is Running：
```
type RunPolicy struct {
	// CleanPodPolicy defines the policy to kill pods after the job completes.
	// Default to Running.
	CleanPodPolicy *CleanPodPolicy `json:"cleanPodPolicy,omitempty"`
```
In actuality：
- mpiJob's default cleanpod policy is None.
- mxJob's default cleanpod policy is All.
- paddleJob's default cleanpod policy is None.
- pytorchJob's default cleanpod policy is None.
- tfJob's default cleanpod policy is Running.
- XGBoostJob's default cleanpod policy is All.

Should we set all jobs default to Running, or remove the comments(// Default to Running.) in common.




Discussion / hints:
Thanks for reporting this.

Yes. This has to be consistent.  I think, we can keep defaults to None IMO. 

/cc @terrytangyuan @gaocegege  @tenzen-y  
I agree. We should set `None` as default. Setting `Running` as default looks weird to me.

Sounds good 

Fix patch:
diff --git a/pkg/apis/kubeflow.org/v1/defaulting_utils.go b/pkg/apis/kubeflow.org/v1/defaulting_utils.go
index 538eb82d66..8898b5dc07 100644
--- a/pkg/apis/kubeflow.org/v1/defaulting_utils.go
+++ b/pkg/apis/kubeflow.org/v1/defaulting_utils.go
@@ -58,3 +58,7 @@ func setTypeNameToCamelCase(replicaSpecs map[commonv1.ReplicaType]*commonv1.Repl
 		}
 	}
 }
+
+func cleanPodPolicyPointer(cleanPodPolicy commonv1.CleanPodPolicy) *commonv1.CleanPodPolicy {
+	return &cleanPodPolicy
+}
diff --git a/pkg/apis/kubeflow.org/v1/mxnet_defaults.go b/pkg/apis/kubeflow.org/v1/mxnet_defaults.go
index 235fcf8cad..2c684c9668 100644
--- a/pkg/apis/kubeflow.org/v1/mxnet_defaults.go
+++ b/pkg/apis/kubeflow.org/v1/mxnet_defaults.go
@@ -46,9 +46,9 @@ func setMXNetTypeNamesToCamelCase(mxJob *MXJob) {
 
 // SetDefaults_MXJob sets any unspecified values to defaults.
 func SetDefaults_MXJob(mxjob *MXJob) {
-	// Set default cleanpod policy to All.
+	// Set default cleanpod policy to None.
 	if mxjob.Spec.RunPolicy.CleanPodPolicy == nil {
-		all := commonv1.CleanPodPolicyAll
+		all := commonv1.CleanPodPolicyNone
 		mxjob.Spec.RunPolicy.CleanPodPolicy = &all
 	}
 
diff --git a/pkg/apis/kubeflow.org/v1/tensorflow_defaults.go b/pkg/apis/kubeflow.org/v1/tensorflow_defaults.go
index 9ffa758473..88ae0c1c6b 100644
--- a/pkg/apis/kubeflow.org/v1/tensorflow_defaults.go
+++ b/pkg/apis/kubeflow.org/v1/tensorflow_defaults.go
@@ -50,9 +50,9 @@ func setTensorflowTypeNamesToCamelCase(tfJob *TFJob) {
 
 // SetDefaults_TFJob sets any unspecified values to defaults.
 func SetDefaults_TFJob(tfJob *TFJob) {
-	// Set default cleanpod policy to Running.
+	// Set default cleanpod policy to None.
 	if tfJob.Spec.RunPolicy.CleanPodPolicy == nil {
-		running := commonv1.CleanPodPolicyRunning
+		running := commonv1.CleanPodPolicyNone
 		tfJob.Spec.RunPolicy.CleanPodPolicy = &running
 	}
 	// Set default success policy to "".
diff --git a/pkg/apis/kubeflow.org/v1/xgboost_defaults.go b/pkg/apis/kubeflow.org/v1/xgboost_defaults.go
index 227bb90fb7..79cddf65c3 100644
--- a/pkg/apis/kubeflow.org/v1/xgboost_defaults.go
+++ b/pkg/apis/kubeflow.org/v1/xgboost_defaults.go
@@ -46,9 +46,9 @@ func setXGBoostJobTypeNamesToCamelCase(xgboostJob *XGBoostJob) {
 
 // SetDefaults_XGBoostJob sets any unspecified values to defaults.
 func SetDefaults_XGBoostJob(xgboostJob *XGBoostJob) {
-	// Set default cleanpod policy to All.
+	// Set default cleanpod policy to None.
 	if xgboostJob.Spec.RunPolicy.CleanPodPolicy == nil {
-		all := commonv1.CleanPodPolicyAll
+		all := commonv1.CleanPodPolicyNone
 		xgboostJob.Spec.RunPolicy.CleanPodPolicy = &all
 	}
 


Test patch:
diff --git a/pkg/apis/kubeflow.org/v1/mxnet_defaults_test.go b/pkg/apis/kubeflow.org/v1/mxnet_defaults_test.go
index 7d2a277e92..0e5ba92581 100644
--- a/pkg/apis/kubeflow.org/v1/mxnet_defaults_test.go
+++ b/pkg/apis/kubeflow.org/v1/mxnet_defaults_test.go
@@ -96,7 +96,7 @@ func TestSetDefaults_MXJob(t *testing.T) {
 					},
 				},
 			},
-			expected: expectedMXNetJob(commonv1.CleanPodPolicyAll, MXJobDefaultRestartPolicy, 1, MXJobDefaultPortName, MXJobDefaultPort),
+			expected: expectedMXNetJob(commonv1.CleanPodPolicyNone, MXJobDefaultRestartPolicy, 1, MXJobDefaultPortName, MXJobDefaultPort),
 		},
 		"Set spec with restart policy": {
 			original: &MXJob{
@@ -118,7 +118,7 @@ func TestSetDefaults_MXJob(t *testing.T) {
 					},
 				},
 			},
-			expected: expectedMXNetJob(commonv1.CleanPodPolicyAll, commonv1.RestartPolicyOnFailure, 1, MXJobDefaultPortName, MXJobDefaultPort),
+			expected: expectedMXNetJob(commonv1.CleanPodPolicyNone, commonv1.RestartPolicyOnFailure, 1, MXJobDefaultPortName, MXJobDefaultPort),
 		},
 		"Set spec with replicas": {
 			original: &MXJob{
@@ -140,7 +140,7 @@ func TestSetDefaults_MXJob(t *testing.T) {
 					},
 				},
 			},
-			expected: expectedMXNetJob(commonv1.CleanPodPolicyAll, MXJobDefaultRestartPolicy, 3, MXJobDefaultPortName, MXJobDefaultPort),
+			expected: expectedMXNetJob(commonv1.CleanPodPolicyNone, MXJobDefaultRestartPolicy, 3, MXJobDefaultPortName, MXJobDefaultPort),
 		},
 
 		"Set spec with default node port name and port": {
@@ -168,7 +168,7 @@ func TestSetDefaults_MXJob(t *testing.T) {
 					},
 				},
 			},
-			expected: expectedMXNetJob(commonv1.CleanPodPolicyAll, MXJobDefaultRestartPolicy, 1, MXJobDefaultPortName, MXJobDefaultPort),
+			expected: expectedMXNetJob(commonv1.CleanPodPolicyNone, MXJobDefaultRestartPolicy, 1, MXJobDefaultPortName, MXJobDefaultPort),
 		},
 
 		"Set spec with node port": {
@@ -196,7 +196,32 @@ func TestSetDefaults_MXJob(t *testing.T) {
 					},
 				},
 			},
-			expected: expectedMXNetJob(commonv1.CleanPodPolicyAll, MXJobDefaultRestartPolicy, 1, MXJobDefaultPortName, 9999),
+			expected: expectedMXNetJob(commonv1.CleanPodPolicyNone, MXJobDefaultRestartPolicy, 1, MXJobDefaultPortName, 9999),
+		},
+
+		"set spec with cleanpod policy": {
+			original: &MXJob{
+				Spec: MXJobSpec{
+					RunPolicy: commonv1.RunPolicy{
+						CleanPodPolicy: cleanPodPolicyPointer(commonv1.CleanPodPolicyAll),
+					},
+					MXReplicaSpecs: map[commonv1.ReplicaType]*commonv1.ReplicaSpec{
+						MXJobReplicaTypeWorker: &commonv1.ReplicaSpec{
+							Template: corev1.PodTemplateSpec{
+								Spec: corev1.PodSpec{
+									Containers: []corev1.Container{
+										corev1.Container{
+											Name:  MXJobDefaultContainerName,
+											Image: testImage,
+										},
+									},
+								},
+							},
+						},
+					},
+				},
+			},
+			expected: expectedMXNetJob(commonv1.CleanPodPolicyAll, MXJobDefaultRestartPolicy, 1, MXJobDefaultPortName, MXJobDefaultPort),
 		},
 	}
 
diff --git a/pkg/apis/kubeflow.org/v1/tensorflow_defaults_test.go b/pkg/apis/kubeflow.org/v1/tensorflow_defaults_test.go
index 49361c84c0..3fa2236490 100644
--- a/pkg/apis/kubeflow.org/v1/tensorflow_defaults_test.go
+++ b/pkg/apis/kubeflow.org/v1/tensorflow_defaults_test.go
@@ -151,7 +151,7 @@ func TestSetDefaultTFJob(t *testing.T) {
 					},
 				},
 			},
-			expected: expectedTFJob(commonv1.CleanPodPolicyRunning, customRestartPolicy, TFJobDefaultPortName, TFJobDefaultPort),
+			expected: expectedTFJob(commonv1.CleanPodPolicyNone, customRestartPolicy, TFJobDefaultPortName, TFJobDefaultPort),
 		},
 		"set replicas with default restartpolicy": {
 			original: &TFJob{
@@ -178,7 +178,7 @@ func TestSetDefaultTFJob(t *testing.T) {
 					},
 				},
 			},
-			expected: expectedTFJob(commonv1.CleanPodPolicyRunning, TFJobDefaultRestartPolicy, TFJobDefaultPortName, TFJobDefaultPort),
+			expected: expectedTFJob(commonv1.CleanPodPolicyNone, TFJobDefaultRestartPolicy, TFJobDefaultPortName, TFJobDefaultPort),
 		},
 		"set replicas with default port": {
 			original: &TFJob{
@@ -201,7 +201,7 @@ func TestSetDefaultTFJob(t *testing.T) {
 					},
 				},
 			},
-			expected: expectedTFJob(commonv1.CleanPodPolicyRunning, customRestartPolicy, "", 0),
+			expected: expectedTFJob(commonv1.CleanPodPolicyNone, customRestartPolicy, "", 0),
 		},
 		"set replicas adding default port": {
 			original: &TFJob{
@@ -230,7 +230,7 @@ func TestSetDefaultTFJob(t *testing.T) {
 					},
 				},
 			},
-			expected: expectedTFJob(commonv1.CleanPodPolicyRunning, customRestartPolicy, customPortName, customPort),
+			expected: expectedTFJob(commonv1.CleanPodPolicyNone, customRestartPolicy, customPortName, customPort),
 		},
 		"set custom cleanpod policy": {
 			original: &TFJob{
@@ -273,8 +273,3 @@ func TestSetDefaultTFJob(t *testing.T) {
 		}
 	}
 }
-
-func cleanPodPolicyPointer(cleanPodPolicy commonv1.CleanPodPolicy) *commonv1.CleanPodPolicy {
-	c := cleanPodPolicy
-	return &c
-}
diff --git a/pkg/apis/kubeflow.org/v1/xgboost_defaults_test.go b/pkg/apis/kubeflow.org/v1/xgboost_defaults_test.go
index b71093b853..329ad99eda 100644
--- a/pkg/apis/kubeflow.org/v1/xgboost_defaults_test.go
+++ b/pkg/apis/kubeflow.org/v1/xgboost_defaults_test.go
@@ -96,7 +96,7 @@ func TestSetDefaults_XGBoostJob(t *testing.T) {
 					},
 				},
 			},
-			expected: expectedXGBoostJob(commonv1.CleanPodPolicyAll, XGBoostJobDefaultRestartPolicy, 1, XGBoostJobDefaultPortName, XGBoostJobDefaultPort),
+			expected: expectedXGBoostJob(commonv1.CleanPodPolicyNone, XGBoostJobDefaultRestartPolicy, 1, XGBoostJobDefaultPortName, XGBoostJobDefaultPort),
 		},
 		"Set spec with restart policy": {
 			original: &XGBoostJob{
@@ -118,7 +118,7 @@ func TestSetDefaults_XGBoostJob(t *testing.T) {
 					},
 				},
 			},
-			expected: expectedXGBoostJob(commonv1.CleanPodPolicyAll, commonv1.RestartPolicyOnFailure, 1, XGBoostJobDefaultPortName, XGBoostJobDefaultPort),
+			expected: expectedXGBoostJob(commonv1.CleanPodPolicyNone, commonv1.RestartPolicyOnFailure, 1, XGBoostJobDefaultPortName, XGBoostJobDefaultPort),
 		},
 		"Set spec with replicas": {
 			original: &XGBoostJob{
@@ -140,7 +140,7 @@ func TestSetDefaults_XGBoostJob(t *testing.T) {
 					},
 				},
 			},
-			expected: expectedXGBoostJob(commonv1.CleanPodPolicyAll, XGBoostJobDefaultRestartPolicy, 3, XGBoostJobDefaultPortName, XGBoostJobDefaultPort),
+			expected: expectedXGBoostJob(commonv1.CleanPodPolicyNone, XGBoostJobDefaultRestartPolicy, 3, XGBoostJobDefaultPortName, XGBoostJobDefaultPort),
 		},
 
 		"Set spec with default node port name and port": {
@@ -168,7 +168,7 @@ func TestSetDefaults_XGBoostJob(t *testing.T) {
 					},
 				},
 			},
-			expected: expectedXGBoostJob(commonv1.CleanPodPolicyAll, XGBoostJobDefaultRestartPolicy, 1, XGBoostJobDefaultPortName, XGBoostJobDefaultPort),
+			expected: expectedXGBoostJob(commonv1.CleanPodPolicyNone, XGBoostJobDefaultRestartPolicy, 1, XGBoostJobDefaultPortName, XGBoostJobDefaultPort),
 		},
 
 		"Set spec with node port": {
@@ -196,7 +196,31 @@ func TestSetDefaults_XGBoostJob(t *testing.T) {
 					},
 				},
 			},
-			expected: expectedXGBoostJob(commonv1.CleanPodPolicyAll, XGBoostJobDefaultRestartPolicy, 1, XGBoostJobDefaultPortName, 9999),
+			expected: expectedXGBoostJob(commonv1.CleanPodPolicyNone, XGBoostJobDefaultRestartPolicy, 1, XGBoostJobDefaultPortName, 9999),
+		},
+		"set spec with cleanpod policy": {
+			original: &XGBoostJob{
+				Spec: XGBoostJobSpec{
+					RunPolicy: commonv1.RunPolicy{
+						CleanPodPolicy: cleanPodPolicyPointer(commonv1.CleanPodPolicyAll),
+					},
+					XGBReplicaSpecs: map[commonv1.ReplicaType]*commonv1.ReplicaSpec{
+						XGBoostJobReplicaTypeWorker: &commonv1.ReplicaSpec{
+							Template: corev1.PodTemplateSpec{
+								Spec: corev1.PodSpec{
+									Containers: []corev1.Container{
+										corev1.Container{
+											Name:  XGBoostJobDefaultContainerName,
+											Image: testImage,
+										},
+									},
+								},
+							},
+						},
+					},
+				},
+			},
+			expected: expectedXGBoostJob(commonv1.CleanPodPolicyAll, XGBoostJobDefaultRestartPolicy, 1, XGBoostJobDefaultPortName, XGBoostJobDefaultPort),
 		},
 	}