Add a test for master restart after being killed #133
Conversation
|
Can one of the admins verify this patch? |
|
[test] |
1 similar comment
|
[test] |
|
Thanks. I wouldn't use a word "failover" as that usually means different usecase in PostgreSQL context. I'll do a better review tomorrow. |
| for cidfile in $CIDFILE_DIR/* ; do | ||
| CONTAINER=$(cat $cidfile) | ||
|
|
||
| cleanup_container |
There was a problem hiding this comment.
I would move this into the second patch, otherwise this "function split" looks like
unnecessary (when somebody looks at the patch separately).
|
Ah, reading the patch carefully, there really is failover scenario tested (that is, however, probably not usable in OpenShift at least ATM). |
|
IOW: promoting slave container to master container is hard task enough to rather start new |
pkubatrh
force-pushed
the
master
branch
6 times, most recently
from
ac593e5
to
e300684
Compare
pkubatrh
changed the title
|
Updated the PR with a new test case (master restart instead of failover). |
| local value | ||
| value=24 | ||
| # Do some real work to test replication in practice | ||
| postgresql_cmd -c "CREATE TABLE t1 (a integer); INSERT INTO t1 VALUES ($value);" | ||
| local table_name="t1" |
There was a problem hiding this comment.
This overwrites previous variable assignment.
|
New push, fixed issues pointed out by @praiskup (overwritten variable + added a comment to global variable) |
| for f in `find ${CIDFILE_DIR} -type f -name "slave-*"`; do | ||
| cidfile=$(basename $f) | ||
| slave_ip=$(get_container_ip $cidfile) | ||
| CONTAINER_IP=$slave_ip | ||
| for i in $(seq $max_attempts); do | ||
| result="$(postgresql_cmd -At -c "select * from t1")" | ||
| result="$(postgresql_cmd -At -c "select * from $table_name")" | ||
| if [[ ! -z "${result}" ]]; then |
There was a problem hiding this comment.
This is probably OK, but I would rather fix it sooner than later ... checking psql output
for (non)empty-ness is not perfect, because error messages comes to standard output ->
thus empty string comes from invalid query. We are fine because 'set -e' is activated ..
but that could change in future... I would vote for explicit string to compare $result with.
pkubatrh
force-pushed
the
master
branch
2 times, most recently
from
ba55e85
to
b4ab64c
Compare
|
Rewrote non-emptyness check into an expected value check, max_attempts variable no longer global. |
| docker stop $(cat $cidfile) | ||
| local rm_cmd='/bin/rm -rf /var/lib/pgsql/data/*' | ||
| docker run $master_args --rm $IMAGE_NAME /bin/sh -c "$rm_cmd" | ||
| rmdir ${volume_dir} |
There was a problem hiding this comment.
Nit, can we do this at one place (looks like code duplication).
| @@ -265,7 +265,8 @@ function run_slave() { | |||
|
|
|||
| function run_master() { | |||
| local suffix="$1"; shift | |||
| docker run $cluster_args -p 5432 \ | |||
| master_args=${master_args-} | |||
| docker run $cluster_args $master_args -p 5432 \ | |||
There was a problem hiding this comment.
I'm still not sure whether we need to '-p 5432'.
|
Incorporated @praiskup's suggestions - new functions for the creation and removal of volumes, got rid of some redundant local variables, changed loops to use container IDs instead of calling 'find' on .cid files. |
|
Weird. On my Fedora 24 box, running 'make test VERSION=9.4' fails very early with:
Actually, the docker build created unusable image, the source 'centos:centos7' works fine. |
|
[test] |
|
My mistake, more info in #136, I'll continue with the review. |
| @@ -309,7 +328,7 @@ function test_value_replication() { | |||
| CONTAINER_IP=$slave_ip | |||
| for i in $(seq $max_attempts); do | |||
| result="$(postgresql_cmd -At -c "select * from $table_name")" | |||
There was a problem hiding this comment.
I'm not sure the loop here has some effect. When the value is not there yet, it is
very likely the $table_name table does not exist yet too. Then, postgresql_cmd fails
and this would fail the whole command, so no other attempt would me made.
Probably || : is missing.
There was a problem hiding this comment.
s/fail the whole command/exit the whole script/
|
Pushed new changes - ignore psql errors when checking for value replication, master_args variable now local |
|
LGTM. |
|
@pkubatrh looks like this is pretty atomic change, would it be fine to squash the commits? |
|
squash merged. |
This PR adds a simple test to check if a master can be properly restarted after being killed in a replication scenario.
First commit makes some necessary changes to existing test code while the second commit adds the actual test.