Replica set example that uses PetSet #184

php-coder · 2016-09-21T16:31:06Z

UPDATE: PR will be merged as #206

This PR adds a new template with MongoDB replica set that uses PetSet.

Here is the quote from "Design Assumptions" of PetSets:

No generic cluster lifecycle - Rather than design a general purpose lifecycle for clustered software, focus on ensuring the information necessary for the software to function is available. For example, rather than providing a "post-creation" hook invoked when the cluster is complete, provide the necessary information to the "first" (or last) pod to determine the identity of the remaining cluster members and allow it to manage its own initialization.

Based on that, I've changed how we're initializing replica set -- instead of using a hook pod that runs at the end of deployment, we're initializing a replica set with a single member on the first node. All other nodes join to replica set during startup.

This also simplifies code and removes 2 extra elections that we had before (see #140). Now the first node is PRIMARY and we don't do any elections during first run of replica set.

Trello card: https://trello.com/c/jPGXhxlJ/694-3-mongodb-replica-set-example-with-persistent-storage

@bparees @rhcarvalho @omron93 I would like to hear your feedback.

Fixes #114

Left to do:

fix todo in scripts
fix todo in docs
explain new scheme of changing password
~~add readiness/liveness probes~~ (resolution: readiness probe breaks all the things, so I put it off)

bparees · 2016-09-22T07:56:54Z

2.4/examples/replica/mongodb-clustered-petset.yaml

+    value: rs0
+  # It must match with Service.metadata.name and PetSet.spec.serviceName
+  - name: MONGODB_SERVICE_NAME
+    description: "The name of the MongoDB Service (used to DNS lookup)"


"The name of the MongoDB service, used for DNS lookup"

bparees · 2016-09-22T07:57:54Z

2.4/examples/replica/mongodb-clustered-petset.yaml

+  - name: MONGODB_KEYFILE_VALUE
+    description: "The value of the MongoDB Key (see for details: https://docs.mongodb.org/v2.4/tutorial/generate-key-file/)"
+    generate: expression
+    from: "[a-zA-Z0-9]{255}"


we should start using the displayname field in our parameters too: https://github.com/openshift/origin/blob/master/examples/quickstarts/dancer-mysql.json#L449-L453

bparees · 2016-09-22T07:58:30Z

2.4/examples/replica/mongodb-clustered-petset.yaml

+    kind: Service
+    metadata:
+      # It must match with PetSet.spec.serviceName and MONGODB_SERVICE_NAME
+      name: mongodb


if it needs to match, why aren't we using a parameter value?

bparees · 2016-09-22T08:06:56Z

2.4/examples/replica/mongodb-clustered-petset.yaml

+          name: mongodb
+      # will route traffic to pods having labels matching this selector
+      selector:
+        app: mongo


parameter to ensure matching?

Because selector is a key/value pair and we can't create parameter for both of them, then let's leave it as-is?

https://github.com/openshift/origin/blob/master/examples/quickstarts/dancer-mysql.json#L34

https://github.com/openshift/origin/blob/master/examples/quickstarts/dancer-mysql.json#L153-L155

IOW you still want to extract value to parameter. OK, will do.

bparees · 2016-09-22T08:10:52Z

2.4/examples/replica/mongodb-clustered-petset.yaml

+    spec:
+      # pets get DNS/hostnames that follow the pattern: ${metadata.name}-NUM.${spec.serviceName}.default.svc.cluster.local
+      # It must match with Service.metadata.name and MONGODB_SERVICE_NAME
+      serviceName: mongodb


bparees · 2016-09-22T08:11:16Z

2.4/examples/replica/mongodb-clustered-petset.yaml

+        metadata:
+          # this label will be used for count running pods
+          labels:
+            app: mongo


bparees · 2016-09-22T08:13:00Z

2.4/examples/replica/mongodb-clustered-petset.yaml

+        spec:
+          containers:
+            - name: mongo-container
+              image: "openshift/mongodb-24-centos7"


i guess we can't use imagestreams w/ petsets either...that's a significant limitation we need to make note of.

however this should still be a parameter so users can pick the image they want to use. (assuming we can make this template work with v2.4, v2.6, and v3.2, which should be a goal. if we can't make it work with all of them, we should target v3.2).

that's a significant limitation we need to make note of.

@bparees I don't fully understand what I should do? Create an issue or document it somehow?

talk to clayton about how we're supposed to use petsets w/o imagestreams.

bparees · 2016-09-22T08:16:26Z

2.4/root/usr/bin/run-mongod-pet

+# `cleanup` references it.
+(
+  unset_env_vars
+  mongod $mongo_common_args --replSet "${MONGODB_REPLICA_NAME}"


why can't we just exec mongo here?

Good question and idea. Before we couldn't but now we I don't see a reason of not doing it.

BTW, I found by the hard way why it's not possible: with exec we'll make our initialization script an orphan and it will become zombie.

bparees · 2016-09-22T08:18:29Z

@php-coder a few questions/comments but mostly i like how clean/straightforward this is.

omron93 · 2016-09-22T14:45:24Z

@php-coder @bparees How long we will support both ways of replset initialization?

bparees · 2016-09-22T15:38:46Z

@omron93 for as little time as possible. technically we don't "support" the existing replica mechanism, it's a proof of concept/demo.

I'd be in favor of completely replacing it with this approach instead. The only downside is that petsets are even more alpha/unsupported than the existing mechanism we're using. But given that neither is supported, it's just an example, i think we should intend to replace what we have with this, once it is ready.

omron93 · 2016-09-22T15:58:11Z

So with petset mongodb should not support scaling?

Now the first node is always PRIMARY and we don't do any elections.

I think we can't assume this. But IMPOV it should not be a problem - using replset addressing should solve this.

omron93 · 2016-09-22T17:39:42Z

@php-coder I like the idea of initializing replset without post deploy hook.

However I understand difference of petset mainly in different type of assigning identifiers/hostnames. The structure of initialization code should be the same. I am against the bringing a new main script (only a copy of run-mongod-replication) and new scripts to initialize replicaset (they should differ mainly in what mongo_initiate() do).

@bparees mentioned in #184 (comment) that we currently do not support current replication. So what about changing also current way of replset initialization not to use post hook and share the code.

The only difference of these ways should be only in way how to determine the address of first master. And replication scripts could be the same.

Way how to run replication:

current replset template -> master determined from IP (please see Initialize replset without deploy hook. #185)
petset -> master determined from hostname
localhost replication -> master determined from env. variable

Also I think a renaming is not the best way. Even with petset it is still replication.

@php-coder I see a lot of improvements which should be applied to current scripts (waiting for replication and so on). But IMPOV I don't like creating completely new scripts which are not shared.

omron93 · 2016-09-22T19:02:44Z

So for each way of running replset things could differ only in the beginning of the replset_supervisor.sh

Something like this

initiate=0
if [ -n "${MONGODB_INITIATE_REPLICA_COUNT}" ]; then
  echo -n "=> Waiting for MongoDB endpoints ..."
  current_endpoints=$(endpoints)
  while [[ "$(echo "${current_endpoints}" | wc -l)" -lt ${MONGODB_INITIAL_REPLICA_COUNT} ]]; do
    sleep 2
    current_endpoints=$(endpoints)
  done
  echo "${current_endpoints}"

  # Let initialize the first member of the cluster
  primary_node=$(echo -n "${current_endpoints}" | sort | head -n 1)
  [ "${primary_node}" == "$(container_addr)" ] && initiate=1
else if [ -n "${MONGODB_REPLICA_MEMBERS+x}" ]; then
  [ "${MONGODB_REPLICA_MEMBERS:-x}" == "" ] && primary_node=$(container_addr) && initiate=1
else
  MEMBER_ID="${HOSTNAME##*-}"
  MEMBER_HOST="$(hostname -f)"
  [ "$MEMBER_ID" = '0' ] && primary_node=${MEMBER_HOST} && initiate=1
fi  

# Initiate replicaset and exit
if [[ "${initiate}" == "1" ]]; then
...

Other code could be the same for all ways.

rhcarvalho · 2016-09-22T20:07:02Z

@omron93 for as little time as possible. technically we don't "support" the existing replica mechanism, it's a proof of concept/demo.

I'd be in favor of completely replacing it with this approach instead.

@bparees people running OSE 3.2 or less won't be able to use the template with PetSets. And there are people interested in doing MongoDB replication that are not running OSCP 3.3.

I would like to have an alternative that works without petsets, even if it has documented limitations.

bparees · 2016-09-23T07:14:31Z

@bparees people running OSE 3.2 or less won't be able to use the template with PetSets. And there are people interested in doing MongoDB replication that are not running OSCP 3.3.
I would like to have an alternative that works without petsets, even if it has documented limitations.

@rhcarvalho fair enough, we may not be able to remove the old mechanism as fast as i would like, but it's still on a relatively fast track to obsolescence imho, which is another good reason to have a separate set of new scripts that do it the right way rather than comingling the logic w/ something we want to get rid of anyway.

php-coder · 2016-09-23T08:19:46Z

@omron93

So what about changing also current way of replset initialization not to use post hook and share the code.

The idea sounds good but sharing the code means that we should test all these cases. More testing requires more strength and time. I don't see why we should spend time on it if later we'll replace the current replication. The benefit of having shared code for a fixed amount of time (let's say half of year) don't convince me.

rhcarvalho · 2016-09-23T08:37:15Z

@bparees @php-coder @omron93 I see we are all on the same page with regards to having a new separate script 👍

@omron93 wrt to the existing scripts and replication strategy, by all means please keep bringing your good ideas and improvements.

omron93 · 2016-09-23T09:32:30Z

I see we are all on the same page with regards to having a new separate script

Ok, if we should support the old way for some time, so lets rename the old one. If we rename this new main scripts I am afraid that after removing the old method it will be too late to rename this new back. And I would like to use new scripts also for replication running on localhost (#178) and it seems to me little bit misleading to use scripts with 'petset' in name.

Also I've many time heard that we should not do bunch code rewrites not to bring some regression or some problem.

OK we can have two separated scripts for new and old way of initialization. But these way should not be so different. As I think, they should differ only in few thinks (see below). And other things (as scaling and so on) could be the same!

`mongo_initiate()' should add only one member during initialization
replset_supervisor.sh in first if statement should decide according hostname (or env var in localhost replication) if this pod should initialize replset (THIS is the main improvement of petset)
in replset_supervisor.sh there should not be MONGODB_INITIAL_REPLICA_COUNT section with warning ... and in the end of if branch pod should not kill itself (see changes in Fix removing master from replset. #186)

That is all! It is not a lot of lines.

In all case main structure on replset_supervisor.sh is

if [ ... ]; then
  #Initialize replset (it differs if only one member or all should be added)
else
  #try to add itself to replset
fi

And yes. I LOT of improvements have to be applied to current scripts! And really thank you @php-coder for them. But I don't think petset brings to much difference to mongodb replication. Yes, we want to remove necessity of post deploy hook, but this is not bound to petset - and this bring bigger rewrite (but not so big:-). And after that, petset brings only change in if statement in replicaset_supervisor.sh and that is all!

@php-coder @rhcarvalho @bparees Thoughts?

bparees · 2016-09-23T09:41:30Z

Ok, if we should support the old way for some time, so lets rename the old one.

if you rename the old one you'll break people who update to the new image but still reference the old script as their entrypoint/args.

omron93 · 2016-09-23T09:51:51Z

if you rename the old one you'll break people who update to the new image but still reference the old script as their entrypoint/args.

I see. So please let choose some names not bound with petset. (for scripts, not for template)

@bparees And renaming back could be possible after removing current way or not?

php-coder · 2016-09-23T09:59:31Z

@omron93

So please let choose some names not bound with petset. (for scripts, not for template).

Could you suggest these names? Otherwise I'm going to leave them as-is :)

bparees · 2016-09-23T09:59:45Z

@bparees And renaming back could be possible after removing current way or not?

renaming is always going to be problematic if it's the name of a script that's going to be referenced by pod definitions.

omron93 · 2016-09-23T10:07:06Z

Could you suggest these names? Otherwise I'm going to leave them as-is :)

Substitute petset with nohook of -v2 or independent or something like this.

bparees · 2016-09-23T10:10:11Z

given that the script is dependent on the image being deployed via a PetSet definition, including PetSet in the name seems important to avoid users doing the wrong thing.

omron93 · 2016-09-23T10:17:02Z

given that the script is dependent on the image being deployed via a PetSet definition, including PetSet in the name seems important to avoid users doing the wrong thing.

@php-coder @bparees @rhcarvalho So please your thought about #184 (comment)? (not only about renaming:-))

I still thinks we should have two scripts - one for initialization with post-deploy hook and one for nohook initialization! No separated scripts for all method how to deploy replication. Or should #178 brings own copy of scripts???

omron93 · 2016-09-23T10:17:54Z

2.4/root/usr/bin/run-mongod-pet

+}
+
+function cleanup() {
+  echo "=> Shutting down MongoDB server ..."


Why mongo_remove was removed?

Idea was that pet shouldn't de-register itself because if it failed, then it will be re-created in a minute with the same hostname. Before we didn't have such guarantee and hence we had to register/de-register pod because it can have a different IP address each time.

And if we scale petset down? - mongodb config will still have the pod in its config...

Yes, on purpose. The idea is that the number of pets in the PetSet reflects directly the replica set configuration. If an admin wants to change the topology permanently, either scale up or down, he needs to manually reconfigure the replica set (we might facilitate that in the future, but not a common use case).

Now one more difference between PetSet and the current approach, is as @php-coder said, that when a pod is restarted it comes back with the same hostname, so the replica set config doesn't change.

The older scripts try to deal with IP changes, removing old IPs and adding new IPs to the config dynamically, and we know how many corner cases that has :)
(e.g., https://bugzilla.redhat.com/show_bug.cgi?id=1260586)

If an admin wants to change the topology permanently, either scale up or down, he needs to manually reconfigure the replica set (we might facilitate that in the future, but not a common use case).

So the cool buttons (up/down arrows in project overview) won't work?

The older scripts try to deal with IP changes, removing old IPs and adding new IPs to the config dynamically, and we know how many corner cases that has :)
(e.g., https://bugzilla.redhat.com/show_bug.cgi?id=1260586)

You wrote in that bug "However, the centos image works as expected.". What is a difference to RHEL image?
Does not #186 fix it?

I am not claiming that current scripts have no bug and there is a lot of space for improvements.

Good question. Can't answer on it right now, probably, I should put back pods de-registration..

Up until now they never did.

I though they did. At least for me - I've tested this locally when I did some changes related to openshift... Why they never did?

I don't know if #186 fixes it. Let's try it :)

Ok. I will try on Monday.

Up until now they never did.

I though they did. At least for me - I've tested this locally when I did some changes related to openshift... Why they never did?

Sorry, I got it wrong. Yes, with the replica set example it is possible to scale up/down.
We want to be able to scale up/down with persistent storage too, and that's what PetSets are for.

(Scaling up a deployment config with access to a persistent volume gives access to the same volume for all replicas...)

Yes, on purpose. The idea is that the number of pets in the PetSet reflects directly the replica set configuration. If an admin wants to change the topology permanently, either scale up or down, he needs to manually reconfigure the replica set (we might facilitate that in the future, but not a common use case).

@rhcarvalho So please how the scaling will work?

DISCLAIMER: this is work-in-progress and all that I say below might change.

how the scaling will work

initial deployment

Let's start with a 3-member replica set. oc new-app template.yaml will create the replica set.
If any of the 3 pods die, it will be automatically restarted and back to the replica set (without any changes to MongoDB's configuration).

scaling up

Now the admin decided she wants 5, not 3 members in the replica set. Given that there are appropriate PVs or a dynamic PV provisioner, she runs oc scale --replicas=5 ....
Keep in mind that before scaling up there is some "preparation", or at least making sure the requirements are met.

New pods are created and they connect to the replica set, changing its configuration.

scaling down

The admin decided to go back to 3 members... so she hits oc scale --replicas=3 ....
At this point we will NOT automatically purge the data in the PVs, instead, that's a manual operation to be performed if she wants to free up the PVs permanently.
Because from the pod's perspective there is no distinction if it terminates because of a crash or a scaling down, it's hostname stays in the replica set config (just like the data is kept).

omron93 · 2016-09-23T10:22:10Z

2.4/root/usr/share/container-scripts/mongodb/replset_petset_supervisor.sh

+    info "Successfully initialized replica set"
+else
+    # replica-1.mongodb.myproject.svc.cluster.local -> replica-0.mongodb.myproject.svc.cluster.local
+    PRIMARY_HOST="$(hostname | sed 's|[0-9]\+$|0|').$(hostname -d)"


First created pod could not be killed by kubernetes? (if something get wrong)

It can, but it keeps it's identity and will come back with the same hostname.

It could be, but it will be restarted immediately.

P.S. Yes, there is a small possibility that exactly in this time another pod will be trying to register. Maybe I should increase amount of attempts for connecting to the primary node.

P.S. Yes, there is a small possibility that exactly in this time another pod will be trying to register. Maybe I should increase amount of attempts for connecting to the primary node.

Yes, it is a small chance. But there is a bigger chance that while the pod is restarting (actually it takes some time with pre 3.2 versions) replset elects new primary.

omron93 · 2016-09-23T10:23:19Z

2.4/root/usr/share/container-scripts/mongodb/replset_petset_supervisor.sh

+WAIT_SECONDARY_STATUS="var mbrs; while(!mbrs || mbrs.length == 0 || mbrs[0].state != 2) { printjson(rs.status()); sleep(1000); mbrs = rs.status().members.filter(function(el){return el.name.indexOf(\"$MEMBER_HOST\") > -1}); }; print(mbrs[0].stateStr);"
+
+# Initialize replica set only if we're the first member
+if [ "$MEMBER_ID" = '0' ]; then


First pod could segfault of fail in some other way. Then replset votes new master, but newly added pods still refers to the first/failed one.

When all pods are up, there will always be 0, 1, 2. And never something like 1, 2, 3 (because 0 died).

Ok. Thanks for explanation.

And if the failed pod is restarted it again tries to initiate replica set?

And if the failed pod is restarted it again tries to initiate replica set?

That's a good question.
@php-coder?

Anyway, I'm keeping in mind that this PR is still WIP, pending the persistent storage. Perhaps the best thing would be if the "initialization" is idempotent, and we can always re-run it without negative side effects.

Another case: first node is restarted and tries to create new replica set even if it's already exists.

bparees

couple minor doc nits, otherwise if you and @rhcarvalho are happy, i'm happy. There is a separate PR against origin to add extended tests for this, right?

bparees · 2016-10-12T18:24:15Z

examples/petset/README.md

+[`oc cluster up`](https://github.com/openshift/origin/blob/master/docs/cluster_up_down.md)
+command.
+
+Assuming you have the `oc` tool, are logged in and have 3 pre-created


s/Assuming/This tutorial assumes/

bparees · 2016-10-12T18:25:02Z

examples/petset/README.md

+persistent volumes (or configured [persistent volume
+provisioning](https://github.com/kubernetes/kubernetes/blob/master/examples/experimental/persistent-volume-provisioning/README.md)).
+
+In the context of a project where you want to create a MongoDB cluster, run\


delete trailing \

bparees · 2016-10-12T18:25:31Z

examples/petset/README.md

+provisioning](https://github.com/kubernetes/kubernetes/blob/master/examples/experimental/persistent-volume-provisioning/README.md)).
+
+In the context of a project where you want to create a MongoDB cluster, run\
+`oc new-app` passing the template file as an argument (`oc new-app` understands URLs too):


delete the parenthetical. they'll figure out that it supports urls :)

bparees · 2016-10-12T18:41:45Z

examples/petset/README.md

+sh-4.2$
+```
+
+And later from one of the pods you can also login into the MongoDB:


s/the MongoDB/MongoDB/

php-coder · 2016-10-12T19:43:10Z

@bparees Fixed all the neats.

There is a separate PR against origin to add extended tests for this, right?

Right. See openshift/origin#11266

bparees · 2016-10-12T19:48:29Z

@php-coder ok. so when you and @rhcarvalho agree, this can be merged.

rhcarvalho

Still some nits to fix, mainly I'd take some unrelated changes to a separate PR.

rhcarvalho · 2016-10-13T13:59:07Z

3.2/README.md

@@ -22,8 +22,6 @@ The following environment variables influence the MongoDB configuration file. Th

 |    Variable name      |    Description                                                            |    Default
 | :-------------------- | ------------------------------------------------------------------------- | ----------------
-|  `MONGODB_NOPREALLOC` | Disable data file preallocation (only for mounted data directory from older MongoDB server).  |  true
-|  `MONGODB_SMALLFILES` | Set MongoDB to use a smaller default data file size (only for mounted data directory from older MongoDB server).  |  true


This change is unrelated to "Replica set example that uses PetSet", could we make it an independent PR?

Extracted to #200

rhcarvalho · 2016-10-13T13:59:24Z

3.2/root/usr/bin/run-mongod

@@ -13,8 +13,6 @@ function usage() {
  echo "  MONGODB_DATABASE"
  echo "  MONGODB_ADMIN_PASSWORD"
  echo "MongoDB settings:"
-  echo "  MONGODB_NOPREALLOC (default: true)"
-  echo "  MONGODB_SMALLFILES (default: true)"


rhcarvalho · 2016-10-13T13:59:45Z

3.2/root/usr/share/container-scripts/mongodb/common.sh

@@ -10,8 +10,6 @@ set -o pipefail
 export MONGODB_DATADIR=/var/lib/mongodb/data
 export CONTAINER_PORT=27017
 # Configuration settings.
-export MONGODB_NOPREALLOC=${MONGODB_NOPREALLOC:-true}
-export MONGODB_SMALLFILES=${MONGODB_SMALLFILES:-true}


rhcarvalho · 2016-10-13T14:00:04Z

3.2/root/usr/bin/run-mongod-replication

@@ -22,8 +22,6 @@ function usage() {
  echo "Optional variables:"
  echo "  MONGODB_INITIAL_REPLICA_COUNT"
  echo "MongoDB settings:"
-  echo "  MONGODB_NOPREALLOC (default: true)"
-  echo "  MONGODB_SMALLFILES (default: true)"


rhcarvalho · 2016-10-13T14:01:08Z

3.2/root/usr/share/container-scripts/mongodb/init-petset-replset.sh

+    quit(1);
+  "
+
+  # TODO: replace this with a call to `replset_addr` from common.sh, once it returns host names.


@php-coder how about we use $(repl_addr) here? I have tried it before and it worked.

rhcarvalho · 2016-10-13T14:20:33Z

examples/petset/README.md

+
+### Known Limitations
+
+* Only MongoDB 3.2 is supported.


If this is the case, we'll need to update other places where we say you can use the other image versions.

How about the RHEL image for 3.2?

we'll need to update other places where we say you can use the other image versions.

Which places? There is only README and it was updated.

How about the RHEL image for 3.2?

I've mentioned it ("(and its RHEL variant").

rhcarvalho · 2016-10-13T14:21:29Z

examples/petset/README.md

+
+* Only MongoDB 3.2 is supported.
+* You have to manually update replica set configuration in case of scaling down.
+* Changing a user's and admin's password is manual process (it requires


s/is manual/is a manual/

Perhaps replace the parentheses with a :?

... is a manual process: it requires ...

Fixed, thanks.

rhcarvalho · 2016-10-13T14:23:13Z

examples/petset/mongodb-petset-persistent.yaml

+        - metadata:
+            name: mongo-data
+            annotations:
+              # Uncommented this if using dynamic volume provisioning.


s/Uncommented/Uncomment

rhcarvalho · 2016-10-13T14:23:38Z

examples/petset/mongodb-petset-persistent.yaml

+          containers:
+            - name: mongo-container
+              image: "centos/mongodb-32-centos7"
+              # list of ports to expose from the container


Do we really need this type of comment?

I removed it now.

rhcarvalho · 2016-10-13T14:25:36Z

examples/petset/mongodb-petset-persistent.yaml

+        spec:
+          containers:
+            - name: mongo-container
+              image: "centos/mongodb-32-centos7"


I thought this was a parameter?

Yes, but because we're supporting 3.2 only at this time, I removed parameter.

Ah, got it. Wouldn't it be easier to have the parameter so that later we don't need to change it? Just change the description listing the other images as we had it.

I find the parameter useful to customize the template in instantiation time.

Ok, resurrected.

php-coder · 2016-10-13T15:49:56Z

@php-coder how about we use $(repl_addr) here? I have tried it before and it worked.

@rhcarvalho I don't see a benefit of it but it can break something in the one step before merging.

rhcarvalho · 2016-10-13T16:06:14Z

@php-coder how about we use $(repl_addr) here? I have tried it before and it worked.

@rhcarvalho I don't see a benefit of it but it can break something in the one step before merging.

okay, we can do that in a separate PR.

rhcarvalho · 2016-10-14T10:57:05Z

3.2/root/usr/share/container-scripts/mongodb/init-petset-replset.sh

+
+  # TODO: replace this with a call to `replset_addr` from common.sh, once it returns host names.
+  local replset_addr
+  replset_addr="${MONGODB_REPLICA_NAME}/$(find_endpoints | paste -s -d,)"


This is just a note.

In the old replset_supervisor.sh#L13-L17, we were waiting until an expected number of lines come out of the call to dig.
I'm afraid here we are going unprotected for the case in which dig returns nothing, and we are unable to reach the replica set, but that should make the next line fail and we exit with an error message in the logs.

As we've noted earlier in a conversation, the error doesn't propagate to run-mongod-pet, and doesn't cause a (perhaps desirable) container restart.

I added a couple of TODOs so that we may revisit this later in follow ups.

rhcarvalho · 2016-10-14T12:39:22Z

3.2/root/usr/bin/run-mongod-replication

@@ -50,7 +50,7 @@ fi
 cache_container_addr
 mongo_common_args="-f $MONGODB_CONFIG_PATH"
 if ! [[ -v MONGODB_USER && -v MONGODB_PASSWORD && -v MONGODB_DATABASE && -v MONGODB_ADMIN_PASSWORD && -v MONGODB_KEYFILE_VALUE && -v MONGODB_REPLICA_NAME ]]; then
-    usage
+  usage


This can be applied to all image versions, and don't need to be here, moving it to #201.

php-coder · 2016-10-14T17:19:21Z

@rhcarvalho Did you finish reviewing this?

rhcarvalho · 2016-10-17T07:59:47Z

Reviewed and squashed. LGTM.

@bparees PTAL.

php-coder · 2016-10-17T13:10:50Z

Found a bug, going to fix it shortly.

rhcarvalho · 2016-10-17T13:36:26Z

3.2/root/usr/share/container-scripts/mongodb/init-petset-replset.sh

+  if [ -z "${endpoints}" ]; then
+    info "ERROR: couldn't join replica set!"
+    info "CAUSE: couldn't find hosts that are belongs to the replica set."
+    return 1


Returning zero or non-zero here makes no difference, right? I believe users have to go read the logs to realize that something is wrong, correct?

Yes. it's correct.

@rhcarvalho When we decided to move retry logic into JavaScript we were wrong actually: there is non-zero possibility that potential member can't connect to replica set at all because primary isn't ready yet.

@rhcarvalho Never mind. My test has failed not because of this, but probably because of exceeding timeout.

Timeout also wasn't a cause. In one of improvements I replaced grep -x by = and it was wrong. Indeed, best is the enemy of the good :-(

rhcarvalho · 2016-10-17T13:40:25Z

3.2/root/usr/share/container-scripts/mongodb/init-petset-replset.sh

+
+  if [ -z "${endpoints}" ]; then
+    info "ERROR: couldn't join replica set!"
+    info "CAUSE: couldn't find hosts that are belongs to the replica set."


Suggestion:

"CAUSE: DNS lookup for '${MONGODB_SERVICE_NAME:-mongodb}' returned no results."

Otherwise, "that are belongs" needs fixing.

rhcarvalho · 2016-10-17T15:11:42Z

3.2/root/usr/share/container-scripts/mongodb/init-petset-replset.sh

@@ -134,6 +134,7 @@ function add_member() {
    info "ERROR: couldn't join to replica set!"
    info "CAUSE: failed after waiting for becoming a member. Command output was:"
    echo "${rs_status_out}"
+    echo "==> End of the error output <=="


This is yet another format for log messages, is this something you're intending to have in the image or just debugging to be removed before merge?

I want to have it in the image because it will be very useful if it fails. Otherwise it's mostly impossible to understand where command's output is ending.

rhcarvalho · 2016-10-17T15:13:43Z

3.2/root/usr/share/container-scripts/mongodb/init-petset-replset.sh

@@ -124,7 +124,7 @@ function add_member() {
    return 1
  fi

-  info "Successfully joined replica set"
+  info "Successfully connected to replica set"


I think this was a non-improvement. To "connect to" a replica set is something like mongo --host rs0/host1,host2,..., and that is not what this message is about. IMHO "joined" seemed more appropriate.

For the history: initial fix was about removing two identical messages about joining to replica set. After quick conversion offline we found a way of making these messages better than before.

bparees · 2016-10-17T16:21:39Z

@rhcarvalho @php-coder i can't tell if this is still considered done or not, looks like you guys are still updating it.

…PetSet.

php-coder · 2016-10-18T12:26:49Z

@bparees It's ready to be merged.

rhcarvalho

Re-reviewed, LGTM.

php-coder · 2016-10-18T15:32:17Z

Because current PR is too big and doesn't loaded sometimes, I'll create another one.

bparees · 2016-10-18T15:40:33Z

we've had bigger PRs (both filechange-wise and comment-wise), i don't think
it's a size issue.

On Tue, Oct 18, 2016 at 11:32 AM, Vyacheslav Semushin <
notifications@github.com> wrote:

Because current PR is too big and doesn't loaded sometimes, I'll create
another one.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#184 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AEvl3ky0R7HkaSEA6Qq5O7GLg2yvmBRfks5q1OaBgaJpZM4KDBBQ
.

Ben Parees | OpenShift

bparees reviewed Sep 22, 2016

View reviewed changes

omron93 mentioned this pull request Sep 22, 2016

Initialize replset without deploy hook. #185

Closed

omron93 reviewed Sep 23, 2016

View reviewed changes

bparees requested changes Oct 12, 2016

View reviewed changes

php-coder changed the title ~~[WIP] Replica set example that uses PetSet~~ Replica set example that uses PetSet Oct 12, 2016

rhcarvalho reviewed Oct 13, 2016

View reviewed changes

php-coder mentioned this pull request Oct 13, 2016

3.2: remove smallfiles and noprealloc options #200

Merged

rhcarvalho reviewed Oct 14, 2016

View reviewed changes

rhcarvalho mentioned this pull request Oct 14, 2016

Apply some improvements to old replica set scripts #201

Merged

rhcarvalho reviewed Oct 14, 2016

View reviewed changes

rhcarvalho force-pushed the petset_support branch from 81a63c3 to c4ff98e Compare October 14, 2016 14:18

rhcarvalho force-pushed the petset_support branch from c4ff98e to 99c8129 Compare October 17, 2016 07:58

rhcarvalho reviewed Oct 17, 2016

View reviewed changes

php-coder and others added 2 commits October 18, 2016 14:24

mongodb-clustered-petset.yaml: add replica set example that is using …

cf99d38

…PetSet.

Document replication with PetSet

9e35dfe

php-coder force-pushed the petset_support branch from 1b13dcc to 9e35dfe Compare October 18, 2016 12:24

rhcarvalho approved these changes Oct 18, 2016

View reviewed changes

php-coder closed this Oct 18, 2016

php-coder mentioned this pull request Oct 18, 2016

Replica set example that uses PetSet (v2) #206

Merged

php-coder deleted the petset_support branch October 18, 2016 16:41

Replica set example that uses PetSet #184

Replica set example that uses PetSet #184

Conversation

php-coder commented Sep 21, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bparees commented Sep 22, 2016

omron93 commented Sep 22, 2016

bparees commented Sep 22, 2016

omron93 commented Sep 22, 2016

omron93 commented Sep 22, 2016

omron93 commented Sep 22, 2016 • edited

rhcarvalho commented Sep 22, 2016

bparees commented Sep 23, 2016

php-coder commented Sep 23, 2016 • edited

rhcarvalho commented Sep 23, 2016

omron93 commented Sep 23, 2016 • edited

bparees commented Sep 23, 2016

omron93 commented Sep 23, 2016

php-coder commented Sep 23, 2016

bparees commented Sep 23, 2016

omron93 commented Sep 23, 2016

bparees commented Sep 23, 2016

omron93 commented Sep 23, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

how the scaling will work

initial deployment

scaling up

scaling down

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

omron93 Sep 23, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bparees left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

php-coder commented Oct 12, 2016

bparees commented Oct 12, 2016

rhcarvalho left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

php-coder commented Sep 21, 2016 •

edited

omron93 commented Sep 22, 2016 •

edited

php-coder commented Sep 23, 2016 •

edited

omron93 commented Sep 23, 2016 •

edited

omron93 Sep 23, 2016 •

edited