Closed Bug 1215204 Opened 9 years ago Closed 8 years ago

Add bouncer submitter to release promotion graph

Categories

(Release Engineering :: Release Automation: Other, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: rail, Assigned: kmoir)

References

Details

Attachments

(18 files, 20 obsolete files)

1.07 KB, patch
rail
: review+
Details | Diff | Splinter Review
520 bytes, patch
rail
: review+
Details | Diff | Splinter Review
759 bytes, patch
kmoir
: checked-in+
Details | Diff | Splinter Review
3.60 KB, patch
Details | Diff | Splinter Review
2.30 KB, patch
jlund
: review+
Details | Diff | Splinter Review
1.91 KB, patch
kmoir
: checked-in+
Details | Diff | Splinter Review
1.14 KB, patch
rail
: review+
kmoir
: checked-in+
Details | Diff | Splinter Review
1.09 KB, patch
rail
: review+
kmoir
: checked-in+
Details | Diff | Splinter Review
998 bytes, patch
rail
: review+
kmoir
: checked-in+
Details | Diff | Splinter Review
1.19 KB, patch
rail
: review+
kmoir
: checked-in+
Details | Diff | Splinter Review
1.28 KB, patch
rail
: review+
kmoir
: checked-in+
Details | Diff | Splinter Review
1.32 KB, patch
kmoir
: checked-in+
Details | Diff | Splinter Review
2.67 KB, patch
rail
: review+
kmoir
: checked-in+
Details | Diff | Splinter Review
750 bytes, patch
rail
: review+
kmoir
: checked-in+
Details | Diff | Splinter Review
1.08 KB, patch
Callek
: review+
kmoir
: checked-in+
Details | Diff | Splinter Review
11.61 KB, patch
Callek
: review+
kmoir
: checked-in+
Details | Diff | Splinter Review
47 bytes, text/x-github-pull-request
kmoir
: checked-in+
Details | Review
4.05 KB, patch
rail
: review+
kmoir
: checked-in+
Details | Diff | Splinter Review
The builder definition may need some changes to not require config time variables, like version number, build number etc.
Assignee: nobody → kmoir
A few questions

So we are just going to replace the existing builders here
http://hg.mozilla.org/build/buildbotcustom/file/664d99eb01d6/process/release.py#l1709
with jobs that can go through the buildbot bridge

The changes to not require require config time variables, like version number, build number etc. are because these are provided in releasetasks?
Flags: needinfo?(rail)
(In reply to Kim Moir [:kmoir] from comment #1)
> A few questions
> 
> So we are just going to replace the existing builders here
> http://hg.mozilla.org/build/buildbotcustom/file/664d99eb01d6/process/release.
> py#l1709
> with jobs that can go through the buildbot bridge

At this stage we are not replacing the builder yet. You need to add a new builder with a different name (including the branch name in the builder name should solve the problem) to generateReleasePromotionBuilders(), see http://hg.mozilla.org/build/buildbotcustom/file/664d99eb01d6/process/release.py#l2079.
 
> The changes to not require require config time variables, like version
> number, build number etc. are because these are provided in releasetasks?

Correct. These variables will be set in the TC task generated by releasetasks. The only way to pass some variables via BBB runtime is using properties, similar to https://github.com/mozilla/releasetasks/blob/master/releasetasks/templates/l10n.yml.tmpl#L28-L36

You may need to change the script to read those properties, similar to https://dxr.mozilla.org/mozilla-central/source/testing/mozharness/scripts/desktop_l10n.py#335 and later access them similar to https://dxr.mozilla.org/mozilla-central/source/testing/mozharness/scripts/desktop_l10n.py#336
Flags: needinfo?(rail)
Attached patch bug1215204bbcustom.patch (obsolete) — Splinter Review
Attached patch bug1215204mh.patch (obsolete) — Splinter Review
Attached patch bug1215204templates.patch (obsolete) — Splinter Review
This is a draft of the templates file, there is still some content to be added
Comment on attachment 8689047 [details] [diff] [review]
bug1215204bbcustom.patch

Rail, I forgot to mention that we discussed yesterday that the properties that are defined in release tasks could be removed but I tried that and then the checkconfig failed.  So I left them in so they could be overwritten
Attachment #8689046 - Flags: review?(rail)
Attachment #8689046 - Flags: review?(rail) → review+
Comment on attachment 8689047 [details] [diff] [review]
bug1215204bbcustom.patch

Not sure that this has everything but I need to test it and it is difficult to do so without landing and testing
Attachment #8689047 - Flags: feedback+
Attachment #8689047 - Flags: feedback+ → feedback?(rail)
Attachment #8689050 - Flags: feedback?(rail)
Attached patch bug1215204templates.patch (obsolete) — Splinter Review
Attachment #8689058 - Attachment is obsolete: true
Comment on attachment 8689047 [details] [diff] [review]
bug1215204bbcustom.patch

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

In overall you are in the proper direction, just need to address 2 major things:

1) don't iterate over platforms, the script has to be run once.
2) clean up unused properties

::: process/release.py
@@ +1853,5 @@
>              },
>          }
>          builders.append(l10n_builder)
>  
> +    for platform in config["release_platforms"]:

I don't think you need to iterate over all platforms. The script should handle all of them in a single run. Actually, it won't add new platforms after the first run, because ti tries to be idempotent and checks if the product already exists and if it does, the script skips it.

So there should be a single call of this script, not per platform.

@@ +1857,5 @@
> +    for platform in config["release_platforms"]:
> +        bouncer_mh_cfg = {
> +            "script_name": "scripts/bouncer_submitter.py",
> +            "extra_args": [
> +                 "-c",  "releases/bouncer_%s_release.py" % pf['product_name'],

this won't work for beta, for example. 
I'd move bouncer_submitter_config (https://hg.mozilla.org/build/buildbot-configs/file/tip/mozilla/release-fennec-mozilla-release.py.template#l178) to branch config, like you did in https://hg.mozilla.org/build/buildbot-configs/rev/93d843ea9c08

@@ +1876,5 @@
> +                                                  )
> +      
> +        bouncer_builder = {
> +            "name": bouncer_buildername, 
> +            "slavenames": pf["slaves"],

this would run windows submission on windows slave, but there is no reason to do that. Once you get rid of the "for platform ..." loop, you can use linux+linux64 slaves for this job.

@@ +1877,5 @@
> +      
> +        bouncer_builder = {
> +            "name": bouncer_buildername, 
> +            "slavenames": pf["slaves"],
> +            'category': bouncer_buildername, 

this should be builderPrefix(''), it's the same for all builder, so we can group them

@@ +1883,5 @@
> +            'slavebuilddir': normalizeName(bouncer_buildername),
> +            'factory': bouncer_submitter_factory,
> +            #'env': builder_env
> +            'properties': {
> +                'slavebuilddir': normalizeName(bouncer_buildername), 

I don't think you need "slavebuilddir" in properties.

@@ +1884,5 @@
> +            'factory': bouncer_submitter_factory,
> +            #'env': builder_env
> +            'properties': {
> +                'slavebuilddir': normalizeName(bouncer_buildername), 
> +                'balrog_api_root': config['balrog_api_root'],

I don't think this script interacts with balrog, does it?

@@ +1886,5 @@
> +            'properties': {
> +                'slavebuilddir': normalizeName(bouncer_buildername), 
> +                'balrog_api_root': config['balrog_api_root'],
> +                'bouncerServer': config['bouncerServer'],
> +                'tuxedoServerUrl':  config['tuxedoServerUrl'],

it's already passed in extra_args, no need to pass it again.

@@ +1887,5 @@
> +                'slavebuilddir': normalizeName(bouncer_buildername), 
> +                'balrog_api_root': config['balrog_api_root'],
> +                'bouncerServer': config['bouncerServer'],
> +                'tuxedoServerUrl':  config['tuxedoServerUrl'],
> +                'bouncerServer': config['bouncerServer'], 

This is passed twice. I don't think you need this.

@@ +1892,5 @@
> +                 #restructure it or use json.dumps (see l10n script)
> +                 'bouncer_aliases': {
> +                     'Firefox-%(version)s': 'firefox-latest',
> +                     'Firefox-%(version)s-stub': 'firefox-stub',
> +                 },

This won't work, because properties have to be strings. I don't think it is used by the script, so you probably don't need this.

@@ +1894,5 @@
> +                     'Firefox-%(version)s': 'firefox-latest',
> +                     'Firefox-%(version)s-stub': 'firefox-stub',
> +                 },
> +                 #this should be in releasetasks
> +                 'platform': platform,

should be platform=None to make buildbot happy.

@@ +1896,5 @@
> +                 },
> +                 #this should be in releasetasks
> +                 'platform': platform,
> +                 'product': pf['product_name'],
> +                 'branch': 'releases/date',

shouldn't be hardcoded.

@@ +1897,5 @@
> +                 #this should be in releasetasks
> +                 'platform': platform,
> +                 'product': pf['product_name'],
> +                 'branch': 'releases/date',
> +                 'bouncer_submitter_config': "releases/bouncer_%s_release.py" % pf['product_name'],                  

this is already in extra_args.
Attachment #8689047 - Flags: feedback?(rail) → feedback+
Comment on attachment 8689050 [details] [diff] [review]
bug1215204mh.patch

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

You also need to override --previous-version with multiple values. The values should be passed from shipt-it via releasetasks. something similar to https://github.com/mozilla/releasetasks/blob/master/releasetasks/templates/funsize.yml.tmpl#L1

Something like

...
  task:
...
    payload:
...
     properties:
...
       - partial_versions: {% for partial_version, partial_info in partial_updates.iteritems() %}{{ partial_version }}{% endfor %}
...

and then something like

self.config["prev_versions"] = self.buildbot_config["properties"].get("partial_versions").split()

::: testing/mozharness/scripts/bouncer_submitter.py
@@ +72,5 @@
> +        #override properties from buildbot properties here as defined by taskcluster properties
> +        self.read_buildbot_config()
> +
> +        #check if release promotion is true first before overwriting these properties
> +        #? do I need to define buildbot_json_path

I don't think you need to define it.

@@ +76,5 @@
> +        #? do I need to define buildbot_json_path
> +        if self.buildbot_config["properties"].get("release_promotion"):
> +            for prop in ['product', 'version', 'build_number', 'revision', 'bouncer_submitter_config']:
> +                if self.buildbot_config["properties"].get(prop):
> +                    self.info("Overriding en_us_binary_url with %s" %

the log message is wrong.
Attachment #8689050 - Flags: feedback?(rail) → feedback+
Attached patch bug1215204bbcustom2.patch (obsolete) — Splinter Review
I couldn't get 'category': builderPrefix('') to be in scope and work
Attachment #8689047 - Attachment is obsolete: true
Attached patch bug1215204mh2.patch (obsolete) — Splinter Review
Attachment #8689050 - Attachment is obsolete: true
Attached patch bug1215204mh3.patch (obsolete) — Splinter Review
Attachment #8689210 - Attachment is obsolete: true
Attached patch bug1215204templates2.patch (obsolete) — Splinter Review
Attachment #8689132 - Attachment is obsolete: true
Attachment #8689549 - Flags: review?(rail)
Comment on attachment 8689200 [details] [diff] [review]
bug1215204bbcustom2.patch

I couldn't get 'category': builderPrefix('') to be in scope and work, not sure how to do this part (re your last review comments)
Attachment #8689200 - Flags: feedback?(rail)
Attachment #8689555 - Flags: feedback?(rail)
Attachment #8689564 - Flags: feedback?(rail)
Comment on attachment 8689200 [details] [diff] [review]
bug1215204bbcustom2.patch

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

Almost there! :)

::: process/release.py
@@ +1875,5 @@
> +      
> +    bouncer_builder = {
> +                      "name": bouncer_buildername, 
> +                      "slavenames": config["platforms"]["linux"]["slaves"] + config["platforms"]["linux64"]["slaves"],
> +                      'category': name,

category has to be builderPrefix('')

@@ +1883,5 @@
> +                      #'env': builder_env,
> +                      'properties': {
> +                                     'bouncerServer': config['bouncerServer'],
> +                                     'tuxedoServerUrl':  config['tuxedoServerUrl'],
> +                                     'bouncerServer': config['bouncerServer'], 

I don't think you need these 3 (actually 2, one duplicate) properties. The only valuable one (tuxedoServerUrl) is already passed via extra_args as --bouncer-api-prefix

@@ +1884,5 @@
> +                      'properties': {
> +                                     'bouncerServer': config['bouncerServer'],
> +                                     'tuxedoServerUrl':  config['tuxedoServerUrl'],
> +                                     'bouncerServer': config['bouncerServer'], 
> +                      #these should be in releasetasks

You can remove this comment, the following 3 properties are required by log_upload.py
Attachment #8689200 - Flags: feedback?(rail) → feedback+
Comment on attachment 8689549 [details] [diff] [review]
bug1215204bbconfigs2.patch

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

::: mozilla/project_branches.py
@@ +197,3 @@
>          'bouncerServer': 'download.mozilla.org',
>          'tuxedoServerUrl':  'https://bounceradmin.mozilla.com/api',
>          'bouncerServer': 'download.mozilla.org',

Can you remove bouncerServer entries when you land this? I don't think we use them yet.
Attachment #8689549 - Flags: review?(rail) → review+
Comment on attachment 8689555 [details] [diff] [review]
bug1215204mh3.patch

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

Feel free to land it on date to test.

::: testing/mozharness/scripts/bouncer_submitter.py
@@ +78,5 @@
> +                if self.buildbot_config["properties"].get(prop):
> +                    self.info("Overriding %s with %s" %
> +                              prop, self.buildbot_config["properties"][prop])
> +                    self.config[prop] = self.buildbot_config["properties"][prop]
> +            if self.buildbot_config["properties".get("partial_versions")]:

should be
 if self.buildbot_config["properties"].get("partial_versions"):
Attachment #8689555 - Flags: feedback?(rail) → feedback+
Comment on attachment 8689564 [details] [diff] [review]
bug1215204templates2.patch

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

BTW, it would be great to add unit tests tests and you can use github PRs for these patches so you get tests running automatically on travis.

::: releasetasks/templates/bouncer.yml.tmpl
@@ +1,1 @@
> +{% for platform, platform_info in bouncer_config["platforms"].iteritems() %}

It doesn't match the buildername(s) in buildbot. Don't need to iterate over platforms.

@@ +1,5 @@
> +{% for platform, platform_info in bouncer_config["platforms"].iteritems() %}
> +# TODO: make a helper function to generate consistent builder names?
> +{% set buildername = "release-{}_{}_{}__bncr_sub".format(branch, product, platform) %}
> +-
> +    taskId: "{{ stableSlugId('{}'.format(buildername)) }}"

taskId: "{{ stableSlugId(buildername) }}"

looks simpler ;)
Attachment #8689564 - Flags: feedback?(rail) → feedback+
fixed project branches patch
Attachment #8689717 - Flags: checked-in+
Attached patch bug1215204templates3.patch (obsolete) — Splinter Review
Attachment #8689564 - Attachment is obsolete: true
Attachment #8689555 - Attachment is obsolete: true
Attached patch bug1215204bbcustom3.patch (obsolete) — Splinter Review
In comment 19, rail suggested that category has to be builderPrefix('').  However, builderPrefix is not in scope for 
generateReleasePromotionBuilders since it is defined within generateReleaseBranchObjects.  So I just used the bit that applied to generateReleasePromotionBuilders for the category
Attachment #8689200 - Attachment is obsolete: true
Attachment #8691534 - Flags: review?(jlund)
Attachment #8690093 - Flags: review?(jlund)
Comment on attachment 8691534 [details] [diff] [review]
bug1215204bbcustom3.patch

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

looks good to me. I didn't cross examine line by line against the 'old way' but I suspect we can iterate on this if we need to :)

tiny nit: there are about 5 spots where bugzilla's splinter view picked up whitespaces.

::: process/release.py
@@ +1854,5 @@
>          builders.append(l10n_builder)
> +  
> +    bouncer_mh_cfg = {
> +        "script_name": "scripts/bouncer_submitter.py",
> +        "extra_args": [

so args like --build-num, --version, and --revision no longer required?

@@ +1873,5 @@
> +                                             )
> +      
> +    bouncer_builder = {
> +                      "name": bouncer_buildername, 
> +                      "slavenames": config["platforms"]["linux"]["slaves"] + config["platforms"]["linux64"]["slaves"],

oh neat, I didn't realize we could use linux32 or 64 for this.

@@ +1877,5 @@
> +                      "slavenames": config["platforms"]["linux"]["slaves"] + config["platforms"]["linux64"]["slaves"],
> +                      "builddir": bouncer_buildername,
> +                      "slavebuilddir": normalizeName(bouncer_buildername),
> +                      "factory": bouncer_submitter_factory,
> +                      "category": "release-%s-%s" % (config["bouncer_branch"], ''),

category ends with '-'?
Attachment #8691534 - Flags: review?(jlund) → review+
Comment on attachment 8690093 [details] [diff] [review]
bug1215204templates3.patch

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

awesome. please bear with me as I get up to speed with you.

setting to r- for now. as always, it can be persuaded to an r+ if I am wrong :)

::: releasetasks/templates/bouncer.yml.tmpl
@@ +1,2 @@
> +# TODO: make a helper function to generate consistent builder names?
> +{% set buildername = "release-{}_{}__bncr_sub".format(branch, product) %}

there is two underscores btween product and bncr. iiuc, this has to match the buildername you defined in bbot-cfg's release.py

@@ +23,5 @@
> +                product: "{{ product }}"
> +                # Quotes cannot be used around this string because the loop causes it to have trailing whitespace
> +                # (which gets stripped by the yaml parser when unquoted). Kindof hacky.
> +                version: {{ version }}
> +                build_number: {{ buildNumber }}

ah right, we moved build number, revision, partials, and version here because they are release specific. ignore my comment in other attachment

@@ +26,5 @@
> +                version: {{ version }}
> +                build_number: {{ buildNumber }}
> +                release_promotion: true
> +                revision: "{{ revision }}"
> +                partial_versions: {% for partial_version, partial_info in partial_updates.iteritems() %}{{ partial_version }}{% endfor %}

I'm a jinja noob. It looks like the output of this will be a string of partials concatenated together without a delim. Am I reading that right?

@@ +33,5 @@
> +            description: "Release Promotion bouncer submission job"
> +            owner: "release@mozilla.com"
> +            source: https://github.com/mozilla/releasetasks
> +
> +        {% if running_tests is defined %}

do we have tests for this?

@@ +39,5 @@
> +            task_name: "{{ buildername }}"
> +        {% endif %}
> +
> +{% endfor %}
> +{% endfor %}

hrm, iiuc, these two endfors are not closing any loops
Attachment #8690093 - Flags: review?(jlund) → review-
Attached patch bug1215204templates4.patch (obsolete) — Splinter Review
Re review comments from comment 28

I don't have tests yet, I have to add them


I'm a jinja noob too.  
For this part
 partial_versions: {% for partial_version, partial_info in partial_updates.iteritems() %}{{ partial_version }}{

I just copied it from the funsize template
Attachment #8690093 - Attachment is obsolete: true
Attachment #8692017 - Flags: review?(jlund)
Comment on attachment 8692017 [details] [diff] [review]
bug1215204templates4.patch

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

again, I'm learning as I go so please bear with me if I end up wrong here...

::: releasetasks/templates/bouncer.yml.tmpl
@@ +1,3 @@
> +# TODO: make a helper function to generate consistent builder names?
> +{% set buildername = "release-{}_{}_bncr_sub".format(branch, product) %}
> +{% for partial_version, partial_info in partial_updates.iteritems() %}

hm, so what are we trying to do here? this will create a task for each partial iiuc and each task would trigger the same buildbot builder with the same properties no? Don't we just want to create one bouncer submitter task?

@@ +27,5 @@
> +                version: {{ version }}
> +                build_number: {{ buildNumber }}
> +                release_promotion: true
> +                revision: "{{ revision }}"
> +                partial_versions: {% for partial_version, partial_info in partial_updates.iteritems() %}{{ partial_version }}{% endfor %}

this is the same look as the one at the top.

iiuc you are trying to account for: https://dxr.mozilla.org/build-central/source/buildbotcustom/process/release.py#1444 by passing the previous partial versions via buildbot properties.

But what I still think we need to do is have separator between partial versions so we don't end up with something like: partial_versions: "44.0b144.0b341.b3'

maybe we should try something like:
code: https://github.com/mozilla/releasetasks/blob/master/releasetasks/templates/l10n.yml.tmpl#L33
output: https://queue.taskcluster.net/v1/task/Fyw6nQxMSNGQxSSfddXqAQ
Attachment #8692017 - Flags: review?(jlund) → review-
Sorry for the many review requests. I find this hard to write because I can't test it first, which I usually do so I'm kind of flying blind.  I added the formatting for the partial_versions
Attachment #8692017 - Attachment is obsolete: true
Attachment #8692578 - Flags: review?(jlund)
Comment on attachment 8692578 [details] [diff] [review]
bug1215204templates5.patch

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

I hear yeah. This looks great. We can iterate on this once we see what it's like on date :)
Attachment #8692578 - Flags: review?(jlund) → review+
Attachment #8691534 - Attachment is obsolete: true
Attachment #8692997 - Flags: checked-in+
Comment on attachment 8692997 [details] [diff] [review]
bug1215204bbcustom3nowhitespace.patch

in production
Have a pull request that passes
https://github.com/mozilla/releasetasks/pull/56

Not sure if I should merge as it seems I can


kmoir	I issued a pull request for my patches for the bouncer submitter but it failed https://github.com/mozilla/releasetasks/pull/54/files
	kmoir	https://tools.taskcluster.net/task-graph-inspector/#VYZ_aV80Suqlh6ERzQIQFg/HoaNq5VESVuxh1w64jfijA
	kmoir	not sure how to debug what went wrong
	jlund	kmoir: so I think everytime we call https://github.com/mozilla/releasetasks/blob/master/releasetasks/templates/release_graph.yml.tmpl
	jlund	via https://github.com/mozilla/releasetasks/blob/77dc076c4c8868eb0ca159f3b5b0bba4d4d9cd81/releasetasks/__init__.py#L17
	jlund	in all our tests (and of course in releaserunner), we need to add a 'bouncer_enabled' arg
	jlund	e.g. https://github.com/mozilla/releasetasks/blob/master/releasetasks/test/test_releasetasks.py#L62
	kmoir	jlund: thanks!
	jlund	kmoir: can you see the log and the failures in the taskcluster task url you pasted above?
	jlund	it's in the 'Run 0' tab
Depends on: 1230227
So I'm trying to run this in release runner and it is failing to create the task graph

https://index.taskcluster.net/v1/task/buildbot.revisions.c5283bf0194859ba1f0a3e3b0c2d889456b625e4.date.linux

Not sure why, going to ask rail about it next week at our all hands
Looks like you need to add "bouncer_enabled": branchConfig[something_that_enables_bouncer] to kwargs in 
https://hg.mozilla.org/build/tools/file/tip/buildfarm/release/release-runner.py#l267 which is passed to make_task_graph() in https://hg.mozilla.org/build/tools/file/tip/buildfarm/release/release-runner.py#l301
Kim, can you also add releasetasks unittests for this feature.
Attachment #8697357 - Flags: review?(rail)
Comment on attachment 8697357 [details] [diff] [review]
bug1215204tools.patch

Hmm, I don't see this variable name anywhere in the configs. Maybe you forgot to add it to project_branches.py?
Attachment #8697357 - Flags: review?(rail) → review-
Attached patch bug1215204date.patch (obsolete) — Splinter Review
Attachment #8697378 - Flags: review?(rail)
Attachment #8697378 - Flags: review?(rail)
Attachment #8697378 - Attachment is obsolete: true
Attachment #8697382 - Flags: review?(rail)
Comment on attachment 8697382 [details] [diff] [review]
bug1215204date.patch

conditional r+: please remove surrounding quotes, it should be True.
Attachment #8697382 - Flags: review?(rail) → review+
Comment on attachment 8697357 [details] [diff] [review]
bug1215204tools.patch

asking for review again since I updated project_branches.py
Attachment #8697357 - Flags: review- → review?
Attachment #8697357 - Flags: review? → review+
Attachment #8697357 - Flags: checked-in+
So releaserunner is failing like this

Traceback (most recent call last):
  File "release-runner.py", line 302, in main
    graph = make_task_graph(**kwargs)
  File "/home/cltbld/releasetasks/releasetasks/__init__.py", line 50, in make_task_graph
    return yaml.safe_load(template.render(**template_vars))
  File "/builds/releaserunner/lib/python2.7/site-packages/jinja2/environment.py", line 894, in render
    return self.environment.handle_exception(exc_info, True)
  File "/home/cltbld/releasetasks/releasetasks/templates/release_graph.yml.tmpl", line 69, in top-level template code
    {{ bouncer_tasks()|indent(4) }}
  File "/home/cltbld/releasetasks/releasetasks/templates/release_graph.yml.tmpl", line 68, in template
    {% macro bouncer_tasks() %}{% include "bouncer.yml.tmpl" %}{% endmacro %}
  File "/home/cltbld/releasetasks/releasetasks/templates/bouncer.yml.tmpl", line 30, in top-level template code
    partial_versions: {% for p in partial_version %}{{ "{}, ".format(p) }}{% endfor %}
UndefinedError: 'partial_version' is undefined

because partial_versions is not defined properly in the template

I changed it to 
partial_versions: {% for p in partial_updates.iteritems() %}{{ "{}, ".format(p) }}{% endfor %}

https://github.com/kmoir/releasetasks/commit/20e8472b63447a4c6051ecb2d273ae1875b44bde

but the task graph is failing by travis, not sure if this is because there are previous commits from other people that are causing it to fail. If you have suggestions on how to fix this it would be appreciated
Flags: needinfo?(rail)
so problem is that product is undefined for task
Flags: needinfo?(rail)
I believe you need to pass more variables to the call, something like in https://github.com/rail/releasetasks/blob/master/releasetasks/test/test_releasetasks.py#L174
Attached patch bug1215204bouncerenabled.patch (obsolete) — Splinter Review
missing bouncer_enabled in process/release.py
Attachment #8700741 - Flags: review?(rail)
Comment on attachment 8700741 [details] [diff] [review]
bug1215204bouncerenabled.patch

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

::: process/release.py
@@ +1882,5 @@
>                        "properties": {
>                            "platform": None,
>                            "product": pf["product_name"],
> +                          "branch": config["bouncer_branch"],
> +                          "branch": config["bouncer_enabled"]

Hmm, forgot to fix the first part? "branch" is set twice here. Probably should be "bouncer_enabled"?
Attachment #8700741 - Flags: review?(rail) → review-
Attachment #8700741 - Attachment is obsolete: true
Attachment #8701027 - Flags: review?(rail)
Attachment #8701027 - Flags: review?(rail) → review+
Attachment #8701027 - Flags: checked-in+
current status

job is being scheduled but has error
kmoir> now the error is that it  "Failed to infer task payload format"
4:53 PM <kmoir> https://tools.taskcluster.net/task-graph-inspector/#Q02OdIodR8CtxK9--UX4gQ/oY1Aif18RWmroandG-OFnQ/
4:54 PM <kmoir> also noticing the scope is buildbot-bridge:builder-name:release-date_firefox_bncr_sub
4:54 PM <kmoir> not sure if I have to change something there
(In reply to Kim Moir [:kmoir] from comment #52)
> current status
> 
> job is being scheduled but has error
> kmoir> now the error is that it  "Failed to infer task payload format"
> 4:53 PM <kmoir>
> https://tools.taskcluster.net/task-graph-inspector/#Q02OdIodR8CtxK9--UX4gQ/
> oY1Aif18RWmroandG-OFnQ/

It looks like buildbot-bridge is broken now, this not the only task with exception. I see l10n repacks orange, even though they worked fine in the past.

I tried to grep logs, but they are too brief:

[root@buildbot-master82.bb.releng.scl3.mozilla.com bbb]# grep -C3  oY1Aif18RWmroandG-OFnQ *.log                                                                
tclistener.log-2015-12-22 13:37:21,713 - bbb.servicebase - Fetching task -Qk6iqXcRPODENVayEKlqg
tclistener.log-2015-12-22 13:37:21,720 - requests.packages.urllib3.connectionpool - Starting new HTTPS connection (1): queue.taskcluster.net
tclistener.log-2015-12-22 13:37:22,253 - bbb.servicebase - Fetching task 4B71MPC4TBqjGyqWi5thNQ
tclistener.log:2015-12-22 13:38:29,822 - bbb.services - Handling Taskcluster exception (canceled) for oY1Aif18RWmroandG-OFnQ
tclistener.log:2015-12-22 13:38:29,822 - bbb.servicebase - Fetching task oY1Aif18RWmroandG-OFnQ
tclistener.log-2015-12-22 13:38:29,836 - bbb.services - No task found in our database, nothing to do.

> 4:54 PM <kmoir> also noticing the scope is
> buildbot-bridge:builder-name:release-date_firefox_bncr_sub

The scope LGTM, compared with other jobs.
It turns out that the job fails in buildbot, see http://buildbot-master72.bb.releng.usw2.mozilla.com:8001/builders/release-date_firefox_bncr_sub/builds/0

Traceback (most recent call last):
  File "/builds/buildbot/build1/lib/python2.7/site-packages/buildbot-0.8.2_hg_41f0fbee10f4_production_0.8-py2.7.egg/buildbot/process/buildstep.py", line 728, in startStep
    d.addCallback(self._startStep_2)
  File "/builds/buildbot/build1/lib/python2.7/site-packages/twisted/internet/defer.py", line 260, in addCallback
    callbackKeywords=kw)
  File "/builds/buildbot/build1/lib/python2.7/site-packages/twisted/internet/defer.py", line 249, in addCallbacks
    self._runCallbacks()
  File "/builds/buildbot/build1/lib/python2.7/site-packages/twisted/internet/defer.py", line 441, in _runCallbacks
    self.result = callback(self.result, *args, **kw)
--- <exception caught here> ---
  File "/builds/buildbot/build1/lib/python2.7/site-packages/buildbot-0.8.2_hg_41f0fbee10f4_production_0.8-py2.7.egg/buildbot/process/buildstep.py", line 769, in _startStep_2
    skip = self.start()
  File "/builds/buildbot/build1/lib/python2.7/site-packages/buildbot-0.8.2_hg_41f0fbee10f4_production_0.8-py2.7.egg/buildbot/steps/shell.py", line 212, in start
    command = properties.render(self.command)
  File "/builds/buildbot/build1/lib/python2.7/site-packages/buildbot-0.8.2_hg_41f0fbee10f4_production_0.8-py2.7.egg/buildbot/process/properties.py", line 108, in render
    return [ self.render(e) for e in value ]
  File "/builds/buildbot/build1/lib/python2.7/site-packages/buildbot-0.8.2_hg_41f0fbee10f4_production_0.8-py2.7.egg/buildbot/process/properties.py", line 106, in render
    return value.render(self.pmap)
  File "/builds/buildbot/build1/lib/python2.7/site-packages/buildbot-0.8.2_hg_41f0fbee10f4_production_0.8-py2.7.egg/buildbot/process/properties.py", line 194, in render
    s = self.fmtstring % pmap
  File "/builds/buildbot/build1/lib/python2.7/site-packages/buildbot-0.8.2_hg_41f0fbee10f4_production_0.8-py2.7.egg/buildbot/process/properties.py", line 169, in __getitem__
    rv = properties[key]
  File "/builds/buildbot/build1/lib/python2.7/site-packages/buildbot-0.8.2_hg_41f0fbee10f4_production_0.8-py2.7.egg/buildbot/process/properties.py", line 50, in __getitem__
    rv = self.properties[name][0]
exceptions.KeyError: 'repo_path'
patch to add missing repo_path as shown in the error here
http://buildbot-master70.bb.releng.use1.mozilla.com:8001/builders/release-date_firefox_bncr_sub/builds/2/steps/download_and_extract_scripts_archive/logs/err.html

also, add script_repo_revision and change the order of the properties to be the same as the l10n builder
Attachment #8703700 - Flags: review?(rail)
Comment on attachment 8703700 [details] [diff] [review]
bug1215204fixproperties.patch

lgtm!
Attachment #8703700 - Flags: review?(rail) → review+
Attachment #8703700 - Flags: checked-in+
Attached patch bug1215204bouncer.patch (obsolete) — Splinter Review
Attachment #8703787 - Attachment is obsolete: true
Attachment #8703787 - Flags: review?(rail)
Attachment #8703794 - Flags: review?(rail)
Comment on attachment 8703794 [details] [diff] [review]
bug1215204bouncer.patch

I think we wanted to have a separate config for date to avoid collisions with production submission or (better) to use staging/dev bouncer like in https://dxr.mozilla.org/build-central/source/buildbot-configs/mozilla/staging_release-fennec-mozilla-release.py.template#191
Attachment #8703794 - Flags: review?(rail) → review-
Attached patch bug1215204staging.patch (obsolete) — Splinter Review
Attached patch bug1215204bouncerconfig.patch (obsolete) — Splinter Review
so this is to add the releases/bouncer_firefox_date.py to date, (just copied the beta version) and the previous patch is submit to the bouncer staging server. Hope this is the correct way forward
Attachment #8704185 - Flags: feedback?(rail)
Attachment #8704183 - Flags: feedback?(rail)
Attachment #8703794 - Attachment is obsolete: true
Attachment #8704183 - Attachment is obsolete: true
Attachment #8704185 - Attachment is obsolete: true
Attachment #8704183 - Flags: feedback?(rail)
Attachment #8704185 - Flags: feedback?(rail)
Attachment #8704198 - Flags: review?(rail)
Attachment #8704198 - Flags: review?(rail) → review+
Attachment #8704198 - Flags: checked-in+
I landed this on date yesterday
Attachment #8704704 - Flags: checked-in+
Attached patch bug1215204properties.patch (obsolete) — Splinter Review
set buildbot_json_path because it's missing
Attachment #8704706 - Flags: review?(rail)
Attached patch bug1215204mhargs.patch (obsolete) — Splinter Review
missing mh args
Attachment #8704707 - Flags: review?(rail)
Comment on attachment 8704707 [details] [diff] [review]
bug1215204mhargs.patch

a nit, I'd add a trailing coma after the last line, so the next patch looks cleaner
Attachment #8704707 - Flags: review?(rail) → review+
Comment on attachment 8704706 [details] [diff] [review]
bug1215204properties.patch

302 jlund

I see buildbot_json_path is mostly used in configs, not in code.
Attachment #8704706 - Flags: review?(rail) → review?(jlund)
Comment on attachment 8704707 [details] [diff] [review]
bug1215204mhargs.patch

landed and added trailing comma
Attachment #8704707 - Flags: checked-in+
Comment on attachment 8704706 [details] [diff] [review]
bug1215204properties.patch

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

this would absolutely work fine but I would ask to tweak it a bit:

::: testing/mozharness/scripts/bouncer_submitter.py
@@ +69,5 @@
>      def _pre_config_lock(self, rw_config):
>          super(BouncerSubmitter, self)._pre_config_lock(rw_config)
>  
>          #override properties from buildbot properties here as defined by taskcluster properties
> +        self.config['buildbot_json_path'] = "buildprops.json"

_pre_config_lock is generally reserved for dynamic config changes that are not known pre run time.

since this value buildbot_json_path is pretty standard and static, we should put it into one of the config files for bouncer

e.g. https://dxr.mozilla.org/build-central/source/mozharness/configs/releases/bouncer_firefox_beta.py?case=true&from=bouncer_firefox_beta.py

saying that, since we will likely end up with more than one config file and you may not wish to put this item in each, you could define this in this class's BaseScript.__init__ under `config`: e.g. like https://dxr.mozilla.org/build-central/source/mozharness/scripts/desktop_l10n.py#168

mh looks for config items in 3 places and merges them by order:

from highest precedence..
1) CLI options
2) 'config' attr within BaseScript init.
3) config files
Attachment #8704706 - Flags: review?(jlund) → review-
Attached patch bug1215204mhargs2.patch (obsolete) — Splinter Review
Attachment #8704707 - Attachment is obsolete: true
this worked on when I reran the the job on a spot instance and modified the scripts locally
cltbld@bld-linux64-spot-535.build.releng.usw2.mozilla.com rel-date_fx_bncr_sub-000000000]$ /tools/buildbot/bin/python scripts/scripts/bouncer_submitter.py -c releases/bouncer_firefox_beta.py --credentials-file oauth.txt --bouncer-api-prefix https://bounceradmin.allizom.org/api --repo projects/date
18:17:23     INFO - MultiFileLogger online at 20160107 18:17:23 in /builds/slave/rel-date_fx_bncr_sub-000000000
18:17:23     INFO - Using buildbot properties:
18:17:23     INFO - {
18:17:23     INFO -     "project": "", 
18:17:23     INFO -     "product": "firefox", 
18:17:23     INFO -     "build_number": 2, 
18:17:23     INFO -     "taskId": "BczAgDHfQdueE5rocPgjnQ", 
18:17:23     INFO -     "repository": "", 
18:17:23     INFO -     "buildername": "release-date_firefox_bncr_sub", 
18:17:23     INFO -     "basedir": "/builds/slave/rel-date_fx_bncr_sub-000000000", 
18:17:23     INFO -     "buildnumber": 7, 
18:17:23     INFO -     "slavename": "bld-linux64-spot-535", 
18:17:23     INFO -     "version": "44.0b31", 
18:17:23     INFO -     "release_promotion": true, 
18:17:23     INFO -     "platform": null, 
18:17:23     INFO -     "branch": "releases/date", 
18:17:23     INFO -     "bouncer_enabled": true, 
18:17:23     INFO -     "script_repo_revision": "production", 
18:17:23     INFO -     "master": "http://buildbot-master73.bb.releng.usw2.mozilla.com:8001/", 
18:17:23     INFO -     "revision": "53f6e3a60916b82de924c8878249930453af9312", 
18:17:23     INFO -     "partial_versions": "44.0b2,", 
18:17:23     INFO -     "repo_path": "projects/date"
18:17:23     INFO - }
18:17:23     INFO - Overriding product with firefox
18:17:23     INFO - Overriding version with 44.0b31
18:17:23     INFO - Overriding build_number with 2
18:17:23     INFO - Overriding revision with 53f6e3a60916b82de924c8878249930453af9312
18:17:23     INFO - Run as scripts/scripts/bouncer_submitter.py -c releases/bouncer_firefox_beta.py --credentials-file oauth.txt --bouncer-api-prefix https://bounceradmin.allizom.org/api --repo projects/date
Attachment #8704706 - Attachment is obsolete: true
Attachment #8705451 - Flags: review?(rail)
Attachment #8705453 - Flags: review?(rail)
Attachment #8705451 - Attachment is obsolete: true
Attachment #8705451 - Flags: review?(rail)
Attachment #8705630 - Flags: review?(rail)
Attachment #8705453 - Flags: review?(rail) → review+
Attachment #8705630 - Flags: review?(rail) → review+
Attachment #8705453 - Flags: checked-in+
Attachment #8705630 - Flags: checked-in+
So the job is being scheduled and running but failing on the buildbot side because it can't access 
https://bounceradmin.allizom.org

<kmoir> my job from last night failed because it couldn't access the bounceradmin dev instance
10:02 AM <kmoir> http://buildbot-master72.bb.releng.usw2.mozilla.com:8001/builders/release-date_firefox_bncr_sub/builds/3/steps/run_script/logs/stdio
10:02 AM <kmoir> how do you access this instance to ensure it is up?
10:02 AM <kmoir> https://bounceradmin.allizom.org/api/product_show?product=Firefox-44.0b33-Complete
10:02 AM <kmoir> I can't access it via a browser either
10:03 AM <kmoir> I looked in the wiki and mana but couldn't find anything on it
Wooohoo, great progress! I think it "Should Just Work (TM)" :), especially if the command line is the same with existing one.
new staging url as per discussion this morning
Attachment #8706459 - Flags: review?(bugspam.Callek)
I ran a build with the last patch manually and I got this error message so will fix this
09:53:58     INFO - <?xml version="1.0" encoding="utf-8"?><locations><product id="5160" name="Firefox-44.0b33-stub"><location id="26241" os="win64">/firefox/releases/44.0b33/win64/:lang/Firefox%20Setup%20Stub%2044.0b33.exe</location></product></locations>
09:53:58    FATAL - Uncaught exception: Traceback (most recent call last):
09:53:58    FATAL -   File "/builds/slave/rel-date_fx_bncr_sub-000000000/scripts/mozharness/base/script.py", line 1721, in run
09:53:58    FATAL -     self.run_action(action)
09:53:58    FATAL -   File "/builds/slave/rel-date_fx_bncr_sub-000000000/scripts/mozharness/base/script.py", line 1663, in run_action
09:53:58    FATAL -     self._possibly_run_method(method_name, error_if_missing=True)
09:53:58    FATAL -   File "/builds/slave/rel-date_fx_bncr_sub-000000000/scripts/mozharness/base/script.py", line 1604, in _possibly_run_method
09:53:58    FATAL -     return getattr(self, method_name)()
09:53:58    FATAL -   File "scripts/scripts/bouncer_submitter.py", line 158, in submit
09:53:58    FATAL -     self.submit_partials()
09:53:58    FATAL -   File "scripts/scripts/bouncer_submitter.py", line 170, in submit_partials
09:53:58    FATAL -     prev_version, prev_build_number = prev_version.split("build")
09:53:58    FATAL - ValueError: need more than 1 value to unpack
09:53:58    FATAL - Running post_fatal callback...
09:53:58    FATAL - Exiting -1
Comment on attachment 8706459 [details] [diff] [review]
bug1215204tuxedourl.patch

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

Would be nice to also touch the rest of the files as well... (but not required by this review):

https://dxr.mozilla.org/build-central/search?q=bounceradmin.allizom&redirect=false&case=false
Attachment #8706459 - Flags: review?(bugspam.Callek) → review+
updates to other pointers to staging bouncer server
Attachment #8706518 - Flags: review?(bugspam.Callek)
Attachment #8706459 - Flags: checked-in+
Attachment #8706518 - Flags: review?(bugspam.Callek) → review+
Attachment #8706518 - Flags: checked-in+
My bouncer job is failing now because
http://buildbot-master74.bb.releng.usw2.mozilla.com:8001/builders/release-date_firefox_bncr_sub/builds/10/steps/run_script/logs/stdio

the versions specified in taskcluster for previous versions look like this

"partial_versions": "44.0b2, 44.0b4, 44.0b6,", 
which gets split and added into prev_versions

while the ones with the old jobs look like this
'prev_versions': ('43.0.1build1', '43.0.3build1', '43.0.2build1'),

Yet the code wants to split them both like this
File "scripts/scripts/bouncer_submitter.py", line 170, in submit_partials
06:25:21    FATAL -     prev_version, prev_build_number = prev_version.split("build")

Should I just add some code to bouncer_submitter.py so that they are split differently for release promotion builds or should the original value be different?
Flags: needinfo?(rail)
I'd probably change https://github.com/rail/releasetasks/blob/master/releasetasks/templates/bouncer.yml.tmpl#L30

to something like 

partial_versions: "{{ ",".join(sorted(partial_updates.keys()) }}"

and adjust the test accordingly.
Flags: needinfo?(rail)
I have a pull request to address the previous comment

https://github.com/mozilla/releasetasks/pulls

However, I'm stuck on another issue
in the templates for releasetasks we don't input the build number for partials for bouncer (However the partial builder is included in the l10n and funsize tasks as a point of reference)
the existing code splits the partials based on "build" in 
http://hg.mozilla.org/build/tools/file/tip/buildfarm/release/release-runner.py#l72
and https://hg.mozilla.org/projects/date/file/tip/testing/mozharness/scripts/bouncer_submitter.py#l170

So I think the https://github.com/mozilla/releasetasks/blob/master/releasetasks/templates/bouncer.yml.tmpl
should include  previousBuildNumber: {{ partial_info["buildNumber"] }} appended to this values in here 
 partial_versions: "{{ ", ".join(sorted(partial_updates.keys())) }}"

so that when the job runs it has values 
like
'prev_versions': ('44.0b2build1', '44.0b4build1', '44.0b6build1'),
instead of the current values in the release promotion jobs on date

that look like this
"44.0b2, 44.0b4, 44.0b6"

is this on the right track?
Yeah, looks like we need to pass the build numbers as well. probably something like https://gist.github.com/rail/e182c4cdd38fab03c9fa may resolve the issue. Not sure how to kill the trailing coma though. Maybe the coma is not that important.
(In reply to Rail Aliiev [:rail] from comment #84)
> Yeah, looks like we need to pass the build numbers as well. probably
> something like https://gist.github.com/rail/e182c4cdd38fab03c9fa may resolve
> the issue. Not sure how to kill the trailing coma though. Maybe the coma is
> not that important.

Just found this:

http://jinja.pocoo.org/docs/dev/templates/#joiner

via:

http://stackoverflow.com/a/33940873
Attached file pull request
Pull request to fix issue with missing build info in bouncer data + remove last comma in list
Attachment #8707946 - Flags: checked-in+
bouncer submitter worked when I updated the dev instance to the latest version of releasetasks and ran another release


http://buildbot-master72.bb.releng.usw2.mozilla.com:8001/builders/release-date_firefox_bncr_sub/builds/6
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
mh patches from date to land on m-i
Comment on attachment 8708020 [details] [diff] [review]
bug1215204mhmi.patch

lgtm
Attachment #8708020 - Flags: review+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: