New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[DO NOT MERGE] Adding code for experiments using pubsub queues #2005

Open

gustavogaldinoo wants to merge 23 commits into master from measurer-pub-sub-experiment

Contributor

gustavogaldinoo commented Jul 25, 2024 •

edited

Loading

This PR adds code to use pub sub queues on the measurer when running cloud experiments, instead of python in-memory queues.

This PR is still a draft, I still want to do some improvements on top of it, but I appreciate any feedback.

Will try to trigger a cloud experiment before adding reviewers to it.


          Adding code for uexperiments ssing pubsub queues

cab13c4

Contributor Author

gustavogaldinoo commented Jul 25, 2024

/gcbrun run_experiment.py -a --experiment-config /opt/fuzzbench/service/experiment-config.yaml --benchmarks sqlite3_ossfuzz bloaty_fuzz_target --experiment-name measurer-pub-sub-test-experiment --fuzzers afl libfuzzer

Contributor Author

gustavogaldinoo commented Jul 25, 2024

/gcbrun run_experiment.py -a --experiment-config /opt/fuzzbench/service/experiment-config.yaml --benchmarks sqlite3_ossfuzz bloaty_fuzz_target --experiment-name pub-sub-test-experiment --fuzzers afl libfuzzer


          removed flaky test

f4fc755

Contributor Author

gustavogaldinoo commented Jul 25, 2024

/gcbrun run_experiment.py -a --experiment-config /opt/fuzzbench/service/experiment-config.yaml --benchmarks sqlite3_ossfuzz bloaty_fuzz_target --experiment-name pub-sub-test-experiment --fuzzers afl libfuzzer

Contributor Author

gustavogaldinoo commented Jul 25, 2024

/gcbrun run_experiment.py -a --experiment-config /opt/fuzzbench/service/experiment-config.yaml --benchmarks sqlite3_ossfuzz bloaty_fuzz_target --experiment-name pub-sub-test-experiment-2 --fuzzers afl libfuzzer

oliverchang requested review from jonathanmetzman and DonggeLiu

July 26, 2024 02:36

jonathanmetzman reviewed

View reviewed changes

Contributor

jonathanmetzman left a comment

Did a first pass. This looks like it's on the right path.

DonggeLiu reviewed

View reviewed changes

Contributor

DonggeLiu left a comment

Thanks @gustavogaldinoo : )

experiment/measurer/measure_manager.py Outdated

-                  measure_manager_loop(experiment, max_total_time, measurers_cpus,
-                                       region_coverage)
+                  cloud_project = experiment_config['cloud_project']
+                  local_experiment = experiment_config['local_experiment']

Contributor

DonggeLiu Jul 30, 2024

Use get() for both because the fields are not mandatory in the config file. E.g.,

cloud_project = experiment_config.get('cloud_project', '')
local_experiment = experiment_config.get('local_experiment', False)

experiment/measurer/measure_manager.py Outdated

+                  if not local_experiment:
+                      measure_manager = GoogleCloudMeasureManager(experiment, cloud_project,
+                                                                  region_coverage,
+                                                                  measurers_cpus)

Contributor

DonggeLiu Jul 30, 2024

Would it be better to initialize the needed manager only? E.g.,

if local_experiment:
  measure_manager = LocalMeasureManager(...)
else:
  measure_manager = GoogleCloudMeasureManager(...)

experiment/measurer/measure_manager.py Outdated

+                  """Base class for measure manager. Encapsulates core methods that will be
+                  implemented for Local and Google Cloud measure managers."""
+                  def __init__(self, experiment: str, region_coverage=False):

Contributor

DonggeLiu Jul 30, 2024

nit: Specify the type of region_coverage for consistency, given that experiment has type hinting.

experiment/measurer/measure_manager.py Outdated

+                      """Initialize and return request and response queues, respectively."""
+                      raise NotImplementedError
+                  def start_workers(self, request_queue, response_queue):

Contributor

DonggeLiu Jul 30, 2024

nit: Given that some of the functions in the file have type-hinting, consider consistently doing so for the params and return types of this function and other functions in the file.

Contributor Author

gustavogaldinoo Jul 30, 2024

Thanks, Dongge. I will try to type hint as much as possible.

I tried to do that at first, but I found a little hard to type hard the base class, as the types differ in each one of derived classes, so I chose to not type hint the base class, is that ok?

experiment/measurer/measure_manager.py Outdated

+                      self.experiment = experiment
+                  def initialize_queues(self):
+                      """Initialize and return request and response queues, respectively."""

Contributor

DonggeLiu Jul 30, 2024

nit: """Initializes and returns ... for consistency. Same for other new functions in this PR.

experiment/measurer/measure_manager.py Outdated


		return message.message.data

		return None

Contributor

DonggeLiu Jul 30, 2024

nit: Personally, I would prefer to rule out the simple edge cases first:

if not response.received_messages:
  return None
....
return message.message.data

experiment/measurer/measure_worker.py Outdated

+                      self.project_id = config['project_id']
+                      self.experiment = config['experiment']
+                      self.request_queue_subscription = f"""request-queue-subscription-
+                          {self.experiment}"""

Contributor

DonggeLiu Jul 30, 2024

nit: Same as above

self.request_queue_subscription = (f'request-queue-subscription-
            {self.experiment}')

experiment/measurer/measure_worker.py Outdated

+                          'name': self.subscription_path,
+                          'topic': topic_path
+                      })
+                      logger.info(f'Subscription {subscription.name} created successfully.')

Contributor

DonggeLiu Jul 30, 2024

nit: Same question about lazy formatting, is that required?

service/experiment-config.yaml

    
              trials: 20

              max_total_time: 82800  # 23 hours, the default time for preemptible experiments.

              trials: 3

              max_total_time: 3660

Contributor

DonggeLiu Jul 30, 2024

Miss-pushed debugging modifications?

service/gcbrun_experiment.py

@@ @@ -16,6 +16,7 @@ @@
               """Entrypoint for gcbrun into run_experiment. This script will get the command
               from the last PR comment containing "/gcbrun" and pass it to run_experiment.py
               which will run an experiment."""
+              # Dummy comment to trigger run experiment action

Contributor

DonggeLiu Jul 30, 2024

Miss-pushed debugging modifications?

Contributor Author

gustavogaldinoo Jul 30, 2024

The purpose of that comment was just to be able to run gcbrun on the PR, I will remove those changes before merging it.


          Some chnges based on PR feedback

fd6a30b

Contributor

jonathanmetzman commented Aug 6, 2024 •

edited

Loading

/gcbrun run_experiment.py -a --experiment-config /opt/fuzzbench/service/experiment-config.yaml --benchmarks sqlite3_ossfuzz bloaty_fuzz_target --experiment-name metzman-2024 --fuzzers afl libfuzzer

Contributor

jonathanmetzman commented Aug 6, 2024

/gcbrun run_experiment.py -a --experiment-config /opt/fuzzbench/service/experiment-config.yaml --benchmarks sqlite3_ossfuzz bloaty_fuzz_target --experiment-name metzman-2024-2 --fuzzers afl libfuzzer


          Fixing local experiments due to misusage of multiprocessing queue ins…

5042cbb

…tead of sync manager queue

gustavogaldinoo mentioned this pull request

[DO NOT MERGE] Adding pubsub requirement and rebuilding dispatcher image #2024

Closed


          Changing dispatcher startup script to use new image

88b0033

Contributor Author

gustavogaldinoo commented Aug 12, 2024

/gcbrun run_experiment.py -a --experiment-config /opt/fuzzbench/service/experiment-config.yaml --benchmarks sqlite3_ossfuzz bloaty_fuzz_target --experiment-name pubsub-measurer-1 --fuzzers afl libfuzzer

gustavogaldinoo added 2 commits

August 12, 2024 20:19


          Adding more error treatments, and logs, also changing create subscrip…

7484c35

…tion call


          Improving error handling and logging when calling google apis

6a157ce

Contributor Author

gustavogaldinoo commented Aug 12, 2024

/gcbrun run_experiment.py -a --experiment-config /opt/fuzzbench/service/experiment-config.yaml --benchmarks sqlite3_ossfuzz bloaty_fuzz_target --experiment-name pubsub-measurer-2 --fuzzers afl libfuzzer


          fixing typecheck

52eacb1

Contributor Author

gustavogaldinoo commented Aug 12, 2024

/gcbrun run_experiment.py -a --experiment-config /opt/fuzzbench/service/experiment-config.yaml --benchmarks sqlite3_ossfuzz bloaty_fuzz_target --experiment-name pubsub-measurer-3 --fuzzers afl libfuzzer


          Fixing task to bytes serializing function

632f23f

Contributor Author

gustavogaldinoo commented Aug 13, 2024

/gcbrun run_experiment.py -a --experiment-config /opt/fuzzbench/service/experiment-config.yaml --benchmarks sqlite3_ossfuzz bloaty_fuzz_target --experiment-name pubsub-measurer-4 --fuzzers afl libfuzzer


          Refactoring put result to response queue method, and enable ordering …

20d2f03

…messages

Contributor Author

gustavogaldinoo commented Aug 13, 2024

/gcbrun run_experiment.py -a --experiment-config /opt/fuzzbench/service/experiment-config.yaml --benchmarks sqlite3_ossfuzz bloaty_fuzz_target --experiment-name pubsub-measurer-5 --fuzzers afl libfuzzer


          Adding tests, enabling message ordering, and passing ordering key as …

a036561

…correct type (str)

Contributor Author

gustavogaldinoo commented Aug 13, 2024

/gcbrun run_experiment.py -a --experiment-config /opt/fuzzbench/service/experiment-config.yaml --benchmarks sqlite3_ossfuzz bloaty_fuzz_target --experiment-name pubsub-measurer-6 --fuzzers afl libfuzzer

gustavogaldinoo added 2 commits

August 14, 2024 16:43


          Fixing pubsub publish call, refactoring, and adding more tests

e413342


          Adding more tests and fixing format

adfe36d


          Formatting

d3b9c07

Contributor Author

gustavogaldinoo commented Aug 14, 2024

/gcbrun run_experiment.py -a --experiment-config /opt/fuzzbench/service/experiment-config.yaml --benchmarks sqlite3_ossfuzz bloaty_fuzz_target --experiment-name pubsub-measurer-7 --fuzzers afl libfuzzer


          Adding debug call, adding test for start workers, and fixing placehol…

67f99a1

…der docstring

Contributor Author

gustavogaldinoo commented Aug 15, 2024

/gcbrun run_experiment.py -a --experiment-config /opt/fuzzbench/service/experiment-config.yaml --benchmarks sqlite3_ossfuzz bloaty_fuzz_target --experiment-name pubsub-measurer-8 --fuzzers afl libfuzzer


          Fixing pubsub pull call

17d6ea0

Contributor Author

gustavogaldinoo commented Aug 16, 2024

/gcbrun run_experiment.py -a --experiment-config /opt/fuzzbench/service/experiment-config.yaml --benchmarks sqlite3_ossfuzz bloaty_fuzz_target --experiment-name pubsub-measurer-10 --fuzzers afl libfuzzer


          Wrapping initializing worker logs in a try statement

e8a20d6

Contributor Author

gustavogaldinoo commented Aug 16, 2024

/gcbrun run_experiment.py -a --experiment-config /opt/fuzzbench/service/experiment-config.yaml --benchmarks sqlite3_ossfuzz bloaty_fuzz_target --experiment-name pubsub-measurer-11 --fuzzers afl libfuzzer


          Changing consume snapshots method to raise queue empty when returning…

b523680

… None and tests

Contributor Author

gustavogaldinoo commented Aug 16, 2024

/gcbrun run_experiment.py -a --experiment-config /opt/fuzzbench/service/experiment-config.yaml --benchmarks sqlite3_ossfuzz bloaty_fuzz_target --experiment-name pubsub-measurer-12 --fuzzers afl libfuzzer

gustavogaldinoo added 2 commits

August 19, 2024 16:18


          Changing code to only use 1 gcloud worker for debugging purporses

f940442


          lint

4b2ec86

Contributor Author

gustavogaldinoo commented Aug 19, 2024

/gcbrun run_experiment.py -a --experiment-config /opt/fuzzbench/service/experiment-config.yaml --benchmarks sqlite3_ossfuzz bloaty_fuzz_target --experiment-name pubsub-measurer-14 --fuzzers afl libfuzzer


          Reverting previous debug commit, and changing get_task method

0c7987d

Contributor Author

gustavogaldinoo commented Aug 19, 2024

/gcbrun run_experiment.py -a --experiment-config /opt/fuzzbench/service/experiment-config.yaml --benchmarks sqlite3_ossfuzz bloaty_fuzz_target --experiment-name pubsub-measurer-15 --fuzzers afl libfuzzer


          Refactoring gcloud workers to initialize a subscriber and publisher c…

05d3668

…lient for each worker

Contributor Author

gustavogaldinoo commented Aug 22, 2024

/gcbrun run_experiment.py -a --experiment-config /opt/fuzzbench/service/experiment-config.yaml --benchmarks sqlite3_ossfuzz bloaty_fuzz_target --experiment-name pubsub-measurer-16 --fuzzers afl libfuzzer

Contributor Author

gustavogaldinoo commented Aug 22, 2024

/gcbrun run_experiment.py -a --experiment-config /opt/fuzzbench/service/experiment-config.yaml --benchmarks sqlite3_ossfuzz bloaty_fuzz_target --experiment-name pubsub-measurer-17 --fuzzers afl libfuzzer

Contributor Author

gustavogaldinoo commented Aug 22, 2024

/gcbrun run_experiment.py -a --experiment-config /opt/fuzzbench/service/experiment-config.yaml --benchmarks sqlite3_ossfuzz bloaty_fuzz_target --experiment-name pubsub-measurer-18 --fuzzers afl libfuzzer

Contributor Author

gustavogaldinoo commented Aug 22, 2024

/gcbrun run_experiment.py -a --experiment-config /opt/fuzzbench/service/experiment-config.yaml --benchmarks sqlite3_ossfuzz bloaty_fuzz_target --experiment-name pubsub-measurer-19 --fuzzers afl libfuzzer


          Only starting measure worker in new process

a365f7e

Contributor Author

gustavogaldinoo commented Aug 22, 2024

/gcbrun run_experiment.py -a --experiment-config /opt/fuzzbench/service/experiment-config.yaml --benchmarks sqlite3_ossfuzz bloaty_fuzz_target --experiment-name pubsub-measurer-20 --fuzzers afl libfuzzer

Contributor Author

gustavogaldinoo commented Aug 30, 2024

This PR is still a WIP.

In the experiments I've tried to run, it seems that we are having a problem in starting the measurer workers processes.

I couldn't debug to know the reason why this is happening.

As today is my last day at Google, am I afraid that I won't be able to debug any further and merge this PR so I am commenting to let you know that this is the current state of it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet