Email based retest request process: proposal for new pull/re-apply feature

Email based retest request process: proposal for new pull/re-apply feature

[Patrick Robb]

[-- Attachment #1: Type: text/plain, Size: 1990 bytes --]

Hi all,

I want to poll the CI group and dev community about a proposed feature
addition to the CI retest request framework. Currently, you can respond to
a patchseries or patch email, requesting a retest like so:

Recheck-request: iol-compile-amd64-testing, iol-unit-amd64-testing

Labs who have added this functionality (UNH and the GitHub Robot) will then
trigger retests according to the contexts provided, using the ORIGINAL dpdk
artifact they produced at the time when the patch was submitted.

This is useful for requesting a retest on a patch when you believe a
failure may have been an infra failure or spurious. It is not useful if you
believe the tree your patch was applied on was in a bad state when your
patch was submitted, and you would like for your patch to be re-applied on
the current tip of the branch. A few people have suggested we add
this feature (re-apply to tip of branch and retest). So, we should probably
add an option allowing people to indicate they want this behavior
instead of the "default" retest.

Ferruh emailed me about this a while ago and proposed the following syntax,
which I am okay with:

Recheck-request,attribute=value: ...

So a practical example would look like:

Recheck-request,pull=True: iol-sample-apps-testing,
iol-compile-amd64-testing, github-robot

Also, I believe that although we should still require people to include the
contexts they're requesting a retest for for posterity (so we can refer
back to it), I think if someone includes the pull keyword, ALL labs should
trigger retests for ALL tests. The reason for this is I don't think we
should display results side by side on a patchseries which are coming from
distinct DPDK artifacts. Readers may assume (rightly, in my opinion) that
when they're looking at a results table for a patchseries, those results
are all coming from identical DPDK artifacts, and not from distinct DPDK
artifacts produced at different times, from different commits.

What do you all think? Thanks.

[-- Attachment #2: Type: text/html, Size: 2178 bytes --]

Re: Email based retest request process: proposal for new pull/re-apply feature

[Aaron Conole]

Patrick Robb <probb@iol.unh.edu> writes:

> Hi all,
>
> I want to poll the CI group and dev community about a proposed feature addition to the CI retest request framework.
> Currently, you can respond to a patchseries or patch email, requesting a retest like so:
>
> Recheck-request: iol-compile-amd64-testing, iol-unit-amd64-testing    
>
> Labs who have added this functionality (UNH and the GitHub Robot) will then trigger retests according to the contexts
> provided, using the ORIGINAL dpdk artifact they produced at the time when the patch was submitted. 
>
> This is useful for requesting a retest on a patch when you believe a failure may have been an infra failure or spurious. It
> is not useful if you believe the tree your patch was applied on was in a bad state when your patch was submitted, and
> you would like for your patch to be re-applied on the current tip of the branch. A few people have suggested we add
> this feature (re-apply to tip of branch and retest). So, we should probably add an option allowing people to indicate they
> want this behavior instead of the "default" retest. 
>
> Ferruh emailed me about this a while ago and proposed the following syntax, which I am okay with:
>
> Recheck-request,attribute=value: ...
>
> So a practical example would look like:
>
> Recheck-request,pull=True: iol-sample-apps-testing, iol-compile-amd64-testing, github-robot
>
> Also, I believe that although we should still require people to include the contexts they're requesting a retest for for
> posterity (so we can refer back to it), I think if someone includes the pull keyword, ALL labs should trigger retests for
> ALL tests. The reason for this is I don't think we should display results side by side on a patchseries which are coming
> from distinct DPDK artifacts. Readers may assume (rightly, in my opinion) that when they're looking at a results table
> for a patchseries, those results are all coming from identical DPDK artifacts, and not from distinct DPDK artifacts
> produced at different times, from different commits.
>
> What do you all think? Thanks.

Why not something like:

Recheck-request: [attribute-list],[test-list]...

For example, then we can do:

Recheck-request: rebase=[identifier],....

where identifier is a branch specifier (or the word 'latest')?

That lets us fixup if the branch picker script guessed a wrong branch.

Just spit-balling on syntax.


That said, I agree - if a rebase has been requested, all tests need to
be rerun.  Maybe we should consider that the test labels should be added
with a run number or something?  Or we could also include that the run
is a rerun.  That way for labs that don't currently support the recheck
request framework, we can easily tell that they weren't re-tried.

Re: Email based retest request process: proposal for new pull/re-apply feature

[Patrick Robb]

[-- Attachment #1: Type: text/plain, Size: 1805 bytes --]

On Tue, Feb 20, 2024 at 1:12 PM Aaron Conole <aconole@redhat.com> wrote:

>
> Why not something like:
>
> Recheck-request: [attribute-list],[test-list]...
>
> For example, then we can do:
>
> Recheck-request: rebase=[identifier],....
>
> where identifier is a branch specifier (or the word 'latest')?
>
I hadn't thought about the option of allowing branch specifiers. Agree that
allowing a human correction process for the pw_maintainer_cli.py script
choosing the wrong branch sounds helpful.

My original idea was offering 2 options (test original artifact, or
re-apply on latest). Do we want to support for checking out to a specific
commit and re-applying there? I figured that would not be worth it (too
much of a niche case), but your comments are making me reconsider.


>
> That lets us fixup if the branch picker script guessed a wrong branch.
>
> Just spit-balling on syntax.
>
>
> That said, I agree - if a rebase has been requested, all tests need to
> be rerun.  Maybe we should consider that the test labels should be added
> with a run number or something?  Or we could also include that the run
> is a rerun.  That way for labs that don't currently support the recheck
> request framework, we can easily tell that they weren't re-tried.
>
> so re-report with a modified test label? That is good in that it shows the
behavior more clearly. But, it also means we will not overwrite any fails.
So the fail will still be there, and the patchwork patch page will grow a
huge table. Maybe this is fine.

Also raises the point of getting more coverage for the retest framework at
other labs. I will email Min Zhou regarding how he uses the dpdk-ci project
for the loongson build jobs and see how well that can integrate with the
get_reruns.py script.

[-- Attachment #2: Type: text/html, Size: 2405 bytes --]

Re: Email based retest request process: proposal for new pull/re-apply feature

[zhoumin]

[-- Attachment #1: Type: text/plain, Size: 2800 bytes --]


On Wed, Feb 21, 2024 at 2:24AM, Patrick Robb wrote:
>
> On Tue, Feb 20, 2024 at 1:12 PM Aaron Conole <aconole@redhat.com 
> <mailto:aconole@redhat.com>> wrote:
>
>
>     Why not something like:
>
>     Recheck-request: [attribute-list],[test-list]...
>
>     For example, then we can do:
>
>     Recheck-request: rebase=[identifier],....
>
>     where identifier is a branch specifier (or the word 'latest')?
>
> I hadn't thought about the option of allowing branch specifiers. Agree 
> that allowing a human correction process for the pw_maintainer_cli.py 
> script choosing the wrong branch sounds helpful.
>
> My original idea was offering 2 options (test original artifact, or 
> re-apply on latest). Do we want to support for checking out to a 
> specific commit and re-applying there? I figured that would not be 
> worth it (too much of a niche case), but your comments are making me 
> reconsider.

I agree with you that allowing developers to correct the target branch 
is useful. But, the developer should just provide the name of branch 
instead of commit ID, which is more reasonable. Of course, the rebasing 
option is more important. So, I consider we can allow developers to 
submit a request as following format:

Recheck-request: 
rebase=True|branch=main|contexts=iol-compile-amd64-testing, 
iol-broadcom-Performance,...

We can use "|" as the separator, for example. `rebase` and `branch` can 
be optional and we can use the default values if the developer doesn't 
provide them. The default is not rebasing for `rebase` option. The 
default is the branch chosen by pw_maintainer_cli.py script for `branch` 
option. The `contexts` option is required.

>
>     Just spit-balling on syntax.
>
>
>     That said, I agree - if a rebase has been requested, all tests need to
>     be rerun.  Maybe we should consider that the test labels should be
>     added
>     with a run number or something?  Or we could also include that the run
>     is a rerun.  That way for labs that don't currently support the
>     recheck
>     request framework, we can easily tell that they weren't re-tried.
>
> so re-report with a modified test label? That is good in that it shows 
> the behavior more clearly. But, it also means we will not overwrite 
> any fails. So the fail will still be there, and the patchwork patch 
> page will grow a huge table. Maybe this is fine.
>
Re-report with a modified test label may be better. That can tell people 
more information about the CI testings, such as that the retest indeed 
happened.
> Also raises the point of getting more coverage for the retest 
> framework at other labs. I will email Min Zhou regarding how he uses 
> the dpdk-ci project for the loongson build jobs and see how well that 
> can integrate with the get_reruns.py script.
>

[-- Attachment #2: Type: text/html, Size: 4898 bytes --]

Re: Email based retest request process: proposal for new pull/re-apply feature

[Aaron Conole]

zhoumin <zhoumin@loongson.cn> writes:

> On Wed, Feb 21, 2024 at 2:24AM, Patrick Robb wrote:
>
>  On Tue, Feb 20, 2024 at 1:12 PM Aaron Conole <aconole@redhat.com> wrote:
>
>  Why not something like:
>
>  Recheck-request: [attribute-list],[test-list]...
>
>  For example, then we can do:
>
>  Recheck-request: rebase=[identifier],....
>
>  where identifier is a branch specifier (or the word 'latest')?
>
>  I hadn't thought about the option of allowing branch specifiers. Agree that allowing a human correction process for
>  the pw_maintainer_cli.py script choosing the wrong branch sounds helpful.
>
>  My original idea was offering 2 options (test original artifact, or re-apply on latest). Do we want to support for
>  checking out to a specific commit and re-applying there? I figured that would not be worth it (too much of a niche
>  case), but your comments are making me reconsider. 
>
> I agree with you that allowing developers to correct the target branch is useful. But, the developer should just provide
> the name of branch instead of commit ID, which is more reasonable. Of course, the rebasing option is more important.
> So, I consider we can allow developers to submit a request as following format:
>
> Recheck-request: rebase=True|branch=main|contexts=iol-compile-amd64-testing, iol-broadcom-Performance,...
>
> We can use "|" as the separator, for example. `rebase` and `branch` can be optional and we can use the default values
> if the developer doesn't provide them. The default is not rebasing for `rebase` option. The default is the branch chosen
> by pw_maintainer_cli.py script for `branch` option. The `contexts` option is required.

Interesting approach.  But I don't know about contexts= or something
like that.  It means there are two passes through the regex.

Also, I don't know about contexts either - if the series was requested
to rebase, every lab that can re-test probably should since the results
aren't going to be valid from the old tests.

>  Just spit-balling on syntax.
>
>  That said, I agree - if a rebase has been requested, all tests need to
>  be rerun.  Maybe we should consider that the test labels should be added
>  with a run number or something?  Or we could also include that the run
>  is a rerun.  That way for labs that don't currently support the recheck
>  request framework, we can easily tell that they weren't re-tried.
>
>  so re-report with a modified test label? That is good in that it shows the behavior more clearly. But, it also means
>  we will not overwrite any fails. So the fail will still be there, and the patchwork patch page will grow a huge table.
>  Maybe this is fine.  
>
> Re-report with a modified test label may be better. That can tell people more information about the CI testings, such as
> that the retest indeed happened.

Just back from PTO - actually, I don't think we need to adjust the
label, but rather the description.  That would allow the mechanism that
overwrites the existing test to keep the "checks" page tidy, but also
making the retest information clear.  WDYT?

>  Also raises the point of getting more coverage for the retest framework at other labs. I will email Min Zhou
>  regarding how he uses the dpdk-ci project for the loongson build jobs and see how well that can integrate with the
>  get_reruns.py script.

That would be great!

Re: Email based retest request process: proposal for new pull/re-apply feature

[Adam Hassick]

On Mon, Mar 4, 2024 at 10:22 AM Aaron Conole <aconole@redhat.com> wrote:
>
> zhoumin <zhoumin@loongson.cn> writes:
>
> > On Wed, Feb 21, 2024 at 2:24AM, Patrick Robb wrote:
> >
> >  On Tue, Feb 20, 2024 at 1:12 PM Aaron Conole <aconole@redhat.com> wrote:
> >
> >  Why not something like:
> >
> >  Recheck-request: [attribute-list],[test-list]...
> >
> >  For example, then we can do:
> >
> >  Recheck-request: rebase=[identifier],....
> >
> >  where identifier is a branch specifier (or the word 'latest')?
> >
> >  I hadn't thought about the option of allowing branch specifiers. Agree that allowing a human correction process for
> >  the pw_maintainer_cli.py script choosing the wrong branch sounds helpful.
> >
> >  My original idea was offering 2 options (test original artifact, or re-apply on latest). Do we want to support for
> >  checking out to a specific commit and re-applying there? I figured that would not be worth it (too much of a niche
> >  case), but your comments are making me reconsider.
> >
> > I agree with you that allowing developers to correct the target branch is useful. But, the developer should just provide
> > the name of branch instead of commit ID, which is more reasonable. Of course, the rebasing option is more important.
> > So, I consider we can allow developers to submit a request as following format:
> >
> > Recheck-request: rebase=True|branch=main|contexts=iol-compile-amd64-testing, iol-broadcom-Performance,...
> >
> > We can use "|" as the separator, for example. `rebase` and `branch` can be optional and we can use the default values
> > if the developer doesn't provide them. The default is not rebasing for `rebase` option. The default is the branch chosen
> > by pw_maintainer_cli.py script for `branch` option. The `contexts` option is required.
>
> Interesting approach.  But I don't know about contexts= or something
> like that.  It means there are two passes through the regex.
>
> Also, I don't know about contexts either - if the series was requested
> to rebase, every lab that can re-test probably should since the results
> aren't going to be valid from the old tests.

I'm not opposed to having the contexts be a key-value pair argument
like the others, however that does break backwards compatibility with
our existing syntax. If we don't care very much about backwards
compatibility, then we could make this change.

Instead of having a boolean and a string parameter for whether to
rebase and the branch to rebase on, we could have a single argument
specifying a branch. Then, labs rebase on the given branch and then
rerun all tests if the "rebase=<branch>" argument is present. This
would look like:

Recheck-request: rebase=main, iol-sample-apps-testing,
iol-unit-amd64-testing, iol-broadcom-Performance

Or, using zhoumin's proposed format:

Recheck-request: rebase=main|contexts=iol-sample-apps-testing,
iol-unit-amd64-testing, iol-broadcom-Performance

I don't think the context should be required if the request includes
the rebase argument, because we do not want to mix valid and invalid
test results as Aaron said.
This would be a valid format if contexts are optional:

Recheck-request: rebase=main

> >  Just spit-balling on syntax.
> >
> >  That said, I agree - if a rebase has been requested, all tests need to
> >  be rerun.  Maybe we should consider that the test labels should be added
> >  with a run number or something?  Or we could also include that the run
> >  is a rerun.  That way for labs that don't currently support the recheck
> >  request framework, we can easily tell that they weren't re-tried.
> >
> >  so re-report with a modified test label? That is good in that it shows the behavior more clearly. But, it also means
> >  we will not overwrite any fails. So the fail will still be there, and the patchwork patch page will grow a huge table.
> >  Maybe this is fine.
> >
> > Re-report with a modified test label may be better. That can tell people more information about the CI testings, such as
> > that the retest indeed happened.
>
> Just back from PTO - actually, I don't think we need to adjust the
> label, but rather the description.  That would allow the mechanism that
> overwrites the existing test to keep the "checks" page tidy, but also
> making the retest information clear.  WDYT?

Yes, if the Community Lab posted new contexts for every retest then
the checks page would get quite long.

> >  Also raises the point of getting more coverage for the retest framework at other labs. I will email Min Zhou
> >  regarding how he uses the dpdk-ci project for the loongson build jobs and see how well that can integrate with the
> >  get_reruns.py script.
>
> That would be great!
>

Re: Email based retest request process: proposal for new pull/re-apply feature

[Patrick Robb]

On Thu, Mar 7, 2024 at 12:06 PM Adam Hassick <ahassick@iol.unh.edu> wrote:
>
>
> I'm not opposed to having the contexts be a key-value pair argument
> like the others, however that does break backwards compatibility with
> our existing syntax. If we don't care very much about backwards
> compatibility, then we could make this change.
>
> Instead of having a boolean and a string parameter for whether to
> rebase and the branch to rebase on, we could have a single argument
> specifying a branch. Then, labs rebase on the given branch and then
> rerun all tests if the "rebase=<branch>" argument is present. This
> would look like:
>
> Recheck-request: rebase=main, iol-sample-apps-testing,
> iol-unit-amd64-testing, iol-broadcom-Performance

I agree with this approach because it preserves backward
compatibility, while still providing us with all the functionality we
need. We will also be able to accept key value arguments in the future
if further feature requests come in which require it.

> I don't think the context should be required if the request includes
> the rebase argument, because we do not want to mix valid and invalid
> test results as Aaron said.
> This would be a valid format if contexts are optional:
>
> Recheck-request: rebase=main

Okay, I agree that contexts should not be considered by labs when we
use rebase - but of course we will still store the contexts (if they
are submitted) alongside the key value args. In the future there may
be an application for this.

Zhoumin, does this sound acceptable, or do you think there are any
flaws? If it works, we will implement the updates and try to upstream
this week. Thanks!

Re: Email based retest request process: proposal for new pull/re-apply feature

[zhoumin]

On Mon, Mar 18, 2024 at 3:59PM, Patrick Robb wrote:
> On Thu, Mar 7, 2024 at 12:06 PM Adam Hassick <ahassick@iol.unh.edu> wrote:
>>
>> I'm not opposed to having the contexts be a key-value pair argument
>> like the others, however that does break backwards compatibility with
>> our existing syntax. If we don't care very much about backwards
>> compatibility, then we could make this change.
>>
>> Instead of having a boolean and a string parameter for whether to
>> rebase and the branch to rebase on, we could have a single argument
>> specifying a branch. Then, labs rebase on the given branch and then
>> rerun all tests if the "rebase=<branch>" argument is present. This
>> would look like:
>>
>> Recheck-request: rebase=main, iol-sample-apps-testing,
>> iol-unit-amd64-testing, iol-broadcom-Performance
> I agree with this approach because it preserves backward
> compatibility, while still providing us with all the functionality we
> need. We will also be able to accept key value arguments in the future
> if further feature requests come in which require it.
>
>> I don't think the context should be required if the request includes
>> the rebase argument, because we do not want to mix valid and invalid
>> test results as Aaron said.
>> This would be a valid format if contexts are optional:
>>
>> Recheck-request: rebase=main
> Okay, I agree that contexts should not be considered by labs when we
> use rebase - but of course we will still store the contexts (if they
> are submitted) alongside the key value args. In the future there may
> be an application for this.
>
> Zhoumin, does this sound acceptable, or do you think there are any
> flaws? If it works, we will implement the updates and try to upstream
> this week. Thanks!

Thanks for your hard work.

I also agree with this approach. The meaning of the key value 
`rebase=main` is sufficient, and loongson lab can support it.

One more thing I want to confirm is whether we should apply the patch 
onto the branch commit which existed at the time when that patch was 
submitted or onto the latest tip of branch if users request doing 
rebase. Users probably request a recheck with `rebase` when the CI lab 
chose a wrong branch onto which apply the patch. I worry we may 
encounter conflicts when apply the patch onto the latest commit of the 
target branch if that branch is just updated before the request.

Re: Email based retest request process: proposal for new pull/re-apply feature

[Patrick Robb]

On Tue, Mar 19, 2024 at 4:37 AM zhoumin <zhoumin@loongson.cn> wrote:
>
>
> One more thing I want to confirm is whether we should apply the patch
> onto the branch commit which existed at the time when that patch was
> submitted or onto the latest tip of branch if users request doing
> rebase. Users probably request a recheck with `rebase` when the CI lab
> chose a wrong branch onto which apply the patch. I worry we may
> encounter conflicts when apply the patch onto the latest commit of the
> target branch if that branch is just updated before the request.
>
>

That's a good edge case to think about...  but I also think if the
patch no longer applies cleanly on tip of intended branch, then we
would be correct to report an apply failure there. And then the
submitter should refactor their patch so it applies, and submit again.

So I think the process is like

A) If retest is requested without rebase key, then retest "original"
dpdk artifact (either by re-using the existing tarball (unh lab) or
tracking the commit from submit time and re-applying onto dpdk at that
commit (loongson)).

B) If rebase key is included, apply to tip of the indicated branch.
If, because the branch has changed, the patch no longer applies, then
we can report an apply failure. Then, submitter has to refactor their
patch and resubmit.

In either case, report the new results with an updated test result in
the email (i.e. report "_Testing PASS RETEST #1" instead of "_Testing
PASS" in the email body).

Re: Email based retest request process: proposal for new pull/re-apply feature

[Aaron Conole]

Patrick Robb <probb@iol.unh.edu> writes:

> On Tue, Mar 19, 2024 at 4:37 AM zhoumin <zhoumin@loongson.cn> wrote:
>>
>>
>> One more thing I want to confirm is whether we should apply the patch
>> onto the branch commit which existed at the time when that patch was
>> submitted or onto the latest tip of branch if users request doing
>> rebase. Users probably request a recheck with `rebase` when the CI lab
>> chose a wrong branch onto which apply the patch. I worry we may
>> encounter conflicts when apply the patch onto the latest commit of the
>> target branch if that branch is just updated before the request.
>>
>>
>
> That's a good edge case to think about...  but I also think if the
> patch no longer applies cleanly on tip of intended branch, then we
> would be correct to report an apply failure there. And then the
> submitter should refactor their patch so it applies, and submit again.

+1

> So I think the process is like
>
> A) If retest is requested without rebase key, then retest "original"
> dpdk artifact (either by re-using the existing tarball (unh lab) or
> tracking the commit from submit time and re-applying onto dpdk at that
> commit (loongson)).
>
> B) If rebase key is included, apply to tip of the indicated branch.
> If, because the branch has changed, the patch no longer applies, then
> we can report an apply failure. Then, submitter has to refactor their
> patch and resubmit.

That makes sense to me.

> In either case, report the new results with an updated test result in
> the email (i.e. report "_Testing PASS RETEST #1" instead of "_Testing
> PASS" in the email body).

Ack - makes sense here too.

Re: Email based retest request process: proposal for new pull/re-apply feature

[zhoumin]

On Tue, Mar 19, 2024 at 5:30PM, Patrick Robb wrote:
> On Tue, Mar 19, 2024 at 4:37 AM zhoumin <zhoumin@loongson.cn> wrote:
>>
>> One more thing I want to confirm is whether we should apply the patch
>> onto the branch commit which existed at the time when that patch was
>> submitted or onto the latest tip of branch if users request doing
>> rebase. Users probably request a recheck with `rebase` when the CI lab
>> chose a wrong branch onto which apply the patch. I worry we may
>> encounter conflicts when apply the patch onto the latest commit of the
>> target branch if that branch is just updated before the request.
>>
>>
> That's a good edge case to think about...  but I also think if the
> patch no longer applies cleanly on tip of intended branch, then we
> would be correct to report an apply failure there. And then the
> submitter should refactor their patch so it applies, and submit again.
Yes, it is more reasonable for submitter.
> So I think the process is like
>
> A) If retest is requested without rebase key, then retest "original"
> dpdk artifact (either by re-using the existing tarball (unh lab) or
> tracking the commit from submit time and re-applying onto dpdk at that
> commit (loongson)).
>
> B) If rebase key is included, apply to tip of the indicated branch.
> If, because the branch has changed, the patch no longer applies, then
> we can report an apply failure. Then, submitter has to refactor their
> patch and resubmit.
Thanks for making the applying process more clear.
> In either case, report the new results with an updated test result in
> the email (i.e. report "_Testing PASS RETEST #1" instead of "_Testing
> PASS" in the email body).
Yes, I agree with this approach and reporting a new title for the retest 
result is necessary.