Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

apps/StatefulSets: Garbage collector should be able to orphan ControllerRevisions too #84984

Open
wants to merge 1 commit into
base: master
from

Conversation

@cofyc
Copy link
Member

cofyc commented Nov 8, 2019

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespace from that line:

/kind api-change
/kind bug

/kind cleanup
/kind design
/kind documentation
/kind failing-test
/kind feature
/kind flake

What this PR does / why we need it:

Garbage collector should be able to orphan ControllerRevisions too.

Right now when a stateful set is deleted by propgationPolicy=Orphan, the GC will orphan its controller revisions. However, controller revisions will be adopted by the deleted StatefulSet again.
Later, controller revisions will be deleted by the GC. So it's not possible to keep the previous controller revisions.

Which issue(s) this PR fixes:

Fixes #84982

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:


@cofyc

This comment has been minimized.

Copy link
Member Author

cofyc commented Nov 8, 2019

/sigs apps

@k8s-ci-robot k8s-ci-robot added sig/apps and removed needs-sig labels Nov 8, 2019
@k8s-ci-robot k8s-ci-robot requested review from krmayankk and soltysh Nov 8, 2019
return fmt.Errorf("original StatefulSet %v/%v is gone: got uid %v, wanted %v", set.Namespace, set.Name, fresh.UID, set.UID)
canAdoptErr := ssc.canAdoptFunc(set)()
if canAdoptErr != nil {
return fmt.Errorf("can't adopt ControllerRevisions: %v", canAdoptErr)

This comment has been minimized.

Copy link
@cofyc

cofyc Nov 8, 2019

Author Member

StatefulSet does not use ControllerRevisionControllerRefManager like DaemonSet to manage controller revisions, so this is quick fix without chaning too much.

Maybe we can migrate to use ControllerRevisionControllerRefManager in the future?

@cofyc

This comment has been minimized.

Copy link
Member Author

cofyc commented Nov 8, 2019

/assign @janetkuo

@janetkuo

This comment has been minimized.

Copy link
Member

janetkuo commented Nov 14, 2019

/approve

Thanks, good catch!
Need another reviewer to lgtm.

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Nov 14, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cofyc, janetkuo

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:

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

@cofyc

This comment has been minimized.

Copy link
Member Author

cofyc commented Nov 15, 2019

@janetkuo Thanks!

@soltysh @krmayankk can you take a look at?

@cofyc

This comment has been minimized.

Copy link
Member Author

cofyc commented Nov 19, 2019

cc @enisoc can you take a look?

@cofyc

This comment has been minimized.

Copy link
Member Author

cofyc commented Nov 19, 2019

/cc @enisoc
Thanks!

@k8s-ci-robot k8s-ci-robot requested a review from enisoc Nov 19, 2019
@cofyc

This comment has been minimized.

Copy link
Member Author

cofyc commented Nov 21, 2019

/cc @kow3ns
/cc @smarterclayton
can you take a look?

@k8s-ci-robot k8s-ci-robot requested review from kow3ns and smarterclayton Nov 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.