You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@beam.apache.org by Danny McCormick <da...@google.com> on 2022/06/30 15:39:52 UTC
[Proposal] Path to Re-enabling Build Comment Triggers
Hey everyone, I've been digging into the issues we've been having with
Jenkins recently[1] which led to us disabling many of our build
comment triggers (e.g. "Run <job name>" triggering a Jenkins job). Moving
forward, I'd like to recommend that we:
1) Try to disable the "Can one of the admins verify this patch?" comments
via the Jenkins plugin configuration.
2) Add trusted repeat contributors to the Jenkins allow-list.
3) Try re-enabling all build triggers.
*Justification*
Right now, around 33% of our PR comments are "Can one of the admins verify
this patch?". Aside from being (IMO) very unhelpful and annoying, these are
actually causing a significant amount of load on the ghprb plugin and
indicate that these PRs are more expensive than those from allow-listed
contributors. Since we believe that the ghprb plugin makes calls to GitHub
that are roughly proportional to <the number of pr comments>X<the number of
build triggers>, reducing our number of issue comments and the load per
comment should give us enough breathing room to enable our triggers again.
I wrote up a more thorough supporting doc here as well - -
https://docs.google.com/document/d/15CILeNjNxCnbigSvxNq4eXPj6x6sn5DGdbTdWu55kCI/edit?usp=sharing
*Disclaimer*
It's really hard to empirically prove any of this after the fact - I think
there's enough evidence to try it, but we should be ready to engage Infra
to restart Jenkins and ready to revert any triggers we add.
Thanks,
Danny
[1] Context on previous investigation here -
https://docs.google.com/document/d/10qyUsvB_uVy5jftfTiwohlvN8Qwix5AuadssyoC4JsE/edit?usp=sharing
Re: [Proposal] Path to Re-enabling Build Comment Triggers
Posted by Danny McCormick via dev <de...@beam.apache.org>.
Given that there have been no objections, I reenabled build comment
triggers this morning in https://github.com/apache/beam/pull/22169.
Hopefully the underlying issues are resolved, but if not we can quickly
revert and engage Infra to restart Jenkins to restore the previous state.
Thanks,
Danny
On Fri, Jul 1, 2022 at 12:42 PM Danny McCormick <da...@google.com>
wrote:
> Hey Yi, that's a good idea. Unfortunately, though, there is not a good way
> to do it without modifying the plugin itself (not just the configuration).
> Modifying the plugin is theoretically an option, but it is not easy -
> finding a way to build/deploy it is hard, it doesn't build out of the box
> right now, and it hasn't been touched in 2 years. If we're going to invest
> the time, I'd vote we either use that time to move off of the plugin
> entirely or move all our Jenkins infra to GitHub Actions (I'd vote to move
> to GHA) - both of those require a lot of effort and are out of the scope of
> this thread.
>
> It is worth being super clear here - anything we do that doesn't involve
> extensively patching the plugin or moving off of it entirely is a band-aid.
> As the community grows and our GitHub automation grows (as Yi pointed out)
> this will continue to be an issue. This change should buy us a significant
> amount of time though.
>
> Thanks,
> Danny
>
> On Fri, Jul 1, 2022 at 11:12 AM Yi Hu via dev <de...@beam.apache.org> wrote:
>
>> Thanks for the investigation done and disabling the "admins verify patch"
>> comment. Wondering if there is a way to early return the bot comments for
>> ghprb (like blacklist)? Besides asf-ci there are also comments
>> from github-actions (e.g. assign reviewer or "stopping review
>> notifications"). In the future if we enable more github action items there
>> might be more bot pr comments.
>>
>> Best,
>> Yi
>>
>> On Fri, Jul 1, 2022 at 8:33 AM Danny McCormick via dev <
>> dev@beam.apache.org> wrote:
>>
>>> Given the early consensus here, I tried disabling the "Can one of the
>>> admins verify this patch?" messages, and verified that it worked. If you
>>> disagree with that decision, please let me know - I expect that is the most
>>> popular part of this proposal though :)
>>>
>>> The remaining questions are:
>>> 1) Are we comfortable adding trusted repeat contributors to the Jenkins
>>> allow-list?
>>> 2) Are we ok trying to enable build triggers?
>>>
>>> Given that disabling the "Can one of the admins verify this patch?"
>>> messages worked, I'm not sure that we need to do (1) - I think it would
>>> help reduce load from the plugin a little bit, but is probably not as
>>> necessary or helpful as what we've already done. I'm pretty comfortable
>>> skipping that step.
>>>
>>> *If there are no objections, I'd like to try reenabling build comment
>>> triggers next week.* Because I'll be offline for the first half of next
>>> week for the American 4th of July holiday, I will plan on trying it next
>>> Wednesday or Thursday if there are no objections.
>>>
>>> Thanks,
>>> Danny
>>>
>>> On Thu, Jun 30, 2022 at 2:54 PM Pablo Estrada <pa...@google.com>
>>> wrote:
>>>
>>>> Agreed! I never know what to do about them : )
>>>>
>>>> On Thu, Jun 30, 2022 at 10:58 AM Robert Burke <ro...@frantil.com>
>>>> wrote:
>>>>
>>>>> +1 to get rid of the "admins very patch" comments if we can. They add
>>>>> no value at all when commented in quadruplicate immediately after PR
>>>>> creation.
>>>>>
>>>>> On Thu, Jun 30, 2022, 8:40 AM Danny McCormick <
>>>>> dannymccormick@google.com> wrote:
>>>>>
>>>>>> Hey everyone, I've been digging into the issues we've been having
>>>>>> with Jenkins recently[1] which led to us disabling many of our build
>>>>>> comment triggers (e.g. "Run <job name>" triggering a Jenkins job). Moving
>>>>>> forward, I'd like to recommend that we:
>>>>>>
>>>>>> 1) Try to disable the "Can one of the admins verify this patch?"
>>>>>> comments via the Jenkins plugin configuration.
>>>>>> 2) Add trusted repeat contributors to the Jenkins allow-list.
>>>>>> 3) Try re-enabling all build triggers.
>>>>>>
>>>>>> *Justification*
>>>>>>
>>>>>> Right now, around 33% of our PR comments are "Can one of the admins
>>>>>> verify this patch?". Aside from being (IMO) very unhelpful and
>>>>>> annoying, these are actually causing a significant amount of load on the
>>>>>> ghprb plugin and indicate that these PRs are more expensive than those from
>>>>>> allow-listed contributors. Since we believe that the ghprb plugin makes
>>>>>> calls to GitHub that are roughly proportional to <the number of pr
>>>>>> comments>X<the number of build triggers>, reducing our number of issue
>>>>>> comments and the load per comment should give us enough breathing room to
>>>>>> enable our triggers again.
>>>>>>
>>>>>> I wrote up a more thorough supporting doc here as well - -
>>>>>> https://docs.google.com/document/d/15CILeNjNxCnbigSvxNq4eXPj6x6sn5DGdbTdWu55kCI/edit?usp=sharing
>>>>>>
>>>>>> *Disclaimer*
>>>>>>
>>>>>> It's really hard to empirically prove any of this after the fact - I
>>>>>> think there's enough evidence to try it, but we should be ready to engage
>>>>>> Infra to restart Jenkins and ready to revert any triggers we add.
>>>>>>
>>>>>> Thanks,
>>>>>> Danny
>>>>>>
>>>>>> [1] Context on previous investigation here -
>>>>>> https://docs.google.com/document/d/10qyUsvB_uVy5jftfTiwohlvN8Qwix5AuadssyoC4JsE/edit?usp=sharing
>>>>>>
>>>>>
Re: [Proposal] Path to Re-enabling Build Comment Triggers
Posted by Danny McCormick via dev <de...@beam.apache.org>.
Hey Yi, that's a good idea. Unfortunately, though, there is not a good way
to do it without modifying the plugin itself (not just the configuration).
Modifying the plugin is theoretically an option, but it is not easy -
finding a way to build/deploy it is hard, it doesn't build out of the box
right now, and it hasn't been touched in 2 years. If we're going to invest
the time, I'd vote we either use that time to move off of the plugin
entirely or move all our Jenkins infra to GitHub Actions (I'd vote to move
to GHA) - both of those require a lot of effort and are out of the scope of
this thread.
It is worth being super clear here - anything we do that doesn't involve
extensively patching the plugin or moving off of it entirely is a band-aid.
As the community grows and our GitHub automation grows (as Yi pointed out)
this will continue to be an issue. This change should buy us a significant
amount of time though.
Thanks,
Danny
On Fri, Jul 1, 2022 at 11:12 AM Yi Hu via dev <de...@beam.apache.org> wrote:
> Thanks for the investigation done and disabling the "admins verify patch"
> comment. Wondering if there is a way to early return the bot comments for
> ghprb (like blacklist)? Besides asf-ci there are also comments
> from github-actions (e.g. assign reviewer or "stopping review
> notifications"). In the future if we enable more github action items there
> might be more bot pr comments.
>
> Best,
> Yi
>
> On Fri, Jul 1, 2022 at 8:33 AM Danny McCormick via dev <
> dev@beam.apache.org> wrote:
>
>> Given the early consensus here, I tried disabling the "Can one of the
>> admins verify this patch?" messages, and verified that it worked. If you
>> disagree with that decision, please let me know - I expect that is the most
>> popular part of this proposal though :)
>>
>> The remaining questions are:
>> 1) Are we comfortable adding trusted repeat contributors to the Jenkins
>> allow-list?
>> 2) Are we ok trying to enable build triggers?
>>
>> Given that disabling the "Can one of the admins verify this patch?"
>> messages worked, I'm not sure that we need to do (1) - I think it would
>> help reduce load from the plugin a little bit, but is probably not as
>> necessary or helpful as what we've already done. I'm pretty comfortable
>> skipping that step.
>>
>> *If there are no objections, I'd like to try reenabling build comment
>> triggers next week.* Because I'll be offline for the first half of next
>> week for the American 4th of July holiday, I will plan on trying it next
>> Wednesday or Thursday if there are no objections.
>>
>> Thanks,
>> Danny
>>
>> On Thu, Jun 30, 2022 at 2:54 PM Pablo Estrada <pa...@google.com> wrote:
>>
>>> Agreed! I never know what to do about them : )
>>>
>>> On Thu, Jun 30, 2022 at 10:58 AM Robert Burke <ro...@frantil.com>
>>> wrote:
>>>
>>>> +1 to get rid of the "admins very patch" comments if we can. They add
>>>> no value at all when commented in quadruplicate immediately after PR
>>>> creation.
>>>>
>>>> On Thu, Jun 30, 2022, 8:40 AM Danny McCormick <
>>>> dannymccormick@google.com> wrote:
>>>>
>>>>> Hey everyone, I've been digging into the issues we've been having with
>>>>> Jenkins recently[1] which led to us disabling many of our build
>>>>> comment triggers (e.g. "Run <job name>" triggering a Jenkins job). Moving
>>>>> forward, I'd like to recommend that we:
>>>>>
>>>>> 1) Try to disable the "Can one of the admins verify this patch?"
>>>>> comments via the Jenkins plugin configuration.
>>>>> 2) Add trusted repeat contributors to the Jenkins allow-list.
>>>>> 3) Try re-enabling all build triggers.
>>>>>
>>>>> *Justification*
>>>>>
>>>>> Right now, around 33% of our PR comments are "Can one of the admins
>>>>> verify this patch?". Aside from being (IMO) very unhelpful and
>>>>> annoying, these are actually causing a significant amount of load on the
>>>>> ghprb plugin and indicate that these PRs are more expensive than those from
>>>>> allow-listed contributors. Since we believe that the ghprb plugin makes
>>>>> calls to GitHub that are roughly proportional to <the number of pr
>>>>> comments>X<the number of build triggers>, reducing our number of issue
>>>>> comments and the load per comment should give us enough breathing room to
>>>>> enable our triggers again.
>>>>>
>>>>> I wrote up a more thorough supporting doc here as well - -
>>>>> https://docs.google.com/document/d/15CILeNjNxCnbigSvxNq4eXPj6x6sn5DGdbTdWu55kCI/edit?usp=sharing
>>>>>
>>>>> *Disclaimer*
>>>>>
>>>>> It's really hard to empirically prove any of this after the fact - I
>>>>> think there's enough evidence to try it, but we should be ready to engage
>>>>> Infra to restart Jenkins and ready to revert any triggers we add.
>>>>>
>>>>> Thanks,
>>>>> Danny
>>>>>
>>>>> [1] Context on previous investigation here -
>>>>> https://docs.google.com/document/d/10qyUsvB_uVy5jftfTiwohlvN8Qwix5AuadssyoC4JsE/edit?usp=sharing
>>>>>
>>>>
Re: [Proposal] Path to Re-enabling Build Comment Triggers
Posted by Yi Hu via dev <de...@beam.apache.org>.
Thanks for the investigation done and disabling the "admins verify patch"
comment. Wondering if there is a way to early return the bot comments for
ghprb (like blacklist)? Besides asf-ci there are also comments
from github-actions (e.g. assign reviewer or "stopping review
notifications"). In the future if we enable more github action items there
might be more bot pr comments.
Best,
Yi
On Fri, Jul 1, 2022 at 8:33 AM Danny McCormick via dev <de...@beam.apache.org>
wrote:
> Given the early consensus here, I tried disabling the "Can one of the
> admins verify this patch?" messages, and verified that it worked. If you
> disagree with that decision, please let me know - I expect that is the most
> popular part of this proposal though :)
>
> The remaining questions are:
> 1) Are we comfortable adding trusted repeat contributors to the Jenkins
> allow-list?
> 2) Are we ok trying to enable build triggers?
>
> Given that disabling the "Can one of the admins verify this patch?"
> messages worked, I'm not sure that we need to do (1) - I think it would
> help reduce load from the plugin a little bit, but is probably not as
> necessary or helpful as what we've already done. I'm pretty comfortable
> skipping that step.
>
> *If there are no objections, I'd like to try reenabling build comment
> triggers next week.* Because I'll be offline for the first half of next
> week for the American 4th of July holiday, I will plan on trying it next
> Wednesday or Thursday if there are no objections.
>
> Thanks,
> Danny
>
> On Thu, Jun 30, 2022 at 2:54 PM Pablo Estrada <pa...@google.com> wrote:
>
>> Agreed! I never know what to do about them : )
>>
>> On Thu, Jun 30, 2022 at 10:58 AM Robert Burke <ro...@frantil.com> wrote:
>>
>>> +1 to get rid of the "admins very patch" comments if we can. They add no
>>> value at all when commented in quadruplicate immediately after PR creation.
>>>
>>> On Thu, Jun 30, 2022, 8:40 AM Danny McCormick <da...@google.com>
>>> wrote:
>>>
>>>> Hey everyone, I've been digging into the issues we've been having with
>>>> Jenkins recently[1] which led to us disabling many of our build
>>>> comment triggers (e.g. "Run <job name>" triggering a Jenkins job). Moving
>>>> forward, I'd like to recommend that we:
>>>>
>>>> 1) Try to disable the "Can one of the admins verify this patch?"
>>>> comments via the Jenkins plugin configuration.
>>>> 2) Add trusted repeat contributors to the Jenkins allow-list.
>>>> 3) Try re-enabling all build triggers.
>>>>
>>>> *Justification*
>>>>
>>>> Right now, around 33% of our PR comments are "Can one of the admins
>>>> verify this patch?". Aside from being (IMO) very unhelpful and
>>>> annoying, these are actually causing a significant amount of load on the
>>>> ghprb plugin and indicate that these PRs are more expensive than those from
>>>> allow-listed contributors. Since we believe that the ghprb plugin makes
>>>> calls to GitHub that are roughly proportional to <the number of pr
>>>> comments>X<the number of build triggers>, reducing our number of issue
>>>> comments and the load per comment should give us enough breathing room to
>>>> enable our triggers again.
>>>>
>>>> I wrote up a more thorough supporting doc here as well - -
>>>> https://docs.google.com/document/d/15CILeNjNxCnbigSvxNq4eXPj6x6sn5DGdbTdWu55kCI/edit?usp=sharing
>>>>
>>>> *Disclaimer*
>>>>
>>>> It's really hard to empirically prove any of this after the fact - I
>>>> think there's enough evidence to try it, but we should be ready to engage
>>>> Infra to restart Jenkins and ready to revert any triggers we add.
>>>>
>>>> Thanks,
>>>> Danny
>>>>
>>>> [1] Context on previous investigation here -
>>>> https://docs.google.com/document/d/10qyUsvB_uVy5jftfTiwohlvN8Qwix5AuadssyoC4JsE/edit?usp=sharing
>>>>
>>>
Re: [Proposal] Path to Re-enabling Build Comment Triggers
Posted by Danny McCormick via dev <de...@beam.apache.org>.
Given the early consensus here, I tried disabling the "Can one of the
admins verify this patch?" messages, and verified that it worked. If you
disagree with that decision, please let me know - I expect that is the most
popular part of this proposal though :)
The remaining questions are:
1) Are we comfortable adding trusted repeat contributors to the Jenkins
allow-list?
2) Are we ok trying to enable build triggers?
Given that disabling the "Can one of the admins verify this patch?"
messages worked, I'm not sure that we need to do (1) - I think it would
help reduce load from the plugin a little bit, but is probably not as
necessary or helpful as what we've already done. I'm pretty comfortable
skipping that step.
*If there are no objections, I'd like to try reenabling build comment
triggers next week.* Because I'll be offline for the first half of next
week for the American 4th of July holiday, I will plan on trying it next
Wednesday or Thursday if there are no objections.
Thanks,
Danny
On Thu, Jun 30, 2022 at 2:54 PM Pablo Estrada <pa...@google.com> wrote:
> Agreed! I never know what to do about them : )
>
> On Thu, Jun 30, 2022 at 10:58 AM Robert Burke <ro...@frantil.com> wrote:
>
>> +1 to get rid of the "admins very patch" comments if we can. They add no
>> value at all when commented in quadruplicate immediately after PR creation.
>>
>> On Thu, Jun 30, 2022, 8:40 AM Danny McCormick <da...@google.com>
>> wrote:
>>
>>> Hey everyone, I've been digging into the issues we've been having with
>>> Jenkins recently[1] which led to us disabling many of our build
>>> comment triggers (e.g. "Run <job name>" triggering a Jenkins job). Moving
>>> forward, I'd like to recommend that we:
>>>
>>> 1) Try to disable the "Can one of the admins verify this patch?"
>>> comments via the Jenkins plugin configuration.
>>> 2) Add trusted repeat contributors to the Jenkins allow-list.
>>> 3) Try re-enabling all build triggers.
>>>
>>> *Justification*
>>>
>>> Right now, around 33% of our PR comments are "Can one of the admins
>>> verify this patch?". Aside from being (IMO) very unhelpful and
>>> annoying, these are actually causing a significant amount of load on the
>>> ghprb plugin and indicate that these PRs are more expensive than those from
>>> allow-listed contributors. Since we believe that the ghprb plugin makes
>>> calls to GitHub that are roughly proportional to <the number of pr
>>> comments>X<the number of build triggers>, reducing our number of issue
>>> comments and the load per comment should give us enough breathing room to
>>> enable our triggers again.
>>>
>>> I wrote up a more thorough supporting doc here as well - -
>>> https://docs.google.com/document/d/15CILeNjNxCnbigSvxNq4eXPj6x6sn5DGdbTdWu55kCI/edit?usp=sharing
>>>
>>> *Disclaimer*
>>>
>>> It's really hard to empirically prove any of this after the fact - I
>>> think there's enough evidence to try it, but we should be ready to engage
>>> Infra to restart Jenkins and ready to revert any triggers we add.
>>>
>>> Thanks,
>>> Danny
>>>
>>> [1] Context on previous investigation here -
>>> https://docs.google.com/document/d/10qyUsvB_uVy5jftfTiwohlvN8Qwix5AuadssyoC4JsE/edit?usp=sharing
>>>
>>
Re: [Proposal] Path to Re-enabling Build Comment Triggers
Posted by Pablo Estrada <pa...@google.com>.
Agreed! I never know what to do about them : )
On Thu, Jun 30, 2022 at 10:58 AM Robert Burke <ro...@frantil.com> wrote:
> +1 to get rid of the "admins very patch" comments if we can. They add no
> value at all when commented in quadruplicate immediately after PR creation.
>
> On Thu, Jun 30, 2022, 8:40 AM Danny McCormick <da...@google.com>
> wrote:
>
>> Hey everyone, I've been digging into the issues we've been having with
>> Jenkins recently[1] which led to us disabling many of our build
>> comment triggers (e.g. "Run <job name>" triggering a Jenkins job). Moving
>> forward, I'd like to recommend that we:
>>
>> 1) Try to disable the "Can one of the admins verify this patch?"
>> comments via the Jenkins plugin configuration.
>> 2) Add trusted repeat contributors to the Jenkins allow-list.
>> 3) Try re-enabling all build triggers.
>>
>> *Justification*
>>
>> Right now, around 33% of our PR comments are "Can one of the admins
>> verify this patch?". Aside from being (IMO) very unhelpful and annoying,
>> these are actually causing a significant amount of load on the ghprb plugin
>> and indicate that these PRs are more expensive than those from allow-listed
>> contributors. Since we believe that the ghprb plugin makes calls to GitHub
>> that are roughly proportional to <the number of pr comments>X<the number of
>> build triggers>, reducing our number of issue comments and the load per
>> comment should give us enough breathing room to enable our triggers again.
>>
>> I wrote up a more thorough supporting doc here as well - -
>> https://docs.google.com/document/d/15CILeNjNxCnbigSvxNq4eXPj6x6sn5DGdbTdWu55kCI/edit?usp=sharing
>>
>> *Disclaimer*
>>
>> It's really hard to empirically prove any of this after the fact - I
>> think there's enough evidence to try it, but we should be ready to engage
>> Infra to restart Jenkins and ready to revert any triggers we add.
>>
>> Thanks,
>> Danny
>>
>> [1] Context on previous investigation here -
>> https://docs.google.com/document/d/10qyUsvB_uVy5jftfTiwohlvN8Qwix5AuadssyoC4JsE/edit?usp=sharing
>>
>
Re: [Proposal] Path to Re-enabling Build Comment Triggers
Posted by Robert Burke <ro...@frantil.com>.
+1 to get rid of the "admins very patch" comments if we can. They add no
value at all when commented in quadruplicate immediately after PR creation.
On Thu, Jun 30, 2022, 8:40 AM Danny McCormick <da...@google.com>
wrote:
> Hey everyone, I've been digging into the issues we've been having with
> Jenkins recently[1] which led to us disabling many of our build
> comment triggers (e.g. "Run <job name>" triggering a Jenkins job). Moving
> forward, I'd like to recommend that we:
>
> 1) Try to disable the "Can one of the admins verify this patch?" comments
> via the Jenkins plugin configuration.
> 2) Add trusted repeat contributors to the Jenkins allow-list.
> 3) Try re-enabling all build triggers.
>
> *Justification*
>
> Right now, around 33% of our PR comments are "Can one of the admins
> verify this patch?". Aside from being (IMO) very unhelpful and annoying,
> these are actually causing a significant amount of load on the ghprb plugin
> and indicate that these PRs are more expensive than those from allow-listed
> contributors. Since we believe that the ghprb plugin makes calls to GitHub
> that are roughly proportional to <the number of pr comments>X<the number of
> build triggers>, reducing our number of issue comments and the load per
> comment should give us enough breathing room to enable our triggers again.
>
> I wrote up a more thorough supporting doc here as well - -
> https://docs.google.com/document/d/15CILeNjNxCnbigSvxNq4eXPj6x6sn5DGdbTdWu55kCI/edit?usp=sharing
>
> *Disclaimer*
>
> It's really hard to empirically prove any of this after the fact - I think
> there's enough evidence to try it, but we should be ready to engage Infra
> to restart Jenkins and ready to revert any triggers we add.
>
> Thanks,
> Danny
>
> [1] Context on previous investigation here -
> https://docs.google.com/document/d/10qyUsvB_uVy5jftfTiwohlvN8Qwix5AuadssyoC4JsE/edit?usp=sharing
>