Closed Bug 1324922 Opened 7 years ago Closed 7 years ago

Stop publishing complete MARs to balrog as a part of build/l10n repacks - final update

Categories

(Release Engineering :: General, defect)

Product:
Release Engineering
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: sfraser, Assigned: sfraser)

References

Details

Attachments

(8 files)

bug 1324922 stop balrog updates before funsize does its thing
58 bytes, text/x-review-board-request
catlee
: review+
Details
disable-balrog-updates.diff
3.07 KB, patch
Details | Diff | Splinter Review
disable-balrog-updates.diff
2.85 KB, patch
Details | Diff | Splinter Review
proposed-disable-balrog-updates.diff
4.02 KB, patch
Details | Diff | Splinter Review
balrog_props.diff
24.37 KB, patch
Details | Diff | Splinter Review
balrog_props.diff
27.89 KB, patch
Details | Diff | Splinter Review
balrog_props.diff
6.42 KB, patch
catlee
: review+
Details | Diff | Splinter Review
balrog_update_mod.diff
2.17 KB, patch
catlee
: review+
Details | Diff | Splinter Review 
Follow up to bug 1195365, final change to prevent balrog submissions before funsize does its partial update generation.
Assignee: nobody → sfraser
Blocks: 1195365
Simon, how does funsize announce a complete MAR file on Mozilla Pulse? I would like to know so we can add checks to our Firefox-UI update tests. Thanks.
I don't believe it does at the moment, it just does a submission to balrog. It'll be the same MAR file produced by the build and signing steps, though, just with the extra partial MARs alongside it.
Maybe Rail can help here. If we want to test the full mar patches, we have to be informed via Pulse.
Flags: needinfo?(rail)
Attachment #8820433 - Flags: review?(jlund)
I think there will be no change for funsize at this point. If we change the routes for the funsize tasks, we should let Henrik know.
Flags: needinfo?(rail)
Ok, so which Pulse route I have to look for to detect a full mar file for testing?
Comment on attachment 8820433 [details]
bug 1324922 stop balrog updates before funsize does its thing

https://reviewboard.mozilla.org/r/99918/#review102478

I'm going to r- for now just to show that I've reviewed the code. As always, I'm open to flipping it to an r+ if you disagree or point out something I missed.

::: testing/mozharness/mozharness/mozilla/building/buildbase.py:2121
(Diff revision 1)
>              props_path = os.path.join(env["UPLOAD_PATH"],
>                      'balrog_props.json')
>              self.generate_balrog_props(props_path)
>              return
>  
> +        if self.config.get('skip_balrog_uploads'):

patch looks good and should do what you want. However, the complexity is starting to increase in this method. e.g. this line must come after line 2114, the `if self.config.get('taskcluster_nightly')` block.

I think it would be great if we made line 2114 its own action and then we can simply uncomment 'update' in the list of actions of linux*

iow -

here (and the linux64 equiv): https://dxr.mozilla.org/mozilla-central/rev/cad2ea346d06ec5a3a70eda912513201dff0c21e/testing/mozharness/configs/builds/releng_base_linux_32_builds.py#22

replace with:

    'default_actions': [
        'clobber',
        'clone-tools',
        'checkout-sources',
        'setup-mock',
        'build',
        'upload-files',
        'sendchange',
        'check-test',
        'generate-build-stats',
        'generate-balrog-props',
        # 'update',
    ],
    
by commenting out update, the update() method will never be invoked at all. You will also have to define the action generate-balrog-props, as it is new, here: https://dxr.mozilla.org/mozilla-central/rev/cad2ea346d06ec5a3a70eda912513201dff0c21e/testing/mozharness/scripts/fx_desktop_build.py#48

finally, you can then remove this block: https://dxr.mozilla.org/mozilla-central/rev/cad2ea346d06ec5a3a70eda912513201dff0c21e/testing/mozharness/mozharness/mozilla/building/buildbase.py#2113-2119

and make it, along with the needed generate_build_props() call, its own method above `def update()` method definition:

e.g.:

```
def generate_balrog_props():
        # generate balrog props as artifacts for subsequent balrogscript task
        # in the taskcluster nightly case
        if self.config.get('taskcluster_nightly'):
        
            # grab any props available from this or previous unclobbered runs
            self.generate_build_props(console_output=False,
                                  halt_on_failure=False)


            env = self.query_mach_build_env(multiLocale=False)
            props_path = os.path.join(env["UPLOAD_PATH"],
                    'balrog_props.json')
            self.generate_balrog_props(props_path)
```

bear in mind, ^ is psuedo bug riddled code but hopefully the general idea makes sense. Do you agree? Questions? I can help if you decide to go this route.
Attachment #8820433 - Flags: review?(jlund) → review-
What effect will that have on thunderbird builds? It's my understanding it uses the same code path, so what would we need to enable to make sure those continue to work as expected?
(In reply to Simon Fraser [:sfraser] ⌚️GMT from comment #8)
> What effect will that have on thunderbird builds? It's my understanding it
> uses the same code path, so what would we need to enable to make sure those
> continue to work as expected?

thunderbird uses releng_base_linux_{64,32}_builds.py ?

whether you comment out `# update` or put `'skip_balrog_uploads': True` in releng_base_linux_{64,32}_builds.py, you will skip submitting to balrog for any platform that uses that file. All other platforms that do not use that config file will continue to function as normal no?
Ah, I'd missed where those were added. How about the attached diff?
Attachment #8824028 - Flags: review?(jlund)
Comment on attachment 8824028 [details] [diff] [review]
disable-balrog-updates.diff

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

looks great! thanks for re-patching. couple tiny nits below before I stamp

::: testing/mozharness/configs/builds/releng_base_linux_32_builds.py
@@ +19,5 @@
>          'sendchange',
>          'check-test',
>          'generate-build-stats',
> +        'generate-balrog-properties',
> +        # 'update',  # decided by query_is_nightly()

the `# decided by query_is_nightly()` part is no longer true in this case and can be removed

::: testing/mozharness/configs/builds/releng_base_linux_64_builds.py
@@ +18,5 @@
>          'sendchange',
>          'check-test',
>          'generate-build-stats',
> +        'generate-balrog-properties',
> +        # 'update',  # decided by query_is_nightly()

same here

::: testing/mozharness/mozharness/mozilla/building/buildbase.py
@@ +2105,5 @@
> +        # generate balrog props as artifacts
> +        if not self.config.get('taskcluster_nightly'):
> +            return
> +
> +        if self.config.get('forced_artifact_build'):

I don't think we will have a case where we end up with a taskcluster nightly build and it being a forced artifact build. given that, we can remove this condition

@@ +2117,5 @@
> +        env = self.query_mach_build_env(multiLocale=False)
> +        props_path = os.path.join(env["UPLOAD_PATH"],
> +                    'balrog_props.json')
> +        self.generate_balrog_props(props_path)
> +        return

I think the explicit return statement here is a no-op
changes made, new diff
After consideration of this patch it will cause problems for Android, as it will change the logic for their balrog updates in taskcluster nightlies. Therefore, holding on to this version of the patch for later use once we don't need to make that distinction between tc and bb any more.
Comment on attachment 8824028 [details] [diff] [review]
disable-balrog-updates.diff

clearing my r? because of comment 13

Did you get someone or get access to land original patch? Did you want to put original back up for r?
Attachment #8824028 - Flags: review?(jlund)
Last I knew I was waiting to see if you were available to do it. Do we need the original back up, if it's still in the history?
Still waiting on someone with access to land the original patch
Flags: needinfo?(jlund)
(In reply to Simon Fraser [:sfraser] ⌚️GMT from comment #16)
> Still waiting on someone with access to land the original patch

Ah sorry. I didn't realize you still don't have access. If you haven't done so, this is how you go about getting access: https://www.mozilla.org/en-US/about/governance/policies/commit/

You will need to ask for level 3, consent to policy, attach your pub ssh key, and you can use 2 releng folk (e.g. myself) to vouch in the bug.


In the meantime, I'll land the original patch on inbound which I believe is: https://reviewboard.mozilla.org/r/99918/diff/1#index_header and nightlies will take effect once it merges to central over the next 1-2 days.

I'm putting this bug as 'leave-open' so sheriffs know not to close it once patch is on central so we can follow up as per: comment 13
Flags: needinfo?(jlund)
Keywords: leave-open
Comment on attachment 8820433 [details]
bug 1324922 stop balrog updates before funsize does its thing

https://reviewboard.mozilla.org/r/99918/#review128198
Attachment #8820433 - Flags: review- → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/3b920caf08cec95491458a5d49ecd206ba1b0069
Bug 1324922 - Stop publishing complete MARs to balrog as a part of build/l10n repacks - final update, r=jlund
(In reply to Jordan Lund (:jlund) from comment #19)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/
> 3b920caf08cec95491458a5d49ecd206ba1b0069
> Bug 1324922 - Stop publishing complete MARs to balrog as a part of
> build/l10n repacks - final update, r=jlund

sfraser, we will want to watch first set of nightlies after this lands on central. I can help sanity check too
Flags: needinfo?(sfraser)
(In reply to Jordan Lund (:jlund) from comment #20)
> (In reply to Jordan Lund (:jlund) from comment #19)
> > https://hg.mozilla.org/integration/mozilla-inbound/rev/
> > 3b920caf08cec95491458a5d49ecd206ba1b0069
> > Bug 1324922 - Stop publishing complete MARs to balrog as a part of
> > build/l10n repacks - final update, r=jlund
> 
> sfraser, we will want to watch first set of nightlies after this lands on
> central. I can help sanity check too

actually, I'm just thinking... this patch is a no-op now right? both fennec and desktop nightlies use taskcluster so I think we will always hit this block for nightlies: https://dxr.mozilla.org/mozilla-central/source/testing/mozharness/mozharness/mozilla/building/buildbase.py#2122,2127

shall we back out and do comment 13 right away? I've lost a bit of context but iirc, this bug was about bbot nightlies originally.
Sorry, I'd missed this n-i.  Some of the lnie numbers have changed so here is an updated diff.
Flags: needinfo?(sfraser) → needinfo?(jlund)
Updated diff to cover Mac and Windows, too
Comment on attachment 8863796 [details] [diff] [review]
balrog_props.diff

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

::: testing/mozharness/configs/builds/releng_base_windows_32_builds.py
@@ +53,4 @@
>      'enable_talos_sendchange': True,
>      'enable_unittest_sendchange': True,
>      'max_build_output_timeout': 60 * 80,
> +    'skip_balrog_uploads': True, # If True, rely on Funsize to update Balrog

does this do anything now that we're not running the update action?

::: testing/mozharness/mozharness/mozilla/building/buildbase.py
@@ +1095,3 @@
>          elif c.get('src_mozconfig_manifest') and not c.get('src_mozconfig'):
>              self.info('Using mozconfig based on manifest contents')
> +            manifest = os.path.join(dirs['abs_work_dir'], c[

this is an odd place to split the line

@@ +2187,5 @@
> +        Wrapper around generate_balrog_props
> +        """
> +        # generate balrog props as artifacts
> +        if not self.config.get('taskcluster_nightly'):
> +            return

this would return False for Windows / OSX builds right now, so we won't end up generating any balrog properties, right?

are those needed for funsize?
(In reply to Chris AtLee [:catlee] from comment #25)
> Comment on attachment 8863796 [details] [diff] [review]
> balrog_props.diff
> 
> Review of attachment 8863796 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: testing/mozharness/configs/builds/releng_base_windows_32_builds.py
> @@ +53,4 @@
> >      'enable_talos_sendchange': True,
> >      'enable_unittest_sendchange': True,
> >      'max_build_output_timeout': 60 * 80,
> > +    'skip_balrog_uploads': True, # If True, rely on Funsize to update Balrog
> 
> does this do anything now that we're not running the update action?/

True, I'll strip it out. 

> 
> ::: testing/mozharness/mozharness/mozilla/building/buildbase.py
> @@ +1095,3 @@
> >          elif c.get('src_mozconfig_manifest') and not c.get('src_mozconfig'):
> >              self.info('Using mozconfig based on manifest contents')
> > +            manifest = os.path.join(dirs['abs_work_dir'], c[
> 
> this is an odd place to split the line

I can undo it, it was an autopep8-ism

> @@ +2187,5 @@
> > +        Wrapper around generate_balrog_props
> > +        """
> > +        # generate balrog props as artifacts
> > +        if not self.config.get('taskcluster_nightly'):
> > +            return
> 
> this would return False for Windows / OSX builds right now, so we won't end
> up generating any balrog properties, right?
> 
> are those needed for funsize?

It doesn't appear so, no, funsize generates the balrog properties from the manifest.
Updated to remove pep8 changes.
Attachment #8865811 - Flags: review+
Comment on attachment 8820433 [details]
bug 1324922 stop balrog updates before funsize does its thing

https://reviewboard.mozilla.org/r/99918/#review140646
Attachment #8820433 - Flags: review?(catlee) → review+
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 82005fcd5885 -d cc27ea7fc377: rebasing 394584:82005fcd5885 "bug 1324922 stop publishing complete MARs in BB, leave that to funsize. r=catlee,jlund" (tip)
merging testing/mozharness/configs/builds/releng_base_linux_32_builds.py
merging testing/mozharness/configs/builds/releng_base_linux_64_builds.py
merging testing/mozharness/configs/builds/releng_base_mac_64_builds.py
merging testing/mozharness/configs/builds/releng_base_mac_64_cross_builds.py
merging testing/mozharness/configs/builds/releng_base_windows_32_builds.py
merging testing/mozharness/configs/builds/releng_base_windows_64_builds.py
merging testing/mozharness/mozharness/mozilla/building/buildbase.py
merging testing/mozharness/scripts/fx_desktop_build.py
warning: conflicts while merging testing/mozharness/configs/builds/releng_base_linux_32_builds.py! (edit, then use 'hg resolve --mark')
warning: conflicts while merging testing/mozharness/configs/builds/releng_base_linux_64_builds.py! (edit, then use 'hg resolve --mark')
warning: conflicts while merging testing/mozharness/configs/builds/releng_base_mac_64_builds.py! (edit, then use 'hg resolve --mark')
warning: conflicts while merging testing/mozharness/configs/builds/releng_base_windows_32_builds.py! (edit, then use 'hg resolve --mark')
warning: conflicts while merging testing/mozharness/configs/builds/releng_base_windows_64_builds.py! (edit, then use 'hg resolve --mark')
warning: conflicts while merging testing/mozharness/mozharness/mozilla/building/buildbase.py! (edit, then use 'hg resolve --mark')
warning: conflicts while merging testing/mozharness/scripts/fx_desktop_build.py! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
https://hg.mozilla.org/integration/mozilla-inbound/rev/656d4714e8031d95fdf4d57b997215c1ced3364f
Bug 1324922: Stop publishing complete MARs to balrog as part of build/repack r=catlee
backed out from m-c seems this cause https://treeherder.mozilla.org/logviewer.html#?job_id=97974137&repo=mozilla-central&lineNumber=54325 on fennec nightlys
Flags: needinfo?(sfraser)
Backout by cbook@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/f8d40e7fe132
Backed out changeset 656d4714e803 for breaking fennec nightlys
Perhaps adding generate-balrog-properties to configs/builds/releng_base_android_64_builds.py ?
Flags: needinfo?(sfraser)
Flags: needinfo?(jlund)
After IRC discussions, see what just a modification of the update function looks like, to keep as much of the same logic as originally present. 

What would this break?
Attachment #8867147 - Flags: review?(catlee)
Attachment #8867147 - Flags: review?(catlee) → review+
Pushed by catlee@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b0b4e3c3f89d
stop balrog updates before funsize does its thing r=catlee,jlund
Sorry but ni because it's important for testing.

Does this mean that this bug will fix Bug 1349227

Currently we don't get enough time to test and report nightly as the current update is released late in our timezone,
so having 2 nightly's will coincide with our timezone and we can report and file bugs,
also if partial would be available for 8 days((2x8=16 builds)) that would be awesome too as if missed updates for days due to any reason still partial builds are smaller than full.

Thanks for the info, Cheers.
Flags: needinfo?(sfraser)
Flags: needinfo?(catlee)
(In reply to jigsawmode from comment #40)
> Sorry but ni because it's important for testing.
> 
> Does this mean that this bug will fix Bug 1349227

No, this just means that complete and partial updates will be available at the same time for nightly builds. It's not directly related to building multiple nightlies per day.
Flags: needinfo?(sfraser)
Flags: needinfo?(catlee)
(In reply to Chris AtLee [:catlee] from comment #41)
> (In reply to jigsawmode from comment #40)
> > Sorry but ni because it's important for testing.
> > 
> > Does this mean that this bug will fix Bug 1349227
> 
> No, this just means that complete and partial updates will be available at
> the same time for nightly builds. It's not directly related to building
> multiple nightlies per day.

Since Nightly is being built twice a day now and heard multiple users complain about getting full pushes twice daily,
why isn't this before enabling it?

Why is this not high priority or fixed before shipping 2 nightly's daily?
Flags: needinfo?(catlee)
I'm sorry that users are still hitting this issue. We decided that the benefits of having multiple nightlies outweighed the downside of some users sometimes getting complete updates.

We are actively working on this issue, and are hoping to have it resolved in the next few weeks.
Flags: needinfo?(catlee)
(In reply to Chris AtLee [:catlee] from comment #43)
> I'm sorry that users are still hitting this issue. We decided that the
> benefits of having multiple nightlies outweighed the downside of some users
> sometimes getting complete updates.
> 
> We are actively working on this issue, and are hoping to have it resolved in
> the next few weeks.


From today's channel meeting notes;
* Early merge plan for beta 57: Sep. 14. Will test beta 1 on dev ed population ~Sept 19

So will this be fixed by then?
Getting full push regularly
(In reply to shellye from comment #44)
> From today's channel meeting notes;
> * Early merge plan for beta 57: Sep. 14. Will test beta 1 on dev ed
> population ~Sept 19
This is unrelated. this note is about 57beta
 
> So will this be fixed by then?
> Getting full push regularly
Chris answered in comment #43.
Fixed as a result of https://bugzilla.mozilla.org/show_bug.cgi?id=1342392
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Removing leave-open keyword from resolved bugs, per :sylvestre.
Keywords: leave-open
Component: General Automation → General
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: