summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorDavid Shrewsbury <shrewsbury.dave@gmail.com>2018-12-06 12:34:19 -0500
committerDavid Shrewsbury <shrewsbury.dave@gmail.com>2018-12-06 14:49:57 -0500
commitb75f463280eac224d7752c8ff6fff9497d9cb32a (patch)
tree0608888ee239af46be294643ff57a93e6825755d
parenta3e9ebf9f15c9cf184ec70ddb62f7473f5649c7c (diff)
Fix race in test_handler_poll_session_expired
Just waiting for a node request state change does not guarantee that we have actually entered the poll() portion of request handling. This was causing the mock.call_count assert to fail on occasion. Change this to simply wait on the call_count to increment, eliminating that race. There was also another potential race in checking that the request handler was removed from the request_handlers structure. It would be possible for the same request to re-enter active handling before we had a chance to check that it was removed when the session exception was thrown. We handle that race by setting the request state to FAILED on the first exception. Change-Id: I646b82243eb7f8c4e83d6678c2c0d265d99e51e0
Notes
Notes (review): Code-Review+2: Tobias Henkel <tobias.henkel@bmw.de> Code-Review+2: Clark Boylan <cboylan@sapwetik.org> Workflow+1: Clark Boylan <cboylan@sapwetik.org> Verified+2: Zuul Submitted-by: Zuul Submitted-at: Tue, 11 Dec 2018 19:00:53 +0000 Reviewed-on: https://review.openstack.org/623269 Project: openstack-infra/nodepool Branch: refs/heads/master
-rw-r--r--nodepool/tests/unit/test_launcher.py47
1 files changed, 35 insertions, 12 deletions
diff --git a/nodepool/tests/unit/test_launcher.py b/nodepool/tests/unit/test_launcher.py
index 46f62d6..958bd60 100644
--- a/nodepool/tests/unit/test_launcher.py
+++ b/nodepool/tests/unit/test_launcher.py
@@ -1653,31 +1653,54 @@ class TestLauncher(tests.DBTestCase):
1653 self.assertEqual(req.state, zk.FULFILLED) 1653 self.assertEqual(req.state, zk.FULFILLED)
1654 self.assertEqual(len(req.nodes), 1) 1654 self.assertEqual(len(req.nodes), 1)
1655 1655
1656 @mock.patch( 1656 @mock.patch('nodepool.driver.NodeRequestHandler.poll')
1657 'nodepool.driver.openstack.handler.OpenStackNodeRequestHandler.poll')
1658 def test_handler_poll_session_expired(self, mock_poll): 1657 def test_handler_poll_session_expired(self, mock_poll):
1659 ''' 1658 '''
1660 Test ZK session lost during handler poll(). 1659 Test ZK session lost during handler poll() removes handler.
1661 ''' 1660 '''
1662 mock_poll.side_effect = kze.SessionExpiredError() 1661 req = zk.NodeRequest()
1662 req.state = zk.REQUESTED
1663 req.node_types.append('fake-label')
1664 self.zk.storeNodeRequest(req)
1665
1666 # We need to stop processing of this request so that it does not
1667 # re-enter request handling, so we can then verify that it was
1668 # actually removed from request_handlers in the final assert of
1669 # this test.
1670 def side_effect():
1671 req.state = zk.FAILED
1672 # Intentionally ignore that it is already locked.
1673 self.zk.storeNodeRequest(req)
1674 raise kze.SessionExpiredError()
1675
1676 mock_poll.side_effect = side_effect
1663 1677
1664 # use a config with min-ready of 0 1678 # use a config with min-ready of 0
1665 configfile = self.setup_config('node_launch_retry.yaml') 1679 configfile = self.setup_config('node_launch_retry.yaml')
1666 self.useBuilder(configfile) 1680 self.useBuilder(configfile)
1681
1682 # Wait for the image to exist before starting the launcher, else
1683 # we'll decline the request.
1684 self.waitForImage('fake-provider', 'fake-image')
1685
1667 pool = self.useNodepool(configfile, watermark_sleep=1) 1686 pool = self.useNodepool(configfile, watermark_sleep=1)
1668 pool.cleanup_interval = 60 1687 pool.cleanup_interval = 60
1669 pool.start() 1688 pool.start()
1670 self.waitForImage('fake-provider', 'fake-image')
1671 1689
1672 req = zk.NodeRequest() 1690 # Wait for request handling to occur
1673 req.state = zk.REQUESTED 1691 while not mock_poll.call_count:
1674 req.node_types.append('fake-label') 1692 time.sleep(.1)
1675 self.zk.storeNodeRequest(req) 1693
1694 # Note: The launcher is not setting FAILED state here, but our mock
1695 # side effect should be doing so. Just verify that.
1696 req = self.waitForNodeRequest(req)
1697 self.assertEqual(zk.FAILED, req.state)
1676 1698
1677 # A session loss during handler poll should at least remove the 1699 # A session loss during handler poll should at least remove the
1678 # request from active handlers 1700 # request from active handlers. The session exception from our first
1679 req = self.waitForNodeRequest(req, states=(zk.PENDING,)) 1701 # time through poll() should handle removing the request handler.
1680 self.assertEqual(1, mock_poll.call_count) 1702 # And our mock side effect should ensure it does not re-enter
1703 # request handling before we check it.
1681 self.assertEqual(0, len( 1704 self.assertEqual(0, len(
1682 pool._pool_threads["fake-provider-main"].request_handlers)) 1705 pool._pool_threads["fake-provider-main"].request_handlers))
1683 1706