PR#525: Resolve exception when DB is disconnected - freshmaker

freshmaker

#525 Resolve exception when DB is disconnected

Merged 4 years ago by apaplaus. Opened 4 years ago by apaplaus.

apaplaus/freshmaker db_reconnect_bug into master

Resolve exception when DB is disconnected

Andrei Paplauski • 4 years ago

95b54c9

freshmaker/producer.py

file modified

		`@@ -31,6 +31,8 @@`
		`from freshmaker.events import BrewContainerTaskStateChangeEvent`
		`from freshmaker.consumer import work_queue_put`

		`+ from sqlalchemy.exc import StatementError`
		`+`

		`class FreshmakerProducer(PollingProducer):`
		`frequency = timedelta(seconds=conf.polling_interval)`
		`@@ -38,6 +40,9 @@`
		`def poll(self):`
		`try:`
		`self.check_unfinished_koji_tasks(db.session)`
		`+ except StatementError as ex:`
		`+ db.session.rollback()`
		`+ log.error("Invalid request, session is rolled back: %s", ex.orig)`
		`except Exception:`
		`msg = 'Error in poller execution:'`
		`log.exception(msg)`

tests/test_producer.py

file modified

+40 -4

		`@@ -57,7 +57,7 @@`
		`self.koji_read_config_patcher.stop()`

		`@patch('freshmaker.kojiservice.KojiService.get_task_info')`
		`- @patch("freshmaker.consumer.get_global_consumer")`
		`+ @patch('freshmaker.consumer.get_global_consumer')`
		`def test_koji_task_failed(self, global_consumer, get_task_info):`
		`consumer = self.create_consumer()`
		`global_consumer.return_value = consumer`
		`@@ -72,7 +72,7 @@`
		`self.assertEqual(event.new_state, "FAILED")`

		`@patch('freshmaker.kojiservice.KojiService.get_task_info')`
		`- @patch("freshmaker.consumer.get_global_consumer")`
		`+ @patch('freshmaker.consumer.get_global_consumer')`
		`def test_koji_task_closed(self, global_consumer, get_task_info):`
		`consumer = self.create_consumer()`
		`global_consumer.return_value = consumer`
		`@@ -87,7 +87,7 @@`
		`self.assertEqual(event.new_state, "CLOSED")`

		`@patch('freshmaker.kojiservice.KojiService.get_task_info')`
		`- @patch("freshmaker.consumer.get_global_consumer")`
		`+ @patch('freshmaker.consumer.get_global_consumer')`
		`def test_koji_task_dry_run(self, global_consumer, get_task_info):`
		`self.build.build_id = -10`
		`consumer = self.create_consumer()`
		`@@ -101,7 +101,7 @@`
		`self.assertRaises(queue.Empty, consumer.incoming.get, block=False)`

		`@patch('freshmaker.kojiservice.KojiService.get_task_info')`
		`- @patch("freshmaker.consumer.get_global_consumer")`
		`+ @patch('freshmaker.consumer.get_global_consumer')`
		`def test_koji_task_open(self, global_consumer, get_task_info):`
		`self.build.build_id = -10`
		`consumer = self.create_consumer()`
		`@@ -113,3 +113,39 @@`
		`producer = FreshmakerProducer(hub)`
		`producer.check_unfinished_koji_tasks(db.session)`
		`self.assertRaises(queue.Empty, consumer.incoming.get, block=False)`
		`+`
		`+ @patch('freshmaker.kojiservice.KojiService.get_task_info')`
		`+ @patch('freshmaker.consumer.get_global_consumer')`
		`+ def test_koji_invalid_request(self, global_consumer, get_task_info):`
		`+ from sqlalchemy.exc import StatementError, InvalidRequestError`
		`+ from sqlalchemy import select`
		`+ self.build.build_id = -10`
		`+ consumer = self.create_consumer()`
		`+ global_consumer.return_value = consumer`
		`+`
		`+ get_task_info.return_value = {'state': koji.TASK_STATES['OPEN']}`
		`+`
		`+ hub = MagicMock()`
		`+ producer = FreshmakerProducer(hub)`
		`+ # make new session to hold new connection and transaction`
		`+ my_session = db.session()`
		`+ # create connection and begin new transaction`
		`+ my_connection = my_session.connection()`
		`+ my_connection.begin()`
		`+ # invalidate one connection to simulate disconnect from DB`
		`+ my_connection.invalidate()`
		`+`
		`+ # check if it will raise Statement Error`
		`+ with self.assertRaises(StatementError) as cm:`
		`+ producer.check_unfinished_koji_tasks(my_session)`
		`+ # check if Statement Error is caused by InvalidRequestError`
		`+ self.assertIsInstance(cm.exception.orig, InvalidRequestError)`
		`+`
		`+ # rollback session, and rollback invalid transaction inside it`
		`+ my_session.rollback()`
		`+`
		`+ # check that new connection is created in our session`
		`+ self.assertIsNot(my_connection, my_session.connection())`
		`+ # Check if connection to db is established again`
		`+ my_session.connection().scalar(select([1]))`
		`+ self.assertFalse(my_session.connection().invalidated)`

apaplaus commented 4 years ago

Now when database is disconnected(restarted) during ongoing transaction,
that transaction will be rolled back and connection must be established
again on next query as expected.

RESOLVE: CLOUDWF-515

gnaponie commented on line 7 of tests/test_producer.py 4 years ago

I know this is picky :D but can you choose between ' and "? You used ' in the previous patch and " here. I know this is inconsistent between the old code base, but a least in the new introduced code it would be nice to be consistent. Maybe one day we can make a big PR to be consistent across all the code base.

apaplaus commented 4 years ago

I know this is picky :D but can you choose between ' and "? You used ' in the previous patch and " here. I know this is inconsistent between the old code base, but a least in the new introduced code it would be nice to be consistent. Maybe one day we can make a big PR to be consistent across all the code base.

Yes, sure. I will fix it.

gnaponie commented 4 years ago

Just a stupid comment, besides that it looks good! +1

rebased onto 2db31b16590e9a45b68265dc3dce6f69355fad0d

4 years ago

rebased onto 95b54c9

4 years ago

Pull-Request has been merged by apaplaus

4 years ago

gnaponie commented 4 years ago

Thanks!