Closed Bug 1284005 Opened 5 years ago Closed 5 years ago

Replace timestamps in full-task-graph with relative timestamps.

Categories

(Firefox Build System :: Task Configuration, task)

task
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: martianwars, Assigned: martianwars, Mentored)

References

Details

Attachments

(1 file, 4 obsolete files)

This bug is in reference to the comment https://bugzilla.mozilla.org/show_bug.cgi?id=1281062#c11
Assignee: nobody → kalpeshk2011
Blocks: 1281062
Attached patch timestamp.patch (obsolete) — Splinter Review
I hope this is okay :) It doesn't cover .taskcluster.yml, because I suppose that uses code outside the tree. Here is the push where I tried it out https://treeherder.mozilla.org/#/jobs?repo=try&revision=0a564ff68df4b5e1f0d16326c9b8718f7cf60991
Attachment #8767426 - Flags: review?(dustin)
Comment on attachment 8767426 [details] [diff] [review]
timestamp.patch

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

Very nice -- good work changing all of the existing datestamps over, too.  The only thing I object to is moving the try special-casing in to create.py.

::: taskcluster/taskgraph/create.py
@@ +86,5 @@
> +        else:
> +            repository = result.group(1)
> +            break
> +    if kind == 'legacy' and repository == 'try':
> +        set_expiration(task_def, json_time_from_now(TRY_EXPIRATION, now))

I think this is the wrong place for this change, as this code shouldn't know anything about specific kinds or repoaitories.  Instead, legacy.py should set the expiration to {'relative-datestamp': TRY_EXPIRATION}.  Then there's no need for a special case here.

::: taskcluster/taskgraph/kind/docker_image.py
@@ -54,5 @@
>              'head_rev': params['head_rev'],
>              'owner': params['owner'],
>              'level': params['level'],
> -            'from_now': json_time_from_now,
> -            'now': current_json_time(),

++ to removing these!

::: taskcluster/taskgraph/kind/legacy.py
@@ -362,5 @@
>              'rank': push_epoch,
>              'owner': params['owner'],
>              'level': params['level'],
> -            'from_now': json_time_from_now,
> -            'now': current_json_time(),

++ to removing these so nothing sneaks back in

@@ -460,5 @@
>  
>              # try builds don't use cache
>              if project == "try":
>                  remove_caches_from_task(build_task)
> -                set_expiration(build_task, json_time_from_now(TRY_EXPIRATION))

See my comments in create.py -- this operation should stay here, not move to create.py

@@ -543,5 @@
>                                                       build_treeherder_config)
>                  set_interactive_task(post_task, interactive)
>  
> -                if project == "try":
> -                    set_expiration(post_task, json_time_from_now(TRY_EXPIRATION))

same

@@ -594,5 @@
>                                                      test_parameters['head_rev'],
>                                                      test_parameters['pushlog_id'])
>  
> -                    if project == "try":
> -                        set_expiration(test_task, json_time_from_now(TRY_EXPIRATION))

same
Attachment #8767426 - Flags: review?(dustin) → review-
Attached patch timestamp.patch (obsolete) — Splinter Review
I hope this is better now.
Attachment #8767426 - Attachment is obsolete: true
Attachment #8767430 - Flags: review?(dustin)
Attached patch timestamp.patch (obsolete) — Splinter Review
A few more improvements, here is the push with the patch https://treeherder.mozilla.org/#/jobs?repo=try&revision=a814e270418d707af59fd469ed46f27f732f238b
Attachment #8767430 - Attachment is obsolete: true
Attachment #8767430 - Flags: review?(dustin)
Attachment #8767431 - Flags: review?(dustin)
Comment on attachment 8767431 [details] [diff] [review]
timestamp.patch

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

This looks great!

One more thing to add: docs!  The task-reference format is documented here:
  https://dxr.mozilla.org/mozilla-central/source/taskcluster/docs/taskgraph.rst#187
so please add something similar about the relative timestamps.

::: taskcluster/taskgraph/kind/legacy.py
@@ +201,3 @@
>      task_def = task['task']
> +    task_def['expires'] = {}
> +    task_def['expires']['relative-datestamp'] = relative_datestamp

why not just
  task_def['expires'] = {'relative-datestamp': relative_datestamp}

@@ +209,5 @@
> +        deadline = json_time_from_now(input_str=task_def['deadline']['relative-datestamp'],
> +                                      now=now,
> +                                      datetime_format=True)
> +        if deadline > timestamp:
> +            task_def['deadline']['relative-datestamp'] = relative_datestamp

very nice!

::: taskcluster/taskgraph/util/time.py
@@ +86,3 @@
>      '''
>      :param str input_str: Input string (see value of)
>      :param datetime now: Optionally set the definition of `now`

need to add a line here about datetime_format

@@ +106,2 @@
>      '''
>      :returns: JSON string representation of the current time.

same
Attachment #8767431 - Flags: review?(dustin) → feedback+
Attached patch timestamp.patch (obsolete) — Splinter Review
I hope this works well! Here is the try push https://treeherder.mozilla.org/#/jobs?repo=try&revision=4f5fb3c7d0d104ae129a981613981385fe1c3b37
Attachment #8767431 - Attachment is obsolete: true
Attachment #8767483 - Flags: review?(dustin)
Comment on attachment 8767483 [details] [diff] [review]
timestamp.patch

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

nicely done!
Attachment #8767483 - Flags: review?(dustin) → review+
Keywords: checkin-needed
Pushed by kwierso@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cc4009e9cecc
Replacing timestamps with relative timestamps. r=dustin
Keywords: checkin-needed
sorry had to back this out for causing conflicts when merging to m-c like :

warning: conflicts while merging taskcluster/taskgraph/kind/docker_image.py! (edit, then use 'hg resolve --mark')
Flags: needinfo?(kalpeshk2011)
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/59aaf686843f
Backed out changeset cc4009e9cecc for causing merge conflicts when merging to m-c
Attached patch timestamp.patchSplinter Review
This is the rebased patch @Tomcat. Will this work?
Flags: needinfo?(kalpeshk2011) → needinfo?(cbook)
Attachment #8768360 - Flags: review+
hi, this seems to fail again

patching file taskcluster/ci/legacy/tasks/post-builds/mulet_simulator.yml
Hunk #2 succeeded at 44 with fuzz 2 (offset 0 lines).
patching file taskcluster/taskgraph/kind/docker_image.py
Hunk #2 FAILED at 44
1 out of 2 hunks FAILED -- saving rejects to file taskcluster/taskgraph/kind/docker_image.py.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh timestamp.patch
Flags: needinfo?(cbook) → needinfo?(kalpeshk2011)
the rej file is 

--- docker_image.py
+++ docker_image.py
@@ -49,18 +45,16 @@ class DockerImageTask(base.Task):
             'project': params['project'],
             'docker_image': docker_image,
             'base_repository': params['base_repository'] or params['head_repository'],
             'head_repository': params['head_repository'],
             'head_ref': params['head_ref'] or params['head_rev'],
             'head_rev': params['head_rev'],
             'owner': params['owner'],
             'level': params['level'],
-            'from_now': json_time_from_now,
-            'now': current_json_time(),
             'source': '{repo}file/{rev}/taskcluster/ci/docker-image/image.yml'
                       .format(repo=params['head_repository'], rev=params['head_rev']),
         }

         tasks = []
         templates = Templates(path)
         for image_name in config['images']:
             context_path = os.path.join('testing', 'docker', image_name)
~
I'm guessing that originally conflicted with

 https://hg.mozilla.org/mozilla-central/rev/b446cef7d820#l3.12

however the updated version of martianwars' patch includes that change, and applies to m-c without issues.  Did your second try maybe not use the updated patch?
Flags: needinfo?(kalpeshk2011)
yeah might be :( sorry adding checkin-needed again
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8c9ee38712de
Replacing timestamps with relative timestamps.
Keywords: checkin-needed
Hi,

sorry had to back this out for failures like https://treeherder.mozilla.org/logviewer.html#?job_id=31321621&repo=mozilla-inbound
Flags: needinfo?(kalpeshk2011)
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e8ec196a048b
Backed out changeset 8c9ee38712de for gecko-decision task failures
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/320ee1117179
Replace timestamps with relative timestamps. r=dustin
Backout by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3ead61d6add6
Backed out changeset 320ee1117179 for gecko-decision task failures.
Attachment #8767483 - Attachment is obsolete: true
Flags: needinfo?(kalpeshk2011)
@Tomcat,
Are you sure you've applied the correct patch? The error is exactly on those lines where the .rej file was pointing at. I've marked the old patch as obsolete.
I can't see the change https://hg.mozilla.org/integration/mozilla-inbound/rev/8c9ee38712de#l26.2
If you see my patch, I've done them on https://bugzilla.mozilla.org/attachment.cgi?id=8768360&action=diff#a/taskcluster/taskgraph/kind/docker_image.py_sec3
Flags: needinfo?(cbook)
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7e73e9581bca
Replacing timestamps with relative timestamps. r=dustin
Flags: needinfo?(cbook)
I think there are some serious bugs with the autoland and inbound repos both merging to mozilla-central.  I filed bug 1285259 about it.
https://hg.mozilla.org/mozilla-central/rev/7e73e9581bca
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Product: TaskCluster → Firefox Build System
You need to log in before you can comment on or make changes to this bug.