From 9b26ce33c33118470474e48224ec9056dc1ef39f Mon Sep 17 00:00:00 2001 From: James McTavish Date: Wed, 3 Aug 2022 14:12:00 +0000 Subject: [PATCH 1/6] Add retry for registration --- src/googleclouddebugger/firebase_client.py | 14 ++++- tests/firebase_client_test.py | 73 ++++++++++++++++------ 2 files changed, 64 insertions(+), 23 deletions(-) diff --git a/src/googleclouddebugger/firebase_client.py b/src/googleclouddebugger/firebase_client.py index 0da2a9e..482a736 100644 --- a/src/googleclouddebugger/firebase_client.py +++ b/src/googleclouddebugger/firebase_client.py @@ -22,6 +22,7 @@ import socket import sys import threading +import time import traceback import firebase_admin @@ -283,7 +284,12 @@ def _MainThreadProc(self): firebase_admin.initialize_app(self._credentials, {'databaseURL': self._database_url}) - self._RegisterDebuggee() + registration_required, delay = True, 0 + while registration_required: + print(f'attempting registration; delay is {delay}') + time.sleep(delay) + registration_required, delay = self._RegisterDebuggee() + self.registration_complete.set() self._SubscribeToBreakpoints() self.subscription_complete.set() @@ -324,10 +330,12 @@ def _RegisterDebuggee(self): self.register_backoff.Succeeded() return (False, 0) # Proceed immediately to subscribing to breakpoints. except BaseException: + # There is no significant benefit to handing different exceptions + # in different ways; we will log and retry regardless. native.LogInfo(f'Failed to register debuggee: {traceback.format_exc()}') except BaseException: - native.LogWarning('Debuggee information not available: ' + - traceback.format_exc()) + native.LogWarning( + f'Debuggee information not available: {traceback.format_exc()}') return (True, self.register_backoff.Failed()) diff --git a/tests/firebase_client_test.py b/tests/firebase_client_test.py index c1690b2..2649764 100644 --- a/tests/firebase_client_test.py +++ b/tests/firebase_client_test.py @@ -20,6 +20,7 @@ from absl.testing import parameterized import firebase_admin.credentials +from firebase_admin.exceptions import FirebaseError TEST_PROJECT_ID = 'test-project-id' METADATA_PROJECT_URL = ('http://metadata.google.internal/computeMetadata/' @@ -59,6 +60,27 @@ def setUp(self): self.breakpoints_changed_count = 0 self.breakpoints = {} + # Speed up the delays for retry loops. + self._client.register_backoff.min_interval_sec /= 100000.0 + self._client.register_backoff.max_interval_sec /= 100000.0 + self._client.register_backoff._current_interval_sec /= 100000.0 + + # Set up patchers. + patcher = patch('firebase_admin.initialize_app') + self._mock_initialize_app = patcher.start() + self.addCleanup(patcher.stop) + + patcher = patch('firebase_admin.db.reference') + self._mock_db_ref = patcher.start() + self.addCleanup(patcher.stop) + + # Set up the mocks for the database refs. + self._mock_register_ref = MagicMock() + self._fake_subscribe_ref = FakeReference() + self._mock_db_ref.side_effect = [ + self._mock_register_ref, self._fake_subscribe_ref + ] + def tearDown(self): self._client.Stop() @@ -105,33 +127,43 @@ def testSetupAuthNoProjectId(self): with self.assertRaises(firebase_client.NoProjectIdError): self._client.SetupAuth() - @patch('firebase_admin.db.reference') - @patch('firebase_admin.initialize_app') - def testStart(self, mock_initialize_app, mock_db_ref): + def testStart(self): + self._mock_register_ref.set.side_effect = [None] + self._client.SetupAuth(project_id=TEST_PROJECT_ID) + print('alive') self._client.Start() self._client.subscription_complete.wait() debuggee_id = self._client._debuggee_id - mock_initialize_app.assert_called_with( + self._mock_initialize_app.assert_called_with( None, {'databaseURL': f'https://{TEST_PROJECT_ID}-cdbg.firebaseio.com'}) self.assertEqual([ call(f'cdbg/debuggees/{debuggee_id}'), call(f'cdbg/breakpoints/{debuggee_id}/active') - ], mock_db_ref.call_args_list) + ], self._mock_db_ref.call_args_list) + + def testStartRegisterRetry(self): + # A new db ref is fetched on each retry. + self._mock_db_ref.side_effect = [ + self._mock_register_ref, self._mock_register_ref, + self._fake_subscribe_ref + ] + + # Fail once, then succeed on retry. + self._mock_register_ref.set.side_effect = [FirebaseError(1, 'foo'), None] + + self._client.SetupAuth(project_id=TEST_PROJECT_ID) + self._client.Start() + self._client.registration_complete.wait() - # TODO: testStartRegisterRetry - # TODO: testStartSubscribeRetry - # - Note: failures don't require retrying registration. + self.assertEqual(2, self._mock_register_ref.set.call_count) - @patch('firebase_admin.db.reference') - @patch('firebase_admin.initialize_app') - def testBreakpointSubscription(self, mock_initialize_app, mock_db_ref): - mock_register_ref = MagicMock() - fake_subscribe_ref = FakeReference() - mock_db_ref.side_effect = [mock_register_ref, fake_subscribe_ref] + def testStartSubscribeRetry(self): + print('TODO') + def testBreakpointSubscription(self): # This class will keep track of the breakpoint updates and will check # them against expectations. class ResultChecker: @@ -182,12 +214,13 @@ def callback(self, new_breakpoints): self._client.subscription_complete.wait() # Send in updates to trigger the subscription callback. - fake_subscribe_ref.update('put', '/', - {breakpoints[0]['id']: breakpoints[0]}) - fake_subscribe_ref.update('patch', '/', - {breakpoints[1]['id']: breakpoints[1]}) - fake_subscribe_ref.update('put', f'/{breakpoints[2]["id"]}', breakpoints[2]) - fake_subscribe_ref.update('put', f'/{breakpoints[0]["id"]}', None) + self._fake_subscribe_ref.update('put', '/', + {breakpoints[0]['id']: breakpoints[0]}) + self._fake_subscribe_ref.update('patch', '/', + {breakpoints[1]['id']: breakpoints[1]}) + self._fake_subscribe_ref.update('put', f'/{breakpoints[2]["id"]}', + breakpoints[2]) + self._fake_subscribe_ref.update('put', f'/{breakpoints[0]["id"]}', None) self.assertEqual(len(expected_results), result_checker._change_count) From 8b9a6fdbff5c408866cf7d5611425878a3cb766a Mon Sep 17 00:00:00 2001 From: James McTavish Date: Fri, 5 Aug 2022 16:05:20 +0000 Subject: [PATCH 2/6] More tests and fixes --- src/googleclouddebugger/firebase_client.py | 23 +++-- tests/firebase_client_test.py | 107 +++++++++++++++++++-- 2 files changed, 116 insertions(+), 14 deletions(-) diff --git a/src/googleclouddebugger/firebase_client.py b/src/googleclouddebugger/firebase_client.py index 482a736..4716ebf 100644 --- a/src/googleclouddebugger/firebase_client.py +++ b/src/googleclouddebugger/firebase_client.py @@ -14,6 +14,7 @@ """Communicates with Firebase RTDB backend.""" from collections import deque +import copy import hashlib import json import os @@ -115,6 +116,7 @@ def __init__(self): # Delay before retrying failed request. self.register_backoff = backoff.Backoff() # Register debuggee. + self.subscribe_backoff = backoff.Backoff() # Subscribe to updates. self.update_backoff = backoff.Backoff() # Update breakpoint. # Maximum number of times that the message is re-transmitted before it @@ -280,18 +282,19 @@ def _MainThreadProc(self): self._breakpoint_subscription. """ # Note: if self._credentials is None, default app credentials will be used. - # TODO: Error handling. firebase_admin.initialize_app(self._credentials, {'databaseURL': self._database_url}) registration_required, delay = True, 0 while registration_required: - print(f'attempting registration; delay is {delay}') time.sleep(delay) registration_required, delay = self._RegisterDebuggee() - self.registration_complete.set() - self._SubscribeToBreakpoints() + + subscription_required, delay = True, 0 + while subscription_required: + time.sleep(delay) + subscription_required, delay = self._SubscribeToBreakpoints() self.subscription_complete.set() def _TransmissionThreadProc(self): @@ -348,7 +351,13 @@ def _SubscribeToBreakpoints(self): path = f'cdbg/breakpoints/{self._debuggee_id}/active' native.LogInfo(f'Subscribing to breakpoint updates at {path}') ref = firebase_admin.db.reference(path) - self._breakpoint_subscription = ref.listen(self._ActiveBreakpointCallback) + try: + self._breakpoint_subscription = ref.listen(self._ActiveBreakpointCallback) + return (False, 0) + except firebase_admin.exceptions.FirebaseError: + native.LogInfo( + f'Failed to subscribe to breakpoints: {traceback.format_exc()}') + return (True, self.subscribe_backoff.Failed()) def _ActiveBreakpointCallback(self, event): if event.event_type == 'put': @@ -418,7 +427,7 @@ def _TransmitBreakpointUpdates(self): try: # Something has changed on the breakpoint. # It should be going from active to final, but let's make sure. - if not breakpoint_data['isFinalState']: + if not breakpoint_data.get('isFinalState'): raise BaseException( f'Unexpected breakpoint update requested: {breakpoint_data}') @@ -441,7 +450,7 @@ def _TransmitBreakpointUpdates(self): # Note that there may not be snapshot data. bp_ref = firebase_admin.db.reference( f'cdbg/breakpoints/{self._debuggee_id}/snapshots/{bp_id}') - bp_ref.set(breakpoint_data) + bp_ref.set(copy.deepcopy(breakpoint_data)) # Now strip potential snapshot data. breakpoint_data.pop('evaluatedExpressions', None) diff --git a/tests/firebase_client_test.py b/tests/firebase_client_test.py index 2649764..a45dc83 100644 --- a/tests/firebase_client_test.py +++ b/tests/firebase_client_test.py @@ -5,6 +5,7 @@ import socket import sys import tempfile +import time from unittest import mock from unittest.mock import MagicMock from unittest.mock import call @@ -61,9 +62,13 @@ def setUp(self): self.breakpoints = {} # Speed up the delays for retry loops. - self._client.register_backoff.min_interval_sec /= 100000.0 - self._client.register_backoff.max_interval_sec /= 100000.0 - self._client.register_backoff._current_interval_sec /= 100000.0 + for backoff in [ + self._client.register_backoff, self._client.subscribe_backoff, + self._client.update_backoff + ]: + backoff.min_interval_sec /= 100000.0 + backoff.max_interval_sec /= 100000.0 + backoff._current_interval_sec /= 100000.0 # Set up patchers. patcher = patch('firebase_admin.initialize_app') @@ -128,10 +133,7 @@ def testSetupAuthNoProjectId(self): self._client.SetupAuth() def testStart(self): - self._mock_register_ref.set.side_effect = [None] - self._client.SetupAuth(project_id=TEST_PROJECT_ID) - print('alive') self._client.Start() self._client.subscription_complete.wait() @@ -161,7 +163,21 @@ def testStartRegisterRetry(self): self.assertEqual(2, self._mock_register_ref.set.call_count) def testStartSubscribeRetry(self): - print('TODO') + mock_subscribe_ref = MagicMock() + mock_subscribe_ref.listen.side_effect = FirebaseError(1, 'foo') + + # A new db ref is fetched on each retry. + self._mock_db_ref.side_effect = [ + self._mock_register_ref, + mock_subscribe_ref, # Fail the first time + self._fake_subscribe_ref # Succeed the second time + ] + + self._client.SetupAuth(project_id=TEST_PROJECT_ID) + self._client.Start() + self._client.subscription_complete.wait() + + self.assertEqual(3, self._mock_db_ref.call_count) def testBreakpointSubscription(self): # This class will keep track of the breakpoint updates and will check @@ -224,6 +240,83 @@ def callback(self, new_breakpoints): self.assertEqual(len(expected_results), result_checker._change_count) + def testEnqueueBreakpointUpdate(self): + active_ref_mock = MagicMock() + snapshot_ref_mock = MagicMock() + final_ref_mock = MagicMock() + + self._mock_db_ref.side_effect = [ + self._mock_register_ref, self._fake_subscribe_ref, active_ref_mock, + snapshot_ref_mock, final_ref_mock + ] + + self._client.SetupAuth(project_id=TEST_PROJECT_ID) + self._client.Start() + self._client.subscription_complete.wait() + + debuggee_id = self._client._debuggee_id + breakpoint_id = 'breakpoint-0' + + input_breakpoint = { + 'id': breakpoint_id, + 'location': { + 'path': 'foo.py', + 'line': 18 + }, + 'isFinalState': True, + 'evaluatedExpressions': ['expressions go here'], + 'stackFrames': ['stuff goes here'], + 'variableTable': ['lots', 'of', 'variables'], + } + short_breakpoint = { + 'id': breakpoint_id, + 'location': { + 'path': 'foo.py', + 'line': 18 + }, + 'isFinalState': True, + 'action': 'CAPTURE', + 'finalTimeUnixMsec': { + '.sv': 'timestamp' + } + } + full_breakpoint = { + 'id': breakpoint_id, + 'location': { + 'path': 'foo.py', + 'line': 18 + }, + 'isFinalState': True, + 'action': 'CAPTURE', + 'evaluatedExpressions': ['expressions go here'], + 'stackFrames': ['stuff goes here'], + 'variableTable': ['lots', 'of', 'variables'], + 'finalTimeUnixMsec': { + '.sv': 'timestamp' + } + } + + self._client.EnqueueBreakpointUpdate(input_breakpoint) + + # Wait for the breakpoint to be sent. + while self._client._transmission_queue: + time.sleep(0.1) + + db_ref_calls = self._mock_db_ref.call_args_list + self.assertEqual( + call(f'cdbg/breakpoints/{debuggee_id}/active/{breakpoint_id}'), + db_ref_calls[2]) + self.assertEqual( + call(f'cdbg/breakpoints/{debuggee_id}/snapshots/{breakpoint_id}'), + db_ref_calls[3]) + self.assertEqual( + call(f'cdbg/breakpoints/{debuggee_id}/final/{breakpoint_id}'), + db_ref_calls[4]) + + active_ref_mock.delete.assert_called_once() + snapshot_ref_mock.set.assert_called_once_with(full_breakpoint) + final_ref_mock.set.assert_called_once_with(short_breakpoint) + def _TestInitializeLabels(self, module_var, version_var, minor_var): self._client.SetupAuth(project_id=TEST_PROJECT_ID) From 888145a0e59db603cce9bd8babeff2b3dfd177f3 Mon Sep 17 00:00:00 2001 From: James McTavish Date: Fri, 5 Aug 2022 16:55:17 +0000 Subject: [PATCH 3/6] fix indentation --- src/googleclouddebugger/firebase_client.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/googleclouddebugger/firebase_client.py b/src/googleclouddebugger/firebase_client.py index 4716ebf..a01d5c9 100644 --- a/src/googleclouddebugger/firebase_client.py +++ b/src/googleclouddebugger/firebase_client.py @@ -354,7 +354,7 @@ def _SubscribeToBreakpoints(self): try: self._breakpoint_subscription = ref.listen(self._ActiveBreakpointCallback) return (False, 0) - except firebase_admin.exceptions.FirebaseError: + except firebase_admin.exceptions.FirebaseError: native.LogInfo( f'Failed to subscribe to breakpoints: {traceback.format_exc()}') return (True, self.subscribe_backoff.Failed()) From d0b17a67ecb7cf0a5753e7451f40f86a328984b4 Mon Sep 17 00:00:00 2001 From: James McTavish Date: Fri, 5 Aug 2022 17:59:07 +0000 Subject: [PATCH 4/6] bugfix: data lost when retrying snapshot updates --- src/googleclouddebugger/firebase_client.py | 12 +- tests/firebase_client_test.py | 151 +++++++++++++++++++++ 2 files changed, 158 insertions(+), 5 deletions(-) diff --git a/src/googleclouddebugger/firebase_client.py b/src/googleclouddebugger/firebase_client.py index a01d5c9..f77a115 100644 --- a/src/googleclouddebugger/firebase_client.py +++ b/src/googleclouddebugger/firebase_client.py @@ -445,22 +445,24 @@ def _TransmitBreakpointUpdates(self): f'cdbg/breakpoints/{self._debuggee_id}/active/{bp_id}') bp_ref.delete() + summary_data = breakpoint_data # Save snapshot data for snapshots only. if is_snapshot: # Note that there may not be snapshot data. bp_ref = firebase_admin.db.reference( f'cdbg/breakpoints/{self._debuggee_id}/snapshots/{bp_id}') - bp_ref.set(copy.deepcopy(breakpoint_data)) + bp_ref.set(breakpoint_data) # Now strip potential snapshot data. - breakpoint_data.pop('evaluatedExpressions', None) - breakpoint_data.pop('stackFrames', None) - breakpoint_data.pop('variableTable', None) + summary_data = copy.deepcopy(breakpoint_data) + summary_data.pop('evaluatedExpressions', None) + summary_data.pop('stackFrames', None) + summary_data.pop('variableTable', None) # Then add it to the list of final breakpoints. bp_ref = firebase_admin.db.reference( f'cdbg/breakpoints/{self._debuggee_id}/final/{bp_id}') - bp_ref.set(breakpoint_data) + bp_ref.set(summary_data) native.LogInfo(f'Breakpoint {bp_id} update transmitted successfully') diff --git a/tests/firebase_client_test.py b/tests/firebase_client_test.py index a45dc83..664310a 100644 --- a/tests/firebase_client_test.py +++ b/tests/firebase_client_test.py @@ -317,6 +317,157 @@ def testEnqueueBreakpointUpdate(self): snapshot_ref_mock.set.assert_called_once_with(full_breakpoint) final_ref_mock.set.assert_called_once_with(short_breakpoint) + def testEnqueueBreakpointUpdateWithFailedLogpoint(self): + active_ref_mock = MagicMock() + snapshot_ref_mock = MagicMock() + final_ref_mock = MagicMock() + + self._mock_db_ref.side_effect = [ + self._mock_register_ref, self._fake_subscribe_ref, active_ref_mock, + final_ref_mock + ] + + self._client.SetupAuth(project_id=TEST_PROJECT_ID) + self._client.Start() + self._client.subscription_complete.wait() + + debuggee_id = self._client._debuggee_id + breakpoint_id = 'logpoint-0' + + input_breakpoint = { + 'id': breakpoint_id, + 'location': { + 'path': 'foo.py', + 'line': 18 + }, + 'action': 'LOG', + 'isFinalState': True, + 'status': { + 'isError': True, + 'refersTo': 'BREAKPOINT_SOURCE_LOCATION', + }, + } + output_breakpoint = { + 'id': breakpoint_id, + 'location': { + 'path': 'foo.py', + 'line': 18 + }, + 'isFinalState': True, + 'action': 'LOG', + 'status': { + 'isError': True, + 'refersTo': 'BREAKPOINT_SOURCE_LOCATION', + }, + 'finalTimeUnixMsec': { + '.sv': 'timestamp' + } + } + + self._client.EnqueueBreakpointUpdate(input_breakpoint) + + # Wait for the breakpoint to be sent. + while self._client._transmission_queue: + time.sleep(0.1) + + db_ref_calls = self._mock_db_ref.call_args_list + self.assertEqual( + call(f'cdbg/breakpoints/{debuggee_id}/active/{breakpoint_id}'), + db_ref_calls[2]) + self.assertEqual( + call(f'cdbg/breakpoints/{debuggee_id}/final/{breakpoint_id}'), + db_ref_calls[3]) + + active_ref_mock.delete.assert_called_once() + final_ref_mock.set.assert_called_once_with(output_breakpoint) + + def testEnqueueBreakpointUpdateRetry(self): + active_ref_mock = MagicMock() + snapshot_ref_mock = MagicMock() + final_ref_mock = MagicMock() + + # This test will have multiple failures, one for each of the firebase writes. + # UNAVAILABLE errors are retryable. + active_ref_mock.delete.side_effect = [ + FirebaseError('UNAVAILABLE', 'active error'), None, None, None + ] + snapshot_ref_mock.set.side_effect = [ + FirebaseError('UNAVAILABLE', 'snapshot error'), None, None + ] + final_ref_mock.set.side_effect = [ + FirebaseError('UNAVAILABLE', 'final error'), None + ] + + self._mock_db_ref.side_effect = [ + self._mock_register_ref, + self._fake_subscribe_ref, # setup + active_ref_mock, # attempt 1 + active_ref_mock, + snapshot_ref_mock, # attempt 2 + active_ref_mock, + snapshot_ref_mock, + final_ref_mock, # attempt 3 + active_ref_mock, + snapshot_ref_mock, + final_ref_mock # attempt 4 + ] + + self._client.SetupAuth(project_id=TEST_PROJECT_ID) + self._client.Start() + self._client.subscription_complete.wait() + + debuggee_id = self._client._debuggee_id + breakpoint_id = 'breakpoint-0' + + input_breakpoint = { + 'id': breakpoint_id, + 'location': { + 'path': 'foo.py', + 'line': 18 + }, + 'isFinalState': True, + 'evaluatedExpressions': ['expressions go here'], + 'stackFrames': ['stuff goes here'], + 'variableTable': ['lots', 'of', 'variables'], + } + short_breakpoint = { + 'id': breakpoint_id, + 'location': { + 'path': 'foo.py', + 'line': 18 + }, + 'isFinalState': True, + 'action': 'CAPTURE', + 'finalTimeUnixMsec': { + '.sv': 'timestamp' + } + } + full_breakpoint = { + 'id': breakpoint_id, + 'location': { + 'path': 'foo.py', + 'line': 18 + }, + 'isFinalState': True, + 'action': 'CAPTURE', + 'evaluatedExpressions': ['expressions go here'], + 'stackFrames': ['stuff goes here'], + 'variableTable': ['lots', 'of', 'variables'], + 'finalTimeUnixMsec': { + '.sv': 'timestamp' + } + } + + self._client.EnqueueBreakpointUpdate(input_breakpoint) + + # Wait for the breakpoint to be sent. Retries will have occured. + while self._client._transmission_queue: + time.sleep(0.1) + + active_ref_mock.delete.assert_has_calls([call()] * 4) + snapshot_ref_mock.set.assert_has_calls([call(full_breakpoint)] * 3) + final_ref_mock.set.assert_has_calls([call(short_breakpoint)] * 2) + def _TestInitializeLabels(self, module_var, version_var, minor_var): self._client.SetupAuth(project_id=TEST_PROJECT_ID) From 113bc068791cd11bcc6bbcd1cdba960da73e4990 Mon Sep 17 00:00:00 2001 From: James McTavish Date: Mon, 8 Aug 2022 14:42:31 +0000 Subject: [PATCH 5/6] Address PR comments --- src/googleclouddebugger/firebase_client.py | 54 +++++++++++----------- tests/firebase_client_test.py | 13 +++--- 2 files changed, 32 insertions(+), 35 deletions(-) diff --git a/src/googleclouddebugger/firebase_client.py b/src/googleclouddebugger/firebase_client.py index f77a115..4cb414a 100644 --- a/src/googleclouddebugger/firebase_client.py +++ b/src/googleclouddebugger/firebase_client.py @@ -20,7 +20,6 @@ import os import platform import requests -import socket import sys import threading import time @@ -282,8 +281,14 @@ def _MainThreadProc(self): self._breakpoint_subscription. """ # Note: if self._credentials is None, default app credentials will be used. - firebase_admin.initialize_app(self._credentials, - {'databaseURL': self._database_url}) + try: + firebase_admin.initialize_app(self._credentials, + {'databaseURL': self._database_url}) + except ValueError: + native.LogWarning( + f'Failed to initialize firebase: {traceback.format_exc()}') + native.LogError('Failed to start debugger agent. Giving up.') + return registration_required, delay = True, 0 while registration_required: @@ -319,28 +324,29 @@ def _RegisterDebuggee(self): Returns: (registration_required, delay) tuple """ + debuggee = None try: debuggee = self._GetDebuggee() self._debuggee_id = debuggee['id'] - - try: - debuggee_path = f'cdbg/debuggees/{self._debuggee_id}' - native.LogInfo( - f'registering at {self._database_url}, path: {debuggee_path}') - firebase_admin.db.reference(debuggee_path).set(debuggee) - native.LogInfo( - f'Debuggee registered successfully, ID: {self._debuggee_id}') - self.register_backoff.Succeeded() - return (False, 0) # Proceed immediately to subscribing to breakpoints. - except BaseException: - # There is no significant benefit to handing different exceptions - # in different ways; we will log and retry regardless. - native.LogInfo(f'Failed to register debuggee: {traceback.format_exc()}') except BaseException: native.LogWarning( f'Debuggee information not available: {traceback.format_exc()}') + return (True, self.register_backoff.Failed()) - return (True, self.register_backoff.Failed()) + try: + debuggee_path = f'cdbg/debuggees/{self._debuggee_id}' + native.LogInfo( + f'registering at {self._database_url}, path: {debuggee_path}') + firebase_admin.db.reference(debuggee_path).set(debuggee) + native.LogInfo( + f'Debuggee registered successfully, ID: {self._debuggee_id}') + self.register_backoff.Succeeded() + return (False, 0) # Proceed immediately to subscribing to breakpoints. + except BaseException: + # There is no significant benefit to handing different exceptions + # in different ways; we will log and retry regardless. + native.LogInfo(f'Failed to register debuggee: {traceback.format_exc()}') + return (True, self.register_backoff.Failed()) def _SubscribeToBreakpoints(self): # Kill any previous subscriptions first. @@ -427,7 +433,7 @@ def _TransmitBreakpointUpdates(self): try: # Something has changed on the breakpoint. # It should be going from active to final, but let's make sure. - if not breakpoint_data.get('isFinalState'): + if not breakpoint_data.get('isFinalState', False): raise BaseException( f'Unexpected breakpoint update requested: {breakpoint_data}') @@ -479,15 +485,7 @@ def _TransmitBreakpointUpdates(self): # This is very common if multiple instances are sending final update # simultaneously. native.LogInfo(f'{err}, breakpoint: {bp_id}') - except socket.error as err: - if retry_count < self.max_transmit_attempts - 1: - native.LogInfo(f'Socket error {err.errno} while sending breakpoint ' - f'{bp_id} update: {traceback.format_exc()}') - retry_list.append((breakpoint_data, retry_count + 1)) - else: - native.LogWarning(f'Breakpoint {bp_id} retry count exceeded maximum') - # Socket errors shouldn't persist like this; reconnect. - #reconnect = True + except BaseException: native.LogWarning(f'Fatal error sending breakpoint {bp_id} update: ' f'{traceback.format_exc()}') diff --git a/tests/firebase_client_test.py b/tests/firebase_client_test.py index 664310a..ccb35a1 100644 --- a/tests/firebase_client_test.py +++ b/tests/firebase_client_test.py @@ -1,8 +1,6 @@ """Unit tests for firebase_client module.""" -import errno import os -import socket import sys import tempfile import time @@ -13,7 +11,6 @@ import requests import requests_mock -from googleapiclient.errors import HttpError from googleclouddebugger import version from googleclouddebugger import firebase_client @@ -146,6 +143,10 @@ def testStart(self): call(f'cdbg/breakpoints/{debuggee_id}/active') ], self._mock_db_ref.call_args_list) + # Verify that the register call has been made. + self._mock_register_ref.set.assert_called_once_with( + self._client._GetDebuggee()) + def testStartRegisterRetry(self): # A new db ref is fetched on each retry. self._mock_db_ref.side_effect = [ @@ -317,9 +318,8 @@ def testEnqueueBreakpointUpdate(self): snapshot_ref_mock.set.assert_called_once_with(full_breakpoint) final_ref_mock.set.assert_called_once_with(short_breakpoint) - def testEnqueueBreakpointUpdateWithFailedLogpoint(self): + def testEnqueueBreakpointUpdateWithLogpoint(self): active_ref_mock = MagicMock() - snapshot_ref_mock = MagicMock() final_ref_mock = MagicMock() self._mock_db_ref.side_effect = [ @@ -386,7 +386,7 @@ def testEnqueueBreakpointUpdateRetry(self): snapshot_ref_mock = MagicMock() final_ref_mock = MagicMock() - # This test will have multiple failures, one for each of the firebase writes. + # This test will have three failures, one for each of the firebase writes. # UNAVAILABLE errors are retryable. active_ref_mock.delete.side_effect = [ FirebaseError('UNAVAILABLE', 'active error'), None, None, None @@ -416,7 +416,6 @@ def testEnqueueBreakpointUpdateRetry(self): self._client.Start() self._client.subscription_complete.wait() - debuggee_id = self._client._debuggee_id breakpoint_id = 'breakpoint-0' input_breakpoint = { From 7efb17eaab1820d8794c6811485b74e5c868600c Mon Sep 17 00:00:00 2001 From: James McTavish Date: Mon, 8 Aug 2022 15:22:53 +0000 Subject: [PATCH 6/6] Add check for snapshot node ref in logpoint test --- tests/firebase_client_test.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/tests/firebase_client_test.py b/tests/firebase_client_test.py index ccb35a1..1986a9a 100644 --- a/tests/firebase_client_test.py +++ b/tests/firebase_client_test.py @@ -381,6 +381,11 @@ def testEnqueueBreakpointUpdateWithLogpoint(self): active_ref_mock.delete.assert_called_once() final_ref_mock.set.assert_called_once_with(output_breakpoint) + # Make sure that the snapshots node was not accessed. + self.assertTrue( + call(f'cdbg/breakpoints/{debuggee_id}/snapshots/{breakpoint_id}') not in + db_ref_calls) + def testEnqueueBreakpointUpdateRetry(self): active_ref_mock = MagicMock() snapshot_ref_mock = MagicMock()