TRAFODION-2940 In HA env, one node lose network, when recover, trafci can't use #1427

mashengchen · 2018-01-31T10:53:08Z

when there loses network for a long time ,and then network recover, there will trigger zookeeper session expired, at this time ,check whether current dcsmaster is leader, if not unbind this node's floating ip, and make dcsmaster init status, then rerun dcs master.

Traf-Jenkins · 2018-01-31T10:57:14Z

Check Test Started: https://jenkins.esgyn.com/job/Check-PR-master/2399/

Traf-Jenkins · 2018-01-31T13:39:58Z

Test Passed. https://jenkins.esgyn.com/job/Check-PR-master/2399/

DaveBirdsall

I had one question about the changes.

DaveBirdsall · 2018-02-06T20:35:00Z

dcs/src/main/java/org/trafodion/dcs/master/DcsMaster.java

+                                LOG.info("Connected to ZooKeeper successful, restart DCS Master.");
+                                // reset lock
+                                isLeader = new CountDownLatch(1);
+                                break;


I'm not sure I understand this logic. Do we sit inside one of these method calls during normal processing? Or does tmpZkc.connect() and close() complete immediately? If so, it looks like we just loop around the while loop and do it again, over and over.

this logic is for when dcsmaster return with network error situation.
in the logic , it will try to connect to zk
if it can't conn ( tmpZkc.connect(); ) , there will in catch block and do retry
if it connect to zk, close the connect, then dcsmaster will run call() method again, in the time dcsmaster rework ,there must hava another backup-master working ( because there must one dcs master work and current master lose network ,then backupmaster take over the role) , so when dcsmaster rework , it will set value in zk /rootpath/dcs/leader/ then hang by lock "isLeader = new CountDownLatch(1);"

As we discussed, Zookeeper connection lost has been covered by session expired event, so this loop is useless.

moscowgentalman · 2018-02-07T03:54:45Z

Hope some DCS experts to take a review. @mashengchen please invite proper DCS experts. I cannot understand these changes well.

mashengchen · 2018-02-08T03:39:00Z

@arvind-narain @kevinxu021 can you help to take a look

hegdean · 2018-02-09T19:17:22Z

Can you please describe with more details regarding the original issue in the JIRA. It is unclear to me the scenario with 2 floating IPs. Let's hold off on the merge until we understand the original issue that will help us understand the changes done

mashengchen · 2018-02-11T09:01:43Z

config ha env
sqstart
use iptables to down master node's network, (iptables -I INPUT -s hostname -j DROP)
sleep for 300 seconds
backup-master take over the master role
recover network (iptables -I INPUT -s hostname -d DROP)
pdsh $MY_NODES ifconfig|grep 23400 will have 2 results, one is the down dcsmaster and another is the backup-master
actually , the old dcsmaster is still in while loop when network recover, so there will have 2 working dcsmaster.

DaveBirdsall · 2018-02-15T00:21:49Z

@hegdean, are you happy with this change now? Should I merge it?

svarnau

I am not versed in networking enough to judge teh whole change. Just comment on duplicated code.

This code was tested against the test scenario you described? And it solved the duplicate IP problem that the old code did not?

svarnau · 2018-02-15T22:18:43Z

dcs/bin/scripts/dcsunbind.sh

+                unbindlb=`echo "$myifport"|awk '{print $NF}'`
+                echo "Virtual ip $gv_float_external_ip is in use on node $HOSTNAME bound to interface $myinterface($unbindlb) - unbinding..."
+                sudo /sbin/ip addr del $unbindip dev $myinterface
+                status=$?


It seems unnecessary to duplicate entire function just to change whether commands run locally or remotely. That can be done with a variable, so that we don't have two copies of this code to maintain.

this function is a little different from the one using PDSH, the ouput of $SQ_PDSH $MY_NODES /sbin/ip addr show is different from /sbin/ip link show , so the cut step after the " | " (pipe) is different.

mashengchen · 2018-03-06T09:53:51Z

@svarnau yes , I had done the test, and it solved the duplicate IP problem

uamgo

Some fixes needed. Waiting for the changes.

uamgo · 2018-03-07T03:00:24Z

dcs/src/main/java/org/trafodion/dcs/master/DcsMaster.java

+                                LOG.info("Connected to ZooKeeper successful, restart DCS Master.");
+                                // reset lock
+                                isLeader = new CountDownLatch(1);
+                                break;


As we discussed, Zookeeper connection lost has been covered by session expired event, so this loop is useless.

uamgo · 2018-03-07T03:38:34Z

dcs/src/main/java/org/trafodion/dcs/master/DcsMaster.java

-            LOG.error(e);
-            System.exit(1);
+            LOG.error(e.getMessage(), e);
+            return 1;


Close zk connection while any connection exception happens. it's better to use specific Exception for zookeeper.

uamgo · 2018-03-07T03:41:14Z

dcs/src/main/java/org/trafodion/dcs/master/DcsMaster.java

@@ -203,8 +244,8 @@ public void run() {
        } catch (KeeperException.NodeExistsException e) {
            // do nothing...some other server has created znodes


it should be better to add warning for this exception that we could what happened. Any events happened should be reflected in log files.

uamgo · 2018-03-07T03:58:55Z

dcs/src/main/java/org/trafodion/dcs/master/DcsMaster.java

+                }
+            }
+            return 1;
+


Add try-catch for each progress.

uamgo · 2018-03-07T04:00:19Z

dcs/src/main/java/org/trafodion/dcs/master/FloatingIp.java

+        if (scriptContext.getExitCode() == 0)
+            LOG.info("Unbind Floating IP successful");
+        else
+            LOG.error("Unbind Floating IP failed");


It's better to print exit code.

uamgo · 2018-03-07T04:06:23Z

dcs/src/main/java/org/trafodion/dcs/master/listener/ListenerWorker.java

@@ -93,6 +93,7 @@ public void run() {
                        try {
                            queue.wait();
                        } catch (InterruptedException e) {
+                            return;


Log message for what happened by debug level.

uamgo · 2018-03-07T04:09:55Z

dcs/src/main/java/org/trafodion/dcs/zookeeper/ZkClient.java

+                        setSessionRecoverSuccessful(false);
+                    }
+                } catch (KeeperException e) {
+                    e.printStackTrace();


Log messages in file.

uamgo · 2018-03-07T04:10:29Z

dcs/src/main/java/org/trafodion/dcs/zookeeper/ZkClient.java

+                connect(true);
+            } catch (IOException e) {
+                setSessionRecoverSuccessful(false);
+                LOG.error("session expired and throw IOException while do reconnect: " + e.getMessage());


Use 2 params API for log. If it doesn't matter for whatever the message is, it should be LOG.warn.

uamgo · 2018-03-07T04:10:42Z

dcs/src/main/java/org/trafodion/dcs/zookeeper/ZkClient.java

+                setSessionRecoverSuccessful(false);
+                LOG.error("session expired and throw IOException while do reconnect: " + e.getMessage());
+            } catch (InterruptedException e) {
+                LOG.error("session expired and throw InterruptedException while do reconnect: " + e.getMessage());


The same as above.

uamgo · 2018-03-07T04:14:29Z

dcs/src/main/java/org/trafodion/dcs/zookeeper/ZkClient.java

+            // session expired, may be never happending. but if it happen there
+            // need to close old client and rebuild new client
+            try {
+                connect(true);


In connect(...), I saw it was catched for zookeeper exception, how can we get exception here?

… can not use

Traf-Jenkins · 2018-03-13T08:57:11Z

Check Test Started: https://jenkins.esgyn.com/job/Check-PR-master/2475/

Traf-Jenkins · 2018-03-13T13:40:43Z

Test Passed. https://jenkins.esgyn.com/job/Check-PR-master/2475/

Traf-Jenkins · 2018-05-30T06:39:12Z

Check Test Started: https://jenkins.esgyn.com/job/Check-PR-master/2691/

Traf-Jenkins · 2018-05-30T09:15:18Z

Test Passed. https://jenkins.esgyn.com/job/Check-PR-master/2691/

hegdean · 2018-05-31T00:05:58Z

@arvind can u please review this

arvind · 2018-05-31T01:26:24Z

I think the wrong Arvind was tagged :)

hegdean · 2018-05-31T01:28:14Z

@arvind-narain can u please review

Traf-Jenkins · 2018-07-27T12:11:47Z

Check Test Started: https://jenkins.esgyn.com/job/Check-PR-master/2887/

Traf-Jenkins · 2018-07-27T16:23:25Z

Test Failed. https://jenkins.esgyn.com/job/Check-PR-master/2887/

DaveBirdsall reviewed Feb 6, 2018

View reviewed changes

svarnau reviewed Feb 15, 2018

View reviewed changes

uamgo reviewed Mar 7, 2018

View reviewed changes

TRAFODION-2940 In HA env, one node lose network, when recover, trafci…

c43bb2e

… can not use

mashengchen force-pushed the TRAFODION-2940 branch from c92bd61 to c43bb2e Compare March 13, 2018 08:53

Merge branch 'master' into TRAFODION-2940

a7eab9f

		@@ -203,8 +244,8 @@ public void run() {
		} catch (KeeperException.NodeExistsException e) {
		// do nothing...some other server has created znodes

TRAFODION-2940 In HA env, one node lose network, when recover, trafci can't use #1427

Are you sure you want to change the base?

TRAFODION-2940 In HA env, one node lose network, when recover, trafci can't use #1427

Conversation

mashengchen commented Jan 31, 2018

Traf-Jenkins commented Jan 31, 2018

Traf-Jenkins commented Jan 31, 2018

DaveBirdsall left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mashengchen Feb 7, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moscowgentalman commented Feb 7, 2018

mashengchen commented Feb 8, 2018

hegdean commented Feb 9, 2018

mashengchen commented Feb 11, 2018

DaveBirdsall commented Feb 15, 2018

svarnau left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mashengchen commented Mar 6, 2018

uamgo 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uamgo Mar 7, 2018 • edited

Choose a reason for hiding this comment

Traf-Jenkins commented Mar 13, 2018

Traf-Jenkins commented Mar 13, 2018

Traf-Jenkins commented May 30, 2018

Traf-Jenkins commented May 30, 2018

hegdean commented May 31, 2018

arvind commented May 31, 2018

hegdean commented May 31, 2018

Traf-Jenkins commented Jul 27, 2018

Traf-Jenkins commented Jul 27, 2018

mashengchen Feb 7, 2018 •

edited

svarnau left a comment •

edited

uamgo Mar 7, 2018 •

edited