From: "Adam Litke" <alitke(a)redhat.com&gt; To: devel(a)ovirt.org Cc: "Nir Soffer" <nsoffer(a)redhat.com&gt;, "Francesco Romani" <fromani(a)redhat.com&gt;, "Federico Simoncelli" <fsimonce(a)redhat.com&gt;, "Dan Kenigsberg" <danken(a)redhat.com&gt; Sent: Thursday, November 20, 2014 9:15:33 PM Subject: contEIOVMs regression? Hi list, I am taking a look at Bug 1157421 [1] which describes a situation where VM's that are paused with an -EIO error are not automatically resumed after the problem with storage has been corrected. I have some patches [2] on gerrit that resolve the problem. Since this appears to be a regression I am looking at a non-intrusive way to fix it in the 3.5 branch. There is some disagreement on the proper way to fix this so I am hoping we can arrive at a solution through an open discussion. The main issue at hand is with the Event/Callback mechanism we use to call clientIF.contEIOVMs. According to my experiments and this online discussion [3] weakref does not work for instance methods such as clientIF.contEIOVMs. Our Event class uses weakref to prevent it from holding references to registered callback functions.

I see two easy ways to fix the regression:

1) Treat clientIF as a singleton class (which it is) and make contEIOVMs a module-level method which gets the clientIF instance and calls it's bound contEIOVMs method. See my patches [2] for the code behind this idea.

2) Allow Event to maintain a strong reference on the bound clientIF.contEIOVMs method. This will allow the current code to work as designed but will change the Event implementation to accomodate this specific use case. Since no one else appears to be using this code, it should have no functional impact.

Are there any other ideas I'm missing? I am aware of plans to refactor this code for 3.6 but I am more interested in a short-term, practical solution to address the current regression.

From: "Adam Litke" <alitke(a)redhat.com&gt; To: "Nir Soffer" <nsoffer(a)redhat.com&gt; Cc: devel(a)ovirt.org, "Francesco Romani" <fromani(a)redhat.com&gt;, "Federico Simoncelli" <fsimonce(a)redhat.com&gt;, "Dan Kenigsberg" <danken(a)redhat.com&gt; Sent: Friday, November 21, 2014 4:46:13 PM Subject: Re: contEIOVMs regression? On 20/11/14 17:37 -0500, Nir Soffer wrote: >----- Original Message ----- >> From: "Adam Litke" <alitke(a)redhat.com&gt; >> To: devel(a)ovirt.org >> Cc: "Nir Soffer" <nsoffer(a)redhat.com&gt;, "Francesco Romani" >> <fromani(a)redhat.com&gt;, "Federico Simoncelli" >> <fsimonce(a)redhat.com&gt;, "Dan Kenigsberg" <danken(a)redhat.com&gt; >> Sent: Thursday, November 20, 2014 9:15:33 PM >> Subject: contEIOVMs regression? >> >> Hi list, >> >> I am taking a look at Bug 1157421 [1] which describes a situation >> where VM's that are paused with an -EIO error are not automatically >> resumed after the problem with storage has been corrected. I have >> some patches [2] on gerrit that resolve the problem. Since this >> appears to be a regression I am looking at a non-intrusive way to fix >> it in the 3.5 branch. There is some disagreement on the proper way to >> fix this so I am hoping we can arrive at a solution through an open >> discussion. >> >> The main issue at hand is with the Event/Callback mechanism we use to >> call clientIF.contEIOVMs. According to my experiments and this online >> discussion [3] weakref does not work for instance methods such as >> clientIF.contEIOVMs. Our Event class uses weakref to prevent it from >> holding references to registered callback functions. > >Why making event system more correct is required to tix [1]? > >> >> I see two easy ways to fix the regression: > >I don't follow, what is the regression? Assuming that at some point contEIOVMs actually worked and was able to automatically resume VMs, then we have a regression because given the weakref problems I am describing herein there is no way that it is working now. The only way we don't have a regression is if this code has never worked to begin with.

>> 1) Treat clientIF as a singleton class (which it is) and make >> contEIOVMs a module-level method which gets the clientIF instance >> and calls it's bound contEIOVMs method. See my patches [2] for the >> code behind this idea. > >This is the wrong direction. There is only one place using that horrible >getInstance(), and it also could just create the single instance that we >need. We should remove getInstance() instead of using it in new code. > >> >> 2) Allow Event to maintain a strong reference on the bound >> clientIF.contEIOVMs method. This will allow the current code to work >> as designed but will change the Event implementation to accomodate >> this specific use case. Since no one else appears to be using this >> code, it should have no functional impact. > >The code is already holding a strong reference now, no change is >needed :-) I disagree. From vdsm/storage/misc.py: class Event(object): ... def register(self, func, oneshot=False): with self._syncRoot: self._registrar[id(func)] = (weakref.ref(func), oneshot) ... # ^^^ "He's dead Jim" The function is converted into a weak reference. Since, in this case, the function is an instance method, the reference is immediately dead on arrival. I have verified this with debugging statements in my environment.

From: "Adam Litke" <alitke(a)redhat.com&gt; To: "Nir Soffer" <nsoffer(a)redhat.com&gt; Cc: devel(a)ovirt.org, "Francesco Romani" <fromani(a)redhat.com&gt;, "Federico Simoncelli" <fsimonce(a)redhat.com&gt;, "Dan Kenigsberg" <danken(a)redhat.com&gt; Sent: Friday, November 21, 2014 5:28:07 PM Subject: Re: contEIOVMs regression? On 21/11/14 10:03 -0500, Nir Soffer wrote: >----- Original Message ----- >> From: "Adam Litke" <alitke(a)redhat.com&gt; >> To: "Nir Soffer" <nsoffer(a)redhat.com&gt; >> Cc: devel(a)ovirt.org, "Francesco Romani" <fromani(a)redhat.com&gt;, "Federico >> Simoncelli" <fsimonce(a)redhat.com&gt;, "Dan >> Kenigsberg" <danken(a)redhat.com&gt; >> Sent: Friday, November 21, 2014 4:46:13 PM >> Subject: Re: contEIOVMs regression? >> >> On 20/11/14 17:37 -0500, Nir Soffer wrote: >> >----- Original Message ----- >> >> From: "Adam Litke" <alitke(a)redhat.com&gt; >> >> To: devel(a)ovirt.org >> >> Cc: "Nir Soffer" <nsoffer(a)redhat.com&gt;, "Francesco Romani" >> >> <fromani(a)redhat.com&gt;, "Federico Simoncelli" >> >> <fsimonce(a)redhat.com&gt;, "Dan Kenigsberg" <danken(a)redhat.com&gt; >> >> Sent: Thursday, November 20, 2014 9:15:33 PM >> >> Subject: contEIOVMs regression? >> >> >> >> Hi list, >> >> >> >> I am taking a look at Bug 1157421 [1] which describes a situation >> >> where VM's that are paused with an -EIO error are not automatically >> >> resumed after the problem with storage has been corrected. I have >> >> some patches [2] on gerrit that resolve the problem. Since this >> >> appears to be a regression I am looking at a non-intrusive way to fix >> >> it in the 3.5 branch. There is some disagreement on the proper way to >> >> fix this so I am hoping we can arrive at a solution through an open >> >> discussion. >> >> >> >> The main issue at hand is with the Event/Callback mechanism we use to >> >> call clientIF.contEIOVMs. According to my experiments and this online >> >> discussion [3] weakref does not work for instance methods such as >> >> clientIF.contEIOVMs. Our Event class uses weakref to prevent it from >> >> holding references to registered callback functions. >> > >> >Why making event system more correct is required to tix [1]? >> > >> >> >> >> I see two easy ways to fix the regression: >> > >> >I don't follow, what is the regression? >> >> Assuming that at some point contEIOVMs actually worked and was able to >> automatically resume VMs, then we have a regression because given the >> weakref problems I am describing herein there is no way that it is >> working now. The only way we don't have a regression is if this code >> has never worked to begin with. > >The current code is master do work - when I fixed this last time, the >problem >was that we did not register the callback before starting the monitors, and >that the monitors did not issue a state change on the first time a monitor >check the domain state. > >I verified that contEIOVMs is called and that it does try to continue vms. Very curious. I am working with 3.5.0. The main difference (other than branch) is that I am working in an environment with no connected storage pool. Though I still can't see how the weakref stuff could be working in master. >If this does not break now (with current code), please open a bug. > >> >> >> 1) Treat clientIF as a singleton class (which it is) and make >> >> contEIOVMs a module-level method which gets the clientIF instance >> >> and calls it's bound contEIOVMs method. See my patches [2] for the >> >> code behind this idea. >> > >> >This is the wrong direction. There is only one place using that horrible >> >getInstance(), and it also could just create the single instance that we >> >need. We should remove getInstance() instead of using it in new code. >> > >> >> >> >> 2) Allow Event to maintain a strong reference on the bound >> >> clientIF.contEIOVMs method. This will allow the current code to work >> >> as designed but will change the Event implementation to accomodate >> >> this specific use case. Since no one else appears to be using this >> >> code, it should have no functional impact. >> > >> >The code is already holding a strong reference now, no change is >> >needed :-) >> >> I disagree. From vdsm/storage/misc.py: >> class Event(object): >> ... >> def register(self, func, oneshot=False): >> with self._syncRoot: >> self._registrar[id(func)] = (weakref.ref(func), oneshot) >> ... >> # ^^^ "He's dead Jim" >> >> The function is converted into a weak reference. Since, in this case, >> the function is an instance method, the reference is immediately dead >> on arrival. I have verified this with debugging statements in my >> environment. > >So you suggest that taking a weakref to an instance method returns >a dead reference? > >I thought that the problem is instance method keep hard reference to >the instance, so the weakref is useless. Yeah, try out this test program to see what I mean: #!/usr/bin/env python import weakref from functools import partial class A(object): def __init__(self): self.r1 = weakref.ref(self.a) self.r2 = partial(A.a, weakref.proxy(self)) def a(self): print "Hello from a" def main(): obj = A() print obj.r1 obj.r2() if __name__ == '__main__': main()