swarm example by nowheresly · Pull Request #482 · sorintlab/stolon

nowheresly · 2018-05-01T19:42:47Z

Hi @sgotti
I was looking for a solution for postgres HA inside docker. So I decided to evaluate your project and played a little bit with your very well documented kubernetes example.
At the end, I was able to create a simple swarm version based on the kubernetes example.
So in case you could be interested in reviewing this swarm example, I would really appreciate to get your feedback.

sgotti · 2018-05-03T09:08:16Z

@nowheresly Thanks for the PR!

We already have docker swarm example here. https://github.com/sorintlab/stolon/tree/master/examples/docker contributed by @ihcsim . I'm not sure how to proceed, is your example very different from the current one?

nowheresly · 2018-05-03T20:38:25Z

Thanks for the reply.
Yes, I already looked at it. But it seemed to me not as up-to-date as the kubernetes example IMO.
This PR uses docker secret, rolling upgrades and network overlay as far as swarm is concerned while using the same docker image as the one in use with the kubernetes example.
If @ihcsim could have a look at this PR, I would be glad to read his comments as well.

ihcsim · 2018-05-03T20:44:34Z

@nowheresly You can just remove my outdated example in your PR.

nowheresly · 2018-05-08T18:58:30Z

Based on the comment of @ihcsim, I just added a commit to remove the previous swarm example.
i can squash the two commits if needed.

ihcsim · 2018-05-09T22:32:27Z

 # Copy binaries to Dockerfile image directories
 declare -a DOCKERFILE_PATHS
-DOCKERFILE_PATHS=(${BASEDIR}/examples/kubernetes/image/docker ${BASEDIR}/examples/docker)
+DOCKERFILE_PATHS=(${BASEDIR}/examples/kubernetes/image/docker ${BASEDIR}/examples/swarm/image/docker ${BASEDIR}/examples/docker)


${BASEDIR}/examples/docker is removed, right?

Yes, you're right. PR adapted. Thanks!

ihcsim · 2018-05-09T22:49:36Z

@@ -0,0 +1,9 @@
+FROM postgres:${PGVERSION}


Isn't this template the same as the one in examples/kubernetes/image?

Yes indeed. The content of both examples/kubernetes/image and examples/swarm/image are the same. My goal with this PR was to update the swarm example base on the kubernetes example. So I chose to use the same docker images for both examples.

@nowheresly I'd like to avoid maintaining multiple set of identical files. I think we could remove these files and update the documentation to point to the examples/kubernetes/image.

ihcsim · 2018-05-09T22:49:50Z

@@ -0,0 +1,20 @@
+ifndef PGVERSION


Isn't this Makefile the same as the one in examples/kubernetes/image?

same reply as above

nowheresly · 2018-05-10T08:28:51Z

Thanks @ihcsim for your review!

ihcsim · 2018-05-10T20:11:23Z

@sgotti This PR LGTM.

sgotti · 2018-05-11T13:15:45Z

@@ -0,0 +1,9 @@
+FROM postgres:${PGVERSION}


@nowheresly I'd like to avoid maintaining multiple set of identical files. I think we could remove these files and update the documentation to point to the examples/kubernetes/image.

sgotti · 2018-05-11T13:16:39Z

 /gopath/
 /release/
+
+.idea/


Don't add changes to .gitignore in this PR.

Sure, this can be done in another PR

sgotti · 2018-05-11T13:21:14Z

 # Copy binaries to Dockerfile image directories
 declare -a DOCKERFILE_PATHS
-DOCKERFILE_PATHS=(${BASEDIR}/examples/kubernetes/image/docker ${BASEDIR}/examples/docker)
+DOCKERFILE_PATHS=(${BASEDIR}/examples/kubernetes/image/docker ${BASEDIR}/examples/swarm/image/docker


Since I'd like to keep only one image let's remove this.

sgotti · 2018-05-11T13:43:42Z

+
+volumes:
+  pgkeeper1:
+    driver: local


Does using the local volume plugin will create some problems since on multiple node cluster since the keeper cannot be started on another node than the one where the volume has been created? Perhaps some notes to change this to a real persistent volume will be good.

You're right, this section deserves probably more explanations.
Out of the box, only the local driver is available with docker swarm. That's the reason why this swarm example is based on the local driver.
I specifically chose to provide different names for volumes of keeper1 and keeper2 so that we can't have any volume mismatch if keeper1 and keeper2 are deployed on the same node of the swarm.
Nevertheless, IMHO, it makes no sense to deploy all keepers on the same node. So I would recommend to set some constraints on both keepers so that each node would have its own keeper. Here is an example of a configuration where keeper1 will be deployed only on a node whose nodename label is equal to node1:

keeper1: deploy: placement: constraints: [node.labels.nodename == node1] ...

In case each keeper is deployed on its own node, a node failure should not prevent the cluster to remain in function.

I'm not sure if my explanations are clear to you. I would propose to add a comment in the README to explain this approach. @sgotti What do you think?

Of course, if a specific docker volume plugin is available that could enable to share volumes between nodes, the volume section of the docker-compose-pg.yml would have to be adapted accordingly to leverage this plugin and there's no need to set any constraints.

@nowheresly Your explanation is right and I agree with you. A comment in the README will be great!

Thanks for the review

@sgotti I updated the README to explain how to set labels on each node and how to set constraints on each keeper. Hopefully, it should make sense to swarm users.

sgotti · 2018-05-14T07:40:28Z

 # Copy binaries to Dockerfile image directories
 declare -a DOCKERFILE_PATHS
-DOCKERFILE_PATHS=(${BASEDIR}/examples/kubernetes/image/docker ${BASEDIR}/examples/docker)
+DOCKERFILE_PATHS=(${BASEDIR}/examples/kubernetes/image/docker


There's a missing closing bracket that is breaking the build

Indeed, sorry for that!

sgotti · 2018-05-22T11:22:42Z

@nowheresly LGTM. Merging. Thanks!

swarm example

nowheresly force-pushed the swarm branch 2 times, most recently from d9c9729 to 0db0644 Compare May 2, 2018 12:26

nowheresly force-pushed the swarm branch from 0db0644 to c642ba3 Compare May 8, 2018 18:54

ihcsim reviewed May 9, 2018

View reviewed changes

nowheresly force-pushed the swarm branch from b64a78b to b8a23ae Compare May 10, 2018 08:20

sgotti requested changes May 11, 2018

View reviewed changes

nowheresly force-pushed the swarm branch 2 times, most recently from 3476b71 to 5d5e13b Compare May 13, 2018 20:49

sgotti reviewed May 14, 2018

View reviewed changes

update of swarm example

0d50135

nowheresly force-pushed the swarm branch from 5d5e13b to 0d50135 Compare May 16, 2018 19:19

sgotti approved these changes May 22, 2018

View reviewed changes

sgotti merged commit 0d50135 into sorintlab:master May 22, 2018

sgotti added a commit that referenced this pull request May 22, 2018

Merge pull request #482 from Nowheresly/swarm

1454745

swarm example

nowheresly deleted the swarm branch May 22, 2018 20:53

sgotti added this to the v0.11.0 milestone Jun 7, 2018

Conversation

nowheresly commented May 1, 2018

Uh oh!

sgotti commented May 3, 2018

Uh oh!

nowheresly commented May 3, 2018

Uh oh!

ihcsim commented May 3, 2018

Uh oh!

nowheresly commented May 8, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nowheresly commented May 10, 2018

Uh oh!

ihcsim commented May 10, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nowheresly May 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sgotti commented May 22, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

nowheresly May 11, 2018 •

edited

Loading