Change in vdsm[master]: acceptor: stop to double close acceptor

[Piotr Kliczewski]

Piotr Kliczewski has uploaded a new change for review.

Change subject: acceptor: stop to double close acceptor
......................................................................

acceptor: stop to double close acceptor

When reactor is stopped it closes all dispatchers so there is no need to
close acceptor one more time.


Change-Id: I9a14cfa84c34241dbb511c0348109073b6865087
Signed-off-by: Piotr Kliczewski <piotr.kliczewski(a)gmail.com&gt;
---
M lib/vdsm/protocoldetector.py
1 file changed, 0 insertions(+), 1 deletion(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/85/63685/1

diff --git a/lib/vdsm/protocoldetector.py b/lib/vdsm/protocoldetector.py
index 196a2ab..40f3c0d 100644
--- a/lib/vdsm/protocoldetector.py
+++ b/lib/vdsm/protocoldetector.py
@@ -208,7 +208,6 @@
     def stop(self):
         self.log.debug("Stopping Acceptor")
         self._reactor.stop()
-        self._acceptor.close()
 
 
 class _CannotDetectProtocol(Exception):


-- 
To view, visit https://gerrit.ovirt.org/63685
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I9a14cfa84c34241dbb511c0348109073b6865087
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Piotr Kliczewski <piotr.kliczewski(a)gmail.com&gt;

gerrit-hooks has posted comments on this change.

Change subject: acceptor: stop to double close acceptor
......................................................................


Patch Set 1:

* Update tracker: IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and
is a valid url.
* Check merged to previous::IGNORE,         Not in stable branch (['ovirt-3.6',
'ovirt-4.0'])

-- 
To view, visit https://gerrit.ovirt.org/63685
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I9a14cfa84c34241dbb511c0348109073b6865087
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Piotr Kliczewski <piotr.kliczewski(a)gmail.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: No

Irit Goihman has posted comments on this change.

Change subject: acceptor: stop to double close acceptor
......................................................................


Patch Set 1: Code-Review+1

-- 
To view, visit https://gerrit.ovirt.org/63685
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I9a14cfa84c34241dbb511c0348109073b6865087
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Piotr Kliczewski <piotr.kliczewski(a)gmail.com&gt;
Gerrit-Reviewer: Irit Goihman <igoihman(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: No

[Piotr Kliczewski]

Piotr Kliczewski has posted comments on this change.

Change subject: acceptor: stop to double close acceptor
......................................................................


Patch Set 1: Verified+1

Verified by running tests several times.

-- 
To view, visit https://gerrit.ovirt.org/63685
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I9a14cfa84c34241dbb511c0348109073b6865087
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Piotr Kliczewski <piotr.kliczewski(a)gmail.com&gt;
Gerrit-Reviewer: Irit Goihman <igoihman(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczewski(a)gmail.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: No

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: acceptor: stop to double close acceptor
......................................................................


Patch Set 1: Code-Review-1

(1 comment)

https://gerrit.ovirt.org/#/c/63685/1/lib/vdsm/protocoldetector.py
File lib/vdsm/protocoldetector.py:

Line 206
Line 207
Line 208
Line 209
Line 210
I think the fix is to not close the reactor - this class does not own the reactor, and
should not close it.

We should close only the acceptor. The caller own the reactor and should close it,
probably in prepareForShutdown.


-- 
To view, visit https://gerrit.ovirt.org/63685
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I9a14cfa84c34241dbb511c0348109073b6865087
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Piotr Kliczewski <piotr.kliczewski(a)gmail.com&gt;
Gerrit-Reviewer: Irit Goihman <igoihman(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczewski(a)gmail.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: Yes

[Piotr Kliczewski]

Piotr Kliczewski has posted comments on this change.

Change subject: acceptor: stop to double close acceptor
......................................................................


Patch Set 1:

(1 comment)

https://gerrit.ovirt.org/#/c/63685/1/lib/vdsm/protocoldetector.py
File lib/vdsm/protocoldetector.py:

Line 206
Line 207
Line 208
Line 209
Line 210
>  I think the fix is to not close the reactor - this class does not own
> the r 
With the current code base reactor is owned by this class. I am not sure
whether clientIf should be responsible to manege reactor life cycle. There are already too
many responsibilities for cif. I do not see any other class which should own it.

Btw changing the ownership of reactor should be not related to the fix for double close
issue.


-- 
To view, visit https://gerrit.ovirt.org/63685
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I9a14cfa84c34241dbb511c0348109073b6865087
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Piotr Kliczewski <piotr.kliczewski(a)gmail.com&gt;
Gerrit-Reviewer: Irit Goihman <igoihman(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczewski(a)gmail.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: Yes

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: acceptor: stop to double close acceptor
......................................................................


Patch Set 1:

(1 comment)

https://gerrit.ovirt.org/#/c/63685/1/lib/vdsm/protocoldetector.py
File lib/vdsm/protocoldetector.py:

Line 206
Line 207
Line 208
Line 209
Line 210
>  With the current code base reactor is owned by this class. I am not
> sure wh 
If this class was the owner of the reactor, it was creating the reactor and
was the only user of the reactor.

The owner is clearly clientIF. It create the reactor on startup, start it, but never stop
it. This is the right place to fix this issue.

If you think clientIF should not create the reactor, you can create it in
vdsm.serve_clients, where we create the scheduler, but this change is not required to fix
reactor closing.


-- 
To view, visit https://gerrit.ovirt.org/63685
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I9a14cfa84c34241dbb511c0348109073b6865087
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Piotr Kliczewski <piotr.kliczewski(a)gmail.com&gt;
Gerrit-Reviewer: Irit Goihman <igoihman(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczewski(a)gmail.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: Yes

[Piotr Kliczewski]

Piotr Kliczewski has posted comments on this change.

Change subject: acceptor: stop to double close acceptor
......................................................................


Patch Set 1:

(1 comment)

https://gerrit.ovirt.org/#/c/63685/1/lib/vdsm/protocoldetector.py
File lib/vdsm/protocoldetector.py:

Line 206
Line 207
Line 208
Line 209
Line 210
>  If this class was the owner of the reactor, it was creating the
> reactor and 
My suggestion is to fix ownership in different patch. I hate adding
responsibility to ClientIF because there is already too much done there. Reactor is used
by clients like migration as well so we need to be careful with the change.

In principal I agree with you that we should close were we create. I propose to fix the
ownership in different patch.


-- 
To view, visit https://gerrit.ovirt.org/63685
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I9a14cfa84c34241dbb511c0348109073b6865087
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Piotr Kliczewski <piotr.kliczewski(a)gmail.com&gt;
Gerrit-Reviewer: Irit Goihman <igoihman(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczewski(a)gmail.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: Yes

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: acceptor: stop to double close acceptor
......................................................................


Patch Set 1:

(1 comment)

https://gerrit.ovirt.org/#/c/63685/1/lib/vdsm/protocoldetector.py
File lib/vdsm/protocoldetector.py:

Line 206
Line 207
Line 208
Line 209
Line 210
>  My suggestion is to fix ownership in different patch. I hate adding
> respons 
In this case we just need a different patch, removing one line here, and
adding one line in clientIF.prepareForShutdown. This is a very simple patch perfect for
backporting to 4.0 and even 3.6 if needed, so I don't see a need for another patch on
top of this.


-- 
To view, visit https://gerrit.ovirt.org/63685
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I9a14cfa84c34241dbb511c0348109073b6865087
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Piotr Kliczewski <piotr.kliczewski(a)gmail.com&gt;
Gerrit-Reviewer: Irit Goihman <igoihman(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczewski(a)gmail.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: Yes

gerrit-hooks has posted comments on this change.

Change subject: acceptor: stop to double close acceptor
......................................................................


Patch Set 2:

* Update tracker: IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and
is a valid url.
* Check merged to previous::IGNORE,         Not in stable branch (['ovirt-3.6',
'ovirt-4.0'])

-- 
To view, visit https://gerrit.ovirt.org/63685
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I9a14cfa84c34241dbb511c0348109073b6865087
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Piotr Kliczewski <piotr.kliczewski(a)gmail.com&gt;
Gerrit-Reviewer: Irit Goihman <igoihman(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczewski(a)gmail.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: No

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: acceptor: stop to double close acceptor
......................................................................


Patch Set 2: Code-Review-1

(3 comments)

https://gerrit.ovirt.org/#/c/63685/2/lib/vdsm/protocoldetector.py
File lib/vdsm/protocoldetector.py:

Line 207
Line 208
Line 209
Line 210
Line 211
Why do you want to remove stop?


https://gerrit.ovirt.org/#/c/63685/2/tests/protocoldetectorTests.py
File tests/protocoldetectorTests.py:

Line 109
Line 110
Line 111
Line 112
Line 113
Why keep the acceptor running?


https://gerrit.ovirt.org/#/c/63685/2/vdsm/clientIF.py
File vdsm/clientIF.py:

Line 285
Line 286
Line 287
Line 288
Line 289
Why keep the acceptor running? I think this patch should only add the line stopping the
reactor.


-- 
To view, visit https://gerrit.ovirt.org/63685
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I9a14cfa84c34241dbb511c0348109073b6865087
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Piotr Kliczewski <piotr.kliczewski(a)gmail.com&gt;
Gerrit-Reviewer: Irit Goihman <igoihman(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczewski(a)gmail.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: Yes

[Piotr Kliczewski]

Piotr Kliczewski has posted comments on this change.

Change subject: acceptor: stop to double close acceptor
......................................................................


Patch Set 2:

(3 comments)

https://gerrit.ovirt.org/#/c/63685/2/lib/vdsm/protocoldetector.py
File lib/vdsm/protocoldetector.py:

Line 207
Line 208
Line 209
Line 210
Line 211
>  Why do you want to remove stop? 
We know we do not want to close
acceptor twice. It is enough to close reactor and acceptor will be closed together with
other dispatchers. It is enough to close reactor so the method is not needed here.


https://gerrit.ovirt.org/#/c/63685/2/tests/protocoldetectorTests.py
File tests/protocoldetectorTests.py:

Line 109
Line 110
Line 111
Line 112
Line 113
>  Why keep the acceptor running? 
Acceptor will be closed when
reactor is closed.


https://gerrit.ovirt.org/#/c/63685/2/vdsm/clientIF.py
File vdsm/clientIF.py:

Line 285
Line 286
Line 287
Line 288
Line 289
>  Why keep the acceptor running? I think this patch should only add the
> line  
Please see my other replies.


-- 
To view, visit https://gerrit.ovirt.org/63685
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I9a14cfa84c34241dbb511c0348109073b6865087
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Piotr Kliczewski <piotr.kliczewski(a)gmail.com&gt;
Gerrit-Reviewer: Irit Goihman <igoihman(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczewski(a)gmail.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: Yes

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: acceptor: stop to double close acceptor
......................................................................


Patch Set 2:

(1 comment)

https://gerrit.ovirt.org/#/c/63685/2/lib/vdsm/protocoldetector.py
File lib/vdsm/protocoldetector.py:

Line 207
Line 208
Line 209
Line 210
Line 211
>  We know we do not want to close acceptor twice. It is enough to close
> react 
I think this is bad reason. The acceptor should not depend on the reactor for
stopping, you should be able to stop it independently of the reactor.

Why not the minimal change that is needed to fix the double close?

We need 3 lines change:
- remove line 210 closing the acceptor
- add this line in clientIF.prepareForShutdown
- close the reactor in protocoldetectorTests.py

I don't see any need to change the apis, and I would like to have start() and stop()
interface for everything, so we can write generic code managing multiple components
instead of specific code for each component.


-- 
To view, visit https://gerrit.ovirt.org/63685
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I9a14cfa84c34241dbb511c0348109073b6865087
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Piotr Kliczewski <piotr.kliczewski(a)gmail.com&gt;
Gerrit-Reviewer: Irit Goihman <igoihman(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczewski(a)gmail.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: Yes

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: acceptor: stop to double close acceptor
......................................................................


Patch Set 2:

(1 comment)

https://gerrit.ovirt.org/#/c/63685/2/lib/vdsm/protocoldetector.py
File lib/vdsm/protocoldetector.py:

Line 207
Line 208
Line 209
Line 210
Line 211
>  I think this is bad reason. The acceptor should not depend on the
> reactor f 
Or - even more minimal fix - how about switching the order?

    def stop(self):
        self._acceptor.close()
        self._ractor.stop()

We can move reactor.stop() from here later.


-- 
To view, visit https://gerrit.ovirt.org/63685
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I9a14cfa84c34241dbb511c0348109073b6865087
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Piotr Kliczewski <piotr.kliczewski(a)gmail.com&gt;
Gerrit-Reviewer: Irit Goihman <igoihman(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczewski(a)gmail.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: Yes