[PATCH] Fix implementation of CTDB control PROCESS_EXISTS (bug 13012)

classic Classic list List threaded Threaded
10 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH] Fix implementation of CTDB control PROCESS_EXISTS (bug 13012)

Samba - samba-technical mailing list
Hi,

CTDB daemon should check processes that are connected to the daemon as
clients (e.g. smbd) and not check any random processes on the system.

Please review and push.

Amitay.

ctdb.patch (18K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix implementation of CTDB control PROCESS_EXISTS (bug 13012)

Samba - samba-technical mailing list
On Thu, 7 Sep 2017 17:39:48 +1000, Amitay Isaacs via samba-technical
<[hidden email]> wrote:

> CTDB daemon should check processes that are connected to the daemon as
> clients (e.g. smbd) and not check any random processes on the system.
>
> Please review and push.

Reviewed-by: Martin Schwenke <[hidden email]>

peace & happiness,
martin

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix implementation of CTDB control PROCESS_EXISTS (bug 13012)

Samba - samba-technical mailing list
On Fri, 8 Sep 2017 16:50:03 +1000, Martin Schwenke via samba-technical
<[hidden email]> wrote:

> On Thu, 7 Sep 2017 17:39:48 +1000, Amitay Isaacs via samba-technical
> <[hidden email]> wrote:
>
> > CTDB daemon should check processes that are connected to the daemon as
> > clients (e.g. smbd) and not check any random processes on the system.
> >
> > Please review and push.  
>
> Reviewed-by: Martin Schwenke <[hidden email]>

... and pushed.

peace & happiness,
martin

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix implementation of CTDB control PROCESS_EXISTS (bug 13012)

Samba - samba-technical mailing list
On Fri, 8 Sep 2017 21:51:16 +1000, Martin Schwenke via samba-technical
<[hidden email]> wrote:

> On Fri, 8 Sep 2017 16:50:03 +1000, Martin Schwenke via samba-technical
> <[hidden email]> wrote:
>
> > On Thu, 7 Sep 2017 17:39:48 +1000, Amitay Isaacs via samba-technical
> > <[hidden email]> wrote:
> >  
> > > CTDB daemon should check processes that are connected to the daemon as
> > > clients (e.g. smbd) and not check any random processes on the system.
> > >
> > > Please review and push.    
> >
> > Reviewed-by: Martin Schwenke <[hidden email]>  
>
> ... and pushed.
There's a little race in the test here.  The attached patch seems to
fix it.

Please review and maybe push...

peace & happiness,
martin

0001-ctdb-tests-Wait-up-to-30-seconds-for-process-to-be-r.patch (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix implementation of CTDB control PROCESS_EXISTS (bug 13012)

Samba - samba-technical mailing list
On Tue, Sep 12, 2017 at 3:30 PM, Martin Schwenke via samba-technical <
[hidden email]> wrote:

> On Fri, 8 Sep 2017 21:51:16 +1000, Martin Schwenke via samba-technical
> <[hidden email]> wrote:
>
> > On Fri, 8 Sep 2017 16:50:03 +1000, Martin Schwenke via samba-technical
> > <[hidden email]> wrote:
> >
> > > On Thu, 7 Sep 2017 17:39:48 +1000, Amitay Isaacs via samba-technical
> > > <[hidden email]> wrote:
> > >
> > > > CTDB daemon should check processes that are connected to the daemon
> as
> > > > clients (e.g. smbd) and not check any random processes on the system.
> > > >
> > > > Please review and push.
> > >
> > > Reviewed-by: Martin Schwenke <[hidden email]>
> >
> > ... and pushed.
>
> There's a little race in the test here.  The attached patch seems to
> fix it.
>
>
Thanks for identifying the race condition.

We also need a fix to test_wrap to correctly set up the PATH.

Attached patches include:
- Martin's fix with missing BUG tag and my reviewed-by.
- Fix to test_wrap.

Please review and push.

Amitay.

ctdb.patch (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix implementation of CTDB control PROCESS_EXISTS (bug 13012)

Samba - samba-technical mailing list
On Tue, 2017-09-12 at 16:00 +1000, Amitay Isaacs via samba-technical
wrote:
>
> Thanks for identifying the race condition.

I'm so glad you folks figured that one out.  It just started showing up
in builds on the Catalyst Cloud and I wasn't looking forward to digging
into it!

Thanks,

Andrew Bartlett

--
Andrew Bartlett                       http://samba.org/~abartlet/
Authentication Developer, Samba Team  http://samba.org
Samba Developer, Catalyst IT          http://catalyst.net.nz/services/samba


Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix implementation of CTDB control PROCESS_EXISTS (bug 13012)

Samba - samba-technical mailing list
On Tue, 12 Sep 2017 18:06:13 +1200, Andrew Bartlett
<[hidden email]> wrote:

> On Tue, 2017-09-12 at 16:00 +1000, Amitay Isaacs via samba-technical
> wrote:
> >
> > Thanks for identifying the race condition.  
>
> I'm so glad you folks figured that one out.  It just started showing up
> in builds on the Catalyst Cloud and I wasn't looking forward to digging
> into it!

I'm 90% sure that's it.  It is has passed a couple of private
autobuilds... but let's see.

If you want to add that patch to the trees you're testing then that
would give us all extra confidence...  ;-)

peace & happiness,
martin

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix implementation of CTDB control PROCESS_EXISTS (bug 13012)

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On Tue, 12 Sep 2017 16:00:35 +1000, Amitay Isaacs <[hidden email]>
wrote:

> On Tue, Sep 12, 2017 at 3:30 PM, Martin Schwenke via samba-technical <
> [hidden email]> wrote:
>
> > On Fri, 8 Sep 2017 21:51:16 +1000, Martin Schwenke via samba-technical
> > <[hidden email]> wrote:
> >  
> > > On Fri, 8 Sep 2017 16:50:03 +1000, Martin Schwenke via samba-technical
> > > <[hidden email]> wrote:
> > >  
>  [...]  
>  [...]  
> > as  
>  [...]  
>  [...]  
> > >
> > > ... and pushed.  
> >
> > There's a little race in the test here.  The attached patch seems to
> > fix it.
> >
> >  
> Thanks for identifying the race condition.
>
> We also need a fix to test_wrap to correctly set up the PATH.
>
> Attached patches include:
> - Martin's fix with missing BUG tag and my reviewed-by.
> - Fix to test_wrap.
>
> Please review and push.

Reviewed-by: Martin Schwenke <[hidden email]>

Mega-push coming...  :-)

peace & happiness,
martin

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix implementation of CTDB control PROCESS_EXISTS (bug 13012)

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On Tue, 2017-09-12 at 17:06 +1000, Martin Schwenke wrote:

> On Tue, 12 Sep 2017 18:06:13 +1200, Andrew Bartlett
> <[hidden email]> wrote:
>
> > On Tue, 2017-09-12 at 16:00 +1000, Amitay Isaacs via samba-technical
> > wrote:
> > >
> > > Thanks for identifying the race condition.  
> >
> > I'm so glad you folks figured that one out.  It just started showing up
> > in builds on the Catalyst Cloud and I wasn't looking forward to digging
> > into it!
>
> I'm 90% sure that's it.  It is has passed a couple of private
> autobuilds... but let's see.
>
> If you want to add that patch to the trees you're testing then that
> would give us all extra confidence...  ;-)

I'm running some builds on master with that patch.

Andrew Bartlett

--
Andrew Bartlett                       http://samba.org/~abartlet/
Authentication Developer, Samba Team  http://samba.org
Samba Developer, Catalyst IT          http://catalyst.net.nz/services/samba


Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix implementation of CTDB control PROCESS_EXISTS (bug 13012)

Samba - samba-technical mailing list
On Tue, 12 Sep 2017 19:12:54 +1200, Andrew Bartlett
<[hidden email]> wrote:

> On Tue, 2017-09-12 at 17:06 +1000, Martin Schwenke wrote:
> > On Tue, 12 Sep 2017 18:06:13 +1200, Andrew Bartlett
> > <[hidden email]> wrote:
> >  
> > > On Tue, 2017-09-12 at 16:00 +1000, Amitay Isaacs via samba-technical
> > > wrote:  
>  [...]  
> > >
> > > I'm so glad you folks figured that one out.  It just started showing up
> > > in builds on the Catalyst Cloud and I wasn't looking forward to digging
> > > into it!  
> >
> > I'm 90% sure that's it.  It is has passed a couple of private
> > autobuilds... but let's see.
> >
> > If you want to add that patch to the trees you're testing then that
> > would give us all extra confidence...  ;-)  
>
> I'm running some builds on master with that patch.

Thanks!  :-)

I'm going to push it along with other stuff now.  Even if it doesn't
fix the issue people are seeing it is still an improvement.  :-)

peace & happiness,
martin