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

Status: RESOLVED FIXED

(Firefox Build System :: Task Configuration, task)

Description

[Kalpesh Krishna [:martianwars]]

This bug is in reference to the comment https://bugzilla.mozilla.org/show_bug.cgi?id=1281062#c11

Comment 1

[Kalpesh Krishna [:martianwars]]

Attachment: timestamp.patch

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

Comment 2

[Dustin J. Mitchell [:dustin] (he/him)]

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

Comment 3

[Kalpesh Krishna [:martianwars]]

Attachment: timestamp.patch

I hope this is better now.

Comment 4

[Kalpesh Krishna [:martianwars]]

Attachment: timestamp.patch

A few more improvements, here is the push with the patch https://treeherder.mozilla.org/#/jobs?repo=try&revision=a814e270418d707af59fd469ed46f27f732f238b

Comment 5

[Dustin J. Mitchell [:dustin] (he/him)]

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

Comment 6

[Kalpesh Krishna [:martianwars]]

Attachment: timestamp.patch

I hope this works well! Here is the try push https://treeherder.mozilla.org/#/jobs?repo=try&revision=4f5fb3c7d0d104ae129a981613981385fe1c3b37

Comment 7

[Dustin J. Mitchell [:dustin] (he/him)]

Comment on attachment 8767483 [details] [diff] [review]
timestamp.patch

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

nicely done!

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cc4009e9cecc
Replacing timestamps with relative timestamps. r=dustin

Comment 9

[Carsten Book [:Tomcat]]

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')

Comment 10

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/59aaf686843f
Backed out changeset cc4009e9cecc for causing merge conflicts when merging to m-c

Comment 11

[Kalpesh Krishna [:martianwars]]

Attachment: timestamp.patch

This is the rebased patch @Tomcat. Will this work?

Comment 12

[Carsten Book [:Tomcat]]

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

Comment 13

[Carsten Book [:Tomcat]]

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)
~

Comment 14

[Dustin J. Mitchell [:dustin] (he/him)]

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?

Comment 15

[Carsten Book [:Tomcat]]

yeah might be :( sorry adding checkin-needed again

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8c9ee38712de
Replacing timestamps with relative timestamps.

Comment 17

[Carsten Book [:Tomcat]]

Hi,

sorry had to back this out for failures like https://treeherder.mozilla.org/logviewer.html#?job_id=31321621&repo=mozilla-inbound

Comment 18

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e8ec196a048b
Backed out changeset 8c9ee38712de for gecko-decision task failures

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/320ee1117179
Replace timestamps with relative timestamps. r=dustin

Comment 20

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3ead61d6add6
Backed out changeset 320ee1117179 for gecko-decision task failures.

Comment 21

[Kalpesh Krishna [:martianwars]]

@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

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7e73e9581bca
Replacing timestamps with relative timestamps. r=dustin

Comment 23

[Dustin J. Mitchell [:dustin] (he/him)]

I think there are some serious bugs with the autoland and inbound repos both merging to mozilla-central.  I filed bug 1285259 about it.

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7e73e9581bca