Swarming: Adds CountTasks endpoint & Refactors swarming Prpc request#5277
Swarming: Adds CountTasks endpoint & Refactors swarming Prpc request#5277IvanBM18 wants to merge 8 commits into
Conversation
Xeicker
left a comment
There was a problem hiding this comment.
For future PRs, when you move code and then add functionality it is a good practice to do each of those steps in separate commits for an easier review
| ]) | ||
| self.assertEqual(spec, expected_spec) | ||
|
|
||
| def test_push_swarming_task(self): |
There was a problem hiding this comment.
Note: this test method is no longer needed here, in its place we added unit tests at the api_test that checks for the same functionality
There was a problem hiding this comment.
This test exercised a much more complex NewTaskRequest, consider adding some of that back to api_test.py.
d45af03 to
562ec1b
Compare
562ec1b to
57886fc
Compare
Xeicker
left a comment
There was a problem hiding this comment.
Other than some nits changes LGTM
| unique_dimensions = {} | ||
| unique_dimensions['os'] = str(job.platform).capitalize() | ||
| unique_dimensions['pool'] = _get_swarming_config().get('swarming_pool') | ||
| unique_dimensions['pool'] = get_swarming_config().get('swarming_pool') |
There was a problem hiding this comment.
We already looked up the config on line 82. Let's use it:
| unique_dimensions['pool'] = get_swarming_config().get('swarming_pool') | |
| unique_dimensions['pool'] = swarming_config.get('swarming_pool') |
| class SwarmingAPI: | ||
| """Client for Swarming pRPC API.""" | ||
|
|
||
| _config: SwarmingConfig = None |
There was a problem hiding this comment.
| _config: SwarmingConfig = None | |
| _config: SwarmingConfig | None = None |
| def _get_api(self) -> SwarmingAPI: | ||
| """Returns the Swarming API instance.""" | ||
| if not self._api: | ||
| self._api = SwarmingAPI() |
There was a problem hiding this comment.
Why instantiate this lazily? Let's just instantiate it at construction time, it's less surprising.
| A dict containing headers, or empty dict if config is missing or | ||
| auth fails. | ||
| """ | ||
| if not self._config: |
There was a problem hiding this comment.
If self._config is None, then this instance is useless. All its methods will just do nothing, and just log errors when invoked. It's a valid design choice, but it's a bit surprising: this is an API client, unless it's not and then nothing works!
I suggest giving this class a factory function that can fail if the config is missing, and exposing that failure to callers:
@staticmethod
def create() -> 'SwarmingAPI' | None:
config = get_swarming_config()
if config is None:
return None
return SwarmingAPI(config)
def __init__(self, config: SwarmingConfig):
self._config = config
self._base_url = ...Then you can use that in SwarmingService to create an optional _api = SwarmingAPI | None member. Alternatively, you can inline the contents of create() in SwarmingService.__init__().
At that point, you've just moved the problem up one layer (now SwarmingService might be useless) but to my sense it's still cleaner.
Now, I think that to make this truly clean you'd want to go even one layer higher and only instantiate SwarmingService in RemoteTaskGate if swarming is enabled, and fail loudly if the config is missing (presumably if swarming is enabled, we can expect to have a SwarmingConfig).
I see that SwarmingService is unconditionally instantiated by RemoteTaskGate as part of its dict of adapters 1. I suggest only instantiating adapters if the relevant feature flag is enabled:
self._service_map = {
adapter.id: adapter.service()
for adapter in remote_task_adapters.RemoteTaskAdapters
if adapter.feature_flag.enabled # new
}Then you only create instances of classes at runtime that you actually expect to use. This further limits the blast radius of changes to swarming code: if there is a bug in SwarmingService.__init__(), it won't be triggered in ClusterFuzz instances where Swarming is disables.
Finally, you can assert/crash if service instances cannot be constructed properly. In our case, you could assert in SwarmingService.__init__() that SwarmingAPI.create() succeeded, and make SwarmingService._api a non-optional member of type SwarmingAPI.
| mock_config.side_effect = ValueError('Failed to load') | ||
| api = SwarmingAPI() | ||
| response = api.push_task(swarming_pb2.NewTaskRequest()) | ||
| self.assertIsNone(response) |
There was a problem hiding this comment.
If you keep this design, you should check that no call was made to post_url().
| def test_push_task_no_credentials(self): | ||
| """Tests that push_task fails when credentials are missing.""" | ||
| self.mock.get_scoped_service_account_credentials.return_value = None | ||
| response = self.api.push_task(swarming_pb2.NewTaskRequest()) |
There was a problem hiding this comment.
We should check that post_url() was not called.
| def test_count_tasks_no_credentials(self): | ||
| """Tests that count_tasks fails when credentials are missing.""" | ||
| self.mock.get_scoped_service_account_credentials.return_value = None | ||
| response = self.api.count_tasks(swarming_pb2.TasksCountRequest()) |
| helpers.patch(self, [ | ||
| 'clusterfuzz._internal.swarming.is_swarming_task', | ||
| 'clusterfuzz._internal.swarming.push_swarming_task', | ||
| 'clusterfuzz._internal.swarming.service.SwarmingService._get_api', |
There was a problem hiding this comment.
If you define SwarmingApi.create(), you can mock that instead of a private helper.
| ]) | ||
| self.assertEqual(spec, expected_spec) | ||
|
|
||
| def test_push_swarming_task(self): |
There was a problem hiding this comment.
This test exercised a much more complex NewTaskRequest, consider adding some of that back to api_test.py.
As part of the Swarming backpressure initiative, we needed to launch request to the prpc's
swarming.v2.Task/CountTasksendpoint so we can calculate the amount of fuzz tasks needed to schedule, to achieve this i moved the prpc request logic to its own module so that we can reuse code and keep responsibilities in separate files.Changes
api.pyfile in the swarming module.__init__.pytoapi.pyinit.pyservice.pyto useSwarmingAPIdirectly.SwarmingAPIto handle the prpc request logic such as token/auth logic, swarming config handling, and any other request logicTests
Note:
This PR is dependendant to this other PR any changes you see related to the proto files are to be reviewed in said pr and will merged trough that PR.
So please review this PR first then you can go on and review this one :D