978307 - support desktop build builders being created from Mozharness (SigningScriptFactory) or through Buildbot (MBF)

Status: RESOLVED FIXED

(Release Engineering :: Applications: MozharnessCore, defect)

Description

[Jordan Lund (:jlund)]

In preparation of porting desktop builds from buildbot factories to mozharness, we need to allow the replacement of certain builders being created from MBF to SigningScriptFactory. This should be on a platform and branch basis so we can do things like enable all the linux builders to be mozharness builds on say Cypress only.

The benefit of landing this ability is we can continue porting platform specific builders over to mozharness on a very strict basis without disturbing the rest of the system.

Comment 1

[Jordan Lund (:jlund)]

Attachment: 978307_support_mozharness_desktop_builders-bbotcustom-140302.diff

Good news: eventually, generateBranchObjects() will be simplified by mozharn desktop builds.

Bad news: I will be making it worse for now.

So this and the accompanying buildbot-configs patch will accomplish the following:
    - allow for desktop mozharness builds to be enabled, per platform, per branch, and per variant (opt, pgo, nightly, xul, etc).

In order to do that, it gets messy. 

*Everything* goes through generateBranchObjects. Try, Repacks, and many sub variants. We also already have some builds that use mozharness: haz and b2g_builds.

I have only finished most of the linux flavors (32/64, opt, asan, debug, stat, pgo, nightly). IIUC, there are 3 remaining: xul, valgrind, and the recent non-unified.

So in order to do this, I needed a way to keep track of:
   1) which platforms support mozharness builds == 'mozharness_config' in pf

   2) of the platforms that support mozharness, which variants use mozharness == done_creating_{pgo, nightly, generic}_build boolean

   3) which platforms use mozharness but are not desktop builds == spider and b2g
   4) Which branches will use mozharness and which will use MBF == 'enable_mozharness_desktop_builds'

Knowing these three objectives, I hope they give logic to my patch. Being able to do this would be awesome. The alternative is to either:
    1) implement everything at once, fight bitrot, and turn it all on simultaneously while most likely breaking things.
    2) duplicate builds of MBF and mozharness side by side, causing huge regressions in performance and cost.

See buildbot-configs patch next for context.

I realize lot's can go wrong with this change. Hence me am asking for 4 eyes from folks familiar with this. As far as testing goes, I have generated builders_lists.py and vimdiff'd the effects. I'll post the output of those in this bug too. Once r+, I'll need to try this out on default (cedar).

Comment 2

[Jordan Lund (:jlund)]

Attachment: 978307_support_mozharness_desktop_builders-bbot-cfgs-140302.diff

[see bbotcustom attachment for details]

Comment 3

[Jordan Lund (:jlund)]

Attachment: blist_wo_mhbuilds

clean builder_list.py generation against current 'default'

Comment 4

[Jordan Lund (:jlund)]

Attachment: blist_w_mhbuilds_cypress_disabled

current builder_list generated with my patch implemented. No branch is enabled for desktop mozharness builds but the piping is in place to switch any branch on.

vimdiff'n this against clean 'default' yields no diff.

Comment 5

[Jordan Lund (:jlund)]

Attachment: blist_w_mhbuilds_cypress_enabled

Finally, this is for when we are ready to enable this on a given branch. I 'flipped the switch' on Cypress for an example (what I have running on my dev-master). NOTE: again, my patch attachments for this bug will not have Cypress enabled or any branch for that matter. This is just to show the affect.

The vimdiff against clean builder_list.py can be found here: http://grab.by/uNr2

As you can see, this only Builder factory classes for Cypress and only Linux variants.

Comment 6

[[e-mail] (:bhearsum)]

Comment on attachment 8384454 [details] [diff] [review]
978307_support_mozharness_desktop_builders-bbotcustom-140302.diff

Review of attachment 8384454 [details] [diff] [review]:
-----------------------------------------------------------------

::: misc.py
@@ +1049,5 @@
> +        # logic in this loop for genereting build names.
> +        is_spider_build = any(name in pf['base_name']
> +                              for name in ['linux64-br-haz', 'linux64-sh-haz'])
> +        is_b2g_build = 'b2g' in pf['product_name']
> +        if 'mozharness_config' in pf and (is_spider_build or is_b2g_build):

Before I review everything else I want to know if you've considered moving this sort of thing to the configs. Perhaps there could be a pf['mozharness_builds'] that has a list of things that should be in mozharness. Do you think that could make it cleaner/simpler here?

Comment 7

[Rail Aliiev [:rail]]

+1 for moving those to the configs. This kind of conditions are very fragile and can bite us in the future.

python -m this | sed -n 4p
 ;)

Comment 8

[[e-mail] (:bhearsum)]

Comment on attachment 8384454 [details] [diff] [review]
978307_support_mozharness_desktop_builders-bbotcustom-140302.diff

Per IRC, new patches are coming at some point.

Comment 9

[Jordan Lund (:jlund)]

status update:

Sorry for the delay, I was on buildduty last week and spent last two days getting a patch for 978319


anyway, I have new patches for this. I hope they address the initial concerns. Rather than adding more lists to buildbot-configs like 'mozharness_builds' and having to maintain that as we add things like b2g devices, I opted for just addressing when we have a desktop build available through mozharness.

So if we have 'mozharness_config', we know there is a mozharness build for this (same logic as before). But what I added, in a nutshell, is the following:

+        continue_with_mozharness_build = True
+        # we use this condition to enable/disable on a per platform basis
+        if pf.get('has_desktop_mozharness_build'):
+            # we use this condition to enable/disable on a per branch basis
+            if not config.get('desktop_mozharness_builds_enabled'):
+                continue_with_mozharness_build = False

This allows us to (a) know when we are doing a ff desktop build and (b) switch it on/off on a platform and branch basis.




One other note on all this. To expand on Comment 1 in this bug: You could ask why we have things like:
+        done_creating_generic_build = False
+        done_creating_pgo_build = False
+        done_creating_nightly_build = False
+        done_creating_nonunified_build = False

my reason for this so I could (i) keep the logic of the existing generateBranchObjects code pretty much untouched and (ii) in addition to having per branch/platform basis, also have the ability to do per builder (so I could leave things like xul builders and valgrind till later).

the nice thing about (i) is that when (ii) is complete, we can remove all these boolean checks and simply say: "hey if this is a desktop mozharness able platform and we have it enabled on the given branch, run something like branchObjects['builders'].extend(generateDesktopMozharnessBuilders(config, name))" and that would mean simply extracting all the top 'mozharness_config' builder logic out of genBranchObj.

Furthermore, when we are finished and don't need (b) or, in other words, all platforms and branches use mozharness builders, then we can simply delete all the bottom MBF factory builder logic out (the part that exists today) of genBranchObj.

tl;dr - this patch keeps the logic of mozharness builds and MBF factories separate/independent.

Comment 10

[Jordan Lund (:jlund)]

Attachment: 978307_support_mozharness_desktop_builders-bbotcustom-140314.diff

I'll leave it up to you guys if you think only one of you needs to r?

these pass checkconfig and test-masters.sh

I also generated builders lists again for three states (just like before):

1) builder list against 'default'
2) builder list against no desktop mh builds enabled on any branch (this patch)
3) builder list against desktop mh builds enabled on only cypress

1 and 2 yielded no diff. This is what I expected as this patch/bug should be a no-op.

1 and 3 only changed the builder factory class for linux platforms on cypress. That too is what's expected. That diff can be seen here: http://www.diffchecker.com/diff

Comment 11

[Jordan Lund (:jlund)]

Attachment: 978307_support_mozharness_desktop_builders-bbot-cfgs-140314.diff

accompanying buildbot-config patch for buildbotcustom

Comment 12

[[e-mail] (:bhearsum)]

Comment on attachment 8390808 [details] [diff] [review]
978307_support_mozharness_desktop_builders-bbotcustom-140314.diff

Review of attachment 8390808 [details] [diff] [review]:
-----------------------------------------------------------------

::: misc.py
@@ +729,5 @@
> +    if kwargs.get('extra_args'):
> +        kwargs['extra_args'] = mh_cfg.get('extra_args', []) + kwargs['extra_args']
> +    else:
> +        kwargs['extra_args'] = mh_cfg.get('extra_args', None)
> +

I think it would be simpler if you did this merging in the calling function.

@@ +1367,5 @@
> +        # and nightly via mozharness and fall back to buildbot for other
> +        # builder factory logic  (outside the mozharness_config condition)
> +        # NOTE: when we no longer need to fall back for remaining builders,
> +        # we should extract the desktop mozharness builder logic out into
> +        # something like: generateDesktopMozharnessBuilders()

Perhaps it would help to create two functions for now: generateDesktopMozharnessBuilders() and generateDesktopMBFBuilders()? I think this would make this much easier to read and modify....

@@ +1421,5 @@
> +                nightly_extra_args = generic_extra_args + ['--enable-pgo',
> +                                                           '--enable-nightly']
> +                pgo_extra_args = generic_extra_args + ['--enable-pgo']
> +                nonunified_extra_args = generic_extra_args + [
> +                    '--custom-build-variant-cfg', 'non-unified']

You're already using extra configs for a bunch of build types (eg, asan). I think you can get rid of these extra args and pass an extra config instead. You can even move the config name to config.py, eg:
branches['mozilla-central']['platforms']['linux']['pgo_mozharness_config'] = 'whatever'

Comment 13

[[e-mail] (:bhearsum)]

Comment on attachment 8390812 [details] [diff] [review]
978307_support_mozharness_desktop_builders-bbot-cfgs-140314.diff

Review of attachment 8390812 [details] [diff] [review]:
-----------------------------------------------------------------

::: mozilla/config.py
@@ +318,5 @@
> +            'mozharness_config': {
> +                'script_name': 'scripts/fx_desktop_build.py',
> +                'extra_args': [
> +                    '--config', 'builds/releng_base_linux_64_builds.py',
> +                    '--custom-build-variant-cfg', 'asan',

This is probably something better off in the mozharness patch review, but why does --custom-build-variant-cfg exist? You can already pass multiple config files with --config.

::: mozilla/project_branches.py
@@ +185,5 @@
>          'enable_talos': True,
> +        # once ready, we can flip this switch and any platform with
> +        # mozharness_config in its build config will use mozharness instead
> +        # of MozharnessBuildFactory on only cypress
> +        'desktop_mozharness_builds_enabled': False,

It's not clear to me why you need this or has_desktop_mozharness_build. Why not key off of pf['mozharness_config'] itself?

Comment 14

[Jordan Lund (:jlund)]

Awesome! I think we are making progress. Thanks for review so far.

> ::: misc.py
> @@ +729,5 @@
> > +    if kwargs.get('extra_args'):
> > +        kwargs['extra_args'] = mh_cfg.get('extra_args', []) + kwargs['extra_args']
> > +    else:
> > +        kwargs['extra_args'] = mh_cfg.get('extra_args', None)
> > +
> 
> I think it would be simpler if you did this merging in the calling function.

sounds good. Will do

> 
> @@ +1367,5 @@
> > +        # and nightly via mozharness and fall back to buildbot for other
> > +        # builder factory logic  (outside the mozharness_config condition)
> > +        # NOTE: when we no longer need to fall back for remaining builders,
> > +        # we should extract the desktop mozharness builder logic out into
> > +        # something like: generateDesktopMozharnessBuilders()
> 
> Perhaps it would help to create two functions for now:
> generateDesktopMozharnessBuilders() and generateDesktopMBFBuilders()? I
> think this would make this much easier to read and modify....

OK cool. How about leaving existing mozharness and MBF builder logic and just create a generateDesktopMozharnessBuilders() for my stuff that I need. I'm not sure how worth it it is to go messing with existing genBranchObj. But of course if you think that's something we need to do, I can refactor it.

> 
> @@ +1421,5 @@
> > +                nightly_extra_args = generic_extra_args + ['--enable-pgo',
> > +                                                           '--enable-nightly']
> > +                pgo_extra_args = generic_extra_args + ['--enable-pgo']
> > +                nonunified_extra_args = generic_extra_args + [
> > +                    '--custom-build-variant-cfg', 'non-unified']
> 
> You're already using extra configs for a bunch of build types (eg, asan). I
> think you can get rid of these extra args and pass an extra config instead.
> You can even move the config name to config.py, eg:
> branches['mozilla-central']['platforms']['linux']['pgo_mozharness_config'] =
> 'whatever'

I think ultimately we will need to leave stuff like --branch and --build-pool in genBranchObj because I think only things like misc.py will know what those values are (you prob know that because you didn't mention it along with this pgo and nightly options)

The reason I didn't do this was for nightly and pgo because we currently implement all the logic for nightly, pgo, and, more recently, non-unified and non-profiling all in misc.py. We do other builds like debug, asan, etc in config.py. I didn't want to break the mold for just mozharness desktop builds because I thought adding whole new platforms to branches['mozilla-central']['platforms'] would break things. Doing what you suggest seems to make more sense as it wouldn't be adding a new platform. However, we would need add 'pgo_mozharness_config' to every platform that supports pgo (and the same for nightly/nonunified etc). And all of these platforms would have the same extra_args. Not a big deal just pointing that out.

Maybe what would be better is if had something like this in config.py:

desktop_mozharness_build_extra_args = {
    'pgo': ['--enable-pgo'],
    'nightly: ['--enable-nightly'],
    'non-unified': ['--custom-build-variant', 'non-unified'],
    etc ...
}

then in misc.py we could say:

if mozharness_desktop_builder:
    if branch_and_platform_need_pgo_builder:
        factory_extra_args = desktop_mozharness_build_extra_args['pgo']
    etc ...

Although that would be confusing because for things like asan and debug, they have their own platforms so they wouldn't follow this desktop_mozharness_build_extra_args pattern... :(

Comment 15

[Jordan Lund (:jlund)]

(In reply to Ben Hearsum [:bhearsum] from comment #13)
> Comment on attachment 8390812 [details] [diff] [review]
> 978307_support_mozharness_desktop_builders-bbot-cfgs-140314.diff
> 
> Review of attachment 8390812 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mozilla/config.py
> @@ +318,5 @@
> > +            'mozharness_config': {
> > +                'script_name': 'scripts/fx_desktop_build.py',
> > +                'extra_args': [
> > +                    '--config', 'builds/releng_base_linux_64_builds.py',
> > +                    '--custom-build-variant-cfg', 'asan',
> 
> This is probably something better off in the mozharness patch review, but
> why does --custom-build-variant-cfg exist? You can already pass multiple
> config files with --config.

ahh but here is the best part. you can for sure just pass more '--config' options but this way, you don't need to specify the path to the platform and config file that uses that. The same goes for '--branch' and '--build-pool'

so this:
./scripts/fx_desktop_build.py --config builds/releng_base_linux_64_builds.py --custom-build-variant-cfg asan --branch mozilla-central --build-pool staging

is the same as:
./scripts/fx_desktop_build.py --config builds/releng_base_linux_64_builds.py --config builds/sub_linux_configs/64_asan.py --config builds/branch_specifics.py --branch mozilla-central --config builds/build_pool_specifics.py --build-pool staging

The first way is cleaner. Don't worry, the logs tells you how it's finding the config files and tells you how it puts them together to make self.config. You can use either way so it's only an optional convenience for some and can be ignored by others. Where I think this will be nice is when we call this script outside of automation and can just say things like 'asan'.

Furthermore, the first way doesn't care what order you put the options where as if you specify many '--config' options, order matters because the hierarchy of precedence goes from last entered to first.

> 
> ::: mozilla/project_branches.py
> @@ +185,5 @@
> >          'enable_talos': True,
> > +        # once ready, we can flip this switch and any platform with
> > +        # mozharness_config in its build config will use mozharness instead
> > +        # of MozharnessBuildFactory on only cypress
> > +        'desktop_mozharness_builds_enabled': False,
> 
> It's not clear to me why you need this or has_desktop_mozharness_build. Why
> not key off of pf['mozharness_config'] itself?

so in https://bugzilla.mozilla.org/show_bug.cgi?id=978307#c9 I talked about needing a way to differientiate between (1) desktop mozharness builders and every other mozharness builder (2) which platforms supported mozharness builders and (3) which branch had mozharness builders switched on.

if I removed 'desktop_mozharness_builds_enabled' then I would only be accomplishing (2) and (3). Keeping it in means I don't have to keep track of all the other mozharness builder platforms (like all the ever growing b2g builders, etc) and instead just point out the ones that are *desktop* mozharness builders.

alternatively I could have somethign like this at the beginning of config.py:
    desktop_mozharness_builders = ['linux', 'linux-debug', etc]

and then in misc have:

instead of:

+        if pf.get('has_desktop_mozharness_build'):
+            # we use this condition to enable/disable on a per branch basis
+            if not config.get('desktop_mozharness_builds_enabled'):
+                continue_with_mozharness_build = False

I put:

+        if platform not in config['desktop_mozharness_builders']:
+            # we use this condition to enable/disable on a per branch basis
+            if not config.get('desktop_mozharness_builds_enabled'):
+                continue_with_mozharness_build = False


Both make sense to me. Whatever you think.

Comment 16

[Jordan Lund (:jlund)]

Attachment: 978307_support_mozharness_desktop_builders-bbotcustom-140315.diff

Myself and Ben met and discussed these latest comments. Here is the revised patches that reflects the comments/discussion.

these new patches have been put through test-masters.sh and dump_master.py

Comment 17

[Jordan Lund (:jlund)]

Attachment: 978307_support_mozharness_desktop_builders-bbot-cfgs-140315.diff

Comment 18

[Jordan Lund (:jlund)]

Attachment: dmast_diff_mast_mhb_cyp_enabled

NOTE: this is not a dump_master.py diff of what these patches will do.

these patches should be a no-op (a dump_master.py diff of these exact patches does yield nothing changed)

This diff attachment is of everything included in the patches but with:
     'cypress': {
         'mozharness_tag': 'default',
         'enable_talos': True,
+        # once ready, we can flip this switch and any platform with
+        # mozharness_config in its build config will use mozharness instead
+        # of MozharnessBuildFactory on only cypress
+        'desktop_mozharness_builds_enabled': True,
     },

instead of:
     'cypress': {
         'mozharness_tag': 'default',
         'enable_talos': True,
+        # once ready, we can flip this switch and any platform with
+        # mozharness_config in its build config will use mozharness instead
+        # of MozharnessBuildFactory on only cypress
+        'desktop_mozharness_builds_enabled': False,
     },

Here is the full output of dump_master.py against cypress desktop mh builds being enabled: http://people.mozilla.org/~jlund/dmast_w_mhbuilds_cypress_enabled-140315

Comment 19

[Jordan Lund (:jlund)]

Attachment: 978307_support_mozharness_desktop_builders-bbotcustom-140317.diff

this includes a fix to allow for non-unified builds on opt *and* debug platforms.

Here is a diff of what I changed: https://pastebin.mozilla.org/4613458

Comment 20

[Jordan Lund (:jlund)]

Attachment: 978307_support_mozharness_desktop_builders-bbot-cfgs-140317.diff

diff of what changed: https://pastebin.mozilla.org/4613463

Comment 21

[Jordan Lund (:jlund)]

Attachment: 978307_support_mozharness_desktop_builders-bbotcustom-140317-2.diff

I jumped the gun on the last one. test-masters.sh failed on try masters when cypress was enabled. the below addition fixes it.

same patch but with:
     if config.get('enable_try'):
         # let's return immediately and let MBF create all the builders
-        return builds_created
+        return desktop_mh_builders

Comment 22

[[e-mail] (:bhearsum)]

Comment on attachment 8392450 [details] [diff] [review]
978307_support_mozharness_desktop_builders-bbotcustom-140317-2.diff

Review of attachment 8392450 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not crazy about this, but I think much of that is due to misc.py sucking already, rather than what you're doing. This is definitely much better and more readable than the previous patches. Ship it now before you get bitrotted :).

::: misc.py
@@ +1772,5 @@
> +            # TEMP CODE. This condition just checks to see if we used
> +            # mozharness to create this builder already. Once we port all
> +            # builders to mozharness we won't need mozilla2_dep_builder at
> +            # all
> +            if not builder_tracker['done_generic_build']:

This checks should probably go around factory instantiation rather than here, otherwise we'll do a bunch more work than we need to with every checkconfig or reconfig.

I wouldn't use the phrase "TEMP CODE", either. I promise you this will live for much longer than you expect.

Comment 23

[Jordan Lund (:jlund)]

> I'm not crazy about this, but I think much of that is due to misc.py sucking
> already, rather than what you're doing. This is definitely much better and
> more readable than the previous patches. Ship it now before you get
> bitrotted :).
> 

\o/

> ::: misc.py
> @@ +1772,5 @@
> > +            # TEMP CODE. This condition just checks to see if we used
> > +            # mozharness to create this builder already. Once we port all
> > +            # builders to mozharness we won't need mozilla2_dep_builder at
> > +            # all
> > +            if not builder_tracker['done_generic_build']:
> 
> This checks should probably go around factory instantiation rather than
> here, otherwise we'll do a bunch more work than we need to with every
> checkconfig or reconfig.
> 
> I wouldn't use the phrase "TEMP CODE", either. I promise you this will live
> for much longer than you expect.

I will remove temp code. The reason I have this after factory and builder dict definitions is because sometimes other builders reuse other parts or reside under nested conditions that depend on each other (namely around the nightly, l10n, pgo stuff).

I chose to disrupt the least amount of existing code as possible and have a consistent way placement/use for `if not builder_tracker['foo']`. I realized this would waste time in check/re-config but I figured that's a minimal dent when compared to test master check/re-configs. (and I thought this would be temp code but I think you're showing me that I will be wrong).

If you wish, I can certainly optimize.

Comment 24

[Jordan Lund (:jlund)]

Attachment: 978307_support_mozharness_desktop_builders-bbotcustom-optimized-140318-2.diff

builder_tracker moved above factory/builder instantiation.

tested against test-masters and diff'd dump_master.py. yields same result.

Comment 25

[Jordan Lund (:jlund)]

Comment on attachment 8392410 [details] [diff] [review]
978307_support_mozharness_desktop_builders-bbot-cfgs-140317.diff

checked in to prevent bitrot: https://hg.mozilla.org/build/buildbot-configs/rev/0a4d5134bd56

Comment 26

[Jordan Lund (:jlund)]

Comment on attachment 8392450 [details] [diff] [review]
978307_support_mozharness_desktop_builders-bbotcustom-140317-2.diff

checked in to prevent bitrot. I will add the other patch (optimization) on top of this once r+: https://hg.mozilla.org/build/buildbotcustom/rev/ef74357cde62

Comment 27

[[e-mail] (:bhearsum)]

Comment on attachment 8393075 [details] [diff] [review]
978307_support_mozharness_desktop_builders-bbotcustom-optimized-140318-2.diff

Review of attachment 8393075 [details] [diff] [review]:
-----------------------------------------------------------------

Much better, thanks!

Comment 28

[Armen [:armenzg]]

Live in production.

Comment 29

[Jordan Lund (:jlund)]

Comment on attachment 8393075 [details] [diff] [review]
978307_support_mozharness_desktop_builders-bbotcustom-optimized-140318-2.diff

https://hg.mozilla.org/build/buildbotcustom/rev/013909d3207d

Comment 30

[Jordan Lund (:jlund)]

Attachment #8393075 [details] [diff] is now in production :)

Comment 31

[Jordan Lund (:jlund)]

thanks for your help ben! Marking as resolved for now.