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

[nsoffer]

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 
Gerrit-Reviewer: Irit Goihman 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Piotr Kliczewski 
Gerrit-Reviewer: Yaniv Bronhaim 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

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

[nsoffer]

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 
Gerrit-Reviewer: Irit Goihman 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Piotr Kliczewski 
Gerrit-Reviewer: Yaniv Bronhaim 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

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

[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 
Gerrit-Reviewer: Irit Goihman 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Piotr Kliczewski 
Gerrit-Reviewer: Yaniv Bronhaim 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

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

[nsoffer]

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 
Gerrit-Reviewer: Irit Goihman 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Piotr Kliczewski 
Gerrit-Reviewer: Yaniv Bronhaim 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

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

[automation]

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 
Gerrit-Reviewer: Irit Goihman 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Piotr Kliczewski 
Gerrit-Reviewer: Yaniv Bronhaim 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

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

[nsoffer]

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 
Gerrit-Reviewer: Irit Goihman 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Piotr Kliczewski 
Gerrit-Reviewer: Yaniv Bronhaim 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

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

[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 
Gerrit-Reviewer: Irit Goihman 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Piotr Kliczewski 
Gerrit-Reviewer: Yaniv Bronhaim 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

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

[nsoffer]

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 
Gerrit-Reviewer: Irit Goihman 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Piotr Kliczewski 
Gerrit-Reviewer: Yaniv Bronhaim 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

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

[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 
Gerrit-Reviewer: Irit Goihman 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Piotr Kliczewski 
Gerrit-Reviewer: Yaniv Bronhaim 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

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

[nsoffer]

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 
Gerrit-Reviewer: Irit Goihman 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Piotr Kliczewski 
Gerrit-Reviewer: Yaniv Bronhaim 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

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

[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 
Gerrit-Reviewer: Irit Goihman 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Piotr Kliczewski 
Gerrit-Reviewer: Yaniv Bronhaim 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

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

[igoihman]

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 
Gerrit-Reviewer: Irit Goihman 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Yaniv Bronhaim 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

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

[automation]

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 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

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 
---
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 
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org