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

swarm example #482

Merged
merged 1 commit into from
May 22, 2018
Merged

swarm example #482

merged 1 commit into from
May 22, 2018

Conversation

Nowheresly
Copy link
Contributor

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.

@Nowheresly Nowheresly force-pushed the swarm branch 2 times, most recently from d9c9729 to 0db0644 Compare May 2, 2018 12:26
@sgotti
Copy link
Member

sgotti commented May 3, 2018

@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
Copy link
Contributor Author

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
Copy link
Contributor

ihcsim commented May 3, 2018

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

@Nowheresly
Copy link
Contributor Author

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.

build Outdated
@@ -98,7 +98,7 @@ cp ${GOPATH}/bin/keeper ${BINDIR}/stolon-keeper

# 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)
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

@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.

@@ -0,0 +1,20 @@
ifndef PGVERSION
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

same reply as above

@Nowheresly
Copy link
Contributor Author

Thanks @ihcsim for your review!

@ihcsim
Copy link
Contributor

ihcsim commented May 10, 2018

@sgotti This PR LGTM.

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

Choose a reason for hiding this comment

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

@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.

.gitignore Outdated
@@ -14,3 +14,5 @@ Session.vim
bin/
/gopath/
/release/

.idea/
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, this can be done in another PR

build Outdated
@@ -98,7 +98,7 @@ cp ${GOPATH}/bin/keeper ${BINDIR}/stolon-keeper

# 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
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, done


volumes:
pgkeeper1:
driver: local
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@Nowheresly Nowheresly May 11, 2018

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the review

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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.

@Nowheresly Nowheresly force-pushed the swarm branch 2 times, most recently from 3476b71 to 5d5e13b Compare May 13, 2018 20:49
build Outdated
@@ -98,7 +98,7 @@ cp ${GOPATH}/bin/keeper ${BINDIR}/stolon-keeper

# 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
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, sorry for that!

@sgotti
Copy link
Member

sgotti commented May 22, 2018

@Nowheresly LGTM. Merging. Thanks!

@sgotti sgotti merged commit 0d50135 into sorintlab:master May 22, 2018
sgotti added a commit that referenced this pull request May 22, 2018
@Nowheresly Nowheresly deleted the swarm branch May 22, 2018 20:53
@sgotti sgotti added this to the v0.11.0 milestone Jun 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants