Flaky e2e tests

[nagar-ajay]

While testing e2e integration tests on my local, I encountered a few scenarios when these tests fail.

• Sometimes tests fail because of the following condition.

  conditions = client.get_job_conditions(name, namespace, job_kind)
  if len(conditions) != 1:
      raise Exception(f"{job_kind} conditions are invalid: {conditions}")

https://github.com/kubeflow/training-operator/blob/master/sdk/python/test/e2e/utils.py#L22

I think the reason is if a container created by tests starts running instantaneously, then that test fails because that job will have two conditions.

• Sometimes tests fail because of the following condition

  conditions = client.get_job_conditions(name, namespace, job_kind)
  if len(conditions) != 3:
      raise Exception(f"{job_kind} conditions are invalid: {conditions}")

https://github.com/kubeflow/training-operator/blob/master/sdk/python/test/e2e/utils.py#L40-L42

With these scenarios, I found that the running condition is missing from training job conditions.

[johnugeorge]

@tenzen-y
For issue 1, Is the verification len(conditions) correct? Separately, unless job goes to running, can we say that job scheduling works?

For issue2, this looks like a bug. If job succeeds before even reconcile update to running state, this can happen.

[tenzen-y]

/kind bug

@nagar-ajay Thank you for reporting this issue.

For issue 1, Is the verification len(conditions) correct? Separately, unless job goes to running, can we say that job scheduling works?

You're right. Confirming a Running condition is better.
Also, for future work, we should set the un-schedulable resources to runPolicy.minResources instead of setting the un-schedulable number of replicas to runPolicy.minAvailable.

https://github.com/kubeflow/training-operator/blob/88719628eb7035cda66ea743c939a7d28599ad23/sdk/python/test/e2e/test_e2e_tfjob.py#L63

For issue2, this looks like a bug. If job succeeds before even reconcile update to running state, this can happen.

@nagar-ajay Which frameworks did you find this bug in?

[nagar-ajay]

Which frameworks did you find this bug in?

XGBoost

[tenzen-y]

/kind e2e-test-failure

I think if the XGBoostJob doesn't have a Running condition and reaches a succeeded or failed condition, adding a Running condition is better:

https://github.com/kubeflow/training-operator/blob/88719628eb7035cda66ea743c939a7d28599ad23/pkg/controller.v1/xgboost/xgboostjob_controller.go#L414

https://github.com/kubeflow/training-operator/blob/88719628eb7035cda66ea743c939a7d28599ad23/pkg/controller.v1/xgboost/xgboostjob_controller.go#L431

@johnugeorge @nagar-ajay WDYT?

[johnugeorge]

You mean, adding running False as well if running is missing?

[tenzen-y]

You mean, adding running False as well if running is missing?

Yes, that's right.

[tenzen-y]

@johnugeorge Friendly ping.

[johnugeorge]

Sorry for late response. It makes sense. I don't think, there is a better way if reconcile sees a terminal condition even before running state.

[nagar-ajay]

As a short-term fix for 2nd issue, I think we can increase the running time of jobs. WDYT? @tenzen-y @johnugeorge

[tenzen-y]

As a short-term fix for 2nd issue, I think we can increase the running time of jobs. WDYT? @tenzen-y @johnugeorge

@nagar-ajay I will work on fixing 2nd issue using my above proposal next week.

[tenzen-y]

/assign