<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Aug 6, 2020 at 11:24 AM 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 Usama,<br>
<br>
On Wed, 29 Jul 2020 15:14:16 +0500<br>
Muhammad Usama <<a href="mailto:m.usama@gmail.com" target="_blank">m.usama@gmail.com</a>> wrote:<br>
<br>
> On Wed, Jul 29, 2020 at 1:42 PM Bo Peng <<a href="mailto:pengbo@sraoss.co.jp" target="_blank">pengbo@sraoss.co.jp</a>> wrote:<br>
> <br>
> > 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>
> ><br>
> <br>
> First of all thanks for the good work. I think this patch will<br>
> drastically improve the watchdog setup experience.<br>
> Overall the patch looks good while I have a couple of comments that can<br>
> also be taken care<br>
> of as a separate patch after committing this one.<br>
> <br>
> -- You have rightly removed wd_port key from config JSON (in<br>
> 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<br>
> may need to bump the<br>
> WD_MESSAGE_DATA_VERSION_MINOR version.<br>
<br>
To prevent incompatibility I think it's better to leave <br>
"wd_port" in POOL_CONFIG struct (pool_config).<br>
<br>
And also I will leave "wd_remote_nodes" in POOL_CONFIG struct,<br>
because it is easy to compare the number of remote pgpool nodes.<br></blockquote><div><br></div><div>I think we can live with minor incompatibility if it is absolutely necessary</div><div>since this feature will be part of the next major release of Pgpool-II.</div><div>So if leaving the wd_port and wd_remote_nodes in POOL_CONFIG struct</div><div>compromises the design in any way then, in my opinion, you should not worry</div><div>about the incompatibility and stick with the current design and just</div><div>increment the WD_MESSAGE_DATA_VERSION_MINOR</div><div><br></div><div>Thanks</div><div>Best regards</div><div>Muhammad Usama</div><div><br></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> -- currently, watchdog uses the WatchdogNode->private_id to identify each<br>
> watchdog<br>
> node locally while private_id of local node is always set to 0.<br>
> 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<br>
> use pgpool_node_id<br>
> as a cluster-wide unique id for each node.<br>
> For that, I think we can also include pgpool_node_id in the<br>
> WD_ADD_NODE_MESSAGE<br>
> message and upon receiving WD_ADD_NODE_MESSAGE watchdog must also<br>
> verify that multiple pgpool-II nodes should not be using the same<br>
> pgpool_node_id.<br>
<br>
Yes. I agree to use pgpool_node_id.<br>
I will add the implementation. </blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> Thanks<br>
> Best regards<br>
> Muhammad Usama<br>
> <br>
> <br>
> <br>
> <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>
> > -------------------------------------------------------------------------------------------------------------<br>
> > > config/pool_config_variables.c: In function ‘SetHBDestIfFunc’:<br>
> > > config/pool_config_variables.c:4945:5: warning: this ‘if’ clause does<br>
> > 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<br>
> > 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<br>
> > 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<br>
> > 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<br>
> > 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<br>
> > 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<br>
> > 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<br>
> > 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>
> > ><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<br>
> > editing.<br>
> > > ><br>
> > > > =======<br>
> > > > Changes<br>
> > > > =======<br>
> > > ><br>
> > > > The current watchdog and heartbeat configuration parameters (for 3<br>
> > 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<br>
> > 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<br>
> > 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<br>
> > 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<br>
> > paramaters as below:<br>
> > > >> > If it is the own node settings, "heartbeat_hostname" and<br>
> > "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>
> ><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>