Bug 194863 - [ews-app] Add model for handling multiple Buildbot instances
Summary: [ews-app] Add model for handling multiple Buildbot instances
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Aakash Jain
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-02-20 12:03 PST by Aakash Jain
Modified: 2019-02-28 12:20 PST (History)
10 users (show)

See Also:


Attachments
Proposed patch (2.66 KB, patch)
2019-02-20 12:56 PST, Aakash Jain
no flags Details | Formatted Diff | Diff
Proposed patch (2.66 KB, patch)
2019-02-24 16:22 PST, Aakash Jain
dewei_zhu: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Aakash Jain 2019-02-20 12:03:05 PST
[ews-app] Add instance table to keep tracking of multiple buildbot instance, or multiple version of same instance (in case Buildbot instance has to be formatted for some reason).
Comment 1 Aakash Jain 2019-02-20 12:56:21 PST
Created attachment 362527 [details]
Proposed patch
Comment 2 Aakash Jain 2019-02-24 16:22:37 PST
Created attachment 362868 [details]
Proposed patch
Comment 3 Jonathan Bedard 2019-02-25 09:40:15 PST
Comment on attachment 362868 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=362868&action=review

> Tools/BuildSlaveSupport/ews-app/ews/models/buildbotinstance.py:29
> +    instance_id = models.AutoField(primary_key=True)

Curious why we need a separate instance_id, can't the hostname be a primary key? Do we ever host multiple buildbot instances on the same hostname (that seems wrong)? Or is it possible that we so significantly change the characteristics of a buildbot instance at a specific hostname that we want the EWS database to conceptually consider the updated buildbot a new instance?
Comment 4 Aakash Jain 2019-02-25 09:50:18 PST
(In reply to Jonathan Bedard from comment #3)
> Curious why we need a separate instance_id, can't the hostname be a primary key? Do we ever host multiple buildbot instances on the same hostname (that seems wrong)?

Yes, it's possible to have two entries in this table with same hostname. This table is specifically being added to handle two cases:
1) if the buildbot machine is formatted, and start sending data again (with same hostname).
2) if we have two buildbot machines(different hostname, e.g.: OpenSource and internal) running in parallel and sending data to Django app.

In case #1, this table would have a new entry for the same hostname, previous entry would be marked inactive (and urls would for inactive hostname would not be created).


> Or is it possible that we so significantly change the characteristics of a buildbot instance at a specific hostname that we want
> the EWS database to conceptually consider the updated buildbot a new
> instance?
Comment 5 Jonathan Bedard 2019-02-25 10:20:19 PST
(In reply to Aakash Jain from comment #4)
> (In reply to Jonathan Bedard from comment #3)
> > Curious why we need a separate instance_id, can't the hostname be a primary key? Do we ever host multiple buildbot instances on the same hostname (that seems wrong)?
> 
> Yes, it's possible to have two entries in this table with same hostname.
> This table is specifically being added to handle two cases:
> 1) if the buildbot machine is formatted, and start sending data again (with
> same hostname).
> 2) if we have two buildbot machines(different hostname, e.g.: OpenSource and
> internal) running in parallel and sending data to Django app.
> 
> In case #1, this table would have a new entry for the same hostname,
> previous entry would be marked inactive (and urls would for inactive
> hostname would not be created).

Is case #1 meant to handle, for instance, changing buildbot from .8 to 1.0, where the old URLs are basically defunct? I ask because I would not expect to have a new entry if the buildbot instance was, for example, just restarted to pick up a new configuration.

As a related note, I assume code updating active/created/modified will be added in the future, I am wondering how we will distinguish case #1 from something like network failures.
 
> > Or is it possible that we so significantly change the characteristics of a buildbot instance at a specific hostname that we want
> > the EWS database to conceptually consider the updated buildbot a new
> > instance?
Comment 6 Aakash Jain 2019-02-25 10:41:35 PST
> Is case #1 meant to handle, for instance, changing buildbot from .8 to 1.0, where the old URLs are basically defunct? 
It can handle those cases as well, however the primary motivation was to handle the case when we have an unexpected data loss event on buildbot machine (e.g.: buildbot machine crashed and couldn't be recovered). In that case buildbot on same hostname would re-start build-id/builder-id/step-id from 1. Therefore the previous urls (e.g: https://ews-build.webkit-uat.org/#/builders/1/builds/1) would be valid, but points to new build instead of old build, so the urls generated using data from old build info might link to new data which would be misleading. Therefore we would want to distinguish whether the build data is before/after the buildbot machine was formatted (and not generate urls at all for old build data). Every build column would also have buildbot_instance_id, and using that we could check if the build data is old or new.


> I ask because I would not expect to have a new entry if the buildbot instance was, for example, just restarted to pick up a new configuration.
Exactly, restart or any buildbot configuration change or any network failures would NOT have any impact on this table. We would be adding a new entry in this table for the same hostname, only after a very major event like buildbot machine crash/data-loss/format.

> As a related note, I assume code updating active/created/modified will be added in the future
Yes, many more patches upcoming.

> I am wondering how we will distinguish case #1 from something like network failures.
Network failure wouldn't have any impact on this table. As to how will we decide to add another entry in this table for existing hostname, that can be either manual or in some automated manner (by having some logic to detect such event). Let's discuss that logic, or whether to not have that logic(and do it manually) later on when I upload that patch.
Comment 7 Dean Johnson 2019-02-25 10:52:58 PST
Comment on attachment 362868 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=362868&action=review

Motivation/concept LGTM; couple questions on implementation / design.

> Tools/BuildSlaveSupport/ews-app/ews/models/buildbotinstance.py:33
> +    modified = models.DateTimeField(auto_now=True)

What value does having a 'modified' column bring, and when do you plan on having it update? Does auto_now imply that it updates whenever something is added/updated that has a reference to a BuildbotInstance?

Also, how do you plan on doing the deactivation of the old master?
Comment 8 Aakash Jain 2019-02-25 11:39:37 PST
> What value does having a 'modified' column bring
When we change an entry from 'active' to 'inactive' (either manually or in some automated manner), the created timestamp would remain same, but 'modified' timestamp would be updated. So it would indicate the timestamp when any entry in this table was last modified. Might help in debugging in some cases. 

> and when do you plan on having it update? Does auto_now imply that it updates whenever something is added/updated that has a reference to a BuildbotInstance?
Django handles it automatically. It is updated whenever the entry in this table is created/updated. 
https://docs.djangoproject.com/en/2.1/ref/models/fields/
"DateField.auto_now": Automatically set the field to now every time the object is saved. Useful for “last-modified” timestamps. 

> Also, how do you plan on doing the deactivation of the old master?
Haven't finalized yet whether it would be using automated logic or manual. Might need some discussion.
Comment 9 WebKit Commit Bot 2019-02-25 18:55:09 PST
Comment on attachment 362868 [details]
Proposed patch

Clearing flags on attachment: 362868

Committed r242066: <https://trac.webkit.org/changeset/242066>
Comment 10 WebKit Commit Bot 2019-02-25 18:55:10 PST
All reviewed patches have been landed.  Closing bug.
Comment 11 Radar WebKit Bug Importer 2019-02-25 19:19:17 PST
<rdar://problem/48388064>