Tf-operator: [proposal] cleanup jobs after finished

[ccding]

Requirement

Delete the tf-job from Kubernetes after it has finished for a (configurable) period of time.

Approach

Add a configuration called cleanupTimeoutAfterFinished in the yaml file for each job.

At the initialization phase of tf-operator, it lists all the tf-jobs and watches for updates. With the statuses of all the tf-jobs stored locally, tf-operator is able to delete them from Kubernetes.

Side-effect

If cleanupTimeoutAfterFinished is not set or is set to 0, tf-operator works the same as that without this proposal. Therefore, there is not any side-effect to existing systems.

[gaocegege]

Make sense.

[ChanYiLin]

Is there any one working on it?
I am willing to do more research on this issue and work on it.

But here are some discussions,

I think the issue is similar to the issue of whether to delete all the pods of tf-job after finishing since users might want to log in to check the logs of training. As far as I know, delete the tf-job will also delete all the resources it has at the same time. So the problem is same here: where to store the status of tf-job itself and worker's logs? Maybe the simplest solution is we need to create a additional pod for tf-operator to store these logs and status.

[gaocegege]

@ChanYiLin

Hi, we already implemented the logic about deleting pods of the tfjob. While the issue is to delete the tfjob after cleanupTimeoutAfterFinished.

But feel free to pick up this issue!

[ChanYiLin]

Oh! I see the closed issue. I didn't keep up with the trend lol.

I have checked the code and just want to make sure.
Do we just let user specify how they want to deal with the pods after tf-job finished, and if the policy is Running or All, then we simply call k8s api to delete those pods without storing training logs?

Then I think the these issues are a little bit different.
In this proposal, we would like "With the statuses of all the tf-jobs stored locally",
then we cannot directly call k8s api to delete the tfjobs but to store the status of this tfjob first, am I right?

[gaocegege]

This issue is similar to #550 , and we could refer to the implementation of job controllers.

[ccding]

Is there any one working on it?

I am working on it.

[ChanYiLin]

@ccding Sure! No problem!

[jlewi]

Why do we want to delete old jobs?
If we want to garbage collect jobs why wouldn't we implement this as a separate agent that could garbage collect other resources like Argo workflows.

Are there any issues within K8s about garbage collecting resources?

Also why would we want this to be a per job parameter as opposed to a setting that applied to all jobs?

[jlewi]

Seems related
https://github.com/lwolf/kube-cleanup-operator

Although not sure why we need a CRD as opposed to just an agent/daemon running in the cluster.

[jlewi]

Why not implement this as an agent/daemon in the cluster that can garbage collect multiple types of resources; e.g.

• K8s jobs
• Argo workflows
• All KF job operators

This could work by relying on common Conditions to determine when a job was considered done and eligible for deletion.

We could also follow the pattern of using K8s annotations to indicate that jobs should be included/excluded from garbage collection.

[jian-he]

The proposal extends the current clean policy to have a delay semantics and add the ability to delete the tfjob object in addition to deleting pods which currently exists.

The use case is to delete old jobs after X days. the delay is because we need debug information to stay for certain time. The deletion is because, if not deleted, the tfjob object will pile up in etcd increasingly and we run thousands of training jobs per day and may eventually overload etcd.

common Conditions to determine when a job was considered done and eligible for deletion.

A generic gc daemon sounds good. But IIUC, we still need to adhocly modify the generic gc daemon code to support different workload types.

As of now, we can rely on the existing deleteAllPods policy with additional support for delay time. That can at least delete all the pods object from etcd eventually. We can have a separate issue for how to delete the tfjob itself.

[ScorpioCPH]

+1, as a production component, tf-operator create too many TFJobs everyday, we now delete these TFJobs every week manually, this feature sounds good for me :)

[jlewi]

If the goal is to prevent users from overloading etcd why would you want to rely on users setting the GC policy per job? If one user forgets to set it and launches a bunch of jobs you will run into the same thing.

Garbage collecting old resources to keep the cluster healthy seems like and admin policy that should be enforced as such and not left to the discretion of individual users.

Garbage collection is a problem not just for TFJobs resources. Our test clusters are suffering because we have too many Argo workflows.

We will have this problem for all kinds of resources that users create in large quantities e.g.

• Argo workflows
• K8s Jobs
• PyTorch/MPI/Chainer/TFJob jobs.

So why not just implement this once in a way that can be reused for all these different resources?

[jlewi]

Here is the issue in K8s community:
kubernetes/kubernetes#64470
Design doc: https://goo.gl/YxtxTi.
To solve it for K8s jobs first and then generalize it.

The proposal is to have a field TTLAfterFinished.

I think an admission controller could be used to enforce a non-zero and maximum TTL for all jobs in the cluster; which addresses my earlier question about making this a per job setting.

It looks like #725 was merged but it doesn't look like like #725 actually solves the problem of deleting older jobs. All #725 does was delay the implementation of pod cleanup policy.

[jlewi]

It looks like the original proposal of adding cleanupTimeoutAfterFinished is very much in line with
kubernetes/kubernetes#64470.

I suggest we follow kubernetes/kubernetes#64470 and add a TTL parameter. If we don't want to wait for Kubernetes to provide a generic solution, I think we can implement it ourselves. Since this will hopefully be a short term solution, I think its fine to implement in a cheap, non-general way (e.g. in the controller).

If we add the TTL can we remove the CleanPodDelay added in #718?

[gaocegege]

I think we could keep the delay until the upstream implemented TTL. WDYT @ccding @jian-he @jlewi

[jlewi]

@gaocegege What purpose does the CleanPodDelay serve? The stated purpose in this issue and in the PR was to garbage collect old jobs to prevent the K8s master from being overwhelmed. How does the CleanPodDelay accomplish that? Won't it just exacerbate the problem by leaving more pods lying around?

So why would we leave it in?

[gaocegege]

@jlewi I think @jian-he wants to keep the pod for a time to get the log, then after a delay, the operator could delete them.

[jlewi]

@gaocegege @jian-he Do these pods not terminate? Is that the problem?

[gaocegege]

@jlewi They want to keep the pods for a time, then delete it. And the delay is for this purpose. If we do not support it, the pod will be deleted immediately.

I think the delay makes sense to me since there are some ppl who want to get the log for a while, then delete.

[jlewi]

Is the complexity of adding yet another knob really worth it? It seems like this field provides minimal benefit over what we already have.

One of the clean pod policies that only deletes non-completed pods. So for completed pods the logs will be preserved for as long as K8s doesn't GC the pods.

For non completed pods cluster level logging seems like the right solution.

I don't think CleanPodDelay will be very useful in practice. If you set it to a short time e.g. 5 minutes the likelihood that you inspect the job before the pods are garbage collected is small. If you set it to a really large time (e.g. hours) you will have a significant negative impact on resource utilization.

I think the right way to fix this is by making it easier fetch logs for an appropriate backend.

[jian-he]

The use-case is not only for logs, but also some local artifacts for debugging purpose.

The original intent is to delete the job after certain time, but you raised a concern for why deleting the job . The CleanPodDelay was chosen because there's already deletion policy for pods, hence this adds one more retention policy to it.

If you set it to a really large time (e.g. hours) you will have a significant negative impact on resource utilization.

Good point, the delay policy should only apply to finished pods. For running pods we can delete it to release the resource.

If we add the TTL can we remove the CleanPodDelay added in #718?

It sounds good too, if we can add a TTL policy for Job as originally proposed.

[jlewi]

@jian-he that makes sense. So per my comment above does the following make sense.

1. Remove CleanPodDelay (#718)
2. Add a Job TTL following kubernetes/kubernetes#64470

[ccding]

submitted a PR #746 to revert the #725

[ccding]

If we don't want to wait for Kubernetes to provide a generic solution, I think we can implement it ourselves.

Implemented #753