Updates the ttl-to-finished KEP to graduate the feature to Beta. #2246

ahg-g · 2021-01-08T21:21:26Z

Enhancement issue: #592

ahg-g · 2021-01-08T21:22:21Z

@janetkuo should I move the "Finished Pods" section to a separate "Future Work" section?

keps/sig-apps/592-ttl-after-finish/README.md

keps/sig-apps/592-ttl-after-finish/kep.yaml

keps/sig-apps/592-ttl-after-finish/README.md

wojtek-t · 2021-01-14T14:09:04Z

keps/sig-apps/592-ttl-after-finish/README.md

+  Unexpected restarts of kube-controller-manager
+
+* **Were upgrade and rollback tested? Was the upgrade->downgrade->upgrade path tested?**
+  Manually tested.


Any findings?

oh, sorry, I thought I would be doing that before submitting the flag flip PR, I will do the test today.

I had some issues with gce-enforcer (deleting firewall rules from my k8s on gce cluster), will try again tomorrow.

done, works as expected.

Thanks - can you just added to the doc that non issues found?

keps/sig-apps/592-ttl-after-finish/README.md

janetkuo

LGTM. I'll let @wojtek-t do the PRR review.

wojtek-t · 2021-01-18T12:59:48Z

keps/sig-apps/592-ttl-after-finish/README.md

+  Unexpected restarts of kube-controller-manager
+
+* **Were upgrade and rollback tested? Was the upgrade->downgrade->upgrade path tested?**
+  Manually tested.


Thanks - can you just added to the doc that non issues found?

wojtek-t · 2021-01-18T13:01:02Z

keps/sig-apps/592-ttl-after-finish/README.md

+* **How can a rollout fail? Can it impact already running workloads?**
+  It shouldn't impact already running workloads. This is an opt-in feature since users need to 
+  explicitly set the TTLSecondsAfterFinished parameter in the job spec, which is silently dropped
+  by the api-server if the feature is disabled.


The field isn't dropped, right?
We're preserving the fields if they were set.

It is dropped: https://github.com/kubernetes/kubernetes/blob/d72c056260e771e8cd01220203582de0c0015786/pkg/registry/batch/job/strategy.go#L79

Hmm - which isn't really a pattern we want to have. We generally want to avoid clearing explicitly set fields, e.g. in:
https://github.com/kubernetes/kubernetes/blob/d72c056260e771e8cd01220203582de0c0015786/pkg/api/pod/util.go#L450

@liggitt - FYI

This is an existing behavior, do we want to change it?

I think we want - though please reach out to @liggitt (or other API approver) for final confirmation.

The current behavior looks correct.. if the feature is disabled, we only preserve the field if it was already set in the persisted object, and clear it otherwise. That

keps/sig-apps/592-ttl-after-finish/README.md

wojtek-t

I'm fine with it modulo one minor nit and the open comment about dropping field - please resolve this one with API approvers.

You also need sig-apps approval.

wojtek-t · 2021-01-19T14:02:38Z

keps/sig-apps/592-ttl-after-finish/README.md

+* **How can a rollout fail? Can it impact already running workloads?**
+  It shouldn't impact already running workloads. This is an opt-in feature since users need to 
+  explicitly set the TTLSecondsAfterFinished parameter in the job spec, which is silently dropped
+  by the api-server if the feature is disabled.


I think we want - though please reach out to @liggitt (or other API approver) for final confirmation.

keps/sig-apps/592-ttl-after-finish/README.md

keps/sig-apps/592-ttl-after-finish/kep.yaml

wojtek-t · 2021-01-19T14:08:24Z

You also need sig-apps approval.

Actually - I see Janet already approved. So I will just wait for the "drop fields" discussion resolution and will approve.

wojtek-t · 2021-01-19T15:26:30Z

LGTM

@ahg-g - please squash commits and I will approve

ahg-g · 2021-01-19T15:44:44Z

LGTM

@ahg-g - please squash commits and I will approve

thanks for the review, done!

wojtek-t · 2021-01-19T15:49:07Z

keps/sig-apps/592-ttl-after-finish/README.md

-promoted to beta after we finalize the decision for whether to generalize it or
-not, and when it satisfies users' need for cleaning up finished resource
-objects, without regressions.
+- The feature implemented for Job, as future work, it can be extended to Pods and perhaps custom resources, but that should happen under separate feature flags.


Your rebase didn't work - it seems you removed all the further commits....

ops, forgot to git add . :) should be fixed now

Enhancement issue: kubernetes#592

wojtek-t · 2021-01-19T16:48:19Z

Thanks!

/lgtm
/approve

k8s-ci-robot · 2021-01-19T16:48:36Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahg-g, janetkuo, wojtek-t

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/prod-readiness/OWNERS~~ [wojtek-t]
~~keps/sig-apps/OWNERS~~ [janetkuo]

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

ahg-g requested review from janetkuo and wojtek-t January 8, 2021 21:21

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 8, 2021

k8s-ci-robot requested review from mattfarina and prydonius January 8, 2021 21:21

k8s-ci-robot added the sig/apps Categorizes an issue or PR as relevant to SIG Apps. label Jan 8, 2021

ahg-g force-pushed the ahg-ttl branch 2 times, most recently from 745d0dd to 8ae83c7 Compare January 8, 2021 21:55

janetkuo reviewed Jan 8, 2021

View reviewed changes

keps/sig-apps/592-ttl-after-finish/README.md Outdated Show resolved Hide resolved

wojtek-t reviewed Jan 11, 2021

View reviewed changes

wojtek-t self-assigned this Jan 11, 2021

ahg-g force-pushed the ahg-ttl branch 4 times, most recently from 3ee19b3 to 2627e6f Compare January 11, 2021 19:59

wojtek-t reviewed Jan 12, 2021

View reviewed changes

ahg-g force-pushed the ahg-ttl branch 2 times, most recently from 72c9ab2 to 9581dd7 Compare January 13, 2021 14:49

wojtek-t reviewed Jan 14, 2021

View reviewed changes

janetkuo approved these changes Jan 14, 2021

View reviewed changes

ahg-g force-pushed the ahg-ttl branch from 9581dd7 to bc95492 Compare January 15, 2021 01:42

wojtek-t reviewed Jan 18, 2021

View reviewed changes

ahg-g force-pushed the ahg-ttl branch from de4f9bb to 46307f4 Compare January 18, 2021 14:11

wojtek-t reviewed Jan 19, 2021

View reviewed changes

ahg-g force-pushed the ahg-ttl branch from e3512b4 to 5aff159 Compare January 19, 2021 15:44

wojtek-t reviewed Jan 19, 2021

View reviewed changes

Updates the ttl-to-finished KEP to graduate the feature to Beta.

281068e

Enhancement issue: kubernetes#592

ahg-g force-pushed the ahg-ttl branch from 5aff159 to 281068e Compare January 19, 2021 15:52

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 19, 2021

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 19, 2021

k8s-ci-robot merged commit 70d927d into kubernetes:master Jan 19, 2021

k8s-ci-robot added this to the v1.21 milestone Jan 19, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updates the ttl-to-finished KEP to graduate the feature to Beta. #2246

Updates the ttl-to-finished KEP to graduate the feature to Beta. #2246

ahg-g commented Jan 8, 2021

ahg-g commented Jan 8, 2021

wojtek-t Jan 14, 2021

ahg-g Jan 14, 2021

ahg-g Jan 15, 2021

ahg-g Jan 15, 2021

wojtek-t Jan 18, 2021

ahg-g Jan 18, 2021

janetkuo left a comment

wojtek-t Jan 18, 2021

wojtek-t Jan 18, 2021

ahg-g Jan 18, 2021

wojtek-t Jan 18, 2021

ahg-g Jan 18, 2021

wojtek-t Jan 19, 2021

liggitt Jan 19, 2021

ahg-g Jan 19, 2021

wojtek-t left a comment

wojtek-t Jan 19, 2021

wojtek-t commented Jan 19, 2021

wojtek-t commented Jan 19, 2021

ahg-g commented Jan 19, 2021

wojtek-t Jan 19, 2021

wojtek-t Jan 19, 2021

ahg-g Jan 19, 2021

wojtek-t commented Jan 19, 2021

k8s-ci-robot commented Jan 19, 2021

Updates the ttl-to-finished KEP to graduate the feature to Beta. #2246

Updates the ttl-to-finished KEP to graduate the feature to Beta. #2246

Conversation

ahg-g commented Jan 8, 2021

ahg-g commented Jan 8, 2021

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

janetkuo 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

wojtek-t left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wojtek-t commented Jan 19, 2021

wojtek-t commented Jan 19, 2021

ahg-g commented Jan 19, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wojtek-t commented Jan 19, 2021

k8s-ci-robot commented Jan 19, 2021