<div dir="ltr"><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Jul 29, 2020 at 1:42 PM Bo Peng <<a href="mailto:pengbo@sraoss.co.jp">pengbo@sraoss.co.jp</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Ishii-san,<br>
<br>
Thank you for your comments.<br>
I have fixed warnings and removed Makefile.in.<br>
Modified patch is attached.<br>
<br>
Usama,<br>
<br>
Do you have any comments?<br></blockquote><div><br></div><div>First of all thanks for the good work. I think this patch will drastically improve the watchdog setup experience. </div><div>Overall the patch looks good while I have a couple of comments that can also be taken care</div><div>of as a separate patch after committing this one.</div><div><br></div><div>-- You have rightly removed wd_port key from config JSON (in get_pool_config_from_json() and get_pool_config_json() functions)<br>So I think this will cause incompatibility with the older versions. So we may need to bump the<br>WD_MESSAGE_DATA_VERSION_MINOR version.</div><div><br>-- currently, watchdog uses the <font face="monospace">WatchdogNode->private_id</font> to identify each watchdog<br></div></div><div class="gmail_quote">node locally while <font face="monospace">private_id</font> of local node is always set to 0.</div><div class="gmail_quote">And now with this patch, the node number for each watchdog node can be<br>consistent across the cluster so we can make this private_id go away and use pgpool_node_id</div><div class="gmail_quote">as a cluster-wide unique id for each node.<br>For that, I think we can also include pgpool_node_id in the WD_ADD_NODE_MESSAGE</div><div class="gmail_quote">message and upon receiving WD_ADD_NODE_MESSAGE watchdog must also</div><div class="gmail_quote">verify that multiple pgpool-II nodes should not be using the same pgpool_node_id.</div><div class="gmail_quote"><br></div><div class="gmail_quote">Thanks</div><div class="gmail_quote">Best regards</div><div class="gmail_quote">Muhammad Usama</div><div class="gmail_quote"><br></div><div class="gmail_quote"><br></div><div class="gmail_quote"><br></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><br>
On Tue, 28 Jul 2020 14:13:14 +0900 (JST)<br>
Tatsuo Ishii <<a href="mailto:ishii@sraoss.co.jp" target="_blank">ishii@sraoss.co.jp</a>> wrote:<br>
<br>
> Hi Peng,<br>
> <br>
> Thank you for the patch (that must be a hard work). I have applied the<br>
> patch to the master branch head and here are some comments:<br>
> <br>
> 1. The patch should not include Makefile.in. In the development stage<br>
> each developer uses their own environment, thus generated files by<br>
> autoconf may vary. This prevented me from applying the patch and I had<br>
> to remove Makefile.in patch part.<br>
> <br>
> 2. I saw some warnings from gcc.<br>
> <br>
> -------------------------------------------------------------------------------------------------------------<br>
> config/pool_config_variables.c: In function ‘SetHBDestIfFunc’:<br>
> config/pool_config_variables.c:4945:5: warning: this ‘if’ clause does not guard... [-Wmisleading-indentation]<br>
> if (g_pool_config.wd_lifecheck_method != LIFECHECK_BY_HB)<br>
> ^~<br>
> config/pool_config_variables.c:4955:2: note: ...this statement, but the latter is misleadingly indented as if it were guarded by the ‘if’<br>
> for (i = 0; i < WD_MAX_IF_NUM; i++)<br>
> ^~~<br>
> pool_config_variables.c: In function ‘SetHBDestIfFunc’:<br>
> pool_config_variables.c:4945:5: warning: this ‘if’ clause does not guard... [-Wmisleading-indentation]<br>
> if (g_pool_config.wd_lifecheck_method != LIFECHECK_BY_HB)<br>
> ^~<br>
> pool_config_variables.c:4955:2: note: ...this statement, but the latter is misleadingly indented as if it were guarded by the ‘if’<br>
> for (i = 0; i < WD_MAX_IF_NUM; i++)<br>
> ^~~<br>
> pool_config_variables.c: In function ‘SetHBDestIfFunc’:<br>
> pool_config_variables.c:4945:5: warning: this ‘if’ clause does not guard... [-Wmisleading-indentation]<br>
> if (g_pool_config.wd_lifecheck_method != LIFECHECK_BY_HB)<br>
> ^~<br>
> pool_config_variables.c:4955:2: note: ...this statement, but the latter is misleadingly indented as if it were guarded by the ‘if’<br>
> for (i = 0; i < WD_MAX_IF_NUM; i++)<br>
> ^~~<br>
> pool_config_variables.c: In function ‘SetHBDestIfFunc’:<br>
> pool_config_variables.c:4945:5: warning: this ‘if’ clause does not guard... [-Wmisleading-indentation]<br>
> if (g_pool_config.wd_lifecheck_method != LIFECHECK_BY_HB)<br>
> ^~<br>
> pool_config_variables.c:4955:2: note: ...this statement, but the latter is misleadingly indented as if it were guarded by the ‘if’<br>
> for (i = 0; i < WD_MAX_IF_NUM; i++)<br>
> ^~~<br>
> -------------------------------------------------------------------------------------------------------------<br>
> <br>
> 3. I confirmed that standard regression tests were all ok on my<br>
> Ubuntu18 box.<br>
> <br>
> Other than the warnings above and Makefile.in issue, this patch looks<br>
> in good shape for me.<br>
> <br>
> Best regards,<br>
> --<br>
> Tatsuo Ishii<br>
> SRA OSS, Inc. Japan<br>
> English: <a href="http://www.sraoss.co.jp/index_en.php" rel="noreferrer" target="_blank">http://www.sraoss.co.jp/index_en.php</a><br>
> Japanese:<a href="http://www.sraoss.co.jp" rel="noreferrer" target="_blank">http://www.sraoss.co.jp</a><br>
> <br>
> > hello,<br>
> > <br>
> > I have completed this patch.<br>
> > (The configuration example docs need to be updated.)<br>
> > <br>
> > This patch can simplify WATCHDOG configurations<br>
> > by using a common configuration file for each pgpool node,<br>
> > and users just copy the configuration file to each other node without editing.<br>
> > <br>
> > =======<br>
> > Changes<br>
> > =======<br>
> > <br>
> > The current watchdog and heartbeat configuration parameters (for 3 pgpool nodes)<br>
> > <br>
> > ---- <br>
> > wd_hostname<br>
> > wd_port<br>
> > wd_heartbeat_port<br>
> > heartbeat_destination0<br>
> > heartbeat_destination_port0<br>
> > heartbeat_destination1<br>
> > heartbeat_destination_port1<br>
> > other_pgpool_hostname0<br>
> > other_pgpool_port0<br>
> > other_pgpool_hostname1<br>
> > other_pgpool_port1<br>
> > ---- <br>
> > <br>
> > are changed to <br>
> > <br>
> > ---- <br>
> > hostname0 = 'server1'<br>
> > wd_port0 = 9000<br>
> > pgpool_port0 = 9999<br>
> > <br>
> > hostname1 = 'server2'<br>
> > wd_port1 = 9000<br>
> > pgpool_port1 = 9999<br>
> > <br>
> > hostname2 = 'server3'<br>
> > wd_port2 = 9000<br>
> > pgpool_port2 = 9999<br>
> > <br>
> > heartbeat_hostname0 = 'server1'<br>
> > heartbeat_port0 = 9694<br>
> > heartbeat_device0 = ''<br>
> > <br>
> > heartbeat_hostname1 = 'server2'<br>
> > heartbeat_port1 = 9694<br>
> > heartbeat_device1 = ''<br>
> > <br>
> > heartbeat_hostname2 = 'server3'<br>
> > heartbeat_port2 = 9694<br>
> > heartbeat_device2 = ''<br>
> > ---- <br>
> > <br>
> > You can specify multiple configurations in <br>
> > "heartbeat_hostname" and "heartbeat_device" by separating <br>
> > them using semicolon (;). <br>
> > <br>
> > And user needs to specify local pgpool node id in a pgpool_node_id file <br>
> > which is created in the direcroty of pgpool.conf.<br>
> > <br>
> > For example:<br>
> > <br>
> > (pgpool node 0)<br>
> > $ cat <directory path of pgpool.conf>/pgpool_node_id<br>
> > 0<br>
> > <br>
> > (pgpool node 1)<br>
> > $ cat <directory path of pgpool.conf>/pgpool_node_id<br>
> > 1<br>
> > <br>
> > (pgpool node 2)<br>
> > $ cat <directory path of pgpool.conf>/pgpool_node_id<br>
> > 2<br>
> > <br>
> > <br>
> > Any feedback about this patch?<br>
> > <br>
> > <br>
> > On Thu, 25 Jun 2020 11:17:13 +0900 (JST)<br>
> > Tatsuo Ishii <<a href="mailto:ishii@sraoss.co.jp" target="_blank">ishii@sraoss.co.jp</a>> wrote:<br>
> > <br>
> >> > Ishii-san, Usama,<br>
> >> > <br>
> >> > Thank you for your advice.<br>
> >> > I have some questions about heartbeat related parameters.<br>
> >> > <br>
> >> > I would like to change the watchdog related parameters as follow:<br>
> >> > <br>
> >> > ----------------------<br>
> >> > wd_hostname0 = <br>
> >> > pgpool_port0 = <br>
> >> > wd_port0 = <br>
> >> > <br>
> >> > wd_hostname1 = <br>
> >> > pgpool_port1 = <br>
> >> > wd_port1 =<br>
> >> > .<br>
> >> > .<br>
> >> > .<br>
> >> > <br>
> >> > heartbeat_hostname0 =<br>
> >> > heartbeat_port0 =<br>
> >> > heartbeat_device0 =<br>
> >> > <br>
> >> > heartbeat_hostname1 =<br>
> >> > heartbeat_port1 =<br>
> >> > heartbeat_device1 =<br>
> >> > ----------------------<br>
> >> > <br>
> >> > However, during the implementation, I noticed more than one network interface<br>
> >> > can be specified to send/receive heartbeat signal in Pgpool-II.<br>
> >> > <br>
> >> > Because only one "wd_heartbeat_port" can be specified,<br>
> >> > "heartbeat_destination_portX" should be specified with the same port number?<br>
> >> > <br>
> >> > <br>
> >> > For example: 3 pgpool node, heartbeat with 2 NIC<br>
> >> > <br>
> >> > Should we configure heartbeat paramaters as below?<br>
> >> > <br>
> >> > ---------------<br>
> >> > wd_heartbeat_port = 9694<br>
> >> > <br>
> >> > heartbeat_destination0 = "192.168.37.102"<br>
> >> > heartbeat_destination1 = '192.168.54.102'<br>
> >> > heartbeat_destination_port0 = 9694 <br>
> >> > heartbeat_destination_port1 = 9694 <br>
> >> > heartbeat_device0 = 'eth0'<br>
> >> > heartbeat_device1 = 'eth1'<br>
> >> > <br>
> >> > heartbeat_destination2 = '192.168.37.103'<br>
> >> > heartbeat_destination3 = '192.168.54.103'<br>
> >> > heartbeat_destination_port2 = 9694 <br>
> >> > heartbeat_destination_port3 = 9694 <br>
> >> > heartbeat_device2 = 'eth0'<br>
> >> > heartbeat_device3 = 'eth1'<br>
> >> > ---------------<br>
> >> > <br>
> >> > <br>
> >> > If above is correct, I would like to design the heartbeat paramaters as below:<br>
> >> > If it is the own node settings, "heartbeat_hostname" and "heartbeat_device" will be ignored.<br>
> >> > <br>
> >> > ---------------<br>
> >> > heartbeat_hostname0 = '192.168.37.101:192.168.54.101'<br>
> >> > heartbeat_port0 = 9694<br>
> >> > heartbeat_device0 = 'eth0:eth1'<br>
> >> > <br>
> >> > heartbeat_hostname1 = '192.168.37.102:192.168.54.102'<br>
> >> > heartbeat_port1 = 9694<br>
> >> > heartbeat_device1 = 'eth0:eth1'<br>
> >> > <br>
> >> > heartbeat_hostname2 = '192.168.37.103:192.168.54.103'<br>
> >> > heartbeat_port2 = 9694<br>
> >> > heartbeat_device2 = 'eth0:eth1'<br>
> >> > ---------------<br>
> >> > <br>
> >> > What do you think?<br>
> >> <br>
> >> Above looks good to me.<br>
> >> <br>
> >> BTW, if the IP addresses are on the same subnet, it is not enough to<br>
> >> specify heartbeat_device. You also need to tweak Linux kernel<br>
> >> parameters<br>
> >> (/proc/sys/net/ipv4/conf/***/{arp_announce,arp_ignore). Maybe we<br>
> >> should note this in the manual.<br>
> >> <br>
> >> I have learned this from following blog (in Japanese).<br>
> >> <br>
> >> <a href="http://www.nminoru.jp/~nminoru/diary/2014/02.html#20140203p1" rel="noreferrer" target="_blank">http://www.nminoru.jp/~nminoru/diary/2014/02.html#20140203p1</a><br>
> >> <br>
> >> Best regards,<br>
> >> --<br>
> >> Tatsuo Ishii<br>
> >> SRA OSS, Inc. Japan<br>
> >> English: <a href="http://www.sraoss.co.jp/index_en.php" rel="noreferrer" target="_blank">http://www.sraoss.co.jp/index_en.php</a><br>
> >> Japanese:<a href="http://www.sraoss.co.jp" rel="noreferrer" target="_blank">http://www.sraoss.co.jp</a><br>
> > <br>
> > <br>
> > -- <br>
> > Bo Peng <<a href="mailto:pengbo@sraoss.co.jp" target="_blank">pengbo@sraoss.co.jp</a>><br>
> > SRA OSS, Inc. Japan<br>
<br>
<br>
-- <br>
Bo Peng <<a href="mailto:pengbo@sraoss.co.jp" target="_blank">pengbo@sraoss.co.jp</a>><br>
SRA OSS, Inc. Japan<br>
</blockquote></div></div>