Hi Guys,
Recently, I has been working on integrate MOM into VDSM. MOM needs to use VDSM API to interact with it. But currently, it requires the instance of clientIF to use vdsm API. Passing clientIF to MOM is not a good choice since it's a vdsm internal object. So I try to remove the parameter 'cif' from the interface definition and change to access the globally unique clientIF instance in API.py.
To get the instance of clientIF, I add a decorator to clientIF to change it into singleton. Actually, clientIF has been working as a global single instance already. We just don't have an interface to get it and so passing it as parameter instead. I think using singleton to get the instance of clientIF is more clean.
Dan and Saggi already gave some comments in http://gerrit.ovirt.org/#change,4839 Thanks for the reviewing! But I think we need more discussion on it, so I post it here because gerrit is not the appropriate to discuss a design issue.
Thanks ! Mark.
On Wed, May 30, 2012 at 10:49:29PM +0800, Mark Wu wrote:
Hi Guys,
Recently, I has been working on integrate MOM into VDSM. MOM needs to use VDSM API to interact with it. But currently, it requires the instance of clientIF to use vdsm API. Passing clientIF to MOM is not a good choice since it's a vdsm internal object. So I try to remove the parameter 'cif' from the interface definition and change to access the globally unique clientIF instance in API.py.
Please remind me - why don't we continue to pass the clientIF instance, even if it means mentioning it each and every time an API.py object is created? It may be annoying (and thus serve as a reminder that we should probably retire much of clientIF...), but it should work.
To get the instance of clientIF, I add a decorator to clientIF to change it into singleton. Actually, clientIF has been working as a global single instance already. We just don't have an interface to get it and so passing it as parameter instead. I think using singleton to get the instance of clientIF is more clean.
Dan and Saggi already gave some comments in http://gerrit.ovirt.org/#change,4839 Thanks for the reviewing! But I think we need more discussion on it, so I post it here because gerrit is not the appropriate to discuss a design issue.
Thanks ! Mark.
On 05/30/2012 11:01 PM, Dan Kenigsberg wrote:
On Wed, May 30, 2012 at 10:49:29PM +0800, Mark Wu wrote:
Hi Guys,
Recently, I has been working on integrate MOM into VDSM. MOM needs to use VDSM API to interact with it. But currently, it requires the instance of clientIF to use vdsm API. Passing clientIF to MOM is not a good choice since it's a vdsm internal object. So I try to remove the parameter 'cif' from the interface definition and change to access the globally unique clientIF instance in API.py.
Please remind me - why don't we continue to pass the clientIF instance, even if it means mentioning it each and every time an API.py object is created? It may be annoying (and thus serve as a reminder that we should probably retire much of clientIF...), but it should work.
In the old MOM integration patch, I passed the clientIF instance to MOM by the following method: vdsmInterface.setConnection(self._cif)
Here's your comments on the patch:
"_cif is not the proper API to interact with Vdsm. API.py is. Please change MOM to conform to this, if possible.
I think that mom should receive an API object (even API.Global()!) that it needs for its operation. Even passing BindingXMLRPC() object is more APIish than the internal clientIF object."
So I try to remove cif from API definition to make MOM can call the VDSM API without having clientIF.
To get the instance of clientIF, I add a decorator to clientIF to change it into singleton. Actually, clientIF has been working as a global single instance already. We just don't have an interface to get it and so passing it as parameter instead. I think using singleton to get the instance of clientIF is more clean.
Dan and Saggi already gave some comments in http://gerrit.ovirt.org/#change,4839 Thanks for the reviewing! But I think we need more discussion on it, so I post it here because gerrit is not the appropriate to discuss a design issue.
Thanks ! Mark.
On Thu, May 31, 2012 at 09:03:37PM +0800, Mark Wu wrote:
On 05/30/2012 11:01 PM, Dan Kenigsberg wrote:
On Wed, May 30, 2012 at 10:49:29PM +0800, Mark Wu wrote:
Hi Guys,
Recently, I has been working on integrate MOM into VDSM. MOM needs to use VDSM API to interact with it. But currently, it requires the instance of clientIF to use vdsm API. Passing clientIF to MOM is not a good choice since it's a vdsm internal object. So I try to remove the parameter 'cif' from the interface definition and change to access the globally unique clientIF instance in API.py.
Please remind me - why don't we continue to pass the clientIF instance, even if it means mentioning it each and every time an API.py object is created? It may be annoying (and thus serve as a reminder that we should probably retire much of clientIF...), but it should work.
In the old MOM integration patch, I passed the clientIF instance to MOM by the following method: vdsmInterface.setConnection(self._cif)
Here's your comments on the patch:
"_cif is not the proper API to interact with Vdsm. API.py is. Please change MOM to conform to this, if possible.
I think that mom should receive an API object (even API.Global()!) that it needs for its operation. Even passing BindingXMLRPC() object is more APIish than the internal clientIF object."
Please do not blame me! ;-)
I do not mind passing an API.Global() that happens to hold an internal private reference to _clientIF. I just want that if we find the way to obliterate clientIF, we won't need to send a patch to MOM, too.
So I try to remove cif from API definition to make MOM can call the VDSM API without having clientIF.
I do not understand - MOM could receive an API object, it does not have to construct it by itself.
Regards, Dan.
On Thu, May 31, 2012 at 04:08:52PM +0300, Dan Kenigsberg wrote:
On Thu, May 31, 2012 at 09:03:37PM +0800, Mark Wu wrote:
On 05/30/2012 11:01 PM, Dan Kenigsberg wrote:
On Wed, May 30, 2012 at 10:49:29PM +0800, Mark Wu wrote:
Hi Guys,
Recently, I has been working on integrate MOM into VDSM. MOM needs to use VDSM API to interact with it. But currently, it requires the instance of clientIF to use vdsm API. Passing clientIF to MOM is not a good choice since it's a vdsm internal object. So I try to remove the parameter 'cif' from the interface definition and change to access the globally unique clientIF instance in API.py.
Please remind me - why don't we continue to pass the clientIF instance, even if it means mentioning it each and every time an API.py object is created? It may be annoying (and thus serve as a reminder that we should probably retire much of clientIF...), but it should work.
In the old MOM integration patch, I passed the clientIF instance to MOM by the following method: vdsmInterface.setConnection(self._cif)
Here's your comments on the patch:
"_cif is not the proper API to interact with Vdsm. API.py is. Please change MOM to conform to this, if possible.
I think that mom should receive an API object (even API.Global()!) that it needs for its operation. Even passing BindingXMLRPC() object is more APIish than the internal clientIF object."
Please do not blame me! ;-)
I do not mind passing an API.Global() that happens to hold an internal private reference to _clientIF. I just want that if we find the way to obliterate clientIF, we won't need to send a patch to MOM, too.
So I try to remove cif from API definition to make MOM can call the VDSM API without having clientIF.
I do not understand - MOM could receive an API object, it does not have to construct it by itself.
Today, the API consists of several, unlinked objects so passing a single API.Global() would not be enough. We either need to allow MOM to construct its own API objects or produce them by calling methods in API.Global(). Personally, I think the code would be cleaner if clientIF is a singleton (Mark's latest patch) as opposed to adding factory methods to API.Global().
On 05/31/2012 09:08 PM, Dan Kenigsberg wrote:
On Thu, May 31, 2012 at 09:03:37PM +0800, Mark Wu wrote:
On 05/30/2012 11:01 PM, Dan Kenigsberg wrote:
On Wed, May 30, 2012 at 10:49:29PM +0800, Mark Wu wrote:
Hi Guys,
Recently, I has been working on integrate MOM into VDSM. MOM needs to use VDSM API to interact with it. But currently, it requires the instance of clientIF to use vdsm API. Passing clientIF to MOM is not a good choice since it's a vdsm internal object. So I try to remove the parameter 'cif' from the interface definition and change to access the globally unique clientIF instance in API.py.
Please remind me - why don't we continue to pass the clientIF instance, even if it means mentioning it each and every time an API.py object is created? It may be annoying (and thus serve as a reminder that we should probably retire much of clientIF...), but it should work.
In the old MOM integration patch, I passed the clientIF instance to MOM by the following method: vdsmInterface.setConnection(self._cif)
Here's your comments on the patch:
"_cif is not the proper API to interact with Vdsm. API.py is. Please change MOM to conform to this, if possible.
I think that mom should receive an API object (even API.Global()!) that it needs for its operation. Even passing BindingXMLRPC() object is more APIish than the internal clientIF object."
Please do not blame me! ;-)
No body dares to blame the VDSM maintainer! :P
I do not mind passing an API.Global() that happens to hold an internal private reference to _clientIF. I just want that if we find the way to obliterate clientIF, we won't need to send a patch to MOM, too.
It seems I misunderstood your previous comments. I am sorry for that.
So I try to remove cif from API definition to make MOM can call the VDSM API without having clientIF.
I do not understand - MOM could receive an API object, it does not have to construct it by itself.
yes, we can pass an API.Global() instance to MOM, but MOM also needs to create API.VM instance. Without clientIF, I could pass a wrapped VM class to MOM. But it's ugly. So I try to remove clientIF in API exposed interface, and make the caller can use vdsm API just after importing the module. It could be cleaner. What's your opinion?
Thanks a lot!! Mark
Regards, Dan.
On Fri, Jun 01, 2012 at 01:59:25PM +0800, Mark Wu wrote:
On 05/31/2012 09:08 PM, Dan Kenigsberg wrote:
On Thu, May 31, 2012 at 09:03:37PM +0800, Mark Wu wrote:
On 05/30/2012 11:01 PM, Dan Kenigsberg wrote:
On Wed, May 30, 2012 at 10:49:29PM +0800, Mark Wu wrote:
Hi Guys,
Recently, I has been working on integrate MOM into VDSM. MOM needs to use VDSM API to interact with it. But currently, it requires the instance of clientIF to use vdsm API. Passing clientIF to MOM is not a good choice since it's a vdsm internal object. So I try to remove the parameter 'cif' from the interface definition and change to access the globally unique clientIF instance in API.py.
Please remind me - why don't we continue to pass the clientIF instance, even if it means mentioning it each and every time an API.py object is created? It may be annoying (and thus serve as a reminder that we should probably retire much of clientIF...), but it should work.
In the old MOM integration patch, I passed the clientIF instance to MOM by the following method: vdsmInterface.setConnection(self._cif)
Here's your comments on the patch:
"_cif is not the proper API to interact with Vdsm. API.py is. Please change MOM to conform to this, if possible.
I think that mom should receive an API object (even API.Global()!) that it needs for its operation. Even passing BindingXMLRPC() object is more APIish than the internal clientIF object."
Please do not blame me! ;-)
No body dares to blame the VDSM maintainer! :P
I do not mind passing an API.Global() that happens to hold an internal private reference to _clientIF. I just want that if we find the way to obliterate clientIF, we won't need to send a patch to MOM, too.
It seems I misunderstood your previous comments. I am sorry for that.
I'm even sorrier for not being clear enough!
So I try to remove cif from API definition to make MOM can call the VDSM API without having clientIF.
I do not understand - MOM could receive an API object, it does not have to construct it by itself.
yes, we can pass an API.Global() instance to MOM, but MOM also needs to create API.VM instance. Without clientIF, I could pass a wrapped VM class to MOM. But it's ugly.
I admit it is not pretty. How about adding factory functions such as Global.constructVm() ? I think it would appease the no-singleton gods.
So I try to remove clientIF in API exposed interface, and make the caller can use vdsm API just after importing the module. It could be cleaner. What's your opinion?
Thanks a lot!!
Thank you. Either way I kinda fear the first out-of-prject dependecy on API.py. The latter API is not yet stable nor complete. Brace yourself to guard against surprises, and to move along as the API changes. I cannot stress enough the need for a unit test for every functionality required by mom.
Regards, Dan.
If you don't want to add it in a parameter then you already suspect that you are doing something wrong. Using a singleton instead of passing a parameter doesn't make the dependency not there. It's just obscures it.
I might not fully understand what you want to do but I think what you want is to have MOM expect a certain interface. Then have an adapter class bridging the two interfaces. The pass the wrapped CIF to MOM.
----- Original Message -----
From: "Mark Wu" wudxw@linux.vnet.ibm.com To: vdsm-devel@lists.fedorahosted.org Cc: "Dan Kenigsberg" danken@redhat.com, "Saggi Mizrahi" smizrahi@redhat.com, "Adam Litke" agl@us.ibm.com, "Ryan Harper" ryanh@us.ibm.com Sent: Wednesday, May 30, 2012 10:49:29 AM Subject: VDSM API/clientIF instance design issue
Hi Guys,
Recently, I has been working on integrate MOM into VDSM. MOM needs to use VDSM API to interact with it. But currently, it requires the instance of clientIF to use vdsm API. Passing clientIF to MOM is not a good choice since it's a vdsm internal object. So I try to remove the parameter 'cif' from the interface definition and change to access the globally unique clientIF instance in API.py.
To get the instance of clientIF, I add a decorator to clientIF to change it into singleton. Actually, clientIF has been working as a global single instance already. We just don't have an interface to get it and so passing it as parameter instead. I think using singleton to get the instance of clientIF is more clean.
Dan and Saggi already gave some comments in http://gerrit.ovirt.org/#change,4839 Thanks for the reviewing! But I think we need more discussion on it, so I post it here because gerrit is not the appropriate to discuss a design issue.
Thanks ! Mark.
On Wed, May 30, 2012 at 11:02:50AM -0400, Saggi Mizrahi wrote:
If you don't want to add it in a parameter then you already suspect that you are doing something wrong. Using a singleton instead of passing a parameter doesn't make the dependency not there. It's just obscures it.
Right. Passing around an internal 'singleton' (whether wrapped or not) is not the correct course.
I might not fully understand what you want to do but I think what you want is to have MOM expect a certain interface. Then have an adapter class bridging the two interfaces. The pass the wrapped CIF to MOM.
It seems that your suggestion obscures the dependency at least as badly as a singleton interface does. Today clientIF is a singleton class in all but the name. It is basically a global container that stores global objects (vmContainer, locks, etc) and global threads (irs, hostStatsThread, etc). The fact that a reference gets passed around doesn't really make it any cleaner. We can all agree that clientIF (in its current form) has overstayed its welcome. Mark and Xu He Jie have some ideas on how to remove it and plan to tackle that work. That being said, MOM is an important feature for oVirt 3.1. Do we really want it to drop out of the release in order to focus on clientIF cleanups (which certainly will not make 3.1)?
Using a singleton simply means that API objects can be instantiated without the caller needing to be aware of clientIF at all. In my opinion, this is a cleanup because we are free to refactor clientIF without affecting the bindings or MOM one bit.
----- Original Message -----
From: "Mark Wu" wudxw@linux.vnet.ibm.com To: vdsm-devel@lists.fedorahosted.org Cc: "Dan Kenigsberg" danken@redhat.com, "Saggi Mizrahi" smizrahi@redhat.com, "Adam Litke" agl@us.ibm.com, "Ryan Harper" ryanh@us.ibm.com Sent: Wednesday, May 30, 2012 10:49:29 AM Subject: VDSM API/clientIF instance design issue
Hi Guys,
Recently, I has been working on integrate MOM into VDSM. MOM needs to use VDSM API to interact with it. But currently, it requires the instance of clientIF to use vdsm API. Passing clientIF to MOM is not a good choice since it's a vdsm internal object. So I try to remove the parameter 'cif' from the interface definition and change to access the globally unique clientIF instance in API.py.
To get the instance of clientIF, I add a decorator to clientIF to change it into singleton. Actually, clientIF has been working as a global single instance already. We just don't have an interface to get it and so passing it as parameter instead. I think using singleton to get the instance of clientIF is more clean.
Dan and Saggi already gave some comments in http://gerrit.ovirt.org/#change,4839 Thanks for the reviewing! But I think we need more discussion on it, so I post it here because gerrit is not the appropriate to discuss a design issue.
Thanks ! Mark.
On Wed, May 30, 2012 at 11:02:50AM -0400, Saggi Mizrahi wrote:
If you don't want to add it in a parameter then you already suspect that you are doing something wrong. Using a singleton instead of passing a parameter doesn't make the dependency not there. It's just obscures it.
Right. Passing around an internal 'singleton' (whether wrapped or not) is not the correct course.
I might not fully understand what you want to do but I think what you want is to have MOM expect a certain interface. Then have an adapter class bridging the two interfaces. The pass the wrapped CIF to MOM.
It seems that your suggestion obscures the dependency at least as badly as a singleton interface does. Today clientIF is a singleton class in all but the name. It is basically a global container that stores global objects (vmContainer, locks, etc) and global threads (irs, hostStatsThread, etc). The fact that a reference gets passed around doesn't really make it any cleaner. We can all agree that clientIF (in its current form) has overstayed its welcome. Mark and Xu He Jie have some ideas on how to remove it and plan to tackle that work. That being said, MOM is an important feature for oVirt 3.1. Do we really want it to drop out of the release in order to focus on clientIF cleanups (which certainly will not make 3.1)?
Using a singleton simply means that API objects can be instantiated without the caller needing to be aware of clientIF at all. In my opinion, this is a cleanup because we are free to refactor clientIF without affecting the bindings or MOM one bit.
----- Original Message -----
From: "Mark Wu" wudxw@linux.vnet.ibm.com To: vdsm-devel@lists.fedorahosted.org Cc: "Dan Kenigsberg" danken@redhat.com, "Saggi Mizrahi" smizrahi@redhat.com, "Adam Litke" agl@us.ibm.com, "Ryan Harper" ryanh@us.ibm.com Sent: Wednesday, May 30, 2012 10:49:29 AM Subject: VDSM API/clientIF instance design issue
Hi Guys,
Recently, I has been working on integrate MOM into VDSM. MOM needs to use VDSM API to interact with it. But currently, it requires the instance of clientIF to use vdsm API. Passing clientIF to MOM is not a good choice since it's a vdsm internal object. So I try to remove the parameter 'cif' from the interface definition and change to access the globally unique clientIF instance in API.py.
To get the instance of clientIF, I add a decorator to clientIF to change it into singleton. Actually, clientIF has been working as a global single instance already. We just don't have an interface to get it and so passing it as parameter instead. I think using singleton to get the instance of clientIF is more clean.
Dan and Saggi already gave some comments in http://gerrit.ovirt.org/#change,4839 Thanks for the reviewing! But I think we need more discussion on it, so I post it here because gerrit is not the appropriate to discuss a design issue.
Thanks ! Mark.
Do I get it right that host-side of MOM communicates with guest side over network interface? If so, why isn't it following best practices of qemu/kvm world and why it doesn't communicate over its own virtio-serial port?
David
Mark Wu píše v St 30. 05. 2012 v 22:49 +0800:
Hi Guys,
Recently, I has been working on integrate MOM into VDSM. MOM needs to use VDSM API to interact with it. But currently, it requires the instance of clientIF to use vdsm API. Passing clientIF to MOM is not a good choice since it's a vdsm internal object. So I try to remove the parameter 'cif' from the interface definition and change to access the globally unique clientIF instance in API.py.
To get the instance of clientIF, I add a decorator to clientIF to change it into singleton. Actually, clientIF has been working as a global single instance already. We just don't have an interface to get it and so passing it as parameter instead. I think using singleton to get the instance of clientIF is more clean.
Dan and Saggi already gave some comments in http://gerrit.ovirt.org/#change,4839 Thanks for the reviewing! But I think we need more discussion on it, so I post it here because gerrit is not the appropriate to discuss a design issue.
Thanks ! Mark.
vdsm-devel mailing list vdsm-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/vdsm-devel
On Wed, May 30, 2012 at 05:26:47PM +0200, David Jaša wrote:
Do I get it right that host-side of MOM communicates with guest side over network interface? If so, why isn't it following best practices of qemu/kvm world and why it doesn't communicate over its own virtio-serial port?
No that is not correct for oVirt's usage of MOM. MOM has several ways of acquiring guest statistics: Client/server over TCP, libvirt (via. virDomainMemoryStats), qemu-ga (client/server over virtio-serial), etc. For vdsm, memory stats are acquired via the vdsm api itself (API.py:VM.getStats()). VDSM acquires these stats via the ovirt-guest-agent over virto-serial.
David
Mark Wu píše v St 30. 05. 2012 v 22:49 +0800:
Hi Guys,
Recently, I has been working on integrate MOM into VDSM. MOM needs to use VDSM API to interact with it. But currently, it requires the instance of clientIF to use vdsm API. Passing clientIF to MOM is not a good choice since it's a vdsm internal object. So I try to remove the parameter 'cif' from the interface definition and change to access the globally unique clientIF instance in API.py.
To get the instance of clientIF, I add a decorator to clientIF to change it into singleton. Actually, clientIF has been working as a global single instance already. We just don't have an interface to get it and so passing it as parameter instead. I think using singleton to get the instance of clientIF is more clean.
Dan and Saggi already gave some comments in http://gerrit.ovirt.org/#change,4839 Thanks for the reviewing! But I think we need more discussion on it, so I post it here because gerrit is not the appropriate to discuss a design issue.
Thanks ! Mark.
vdsm-devel mailing list vdsm-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/vdsm-devel
--
David Jaša, RHCE
SPICE QE based in Brno GPG Key: 22C33E24 Fingerprint: 513A 060B D1B4 2A72 7F0D 0278 B125 CD00 22C3 3E24
vdsm-devel mailing list vdsm-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/vdsm-devel
vdsm-devel@lists.fedorahosted.org