TTL for cleaning up Jobs after they finish #66840

janetkuo · 2018-07-31T21:50:15Z

What this PR does / why we need it: kubernetes/enhancements#592

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #64470
For kubernetes/enhancements#592

Special notes for your reviewer: @kubernetes/sig-apps-pr-reviews

Release note:

Add a TTL machenism to clean up Jobs after they finish.

tnozicka · 2018-08-01T11:19:46Z

@janetkuo could we use dynamic client/informer to handle TTL for both finished Jobs and Pods in 1 controller? Seems like this would be fairly similar logic.

kow3ns

Generally approve. Needs e2e as well.

kow3ns · 2018-08-02T15:54:34Z

pkg/apis/batch/types.go

+	// finishes. This field is alpha-level and is only honored by servers that
+	// enable the TTLAfterFinished feature.
+	// +optional
+	TTLSecondsAfterFinished *int32


Might want to consider using TTLSecondsAfterCompletion to be consistent with the terminology in JobStatus.

Jobs finishes once its status is either complete or failed. Combined complete + failed into finished for simplicity, based on user feedback from kubeflow.

kow3ns · 2018-08-02T15:55:47Z

pkg/features/kube_features.go

+	// alpha: v1.12
+	//
+	// Allow TTL controller to clean up Pods and Jobs after they finish.
+	TTLAfterFinished utilfeature.Feature = "TTLAfterFinished"


Same as previous comment with respect to Completion instead of Finished.

kow3ns · 2018-08-02T15:57:40Z

pkg/controller/ttlafterfinished/ttlafterfinished_controller.go

+	clock clock.Clock
+}
+
+func New(jobInformer batchinformers.JobInformer, client clientset.Interface) *TTLAfterFinishedController {


Add comment for publicly exposed method.

kow3ns · 2018-08-02T16:02:25Z

pkg/controller/ttlafterfinished/ttlafterfinished_controller.go

+	"k8s.io/kubernetes/pkg/util/metrics"
+)
+
+type TTLAfterFinishedController struct {


You should describe the type. You should add a comment explaining what the controller does and the design choice to have the contorller live outside of JobController.

kow3ns · 2018-08-02T16:02:46Z

pkg/controller/ttlafterfinished/ttlafterfinished_controller.go

+	return tc
+}
+
+func (tc *TTLAfterFinishedController) Run(workers int, stopCh <-chan struct{}) {


Needs a comment.

kow3ns · 2018-08-02T16:10:53Z

pkg/controller/ttlafterfinished/ttlafterfinished_controller.go

+	if err != nil {
+		return nil, err
+	}
+	// TODO: handle finish time that happens in the future


You should come up with a well defined behavior for when this occurs. Consider simply deferring destroying the victim after the finish time. It's conservative, but it can't do harm.

Done. Because it happens in the future it'll only delay destorying the Job.

kow3ns · 2018-08-02T16:19:01Z

/approve

janetkuo · 2018-08-03T00:45:04Z

@tnozicka this TTL controller only needs to watch Jobs and Pods for now; Jobs and Pods have different ways of indicating that they "finished". I'd like to switch to dynamic client/informer when we extend this to custom resources.

kow3ns · 2018-08-07T19:52:42Z

/approve

kow3ns · 2018-08-10T02:34:10Z

/lgtm

janetkuo · 2018-08-10T16:41:10Z

/assign @deads2k @liggitt

Need your approval. Thanks!

janetkuo · 2018-09-04T20:12:40Z

Just fixing verify failure. Re-applying tag.

pkg/apis/batch/validation/validation_test.go

pkg/registry/batch/job/strategy.go

mikedanese · 2018-09-04T20:36:24Z

pkg/registry/batch/job/strategy_test.go

+)
+
+var (
+	ttlEnabled = utilfeature.DefaultFeatureGate.Enabled(features.TTLAfterFinished)


declare this in TestJobsStrategy since it's the only place you are using it.

mikedanese · 2018-09-04T20:50:00Z

cmd/kube-controller-manager/app/options/options.go

@@ -172,6 +173,9 @@ func NewKubeControllerManagerOptions() (*KubeControllerManagerOptions, error) {
 		ServiceController: &cmoptions.ServiceControllerOptions{
 			ConcurrentServiceSyncs: componentConfig.ServiceController.ConcurrentServiceSyncs,
 		},
+		TTLAfterFinishedController: &TTLAfterFinishedControllerOptions{
+			ConcurrentTTLSyncs: componentConfig.TTLAfterFinishedController.ConcurrentTTLSyncs,


I'm a proponent of YAGNI when it comes to configuration like parallel syncs. Is there a reason we want it explicitly for this feature as compared to other loops? An operator guessed value might be slighter better than a developer guessed value in an edge situation but the cost is extra code, maintainablity and suportability complexity. This was @cheftako's request

I'd prefer adding workqueue latency and handletime metrics and explore adaptive worker counts then hand knobs to operators as a last resort.

cmd/kube-controller-manager/app/controllermanager.go

…abled 1. If TTLAfterFinished feature is enabled, the value should be non-negative. 2. If TTLAfterFinished feature is disabled, the field value should not be kept.

make clean && make generated_files

mikedanese · 2018-09-04T21:29:30Z

pkg/controller/ttlafterfinished/ttlafterfinished_controller.go

+	job := cur.(*batch.Job)
+	glog.V(4).Infof("Updating job %s/%s", job.Namespace, job.Name)
+
+	if job.DeletionTimestamp == nil && needsCleanup(job) {


why not move job.DeletionTimestamp == nil into needsCleanup?

mikedanese · 2018-09-04T21:34:19Z

pkg/controller/ttlafterfinished/ttlafterfinished_controller.go

+}
+
+func getFinishAndExpireTime(j *batch.Job) (*time.Time, *time.Time, error) {
+	if !needsCleanup(j) {


Call this once at the begining of processJob, then don't check it again in all these helpers?

mikedanese · 2018-09-04T21:39:40Z

/approve

I'd like to see whether we can remove the flag before beta, but I won't block this PR.

mikedanese · 2018-09-04T21:41:04Z

per previous lgtms
/lgtm

k8s-ci-robot · 2018-09-04T21:41:13Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cheftako, dixudx, janetkuo, kow3ns, liggitt, mikedanese, tnozicka

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~api/OWNERS~~ [liggitt]
~~cmd/kube-controller-manager/OWNERS~~ [mikedanese]
~~docs/api-reference/OWNERS~~ [liggitt]
~~pkg/apis/OWNERS~~ [liggitt]
~~pkg/controller/OWNERS~~ [janetkuo,mikedanese]
~~pkg/features/OWNERS~~ [kow3ns,liggitt]
~~pkg/registry/OWNERS~~ [liggitt]
~~plugin/pkg/auth/OWNERS~~ [liggitt,mikedanese]
~~staging/src/k8s.io/api/OWNERS~~ [liggitt]
~~staging/src/k8s.io/kube-controller-manager/OWNERS~~ [liggitt]
~~test/OWNERS~~ [janetkuo,kow3ns,liggitt,mikedanese]

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

janetkuo · 2018-09-04T22:26:17Z

/test pull-kubernetes-e2e-kops-aws

janetkuo · 2018-09-05T01:23:22Z

/test pull-kubernetes-e2e-kops-aws

k8s-github-robot · 2018-09-05T02:18:46Z

/test all [submit-queue is verifying that this PR is safe to merge]

janetkuo · 2018-09-05T03:22:43Z

/retest

k8s-github-robot · 2018-09-05T03:49:34Z

Automatic merge from submit-queue (batch tested with PRs 66840, 68159). If you want to cherry-pick this change to another branch, please follow the instructions here: https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md.

k8s-ci-robot · 2018-09-05T04:20:18Z

@janetkuo: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-kubernetes-e2e-gce-100-performance	`13b76d5`	link	`/test pull-kubernetes-e2e-gce-100-performance`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

janetkuo assigned kow3ns Jul 31, 2018

janetkuo requested a review from kow3ns July 31, 2018 21:50

k8s-ci-robot requested review from david-mcmahon and dchen1107 July 31, 2018 21:50

k8s-github-robot added the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label Jul 31, 2018

janetkuo changed the title ~~Job ttl~~ TTL for cleaning up Jobs after they finish Jul 31, 2018

david-mcmahon removed their request for review July 31, 2018 21:53

janetkuo removed the request for review from dchen1107 July 31, 2018 22:53

janetkuo force-pushed the job-ttl branch 3 times, most recently from a1ff331 to 94efaae Compare August 1, 2018 01:16

kow3ns approved these changes Aug 2, 2018

View reviewed changes

janetkuo force-pushed the job-ttl branch from 94efaae to 60b02a2 Compare August 6, 2018 21:45

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 6, 2018

janetkuo force-pushed the job-ttl branch 4 times, most recently from a439b11 to 322c881 Compare August 6, 2018 22:18

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 10, 2018

janetkuo added lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. kind/feature Categorizes issue or PR as related to a new feature. labels Sep 4, 2018

mikedanese reviewed Sep 4, 2018

View reviewed changes

pkg/apis/batch/validation/validation_test.go Show resolved Hide resolved

mikedanese reviewed Sep 4, 2018

View reviewed changes

pkg/registry/batch/job/strategy.go Show resolved Hide resolved

mikedanese reviewed Sep 4, 2018

View reviewed changes

cmd/kube-controller-manager/app/controllermanager.go Outdated Show resolved Hide resolved

janetkuo added 4 commits September 4, 2018 14:21

Validate Job .spec.ttlSecondsAfterFinished; clear it when feature dis…

47d06c4

…abled 1. If TTLAfterFinished feature is enabled, the value should be non-negative. 2. If TTLAfterFinished feature is disabled, the field value should not be kept.

Add e2e test for TTL after finished

0a6389e

Make number of workers configurable

cbdc9b6

Autogen

13b76d5

make clean && make generated_files

janetkuo force-pushed the job-ttl branch from 5c6167e to 13b76d5 Compare September 4, 2018 21:21

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 4, 2018

mikedanese reviewed Sep 4, 2018

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 4, 2018

k8s-ci-robot assigned mikedanese Sep 4, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 4, 2018

k8s-github-robot merged commit c50a347 into kubernetes:master Sep 5, 2018

TTL for cleaning up Jobs after they finish #66840

TTL for cleaning up Jobs after they finish #66840

Conversation

janetkuo commented Jul 31, 2018 • edited

tnozicka commented Aug 1, 2018

kow3ns left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kow3ns commented Aug 2, 2018

janetkuo commented Aug 3, 2018

kow3ns commented Aug 7, 2018

kow3ns commented Aug 10, 2018

janetkuo commented Aug 10, 2018

janetkuo commented Sep 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikedanese Sep 4, 2018 • edited

Choose a reason for hiding this comment

mikedanese Sep 4, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikedanese commented Sep 4, 2018

mikedanese commented Sep 4, 2018

k8s-ci-robot commented Sep 4, 2018

janetkuo commented Sep 4, 2018

janetkuo commented Sep 5, 2018

k8s-github-robot commented Sep 5, 2018

janetkuo commented Sep 5, 2018

k8s-github-robot commented Sep 5, 2018

k8s-ci-robot commented Sep 5, 2018

janetkuo commented Jul 31, 2018 •

edited

mikedanese Sep 4, 2018 •

edited

mikedanese Sep 4, 2018 •

edited