Re: [Gluster-devel] Gerrit review, submit type and Jenkins testing

[Michael Adam]

On 2016-01-08 at 12:03 +0530, Raghavendra Talur wrote:
> Top posting, this is a very old thread.
> 
> Keeping in view the recent NetBSD problems and the number of bugs creeping
> in, I suggest we do these things right now:
> 
> a. Change the gerrit merge type to fast forward only.
> As explained below in the thread, with our current setup even if both
> PatchA and PatchB pass regression separately when both are merged it is
> possible that a functional bug creeps in.
> This is the only solution to prevent that from happening.
> I will work with Kaushal to get this done.
> 
> b. In Jenkins, remove gerrit trigger and make it a manual operation
> 
> Too many developers use the upstream infra as a test cluster and it is
> *not*.
> It is a verification mechanism for maintainers to ensure that the patch
> does not cause regression.
>
> It is required that all developers run full regression on their machines
> before asking for reviews.

Hmm, I am not 100% sure I would underwrite that.
I am coming from the Samba process, where we have exactly
that: A developer should have run full selftest before
submitting the change for review. Then after two samba
team developers have given their review+ (counting the
author), it can be pushed to our automatism that keeps
rebasing on current upstream and running selftest until
either selftest succeeds and is pushed as a fast forward
or selftest fails.

The reality is that people are lazy and think they know
when they can skip selftest. But people are deceived and
overlook problems.  Hence either reviewers run into failures
or the automatic pre-push selftest fails. The problem
I see with this is that it wastes the precios time of
the reviewers.

When I started contributing to Gluster, I found it to
be a big, big plus to have automatic regression runs
as a first step after submission, so that a reviewer
has the option to only start looking at the patch once
automatic tests have passed.

I completely agree that the fast-forward-only and
post-review-pre-merge-regression-run approach
is the way to go, only this way the original problem
described by Talur can be avoided.

But would it be possible to keep and even require some
amount of automatic pre-review test run (build and at
least some amount of runtimte test)?
It really prevents waste of time of reviewers/maintainers.

The problem with this is of course that it can increase
the (real) time needed to complete a review from submission
until upstream merge.

Just a few thoughts...

Cheers - Michael



signature.asc
Description: PGP signature
___
Gluster-devel mailing list
Gluster-devel@gluster.org
http://www.gluster.org/mailman/listinfo/gluster-devel

Re: [Gluster-devel] Gerrit review, submit type and Jenkins testing

[Raghavendra Talur]

On Jan 12, 2016 2:50 AM, "Niels de Vos"  wrote:
>
> On Fri, Jan 08, 2016 at 12:46:09PM +0530, Raghavendra Talur wrote:
> > On Fri, Jan 8, 2016 at 12:28 PM, Kaushal M  wrote:
> >
> > > On Fri, Jan 8, 2016 at 12:10 PM, Kaushal M 
wrote:
> > > > On Fri, Jan 8, 2016 at 12:03 PM, Raghavendra Talur <
rta...@redhat.com>
> > > wrote:
> > > >> Top posting, this is a very old thread.
> > > >>
> > > >> Keeping in view the recent NetBSD problems and the number of bugs
> > > creeping
> > > >> in, I suggest we do these things right now:
> > > >>
> > > >> a. Change the gerrit merge type to fast forward only.
> > > >> As explained below in the thread, with our current setup even if
both
> > > PatchA
> > > >> and PatchB pass regression separately when both are merged it is
> > > possible
> > > >> that a functional bug creeps in.
> > > >> This is the only solution to prevent that from happening.
> > > >> I will work with Kaushal to get this done.
> > > >>
> > > >> b. In Jenkins, remove gerrit trigger and make it a manual operation
> > > >
> > > > Making it manual might be too much work for maintainers. I suggest
(as
> > > > I've suggested before) we make regressions trigger when a change has
> > > > been reviewed +2 by a maintainer.
> > > >
> > >
> >
> > Makes sense. I have disabled it completely for now and lets keep it that
> > way till
> > developers realize it(a day should be enough). We will change this
trigger
> > to on Code Review +2 by tomorrow.
>
> Ah! And I have been wondering why patches don't get verified
> anymore :-/
>
> An email to the maintainers list as a heads up would have been welcome.
>
> How would we handle patches that get sent by maintainers? Most
> developers that do code reviews will only +1 those changes. Those will
> never get automatically regression tested then. I dont think a
> maintainer should +2 their own patch immediately either, that suggests
> no further reviews are needed.
>
> Niels

After realising this we configured Jenkins to be triggered either on code
review +2 or a verified +1. Even if it is the maintainer who sent the
patch, he/she can certainly give a +1 verified.

There seems to be some problem with both these type of events though. I
tried various combinations yesterday,  yet the events don't reach Jenkins.
I am afraid we will have to go back to patch set triggers until we update
our plugins.
___
Gluster-devel mailing list
Gluster-devel@gluster.org
http://www.gluster.org/mailman/listinfo/gluster-devel

Re: [Gluster-devel] Gerrit review, submit type and Jenkins testing

[Emmanuel Dreyfus]

Niels de Vos  wrote:

> How would we handle patches that get sent by maintainers? Most
> developers that do code reviews will only +1 those changes. Those will
> never get automatically regression tested then. I dont think a
> maintainer should +2 their own patch immediately either, that suggests
> no further reviews are needed.

Indeed it is a bit odd, but I just CR +2 my own changes...

-- 
Emmanuel Dreyfus
http://hcpnet.free.fr/pubz
m...@netbsd.org
___
Gluster-devel mailing list
Gluster-devel@gluster.org
http://www.gluster.org/mailman/listinfo/gluster-devel

Re: [Gluster-devel] Gerrit review, submit type and Jenkins testing

[Raghavendra Talur]

On Jan 12, 2016 3:44 AM, "Michael Adam"  wrote:
>
> On 2016-01-08 at 12:03 +0530, Raghavendra Talur wrote:
> > Top posting, this is a very old thread.
> >
> > Keeping in view the recent NetBSD problems and the number of bugs
creeping
> > in, I suggest we do these things right now:
> >
> > a. Change the gerrit merge type to fast forward only.
> > As explained below in the thread, with our current setup even if both
> > PatchA and PatchB pass regression separately when both are merged it is
> > possible that a functional bug creeps in.
> > This is the only solution to prevent that from happening.
> > I will work with Kaushal to get this done.
> >
> > b. In Jenkins, remove gerrit trigger and make it a manual operation
> >
> > Too many developers use the upstream infra as a test cluster and it is
> > *not*.
> > It is a verification mechanism for maintainers to ensure that the patch
> > does not cause regression.
> >
> > It is required that all developers run full regression on their machines
> > before asking for reviews.
>
> Hmm, I am not 100% sure I would underwrite that.
> I am coming from the Samba process, where we have exactly
> that: A developer should have run full selftest before
> submitting the change for review. Then after two samba
> team developers have given their review+ (counting the
> author), it can be pushed to our automatism that keeps
> rebasing on current upstream and running selftest until
> either selftest succeeds and is pushed as a fast forward
> or selftest fails.
>
> The reality is that people are lazy and think they know
> when they can skip selftest. But people are deceived and
> overlook problems.  Hence either reviewers run into failures
> or the automatic pre-push selftest fails. The problem
> I see with this is that it wastes the precios time of
> the reviewers.
>
> When I started contributing to Gluster, I found it to
> be a big, big plus to have automatic regression runs
> as a first step after submission, so that a reviewer
> has the option to only start looking at the patch once
> automatic tests have passed.
>
> I completely agree that the fast-forward-only and
> post-review-pre-merge-regression-run approach
> is the way to go, only this way the original problem
> described by Talur can be avoided.
>
> But would it be possible to keep and even require some
> amount of automatic pre-review test run (build and at
> least some amount of runtimte test)?
> It really prevents waste of time of reviewers/maintainers.
>
> The problem with this is of course that it can increase
> the (real) time needed to complete a review from submission
> until upstream merge.
>
> Just a few thoughts...
>
> Cheers - Michael
>

We had same concern from many other maintainers. I guess it would be better
if test runs both before and after review. With these changes we would have
removed test runs of work in progress patches.
___
Gluster-devel mailing list
Gluster-devel@gluster.org
http://www.gluster.org/mailman/listinfo/gluster-devel

Re: [Gluster-devel] Gerrit review, submit type and Jenkins testing

[Raghavendra Gowdappa]

- Original Message -
> From: "Raghavendra Talur" <rta...@redhat.com>
> To: "Michael Adam" <ob...@samba.org>
> Cc: "Gluster Devel" <gluster-devel@gluster.org>
> Sent: Tuesday, January 12, 2016 8:46:05 AM
> Subject: Re: [Gluster-devel] Gerrit review, submit type and Jenkins testing
> 
> 
> 
> 
> On Jan 12, 2016 3:44 AM, "Michael Adam" < ob...@samba.org > wrote:
> > 
> > On 2016-01-08 at 12:03 +0530, Raghavendra Talur wrote:
> > > Top posting, this is a very old thread.
> > > 
> > > Keeping in view the recent NetBSD problems and the number of bugs
> > > creeping
> > > in, I suggest we do these things right now:
> > > 
> > > a. Change the gerrit merge type to fast forward only.
> > > As explained below in the thread, with our current setup even if both
> > > PatchA and PatchB pass regression separately when both are merged it is
> > > possible that a functional bug creeps in.
> > > This is the only solution to prevent that from happening.
> > > I will work with Kaushal to get this done.
> > > 
> > > b. In Jenkins, remove gerrit trigger and make it a manual operation
> > > 
> > > Too many developers use the upstream infra as a test cluster and it is
> > > *not*.
> > > It is a verification mechanism for maintainers to ensure that the patch
> > > does not cause regression.
> > > 
> > > It is required that all developers run full regression on their machines
> > > before asking for reviews.
> > 
> > Hmm, I am not 100% sure I would underwrite that.
> > I am coming from the Samba process, where we have exactly
> > that: A developer should have run full selftest before
> > submitting the change for review. Then after two samba
> > team developers have given their review+ (counting the
> > author), it can be pushed to our automatism that keeps
> > rebasing on current upstream and running selftest until
> > either selftest succeeds and is pushed as a fast forward
> > or selftest fails.
> > 
> > The reality is that people are lazy and think they know
> > when they can skip selftest. But people are deceived and
> > overlook problems. Hence either reviewers run into failures
> > or the automatic pre-push selftest fails. The problem
> > I see with this is that it wastes the precios time of
> > the reviewers.
> > 
> > When I started contributing to Gluster, I found it to
> > be a big, big plus to have automatic regression runs
> > as a first step after submission, so that a reviewer
> > has the option to only start looking at the patch once
> > automatic tests have passed.
> > 
> > I completely agree that the fast-forward-only and
> > post-review-pre-merge-regression-run approach
> > is the way to go, only this way the original problem
> > described by Talur can be avoided.
> > 
> > But would it be possible to keep and even require some
> > amount of automatic pre-review test run (build and at
> > least some amount of runtimte test)?
> > It really prevents waste of time of reviewers/maintainers.
> > 
> > The problem with this is of course that it can increase
> > the (real) time needed to complete a review from submission
> > until upstream merge.
> > 
> > Just a few thoughts...
> > 
> > Cheers - Michael
> > 
> 
> We had same concern from many other maintainers. I guess it would be better
> if test runs both before and after review. With these changes we would have
> removed test runs of work in progress patches.

Yes. I think It would be better one set of regressions to be run before a +1. 
Normally it takes couple of iterations to +1 a patch. So, I think it would 
unnecessarily serialize regression runs and review. If reviews are run 
parallely, developers can work on regression failures parallely while others do 
the review.

> 
> ___
> Gluster-devel mailing list
> Gluster-devel@gluster.org
> http://www.gluster.org/mailman/listinfo/gluster-devel
___
Gluster-devel mailing list
Gluster-devel@gluster.org
http://www.gluster.org/mailman/listinfo/gluster-devel

Re: [Gluster-devel] Gerrit review, submit type and Jenkins testing

[Niels de Vos]

On Fri, Jan 08, 2016 at 12:46:09PM +0530, Raghavendra Talur wrote:
> On Fri, Jan 8, 2016 at 12:28 PM, Kaushal M  wrote:
> 
> > On Fri, Jan 8, 2016 at 12:10 PM, Kaushal M  wrote:
> > > On Fri, Jan 8, 2016 at 12:03 PM, Raghavendra Talur 
> > wrote:
> > >> Top posting, this is a very old thread.
> > >>
> > >> Keeping in view the recent NetBSD problems and the number of bugs
> > creeping
> > >> in, I suggest we do these things right now:
> > >>
> > >> a. Change the gerrit merge type to fast forward only.
> > >> As explained below in the thread, with our current setup even if both
> > PatchA
> > >> and PatchB pass regression separately when both are merged it is
> > possible
> > >> that a functional bug creeps in.
> > >> This is the only solution to prevent that from happening.
> > >> I will work with Kaushal to get this done.
> > >>
> > >> b. In Jenkins, remove gerrit trigger and make it a manual operation
> > >
> > > Making it manual might be too much work for maintainers. I suggest (as
> > > I've suggested before) we make regressions trigger when a change has
> > > been reviewed +2 by a maintainer.
> > >
> >
> 
> Makes sense. I have disabled it completely for now and lets keep it that
> way till
> developers realize it(a day should be enough). We will change this trigger
> to on Code Review +2 by tomorrow.

Ah! And I have been wondering why patches don't get verified
anymore :-/

An email to the maintainers list as a heads up would have been welcome.

How would we handle patches that get sent by maintainers? Most
developers that do code reviews will only +1 those changes. Those will
never get automatically regression tested then. I dont think a
maintainer should +2 their own patch immediately either, that suggests
no further reviews are needed.

Niels


signature.asc
Description: PGP signature
___
Gluster-devel mailing list
Gluster-devel@gluster.org
http://www.gluster.org/mailman/listinfo/gluster-devel

Re: [Gluster-devel] Gerrit review, submit type and Jenkins testing

[Raghavendra Talur]

Top posting, this is a very old thread.

Keeping in view the recent NetBSD problems and the number of bugs creeping
in, I suggest we do these things right now:

a. Change the gerrit merge type to fast forward only.
As explained below in the thread, with our current setup even if both
PatchA and PatchB pass regression separately when both are merged it is
possible that a functional bug creeps in.
This is the only solution to prevent that from happening.
I will work with Kaushal to get this done.

b. In Jenkins, remove gerrit trigger and make it a manual operation

Too many developers use the upstream infra as a test cluster and it is
*not*.
It is a verification mechanism for maintainers to ensure that the patch
does not cause regression.
It is required that all developers run full regression on their machines
before asking for reviews.
Reviewers should review the patch only when the developer has given a +1
verified on the patch.
Again, I will work with Kaushal to get this done.

P.S: Stop using the "universal" jenkins account to trigger jenkins build if
you are not a maintainer.
If you are a maintainer and don't have your own jenkins account then get
one soon!

Thanks,
Raghavendra Talur



On Tue, Nov 10, 2015 at 11:33 AM, Atin Mukherjee <atin.mukherje...@gmail.com
> wrote:

> -Atin
> Sent from one plus one
>
> On Nov 10, 2015 11:24 AM, "Kaushal M" <kshlms...@gmail.com> wrote:
> >
> > On Tue, Nov 10, 2015 at 9:24 AM, Raghavendra Gowdappa
> > <rgowd...@redhat.com> wrote:
> > >
> > >
> > > - Original Message -
> > >> From: "Raghavendra Talur" <rta...@redhat.com>
> > >> To: "Gluster Devel" <gluster-devel@gluster.org>
> > >> Sent: Tuesday, November 10, 2015 3:10:34 AM
> > >> Subject: [Gluster-devel] Gerrit review, submit type and Jenkins
> testing
> > >>
> > >> Hi,
> > >>
> > >> While trying to understand how our gerrit+jenkins setup works, I
> realized of
> > >> a possibility of allowing bugs to get in.
> > >>
> > >> Currently, our gerrit is setup to have cherry-pick as the submit
> type. Now
> > >> consider a case where:
> > >>
> > >> Dev1 sends a commit B with parent commit A(A is already merged).
> > >> Dev2 sends a commit C with parent commit A(A is already merged).
> > >>
> > >> Both the patches get +2 from Jenkins.
> > >>
> > >> Maintainer merges commit B from Dev1.
> > >> Another maintainer merges commit C from Dev2.
> > >>
> > >> If the two commits B and C changed code which had no merge conflicts
> but were
> > >> conflicting in logic,
> > >> then we have a master which has bugs.
> > >>
> > >> If Dev3 now sends a commit D with re-based master as parent, we have
> the
> > >> following cases:
> > >>
> > >> 1. If bug introduced above is not racy, we have tests always failing
> for Dev3
> > >> on commit D. Tests that fail would be from components that commit B
> and C
> > >> changed. Dev3 has no idea on how to fix them and has to enlist help
> from
> > >> Dev1 and Dev2.
> > >>
> > >> 2. If bug introduced above is racy, then there is a probability that
> Dev3
> > >> escapes from this trouble and someone else will bear it later. Even
> if the
> > >> racy code is hit and test fails, Dev3 will probably re-trigger the
> tests
> > >> given that they failed for a component which is not related to
> his/her code
> > >> and the bug stays in code longer.
> > >>
> > >> The most obvious but not practical solution to the above problem is
> to change
> > >> the submit type in gerrit to "fast-forward only". It would then
> ensure that
> > >> once commit B is merged, Dev2 has to re-base and re-run the tests on
> commit
> > >> C with commit B as parent, before it could be merged. It is not
> practical
> > >> because it will cause all patches in review to get re-based and
> re-triggered
> > >> whenever a patch is merged.
> > >>
> > >> A little modification to the above solution would be to
> > >>
> > >>
> > >> * change submit type to fast-forward only
> > >> * don't run any jenkins job on patches till they get +2 from
> reviewers
> > >> * once a +2 is given, run jenkins job on patch and automatically
> submit
> > >> it if test passes.
> > >> * automatically rebase a

Re: [Gluster-devel] Gerrit review, submit type and Jenkins testing

[Kaushal M]

On Fri, Jan 8, 2016 at 12:10 PM, Kaushal M  wrote:
> On Fri, Jan 8, 2016 at 12:03 PM, Raghavendra Talur  wrote:
>> Top posting, this is a very old thread.
>>
>> Keeping in view the recent NetBSD problems and the number of bugs creeping
>> in, I suggest we do these things right now:
>>
>> a. Change the gerrit merge type to fast forward only.
>> As explained below in the thread, with our current setup even if both PatchA
>> and PatchB pass regression separately when both are merged it is possible
>> that a functional bug creeps in.
>> This is the only solution to prevent that from happening.
>> I will work with Kaushal to get this done.
>>
>> b. In Jenkins, remove gerrit trigger and make it a manual operation
>
> Making it manual might be too much work for maintainers. I suggest (as
> I've suggested before) we make regressions trigger when a change has
> been reviewed +2 by a maintainer.
>
>>
>> Too many developers use the upstream infra as a test cluster and it is
>> *not*.
>> It is a verification mechanism for maintainers to ensure that the patch does
>> not cause regression.
>> It is required that all developers run full regression on their machines
>> before asking for reviews.
>> Reviewers should review the patch only when the developer has given a +1
>> verified on the patch.
>> Again, I will work with Kaushal to get this done.
>>
>> P.S: Stop using the "universal" jenkins account to trigger jenkins build if
>> you are not a maintainer.
>> If you are a maintainer and don't have your own jenkins account then get one
>> soon!
>
> I think I'll go ahead and remove this account.

This is done now.

>
>>
>> Thanks,
>> Raghavendra Talur
>>
___
Gluster-devel mailing list
Gluster-devel@gluster.org
http://www.gluster.org/mailman/listinfo/gluster-devel

Re: [Gluster-devel] Gerrit review, submit type and Jenkins testing

[Raghavendra Talur]

On Fri, Jan 8, 2016 at 12:28 PM, Kaushal M  wrote:

> On Fri, Jan 8, 2016 at 12:10 PM, Kaushal M  wrote:
> > On Fri, Jan 8, 2016 at 12:03 PM, Raghavendra Talur 
> wrote:
> >> Top posting, this is a very old thread.
> >>
> >> Keeping in view the recent NetBSD problems and the number of bugs
> creeping
> >> in, I suggest we do these things right now:
> >>
> >> a. Change the gerrit merge type to fast forward only.
> >> As explained below in the thread, with our current setup even if both
> PatchA
> >> and PatchB pass regression separately when both are merged it is
> possible
> >> that a functional bug creeps in.
> >> This is the only solution to prevent that from happening.
> >> I will work with Kaushal to get this done.
> >>
> >> b. In Jenkins, remove gerrit trigger and make it a manual operation
> >
> > Making it manual might be too much work for maintainers. I suggest (as
> > I've suggested before) we make regressions trigger when a change has
> > been reviewed +2 by a maintainer.
> >
>

Makes sense. I have disabled it completely for now and lets keep it that
way till
developers realize it(a day should be enough). We will change this trigger
to on Code Review +2 by tomorrow.

>>
> >> Too many developers use the upstream infra as a test cluster and it is
> >> *not*.
> >> It is a verification mechanism for maintainers to ensure that the patch
> does
> >> not cause regression.
> >> It is required that all developers run full regression on their machines
> >> before asking for reviews.
> >> Reviewers should review the patch only when the developer has given a +1
> >> verified on the patch.
> >> Again, I will work with Kaushal to get this done.
> >>
> >> P.S: Stop using the "universal" jenkins account to trigger jenkins
> build if
> >> you are not a maintainer.
> >> If you are a maintainer and don't have your own jenkins account then
> get one
> >> soon!
> >
> > I think I'll go ahead and remove this account.
>
> This is done now.
>
> >
> >>
> >> Thanks,
> >> Raghavendra Talur
> >>
>
___
Gluster-devel mailing list
Gluster-devel@gluster.org
http://www.gluster.org/mailman/listinfo/gluster-devel

Re: [Gluster-devel] Gerrit review, submit type and Jenkins testing

[Kaushal M]

On Fri, Jan 8, 2016 at 12:03 PM, Raghavendra Talur  wrote:
> Top posting, this is a very old thread.
>
> Keeping in view the recent NetBSD problems and the number of bugs creeping
> in, I suggest we do these things right now:
>
> a. Change the gerrit merge type to fast forward only.
> As explained below in the thread, with our current setup even if both PatchA
> and PatchB pass regression separately when both are merged it is possible
> that a functional bug creeps in.
> This is the only solution to prevent that from happening.
> I will work with Kaushal to get this done.
>
> b. In Jenkins, remove gerrit trigger and make it a manual operation

Making it manual might be too much work for maintainers. I suggest (as
I've suggested before) we make regressions trigger when a change has
been reviewed +2 by a maintainer.

>
> Too many developers use the upstream infra as a test cluster and it is
> *not*.
> It is a verification mechanism for maintainers to ensure that the patch does
> not cause regression.
> It is required that all developers run full regression on their machines
> before asking for reviews.
> Reviewers should review the patch only when the developer has given a +1
> verified on the patch.
> Again, I will work with Kaushal to get this done.
>
> P.S: Stop using the "universal" jenkins account to trigger jenkins build if
> you are not a maintainer.
> If you are a maintainer and don't have your own jenkins account then get one
> soon!

I think I'll go ahead and remove this account.

>
> Thanks,
> Raghavendra Talur
>
___
Gluster-devel mailing list
Gluster-devel@gluster.org
http://www.gluster.org/mailman/listinfo/gluster-devel

[Gluster-devel] Gerrit review, submit type and Jenkins testing

[Raghavendra Talur]

Hi,

While trying to understand how our gerrit+jenkins setup works, I realized
of a possibility of allowing bugs to get in.

Currently, our gerrit is setup to have cherry-pick as the submit type. Now
consider a case where:

Dev1 sends a commit B with parent commit A(A is already merged).
Dev2 sends a commit C with parent commit A(A is already merged).

Both the patches get +2 from Jenkins.

Maintainer merges commit B from Dev1.
Another maintainer merges commit C from Dev2.

If the two commits B and C changed code which had no merge conflicts but
were conflicting in logic,
then we have a master which has bugs.

If Dev3 now sends a commit D with re-based master as parent, we have the
following cases:

1. If bug introduced above is not racy, we have tests always failing for
Dev3 on commit D. Tests that fail would be from components that commit B
and C changed. Dev3 has no idea on how to fix them and has to enlist help
from Dev1 and Dev2.

2. If bug introduced above is racy, then there is a probability that Dev3
escapes from this trouble and someone else will bear it later. Even if the
racy code is hit and test fails, Dev3 will probably re-trigger the tests
given that they failed for a component which is not related to his/her code
and the bug stays in code longer.

The most obvious but not practical solution to the above problem is to
change the submit type in gerrit to "fast-forward only". It would then
ensure that once commit B is merged, Dev2 has to re-base and re-run the
tests on commit C with commit B as parent, before it could be merged. It is
not practical because it will cause all patches in review to get re-based
and re-triggered whenever a patch is merged.

A little modification to the above solution would be to

   - change submit type to fast-forward only
   - don't run any jenkins job on patches till they get +2 from reviewers
   - once a +2 is given, run jenkins job on patch and automatically submit
   it if test passes.
   - automatically rebase all patches on review with new master and mark
   conflict if merge conflict arises.

As a side effect of this, Dev would now be forced to run a complete
regression on dev machine before sending a patch for review.

Any thoughts on the above solutions or other suggestions?

Thanks,
Raghavendra Talur
___
Gluster-devel mailing list
Gluster-devel@gluster.org
http://www.gluster.org/mailman/listinfo/gluster-devel

Re: [Gluster-devel] Gerrit review, submit type and Jenkins testing

[Atin Mukherjee]

-Atin
Sent from one plus one
On Nov 10, 2015 11:24 AM, "Kaushal M" <kshlms...@gmail.com> wrote:
>
> On Tue, Nov 10, 2015 at 9:24 AM, Raghavendra Gowdappa
> <rgowd...@redhat.com> wrote:
> >
> >
> > - Original Message -
> >> From: "Raghavendra Talur" <rta...@redhat.com>
> >> To: "Gluster Devel" <gluster-devel@gluster.org>
> >> Sent: Tuesday, November 10, 2015 3:10:34 AM
> >> Subject: [Gluster-devel] Gerrit review, submit type and Jenkins testing
> >>
> >> Hi,
> >>
> >> While trying to understand how our gerrit+jenkins setup works, I
realized of
> >> a possibility of allowing bugs to get in.
> >>
> >> Currently, our gerrit is setup to have cherry-pick as the submit type.
Now
> >> consider a case where:
> >>
> >> Dev1 sends a commit B with parent commit A(A is already merged).
> >> Dev2 sends a commit C with parent commit A(A is already merged).
> >>
> >> Both the patches get +2 from Jenkins.
> >>
> >> Maintainer merges commit B from Dev1.
> >> Another maintainer merges commit C from Dev2.
> >>
> >> If the two commits B and C changed code which had no merge conflicts
but were
> >> conflicting in logic,
> >> then we have a master which has bugs.
> >>
> >> If Dev3 now sends a commit D with re-based master as parent, we have
the
> >> following cases:
> >>
> >> 1. If bug introduced above is not racy, we have tests always failing
for Dev3
> >> on commit D. Tests that fail would be from components that commit B
and C
> >> changed. Dev3 has no idea on how to fix them and has to enlist help
from
> >> Dev1 and Dev2.
> >>
> >> 2. If bug introduced above is racy, then there is a probability that
Dev3
> >> escapes from this trouble and someone else will bear it later. Even if
the
> >> racy code is hit and test fails, Dev3 will probably re-trigger the
tests
> >> given that they failed for a component which is not related to his/her
code
> >> and the bug stays in code longer.
> >>
> >> The most obvious but not practical solution to the above problem is to
change
> >> the submit type in gerrit to "fast-forward only". It would then ensure
that
> >> once commit B is merged, Dev2 has to re-base and re-run the tests on
commit
> >> C with commit B as parent, before it could be merged. It is not
practical
> >> because it will cause all patches in review to get re-based and
re-triggered
> >> whenever a patch is merged.
> >>
> >> A little modification to the above solution would be to
> >>
> >>
> >> * change submit type to fast-forward only
> >> * don't run any jenkins job on patches till they get +2 from
reviewers
> >> * once a +2 is given, run jenkins job on patch and automatically
submit
> >> it if test passes.
> >> * automatically rebase all patches on review with new master and
mark
> >> conflict if merge conflict arises.
> >
> > Seems like a good suggestion. How about a slight variation to the above
process? Can we run one initial set of regression immediately after
submission, but before any reviews? That way reviewers can prioritize those
patches that have passed regression over the ones that have failed? Flip
side is that minimum two sets of regressions are needed to merge any patch.
I am making this suggestion with the assumption that dev/reviewer time is
more precious than machine time. Of course, this will have issues with
patches that need to get in urgently (user/customer hot fix etc) where time
is a constraint. But that can be worked around on a case-by-case basis.
>
> We would still be running smoke, which would catch any very obvious
> mistakes, isn't this enough?
>
> Regarding the initial regression run, would it include the complete
> regression suite or just a subset. If it is the complete set, then it
> would be no different from what we are doing now. If it is a subset,
> then we will need to come up with a subset of the regression suite
> that catches most of obvious mistakes. We've had discussions several
> times (don't remember if it was on the mailing lists, but I have had
> conversations) about doing this. Every time we ended up on the
> question of how we choose the subset, which is where we stopped.
How about running tests/basic/*.t? I know coverage wise this isn't good
enough, but still better than running everything or nothing.
>
> >
> >>
> >> As a side effect of this, Dev would now be forced to run a complete
> >&g

Re: [Gluster-devel] Gerrit review, submit type and Jenkins testing

[Kaushal M]

On Tue, Nov 10, 2015 at 9:24 AM, Raghavendra Gowdappa
<rgowd...@redhat.com> wrote:
>
>
> - Original Message -
>> From: "Raghavendra Talur" <rta...@redhat.com>
>> To: "Gluster Devel" <gluster-devel@gluster.org>
>> Sent: Tuesday, November 10, 2015 3:10:34 AM
>> Subject: [Gluster-devel] Gerrit review, submit type and Jenkins testing
>>
>> Hi,
>>
>> While trying to understand how our gerrit+jenkins setup works, I realized of
>> a possibility of allowing bugs to get in.
>>
>> Currently, our gerrit is setup to have cherry-pick as the submit type. Now
>> consider a case where:
>>
>> Dev1 sends a commit B with parent commit A(A is already merged).
>> Dev2 sends a commit C with parent commit A(A is already merged).
>>
>> Both the patches get +2 from Jenkins.
>>
>> Maintainer merges commit B from Dev1.
>> Another maintainer merges commit C from Dev2.
>>
>> If the two commits B and C changed code which had no merge conflicts but were
>> conflicting in logic,
>> then we have a master which has bugs.
>>
>> If Dev3 now sends a commit D with re-based master as parent, we have the
>> following cases:
>>
>> 1. If bug introduced above is not racy, we have tests always failing for Dev3
>> on commit D. Tests that fail would be from components that commit B and C
>> changed. Dev3 has no idea on how to fix them and has to enlist help from
>> Dev1 and Dev2.
>>
>> 2. If bug introduced above is racy, then there is a probability that Dev3
>> escapes from this trouble and someone else will bear it later. Even if the
>> racy code is hit and test fails, Dev3 will probably re-trigger the tests
>> given that they failed for a component which is not related to his/her code
>> and the bug stays in code longer.
>>
>> The most obvious but not practical solution to the above problem is to change
>> the submit type in gerrit to "fast-forward only". It would then ensure that
>> once commit B is merged, Dev2 has to re-base and re-run the tests on commit
>> C with commit B as parent, before it could be merged. It is not practical
>> because it will cause all patches in review to get re-based and re-triggered
>> whenever a patch is merged.
>>
>> A little modification to the above solution would be to
>>
>>
>> * change submit type to fast-forward only
>> * don't run any jenkins job on patches till they get +2 from reviewers
>> * once a +2 is given, run jenkins job on patch and automatically submit
>> it if test passes.
>> * automatically rebase all patches on review with new master and mark
>> conflict if merge conflict arises.
>
> Seems like a good suggestion. How about a slight variation to the above 
> process? Can we run one initial set of regression immediately after 
> submission, but before any reviews? That way reviewers can prioritize those 
> patches that have passed regression over the ones that have failed? Flip side 
> is that minimum two sets of regressions are needed to merge any patch. I am 
> making this suggestion with the assumption that dev/reviewer time is more 
> precious than machine time. Of course, this will have issues with patches 
> that need to get in urgently (user/customer hot fix etc) where time is a 
> constraint. But that can be worked around on a case-by-case basis.

We would still be running smoke, which would catch any very obvious
mistakes, isn't this enough?

Regarding the initial regression run, would it include the complete
regression suite or just a subset. If it is the complete set, then it
would be no different from what we are doing now. If it is a subset,
then we will need to come up with a subset of the regression suite
that catches most of obvious mistakes. We've had discussions several
times (don't remember if it was on the mailing lists, but I have had
conversations) about doing this. Every time we ended up on the
question of how we choose the subset, which is where we stopped.

>
>>
>> As a side effect of this, Dev would now be forced to run a complete
>> regression on dev machine before sending a patch for review.
>>
>> Any thoughts on the above solutions or other suggestions?
>>
>> Thanks,
>> Raghavendra Talur
>>
>>
>>
>>
>>
>>
>>
>>
>>
>> ___
>> Gluster-devel mailing list
>> Gluster-devel@gluster.org
>> http://www.gluster.org/mailman/listinfo/gluster-devel
> ___
> Gluster-devel mailing list
> Gluster-devel@gluster.org
> http://www.gluster.org/mailman/listinfo/gluster-devel
___
Gluster-devel mailing list
Gluster-devel@gluster.org
http://www.gluster.org/mailman/listinfo/gluster-devel

Re: [Gluster-devel] Gerrit review, submit type and Jenkins testing

[Atin Mukherjee]

On 11/10/2015 03:10 AM, Raghavendra Talur wrote:
> Hi,
> 
> While trying to understand how our gerrit+jenkins setup works, I
> realized of a possibility of allowing bugs to get in.
> 
> Currently, our gerrit is setup to have cherry-pick as the submit type.
> Now consider a case where:
> 
> Dev1 sends a commit B with parent commit A(A is already merged).
> Dev2 sends a commit C with parent commit A(A is already merged).
> 
> Both the patches get +2 from Jenkins.
> 
> Maintainer merges commit B from Dev1.
> Another maintainer merges commit C from Dev2.
> 
> If the two commits B and C changed code which had no merge conflicts but
> were conflicting in logic,
> then we have a master which has bugs.
> 
> If Dev3 now sends a commit D with re-based master as parent, we have the
> following cases:
> 
> 1. If bug introduced above is not racy, we have tests always failing for
> Dev3 on commit D. Tests that fail would be from components that commit B
> and C changed. Dev3 has no idea on how to fix them and has to enlist
> help from Dev1 and Dev2.
> 
> 2. If bug introduced above is racy, then there is a probability that
> Dev3 escapes from this trouble and someone else will bear it later. Even
> if the racy code is hit and test fails, Dev3 will probably re-trigger
> the tests given that they failed for a component which is not related to
> his/her code and the bug stays in code longer.
> 
> The most obvious but not practical solution to the above problem is to
> change the submit type in gerrit to "fast-forward only". It would then
> ensure that once commit B is merged, Dev2 has to re-base and re-run the
> tests on commit C with commit B as parent, before it could be merged. It
> is not practical because it will cause all patches in review to get
> re-based and re-triggered whenever a patch is merged.
> 
> A little modification to the above solution would be to 
> 
>   * change submit type to fast-forward only
>   * don't run any jenkins job on patches till they get +2 from reviewers
>   * once a +2 is given, run jenkins job on patch and automatically
> submit it if test passes.
>   * automatically rebase all patches on review with new master and mark
> conflict if merge conflict arises.
The overall idea looks good to me, however I'd be bit hesitant to give a
+2 before seeing the regression votes until and unless the patch is
pretty straight forwad. For me +1 sounds to be a good level to trigger
the regression. Once the regression passes, maintainers can +2 a patch
and then merge it manually. And then the 4th point follows.

Thoughts?

~Atin
> 
> As a side effect of this, Dev would now be forced to run a complete
> regression on dev machine before sending a patch for review.
> 
> Any thoughts on the above solutions or other suggestions?
> 
> Thanks,
> Raghavendra Talur
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> ___
> Gluster-devel mailing list
> Gluster-devel@gluster.org
> http://www.gluster.org/mailman/listinfo/gluster-devel
> 
___
Gluster-devel mailing list
Gluster-devel@gluster.org
http://www.gluster.org/mailman/listinfo/gluster-devel

Re: [Gluster-devel] Gerrit review, submit type and Jenkins testing

[Kaushal M]

On Tue, Nov 10, 2015 at 3:10 AM, Raghavendra Talur  wrote:
> Hi,
>
> While trying to understand how our gerrit+jenkins setup works, I realized of
> a possibility of allowing bugs to get in.
>
> Currently, our gerrit is setup to have cherry-pick as the submit type. Now
> consider a case where:
>
> Dev1 sends a commit B with parent commit A(A is already merged).
> Dev2 sends a commit C with parent commit A(A is already merged).
>
> Both the patches get +2 from Jenkins.
>
> Maintainer merges commit B from Dev1.
> Another maintainer merges commit C from Dev2.
>
> If the two commits B and C changed code which had no merge conflicts but
> were conflicting in logic,
> then we have a master which has bugs.
>
> If Dev3 now sends a commit D with re-based master as parent, we have the
> following cases:
>
> 1. If bug introduced above is not racy, we have tests always failing for
> Dev3 on commit D. Tests that fail would be from components that commit B and
> C changed. Dev3 has no idea on how to fix them and has to enlist help from
> Dev1 and Dev2.
>
> 2. If bug introduced above is racy, then there is a probability that Dev3
> escapes from this trouble and someone else will bear it later. Even if the
> racy code is hit and test fails, Dev3 will probably re-trigger the tests
> given that they failed for a component which is not related to his/her code
> and the bug stays in code longer.
>
> The most obvious but not practical solution to the above problem is to
> change the submit type in gerrit to "fast-forward only". It would then
> ensure that once commit B is merged, Dev2 has to re-base and re-run the
> tests on commit C with commit B as parent, before it could be merged. It is
> not practical because it will cause all patches in review to get re-based
> and re-triggered whenever a patch is merged.
>
> A little modification to the above solution would be to
>
> change submit type to fast-forward only
> don't run any jenkins job on patches till they get +2 from reviewers
> once a +2 is given, run jenkins job on patch and automatically submit it if
> test passes.
> automatically rebase all patches on review with new master and mark conflict
> if merge conflict arises.

Have you checked if this is even possible?

>
> As a side effect of this, Dev would now be forced to run a complete
> regression on dev machine before sending a patch for review.
>
> Any thoughts on the above solutions or other suggestions?
>
> Thanks,
> Raghavendra Talur
>
>
>
>
>
>
>
>
>
> ___
> Gluster-devel mailing list
> Gluster-devel@gluster.org
> http://www.gluster.org/mailman/listinfo/gluster-devel
___
Gluster-devel mailing list
Gluster-devel@gluster.org
http://www.gluster.org/mailman/listinfo/gluster-devel

Re: [Gluster-devel] Gerrit review, submit type and Jenkins testing

[Raghavendra Gowdappa]

- Original Message -
> From: "Raghavendra Talur" <rta...@redhat.com>
> To: "Gluster Devel" <gluster-devel@gluster.org>
> Sent: Tuesday, November 10, 2015 3:10:34 AM
> Subject: [Gluster-devel] Gerrit review, submit type and Jenkins testing
> 
> Hi,
> 
> While trying to understand how our gerrit+jenkins setup works, I realized of
> a possibility of allowing bugs to get in.
> 
> Currently, our gerrit is setup to have cherry-pick as the submit type. Now
> consider a case where:
> 
> Dev1 sends a commit B with parent commit A(A is already merged).
> Dev2 sends a commit C with parent commit A(A is already merged).
> 
> Both the patches get +2 from Jenkins.
> 
> Maintainer merges commit B from Dev1.
> Another maintainer merges commit C from Dev2.
> 
> If the two commits B and C changed code which had no merge conflicts but were
> conflicting in logic,
> then we have a master which has bugs.
> 
> If Dev3 now sends a commit D with re-based master as parent, we have the
> following cases:
> 
> 1. If bug introduced above is not racy, we have tests always failing for Dev3
> on commit D. Tests that fail would be from components that commit B and C
> changed. Dev3 has no idea on how to fix them and has to enlist help from
> Dev1 and Dev2.
> 
> 2. If bug introduced above is racy, then there is a probability that Dev3
> escapes from this trouble and someone else will bear it later. Even if the
> racy code is hit and test fails, Dev3 will probably re-trigger the tests
> given that they failed for a component which is not related to his/her code
> and the bug stays in code longer.
> 
> The most obvious but not practical solution to the above problem is to change
> the submit type in gerrit to "fast-forward only". It would then ensure that
> once commit B is merged, Dev2 has to re-base and re-run the tests on commit
> C with commit B as parent, before it could be merged. It is not practical
> because it will cause all patches in review to get re-based and re-triggered
> whenever a patch is merged.
> 
> A little modification to the above solution would be to
> 
> 
> * change submit type to fast-forward only
> * don't run any jenkins job on patches till they get +2 from reviewers
> * once a +2 is given, run jenkins job on patch and automatically submit
> it if test passes.
> * automatically rebase all patches on review with new master and mark
> conflict if merge conflict arises.

Seems like a good suggestion. How about a slight variation to the above 
process? Can we run one initial set of regression immediately after submission, 
but before any reviews? That way reviewers can prioritize those patches that 
have passed regression over the ones that have failed? Flip side is that 
minimum two sets of regressions are needed to merge any patch. I am making this 
suggestion with the assumption that dev/reviewer time is more precious than 
machine time. Of course, this will have issues with patches that need to get in 
urgently (user/customer hot fix etc) where time is a constraint. But that can 
be worked around on a case-by-case basis.

> 
> As a side effect of this, Dev would now be forced to run a complete
> regression on dev machine before sending a patch for review.
> 
> Any thoughts on the above solutions or other suggestions?
> 
> Thanks,
> Raghavendra Talur
> 
> 
> 
> 
> 
> 
> 
> 
> 
> ___
> Gluster-devel mailing list
> Gluster-devel@gluster.org
> http://www.gluster.org/mailman/listinfo/gluster-devel
___
Gluster-devel mailing list
Gluster-devel@gluster.org
http://www.gluster.org/mailman/listinfo/gluster-devel