HI Anders/Hans
On 20/01/18 00:56, Anders Widell wrote:
Ack from me also, with comments:
* I think my major comment is that I had originally envisioned that
you would use the "etcdctl lock" command (in the V3 API) and that the
active SC would hold the lock for as long as it is active. The lock
would not be needed for reading. Your approach of only creating the
lock when you wish to change active controller could be fine though.
However, you shouldn't need the lock for reading - only when you wish
to update the active controller. Regarding the Watch: I think you
should have the watch on the lock instead of (or in addition to) the
data you are protecting. At a fail-over, the old standby would acquire
the lock and wait a while to give the old active enough time to detect
that a fail-over is pending (it notices that the lock has been
created). The old active would then be able to remove the lock and
prevent the fail-over from happening. We can look into this in the
next iteration (next release) and keep it as it is for now.
[Gary] I will remove the opensaf_active_controller key and just have a
key for the lock. The node is stored in the corresponding value. It's a
lot simpler that way so I will do it for this release. The lock will not
have a TTL and be persistent (until removed by another controller).
* You ought to utilise the test-and-set functionality in the etcd v2
protocol, in the cases where you are changing the value of a key and
know (think you know) the previous value. unlock is an example of
this, fail-over probably also. We could add this later but I think you
should at least extend the plugin API already now, so that it takes a
"previous value" parameter where applicable.
* You have a try-again loop when you acquire the lock, but if the
maximum number of retries have been done then you continue as if the
lock was acquired successfully. It doesn't seem to be correct?
[Gary] Yes, will fix.
* It is not obvious (to me) that no more Watch thread can be created
simultaneously. Could you add a flag that keeps track of if there is
an existing thread, and add assert statements checking that there is
no existing thread when you call MonitorActive() to create a new one?
[Gary] OK, will add a conditional statement.
* As Hans points out below, it seems that it is also possible that the
watch thread could disappear silently in some error case.
[Gary] Will make it assert in that case.
* As already pointed out by Hans, we should store our keys in some
directory in the etcd database, so that the same database can be used
for other purposes as well. I think the plugin (shell script) should
add a directory prefix to the key.
[Gary] Yes, good idea. The directory prefix will be handled by the
plugin, in case the underlying key-value store doesn't handle
directories etc.
AndersW> Split-brain should not be possible, however the current
algorithm will not guarantee that the active SC will be in the largest
partition in case TIPC connectivity is broken (partitioned). So it
could happen that a single isolated node (from TIPC point of view) is
the active SC, even though a larger TIPC partition exists. I think
this could be solved by writing the size of the cluster into the lock.
An existing active SC shall reject a fail-over if it is being
initiated from a node in a smaller partition.
[Gary] Can we postpone this for the next release?
Thanks
Gary
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Opensaf-devel mailing list
Opensaf-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/opensaf-devel
