-
Notifications
You must be signed in to change notification settings - Fork 14.5k
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
Add replicated MySQL tutorial #1722
Add replicated MySQL tutorial #1722
Conversation
you might also be interested in the mongodb stateful set example we did for openshift (when they were still called petsets): |
@@ -0,0 +1,144 @@ | |||
apiVersion: apps/v1beta1 |
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.
Perhaps by convention, we should have the headless service be in the same file as the statefulset itself? I think most other examples do that.
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.
In my Vitess chart, I found it useful to have multiple StatefulSets share a single headless service. Is that a reasonable pattern? If so, the convention of putting the headless service together with the StatefulSet wouldn't make sense. For the tutorial, I also like the idea of keeping the service separate to reduce noise, and so I can separately explain the parts. If we still want to stick to the convention though, I'll do that. Let me know what you think.
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.
+1 for having both in the same file, I like the convention and it's a bit easier to create from a single file rather than creating from two separate files.
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.
Since this initial comment thread, I've also added a client service in addition to the headless one. So now I have two services in mysql-services.yaml
. I also have mysql-configmap.yaml
, so 3 files total. I think if it was just a StatefulSet and one headless service, I would agree with putting them together to get to the one-file ideal. However, with 2 services and a ConfigMap, I feel like the separation adds enough clarity to be worth the extra files.
# Generate mysql server-id from pod ordinal index.\n | ||
[[ `hostname` =~ -([0-9]+)$ ]] || exit 1\n | ||
echo [mysqld] > /mnt/conf.d/server-id.cnf\n | ||
echo server-id=$((100 + ${BASH_REMATCH[1]})) >> /mnt/conf.d/server-id.cnf\n |
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.
Why do we do (100 + ordinal_idx)?
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.
MySQL server id 0 is reserved, so we need some offset on the ordinal index. I didn't want to use something like ordinal+1
because it would be easy to see server id 1 and mistakenly assume that corresponds to ordinal 1. I'll add a comment somewhere nearby to make this less mysterious.
|
||
echo "Initializing replication from clone position" | ||
[[ `cat xtrabackup_binlog_info` =~ ^(.*?)[[:space:]]+(.*?)$ ]] || exit 1 | ||
mv xtrabackup_binlog_info xtrabackup_binlog_info.orig |
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.
What happens if the container fails after this step?
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.
I wanted the CHANGE MASTER TO
to be executed at-most-once, because it's dangerous to re-point the replication position if replication has already started making progress. So, if the container fails after moving xtrabackup_binlog_info
but before finishing CHANGE MASTER TO
, it will leave the slave without any replication and the operator will need to resolve it.
|
Regarding the above questions:
|
47149b3
to
d9e3f5a
Compare
Reviewed 1 of 4 files at r1, 3 of 5 files at r2. docs/tutorials/replicated-stateful-application/mysql-statefulset.yaml, line 1 at r1 (raw file): Previously, enisoc (Anthony Yeh) wrote…> In my Vitess chart, I found it useful to have multiple StatefulSets share a single headless service. Is that a reasonable pattern? If so, the convention of putting the headless service together with the StatefulSet wouldn't make sense. For the tutorial, I also like the idea of keeping the service separate to reduce noise, and so I can separately explain the parts. If we still want to stick to the convention though, I'll do that. Let me know what you think.docs/tutorials/replicated-stateful-application/mysql-statefulset.yaml, line 2 at r2 (raw file):
We discussed this offline already, but given the lack of security, when the tutorial is presented, we want to stress that this is not a production ready example. Comments from Reviewable |
d9e3f5a
to
5bcc0e8
Compare
The tutorial itself is now ready for review. |
97d5f8f
to
8ec6913
Compare
@steveperry-53, can you handle the doc review on this one, as it's a Tutorial? |
Reviewed 6 of 6 files at r4, 1 of 1 files at r5. docs/tutorials/replicated-stateful-application/run-replicated-stateful-application.md, line 23 at r5 (raw file):
I like the phrasing of the above, and this addresses my earlier concern. docs/tutorials/replicated-stateful-application/run-replicated-stateful-application.md, line 33 at r5 (raw file):
Might want to just add the hyperlinks for Pods, Services, and Config Maps. docs/tutorials/replicated-stateful-application/run-replicated-stateful-application.md, line 99 at r5 (raw file):
Do you mean the SRV record associated with the master? Might want to explicit. docs/tutorials/replicated-stateful-application/run-replicated-stateful-application.md, line 137 at r5 (raw file):
We might want to link this to the Stateful Set concept, or the Stateful Set Basics tutorial when all three PRs get merged. docs/tutorials/replicated-stateful-application/run-replicated-stateful-application.md, line 152 at r5 (raw file):
Maybe "in the order in which they are defined" docs/tutorials/replicated-stateful-application/run-replicated-stateful-application.md, line 171 at r5 (raw file):
You might want to expand on why its important that docs/tutorials/replicated-stateful-application/run-replicated-stateful-application.md, line 211 at r5 (raw file):
Do you know what the behavior is when the A record pointed to by the SRV record is removed and re-added resulting in an IP Address change (e.g. The node hosting the master fails, and it is rescheduled to a new node)? Are we sure that, after the connection between the Master and Slaves fails, the Slaves will call getbyhostname to re-resolve the IP address associated with SRV recorded and that they do not cache the previously resolved address? docs/tutorials/replicated-stateful-application/run-replicated-stateful-application.md, line 480 at r5 (raw file):
You should indicate to the user that she needs to manually delete any provisioned storage (i.e. Persistent Volumes). Comments from Reviewable |
Review status: 7 of 10 files reviewed at latest revision, 10 unresolved discussions. docs/tutorials/replicated-stateful-application/run-replicated-stateful-application.md, line 33 at r5 (raw file): Previously, kow3ns (Kenneth Owens) wrote…> Might want to just add the hyperlinks for Pods, Services, and Config Maps.docs/tutorials/replicated-stateful-application/run-replicated-stateful-application.md, line 99 at r5 (raw file): Previously, kow3ns (Kenneth Owens) wrote…> Do you mean the SRV record associated with the master? Might want to explicit.docs/tutorials/replicated-stateful-application/run-replicated-stateful-application.md, line 137 at r5 (raw file): Previously, kow3ns (Kenneth Owens) wrote…> We might want to link this to the Stateful Set concept, or the Stateful Set Basics tutorial when all three PRs get merged.docs/tutorials/replicated-stateful-application/run-replicated-stateful-application.md, line 152 at r5 (raw file): Previously, kow3ns (Kenneth Owens) wrote…> Maybe "in the order in which they are defined"docs/tutorials/replicated-stateful-application/run-replicated-stateful-application.md, line 171 at r5 (raw file): Previously, kow3ns (Kenneth Owens) wrote…> You might want to expand on why its important that `0` is the canonical element of the set that is assigned to be the master. If the user fully understands how ordered Pod creation works and how MySQL master-slave replication works, they should be able to derive why this is so, but it might not hurt to be explicit.docs/tutorials/replicated-stateful-application/run-replicated-stateful-application.md, line 211 at r5 (raw file): Previously, kow3ns (Kenneth Owens) wrote…> Do you know what the behavior is when the A record pointed to by the SRV record is removed and re-added resulting in an IP Address change (e.g. The node hosting the master fails, and it is rescheduled to a new node)? Are we sure that, after the connection between the Master and Slaves fails, the Slaves will call getbyhostname to re-resolve the IP address associated with SRV recorded and that they do not cache the previously resolved address?docs/tutorials/replicated-stateful-application/run-replicated-stateful-application.md, line 480 at r5 (raw file): Previously, kow3ns (Kenneth Owens) wrote…> You should indicate to the user that she needs to manually delete any provisioned storage (i.e. Persistent Volumes).Comments from Reviewable |
d4b3879
to
80c8a89
Compare
Reviewed 6 of 6 files at r6. docs/tutorials/replicated-stateful-application/run-replicated-stateful-application.md, line 99 at r5 (raw file): Previously, enisoc (Anthony Yeh) wrote…
Yes, and the CNAME of the Headless Service fronts the SRV records which point to the A Records. I was indicating that "(through its DNS entry within the Headless Service)" might be a bit confusing. Not a blocker. docs/tutorials/replicated-stateful-application/run-replicated-stateful-application.md, line 137 at r5 (raw file): Previously, enisoc (Anthony Yeh) wrote…
First use sounds like a good start. docs/tutorials/replicated-stateful-application/run-replicated-stateful-application.md, line 152 at r5 (raw file): Previously, enisoc (Anthony Yeh) wrote…
I think what is lacking is the where. "In the order defined in the manifest" makes it more clear, but plugging that in makes the next sentence sound clunky. Since you have a link to Init Containers, users that want to explore that concept have a quick link. I'll bite. docs/tutorials/replicated-stateful-application/run-replicated-stateful-application.md, line 211 at r5 (raw file): Previously, enisoc (Anthony Yeh) wrote…
Sounds good, as long as the application isn't doing explicit address caching, it should converge as you say. Comments from Reviewable |
I'm starting a writing review now. |
Anthony, This tutorial looks great. Just a few comments: I don't think we need a new directory for this tutorial. I think it should go in the run-stateful-application directory. There are several places where I think you could use present tense instead of future tense. @jeffmendoza, @pwittrock, and I have discussed how to get config files into the hands of the reader. I haven't published this in our contributor guidelines yet, here's our current thought: Don't have the reader copy and paste the config file. Instead, create a $REPO variable, and then create the API object directly from the config file in the repo. Here's an example. Under Cleaning Up, do the steps need to be done in order? If so, let's use a numbered list instead of a bulleted list. In a few places, we might want to say "MySQL master" instead of "master." Some links are broken. I assume that's because the target topics aren't submitted yet. |
80c8a89
to
8d12e8c
Compare
@steveperry-53 I think I've addressed all your comments. The only remaining broken links should be those to the StatefulSet concept page, being added in #1719. |
The
Can be:
|
Added Tech Review LGTM from @kow3ns . Do we want to resolve @jeffmendoza 's comments before merging? |
The comment by @jeffmendoza is resolved in the final commit. |
Great. Merging now. |
cc @erictune @foxish @kow3ns @janetkuo @kubernetes/sig-apps
This is a replicated MySQL StatefulSet tutorial. It was inspired by #1599, but uses StatefulSet to achieve the following properties:
This example has the following (known) caveats:
This change is