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

Notice of deprecation for recycler. #36760

Closed
wants to merge 4 commits into from

Conversation

childsb
Copy link
Contributor

@childsb childsb commented Nov 14, 2016

This PR is to give notice to user that the recycler function will be removed.

The recycler is difficult to support, breaks in many scenarios and the use cases its covering are replaced with dynamic provision and deletion of volumes.


This change is Reviewable

@childsb childsb added the sig/storage Categorizes an issue or PR as relevant to SIG Storage. label Nov 14, 2016
@k8s-github-robot k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note-label-needed labels Nov 14, 2016
@k8s-ci-robot
Copy link
Contributor

Jenkins verification failed for commit 9a67951. Full PR test history.

The magic incantation to run this job again is @k8s-bot verify test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@saad-ali saad-ali added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels Nov 22, 2016
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Nov 22, 2016
@saad-ali saad-ali added this to the v1.5 milestone Nov 22, 2016
@saad-ali
Copy link
Member

/lgtm

CC @kubernetes/sig-storage

Please fix broken munge docs:

FAILED   hack/make-rules/../../hack/verify-munge-docs.sh	16s

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 22, 2016
@saad-ali
Copy link
Member

We should also update the official k8s documentation to indicate that the recycler policy will be deprecated and should not be used--those docs will be much more user visible.

@@ -0,0 +1,32 @@
## Deprecation of Persistent Volume Recycler Behavior
The function of the persistent volume recycler will change in future kubernetes releases. The current implementation will remain and function as is for the short term but is expected to be removed or replaced in future releases.
Copy link
Member

Choose a reason for hiding this comment

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

We can't really remove it. It's part of the v1 API.


Additionally, delete/create is an admin control plane operation and the node hosting the recycle POD will need network access to the storage control plane and the admin credentials. Its desirable to isolate storage control plan from the user network for security.

## Rational
Copy link
Member

Choose a reason for hiding this comment

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

Rationale ?

## Timeline

* Kube 1.5 : Notice of deprecation
* Kube 1.5 + 1 year : Remove old recycler API
Copy link
Member

Choose a reason for hiding this comment

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

This is, unfortunately, not my interpretation of the API compat guarantees. I think we have to keep this for all v1 installations. When we launch API v2, we can remove this from v2 after now + 1 year (which is likely to happen before v2) launches.

@bgrant0607 - have we any precedent for API deprecations yet?

Copy link
Member

Choose a reason for hiding this comment

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

@thockin pod.spec.serviceAccount has been deprecated since 1.0, but has not been removed. node.status.phase has been deprecated and is not populated, but has not been removed.

Technically, we're not supposed to remove fields in the current API version. As I recall, we had planned to remove deprecated fields in the next major API version:
https://github.com/kubernetes/kubernetes/blob/master/docs/design/versioning.md

However, I could be persuaded that 1 year beyond deprecation is sufficient, assuming that we add fine-grain versioning (#34508).

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, Brian - that was my comprehension, too.

If we add finer-grained API versioning, I could likewise be convinced. Absent that, we can mark this as deprecated but we can't remove it. Removing the functionality but leaving the API is pretty sketchy, even.

Copy link
Member

Choose a reason for hiding this comment

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

@childsb Can we change the language here to indicate that this will be removed from the next API version, but will remain in the v1 API

@@ -0,0 +1,32 @@
## Deprecation of Persistent Volume Recycler Behavior
Copy link
Member

Choose a reason for hiding this comment

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

I don't know where this file should live, but the code is probably not the right place. Maybe in the API docs repo, linked from the docs about the feature?

@devin-donnelly

Copy link
Member

Choose a reason for hiding this comment

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

@kubernetes/docs I know the release is imminent, but I want to put this into the queue. Do we have a doc or index of docs that cover deprecated features?

@k8s-github-robot
Copy link

/lgtm cancel //PR changed after LGTM, removing LGTM. @childsb @saad-ali

@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 22, 2016
@childsb
Copy link
Contributor Author

childsb commented Nov 22, 2016

I updated with the concerns in the PR except for 'where should it live'.

If this should move, let me know where. If we dont know i suggest we merge as is for 1.5 and defer to cunninghams law.

@dims
Copy link
Member

dims commented Nov 23, 2016

@saad-ali ping

**USE RECYCLE API AT YOUR OWN RISK AS IT MAY BE REMOVED OR SIGNIFICANTLY CHANGED IN FUTURE KUBERNETES RELEASES**.

## Problem Scenarios
**Scenario 1**: User attaches a volume to their POD and is assigned a suplimental group but their default group is some other group. New files are created as the default group and for the recycler to remove them it would have to run as the default group. The scenario is similar and problimatic for chown'ed and chgroup'ed files. The recycler has no way to determine at launch which group to run as to remove every file on a recycled volume.
Copy link
Contributor

@wattsteve wattsteve Nov 23, 2016

Choose a reason for hiding this comment

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

spelling - supplemental and problematic

**Scenario 2**: Malicious user writes a POD that uses debugfs to inspect attached volumes for previously deleted files. Malicious user would gain access to "recycled" files from other users.

To work around either of these scenarios kubernetes will need to delete the partition on the storage volume and re-create. In the current architecture the volume attached to the recycler is unavialble for deletion as its in use.

Copy link
Contributor

Choose a reason for hiding this comment

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

spelling - unavailable

@thockin
Copy link
Member

thockin commented Nov 30, 2016

I spoke with @devin-donnelly on this, and he's going to find a place for us to register and store deprecation notices such as this.

@mbohlool we should probably consider another annotation on fields and structs to indicate that they are deprecated, but we need to spec not just that it is deprecated, but also where to find more information, and maybe when or what what API version it will stop working in.

@spiffxp
Copy link
Member

spiffxp commented Nov 30, 2016

I agree we need something nicer for deprecation notices, for now does anyone mind if I just ping the 1.5 known issues accumulator (#37134) and use that as my reference point?

@thockin
Copy link
Member

thockin commented Nov 30, 2016 via email

@saad-ali
Copy link
Member

saad-ali commented Dec 3, 2016

I spoke with @devin-donnelly on this, and he's going to find a place for us to register and store deprecation notices such as this.

We should get that set up within the 1.5 timeframe (i.e. <1 week).

@dims
Copy link
Member

dims commented Dec 9, 2016

@childsb looks like this missed the boat. Move the milestone please if appropriate?

@saad-ali
Copy link
Member

Note that the 1.5.0 release notes are currently pointing to this PR for "notice of deprecation for recycler". Let's find a permanent place for this notice and update the release notes link to point to that.

@childsb childsb removed this from the v1.5 milestone Dec 22, 2016
@thockin
Copy link
Member

thockin commented Dec 23, 2016 via email

@k8s-github-robot
Copy link

[APPROVALNOTIFIER] Needs approval from an approver in each of these OWNERS Files:

We suggest the following people:
cc @jsafrane
You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-github-robot k8s-github-robot assigned thockin and unassigned saad-ali Jan 30, 2017
@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 23, 2017
@k8s-github-robot
Copy link

This PR hasn't been active in 90 days. Closing this PR. Please reopen if you would like to work towards merging this change, if/when the PR is ready for the next round of review.

cc @childsb @saad-ali @thockin

You can add 'keep-open' label to prevent this from happening again, or add a comment to keep it open another 90 days

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/storage Categorizes an issue or PR as relevant to SIG Storage. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants