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

RESOLVED FIXED

Status

RESOLVED FIXED
2 years ago
5 months ago

People

(Reporter: sfraser, Assigned: sfraser)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(8 attachments)

(Assignee)

Description

2 years ago
Follow up to bug 1195365, final change to prevent balrog submissions before funsize does its partial update generation.
(Assignee)

Updated

2 years ago
Assignee: nobody → sfraser
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
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.
(Assignee)

Comment 3

2 years ago
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)
(Assignee)

Updated

2 years ago
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 7

2 years ago
mozreview-review
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-
(Assignee)

Comment 8

2 years ago
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?
(Assignee)

Comment 10

2 years ago
Created attachment 8824028 [details] [diff] [review]
disable-balrog-updates.diff

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
(Assignee)

Comment 12

2 years ago
Created attachment 8824031 [details] [diff] [review]
disable-balrog-updates.diff

changes made, new diff
(Assignee)

Comment 13

2 years ago
Created attachment 8824037 [details] [diff] [review]
proposed-disable-balrog-updates.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)
(Assignee)

Comment 15

2 years ago
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?
(Assignee)

Comment 16

2 years ago
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 18

2 years ago
mozreview-review
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.
(Assignee)

Comment 23

a year ago
Created attachment 8860323 [details] [diff] [review]
balrog_props.diff

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)
(Assignee)

Comment 24

a year ago
Created attachment 8863796 [details] [diff] [review]
balrog_props.diff

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?
(Assignee)

Comment 26

a year ago
(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.
(Assignee)

Comment 27

a year ago
Created attachment 8865811 [details] [diff] [review]
balrog_props.diff

Updated to remove pep8 changes.
Attachment #8865811 - Flags: review+
Comment hidden (mozreview-request)

Comment 29

a year ago
mozreview-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+

Comment 30

a year ago
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)

Comment 34

a year ago
Backout by cbook@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/f8d40e7fe132
Backed out changeset 656d4714e803 for breaking fennec nightlys
(Assignee)

Comment 35

a year ago
Perhaps adding generate-balrog-properties to configs/builds/releng_base_android_64_builds.py ?
Flags: needinfo?(sfraser)
(Assignee)

Updated

a year ago
Flags: needinfo?(jlund)
(Assignee)

Comment 36

a year ago
Created attachment 8867147 [details] [diff] [review]
balrog_update_mod.diff

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+
Comment hidden (mozreview-request)

Comment 38

a year ago
Pushed by catlee@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b0b4e3c3f89d
stop balrog updates before funsize does its thing r=catlee,jlund

Comment 40

a year ago
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)

Comment 42

a year ago
(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)

Comment 44

a year ago
(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.
(Assignee)

Comment 46

a year ago
Fixed as a result of https://bugzilla.mozilla.org/show_bug.cgi?id=1342392
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Removing leave-open keyword from resolved bugs, per :sylvestre.
Keywords: leave-open
Component: General Automation → General
Product: Release Engineering → Release Engineering
You need to log in before you can comment on or make changes to this bug.