From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 41A8442CA9 for ; Tue, 13 Jun 2023 15:28:13 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 3E0BD40A8A; Tue, 13 Jun 2023 15:28:13 +0200 (CEST) Received: from mail-oa1-f47.google.com (mail-oa1-f47.google.com [209.85.160.47]) by mails.dpdk.org (Postfix) with ESMTP id 1416840698 for ; Tue, 13 Jun 2023 15:28:12 +0200 (CEST) Received: by mail-oa1-f47.google.com with SMTP id 586e51a60fabf-1a69a593245so1881460fac.0 for ; Tue, 13 Jun 2023 06:28:12 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=iol.unh.edu; s=unh-iol; t=1686662891; x=1689254891; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=xaIZERhyhu8obDpcyydEB7W1RTl9oG6SKYA0eHhHKlQ=; b=MmeaGNSGE/Bp38tyOBAINPDDx4Man3HoFx5Fe1jbviXefHNH+XArP/Fqb/d9svore5 /BxHpSgRjMds4q6w9A2HKQxHYv+WPz7eXb1/USP5/52OqsF4R+vNtcoIgL0KbxA1EeBh aMCV5Td5b+ngfOGqVMAvHRTteS6SN08dluEFY= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1686662891; x=1689254891; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=xaIZERhyhu8obDpcyydEB7W1RTl9oG6SKYA0eHhHKlQ=; b=eYQW5gaG0Hj5je7vgRDrAvxaFxxU8dVgY6CaLZ5CIC0MMsI8uBSDqzqrnrAsNxzCcw /9XI21ZCViccWo9GmI3q2TPFN8SWYpfHoaqOI44mxYud0i5CepVYN9LLNJHh0OSsXcoN Axo5pJH+3vfk4ZhzkqQMwgRZ8FEgJlQb/S4NfEo98T3WPvx6XlIoyNP5Km3jOrQA4YyV cKj7Sgz+Nxg2vYKSALlN8eWCeSC2/6lIv2fJhX4PlT/phAftF+ImA523AzpRVh2Qujcb IcmJgqwf2dm16Yi092aCUBZR0JLlRXElIzOjGMIGhn2nxw1wdkiPqCvqodxm9/UCdyK3 QjxQ== X-Gm-Message-State: AC+VfDwq+JSsnmXd0YZAjvckSd69Uava5qVbrsQ4fHOpHBht38OzL0mQ MYs2IEIETF9+VLTstdt8YZ3XHs+eagwToaffM7WwEg== X-Google-Smtp-Source: ACHHUZ4a2x7eWcW+M3zmzJoJYedlzXxUOZWBJY1l7W5KZQPiA6Nln1M63bEiW1QJ4JxC9D0bSGzPUEtmzdxlR31bOGY= X-Received: by 2002:a05:6870:b791:b0:19e:fae6:c9ab with SMTP id ed17-20020a056870b79100b0019efae6c9abmr9594955oab.57.1686662891247; Tue, 13 Jun 2023 06:28:11 -0700 (PDT) MIME-Version: 1.0 References: <3fa6546b-8152-e317-30f0-30d5118b9fc4@amd.com> In-Reply-To: From: Patrick Robb Date: Tue, 13 Jun 2023 09:28:00 -0400 Message-ID: Subject: Re: Email Based Re-Testing Framework To: Aaron Conole Cc: Ferruh Yigit , ci@dpdk.org, "Tu, Lijuan" , zhoumin , Michael Santana , Lincoln Lavoie Content-Type: multipart/alternative; boundary="0000000000007d3f4605fe02cebc" X-BeenThere: ci@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK CI discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: ci-bounces@dpdk.org --0000000000007d3f4605fe02cebc Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable > > 1. Ephemeral lab/network failures, or flaky unit tests that sometimes > fail. In this case, we probably just want to re-run the tree as-is. > > 2. Failing tree before apply. In this case, we have applied the series > to a tree, but the tree isn't in a good state and will fail > regardless of the series being applied. > I had originally thought of this only as rerunning the tree as-is, though if the community wants the 2nd option to be available that works too. It does increase the scope of the task and complexity of the request text format to be understood for submitters. Personally, where I fall is that there isn't enough additional value to justify doing more than rerunning as-is. Plus, if we do end up with a bad tree, submitters can resubmit their patches when the tree is in a good state again, right? Or alternatively, lab managers may be aware of this situation and be able to order a rerun for the last x days (duration of bad tree) on the most up to date tree. On Mon, Jun 12, 2023 at 11:01=E2=80=AFAM Aaron Conole = wrote: > Patrick Robb writes: > > > On Wed, Jun 7, 2023 at 8:53=E2=80=AFAM Aaron Conole wrote: > > > > Patrick Robb writes: > > > > > Also it can be useful to run daily sub-tree testing by request, if > possible. > > > > > > That wouldn't be too difficult. I'll make a ticket for this. > Although, for testing on the daily sub-trees, > > since that's > > > UNH-IOL specific, that wouldn't necessarily have to be done via an > email based testing request > > framework. We > > > could also just add a button to our dashboard which triggers a > sub-tree ci run. That would help keep > > narrow > > > the scope of what the email based retesting framework is for. But, > both email or a dashboard button > > would > > > both work. > > > > We had discussed this long ago - including agreeing on a format, IIRC. > > > > See the thread starting here: > > https://mails.dpdk.org/archives/ci/2021-May/001189.html > > > > The idea was to have a line like: > > > > Recheck-request: > > > > I like using this simpler format which is easier to parse. As Thomas > pointed out, specifying labs does not really > > provide extra information if we are already going to request by > label/context, which is already specifies the > > lab. > > One thing we haven't discussed or determined is if we should have the > ability to re-apply the series or simply to rerun the patches based on > the original sha sum. There are two cases I can think of: > > 1. Ephemeral lab/network failures, or flaky unit tests that sometimes > fail. In this case, we probably just want to re-run the tree as-is. > > 2. Failing tree before apply. In this case, we have applied the series > to a tree, but the tree isn't in a good state and will fail > regardless of the series being applied. > > WDYT? Does (2) case warrant any consideration as a possible reason to > retest? If so, what is the right way of handling that situation? > > > where was the tests in the check labels. In fact, what > > started the discussion was a patch for the pw-ci scripts that > > implemented part of it. > > > > I don't see how to make your proposal as easily parsed. > > > > WDYT? Can you re-read that thread and come up with comments? > > > > Will do. And thanks, this thread is very informative. > > > > It is important to use the 'msgid' field to distinguish recheck > > requests. Otherwise, we will continuously reparse the same > > recheck request and loop forever. Additionally, we've discussed using= a > > counter to limit the recheck requests to a single 'recheck' per test > > name. > > > > We can track message ids to avoid considering a single retest request > twice. Perhaps we can accomplish the > > same thing by tracking retested patchseries ids and their total number > of requested retests (which could be 1 > > retest per patchseries). > > > > +function check_series_needs_retest() { > > > > + local pw_instance=3D"$1" > > + > > + series_get_active_branches "$pw_instance" | while IFS=3D\| read -r > series_id project url repo branchname; do > > + local patch_comments_url=3D$(curl -s "$userpw" "$url" | jq -rc > '.comments') > > + if [ "Xnull" !=3D "X$patch_comments_url" ]; then > > + local comments_json=3D$(curl -s "$userpw" > "$patch_comments_url") > > + local seq_end=3D$(echo "$comments_json" | jq -rc 'length') > > + if [ "$seq_end" -a $seq_end -gt 0 ]; then > > + seq_end=3D$((seq_end-1)) > > + for comment_id in $(seq 0 $seq_end); do > > + local recheck_requested=3D$(echo "$comments_json" = | > jq -rc ".[$comment_id].content" | grep > > "^Recheck-request: ") > > + if [ "X$recheck_requested" !=3D "X" ]; then > > + local msgid=3D$(echo "$comments_json" | jq -rc > ".[$comment_id].msgid") > > + run_recheck "$pw_instance" "$series_id" > "$project" "$url" "$repo" "$branchname" > > "$recheck_requested" "$msgid" > > + fi > > + done > > + fi > > + fi > > + done > > +} > > This is already a superior approach to what I had in mind for acquiring > comments. Unless you're opposed, I > > think at the communit lab we can experiment based on this starting poin= t > to verify the process is sound, but I > > don't see any problems here. > > > > I think that if we're able to specify multiple contexts, then there's > not really any reason to run multiple > > rechecks per patchset. > > > > Agreed. > > > > There was also an ask on filtering requesters (only maintainers and > > > > patch authors can ask for a recheck). > > > > If we can use the maintainers file as a single source of truth that is > convenient and stable as the list of > > maintainers changes. But, also I think retesting request permission > should be extended to the submitter too. > > They may want to initiate a re-run without engaging a maintainer. It's > not likely to cause a big increase in test > > load for us or other labs, so there's no harm there. > > > > No, an explicit list is actually better. > > > > When a new check is added, for someone looking at the mails (maybe 2/3 > > > > weeks later), and reading just "all", he would have to know what > > > > checks were available at the time. > > > > Context/Labels rarely change, so I don't think this concern is too > serious. But, if people dont mind comma > > separating an entire list of contexts, that's fine. > > > > Thanks, > > Patrick > > --=20 Patrick Robb Technical Service Manager UNH InterOperability Laboratory 21 Madbury Rd, Suite 100, Durham, NH 03824 www.iol.unh.edu --0000000000007d3f4605fe02cebc Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
1. Ephem= eral lab/network failures, or flaky unit tests that sometimes
=C2=A0 =C2= =A0fail.=C2=A0 In this case, we probably just want to re-run the tree as-is= .

2. Failing tree before apply.=C2=A0 In this case, we have applied = the series
=C2=A0 =C2=A0to a tree, but the tree isn't in a good stat= e and will fail
=C2=A0 =C2=A0regardless of the series being applied.
=

I had originally thought of this only as r= erunning the tree as-is, though if the community wants the 2nd option to be= available that works too. It does increase the scope of the task and compl= exity of the request text format to be understood for submitters. Personall= y, where I fall is that there isn't enough additional value to justify = doing more than rerunning as-is. Plus, if we do end up with a bad tree, sub= mitters can resubmit their patches when the tree is in a good state again,= =C2=A0right? Or alternatively, lab managers may be aware of this situation = and be able to order a rerun for the last x days (duration of bad tree) on = the most up to date tree.=C2=A0

<= div dir=3D"ltr" class=3D"gmail_attr">On Mon, Jun 12, 2023 at 11:01=E2=80=AF= AM Aaron Conole <aconole@redhat.co= m> wrote:
Patrick Robb <pr= obb@iol.unh.edu> writes:

> On Wed, Jun 7, 2023 at 8:53=E2=80=AFAM Aaron Conole <aconole@redhat.com> wrote:=
>
>=C2=A0 Patrick Robb <probb@iol.unh.edu> writes:
>
>=C2=A0 >=C2=A0 Also it can be useful to run daily sub-tree testing b= y request, if possible.
>=C2=A0 >
>=C2=A0 > That wouldn't be too difficult. I'll make a ticket = for this. Although, for testing on the daily sub-trees,
>=C2=A0 since that's
>=C2=A0 > UNH-IOL specific, that wouldn't necessarily have to be = done via an email based testing request
>=C2=A0 framework. We
>=C2=A0 > could also just add a button to our dashboard which trigger= s a sub-tree ci run. That would help keep
>=C2=A0 narrow
>=C2=A0 > the scope of what the email based retesting framework is fo= r. But, both email or a dashboard button
>=C2=A0 would
>=C2=A0 > both work.
>
>=C2=A0 We had discussed this long ago - including agreeing on a format,= IIRC.
>
>=C2=A0 See the thread starting here:
>=C2=A0 =C2=A0 https://mails.dpdk.org/arch= ives/ci/2021-May/001189.html
>
>=C2=A0 The idea was to have a line like:
>
>=C2=A0 Recheck-request: <test names>
>
> I like using this simpler format which is easier to parse. As Thomas p= ointed out, specifying labs does not really
> provide extra information if we are already going to request by label/= context, which is already specifies the
> lab.=C2=A0

One thing we haven't discussed or determined is if we should have the ability to re-apply the series or simply to rerun the patches based on
the original sha sum.=C2=A0 There are two cases I can think of:

1. Ephemeral lab/network failures, or flaky unit tests that sometimes
=C2=A0 =C2=A0fail.=C2=A0 In this case, we probably just want to re-run the = tree as-is.

2. Failing tree before apply.=C2=A0 In this case, we have applied the serie= s
=C2=A0 =C2=A0to a tree, but the tree isn't in a good state and will fai= l
=C2=A0 =C2=A0regardless of the series being applied.

WDYT?=C2=A0 Does (2) case warrant any consideration as a possible reason to=
retest?=C2=A0 If so, what is the right way of handling that situation?

>=C2=A0 where <test names> was the tests in the check labels.=C2= =A0 In fact, what
>=C2=A0 started the discussion was a patch for the pw-ci scripts that >=C2=A0 implemented part of it.
>
>=C2=A0 I don't see how to make your proposal as easily parsed.
>
>=C2=A0 WDYT?=C2=A0 Can you re-read that thread and come up with comment= s?
>
>=C2=A0 Will do. And thanks, this thread is very informative.
>
>=C2=A0 It is important to use the 'msgid' field to distinguish = recheck
>=C2=A0 requests.=C2=A0 Otherwise, we will continuously reparse the same=
>=C2=A0 recheck request and loop forever.=C2=A0 Additionally, we've = discussed using a
>=C2=A0 counter to limit the recheck requests to a single 'recheck&#= 39; per test
>=C2=A0 name.
>
> We can track message ids to avoid considering a single retest request = twice. Perhaps we can accomplish the
> same thing by tracking retested patchseries ids and their total number= of requested retests (which could be 1
> retest per patchseries).
>
>=C2=A0 =C2=A0+function check_series_needs_retest() {
>
> +=C2=A0 =C2=A0 local pw_instance=3D"$1"
> +
> +=C2=A0 =C2=A0 series_get_active_branches "$pw_instance" | w= hile IFS=3D\| read -r series_id project url repo branchname; do
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 local patch_comments_url=3D$(curl -s &quo= t;$userpw" "$url" | jq -rc '.comments')
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 if [ "Xnull" !=3D "X$patch= _comments_url" ]; then
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 local comments_json=3D$(cur= l -s "$userpw" "$patch_comments_url")
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 local seq_end=3D$(echo &quo= t;$comments_json" | jq -rc 'length')
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if [ "$seq_end" -= a $seq_end -gt 0 ]; then
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 seq_end=3D$((= seq_end-1))
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 for comment_i= d in $(seq 0 $seq_end); do
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= local recheck_requested=3D$(echo "$comments_json" | jq -rc "= ;.[$comment_id].content" | grep
> "^Recheck-request: ")
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= if [ "X$recheck_requested" !=3D "X" ]; then
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 local msgid=3D$(echo "$comments_json" | jq -rc &qu= ot;.[$comment_id].msgid")
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 run_recheck "$pw_instance" "$series_id" = "$project" "$url" "$repo" "$branchname&q= uot;
> "$recheck_requested" "$msgid"
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= fi
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 done
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 fi
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 fi
> +=C2=A0 =C2=A0 done
> +}
> This is already a superior approach to what I had in mind for acquirin= g comments. Unless you're opposed, I
> think at the communit lab we can experiment based on this starting poi= nt to verify the process is sound, but I
> don't see any problems here.
>
>=C2=A0 I think that if we're able to specify multiple contexts, the= n there's not really any reason to run multiple
>=C2=A0 rechecks per patchset.
>
> Agreed.
>
>=C2=A0 There was also an ask on filtering requesters (only maintainers = and
>
>=C2=A0 patch authors can ask for a recheck).
>
> If we can use the maintainers file as a single source of truth that is= convenient and stable as the list of
> maintainers changes. But, also I think retesting request permission sh= ould be extended to the submitter too.
> They may want to initiate a re-run without engaging a maintainer. It&#= 39;s not likely to cause a big increase in test
> load for us or other labs, so there's no harm there.
>
>=C2=A0 No, an explicit list is actually better.
>
>=C2=A0 When a new check is added, for someone looking at the mails (may= be 2/3
>
>=C2=A0 weeks later), and reading just "all", he would have to= know what
>
>=C2=A0 checks were available at the time.
>
> Context/Labels rarely change, so I don't think this concern is too= serious. But, if people dont mind comma
> separating an entire list of contexts, that's fine.
>
> Thanks,
> Patrick



--

Patrick Robb

<= span style=3D"font-size:10pt;font-family:Arial;color:rgb(0,0,0);background-= color:transparent;vertical-align:baseline;white-space:pre-wrap">Technical S= ervice Manager

UNH InterOperability Laboratory

21 Madbury Rd, Suite 100, Durham, NH 03824

www.iol.unh.edu


--0000000000007d3f4605fe02cebc--