Add support for FUZZ_TARGET_BUILD_BUCKET_PATH (#663) by oliverchang · Pull Request #676 · google/clusterfuzz

oliverchang · 2019-07-12T01:18:02Z

Add support for using FUZZ_TARGET_BUILD_BUCKET_PATH, which is of the format:
gs://bucket/subdir/%TARGET%/([0-9]+).zip.
- When this is specified, build_manager will pick a fuzz target first (if it hasn't been picked already), and substitute'%TARGET% with the actual target name. This is the only new behaviour, as the resulting bucket path can then be passed to RegularBuild to continue regular setup.
Also add build_manager.get_primary_bucket_path, so that regression/progression can list builds properly.
Replace calls to setup_regular_build with setup_build.
Also remove revision fixing in schedule_corpus_pruning_tasks. This was needed back when
we were merging sancovs, and is no longer needed.
Move fuzzer_selection.get_fuzz_target_weights() out of build_manager and into fuzz_task. This removes an undesired dependency where get_fuzz_target_weights checks what the current task is, and makes it easier to prevent calling it multiple times unnecessarily.
Make revision_pattern_from_build_bucket_path less greedy to support zip filenames of the format ([0-9]+).zip

oliverchang · 2019-07-12T01:36:16Z

/gcbrun

oliverchang · 2019-07-12T02:44:25Z

/gcbrun

oliverchang · 2019-07-12T03:50:25Z

/gcbrun

oliverchang · 2019-07-12T04:01:57Z

/gcbrun

oliverchang · 2019-07-12T04:27:42Z

/gcbrun

oliverchang · 2019-07-12T04:28:53Z

/gcbrun

oliverchang · 2019-07-12T04:51:51Z

/gcbrun

oliverchang · 2019-07-12T05:01:35Z

/gcbrun

Dor1s

LGTM as it doesn't affect any production instances right now, but I guess we're going to discuss some stuff on Monday anyway

Dor1s · 2019-07-12T20:36:12Z

+
+    return fuzz_target_build_bucket_path.replace('%TARGET%', fuzz_target)
+
+  raise BuildManagerException('No primary bucket path defined.')


nit: maybe move this to line 1105 with an inverted condition, and then carry on with the other code without an extra intent

You mean like this?

release_build_bucket_path = environment.get_value('RELEASE_BUILD_BUCKET_PATH') if release_build_bucket_path: return release_build_bucket_path fuzz_target_build_bucket_path = environment.get_value( 'FUZZ_TARGET_BUILD_BUCKET_PATH') if not fuzz_target_build_bucket_path: raise BuildManagerException('No primary bucket path defined.') fuzz_target = environment.get_value('FUZZ_TARGET') if not fuzz_target: raise BuildManagerException('FUZZ_TARGET is not defined.') return fuzz_target_build_bucket_path.replace('%TARGET%', fuzz_target)

I feel like in this case avoiding the extra indents makes the logic a little less clear.

Yeah, this looks more straightforward to me, but it's not a big deal, up to you :)

I am ok with either (like current one slightly better since it give correlation for fuzz target branch). i just want BuildManagerException on more informative on RELEASE_BUILD_BUCKET_PATH and FUZZ_TARGET_BUILD_BUCKET_PATH

could you clarify where more info could be added to the exception messages?

I mean ". Please define RELEASE_BUILD_BUCKET_PATH or FUZZ_TARGET_BUILD_BUCKET_PATH in the job definition."

Dor1s · 2019-07-12T20:46:06Z

-  if not bucket_paths_env_vars:
+def _get_targets_list(bucket_path):
+  """Get the target list for a given bucket path."""
+  targets_list_path = os.path.join(


The path here will be gs://bucket/subdir/targets.list, right? Would we want revision separation for this, maybe like:

gs://bucket/subdir/targets_list/(0-9)+.txt

?

I can't find a good reason to keep it right away, but feel hesitant not to preserve those targets.list files either. Let me comment on the doc as well, we can discuss on Monday, if you want.

Will reply on the doc/tomorrow.

Can you add more details to this function description or comment here.

Dor1s · 2019-07-12T21:04:01Z

  if system_binary:
    return setup_system_binary()

+  fuzz_target_build_bucket_path = environment.get_value(


Was thinking if we could reuse get_primary_bucket_path here somehow, but don't see a way given that fuzz target is not chosen yet and the order of preference here is different.

Yes, the use case is a little different here, so I don't think we can re-use it here.

Dor1s · 2019-07-16T16:22:16Z

+
+    return fuzz_target_build_bucket_path.replace('%TARGET%', fuzz_target)
+
+  raise BuildManagerException('No primary bucket path defined.')


Yeah, this looks more straightforward to me, but it's not a big deal, up to you :)

inferno-chromium

Thanks a ton, once this is stable, please put the env var and short deascription in main documentation.

inferno-chromium · 2019-07-16T23:50:06Z

+
+    return fuzz_target_build_bucket_path.replace('%TARGET%', fuzz_target)
+
+  raise BuildManagerException('No primary bucket path defined.')


I am ok with either (like current one slightly better since it give correlation for fuzz target branch). i just want BuildManagerException on more informative on RELEASE_BUILD_BUCKET_PATH and FUZZ_TARGET_BUILD_BUCKET_PATH

inferno-chromium · 2019-07-16T23:50:37Z

-  if not bucket_paths_env_vars:
+def _get_targets_list(bucket_path):
+  """Get the target list for a given bucket path."""
+  targets_list_path = os.path.join(


Can you add more details to this function description or comment here.

inferno-chromium · 2019-07-16T23:52:22Z

+  """Set up targets build."""
+  targets_list = _get_targets_list(bucket_path)
+  if not targets_list:
+    raise BuildManagerException('No targets found')


in targets.list file ?

oliverchang · 2019-07-17T00:37:59Z

/gcbrun

oliverchang · 2019-07-17T00:58:30Z

/gcbrun

separate targets

9a5a5a3

googlebot added the cla: yes CLA signed. label Jul 12, 2019

oliverchang changed the title ~~WIP: Add support for UZZ_TARGET_BUILD_BUCKET_PATH~~ WIP: Add support for FUZZ_TARGET_BUILD_BUCKET_PATH Jul 12, 2019

oliverchang changed the title ~~WIP: Add support for FUZZ_TARGET_BUILD_BUCKET_PATH~~ WIP: Add support for FUZZ_TARGET_BUILD_BUCKET_PATH (#663) Jul 12, 2019

oliverchang added 2 commits July 12, 2019 12:11

move fuzzer weights retrieval out of build_manager

5da4a75

fix test

3dd67f3

fixes

e43572a

oliverchang added 2 commits July 12, 2019 14:00

one more cleanup

3d6da04

fix test

9d93de7

Fix test

fc4a777

really fix tests

64f3b13

oliverchang changed the title ~~WIP: Add support for FUZZ_TARGET_BUILD_BUCKET_PATH (#663)~~ Add support for FUZZ_TARGET_BUILD_BUCKET_PATH (#663) Jul 12, 2019

oliverchang requested review from Dor1s and inferno-chromium and removed request for Dor1s July 12, 2019 05:23

Dor1s approved these changes Jul 12, 2019

View reviewed changes

Dor1s approved these changes Jul 16, 2019

View reviewed changes

inferno-chromium approved these changes Jul 16, 2019

View reviewed changes

comments

1d16ee2

excception message

27bb4af

oliverchang merged commit beb3f03 into master Jul 17, 2019

inferno-chromium deleted the builds-draft branch July 17, 2019 01:23


		return fuzz_target_build_bucket_path.replace('%TARGET%', fuzz_target)

		raise BuildManagerException('No primary bucket path defined.')

Conversation

oliverchang commented Jul 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oliverchang commented Jul 12, 2019

Uh oh!

oliverchang commented Jul 12, 2019

Uh oh!

oliverchang commented Jul 12, 2019

Uh oh!

oliverchang commented Jul 12, 2019

Uh oh!

oliverchang commented Jul 12, 2019

Uh oh!

oliverchang commented Jul 12, 2019

Uh oh!

oliverchang commented Jul 12, 2019

Uh oh!

oliverchang commented Jul 12, 2019

Uh oh!

Dor1s left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

inferno-chromium left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oliverchang commented Jul 17, 2019

Uh oh!

oliverchang commented Jul 17, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

oliverchang commented Jul 12, 2019 •

edited

Loading