Getting a profile from Talos in automation shouldn't require --rebuild-talos > 1

ASSIGNED
Assigned to

Status

Testing
Talos
ASSIGNED
6 months ago
a month ago

People

(Reporter: mconley, Assigned: wcosta, NeedInfo)

Tracking

(Depends on: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

I have a Talos regression that I'm dealing with, and I cannot reproduce it locally. I can reproduce it consistently in automation, so I'd like to gather a profile from the testing machine. Normally I do that with the mozharness: --geckoProfile incantation in the try syntax, but that doesn't appear to be working.

What _does_ appear to be working is:

try: -b o -p macosx64 -u none -t other-e10s --geckoProfile --rebuild-talos 2

Notice that I don't need mozharness, and that I require --rebuild-talos 2. This is because of this bit of code:

http://searchfox.org/mozilla-central/rev/bd9dc581ea75a562f74674651b41d60de3efa393/taskcluster/taskgraph/target_tasks.py#75-82

which seems to want talos_trigger_tests > 1 in order to do any profiling.
The > 1 stuff was added in bug 1333167, it seems.
Blocks: 1333167
I could muck around in target_tasks.py, but it might be better if somebody who knows what they're doing looks at this stuff...

Hey wcosta, is fixing this as simple as making that > 1 a > 0?
Flags: needinfo?(wcosta)
(Assignee)

Comment 3

6 months ago
I take a look at fixing this. :dustin, do you know if there is a specific reason for this behavior or it is just a bug?
Assignee: nobody → wcosta
Status: NEW → ASSIGNED
Flags: needinfo?(wcosta) → needinfo?(dustin)
Hm no idea.  The `profile` attribute is not documented, and a quick grep shows that being the only place it's used.  Attributes are only used to select tasks for creation -- they describe, rather than affect, what the task does.  So setting task.attributes['profile'] = 'anything' cannot change what the task does.  So I suspect those assignments are useless and should be deleted.

To communicate --geckoProfile through to mozharness, it needs to be added to the extra options. That happens here:

taskcluster/taskgraph/transforms/tests.py
@transforms.add
def set_profile(config, tests):
    """Set profiling mode for tests."""
    for test in tests:
        if config.config['args'].profile and test['suite'] == 'talos':
            test['mozharness']['extra-options'].append('--geckoProfile')
        yield test

config.config['args'].profile is set to true in the try syntax command line parser when --geckoProfile is present.

...

Ah, but this is Talos on OS X.  Which is still run by Buildbot, and currently run via BBB (Buildbot Bridge).  So the task looks like this:

{
  "test-linux64-nightly/opt-talos-g1-e10s": {
    "attributes": {
      "build_platform": "linux64-nightly",
      "build_type": "opt",
      "e10s": true,
      "kind": "test",
      "nightly": true,
      "run_on_projects": [
        "mozilla-beta",
        "mozilla-aurora",
        "mozilla-central",
        "mozilla-inbound",
        "autoland",
        "try"
      ],
      "talos_try_name": "g1-e10s",
      "test_chunk": "1",
      "test_platform": "linux64-nightly/opt",
      "unittest_flavor": "talos",
      "unittest_suite": "talos"
    },
    "dependencies": {},
    "kind": "test",
    "label": "test-linux64-nightly/opt-talos-g1-e10s",
    "optimizations": [],
    "task": {
      "created": {
        "relative-datestamp": "0 seconds"
      },
      "deadline": {
        "relative-datestamp": "1 day"
      },
      "expires": {
        "relative-datestamp": "14 days"
      },
      "extra": {
        "chunks": {
          "current": 1,
          "total": 1
        },
        "suite": {
          "flavor": "talos",
          "name": "talos"
        }
      },
      "metadata": {
        "description": "Talos g1 ([Treeherder push](https://treeherder.mozilla.org/#/jobs?repo=try&revision=999bc2c54b7fd25a87662d6eaec26c7801055b5a))",
        "name": "test-linux64-nightly/opt-talos-g1-e10s",
        "owner": "dmitchell@mozilla.com",
        "source": "https://hg.mozilla.org/try//file/999bc2c54b7fd25a87662d6eaec26c7801055b5a/taskcluster/ci/test"
      },
      "payload": {
        "buildername": "Ubuntu HW 12.04 x64 try talos g1-e10s",
        "properties": {
          "installer_path": "public/build/target.tar.bz2",
          "product": "firefox",
          "who": "dmitchell@mozilla.com"
        },
        "sourcestamp": {
          "branch": "try",
          "repository": "https://hg.mozilla.org/try/",
          "revision": "999bc2c54b7fd25a87662d6eaec26c7801055b5a"
        }
      },
      "priority": "very-low",
      "provisionerId": "buildbot-bridge",
      "routes": [
        "tc-treeherder.v2.try.999bc2c54b7fd25a87662d6eaec26c7801055b5a.190697",
        "tc-treeherder-stage.v2.try.999bc2c54b7fd25a87662d6eaec26c7801055b5a.190697"
      ],
      "scopes": [],
      "tags": {
        "createdForUser": "dmitchell@mozilla.com"
      },
      "workerType": "buildbot-bridge"
    }
  }
}

and indeed, there's nothing about profiling in there.  I'm not sure how, or even if it's possible, we can pass that profiling information along via BBB.
Flags: needinfo?(dustin)

Comment 5

6 months ago
I wonder if this is related to bug 1368180? We realized a few weeks ago that BBB isn't creating sourcestamps/changes for buildsets, and I think that's where the try syntax would end up.

IIRC, there is logic in mozharness to parse out the try syntax from the buildbot properties, which normally contains the sourcestamp information, including try syntax in the commit messages.
(Assignee)

Comment 6

6 months ago
(In reply to Chris AtLee [:catlee] from comment #5)
> I wonder if this is related to bug 1368180? We realized a few weeks ago that
> BBB isn't creating sourcestamps/changes for buildsets, and I think that's
> where the try syntax would end up.
> 
> IIRC, there is logic in mozharness to parse out the try syntax from the
> buildbot properties, which normally contains the sourcestamp information,
> including try syntax in the commit messages.

I think it is a two pieces fix;

1) Let BBB pass through the mozharness try command line
2) Add try syntax to BBB task

*2* is straightforward to implement, *1*, I don't know.

Comment 7

6 months ago
https://dxr.mozilla.org/mozilla-central/source/testing/mozharness/mozharness/mozilla/testing/try_tools.py#164 looks like it's responsible for extracting extra arguments to pass along to the test harness based on the try syntax in the commit message.

BBB already allows passing arbitrary properties via the task definition to buildbot. mozharness right now doesn't have support to read the try syntax from a property; it's expecting it to be in the 'sourcestamp' information.

Comment 8

6 months ago
I see at least two ways forward:
1) Modify task definitions to add the extra try options to a property; modify mozharness to look for that property and adjust command line appropriately

2) Modify task definitions to include the per-commit "change" information, which is where the commit messages are expected to be found; modify BBB to create the appropriate entries in BB


Approach 2) is more correct I think, and would also fix bug 1368180. It's also more complicated to implement.

Comment 9

6 months ago
https://github.com/catlee/buildbot-bridge/commit/9a669dda4ac7d463e2723e25b313ae3abb8dc27b has a WIP for BBB support for the commit data. I would like to verify where the change property source information is coming from, since it's not represented in the DB. I would also like to have a better schema for the changes field.
Comment hidden (mozreview-request)

Comment 11

6 months ago
mozreview-review
Comment on attachment 8876412 [details]
Bug 1370907: Pass the try command line to BBB.

https://reviewboard.mozilla.org/r/147772/#review152380

::: taskcluster/taskgraph/try_option_syntax.py:199
(Diff revision 1)
>      return result
>  
>  
>  def split_try_msg(message):
>      try:
> +        message, _, mh_options = message.partition('mozharness:')

does `mozharness:` always come after `try:`?

::: taskcluster/taskgraph/try_option_syntax.py:203
(Diff revision 1)
>      try:
> +        message, _, mh_options = message.partition('mozharness:')
>          try_idx = message.index('try:')
>      except ValueError:
>          return []
> -    message = message[try_idx:].split('\n')[0]
> +    message = message[try_idx:].split('\n')[0] + mh_options

I'm confused what this change to `split_try_message` does -- it looks like it removes `mozharness:` from the commit message, but includes all options after `try:` up to a newline, and all options after `mozharness:`.  How does that relate to passing the message to the builder?
(Assignee)

Comment 12

6 months ago
mozreview-review-reply
Comment on attachment 8876412 [details]
Bug 1370907: Pass the try command line to BBB.

https://reviewboard.mozilla.org/r/147772/#review152380

> does `mozharness:` always come after `try:`?

Hrm, that's a good question, I picked from try chooser, I was biased that mozharness: is part of try:

> I'm confused what this change to `split_try_message` does -- it looks like it removes `mozharness:` from the commit message, but includes all options after `try:` up to a newline, and all options after `mozharness:`.  How does that relate to passing the message to the builder?

We only need to mozharness: to pass the try message to BBB, for TC jobs, the in tree config parses the command line directly. I think I need to put a comment on that.
(In reply to Wander Lairson Costa [:wcosta] from comment #12)
> We only need to mozharness: to pass the try message to BBB, for TC jobs, the
> in tree config parses the command line directly. I think I need to put a
> comment on that.

I don't think that the result of `split_try_message` ends up in `config.params['message']`, which is what gets passed to BBB - that was the source of m confusion.
Comment hidden (mozreview-request)

Comment 15

6 months ago
mozreview-review
Comment on attachment 8876412 [details]
Bug 1370907: Pass the try command line to BBB.

https://reviewboard.mozilla.org/r/147772/#review152900

::: taskcluster/taskgraph/try_option_syntax.py:204
(Diff revisions 1 - 2)
>          message, _, mh_options = message.partition('mozharness:')
>          try_idx = message.index('try:')
>      except ValueError:
>          return []
> +    # in-tree command line parser doesn't need mozharness: keyword,
> +    # but it is necessary if we are running buildbot-bridge

For my reference, this is a way of ensuring that any options after `mozharness:` are included in try sytanx, even if the `mozharness:` is on a line other thanthe first.  Newlines in command-line syntax!
Attachment #8876412 - Flags: review?(dustin) → review+

Updated

5 months ago
Depends on: 1368180

Comment 16

5 months ago
mozreview-review
Comment on attachment 8876412 [details]
Bug 1370907: Pass the try command line to BBB.

https://reviewboard.mozilla.org/r/147772/#review168304

::: taskcluster/taskgraph/transforms/job/mozharness.py:270
(Diff revision 2)
>          'sourcestamp': {
>              'branch': branch,
>              'repository': config.params['head_repository'],
>              'revision': config.params['head_rev'],
> +            'changes': [{
> +                'comments': config.params['message'],

We won't be able to add these change objects via BBB because they're missing some fields that are required by the SQL schema.

The current schema is:
CREATE TABLE changes (
                `changeid` INTEGER PRIMARY KEY AUTOINCREMENT,
                `author` VARCHAR(1024) NOT NULL,
                `comments` VARCHAR(1024) NOT NULL,
                `is_dir` SMALLINT NOT NULL, -- old, for CVS
                `branch` VARCHAR(1024) NULL,
                `revision` VARCHAR(256),
                `revlink` VARCHAR(256) NULL,
                `when_timestamp` INTEGER NOT NULL,
                `category` VARCHAR(256) NULL,
                `repository` TEXT NOT NULL default '',
                `project` TEXT NOT NULL default ''
            );

We need to provide values for at least all the NOT NULL or don't have default values.

`changeid` is autogenerated, no need to do anything there
`author` should be specified
`comments` is what you're handling right now
`is_dir` we can make BBB set to 0. it's never used
`branch` should be like 'mozilla-inbound'
`revision` should be the push's revision
`revlink` is like https://hg.mozilla.org/mozilla-central/rev/{revision}
`when_timestamp` is the unix timestamp of the push time. if that's hard to get, the current time is fine.
Attachment #8876412 - Flags: review?(catlee) → review-

Comment 17

5 months ago
mozreview-review
Comment on attachment 8876412 [details]
Bug 1370907: Pass the try command line to BBB.

https://reviewboard.mozilla.org/r/147772/#review168308
Comment hidden (mozreview-request)
Any chance this could be moved forward soon please? I've just been pointed to using the expected command, and had to dig through source and bugs until I found this.
Flags: needinfo?(catlee)
Comment hidden (mozreview-request)
You need to log in before you can comment on or make changes to this bug.