Swarming: Adds CountTasks endpoint & Refactors swarming Prpc request

[IvanBM18]

As part of the Swarming backpressure initiative, we needed to launch request to the prpc's swarming.v2.Task/CountTasks endpoint 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

• Created a new api.py file in the swarming module.
  • Moved the request logic from __init__.py to api.py
  • Removed push_swarming_task from init.py
  • Updated service.py to use SwarmingAPI directly.
  • Created a new class SwarmingAPI to handle the prpc request logic such as token/auth logic, swarming config handling, and any other request logic
• Created new unit tests for this api and Updated existing unit tests.

Tests

• Created unit tests for the new api & endpoint
• Scheduled swarming jobs to verify that the refactor didn't broke the existing swarming logic
  • See example logs

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

[Xeicker]

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

[Xeicker]

Other than some nits changes LGTM

[letitz]

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.

[IvanBM18]

Thanks a ton for this review, its pretty insightful and i actually learned a ton from it :D.
It makes sense for me to go ahead and "propagate" the problem to be exposed by the service.

But now that we mean to propagate issues, then i think its best if we revisit the exceptions too, so that we:

• Log and catch for specific exceptions in the service
• Unify the error catching for auth logic by letting the errors to be raised by the requests module instead

Also now the service will crash horribly with a ValueError (makes more sense than assertionError) if no config was found for envs that enable swarming

[IvanBM18]

Adding the last bit of changes(making the service crash at constructor) made the pr way bigger with changes to multiple test files, hence i created a separate feature branch for those specific changes to be reviewed in this PR