Hasal is overwriting push/commit information

RESOLVED FIXED

Status

Testing
General
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: wlach, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Compare:

https://treeherder.allizom.org/#/jobs?repo=mozilla-central&revision=57a8cde3f08ca9d60bcd8bdd698ceec687f0aed2

With:

https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&revision=57a8cde3f08ca9d60bcd8bdd698ceec687f0aed2

The former has a bogus commit message saying `Updata Hasal Result - 1479729941.36`

We need to fix this before we can allow Hasal to post to Treeherder production. The problematic code is here:

It should be rewritten to get the existing result set from Treeherder. Just taking out the code to create the result set should be sufficient (if you need it for testing purposes, hide it behind an option which is off by default):

https://github.com/Mozilla-TWQA/Hasal/blob/master/server/perfherder_uploader.py#L52
Flags: needinfo?(sho)
Flags: needinfo?(bchien)

Comment 1

2 years ago
Hi Will,

Since Askeing is OOO these two days, I will try to fix this issue for him.
So if I understand you correctly, I should remove the line from 54 to 71, return the trsc directly instead of rewriting the value in it, right?
Flags: needinfo?(sho)
(In reply to Shako Ho from comment #1)
> Hi Will,
> 
> Since Askeing is OOO these two days, I will try to fix this issue for him.
> So if I understand you correctly, I should remove the line from 54 to 71,
> return the trsc directly instead of rewriting the value in it, right?

Just remove the code to submit the resultset. On treeherder stage/production, it will always exist by the time you submit.

Comment 3

2 years ago
Ok, I think I just need to remove the line 297, then it will your expectation, right?
(In reply to Shako Ho from comment #3)
> Ok, I think I just need to remove the line 297, then it will your
> expectation, right?

That should be ok, but if you're going to leave most of the code in I recommend commenting it out and leaving in something like this, so it doesn't get accidentally re-enabled:

# don't post resultset, that overwrites existing data. see: https://bugzilla.mozilla.org/show_bug.cgi?id=1320694
# client.post_collection(self.repo, trsc)

I want to emphasize that this bug was pretty bad, as it destroys information on what was pushed to treeherder. It's good that we caught this error in staging, but let's be very careful that this code doesn't ever get applied against production.

Comment 5

2 years ago
Ok, thanks. I will patch this when I step in offic this morning.

Sorry for introducing such bug, and luckily we didn't bring any damage on production.

In addition to this, I would like to know more detail about this bug, as I know, Askeing did the local testing and stage testing for couple weeks, did we introduce such bug because of ingoring error log from treeherder or misunderstanding the document or something else? I just want to make sure we won't introduce similar bugs because of missing the same thing.

Thanks
(In reply to Shako Ho from comment #5)
> Ok, thanks. I will patch this when I step in offic this morning.
> 
> Sorry for introducing such bug, and luckily we didn't bring any damage on
> production.
> 
> In addition to this, I would like to know more detail about this bug, as I
> know, Askeing did the local testing and stage testing for couple weeks, did
> we introduce such bug because of ingoring error log from treeherder or
> misunderstanding the document or something else? I just want to make sure we
> won't introduce similar bugs because of missing the same thing.

No, I think it would have been hard to know this would be a problem in advance. It's understandable that the code was written -- the revision info was needed when testing against a dev instance. It's only when submitting to the real instance that it became a problem. In the future, maybe we need a better/easier way of just pulling down this revision information (and nothing else) from hg.mozilla.org

Probably the best thing to do in the future would be to get feedback from either me, :emorley or :camd when working on code which submits to Treeherder. Any of us would probably have been able to spot this error.
Removing needinfo flag from bchien
Flags: needinfo?(bchien)

Comment 8

2 years ago
A quick update:  patch is merged, you should not see the resultset is overwritting
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.