Fail job for non-retryable exit codes by kellyaa · Pull Request #2071 · kubeflow/trainer

kellyaa · 2024-04-18T16:34:43Z

Fixes #2044

Signed-off-by: Kelly A <kellyaa@users.noreply.github.com>

coveralls · 2024-04-18T19:07:11Z

Pull Request Test Coverage Report for Build 8850191138

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

0 of 21 (0.0%) changed or added relevant lines in 1 file are covered.
1 unchanged line in 1 file lost coverage.
Overall coverage decreased (-0.003%) to 35.303%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
pkg/controller.v1/common/pod.go	0	21	0.0%

Files with Coverage Reduction	New Missed Lines	%
pkg/controller.v1/common/pod.go	1	2.77%

Totals
Change from base Build 8839866996:	-0.003%
Covered Lines:	4380
Relevant Lines:	12407

💛 - Coveralls

tenzen-y

Thank you for creating this PR!
Could you add another test case here: https://github.com/kubeflow/training-operator/blob/7345e33b333ba5084127efe027774dd7bed8f6e6/pkg/controller.v1/tensorflow/pod_test.go?

I think that this is good example to implement another case: https://github.com/kubeflow/training-operator/blob/7345e33b333ba5084127efe027774dd7bed8f6e6/pkg/controller.v1/tensorflow/pod_test.go#L212-L275

tenzen-y · 2024-04-19T14:51:56Z

pkg/controller.v1/common/pod.go

+			} else if pod.Status.Phase == v1.PodFailed &&
+				(spec.RestartPolicy == apiv1.RestartPolicyExitCode && !trainutil.IsRetryableExitCode(exitCode)) {
+				logger.Infof("Pod has a non-retryable exit code. Failing job. %v %v", pod.Namespace, pod.Name)
+				msg := fmt.Sprintf("job %s is failing because %s replica(s) failed.",
+					metaObject.GetName(), rType)
+				jc.Recorder.Event(runtimeObject, v1.EventTypeWarning, commonutil.NewReason(jobKind, commonutil.JobFailedReason), msg)
+				commonutil.UpdateJobConditions(jobStatus, apiv1.JobFailed, v1.ConditionTrue, commonutil.NewReason(jobKind, commonutil.JobFailedReason), msg)


Could you improve debuggability?

Suggested change

} else if pod.Status.Phase == v1.PodFailed &&

(spec.RestartPolicy == apiv1.RestartPolicyExitCode && !trainutil.IsRetryableExitCode(exitCode)) {

logger.Infof("Pod has a non-retryable exit code. Failing job. %v %v", pod.Namespace, pod.Name)

msg := fmt.Sprintf("job %s is failing because %s replica(s) failed.",

metaObject.GetName(), rType)

jc.Recorder.Event(runtimeObject, v1.EventTypeWarning, commonutil.NewReason(jobKind, commonutil.JobFailedReason), msg)

commonutil.UpdateJobConditions(jobStatus, apiv1.JobFailed, v1.ConditionTrue, commonutil.NewReason(jobKind, commonutil.JobFailedReason), msg)

} else if pod.Status.Phase == v1.PodFailed &&

(spec.RestartPolicy == apiv1.RestartPolicyExitCode && !trainutil.IsRetryableExitCode(exitCode)) {

logger.Infof("Pod %q has a non-retryable exit code. Failing job.", klog.KObj(pod))

msg := fmt.Sprintf("job %q is failing because %q replica(s) failed.",

metaObject.GetName(), rType)

jc.Recorder.Event(runtimeObject, v1.EventTypeWarning, commonutil.NewReason(jobKind, commonutil.JobFailedReason), msg)

commonutil.UpdateJobConditions(jobStatus, apiv1.JobFailed, v1.ConditionTrue, commonutil.NewReason(jobKind, commonutil.JobFailedReason), msg)

Additionally, could you add another level nest?

if pod.Status.Phase == v1.PodFailed { failedPodsCount.Inc() if spec.RestartPolicy == apiv1.RestartPolicyExitCode && trainutil.IsRetryableExitCode(exitCode) || spec.RestartPolicy == apiv1.RestartPolicyOnFailure || spec.RestartPolicy == apiv1.RestartPolicyAlways { // Existing Codes } else if spec.RestartPolicy == apiv1.RestartPolicyExitCode && !trainutil.IsRetryableExitCode(exitCode) { // New Codes } }

andreyvelich · 2024-04-23T12:19:42Z

@kellyaa Were you able to find time to implement integration test for your change ?
We want to cut the first RC.0 soon, and it would be nice to include your change there.

kellyaa · 2024-04-23T15:14:02Z

@andreyvelich Working on it as we speak and hope to get it in today. Please let me know if this is holding up the train.

johnugeorge · 2024-04-23T16:00:15Z

Thanks @kellyaa

Signed-off-by: Kelly A <kellyaa@users.noreply.github.com>

kellyaa · 2024-04-23T16:42:54Z

Test case added and updated with improved logging statements. Ready for re-review!

… testing Signed-off-by: Kelly A <kellyaa@users.noreply.github.com>

tenzen-y · 2024-04-24T07:49:00Z

@kellyaa The new test case could not perform appropriately:

• [FAILED] [30.003 seconds]
TFJob controller Test Unretryable Exit Code [It] should set the job status to Failed
/home/runner/work/training-operator/training-operator/go/src/github.com/kubeflow/training-operator/pkg/controller.v1/tensorflow/pod_test.go:278

https://github.com/kubeflow/training-operator/actions/runs/8807089592/job/24174443567?pr=2071#step:4:609

So, could you fix that problem?

Signed-off-by: Kelly A <kellyaa@users.noreply.github.com>

kellyaa · 2024-04-24T20:57:28Z

Resubmitted! Thanks for your patience as I ramp up on this code!

tedhtchang · 2024-04-24T22:00:58Z

/test all

Signed-off-by: Kelly A <kellyaa@users.noreply.github.com>

tedhtchang · 2024-04-25T16:41:11Z

/test all

Signed-off-by: Kelly A <kellyaa@users.noreply.github.com>

kellyaa · 2024-04-25T21:39:49Z

/test all

andreyvelich · 2024-04-25T21:57:34Z

@kellyaa Please rebase your PR to get the latest fix for CI

kellyaa · 2024-04-26T00:34:57Z

/test all

tenzen-y

@kellyaa Thank you for this update!
Overall lgtm

tenzen-y · 2024-04-26T07:37:25Z

pkg/controller.v1/tensorflow/pod_test.go

+			Eventually(func() bool {
+				updatedJob := &kubeflowv1.TFJob{}
+				err := testK8sClient.Get(ctx, types.NamespacedName{Name: tfJob.GetName(), Namespace: metav1.NamespaceDefault}, updatedJob)
+				if err != nil {
+					return false
+				}
+				for _, condition := range updatedJob.Status.Conditions {
+					if condition.Type == kubeflowv1.JobFailed && condition.Status == corev1.ConditionTrue {
+						return true
+					}
+				}
+				return false
+			}, testutil.Timeout, testutil.Interval).Should(BeTrue(), "TFJob should be in Failed state")


Suggested change

Eventually(func() bool {

updatedJob := &kubeflowv1.TFJob{}

err := testK8sClient.Get(ctx, types.NamespacedName{Name: tfJob.GetName(), Namespace: metav1.NamespaceDefault}, updatedJob)

if err != nil {

return false

}

for _, condition := range updatedJob.Status.Conditions {

if condition.Type == kubeflowv1.JobFailed && condition.Status == corev1.ConditionTrue {

return true

}

}

return false

}, testutil.Timeout, testutil.Interval).Should(BeTrue(), "TFJob should be in Failed state")

Eventually(func(g Gomega) {

updatedJob := &kubeflowv1.TFJob{}

g.Expect(testK8sClient.Get(ctx, types.NamespacedName{Name: tfJob.GetName(), Namespace: metav1.NamespaceDefault}, updatedJob)).Should(Succeeded())

g.Expect(updatedJob.Status.Conditions).Should(ContainElements(

BeComparableTo(metav1.Condition{

Type: kubeflowv1.JobFailed,

Status: corev1.ConditionTrue,

}, cmpopts.IgnoreFields(metav1.Condition{}, "LastTransitionTime", "Reason", "Message", "ObservedGeneration"), "TFJob should be in Failed state"),

))

}, testutil.Timeout, testutil.Interval).Should(Succeeded())

Could you improve debuggability like this?

tenzen-y · 2024-04-26T07:38:23Z

pkg/controller.v1/tensorflow/pod_test.go

+					},
+				},
+			})
+			Expect(testK8sClient.Status().Update(ctx, created))


Suggested change

Expect(testK8sClient.Status().Update(ctx, created))

Expect(testK8sClient.Status().Update(ctx, created)).Should(Succeeded())

tenzen-y · 2024-04-26T07:40:01Z

pkg/controller.v1/tensorflow/pod_test.go

+			})
+			Expect(testK8sClient.Status().Update(ctx, created))
+
+			_ = reconciler.ReconcileJobs(tfJob, tfJob.Spec.TFReplicaSpecs, tfJob.Status, &tfJob.Spec.RunPolicy)


Suggested change

_ = reconciler.ReconcileJobs(tfJob, tfJob.Spec.TFReplicaSpecs, tfJob.Status, &tfJob.Spec.RunPolicy)

Expect(reconciler.ReconcileJobs(tfJob, tfJob.Spec.TFReplicaSpecs, tfJob.Status, &tfJob.Spec.RunPolicy)).Should(Succeeded)

I've noticed that if I use Expect here when testing locally, it fails because the reconciler tries to recreate the pod that exists. If I take this line out completely, it still succeeds on my laptop. However, it fails in the 1.27.1 GH Action tests.

Oh, I see. Thank you for sharing that.
Indeed, our tests have some unintended and not understandable issues...

So, let's keep using your implementations for now.

tedhtchang

/LGTM Verified with the following:

git clone --single-branch --branch master https://github.com/kellyaa/training-operator.git
cd training-operator
export INGRESS_NGINX_VERSION=controller-v1.9.6

# Setup KinD
echo "Creating KinD cluster"
kind delete cluster -n training-operator-cluster
cat <<EOF | kind create cluster --name training-operator-cluster --config -
kind: Cluster
apiVersion: kind.x-k8s.io/v1alpha4
nodes:
  - role: control-plane
    image: kindest/node:v1.25.3@sha256:f52781bc0d7a19fb6c405c2af83abfeb311f130707a0e219175677e366cc45d1
    extraPortMappings:
    - containerPort: 80
      hostPort: 80
      protocol: TCP
    kubeadmConfigPatches:
      - |
        kind: InitConfiguration
        nodeRegistration:
          kubeletExtraArgs:
            node-labels: "ingress-ready=true"
EOF
echo "Deploying Ingress controller into KinD cluster"
curl https://raw.githubusercontent.com/kubernetes/ingress-nginx/"${INGRESS_NGINX_VERSION}"/deploy/static/provider/kind/deploy.yaml | sed "s/--publish-status-address=localhost/--report-node-internal-ip-address\\n        - --status-update-interval=10/g" | kubectl apply -f -
kubectl annotate ingressclass nginx "ingressclass.kubernetes.io/is-default-class=true"
kubectl -n ingress-nginx wait --timeout=300s --for=condition=Available deployments --all


cat <<EOF | kubectl apply -f -
apiVersion: v1
kind: Namespace
metadata:
  labels:
    kubernetes.io/metadata.name: kubeflow
  name: kubeflow
spec:
  finalizers:
  - kubernetes
---
apiVersion: v1
kind: Secret
metadata:
  name: training-operator-webhook-cert
  namespace: kubeflow
type: Opaque
EOF
# Run k8s api and controller locally. Make sure controller log has no erro
make install && make run

Open another session:

cat <<EOF | oc create -f -
apiVersion: kubeflow.org/v1
kind: PyTorchJob
metadata:
  name: kfto-sft
  namespace: default
spec:
  pytorchReplicaSpecs:
    Master:
      restartPolicy: ExitCode
      replicas: 1
      template:
        spec:
          restartPolicy: ExitCode
          containers:
            - name: pytorch
              image: quay.io/tedchang/alpine:latest
              imagePullPolicy: Always
              command: [/bin/sh, -c, 'sleep 30 && exit 2']
              resources:
                requests:
                  cpu: 1
                  memory: "200Mi"
    Worker:
      restartPolicy: ExitCode
      replicas: 1
      template:
        spec:
          restartPolicy: ExitCode
          containers:
            - name: pytorch
              image: quay.io/tedchang/alpine:latest
              imagePullPolicy: Always
              command: [/bin/sh, -c, 'sleep 15 && exit 2']
              resources:
                requests:
                  cpu: 1
                  memory: "200Mi"
EOF
kubectl -n default wait --timeout=300s --for=condition=Created pytorchjobs kfto-sft
echo "pytorchjobs created"
kubectl -n default wait --timeout=300s --for=condition=Running pytorchjobs kfto-sft
echo "pytorchjobs running"
kubectl -n default wait --timeout=300s --for=condition=Failed pytorchjobs kfto-sft
echo "Job failed as expected"

tedhtchang · 2024-04-26T08:27:00Z

@kellyaa you may need to sign Corporate Agreement cla using IBM email

andreyvelich · 2024-04-26T14:47:51Z

Thank you for the update!
@kellyaa Pease can you address @tenzen-y comments and we can merge it ?

Signed-off-by: Kelly A <kellyaa@users.noreply.github.com>

tenzen-y · 2024-04-26T14:58:08Z

pkg/controller.v1/tensorflow/pod_test.go


 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
+	. "github.com/onsi/gomega/gstruct"


Suggested change

. "github.com/onsi/gomega/gstruct"

Could you remove this dependency?

Oh, you introduced the IgnoreExtras.
NVM

I needed it in order to use MatchFields, IgnoreExtras, Fields. For some reason ContainElements(BeComparableTo was not complaining that the fields did not match even though they appeared to be matching. Open to any other suggestions here.

Uhm, It sounds curious.
Let me try to investigate it.
Anyway, I think that we can merge this for now.

tenzen-y

@kellyaa Thank you for fixing this!
/lgtm
/approve
/hold for CI

terrytangyuan

Thank you!

/lgtm
/approve

google-oss-prow · 2024-04-26T15:17:28Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kellyaa, tedhtchang, tenzen-y, terrytangyuan

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [tenzen-y,terrytangyuan]

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

tenzen-y · 2024-04-26T15:34:19Z

/hold cancel

* Fail job for non-retryable exit codes Signed-off-by: Kelly A <kellyaa@users.noreply.github.com> * Add test for non-retryable exit code Signed-off-by: Kelly A <kellyaa@users.noreply.github.com> * Add if nesting, remove manual creation of node in non-retry exit code testing Signed-off-by: Kelly A <kellyaa@users.noreply.github.com> * Fix broken no-retry exit code test Signed-off-by: Kelly A <kellyaa@users.noreply.github.com> * Unbreak test for 1.27 Signed-off-by: Kelly A <kellyaa@users.noreply.github.com> * Remove pod creation for noretry exit code test Signed-off-by: Kelly A <kellyaa@users.noreply.github.com> * Make nonretry exit code test more debugable Signed-off-by: Kelly A <kellyaa@users.noreply.github.com> --------- Signed-off-by: Kelly A <kellyaa@users.noreply.github.com> Signed-off-by: Weiyu Yen <ckyuto@gmail.com>

* Fail job for non-retryable exit codes Signed-off-by: Kelly A <kellyaa@users.noreply.github.com> * Add test for non-retryable exit code Signed-off-by: Kelly A <kellyaa@users.noreply.github.com> * Add if nesting, remove manual creation of node in non-retry exit code testing Signed-off-by: Kelly A <kellyaa@users.noreply.github.com> * Fix broken no-retry exit code test Signed-off-by: Kelly A <kellyaa@users.noreply.github.com> * Unbreak test for 1.27 Signed-off-by: Kelly A <kellyaa@users.noreply.github.com> * Remove pod creation for noretry exit code test Signed-off-by: Kelly A <kellyaa@users.noreply.github.com> * Make nonretry exit code test more debugable Signed-off-by: Kelly A <kellyaa@users.noreply.github.com> --------- Signed-off-by: Kelly A <kellyaa@users.noreply.github.com>

google-oss-prow bot requested review from jinchihe and kuizhiqing April 18, 2024 16:34

google-oss-prow bot added the size/XS label Apr 18, 2024

Fail job for non-retryable exit codes

c2bf5c8

Signed-off-by: Kelly A <kellyaa@users.noreply.github.com>

kellyaa force-pushed the master branch from 79c7702 to c2bf5c8 Compare April 18, 2024 16:37

anishasthana mentioned this pull request Apr 18, 2024

PytorchJob restartPolicy: ExitCode leaves job in a "Restarting" state after a non-retryable error #2044

Closed

tenzen-y reviewed Apr 19, 2024

View reviewed changes

Add test for non-retryable exit code

141c150

Signed-off-by: Kelly A <kellyaa@users.noreply.github.com>

google-oss-prow bot added size/M and removed size/XS labels Apr 23, 2024

Add if nesting, remove manual creation of node in non-retry exit code…

6372886

… testing Signed-off-by: Kelly A <kellyaa@users.noreply.github.com>

Fix broken no-retry exit code test

28c47f5

Signed-off-by: Kelly A <kellyaa@users.noreply.github.com>

Unbreak test for 1.27

78f53ca

Signed-off-by: Kelly A <kellyaa@users.noreply.github.com>

Remove pod creation for noretry exit code test

733d949

Signed-off-by: Kelly A <kellyaa@users.noreply.github.com>

Merge branch 'kubeflow:master' into master

0148574

tenzen-y reviewed Apr 26, 2024

View reviewed changes

tedhtchang approved these changes Apr 26, 2024

View reviewed changes

Make nonretry exit code test more debugable

4396d2e

Signed-off-by: Kelly A <kellyaa@users.noreply.github.com>

tenzen-y reviewed Apr 26, 2024

View reviewed changes

google-oss-prow bot added the do-not-merge/hold label Apr 26, 2024

google-oss-prow bot assigned tenzen-y Apr 26, 2024

google-oss-prow bot added lgtm approved labels Apr 26, 2024

terrytangyuan approved these changes Apr 26, 2024

View reviewed changes

google-oss-prow bot assigned terrytangyuan Apr 26, 2024

google-oss-prow bot removed the do-not-merge/hold label Apr 26, 2024

google-oss-prow bot merged commit 732fc12 into kubeflow:master Apr 26, 2024

tenzen-y mentioned this pull request Apr 26, 2024

Test: Simplify and Identify pod-controller envtest #2084

Merged

1 task

	Expect(testK8sClient.Status().Update(ctx, created))
	Expect(testK8sClient.Status().Update(ctx, created)).Should(Succeeded())

	_ = reconciler.ReconcileJobs(tfJob, tfJob.Spec.TFReplicaSpecs, tfJob.Status, &tfJob.Spec.RunPolicy)
	Expect(reconciler.ReconcileJobs(tfJob, tfJob.Spec.TFReplicaSpecs, tfJob.Status, &tfJob.Spec.RunPolicy)).Should(Succeeded)

Conversation

kellyaa commented Apr 18, 2024

Uh oh!

coveralls commented Apr 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 8850191138

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

Uh oh!

tenzen-y left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andreyvelich commented Apr 23, 2024

Uh oh!

kellyaa commented Apr 23, 2024

Uh oh!

johnugeorge commented Apr 23, 2024

Uh oh!

kellyaa commented Apr 23, 2024

Uh oh!

tenzen-y commented Apr 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kellyaa commented Apr 24, 2024

Uh oh!

tedhtchang commented Apr 24, 2024

Uh oh!

tedhtchang commented Apr 25, 2024

Uh oh!

kellyaa commented Apr 25, 2024

Uh oh!

andreyvelich commented Apr 25, 2024

Uh oh!

kellyaa commented Apr 26, 2024

Uh oh!

tenzen-y left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tedhtchang left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tedhtchang commented Apr 26, 2024

Uh oh!

andreyvelich commented Apr 26, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tenzen-y left a comment

Choose a reason for hiding this comment

Uh oh!

terrytangyuan left a comment

Choose a reason for hiding this comment

Uh oh!

google-oss-prow bot commented Apr 26, 2024

coveralls commented Apr 18, 2024 •

edited

Loading

tenzen-y commented Apr 24, 2024 •

edited

Loading

tedhtchang left a comment •

edited

Loading