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.

Sign up for GitHub

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

Jump to bottom

KEP 1847 StatefulSet autodelete documentation #30597

Merged
merged 1 commit into from
Dec 1, 2021
Merged

KEP 1847 StatefulSet autodelete documentation #30597

merged 1 commit into from
Dec 1, 2021

Conversation

mattcary
Copy link
Contributor

@k8s-ci-robot k8s-ci-robot added this to the 1.23 milestone Nov 22, 2021
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 22, 2021
@netlify
Copy link

netlify bot commented Nov 22, 2021 •
edited

👷 Deploy Preview for kubernetes-io-vnext-staging processing.

🔨 Explore the source changes: 40e06a6

🔍 Inspect the deploy log: https://app.netlify.com/sites/kubernetes-io-vnext-staging/deploys/61a7d0a2c815670007ffe3d9

@k8s-ci-robot k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Nov 22, 2021
@mattcary mattcary mentioned this pull request Nov 22, 2021
Closed
@k8s-ci-robot k8s-ci-robot added language/en Issues or PRs related to English language sig/docs Categorizes an issue or PR as relevant to SIG Docs. labels Nov 22, 2021
@jlbutler
Copy link
Contributor

/assign @nate-double-u

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Nov 22, 2021
@nate-double-u
Copy link
Contributor

nate-double-u commented Nov 23, 2021 •
edited

Deploy preview: https://deploy-preview-30597--kubernetes-io-vnext-staging.netlify.app/docs/concepts/workloads/controllers/statefulset/#persistentvolumeclaim-retention

sftim
sftim reviewed Nov 23, 2021
View reviewed changes
Copy link
Contributor

@sftim sftim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi.

Here's some suggestions. I hope they make sense and help the page improve.

I haven't tested them so there is a chance I've made a Markdown error. If so, sorry about that.

content/en/docs/concepts/workloads/controllers/statefulset.md Outdated Show resolved Hide resolved
content/en/docs/concepts/workloads/controllers/statefulset.md Outdated Show resolved Hide resolved
content/en/docs/concepts/workloads/controllers/statefulset.md Outdated Show resolved Hide resolved
content/en/docs/concepts/workloads/controllers/statefulset.md Outdated Show resolved Hide resolved
content/en/docs/concepts/workloads/controllers/statefulset.md Outdated Show resolved Hide resolved
content/en/docs/concepts/workloads/controllers/statefulset.md Outdated Show resolved Hide resolved
content/en/docs/concepts/workloads/controllers/statefulset.md Outdated Show resolved Hide resolved
content/en/docs/concepts/workloads/controllers/statefulset.md Outdated Show resolved Hide resolved
@mattcary
Copy link
Contributor Author

Hi.

Here's some suggestions. I hope they make sense and help the page improve.

I haven't tested them so there is a chance I've made a Markdown error. If so, sorry about that.

Thanks! They mostly make good sense. I'll commit them through github, then see if I can run a local server to double-check the markdown.

sftim
sftim reviewed Nov 23, 2021
View reviewed changes
Copy link
Contributor

@sftim sftim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://deploy-preview-30597--kubernetes-io-vnext-staging.netlify.app/docs/concepts/workloads/controllers/statefulset/#persistentvolumeclaim-retention LGTM

This'll need tech review from SIG Apps, as I haven't checked at all that this matches the actual behavior in code (should be a quick sign off, though).

@sftim
Copy link
Contributor

sftim commented Nov 23, 2021

@mattcary I reckon it'd be OK to squash commits at this point.

@mattcary
Copy link
Contributor Author

Thanks! Commits squashed, after some amount of git submodules struggling to get the local server to work :-/

/assign @smarterclayton
for sig-apps signoffs as he did the API review.

@sftim
Copy link
Contributor

sftim commented Nov 27, 2021

/sig apps

@k8s-ci-robot k8s-ci-robot added the sig/apps Categorizes an issue or PR as relevant to SIG Apps. label Nov 27, 2021
sftim
sftim reviewed Nov 27, 2021
View reviewed changes
Copy link
Contributor

@sftim sftim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Markdown LGTM too. Any remaining feedback on that side looks like nits.

@k8s-ci-robot k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 29, 2021
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 29, 2021
smarterclayton
smarterclayton reviewed Nov 30, 2021
View reviewed changes
content/en/docs/concepts/workloads/controllers/statefulset.md Outdated
For each policy that you can configure, you can set the value to either `Delete` or `Retain`.

`Delete`
: The PVCs created from the StatefulSet `volumeClaimTemplate` are deleted, for
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comma is a bit weird here but that may just be me.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks about the same to me, so I'll take it out :-)

smarterclayton
smarterclayton reviewed Nov 30, 2021
View reviewed changes
content/en/docs/concepts/workloads/controllers/statefulset.md Outdated
`Delete`
: The PVCs created from the StatefulSet `volumeClaimTemplate` are deleted, for
each Pod affected by the policy. With the `whenDeleted` policy, as all Pods are
deleted when the Statefulset is deleted, all PVCs from the `volumeClaimTemplate`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This clause as all Pods are deleted when the Statefulset is deleted is a bit difficult to follow. Might suggest

With the whenDeleted policy, each PVC from the volumeClaimTemplate will be deleted after the corresponding pod is deleted. [ed: you may want to describe exactly the ordering here - does is the ordering dependent on the foreground policy? Does the controller wait until it observes the pod is fully deleted? These help clarify how someone knows what to debug when it goes wrong].

(see my editor note, I think we should clarify the time order in words of what waits before what)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Improved the wording, or tried to, in the context of your comment below.

smarterclayton
smarterclayton reviewed Nov 30, 2021
View reviewed changes
content/en/docs/concepts/workloads/controllers/statefulset.md Outdated
PVC’s pod, and you usually don’t want Pod deletions to remove the associated PVCs. This means that the
StatefulSet controller must wait for scale-down events (when the StatefulSet replica count is changed
to be less than the number of replicas currently in the cluster), and then sets pod owner references
only on the specific Pods that are due to be terminated due to the scale-down action.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh... reading this sounds complicated :). Does this mean that the stateful set controller during a restart can "lose" a scale down event? Or is the event caught because there is a PVC still there when the controller restarts? As a user, I need to know whether force deleting a pod while the controller is down will result in the PVC being left around or not (probably as an api reviewer too :)), and this paragraph doesn't quite give clarity on that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clarified, hopefully.

@mattcary mattcary force-pushed the kep-1847 branch 3 times, most recently from 9233725 to bc8803c Compare November 30, 2021 23:32
smarterclayton
smarterclayton reviewed Dec 1, 2021
View reviewed changes
content/en/docs/concepts/workloads/controllers/statefulset.md Outdated
associated StatefulSet template PVCs, before the Pod is deleted. This causes the PVCs
to be garbage collected after only the condemned Pods have terminated.

This meanas that if the controller crashes and restarts, no Pod will be deleted before its
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo means

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oops thx

@smarterclayton
Copy link
Contributor

lgtm and content is good (one typo), thanks

@nate-double-u
Copy link
Contributor

/lgtm
/cc @jlbutler

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 1, 2021
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: bd7cc8a92d13a8da773e65c04c4d49d9ddaafd34

@jlbutler
Copy link
Contributor

jlbutler commented Dec 1, 2021

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jlbutler

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 1, 2021
@k8s-ci-robot k8s-ci-robot merged commit bbdb8c5 into kubernetes:dev-1.23 Dec 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@smarterclayton smarterclayton smarterclayton left review comments

@nate-double-u nate-double-u nate-double-u left review comments

@sftim sftim sftim left review comments

@kow3ns kow3ns Awaiting requested review from kow3ns

@jlbutler jlbutler Awaiting requested review from jlbutler

Assignees

@smarterclayton smarterclayton

@nate-double-u nate-double-u

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. language/en Issues or PRs related to English language lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/docs Categorizes an issue or PR as relevant to SIG Docs. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
1.23
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@mattcary @jlbutler @nate-double-u @sftim @smarterclayton @k8s-ci-robot