Change in vdsm[master]: A new method to read from VMs' channels.

Gal Hammer has uploaded a new change for review.

Change subject: A new method to read from VMs' channels.
......................................................................

A new method to read from VMs' channels.

Replaced having a thread-per-VM that monitor and read from the
VM's virtual channel with a 1-thread listener that handle all
VMs' channels.

Change-Id: I687a22a3fba6c1d2b42e837c5f2024aee0a98bdc
---
M vdsm.spec.in
M vdsm/Makefile.am
M vdsm/clientIF.py
M vdsm/guestIF.py
M vdsm/libvirtvm.py
A vdsm/vmChannels.py
6 files changed, 164 insertions(+), 69 deletions(-)

git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/69/869/1
--
To view, visit http://gerrit.ovirt.org/869
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I687a22a3fba6c1d2b42e837c5f2024aee0a98bdc
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Gal Hammer

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: A new method to read from VMs' channels.
......................................................................

Patch Set 1: I would prefer that you didn't submit this

(13 inline comments)

I need to read it more closer later

....................................................
Commit Message
Line 11: VMs' channels.
describe our motivation - the huge memory overhead of each thread.

....................................................
File vdsm/clientIF.py
Line 189:             self.channelListener.join()
why wait here for the listener?
stop it here and (only if needed) wait when the process tries to exit

....................................................
File vdsm/guestIF.py
Line 50:     def __init__(self, socketName, channelListener, log, user='Unknown', ips='', connect=True):
longline

Line 77:             # Timeouts are handled in the channels' listener.
so why do you bother touching it here?

Line 85:             logging.debug("Connection attempt failed: %s", e)
exc_info=True gives more interesting info than 'e'

Line 228:     def _onChannelRead(self):
I think _onChannel may be called while it is already running, causing havoc to self._buffer.

we may need to use StringIO

....................................................
File vdsm/libvirtvm.py
Line 1019:         self.guestAgent = guestIF.GuestAgent(self._guestSocektFile, self.cif.channelListener,
please keep lines shorter than 80 chars. I have a small laptop.

....................................................
File vdsm/Makefile.am
Line 38:     
red white space

....................................................
File vdsm/vmChannels.py
Line 21: import threading, time, select
for some reason, current style mandates separate import statement per module.

Line 31:     
gerrit does not like your trailing spaces.

Line 57:         now = int(time.time())
why not work with floats?

Line 103:         self.log.info("Setting channels' timeout to %d seconds.", seconds)
again, why limit yourself to integral seconds?

Line 110:             read_callback, timeout_callback, param)
passing args and kwargs is more pythonic and general than "param".

--
To view, visit http://gerrit.ovirt.org/869
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I687a22a3fba6c1d2b42e837c5f2024aee0a98bdc
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Gal Hammer 
Gerrit-Reviewer: Dan Kenigsberg

Gal Hammer has posted comments on this change.

Change subject: A new method to read from VMs' channels.
......................................................................

Patch Set 1: (8 inline comments)

....................................................
Commit Message
Line 11: VMs' channels.
Done

....................................................
File vdsm/clientIF.py
Line 189:             self.channelListener.join()
Done

....................................................
File vdsm/guestIF.py
Line 50:     def __init__(self, socketName, channelListener, log, user='Unknown', ips='', connect=True):
Done

Line 77:             # Timeouts are handled in the channels' listener.
Because I touched it when trying to connect.

Line 85:             logging.debug("Connection attempt failed: %s", e)
I don't agree. I don't care about the call stack here, just the reason for the connection failure.

Line 228:     def _onChannelRead(self):
I don't understand. This method is called only by the VmChannel.Listener's thread.

....................................................
File vdsm/libvirtvm.py
Line 1019:         self.guestAgent = guestIF.GuestAgent(self._guestSocektFile, self.cif.channelListener,
You can't blame me for anything bad happens to you! :-P

Done.

....................................................
File vdsm/Makefile.am
Line 38:     
Done

--
To view, visit http://gerrit.ovirt.org/869
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I687a22a3fba6c1d2b42e837c5f2024aee0a98bdc
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Gal Hammer 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Gal Hammer

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: A new method to read from VMs' channels.
......................................................................

Patch Set 2: I would prefer that you didn't submit this

(3 inline comments)

....................................................
File vdsm/clientIF.py
Line 119:         self.channelListener = Listener(self.log)
is this expected to be used outside clientIF? if not, keep _private.

on a second thought, believe that it would be even nicer to hide this singleton litenter inside guestIF, and create it automatically on first creation of a guestIF().

....................................................
File vdsm/vmChannels.py
Line 59:         now = int(time.time())
repeat - why on earth would we want to introduce this (up to) 1 sec rounding error?

Line 111:             read_callback, timeout_callback, param)
ignoring comments does not make them go away...

--
To view, visit http://gerrit.ovirt.org/869
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I687a22a3fba6c1d2b42e837c5f2024aee0a98bdc
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Gal Hammer 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Gal Hammer

Gal Hammer has posted comments on this change.

Change subject: A new method to read from VMs' channels.
......................................................................

Patch Set 2: (3 inline comments)

....................................................
File vdsm/clientIF.py
Line 119:         self.channelListener = Listener(self.log)
Yes. It it accessible by all guestIF instances.

This is not a singleton.

....................................................
File vdsm/vmChannels.py
Line 59:         now = int(time.time())
Because I don't care about milliseconds resolution?

Line 111:             read_callback, timeout_callback, param)
Sure it does. I don't see it now.

Is changing the "param" to "opaque" is good enough?

--
To view, visit http://gerrit.ovirt.org/869
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I687a22a3fba6c1d2b42e837c5f2024aee0a98bdc
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Gal Hammer 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Gal Hammer

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: A new method to read from VMs' channels.
......................................................................

Patch Set 2: (1 inline comment)

....................................................
File vdsm/vmChannels.py
Line 111:             read_callback, timeout_callback, param)
I think it is ugly and unpythonic, but it does adhere to the http://en.wikipedia.org/wiki/Principle_of_least_action

--
To view, visit http://gerrit.ovirt.org/869
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I687a22a3fba6c1d2b42e837c5f2024aee0a98bdc
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Gal Hammer 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Gal Hammer

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: A new method to read from VMs' channels.
......................................................................

Patch Set 3: Looks good to me, but someone else must approve

though you could avoid lines longer than 80 chars.

--
To view, visit http://gerrit.ovirt.org/869
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I687a22a3fba6c1d2b42e837c5f2024aee0a98bdc
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Gal Hammer 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Gal Hammer

Saggi Mizrahi has posted comments on this change.

Change subject: A new method to read from VMs' channels.
......................................................................

Patch Set 3: Looks good to me, but someone else must approve

(3 inline comments)

....................................................
File vdsm/guestIF.py
Line 68:         utils.execCmd([constants.EXT_PREPARE_VMCHANNEL, self._socketName])
error checking?

Line 69:         # Set a timeout for connect retires.
Kinda stating the obvious

Line 76:             logging.debug("Attempting connection to %s", self._socketName)
self.log

--
To view, visit http://gerrit.ovirt.org/869
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I687a22a3fba6c1d2b42e837c5f2024aee0a98bdc
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Gal Hammer 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Gal Hammer 
Gerrit-Reviewer: Saggi Mizrahi

[Haim Ateya]

Haim Ateya has posted comments on this change.

Change subject: A new method to read from VMs' channels.
......................................................................

Patch Set 4: Fails

please fix migration.

--
To view, visit http://gerrit.ovirt.org/869
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I687a22a3fba6c1d2b42e837c5f2024aee0a98bdc
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Gal Hammer 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Gal Hammer 
Gerrit-Reviewer: Haim Ateya 
Gerrit-Reviewer: Saggi Mizrahi

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: A new method to read from VMs' channels.
......................................................................

Patch Set 5: Looks good to me, but someone else must approve

--
To view, visit http://gerrit.ovirt.org/869
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I687a22a3fba6c1d2b42e837c5f2024aee0a98bdc
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Gal Hammer 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Gal Hammer 
Gerrit-Reviewer: Haim Ateya 
Gerrit-Reviewer: Saggi Mizrahi

[Haim Ateya]

Haim Ateya has posted comments on this change.

Change subject: A new method to read from VMs' channels.
......................................................................

Patch Set 5: Fails

NACK - I still have a problem, when we migrate vm between 2 hosts, unregister network socket is called during migration, and then again, during destroy, which called by engine, the second call cause an exception in the logs. 
please check if socket exists before trying to unregister it.

--
To view, visit http://gerrit.ovirt.org/869
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I687a22a3fba6c1d2b42e837c5f2024aee0a98bdc
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Gal Hammer 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Gal Hammer 
Gerrit-Reviewer: Haim Ateya 
Gerrit-Reviewer: Saggi Mizrahi

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: A new method to read from VMs' channels.
......................................................................

Patch Set 7:

Haim, Gal, I do not quite follow, but please note that test-and-later-unregister  is raceful.

--
To view, visit http://gerrit.ovirt.org/869
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I687a22a3fba6c1d2b42e837c5f2024aee0a98bdc
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Gal Hammer 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Gal Hammer 
Gerrit-Reviewer: Haim Ateya 
Gerrit-Reviewer: Saggi Mizrahi

[Haim Ateya]

Haim Ateya has posted comments on this change.

Change subject: A new method to read from VMs' channels.
......................................................................

Patch Set 7: Fails

well - we seem to have a race where we fail to connect to socket after migration, and guest information is not reported, adding logs on one host seem to 'resolve' the issue.
also, why we call for unregister socket 3 times on migration scenario ?

--
To view, visit http://gerrit.ovirt.org/869
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I687a22a3fba6c1d2b42e837c5f2024aee0a98bdc
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Gal Hammer 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Gal Hammer 
Gerrit-Reviewer: Haim Ateya 
Gerrit-Reviewer: Saggi Mizrahi

[Haim Ateya]

Haim Ateya has posted comments on this change.

Change subject: A new method to read from VMs' channels.
......................................................................

Patch Set 10: Verified

--
To view, visit http://gerrit.ovirt.org/869
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I687a22a3fba6c1d2b42e837c5f2024aee0a98bdc
Gerrit-PatchSet: 10
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Gal Hammer 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Gal Hammer 
Gerrit-Reviewer: Haim Ateya 
Gerrit-Reviewer: Saggi Mizrahi

[Dan Kenigsberg]

Dan Kenigsberg has submitted this change and it was merged.

Change subject: A new method to read from VMs' channels.
......................................................................

A new method to read from VMs' channels.

Replaced having a thread-per-VM that monitors and reads from the
VM's virtual channel with a 1-thread listener that handles all
VMs' channels.

This patch should reduce vdsm's memory usage consumed by memory
overhread of each thread.

Change-Id: I687a22a3fba6c1d2b42e837c5f2024aee0a98bdc
---
M vdsm.spec.in
M vdsm/Makefile.am
M vdsm/clientIF.py
M vdsm/guestIF.py
M vdsm/libvirtvm.py
A vdsm/vmChannels.py
6 files changed, 221 insertions(+), 69 deletions(-)

Approvals:
  Haim Ateya: Verified
  Dan Kenigsberg: Looks good to me, approved

--
To view, visit http://gerrit.ovirt.org/869
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: I687a22a3fba6c1d2b42e837c5f2024aee0a98bdc
Gerrit-PatchSet: 10
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Gal Hammer 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Gal Hammer 
Gerrit-Reviewer: Haim Ateya 
Gerrit-Reviewer: Saggi Mizrahi

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: A new method to read from VMs' channels.
......................................................................

Patch Set 10: Looks good to me, approved

(5 inline comments)

Gal, I'm taking this in due to my own time constraints. Please fix my comments in a later patch.

....................................................
File vdsm/vmChannels.py
Line 65:                     self.log.exception("Exception on timeout callback.")
exc_info=True

Line 93:         try:
with self._update_lock:
  bla

is so nicer!

Line 108:                 if obj['connect_cb'](obj['opaque']) == True:
I find comparing to True redundant.

Line 115:                 self.log.exception("Exception on connect callback.")
logging the exception here is highly recommended. exc_info=True

Line 148:         try:
with

--
To view, visit http://gerrit.ovirt.org/869
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I687a22a3fba6c1d2b42e837c5f2024aee0a98bdc
Gerrit-PatchSet: 10
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Gal Hammer 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Gal Hammer 
Gerrit-Reviewer: Haim Ateya 
Gerrit-Reviewer: Saggi Mizrahi

Gal Hammer has posted comments on this change.

Change subject: A new method to read from VMs' channels.
......................................................................

Patch Set 10: (5 inline comments)

....................................................
File vdsm/vmChannels.py
Line 65:                     self.log.exception("Exception on timeout callback.")
No need. The log.exception include the back trace.

Line 93:         try:
Done

Line 108:                 if obj['connect_cb'](obj['opaque']) == True:
Done

Line 115:                 self.log.exception("Exception on connect callback.")
The log.exception include the back trace. Same as exc_info=True, but much nicer (to my opinion).

Line 148:         try:
Done

--
To view, visit http://gerrit.ovirt.org/869
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I687a22a3fba6c1d2b42e837c5f2024aee0a98bdc
Gerrit-PatchSet: 10
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Gal Hammer 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Gal Hammer 
Gerrit-Reviewer: Haim Ateya 
Gerrit-Reviewer: Saggi Mizrahi