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 3DFE742C55 for ; Thu, 8 Jun 2023 03:47:20 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 14FAA410D3; Thu, 8 Jun 2023 03:47:20 +0200 (CEST) Received: from mail-ot1-f54.google.com (mail-ot1-f54.google.com [209.85.210.54]) by mails.dpdk.org (Postfix) with ESMTP id 3FCF140042 for ; Thu, 8 Jun 2023 03:47:19 +0200 (CEST) Received: by mail-ot1-f54.google.com with SMTP id 46e09a7af769-6b166023d47so33360a34.1 for ; Wed, 07 Jun 2023 18:47:19 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=iol.unh.edu; s=unh-iol; t=1686188838; x=1688780838; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=ifY2o1UGFRmh/6iSgx84kUULzAcnucIQPdWcPVey3eM=; b=JqyG79VbjZ52joKpJbqVMcDAaAXe3BnJYQmLnVV76aUFfEQpWMpeviWD7gJmG0JUeF zp3c977232zGJqCO4SsuesmWjG6OPT+rw4hI9YpKIvbqQeHVEwbrhE08kkfX5rtcXo4w xYCkWiGTmLU4Jn8mpxQ1n5fdfn5wZoGqEw+i8= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1686188838; x=1688780838; 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=ifY2o1UGFRmh/6iSgx84kUULzAcnucIQPdWcPVey3eM=; b=dykPD4qoHM/+NY+ICklcfawuAGQax4eRKfGVanX0W2Ckn5mxdDsmA7FmlKSEwY9AoP Hu6JfZht7RXz9VmhIr77KvG4Zk2YLYPU6I17hHiD+qpQEcoqNg57cTZxwn4s72kuk8Qt YphyCp9ArSHVnqeZmPzgQV5SSP0JAaY/GJ9ye+Dxos3AKE9Dukm4fWIRRCL4IDX4C/8/ g9rmjsB8y12JShmBpQ/Z+M4U4Rf84D1wBDB1cYgTO4tHKAXaU0cAuBBAEdAaVMXU/OAC i1iVSOIvsK+tFdJyupdWG6R7b92etvz8s4JL0mAeFmn50Skavrz68gMHq0rZuPXOEQua 31sQ== X-Gm-Message-State: AC+VfDwEX0WF17gzCsLRLw9x7jKNMSWpvxNlQNjHSzgnmKQsQwnRWb3o A4N3CgANCyDb2/yddPGt1FBoS7BbTw+g/TCyuJQkrA== X-Google-Smtp-Source: ACHHUZ7q2E7SnF5uMIfrLHtIDas4P+oiiPwcr46/ocMmM5wju2Yn6xWdJ2DJQQRhswhUBIWs9vHNCQoGID84ZIDK99s= X-Received: by 2002:a05:6870:e881:b0:19e:e6e8:46f9 with SMTP id q1-20020a056870e88100b0019ee6e846f9mr6980618oan.30.1686188838322; Wed, 07 Jun 2023 18:47:18 -0700 (PDT) MIME-Version: 1.0 References: <3fa6546b-8152-e317-30f0-30d5118b9fc4@amd.com> In-Reply-To: From: Patrick Robb Date: Wed, 7 Jun 2023 21:47:07 -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="000000000000bb962005fd946e1f" 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 --000000000000bb962005fd946e1f Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Wed, Jun 7, 2023 at 8:53=E2=80=AFAM Aaron Conole wr= ote: > 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 emai= l > 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. > > 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 point 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 --000000000000bb962005fd946e1f Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


=
On Wed, Jun 7, 2023 at 8:53=E2=80=AFA= M Aaron Conole <aconole@redhat.com= > wrote:
= Patrick Robb <pro= bb@iol.unh.edu> writes:

>=C2=A0 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. A= lthough, 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-tre= e ci run. That would help keep narrow
> the scope of what the email based retesting framework is for. But, bot= h 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:
=C2=A0 https://mails.dpdk.org/archives/ci/202= 1-May/001189.html

The idea was to have a line like:

Recheck-request: <test names>
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 r= equest by label/context, which is already specifies the lab.=C2=A0=C2=A0

where <test names> was the tests in the check labels.=C2=A0 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?=C2=A0 Can you re-read that thread and come up with comments?
=C2=A0Will do. And thanks, this thread is very informative.= =C2=A0
It is importa= nt to use the 'msgid' field to distinguish recheck
requests.=C2= =A0 Otherwise, we will continuously reparse the same
recheck request and= loop forever.=C2=A0 Additionally, we've discussed using a
counter t= o limit the recheck requests to a single 'recheck' per test
name= .
We can track message ids to avoid considering a sing= le retest request twice. Perhaps we can accomplish the same thing by tracki= ng retested patchseries=C2=A0ids and their total number of requested retest= s (which could be 1 retest per patchseries).=C2=A0
=C2=A0+function check_series_needs_retest(= ) {
+ =C2=A0 =C2=A0local pw_instance=3D"$1"
+
+= =C2=A0 =C2=A0series_get_active_branches "$pw_instance" | while I= FS=3D\| read -r series_id project url repo branchname; do
+ =C2=A0 =C2= =A0 =C2=A0 =C2=A0local patch_comments_url=3D$(curl -s "$userpw" &= quot;$url" | jq -rc '.comments')
+ =C2=A0 =C2=A0 =C2=A0 =C2= =A0if [ "Xnull" !=3D "X$patch_comments_url" ]; then
= + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0local comments_json=3D$(curl -s = "$userpw" "$patch_comments_url")
+ =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0local seq_end=3D$(echo "$comments_json" |= jq -rc 'length')
+ =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if = [ "$seq_end" -a $seq_end -gt 0 ]; then
+ =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0seq_end=3D$((seq_end-1))
+ =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0for comment_id 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=A0local 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= =A0if [ "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=A0local msgid=3D$(echo "$comments_json" | jq -rc ".[$com= ment_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=A0run_recheck "$pw_instance" &qu= ot;$series_id" "$project" "$url" "$repo"= "$branchname" "$recheck_requested" "$msgid"<= br>+ =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0f= i
+ =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0done
+ =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0fi
+ =C2=A0 =C2=A0 =C2=A0 =C2=A0fi=
+ =C2=A0 =C2=A0done
+}
This is already a superior approach to wh= at I had in mind for acquiring comments. Unless you're opposed, I think= at the communit lab we can experiment based on this starting point to veri= fy the process is sound, but I don't see any problems here.=C2=A0
=
I think that if we&#= 39;re able to specify multiple contexts, then there's not really any re= ason 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).=C2= =A0
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 su= bmitter too. They may want to initiate a re-run without engaging a maintain= er. It's not likely to cause a big increase in test load for us or othe= r labs, so there's no harm there.=C2=A0
No, an explicit list is actually better.
When a new check is add= ed, 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.=C2=A0
=
Context/Labels rarely change, so I don't think this concern is too= serious. But, if people dont=C2=A0mind comma separating=C2=A0an entire lis= t of contexts, that's fine.=C2=A0

Thanks,
Patrick

--=

Patrick Robb

Technical Service Manager

UNH InterOperability Laboratory

= 21 Madbury= Rd, Suite 100, Durham, NH 03824

www.iol= .unh.edu


--000000000000bb962005fd946e1f--