-
Notifications
You must be signed in to change notification settings - Fork 14.4k
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
Point the old bootstrapping tutorial URL at new StatefulSet docs #1707
Point the old bootstrapping tutorial URL at new StatefulSet docs #1707
Conversation
@@ -16,8 +16,8 @@ spec: | |||
selector: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we fix terminationGracePeriod for these fixtures not to be zero?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
Is the main purpose of this tutorial to teach users how to run a VM-like workload? Or is it to demonstrate StatefulSet patterns? If it's the latter, I think there will be a lot of overlap with #1722 which uses the same patterns, but for MySQL. |
@enisoc the main purpose of this tutorial is teaching users how Kubenetes + pods are different from VMs, based on http://kubernetes.io/docs/user-guide/petset/bootstrapping/, and deprecating the bootstrapping guide |
OK in that case I'll try to minimize overlap in my tutorial. |
|
||
{% capture overview %} | ||
|
||
This page helps you become familiar with the runtime initialization of [Stateful Sets](/docs/user-guide/petset). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not necessarily new to this change, but the topic of this item is "VM-style workload" but nothing in the first 40 lines explains why that is. A sentence in the overview or objectives that explains how the title relates to what is being shown would be good.
I'm starting a writing review now. |
See this commit for suggested changes. |
Reviewed 7 of 8 files at r1, 2 of 2 files at r2, 1 of 1 files at r5. _data/tutorials.yml, line 56 at r5 (raw file):
Merge StatefulSet into one word in title and URL. docs/tutorials/stateful-application/vm-style-workload-using-stateful-set.md, line 29 at r5 (raw file):
Remove extra "a" before "feature". docs/tutorials/stateful-application/vm-style-workload-using-stateful-set.md, line 51 at r5 (raw file):
Add "the" at the end. docs/tutorials/stateful-application/vm-style-workload-using-stateful-set.md, line 73 at r5 (raw file):
Double "the". docs/tutorials/stateful-application/vm-style-workload-using-stateful-set.md, line 84 at r5 (raw file):
Double "of". docs/tutorials/stateful-application/vm-style-workload-using-stateful-set.md, line 201 at r5 (raw file):
Add "the" at the end. docs/tutorials/stateful-application/vm-style-workload-using-stateful-set.md, line 315 at r5 (raw file):
We should elaborate on why this is important, especially since my replicated MySQL tutorial does the opposite. The difference is in this case you're allowing the master to change dynamically, whereas mine requires that the master is always index 0. docs/tutorials/stateful-application/vm-style-workload-using-stateful-set.md, line 323 at r5 (raw file):
TODO: Add a link to Replicated Stateful Application tutorial. Comments from Reviewable |
Review status: all files reviewed at latest revision, 11 unresolved discussions, some commit checks failed. docs/tutorials/stateful-application/statefulset_vm.yaml, line 36 at r5 (raw file):
The overall script should somehow exit with a non-zero error code if the whole process didn't succeed. You don't want to run the real workload if the initialization was incomplete. In Bash you can just add In general, this approach of trying to merge in potential changes from the image (with At the least, we should warn that you shouldn't actually do this; it's just a toy example to demonstrate some concepts. A step further would be to do nothing at all if the PVC has ever been initialized before. That would at least be safer, although it means you should never change the image. Personally my recommendation is to drop this tutorial altogether for now. If there are still concepts we want to teach that aren't covered in other tutorials, we should start over and rethink how to present them in a less misleading way. I suspect I'm in the minority in that so I'll defer to the wisdom of the team. Comments from Reviewable |
To summarize: we discussed this offline and agreed to remove the VM-style tutorial for now because our other new StatefulSet tutorials cover the important concepts with use cases that have clearer motivations. This PR will be repurposed to just point the old bootstrapping tutorial URL at new StatefulSet docs. |
PR & title updated, PTAL |
LGTM |
* This document has been deprecated. For information on working with StatefulSet, see the tutorial on [how to run replicated stateful applications](/docs/tutorials/stateful-application/run-replicated-stateful-application). | ||
* Starting in Kubernetes version 1.5, PetSet has been renamed to [StatefulSet](/docs/concepts/abstractions/controllers/statefulsets). To use (or continue to use) PetSet in Kubernetes 1.5, you _must_ migrate your exsiting PetSets to StatefulSets. For information on working with StatefulSet, see the tutorial on [how to run replicated stateful applications](/docs/tutorials/stateful-application/run-replicated-stateful-application). | ||
|
||
* __This document has been deprecated__, but can still apply if you're using Kubernetes version 1.4 or earlier. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're going to say this document is deprecated but can still apply, then we need to keep the content of the document.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep. I'm suggesting below that we do not actually delete the content.
@devin-donnelly I think it would make sense to keep the doc if it was helping 1.4 users to maintain existing PetSets (like if it was a Task page describing how to do a manual rolling update), but to me the point of this particular doc is encouraging users to adopt PetSet for new use cases. Since PetSet is deprecated, our plan was to discourage against creating new PetSets and instead recommend upgrading to 1.5+ and using StatefulSets. |
@enisoc I understand. The only users I feel like we might miss are those that don't want to risk upgrading off 1.4 but still want to use PetSet (despite it being Alpha). I don't know if there are a lot of those users out there; any insight? |
@enisoc After a close reading, I strike my objection. Is there existing documentation that's a more basic "PetSet Guide" that needs to be marked as deprecated, however? |
@janetkuo Would you like to squash commits before we merge this? |
@devin-donnelly Yeah I think user-guide/petset fits your description for the basic guide that should probably be kept with just a deprecation warning at the top. |
--- | ||
|
||
* TOC | ||
{:toc} | ||
Starting in Kubernetes version 1.5, PetSet has been renamed to [StatefulSet](/docs/concepts/abstractions/controllers/statefulsets). To use (or continue to use) PetSet in Kubernetes 1.5, you _must_ migrate your exsiting PetSets to StatefulSets. For information on working with StatefulSet, see the tutorial on [how to run replicated stateful applications](/docs/tutorials/stateful-application/run-replicated-stateful-application). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
existing*
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And should we link "migrate ..." directly to the PetSet upgrade task?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done and done.
I didn't see a PR yet for the above so created #1874 |
I'll go ahead and merge this. |
cc @bprashanth @erictune @foxish @kow3ns @enisoc @kubernetes/sig-apps
This change is