KEP for TTL-after-finished controller #2552

janetkuo · 2018-08-16T17:48:47Z

From: https://goo.gl/YxtxTi

janetkuo · 2018-08-16T17:50:04Z

/assign @tnozicka @kow3ns @enisoc

cc @kubernetes/sig-apps-pr-reviews

kow3ns · 2018-08-16T21:28:01Z

/approve

kow3ns · 2018-08-16T21:28:36Z

/lgtm

liggitt · 2018-08-17T19:59:14Z

keps/sig-apps/0024-ttl-after-finish.md

+Because Job controller depends on Pods to exist to work correctly. In Job
+validation, `ttlSecondsAfterFinished` of its pod template shouldn't be set, to
+prevent users from breaking their Jobs. Users should set TTL seconds on a Job,
+instead of Pods owned by a Job.


will job validation prevent this? how does this work with podgc today? does that mess up jobs?

When we introduce TTLSecondsAfterFinished to pods, we will implement job validation so that job won't create pods with TTLSecondsAfterFinished set, and therefore won't mess up jobs. When TTLSecondsAfterFinished is implemented for pods, it can replace podgc.

Are there other higher level resources that should make sure to validate this since PodTemplateSpec is fairly common?

w.r.t. validation I guess this is exclusive with restartPolicy: Always, often used with workload controllers.

it's common for higher level resources to call generic pod spec validation, then add their own restrictions on top:

Job makes sure the pod spec specifies restart policy of "never":
https://github.com/kubernetes/kubernetes/blob/master/pkg/apis/batch/validation/validation.go#L121-L127

ReplicationController makes sure the pod spec specifies restart policy of "always":
https://github.com/kubernetes/kubernetes/blob/master/pkg/apis/core/validation/validation.go#L3969-L3972

agree that pod spec validation should only allow this to be set on a pod spec for restart policies that make sense

Thanks for the advice. Added validation for restart policy.

liggitt · 2018-08-17T20:01:09Z

keps/sig-apps/0024-ttl-after-finish.md

+  time.
+
+Mitigations:
+* In Kubernetes, it's required to run NTP on all nodes ([#6159][]) to avoid time


does that ensure clocks are correct inside the controller manager container?

Clocks aren't always correct, but the difference should be small. We can also add a validation to disallow non-zero TTL less than 1 minute.

tnozicka · 2018-08-21T14:34:02Z

keps/sig-apps/0024-ttl-after-finish.md

+This can be promoted to beta when it satisfies users' need for cleaning up
+finished resource objects, without regressions.
+
+This will be promoted to GA once it's gone a sufficient amount of time as beta


From the previous dicussions on the google doc:

Because metadata is used by all resources, but not all resources are finishable (only Pods/Jobs). We want to implement this for Pods/Jobs first. If users like this feature, and more use cases from custom resources, we can move it to metadata. To avoid the overhead, we can either remove this alpha field in spec and add a field in metadata, or generalize by keeping it in the spec of each finishable resource.

I am not sure I want us promoting something to beta/GA if we know that we will replace it with a generic mechanism. We should flesh out the path here a bit more in that regard.

This will only be promoted to beta after this decision is finalized, i.e. whether we decide to replace it with a generic mechanism or not to do it. Adding this feature as alpha first is essential to help us gather feedback before deciding to generalize it or not.

This will only be promoted to beta after this decision is finalized, i.e. whether we decide to replace it with a generic mechanism or not to do it. Adding this feature as alpha first is essential to help us gather feedback before deciding to generalize it or not.

I agree we should try this as alpha to gather feedback. I was asking to formalize what you just said as beta graduation criteria in the proposal.

Thanks, updated graduation criteria.

tnozicka

nits, few questions, looks good overall

tnozicka · 2018-08-22T07:08:52Z

keps/sig-apps/0024-ttl-after-finish.md

+higher-level resource (e.g. CronJob for Jobs or Job for Pods), or owned by some
+other resources, it's difficult for the users to clean them up automatically,
+and those Jobs and Pods can accumulate and overload a Kubernetes cluster very
+easily. Even if we can avoid the overload issue by implementing a cluster-wide


avoid the overload issue by implementing a cluster-wide (global) resource quota

I was told yesterday we already have it with count/* quota. The non-terminated count for pods apply only for a specific count/pods. Apologies if I got you confused by my comment on the google doc preceding this.

We currently have ResourceQuota, which sets quota restrictions per namespace. Is there a per cluster / global quota available today?

yeah, seems like we have only quota per namespace, best you get to protect the etcd/cluster now is that the cluster object count is limited by multiplying the quota per namespace by the number of namespaces.

tnozicka · 2018-08-22T07:14:02Z

keps/sig-apps/0024-ttl-after-finish.md

+
+```go
+type JobSpec struct {
+ 	// ttlSecondsAfterFinished limits the lifetime of a Job that has finished


nit: s/ttlSecondsAfterFinished/TTLSecondsAfterFinished (applies globally)

doc is supposed to reference the serialized name so the generated doc matches what people see in the API

Thanks for the clarification, I wasn't aware API is an exception for the Golang rule where every comment should begin with name of the type.

tnozicka · 2018-08-22T07:31:39Z

keps/sig-apps/0024-ttl-after-finish.md

+Because Job controller depends on Pods to exist to work correctly. In Job
+validation, `ttlSecondsAfterFinished` of its pod template shouldn't be set, to
+prevent users from breaking their Jobs. Users should set TTL seconds on a Job,
+instead of Pods owned by a Job.


Are there other higher level resources that should make sure to validate this since PodTemplateSpec is fairly common?

w.r.t. validation I guess this is exclusive with restartPolicy: Always, often used with workload controllers.

tnozicka · 2018-08-22T07:38:58Z

keps/sig-apps/0024-ttl-after-finish.md

+1. Check its `.status.conditions` to see if it has finished (`Complete` or
+   `Failed`). If it hasn't finished, do nothing. 
+1. Otherwise, if the Job has finished, check if Job's 
+   `.spec.ttlSecondsAfterFinished` field is set. Do nothing if the TTL field is


Should this be logically the first step? Does it make sense to check the condition if TTLSecondsAfterFinished isn't set?

Since both steps merely check the fields of the Job, the order is interchangeable. We don't need to do any further computation if either the job hasn't finished or its ttl it's set.

I think there is an important distinction here. .spec.ttlSecondsAfterFinished is the field that allows this controller to process those jobs. This is what discriminates if a Job is added to a queue or not and we should be very explicit about this. No other action should be taken on a Job if this field is not set. Logically it doesn't make sense to check a status for a Job if you shouldn't be managing it at all. Controllers/code can evolve and we should structure it well from the start even though now the read-only actions don't cause issues.

To clarify, Jobs are only added to the workqueue (for cleanup) when they (1) finished (2) have a TTL set. No action will be taken if not both conditions are met.

janetkuo · 2018-08-22T17:57:45Z

Thanks for the review. Rebased and addressed comments. PTAL

kow3ns · 2018-08-23T17:20:48Z

/lgtm

calebamiles · 2018-08-23T18:11:55Z

/approve

k8s-ci-robot · 2018-08-23T18:12:02Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: calebamiles, kow3ns

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:

~~keps/OWNERS~~ [calebamiles]

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

smarterclayton · 2018-08-24T15:43:47Z

@stevekuznetsov as someone who has actually implemented a variant of this for a CI cluster, can you comment about your usecase here and give some concrete feedback

k8s-ci-robot requested review from jdumars and prydonius August 16, 2018 17:49

k8s-ci-robot assigned enisoc, kow3ns and tnozicka Aug 16, 2018

janetkuo mentioned this pull request Aug 16, 2018

TTL after finish kubernetes/enhancements#592

Closed

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

liggitt reviewed Aug 17, 2018

View reviewed changes

tnozicka reviewed Aug 21, 2018

View reviewed changes

tnozicka reviewed Aug 22, 2018

View reviewed changes

KEP for TTL-after-finished controller

80e8c2b

janetkuo force-pushed the job-ttl-cleanup branch from 3d99102 to 80e8c2b Compare August 22, 2018 17:55

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

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

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

k8s-ci-robot merged commit 9805a93 into kubernetes:master Aug 23, 2018

janetkuo mentioned this pull request Aug 29, 2018

Job objects are not cleaned up kubernetes/kubernetes#64470

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KEP for TTL-after-finished controller #2552

KEP for TTL-after-finished controller #2552

janetkuo commented Aug 16, 2018 •

edited

Loading

janetkuo commented Aug 16, 2018

kow3ns commented Aug 16, 2018

kow3ns commented Aug 16, 2018

liggitt Aug 17, 2018

janetkuo Aug 17, 2018 •

edited

Loading

tnozicka Aug 22, 2018

liggitt Aug 22, 2018 •

edited

Loading

janetkuo Aug 22, 2018

liggitt Aug 17, 2018

janetkuo Aug 17, 2018

tnozicka Aug 21, 2018

janetkuo Aug 21, 2018 •

edited

Loading

tnozicka Aug 22, 2018 •

edited

Loading

janetkuo Aug 22, 2018

tnozicka left a comment

tnozicka Aug 22, 2018

janetkuo Aug 22, 2018

tnozicka Aug 24, 2018

tnozicka Aug 22, 2018

liggitt Aug 22, 2018

tnozicka Aug 22, 2018

tnozicka Aug 22, 2018

tnozicka Aug 22, 2018

janetkuo Aug 22, 2018

tnozicka Aug 24, 2018 •

edited

Loading

janetkuo Aug 24, 2018

janetkuo commented Aug 22, 2018

kow3ns commented Aug 23, 2018

calebamiles commented Aug 23, 2018

k8s-ci-robot commented Aug 23, 2018

smarterclayton commented Aug 24, 2018

KEP for TTL-after-finished controller #2552

KEP for TTL-after-finished controller #2552

Conversation

janetkuo commented Aug 16, 2018 • edited Loading

janetkuo commented Aug 16, 2018

kow3ns commented Aug 16, 2018

kow3ns commented Aug 16, 2018

Choose a reason for hiding this comment

janetkuo Aug 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt Aug 22, 2018 • edited Loading

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

janetkuo Aug 21, 2018 • edited Loading

Choose a reason for hiding this comment

tnozicka Aug 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnozicka 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

tnozicka Aug 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janetkuo commented Aug 22, 2018

kow3ns commented Aug 23, 2018

calebamiles commented Aug 23, 2018

k8s-ci-robot commented Aug 23, 2018

smarterclayton commented Aug 24, 2018

janetkuo commented Aug 16, 2018 •

edited

Loading

janetkuo Aug 17, 2018 •

edited

Loading

liggitt Aug 22, 2018 •

edited

Loading

janetkuo Aug 21, 2018 •

edited

Loading

tnozicka Aug 22, 2018 •

edited

Loading

tnozicka Aug 24, 2018 •

edited

Loading