SRU quality and preventing regressions

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
11 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

SRU quality and preventing regressions

Robie Basak-4
I'm fairly new to ~ubuntu-sru, and I noticed a couple of things that I
think we could do better in terms of preventing SRU regressions:

1) Actual consideration of how regressions might manifest in "Regression
Potential" paperwork.

2) A description of what was actually tested when marking
"verification-done".

Both of these are already requirements of the SRU process, but many
uploaders appear to be unaware of this. Since I started on ~ubuntu-sru,
I have been following the process and either fixing up or deferring SRUs
that do not follow the process. Please help to save everyone time by
making sure this is done properly the first time.

With my DMB hat, we considered and accepted our first application to
~ubuntu-sru-developers last week. In also having an SRU hat[1], I'd like
to see ~ubuntu-sru-developers holding the torch for SRU quality, but
that's tough when I think other existing uploaders could do better.

Details:

1) "Regression Potential"

"Regression Potential" is supposed to describe:

        ...how regressions are most likely to manifest, or may manifest
        even if it is unlikely, as a result of this change. It is
        assumed that any SRU candidate patch is well-tested before
        upload and has a low overall risk of regression, but it's
        important to make the effort to think about what could happen in
        the event of a regression.

Note that "Low" or "None", or an explanation of why it is "Low" or
"None", is insufficient and doesn't meet this requirement.

If I don't have enough information to be able to fill this in myself
quickly, or if I have a particuarly big queue when I'm reviewing, I will
continue to bounce this back to the uploader and delay the SRU. I prefer
this over accepting something that will get insufficient verification
and risk regressing the stable release.

2) "verification-done"

When marking "verification-done", please describe what packages were
tested and what versions. This is explicitly requested in the acceptance
message, but I see many people not doing this.

We have had at least one very severe regression because the version
tested was not the version released. To prevent this happening again, I
will continue to bounce back any "verification-done" that does not
explicitly state what package versions were tested.

Just switching the tags is similarly unacceptable. I will continue to
decline to release and just switch the tag back when I see this.

Automated testing is still fine - just arrange the automatic testing to
print out the package versions used, and copy that detail in, explaining
it was automatically tested.

If you are a sponsor, please hold contributors to these standards too.
Otherwise they'll only get frustrated later when their SRUs get delayed.

Thanks,

Robie

[1] currently I believe there are four people who wear both hats, so
    this isn't unique to me

--
ubuntu-devel mailing list
[hidden email]
Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/ubuntu-devel

signature.asc (836 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: SRU quality and preventing regressions

Mathieu Trudel-Lapierre-4
On 2017-03-21 02:25 PM, Robie Basak wrote:
[...]

> 1) "Regression Potential"
>
> "Regression Potential" is supposed to describe:
>
> ...how regressions are most likely to manifest, or may manifest
> even if it is unlikely, as a result of this change. It is
> assumed that any SRU candidate patch is well-tested before
> upload and has a low overall risk of regression, but it's
> important to make the effort to think about what could happen in
> the event of a regression.
>
> Note that "Low" or "None", or an explanation of why it is "Low" or
> "None", is insufficient and doesn't meet this requirement.
>
> If I don't have enough information to be able to fill this in myself
> quickly, or if I have a particuarly big queue when I'm reviewing, I will
> continue to bounce this back to the uploader and delay the SRU. I prefer
> this over accepting something that will get insufficient verification
> and risk regressing the stable release.
I think that's correct, with one caveat. It's not gaining us anything if
what is added is beyond unlikely. SRUs need to be evaluated on a case by
case basis, sometimes the best you can say is "This has a low potential
of regression because $reasons.", but those are indeed not the norm.

> 2) "verification-done"
>
> When marking "verification-done", please describe what packages were
> tested and what versions. This is explicitly requested in the acceptance
> message, but I see many people not doing this.
I agree, but this is valid criticism for any change on any bug on
Launchpad. Marking something as Confirmed, or Invalid, or even removing
tasks without a comment explaining why is just bad form.

It's nice to write this out here, but perhaps it would be good to update
the templates for comments on the bugs when a SRU lands in -proposed to
clarify it? I would not expect all our contributors on SRU bugs to see
ubuntu-devel@ discussions.

--
Mathieu Trudel-Lapierre <[hidden email]>
Freenode: cyphermox, Jabber: [hidden email]
4096R/65B58DA1 818A D123 0992 275B 23C2  CF89 C67B B4D6 65B5 8DA1



--
ubuntu-devel mailing list
[hidden email]
Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/ubuntu-devel

signature.asc (817 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: SRU quality and preventing regressions

Robie Basak-4
Hi Mathieu,

On Tue, Mar 28, 2017 at 02:17:29PM -0400, Mathieu Trudel-Lapierre wrote:

> > "Regression Potential" is supposed to describe:
> >
> > ...how regressions are most likely to manifest, or may manifest
> > even if it is unlikely, as a result of this change. It is
> > assumed that any SRU candidate patch is well-tested before
> > upload and has a low overall risk of regression, but it's
> > important to make the effort to think about what could happen in
> > the event of a regression.
> >
> > Note that "Low" or "None", or an explanation of why it is "Low" or
> > "None", is insufficient and doesn't meet this requirement.
> >
> > If I don't have enough information to be able to fill this in myself
> > quickly, or if I have a particuarly big queue when I'm reviewing, I will
> > continue to bounce this back to the uploader and delay the SRU. I prefer
> > this over accepting something that will get insufficient verification
> > and risk regressing the stable release.
> I think that's correct, with one caveat. It's not gaining us anything if
> what is added is beyond unlikely. SRUs need to be evaluated on a case by
> case basis, sometimes the best you can say is "This has a low potential
> of regression because $reasons.", but those are indeed not the norm.
Sure, I have no problem with being told why the regression potential is
low. But the SRU policy currently says:

  * It is assumed that any SRU candidate...has a low overall risk of
    regression...

  * ...it's important to make the effort to think about what could
    happen in the event of a regression.

I'm just implementing the current SRU policy as stated. If the second
point isn't covered, then the provided SRU information doesn't meet SRU
policy.

If there's some exceptional reason why the SRU policy should be bent for
a particular case, I'd be happy to consider that. Appropriate pragmatism
is my understanding of how Ubuntu development works. But in that case,
the exceptional reason needs to be documented in the bug, and it would
help if it were explicit that an exception is being requested.

If the risk is beyond unlikely, a justification as to why that is the
case is fine. But a missing justification is a reject in my book, and
that's the point I wanted to make.

> > 2) "verification-done"
> >
> > When marking "verification-done", please describe what packages were
> > tested and what versions. This is explicitly requested in the acceptance
> > message, but I see many people not doing this.
> I agree, but this is valid criticism for any change on any bug on
> Launchpad. Marking something as Confirmed, or Invalid, or even removing
> tasks without a comment explaining why is just bad form.
>
> It's nice to write this out here, but perhaps it would be good to update
> the templates for comments on the bugs when a SRU lands in -proposed to
> clarify it? I would not expect all our contributors on SRU bugs to see
> ubuntu-devel@ discussions.
The automatic instructions do already say:

    If this package fixes the bug for you, please add a comment to this
    bug, mentioning the version of the package you tested...

and:

    In either case, details of your testing will help us make a better
    decision.

So I think we have already been saying this, but perhaps it could be
made clearer. Personally, I'd like to make it clearer that I want:

  1) Package names tested (where the bug has tasks for multiple
     packages).

  2) Version strings of packages tested (shortcuts like "from
     yakkety-proposed" isn't enough, as the answer to that changes and
     mistakes have been made in the past).

  3) A summary of the testing performed; "as described in Test Case" is
     sufficient if the description there is clear enough.

Robie

--
ubuntu-devel mailing list
[hidden email]
Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/ubuntu-devel

signature.asc (836 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: SRU quality and preventing regressions

Marco Trevisan
In reply to this post by Robie Basak-4
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

Il 22/03/2017 02:25, Robie Basak ha scritto:

> 1) "Regression Potential"
>
> "Regression Potential" is supposed to describe:
>
> ...how regressions are most likely to manifest, or may manifest
> even if it is unlikely, as a result of this change. It is assumed
> that any SRU candidate patch is well-tested before upload and has a
> low overall risk of regression, but it's important to make the
> effort to think about what could happen in the event of a
> regression.
>
> Note that "Low" or "None", or an explanation of why it is "Low" or
> "None", is insufficient and doesn't meet this requirement.

This is true... But you've to admit that there are fixes where it's
just not possible to think a regression... Like an obvious
null-pointer deference fix. That could just make things not to crash
(unless the code path won't lead to somewhere else unwanted, but I'm
thinking to the simplest case).

> 2) "verification-done"
>
> When marking "verification-done", please describe what packages
> were tested and what versions. This is explicitly requested in the
> acceptance message, but I see many people not doing this.
>
> We have had at least one very severe regression because the
> version tested was not the version released. To prevent this
> happening again, I will continue to bounce back any
> "verification-done" that does not explicitly state what package
> versions were tested.

Ok, that makes sense... However having a pattern to follow and an SRU
bug template also for verifying would help a lot.
In the same way we've for opening an SRU bug.

So I hope the SRU team could update the wiki with such informations.

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.22 (GNU/Linux)

iF4EAREIAAYFAlj4j9IACgkQy6VOJFdF1Op9IAD/e8G3z4x4Ytzad64/kRhOdxer
omHXterc0osU3RYSWEIA/RbmN2dIKeI3eoP8KLhxke9y+8pHRgWsOlPh4HJ+hkFc
=/eZa
-----END PGP SIGNATURE-----

--
ubuntu-devel mailing list
[hidden email]
Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/ubuntu-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: SRU quality and preventing regressions

Robie Basak-4
On Thu, Apr 20, 2017 at 06:39:15PM +0800, Marco Trevisan wrote:

> Il 22/03/2017 02:25, Robie Basak ha scritto:
> > 1) "Regression Potential"
> >
> > "Regression Potential" is supposed to describe:
> >
> > ...how regressions are most likely to manifest, or may manifest
> > even if it is unlikely, as a result of this change. It is assumed
> > that any SRU candidate patch is well-tested before upload and has a
> > low overall risk of regression, but it's important to make the
> > effort to think about what could happen in the event of a
> > regression.
> >
> > Note that "Low" or "None", or an explanation of why it is "Low" or
> > "None", is insufficient and doesn't meet this requirement.
>
> This is true... But you've to admit that there are fixes where it's
> just not possible to think a regression... Like an obvious
> null-pointer deference fix. That could just make things not to crash
> (unless the code path won't lead to somewhere else unwanted, but I'm
> thinking to the simplest case).
Well, see https://wiki.ubuntu.com/StableReleaseUpdates#Why and bugs
81125, 309674 and 559822. The act of rebuilding can cause a regression.
Build dependencies might have changed, or something might be racing, or
there may be packaging interactions because the package version string
has changed. I think it's worth considering these and similar
possibilities and how this kind of thing may relate to a particular SRU,
and it'll give the SRU team more confidence that thought has been put
into not regressing users.

Most of the time I see "None", I can think of more likely causes than
these that are specific to the situation.

Remember the point of this is to inform SRU verification. We want to
know where to look when testing.

> Ok, that makes sense... However having a pattern to follow and an SRU
> bug template also for verifying would help a lot.
> In the same way we've for opening an SRU bug.
>
> So I hope the SRU team could update the wiki with such informations.

Do you think people would look at the wiki at this stage? Or do you
think we need to amend the verification instructions provided
automatically in the bug comments?

I want three things:

1) What package name and version string was tested (saying "the one from
xenial-proposed" isn't enough for me, as this is where we see things
going wrong as that version can change).

2) What testing was done (saying "the test case from the bug
description" or similar is fine). If extra testing was agreed when the
SRU was accepted, then it is useful to call that out explicitly as done
so that we don't have to ask to make sure.

3) What the result was (saying "tests passed as expected" or similar is
fine).

Robie

--
ubuntu-devel mailing list
[hidden email]
Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/ubuntu-devel

signature.asc (836 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: SRU quality and preventing regressions

Sebastien Bacher
Hey Robie,

Thanks for pushing for those process improvements.

Le 20/04/2017 à 14:36, Robie Basak a écrit :
> Do you think people would look at the wiki

Some people are probably going to, and if we have documentation it
should reflect our current practices otherwise we better just delete it
or remove specifics that would be less misleading to contributors who
read it.

When Marco verified that indicator SRU ealier I mentioned to him that he
should include the version (which I knew you mentioned in your email)
but he asked where it was written since he couldn't find that
requirement ... and indeed it's not in the wiki. So we have at least one
example where something checked the wiki documentation as a reference,
wiki also is easy enough to edit so there is no real reason to not do it.

Cheers,

Sebastien Bacher


--
ubuntu-devel mailing list
[hidden email]
Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/ubuntu-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: SRU quality and preventing regressions

Robie Basak-4
On Thu, Apr 20, 2017 at 03:28:39PM +0200, Sebastien Bacher wrote:
> requirement ... and indeed it's not in the wiki. So we have at least one
> example where something checked the wiki documentation as a reference,
> wiki also is easy enough to edit so there is no real reason to not do it.

Thanks. I don't think I'm actually suggesting any change in policy here
- it seems to me that what I'm asking for isn't consistently written
everywhere even though it is actually current policy (because it has
always* been asked for in at least one place). So I'll JFDI and edit
everything for clarity, including the wiki, unless someone objects.

Robie

--
ubuntu-devel mailing list
[hidden email]
Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/ubuntu-devel

signature.asc (836 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: SRU quality and preventing regressions

Sebastien Bacher
Le 20/04/2017 à 15:39, Robie Basak a écrit :
> So I'll JFDI and edit
> everything for clarity, including the wiki

Sounds good to me, thanks Robie!

Cheers,

Sebastien Bacher


--
ubuntu-devel mailing list
[hidden email]
Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/ubuntu-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: SRU quality and preventing regressions

Steve Langasek-6
In reply to this post by Robie Basak-4
Hi Robie,

On Thu, Apr 20, 2017 at 01:36:26PM +0100, Robie Basak wrote:
> 1) What package name and version string was tested (saying "the one from
> xenial-proposed" isn't enough for me, as this is where we see things
> going wrong as that version can change).

On Thu, Apr 20, 2017 at 02:39:07PM +0100, Robie Basak wrote:
> On Thu, Apr 20, 2017 at 03:28:39PM +0200, Sebastien Bacher wrote:
> > requirement ... and indeed it's not in the wiki. So we have at least one
> > example where something checked the wiki documentation as a reference,
> > wiki also is easy enough to edit so there is no real reason to not do it.

> Thanks. I don't think I'm actually suggesting any change in policy here
> - it seems to me that what I'm asking for isn't consistently written
> everywhere even though it is actually current policy (because it has
> always* been asked for in at least one place). So I'll JFDI and edit
> everything for clarity, including the wiki, unless someone objects.

FTR I do consider your point 1) to be a policy change, and not one I'm
sanguine about.  We struggle to get SRUs through the verification process
today, and I don't think raising further barriers significantly improves the
outcomes for our users but /would/ slow things down by requiring further
round trips.

You are of course right that we need to have confidence the package which
was tested is the actual package which will be released.  But I don't think
that means we should have a hard rule that an SRU verification comment needs
to list the version number, because while the version in -proposed can
change over time, in most cases it's *not* ambiguous which version was in
-proposed when the user tested.

What do you think about a wording such as this?:

  - The SRU verification must clearly indicate which package was tested.
    The most reliable way to ensure this is to list the version number of
    the tested package.

Thanks,
--
Steve Langasek                   Give me a lever long enough and a Free OS
Debian Developer                   to set it on, and I can move the world.
Ubuntu Developer                                    http://www.debian.org/
[hidden email]                                     [hidden email]

--
ubuntu-devel mailing list
[hidden email]
Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/ubuntu-devel

signature.asc (817 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: SRU quality and preventing regressions

Dimitri John Ledkov
On 21 April 2017 at 00:46, Steve Langasek <[hidden email]> wrote:

> Hi Robie,
>
> On Thu, Apr 20, 2017 at 01:36:26PM +0100, Robie Basak wrote:
>> 1) What package name and version string was tested (saying "the one from
>> xenial-proposed" isn't enough for me, as this is where we see things
>> going wrong as that version can change).
>
> On Thu, Apr 20, 2017 at 02:39:07PM +0100, Robie Basak wrote:
>> On Thu, Apr 20, 2017 at 03:28:39PM +0200, Sebastien Bacher wrote:
>> > requirement ... and indeed it's not in the wiki. So we have at least one
>> > example where something checked the wiki documentation as a reference,
>> > wiki also is easy enough to edit so there is no real reason to not do it.
>
>> Thanks. I don't think I'm actually suggesting any change in policy here
>> - it seems to me that what I'm asking for isn't consistently written
>> everywhere even though it is actually current policy (because it has
>> always* been asked for in at least one place). So I'll JFDI and edit
>> everything for clarity, including the wiki, unless someone objects.
>
> FTR I do consider your point 1) to be a policy change, and not one I'm
> sanguine about.  We struggle to get SRUs through the verification process
> today, and I don't think raising further barriers significantly improves the
> outcomes for our users but /would/ slow things down by requiring further
> round trips.
>

I think 1) was due failure to validate a complex SRU of multiple
packages.... and people were testing package version numbers from
ppa's rather than -proposed, and things got delayed by a significant
amount of time.

> You are of course right that we need to have confidence the package which
> was tested is the actual package which will be released.  But I don't think
> that means we should have a hard rule that an SRU verification comment needs
> to list the version number, because while the version in -proposed can
> change over time, in most cases it's *not* ambiguous which version was in
> -proposed when the user tested.
>
> What do you think about a wording such as this?:
>
>   - The SRU verification must clearly indicate which package was tested.
>     The most reliable way to ensure this is to list the version number of
>     the tested package.

This should be added to the SRU stock response of "Hello Reporter, or
anybody else affected..." message,

e.g. end it with:
"

! NB PLEASE state the package version tested in full using dpkg-query
-W $package !
"

--
Regards,

Dimitri.

--
ubuntu-devel mailing list
[hidden email]
Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/ubuntu-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: SRU quality and preventing regressions

Robie Basak-4
In reply to this post by Steve Langasek-6
Hi Steve,

On Thu, Apr 20, 2017 at 04:46:23PM -0700, Steve Langasek wrote:
> On Thu, Apr 20, 2017 at 01:36:26PM +0100, Robie Basak wrote:
> > 1) What package name and version string was tested (saying "the one from
> > xenial-proposed" isn't enough for me, as this is where we see things
> > going wrong as that version can change).

> FTR I do consider your point 1) to be a policy change, and not one I'm
> sanguine about.  We struggle to get SRUs through the verification process
> today, and I don't think raising further barriers significantly improves the
> outcomes for our users but /would/ slow things down by requiring further
> round trips.

I always assumed that "If this package fixes the bug for you, please
add a comment to this bug, mentioning the version of the package you
tested..." meant that one can expect the version string to be explicitly
present in a verification comment compliant with this request. Do you
disagree, or do you not consider the templated request to be de-facto
policy?

I agree that we should not be raising any further barriers. My goal here
is also to reduce round trips and related SRU friction.

Right now when I review bugs showing as green in the pending-sru report,
I find myself spending time trawling through the comments to figure out
what was verified, since from recent experience the tag doesn't give me
sufficient confidence. I'd be able to get through releasing SRUs quicker
if it were called out explicitly.

If someone is actually verifying the bug and reporting the verification,
how onerous is it really to ask that the package version tested be
explicitly reported? The version string should normally be right in
front of the verifier and trivial to copy and paste (in the common case).

If it isn't explicit and I am not sure, then it is _this_ that creates
an extra round trip currently, for me anyway, when I ask for
clarification. If we had consensus that reporting the version string was
the right thing to do, and it was documented correctly everywhere, then
I am hoping that these extra round trips will no longer be necessary.

I could even write a bot that flips back the tag with an explanation and
a request for clarification if the expected package version string
hasn't appeared in a comment. This would eliminate the necessity for a
human round trip entirely. I'm not sure it's the right thing to do, but
I hope it illustrates my point.

As an aside, I've been considering doing the same with dep8 failures -
automatically ask the SRU driver to investigate and report ("not
relevant to this SRU because <reasons>" would be a fine response), thus
saving ~ubuntu-sru time, and hopefully without any need for an
additional ~ubuntu-sru round trip since a bot would mention it as soon
as the information is available.

> You are of course right that we need to have confidence the package which
> was tested is the actual package which will be released.  But I don't think
> that means we should have a hard rule that an SRU verification comment needs
> to list the version number, because while the version in -proposed can
> change over time, in most cases it's *not* ambiguous which version was in
> -proposed when the user tested.
>
> What do you think about a wording such as this?:
>
>   - The SRU verification must clearly indicate which package was tested.
>     The most reliable way to ensure this is to list the version number of
>     the tested package.
I don't object to the wording, but I question whether it really is so
onerous to simply require it.

Robie

--
ubuntu-devel mailing list
[hidden email]
Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/ubuntu-devel

signature.asc (836 bytes) Download Attachment
Loading...