Closed
Bug 1143910
Opened 10 years ago
Closed 10 years ago
Submit balrog data to multiple servers
Categories
(Release Engineering :: General, defect)
Release Engineering
General
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: rail, Assigned: rail)
References
Details
Attachments
(3 files, 3 obsolete files)
(deleted),
patch
|
jlund
:
review+
bhearsum
:
review+
rail
:
checked-in+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bhearsum
:
review+
rail
:
checked-in+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jlund
:
review+
rail
:
checked-in+
|
Details | Diff | Splinter Review |
It would be great to mirror some production data to dev balrog instance.
This patch changes the config to support multiple servers and gets rid of balrog_username in favour of balrog_usernames = {None: "default_username"}.
It also makes sure that some of the servers are mandatory (ignore_failures: False) and some not.
I haven't tested this yet.
Attachment #8578314 -
Flags: feedback?(jlund)
Attachment #8578314 -
Flags: feedback?(bhearsum)
Comment 1•10 years ago
|
||
Comment on attachment 8578314 [details] [diff] [review]
mirror_to_dev_balrog-mozharness.diff
Rail told me he has a newer version of this patch incoming....
Attachment #8578314 -
Attachment is obsolete: true
Attachment #8578314 -
Flags: feedback?(jlund)
Attachment #8578314 -
Flags: feedback?(bhearsum)
Assignee | ||
Comment 2•10 years ago
|
||
I tested this patch on Android nightly and L10N, Firefox Nightly. You can see the results here: http://dev-master2.bb.releng.use1.mozilla.com:8033/builders
No more balrog vars in build_pool_specifics.py. From what I found the only consumer was fx_desktop_build.py. Other scripts use balrog/*.py files. I added --config ... in the next patch.
Attachment #8578751 -
Flags: review?(jlund)
Attachment #8578751 -
Flags: review?(bhearsum)
Assignee | ||
Comment 3•10 years ago
|
||
Attachment #8578752 -
Flags: review?(jlund)
Attachment #8578752 -
Flags: review?(bhearsum)
Comment 4•10 years ago
|
||
Comment on attachment 8578751 [details] [diff] [review]
mirror_to_dev_balrog-mozharness.diff
Review of attachment 8578751 [details] [diff] [review]:
-----------------------------------------------------------------
Have well have you tested this? It looks fine to me, and I'm extremely happy to get rid of the duplicate balrog server information, but it looks very risky to land. Maybe pinned Mozharness makes it safer because you can test it out on a twig first?
::: mozharness/mozilla/updates/balrog.py
@@ +118,5 @@
>
> + server_args = [
> + "--api-root", server["balrog_api_root"],
> + "--username", self._query_balrog_username(server)
> + ]
Seeing this repeated 3 times has a smell. Do you think it's worth refactoring?
Attachment #8578751 -
Flags: review?(bhearsum) → review+
Comment 5•10 years ago
|
||
Comment on attachment 8578752 [details] [diff] [review]
balrog_configs-buildbot-configs.diff
Review of attachment 8578752 [details] [diff] [review]:
-----------------------------------------------------------------
Is this safe to land _without_ the Mozharness patch being present? Pinned Mozharness menas this will be active for some branches before the Mozharness changes take effect.
Attachment #8578752 -
Flags: review?(bhearsum) → review+
Assignee | ||
Comment 6•10 years ago
|
||
(In reply to Ben Hearsum [:bhearsum] from comment #4)
> Comment on attachment 8578751 [details] [diff] [review]
> mirror_to_dev_balrog-mozharness.diff
>
> Review of attachment 8578751 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> Have well have you tested this? It looks fine to me, and I'm extremely happy
> to get rid of the duplicate balrog server information, but it looks very
> risky to land. Maybe pinned Mozharness makes it safer because you can test
> it out on a twig first?
>
> ::: mozharness/mozilla/updates/balrog.py
> @@ +118,5 @@
> >
> > + server_args = [
> > + "--api-root", server["balrog_api_root"],
> > + "--username", self._query_balrog_username(server)
> > + ]
>
> Seeing this repeated 3 times has a smell. Do you think it's worth
> refactoring?
I thought about that, but then decided to leave it like this, because different scripts may have different arguments. It's a bit verbose, but explicit and easy to find/change the arguments.
Assignee | ||
Comment 7•10 years ago
|
||
(In reply to Ben Hearsum [:bhearsum] from comment #5)
> Comment on attachment 8578752 [details] [diff] [review]
> balrog_configs-buildbot-configs.diff
>
> Review of attachment 8578752 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> Is this safe to land _without_ the Mozharness patch being present? Pinned
> Mozharness menas this will be active for some branches before the Mozharness
> changes take effect.
Yes, I think they are safe to land before mozharness changes.
The plan is:
1) land the configs+mozharness, reconfig. IIRC, it will affect only Android builds.
2) Land an in-tree patch (TBD) to activate the change for desktop builds.
Assignee | ||
Comment 8•10 years ago
|
||
Attachment #8579285 -
Flags: review?(bhearsum)
Assignee | ||
Comment 9•10 years ago
|
||
Typoed
Attachment #8579285 -
Attachment is obsolete: true
Attachment #8579285 -
Flags: review?(bhearsum)
Attachment #8579287 -
Flags: review?(bhearsum)
Updated•10 years ago
|
Attachment #8579287 -
Flags: review?(bhearsum) → review+
Comment 10•10 years ago
|
||
Comment on attachment 8578751 [details] [diff] [review]
mirror_to_dev_balrog-mozharness.diff
Review of attachment 8578751 [details] [diff] [review]:
-----------------------------------------------------------------
looks good!
the `all(rc == 0 for rc in return_codes)` parts worries me as return_codes in mh when 0 (False) are expected to signify success while > 0 (True) signifies a failure. see in line comments for more details
r- for now. I'll r+ this patch if you explain that I am reading incorrectly
::: mozharness/mozilla/building/buildbase.py
@@ -1621,3 @@
> return
>
> - if c['balrog_api_root']:
wow, did I really check for balrog_api_root twice? :)
::: mozharness/mozilla/updates/balrog.py
@@ +62,5 @@
> + if server["ignore_failures"]:
> + self.info("Ignoring result, ignore_failures set to True")
> + else:
> + return_codes.append(return_code)
> + return all(rc == 0 for rc in return_codes)
so, iiuc, before this used to return an int and 0 (which equates to False) meant success. Now this returns a boolean and True equates to a success.
this will likely be a problem here:
http://mxr.mozilla.org/build/source/mozharness/mozharness/mozilla/building/buildbase.py#1624
Attachment #8578751 -
Flags: review?(jlund) → review-
Assignee | ||
Comment 11•10 years ago
|
||
Good catch! This should fix it. Interdiff: https://gist.github.com/rail/3de6b7987821852b5f10
Attachment #8578751 -
Attachment is obsolete: true
Attachment #8579697 -
Flags: review?(jlund)
Comment 12•10 years ago
|
||
Comment on attachment 8579697 [details] [diff] [review]
mirror_to_dev_balrog-mozharness-1.diff
Review of attachment 8579697 [details] [diff] [review]:
-----------------------------------------------------------------
nice. those built-in functions sure come in handy!
Attachment #8579697 -
Flags: review?(jlund) → review+
Comment 13•10 years ago
|
||
Comment on attachment 8578752 [details] [diff] [review]
balrog_configs-buildbot-configs.diff
Review of attachment 8578752 [details] [diff] [review]:
-----------------------------------------------------------------
looks good I think. If this lands and reconfigs before mozharness updates across all trees, I'm guessing this will just be a no-op for the desktop builds because build_pool_specifics.py will still have the balrog keys that configs/balrog/{staging,production}.py have.
the order that does matter is landing + reconfig buildbot patch before pushing out the mozharness patch.
Attachment #8578752 -
Flags: review?(jlund) → review+
Assignee | ||
Comment 14•10 years ago
|
||
Yeah, once the configs are landed we can proceed with other patches, including TBD in-tree pinning.
Assignee | ||
Comment 15•10 years ago
|
||
Comment on attachment 8578752 [details] [diff] [review]
balrog_configs-buildbot-configs.diff
https://hg.mozilla.org/build/buildbot-configs/rev/1d12367da5cb
Attachment #8578752 -
Flags: checked-in+
Assignee | ||
Comment 16•10 years ago
|
||
Comment on attachment 8579287 [details] [diff] [review]
puppet.diff
remote: View your changes here:
remote: https://hg.mozilla.org/build/puppet/rev/9ab40dc6cd57
remote: https://hg.mozilla.org/build/puppet/rev/0bf85d2de5ee
Attachment #8579287 -
Flags: checked-in+
Assignee | ||
Comment 17•10 years ago
|
||
Comment on attachment 8579697 [details] [diff] [review]
mirror_to_dev_balrog-mozharness-1.diff
https://hg.mozilla.org/build/mozharness/rev/ff37fa579a65
Attachment #8579697 -
Flags: checked-in+
Comment 18•10 years ago
|
||
Comment 19•10 years ago
|
||
mozharness production tag moved to: https://hg.mozilla.org/build/mozharness/rev/production
Assignee | ||
Comment 20•10 years ago
|
||
It worked fine for m-c en-US nighlty.
Assignee | ||
Comment 21•10 years ago
|
||
Not sure if I want to bother myself with nightly l10n repacks, because they will be switching to mozharness soon.
Assignee | ||
Updated•10 years ago
|
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Updated•6 years ago
|
Component: General Automation → General
You need to log in
before you can comment on or make changes to this bug.
Description
•