Skip to content

Trello: Pospispa 566 postpone pvc deletion if used in a pod #7577

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
Merged
2 commits merged into from Feb 13, 2018
Merged

Trello: Pospispa 566 postpone pvc deletion if used in a pod #7577

2 commits merged into from Feb 13, 2018

Conversation

ghost
Copy link

@ghost ghost commented Feb 5, 2018

@ghost ghost added branch/enterprise-3.6 branch/enterprise-3.7 branch/enterprise-3.9 peer-review-needed Signifies that the peer review team needs to review this PR labels Feb 5, 2018
@ghost ghost added this to the Next Release milestone Feb 5, 2018
@ghost ghost self-assigned this Feb 5, 2018
@openshift-ci-robot openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Feb 5, 2018
liangxia
liangxia reviewed Feb 6, 2018
View reviewed changes
install_config/configuring_pvc_protection.adoc Outdated
[source, yaml]
----
kubeletArguments:
feature-gates:
Copy link
Member

Choose a reason for hiding this comment

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

2 spaces indent above, but 3 spaces here.

liangxia
liangxia reviewed Feb 6, 2018
View reviewed changes
install_config/configuring_pvc_protection.adoc Outdated
----
kubeletArguments:
feature-gates:
- PVCProtection=true
Copy link
Member

Choose a reason for hiding this comment

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

This line should be the same indent level as above line.

@ghost
Copy link
Author

ghost commented Feb 6, 2018

@liangxia Changes made, please review. Thanks!

@liangxia
Copy link
Member

liangxia commented Feb 7, 2018

LGTM now. Thanks.

@ghost
Copy link
Author

ghost commented Feb 7, 2018 •
edited by ghost
Loading

[rev_history]
|xref:../architecture/additional_concepts/storage.adoc#architecture-additional-concepts-storage[Additional Concepts -> Persistent Storage]
|Added the xref:../architecture/additional_concepts/storage.adoc#pvcprotection[Configuring for Local Volume] section.
%
|xref:../install_config/configuring_pvc_protection.adoc[Configuring Persistent Volume Claim Protection]
|Added the xref:../install_config/configuring_pvc_protection.adoc#install-config-configuring-pvc-protection[Configuring Persistent Volume Claim Protection] section.
%

Persistent Volume Claim Protection
b870b11 
PVC Protection alpha feature was added to K8s 1.9 in PRs:
- kubernetes/kubernetes#55824
- kubernetes/kubernetes#55873

and documented in K8s 1.9 in PR:
- kubernetes/website#6415

That's why the PVC Protection is a new alpha feature in OpenShift 3.9 and that's why PVC Protection documentation is added.
@openshift-ci-robot openshift-ci-robot added 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 Feb 7, 2018
@ghost
Copy link
Author

ghost commented Feb 12, 2018

@openshift/team-documentation PTAL. Rev history in the previous comment.

bfallonf
bfallonf reviewed Feb 12, 2018
View reviewed changes
install_config/configuring_pvc_protection.adoc
@@ -0,0 +1,59 @@
[[install-config-configuring-pvc-protection]]
= Configuring Persistent Volume Claim Protection
Copy link

Choose a reason for hiding this comment

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

@tmorriso-rh Is this configuration specific to installation time? I'm wondering if this file would be better at home in the Admin Guide. Perhaps even in the Managing Pods section:

https://docs.openshift.com/container-platform/3.7/admin_guide/managing_pods.html

Copy link
Author

Choose a reason for hiding this comment

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

Yes, this is specific to installation.

bfallonf
bfallonf reviewed Feb 12, 2018
View reviewed changes
install_config/configuring_pvc_protection.adoc
[NOTE]
====
PVC protection is an alpha feature and may change in a future release of {product-title}.
====
Copy link

Choose a reason for hiding this comment

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

It might be good to add the description of why someone would want to do this from lines 117-124 in the file you've changed above.

Copy link
Author

Choose a reason for hiding this comment

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

I added this information. Thanks.

@bfallonf
Copy link

@tmorriso-rh I made some comments. Let me know what you think!

@ghost ghost merged commit 513488c into openshift:master Feb 13, 2018
@ghost
Copy link
Author

ghost commented Feb 13, 2018

/cherrypick enterprise-3.6-stage

@openshift-cherrypick-robot
Copy link

@tmorriso-rh: #7577 failed to apply on top of branch "enterprise-3.6-stage":

error: Failed to merge in the changes.
Using index info to reconstruct a base tree...
M	_topic_map.yml
M	architecture/additional_concepts/storage.adoc
Falling back to patching base and 3-way merge...
Auto-merging architecture/additional_concepts/storage.adoc
Auto-merging _topic_map.yml
CONFLICT (content): Merge conflict in _topic_map.yml
Patch failed at 0001 Persistent Volume Claim Protection

In response to this:

/cherrypick enterprise-3.6-stage

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@vikram-redhat
Copy link
Contributor

@tmorriso-rh - there are 2 commits in this PR. Commits should be squashed.

@vikram-redhat vikram-redhat modified the milestones: Next Release, Staging, Published - 02/16/2018 Feb 16, 2018
@ghost ghost mentioned this pull request Feb 20, 2018
Merged
@ghost ghost removed branch/dedicated branch/enterprise-3.6 branch/enterprise-3.7 branch/online peer-review-needed Signifies that the peer review team needs to review this PR labels Feb 23, 2018
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
branch/enterprise-3.9 size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
Published - 02/16/2018
Development

Successfully merging this pull request may close these issues.
5 participants
@liangxia @bfallonf @openshift-cherrypick-robot @vikram-redhat @openshift-ci-robot