Re: Issue 1175 in ganeti: ext3 filesystem hardcoded in fstab of import script

[ganeti]

Comment #6 on issue 1175 by hostingn...@gmail.com: ext3 filesystem  
hardcoded in fstab of import script

https://code.google.com/p/ganeti/issues/detail?id=1175

@ius... thanks for the tip but backports packages are not considered as  
stable, hence "backports", and as such I do not want such package on a  
production system


--
You received this message because this project is configured to send all  
issue notifications to this address.

You may adjust your notification preferences at:
https://code.google.com/hosting/settings

Re: [MERGE] stable-2.17 to master

['Iustin Pop' via ganeti-devel]

2016-06-10 16:25 GMT+02:00 'Brian Foley' via ganeti-devel <
ganeti-devel@googlegroups.com>:

> commit 7eb49311e18865db76c4e8da5eb4b2e166db2d55
> Merge: a32eb3c 17a1c27
> Author: Brian Foley 
> Date:   Fri Jun 10 15:20:33 2016 +0100
>
> Merge branch 'stable-2.17'
>
> * stable-2.17
>   
>
> * stable-2.16
>   Fix optimisation: Correctly extract secondary node
>   Tune getNodeInstances DRBD secondary computation
>   Get haskell daemons to only compress files > 4kB
>   Use zlib compression level 3 in Haskell RPC code
>
> * stable-2.15
>   Fixup compatibility with GHC 7.4/base 4.5
>
> Signed-off-by: Brian Foley 
>

LGTM, thanks.

iustin

[MERGE] stable-2.17 to master

['Brian Foley' via ganeti-devel]

commit 7eb49311e18865db76c4e8da5eb4b2e166db2d55
Merge: a32eb3c 17a1c27
Author: Brian Foley 
Date:   Fri Jun 10 15:20:33 2016 +0100

Merge branch 'stable-2.17'

* stable-2.17
  

* stable-2.16
  Fix optimisation: Correctly extract secondary node
  Tune getNodeInstances DRBD secondary computation
  Get haskell daemons to only compress files > 4kB
  Use zlib compression level 3 in Haskell RPC code

* stable-2.15
  Fixup compatibility with GHC 7.4/base 4.5

Signed-off-by: Brian Foley 

diff --cc src/Ganeti/Codec.hs
index a703ba8,6f54c0d..b3b4c87
--- a/src/Ganeti/Codec.hs
+++ b/src/Ganeti/Codec.hs
@@@ -42,12 -40,12 +42,12 @@@ module Ganeti.Code
  import Prelude ()
  import Ganeti.Prelude
  
- import Codec.Compression.Zlib (compress)
+ import Codec.Compression.Zlib
  import qualified Codec.Compression.Zlib.Internal as I
  import Control.Monad (liftM)
 -import Control.Monad.Error.Class (MonadError(..))
  import qualified Data.ByteString.Lazy as BL
  import qualified Data.ByteString.Lazy.Internal as BL
 +import Control.Monad.Error.Class (MonadError(..))
  
  import Ganeti.BasicTypes
  

Re: [MERGE] stable-2.16 to stable-2.17

['Iustin Pop' via ganeti-devel]

2016-06-10 15:40 GMT+02:00 'Brian Foley' via ganeti-devel <
ganeti-devel@googlegroups.com>:

> commit b462d8c77bff0789e8a951288dea34226ab8b6d7
> Merge: 20c24a8 90281b4
> Author: Brian Foley 
> Date:   Fri Jun 10 14:35:13 2016 +0100
>
> Merge branch 'stable-2.16' into stable-2.17
>
> * stable-2.16
>   Fix optimisation: Correctly extract secondary node
>   Tune getNodeInstances DRBD secondary computation
>   Get haskell daemons to only compress files > 4kB
>   Use zlib compression level 3 in Haskell RPC code
>
> * stable-2.15
>   Fixup compatibility with GHC 7.4/base 4.5
>
> Manually resolve import conflicts.
>
> Signed-off-by: Brian Foley 
>

LGTM, although I can't check the merge, but the diffs look sane.

diff --cc src/Ganeti/Codec.hs
> index 9a41499,404c70b..6f54c0d
> --- a/src/Ganeti/Codec.hs
> +++ b/src/Ganeti/Codec.hs
> @@@ -37,21 -37,18 +37,23 @@@ module Ganeti.Code
> , decompressZlib
> ) where
>
>  +import Prelude ()
>  +import Ganeti.Prelude
>

Oh, this is interesting, I was wondering if there is such a module, and it
seems there is :)

iustin

Re: Issue 1175 in ganeti: ext3 filesystem hardcoded in fstab of import script

[ganeti]

Comment #5 on issue 1175 by ius...@google.com: ext3 filesystem hardcoded in  
fstab of import script

https://code.google.com/p/ganeti/issues/detail?id=1175

@hosting: FYI, there's a newer package in jeesie-backports; I'd recommend  
using that.


--
You received this message because this project is configured to send all  
issue notifications to this address.

You may adjust your notification preferences at:
https://code.google.com/hosting/settings

[MERGE] stable-2.16 to stable-2.17

['Brian Foley' via ganeti-devel]

commit b462d8c77bff0789e8a951288dea34226ab8b6d7
Merge: 20c24a8 90281b4
Author: Brian Foley 
Date:   Fri Jun 10 14:35:13 2016 +0100

Merge branch 'stable-2.16' into stable-2.17

* stable-2.16
  Fix optimisation: Correctly extract secondary node
  Tune getNodeInstances DRBD secondary computation
  Get haskell daemons to only compress files > 4kB
  Use zlib compression level 3 in Haskell RPC code

* stable-2.15
  Fixup compatibility with GHC 7.4/base 4.5

Manually resolve import conflicts.

Signed-off-by: Brian Foley 

diff --cc src/Ganeti/Codec.hs
index 9a41499,404c70b..6f54c0d
--- a/src/Ganeti/Codec.hs
+++ b/src/Ganeti/Codec.hs
@@@ -37,21 -37,18 +37,23 @@@ module Ganeti.Code
, decompressZlib
) where
  
 +import Prelude ()
 +import Ganeti.Prelude
 +
- import Codec.Compression.Zlib (compress)
+ import Codec.Compression.Zlib
  import qualified Codec.Compression.Zlib.Internal as I
 -import Control.Monad.Error
 +import Control.Monad (liftM)
 +import Control.Monad.Error.Class (MonadError(..))
  import qualified Data.ByteString.Lazy as BL
  import qualified Data.ByteString.Lazy.Internal as BL
 -import Data.Monoid (mempty)
 +
 +import Ganeti.BasicTypes
  
+ 
  -- | Compresses a lazy bytestring.
  compressZlib :: BL.ByteString -> BL.ByteString
- compressZlib = compress
+ compressZlib = compressWith $
+   defaultCompressParams { compressLevel = CompressionLevel 3 }
  
  -- | Decompresses a lazy bytestring, throwing decoding errors using
  -- 'throwError'.
diff --cc src/Ganeti/JQScheduler.hs
index a229b2f,df6fefc..4c594fa
--- a/src/Ganeti/JQScheduler.hs
+++ b/src/Ganeti/JQScheduler.hs
@@@ -55,22 -52,12 +55,22 @@@ import Control.Applicative (liftA2
  import Control.Arrow
  import Control.Concurrent
  import Control.Exception
 -import Control.Monad
 +import Control.Monad ( when
 + , mfilter
 + , liftM
 + , void
 + , unless
 + , forever
 + , forM_)
  import Control.Monad.IO.Class
  import Data.Function (on)
- import Data.IORef
 -import Data.Functor ((<$))
+ import Data.IORef (IORef, atomicModifyIORef, newIORef, readIORef)
 -import Data.List
 +import Data.List ( find
 + , deleteFirstsBy
 + , sortBy
 + , intercalate
 + , partition
 + , insertBy)
  import Data.Maybe
  import qualified Data.Map as Map
  import Data.Ord (comparing)

Re: [MERGE] stable-2.15 to stable 2.16

['Iustin Pop' via ganeti-devel]

2016-06-10 15:28 GMT+02:00 'Brian Foley' via ganeti-devel <
ganeti-devel@googlegroups.com>:

> commit 5785f214a9e728465a4bfc1aef7ded306225cfa2
> Merge: 40cd52f 2429235
> Author: Brian Foley 
> Date:   Fri Jun 10 14:23:10 2016 +0100
>
> Merge branch 'stable-2.15' into stable-2.16
>
> * stable-2.15
>   Fixup compatibility with GHC 7.4/base 4.5
>
> Signed-off-by: Brian Foley 
>

LGTM, thanks.

iustin

[MERGE] stable-2.15 to stable 2.16

['Brian Foley' via ganeti-devel]

commit 5785f214a9e728465a4bfc1aef7ded306225cfa2
Merge: 40cd52f 2429235
Author: Brian Foley 
Date:   Fri Jun 10 14:23:10 2016 +0100

Merge branch 'stable-2.15' into stable-2.16

* stable-2.15
  Fixup compatibility with GHC 7.4/base 4.5

Signed-off-by: Brian Foley 

Re: [PATCH stable-2.15] Fixup compatibility with GHC 7.4/base 4.5

['Brian Foley' via ganeti-devel]

On Fri, Jun 10, 2016 at 01:51:56PM +0200, 'Iustin Pop' via ganeti-devel wrote:
> It looks like commit c429dd26 introduced the use of
> atomicModifyIORef', which is only present in base 4.6 (GHC 7.6).
> Let's fix that by importing the actual implementation of said function
> from base 4.6 in case we're running with earlier versions.
> 
> Signed-off-by: Iustin Pop 

LGTM. Thanks!

[PATCH stable-2.15] Fixup compatibility with GHC 7.4/base 4.5

['Iustin Pop' via ganeti-devel]

It looks like commit c429dd26 introduced the use of
atomicModifyIORef', which is only present in base 4.6 (GHC 7.6).
Let's fix that by importing the actual implementation of said function
from base 4.6 in case we're running with earlier versions.

Signed-off-by: Iustin Pop 
---
 src/Ganeti/Compat.hs  | 14 ++
 src/Ganeti/JQScheduler.hs |  3 ++-
 2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/src/Ganeti/Compat.hs b/src/Ganeti/Compat.hs
index e5276d1..8e002e7 100644
--- a/src/Ganeti/Compat.hs
+++ b/src/Ganeti/Compat.hs
@@ -41,10 +41,12 @@ module Ganeti.Compat
   ( rwhnf
   , Control.Parallel.Strategies.parMap
   , finiteBitSize
+  , atomicModifyIORef'
   ) where
 
 import qualified Control.Parallel.Strategies
 import qualified Data.Bits
+import qualified Data.IORef
 
 -- | Wrapper over the function exported from
 -- "Control.Parallel.Strategies".
@@ -67,3 +69,15 @@ finiteBitSize :: (Data.Bits.FiniteBits a) => a -> Int
 finiteBitSize = Data.Bits.finiteBitSize
 #endif
 {-# INLINE finiteBitSize #-}
+
+-- FIXME: remove this when dropping support for GHC 7.4.
+atomicModifyIORef' :: Data.IORef.IORef a -> (a -> (a, b)) -> IO b
+#if MIN_VERSION_base(4,6,0)
+atomicModifyIORef' = Data.IORef.atomicModifyIORef'
+#else
+atomicModifyIORef' ref f = do
+b <- Data.IORef.atomicModifyIORef ref $ \a ->
+case f a of
+v@(a',_) -> a' `seq` v
+b `seq` return b
+#endif
diff --git a/src/Ganeti/JQScheduler.hs b/src/Ganeti/JQScheduler.hs
index 4f18f47..3a46934 100644
--- a/src/Ganeti/JQScheduler.hs
+++ b/src/Ganeti/JQScheduler.hs
@@ -56,7 +56,7 @@ import Control.Monad
 import Control.Monad.IO.Class
 import Data.Function (on)
 import Data.Functor ((<$))
-import Data.IORef
+import Data.IORef (IORef, atomicModifyIORef, newIORef, readIORef)
 import Data.List
 import Data.Maybe
 import qualified Data.Map as Map
@@ -66,6 +66,7 @@ import qualified Data.Set as S
 import System.INotify
 
 import Ganeti.BasicTypes
+import Ganeti.Compat
 import Ganeti.Constants as C
 import Ganeti.Errors
 import Ganeti.JQScheduler.Filtering (applyingFilter, jobFiltering)
-- 
2.8.0.rc3.226.g39d4020

Re: [PATCH master] Fixup compatibility with GHC 7.4/base 4.5

['Iustin Pop' via ganeti-devel]

On Fri, Jun 10, 2016 at 12:07:45PM +0100, Brian Foley wrote:
> On Fri, Jun 10, 2016 at 12:33:23PM +0200, 'Iustin Pop' via ganeti-devel wrote:
> > On Fri, Jun 10, 2016 at 12:30:03PM +0200, Iustin Pop wrote:
> > > It looks like commit c429dd26 introduced the use of atomicModifyIORef', 
> > > which
> > > is only present in base 4.6 (GHC 7.6).  Let's temporarily fix that by 
> > > adding a
> > > small compat layer (which undoes the optimisations of using strict on 
> > > 7.4, but
> > > at least it works), until we decide to officially drop support for 7.4.
> > 
> > Forgot to add that I manually tested this on both ghc 7.4 (wheezy) and
> > 7.6.3.
> > 
> > iustin
> 
> Thanks.
> 
> I also noticed this problem later on when reading some the docs, but didn't
> get around to fixing it. Given the memory leak was pretty severe, could we 
> just
> put the implementation of atomicModifyIORef' from base-4.6.0.0 into the #ifdef
> rather than making it non-strict?
> 
> https://hackage.haskell.org/package/base-4.6.0.0/docs/src/Data-IORef.html#line-132

Good point. I chose the lazier way as I was not sure we care about
wheezy performance, but this is trivial enough indeed.

> Also, could you make this patch against stable-2.15 please (given that's where
> atomicModifyIORef' was introduced). I'll merge it forward to all the newer
> branches.

Sure thing! I looked at the commit and it was not included in any tag
(git describe --contains), so I thought it was done on master.

thanks,
iustin

Re: [PATCH master] Fixup compatibility with GHC 7.4/base 4.5

['Brian Foley' via ganeti-devel]

On Fri, Jun 10, 2016 at 12:33:23PM +0200, 'Iustin Pop' via ganeti-devel wrote:
> On Fri, Jun 10, 2016 at 12:30:03PM +0200, Iustin Pop wrote:
> > It looks like commit c429dd26 introduced the use of atomicModifyIORef', 
> > which
> > is only present in base 4.6 (GHC 7.6).  Let's temporarily fix that by 
> > adding a
> > small compat layer (which undoes the optimisations of using strict on 7.4, 
> > but
> > at least it works), until we decide to officially drop support for 7.4.
> 
> Forgot to add that I manually tested this on both ghc 7.4 (wheezy) and
> 7.6.3.
> 
> iustin

Thanks.

I also noticed this problem later on when reading some the docs, but didn't
get around to fixing it. Given the memory leak was pretty severe, could we just
put the implementation of atomicModifyIORef' from base-4.6.0.0 into the #ifdef
rather than making it non-strict?

https://hackage.haskell.org/package/base-4.6.0.0/docs/src/Data-IORef.html#line-132

Also, could you make this patch against stable-2.15 please (given that's where
atomicModifyIORef' was introduced). I'll merge it forward to all the newer
branches.

Cheers,
Brian.

Re: [PATCH master 8/8] Ignore another bug in System.Time.diffClockTimes

['Brian Foley' via ganeti-devel]

On Thu, Jun 09, 2016 at 11:13:58PM +0200, Iustin Pop wrote:
> From: Iustin Pop 
> 
> Reading the source for that function is scary: it uses local time conversions
> in the context of what should be absolute (not local) time, so of course it 
> has
> daylight savings time issues. It would be hard to work around those, so add a
> FIXME and work around the test failure.
> 
> Found by using seed 7676442054323374465 on a 64-bit machine:

Ugh. LGTM for now.

Re: [PATCH master 7/8] Replace getNode with getNodeByUuid in certain cases

['Brian Foley' via ganeti-devel]

On Thu, Jun 09, 2016 at 11:13:57PM +0200, Iustin Pop wrote:
> From: Iustin Pop 
> 
> Do this in cases where this should already be the case:
> - some functions are explicitly denoted to take UUIDs (documentation/parameter
>   name)
> - some functions (e.g. clusterMasterNode, disk IDs, instance primary node) we
>   know return an UUID
> 
> But still, this might be a behaviour change, although unlikely.
> 
> Signed-off-by: Iustin Pop 

I'm reasonably convinced that there should be no behavioural change, the
only possibly dubious case I could see was getInstPrimaryNode, and as
far as I can see all the callers of that use exact instance names.

LGTM.

Re: [PATCH master 6/8] Rework getInstDisks/getInstDisksFromObj

['Brian Foley' via ganeti-devel]

On Thu, Jun 09, 2016 at 11:13:56PM +0200, Iustin Pop wrote:
> From: Iustin Pop 
> 
> The function getInstDisksFromObj takes an Instance object, extracts its uuids,
> and passes that to getInstDisks. This function then looks up the given UUID in
> the configuration, retrieves the instance object associated with it, and
> proceeds to retrieves the disks of that object. In effect, this call chain not
> only does an unnecessary lookup, it also doesn't do what it claims, since it
> will not actually use the passed object. In some cases, the callers of
> getInstDisksFromObj were even looking the instance up before hand to pass it 
> to
> the function…
> 
> Drop the getInstDisks (by uuid/name version), since it's not used except by
> getInstDisksFromObj, and move parts of its functionality directly to
> getInstDisksFromObj, which is now renamed for simplicity.
> 
> Signed-off-by: Iustin Pop 

Ugh. What a mess. Thanks for untangling it. LGTM.

Re: [PATCH master] Fixup compatibility with GHC 7.4/base 4.5

['Iustin Pop' via ganeti-devel]

On Fri, Jun 10, 2016 at 12:30:03PM +0200, Iustin Pop wrote:
> It looks like commit c429dd26 introduced the use of atomicModifyIORef', which
> is only present in base 4.6 (GHC 7.6).  Let's temporarily fix that by adding a
> small compat layer (which undoes the optimisations of using strict on 7.4, but
> at least it works), until we decide to officially drop support for 7.4.

Forgot to add that I manually tested this on both ghc 7.4 (wheezy) and
7.6.3.

iustin

Re: [PATCH master 4/8] Split getInstance into *ByUuid and *ByPartialName

['Iustin Pop' via ganeti-devel]

On Fri, Jun 10, 2016 at 11:28:16AM +0100, Brian Foley wrote:
> On Thu, Jun 09, 2016 at 11:13:54PM +0200, Iustin Pop wrote:
> > From: Iustin Pop 
> > 
> > This function (and getNode, next patch) were two pain points when I tried to
> > convert UUIDs to another type, since they're called via a single type
> > (currently String) that wants to dispatch to either UUID or name. In many 
> > cases
> > we know exactly what we want to search for, so having only this function is
> > overly generic. Not useful yet, but will be later for the string type
> > conversion.
> > 
> > Additionally change the name of the existing function getInstanceByName to
> > match the new functions better.
> > 
> > Signed-off-by: Iustin Pop 
> 
> It took me a little while to convince myself that some of the callers should
> use getInstanceByExactName, but I think you're correct.

Note that I don't change that fact; I just renamed getInstanceByName to
getInstanceByExactName to make a clear distinction between PartialName
and ExactName.

But yes, it's a good point if they need to do that.

thanks,
iustin

[PATCH master] Fixup compatibility with GHC 7.4/base 4.5

['Iustin Pop' via ganeti-devel]

It looks like commit c429dd26 introduced the use of atomicModifyIORef', which
is only present in base 4.6 (GHC 7.6).  Let's temporarily fix that by adding a
small compat layer (which undoes the optimisations of using strict on 7.4, but
at least it works), until we decide to officially drop support for 7.4.

Signed-off-by: Iustin Pop 
---
 src/Ganeti/Compat.hs  | 10 ++
 src/Ganeti/JQScheduler.hs |  5 +++--
 2 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/src/Ganeti/Compat.hs b/src/Ganeti/Compat.hs
index e5276d1..3ee3293 100644
--- a/src/Ganeti/Compat.hs
+++ b/src/Ganeti/Compat.hs
@@ -41,10 +41,12 @@ module Ganeti.Compat
   ( rwhnf
   , Control.Parallel.Strategies.parMap
   , finiteBitSize
+  , atomicModifyIORef'
   ) where
 
 import qualified Control.Parallel.Strategies
 import qualified Data.Bits
+import qualified Data.IORef
 
 -- | Wrapper over the function exported from
 -- "Control.Parallel.Strategies".
@@ -67,3 +69,11 @@ finiteBitSize :: (Data.Bits.FiniteBits a) => a -> Int
 finiteBitSize = Data.Bits.finiteBitSize
 #endif
 {-# INLINE finiteBitSize #-}
+
+-- FIXME: remove this when dropping support for GHC 7.4.
+atomicModifyIORef' :: Data.IORef.IORef a -> (a -> (a, b)) -> IO b
+#if MIN_VERSION_base(4,6,0)
+atomicModifyIORef' = Data.IORef.atomicModifyIORef'
+#else
+atomicModifyIORef' = Data.IORef.atomicModifyIORef
+#endif
diff --git a/src/Ganeti/JQScheduler.hs b/src/Ganeti/JQScheduler.hs
index 2eae077..9d0f72d 100644
--- a/src/Ganeti/JQScheduler.hs
+++ b/src/Ganeti/JQScheduler.hs
@@ -64,7 +64,7 @@ import Control.Monad ( when
  , forM_)
 import Control.Monad.IO.Class
 import Data.Function (on)
-import Data.IORef
+import Data.IORef (IORef, atomicModifyIORef, newIORef, readIORef)
 import Data.List ( find
  , deleteFirstsBy
  , sortBy
@@ -79,6 +79,7 @@ import qualified Data.Set as S
 import System.INotify
 
 import Ganeti.BasicTypes
+import Ganeti.Compat
 import Ganeti.Constants as C
 import Ganeti.Errors
 import Ganeti.JQScheduler.Filtering (applyingFilter, jobFiltering)
@@ -449,7 +450,7 @@ scheduleSomeJobs qstate = do
 showQueue :: Queue -> String
 showQueue (Queue {qEnqueued=waiting, qRunning=running}) =
   let showids = show . map (fromJobId . qjId . jJob)
-  in "Waiting jobs: " ++ showids waiting 
+  in "Waiting jobs: " ++ showids waiting
++ "; running jobs: " ++ showids running
 
 -- | Check if a job died, and clean up if so. Return True, if
-- 
2.8.0.rc3.226.g39d4020

Re: [PATCH master 5/8] Split getNode into *ByUuid and *ByPartialName

['Brian Foley' via ganeti-devel]

On Thu, Jun 09, 2016 at 11:13:55PM +0200, Iustin Pop wrote:
> From: Iustin Pop 
> 
> This follows the getInstance changes, with the same rationale.
> 
> Signed-off-by: Iustin Pop 

LGTM

Re: [PATCH master 4/8] Split getInstance into *ByUuid and *ByPartialName

['Brian Foley' via ganeti-devel]

On Thu, Jun 09, 2016 at 11:13:54PM +0200, Iustin Pop wrote:
> From: Iustin Pop 
> 
> This function (and getNode, next patch) were two pain points when I tried to
> convert UUIDs to another type, since they're called via a single type
> (currently String) that wants to dispatch to either UUID or name. In many 
> cases
> we know exactly what we want to search for, so having only this function is
> overly generic. Not useful yet, but will be later for the string type
> conversion.
> 
> Additionally change the name of the existing function getInstanceByName to
> match the new functions better.
> 
> Signed-off-by: Iustin Pop 

It took me a little while to convince myself that some of the callers should
use getInstanceByExactName, but I think you're correct.

LGTM.

> ---
>  src/Ganeti/Config.hs | 43 ++-
>  1 file changed, 26 insertions(+), 17 deletions(-)
> 
> diff --git a/src/Ganeti/Config.hs b/src/Ganeti/Config.hs
> index bce7ae3..0d935c4 100644
> --- a/src/Ganeti/Config.hs
> +++ b/src/Ganeti/Config.hs
> @@ -275,22 +275,24 @@ getNode cfg name =
>(nodeName . (M.!) nodes) nodes
>  in getItem "Node" name by_name
>  
> --- | Looks up an instance by name or uuid.
> -getInstance :: ConfigData -> String -> ErrorResult Instance
> -getInstance cfg name =
> +-- | Looks up an instance by uuid.
> +getInstanceByUuid :: ConfigData -> String -> ErrorResult Instance
> +getInstanceByUuid cfg uuid =
>let instances = fromContainer (configInstances cfg)
> -  in case getItem' "Instance" name instances of
> -   -- if not found by uuid, we need to look it up by name
> -   Ok inst -> Ok inst
> -   Bad _ -> let by_name =
> -  M.delete ""
> -  . M.mapKeys (fromMaybe "" . instName . (M.!) instances)
> -  $ instances
> -in getItem "Instance" name by_name
> -
> --- | Looks up an instance by exact name match
> -getInstanceByName :: ConfigData -> String -> ErrorResult Instance
> -getInstanceByName cfg name =
> +  in getItem' "Instance" uuid instances
> +
> +-- | Looks up an instance by approximate name.
> +getInstanceByPartialName :: ConfigData -> String -> ErrorResult Instance
> +getInstanceByPartialName cfg name =
> +  let instances = fromContainer (configInstances cfg)
> +  by_name = M.delete ""
> +  . M.mapKeys (fromMaybe "" . instName . (M.!) instances)
> +  $ instances
> +  in getItem "Instance" name by_name
> +
> +-- | Looks up an instance by exact name match.
> +getInstanceByExactName :: ConfigData -> String -> ErrorResult Instance
> +getInstanceByExactName cfg name =
>let instances = M.elems . fromContainer . configInstances $ cfg
>matching = F.find (maybe False (== name) . instName) instances
>in case matching of
> @@ -299,6 +301,13 @@ getInstanceByName cfg name =
> ("Instance name " ++ name ++ " not found")
> ECodeNoEnt
>  
> +-- | Looks up an instance by partial name or uuid.
> +getInstance :: ConfigData -> String -> ErrorResult Instance
> +getInstance cfg name =
> +  case getInstanceByUuid cfg name of
> +x@(Ok _) -> x
> +Bad _ -> getInstanceByPartialName cfg name
> +
>  -- | Looks up a disk by uuid.
>  getDisk :: ConfigData -> String -> ErrorResult Disk
>  getDisk cfg name =
> @@ -432,7 +441,7 @@ getFilledInstOsParams cfg inst =
>  -- | Looks up an instance's primary node.
>  getInstPrimaryNode :: ConfigData -> String -> ErrorResult Node
>  getInstPrimaryNode cfg name =
> -  getInstanceByName cfg name
> +  getInstanceByExactName cfg name
>>>= withMissingParam "Instance without primary node" return . 
> instPrimaryNode
>>>= getNode cfg
>  
> @@ -451,7 +460,7 @@ getDrbdDiskNodes cfg disk =
>  -- the primary node has to be appended to the results.
>  getInstAllNodes :: ConfigData -> String -> ErrorResult [Node]
>  getInstAllNodes cfg name = do
> -  inst <- getInstanceByName cfg name
> +  inst <- getInstanceByExactName cfg name
>inst_disks <- getInstDisksFromObj cfg inst
>let disk_nodes = concatMap (getDrbdDiskNodes cfg) inst_disks
>pNode <- getInstPrimaryNode cfg name
> -- 
> 2.8.1
> 

Re: [PATCH master 3/8] Switch TagSet from type synonim to a newtype

['Brian Foley' via ganeti-devel]

On Thu, Jun 09, 2016 at 11:13:53PM +0200, Iustin Pop wrote:
> From: Iustin Pop 
> 
> This started from the fact that recent QuickCheck declares itself an arbitrary
> instance "(Arbitrary a) => Arbitrary (Set a)", which conflicts with our
> slightly more specific instance.  The easiest way to fix this is (especially
> since this is test code) an OverlappingInstances language extension, but that
> is deprecated in GHC 7.10 :(
> 
> The solution is then to change this to a proper type, instead of an alias.  In
> the exercise, I found out that we were not really using TagSet, but instead
> directly using Set, which is not nice. Plus many redefinitions at the call 
> site
> of the same arbitrary instance! Not happy with type synonims since they're 
> just
> smoke and mirrors…
> 
> Signed-off-by: Iustin Pop 

Getting rid of all those extra Set.fromList <$> genTags and all those other
explicit uses of Set is definitely an improvement.

LGTM. Thanks!

> ---
>  src/Ganeti/Objects.hs | 12 ++--
>  src/Ganeti/Objects/Lens.hs|  3 +--
>  src/Ganeti/THH/Field.hs   | 18 ++
>  src/Ganeti/Types.hs   |  5 ++---
>  src/Ganeti/WConfd/Ssconf.hs   |  2 +-
>  test/hs/Test/Ganeti/Objects.hs| 12 ++--
>  test/hs/Test/Ganeti/Query/Instance.hs |  3 +--
>  7 files changed, 31 insertions(+), 24 deletions(-)
> 
> diff --git a/src/Ganeti/Objects.hs b/src/Ganeti/Objects.hs
> index 603f809..00bb7a2 100644
> --- a/src/Ganeti/Objects.hs
> +++ b/src/Ganeti/Objects.hs
> @@ -80,11 +80,12 @@ module Ganeti.Objects
>, Cluster(..)
>, ConfigData(..)
>, TimeStampObject(..) -- re-exported from Types
> -  , UuidObject(..) -- re-exported from Types
> -  , SerialNoObject(..) -- re-exported from Types
> -  , TagsObject(..) -- re-exported from Types
> -  , DictObject(..) -- re-exported from THH
> -  , TagSet -- re-exported from THH
> +  , UuidObject(..)  -- re-exported from Types
> +  , SerialNoObject(..)  -- re-exported from Types
> +  , TagsObject(..)  -- re-exported from Types
> +  , DictObject(..)  -- re-exported from THH
> +  , TagSet(..)  -- re-exported from THH
> +  , emptyTagSet -- re-exported from THH
>, Network(..)
>, AddressPool(..)
>, Ip4Address()
> @@ -749,4 +750,3 @@ $(buildObject "MasterNetworkParameters" 
> "masterNetworkParameters"
>, simpleField "netdev"[t| String   |]
>, simpleField "ip_family" [t| IpFamily |]
>])
> -
> diff --git a/src/Ganeti/Objects/Lens.hs b/src/Ganeti/Objects/Lens.hs
> index 3f27981..8b8da74 100644
> --- a/src/Ganeti/Objects/Lens.hs
> +++ b/src/Ganeti/Objects/Lens.hs
> @@ -40,7 +40,6 @@ import qualified Data.ByteString as BS
>  import qualified Data.ByteString.UTF8 as UTF8
>  import Control.Lens (Simple)
>  import Control.Lens.Iso (Iso, iso)
> -import qualified Data.Set as Set
>  import System.Time (ClockTime(..))
>  
>  import Ganeti.Lens (makeCustomLenses, Lens')
> @@ -64,7 +63,7 @@ class SerialNoObject a => SerialNoObjectL a where
>  
>  -- | Class of objects that have tags.
>  class TagsObject a => TagsObjectL a where
> -  tagsL :: Lens' a (Set.Set String)
> +  tagsL :: Lens' a (TagSet)
>  
>  $(makeCustomLenses ''AddressPool)
>  
> diff --git a/src/Ganeti/THH/Field.hs b/src/Ganeti/THH/Field.hs
> index 6047ca4..2dea7f7 100644
> --- a/src/Ganeti/THH/Field.hs
> +++ b/src/Ganeti/THH/Field.hs
> @@ -43,7 +43,8 @@ module Ganeti.THH.Field
>, timeStampFields
>, uuidFields
>, serialFields
> -  , TagSet
> +  , TagSet(..)
> +  , emptyTagSet
>, tagsFields
>, fileModeAsIntField
>, processIdField
> @@ -121,12 +122,21 @@ serialFields =
>  uuidFields :: [Field]
>  uuidFields = [ presentInForthcoming $ simpleField "uuid" [t| BS.ByteString 
> |] ]
>  
> --- | Tag set type alias.
> -type TagSet = Set.Set String
> +-- | Tag set type.
> +newtype TagSet = TagSet { fromTagSet :: Set.Set String }
> +  deriving (Show, Eq)
> +
> +instance JSON.JSON TagSet where
> +  readJSON = liftM TagSet . JSON.readJSON
> +  showJSON = JSON.showJSON . fromTagSet
> +
> +-- | The empty tag set.
> +emptyTagSet :: TagSet
> +emptyTagSet = TagSet Set.empty
>  
>  -- | Tag field description.
>  tagsFields :: [Field]
> -tagsFields = [ defaultField [| Set.empty |] $
> +tagsFields = [ defaultField [| emptyTagSet |] $
> simpleField "tags" [t| TagSet |] ]
>  
>  -- ** Fields related to POSIX data types
> diff --git a/src/Ganeti/Types.hs b/src/Ganeti/Types.hs
> index 437e62a..c332ba0 100644
> --- a/src/Ganeti/Types.hs
> +++ b/src/Ganeti/Types.hs
> @@ -197,12 +197,12 @@ import Control.Monad (liftM)
>  import qualified Text.JSON as JSON
>  import Text.JSON (JSON, readJSON, showJSON)
>  import Data.Ratio (numerator, denominator)
> -import qualified Data.Set as Set
>  import System.Time (ClockTime)
>  
>  import qualified Ganeti.ConstantUtils as ConstantUtils
>  import Ganeti.JSON
>  import qualified Ganeti.THH as THH
> 

Re: [PATCH master 2/8] Slightly improve the JSScheduler/prop_jobFiltering test

['Brian Foley' via ganeti-devel]

On Thu, Jun 09, 2016 at 11:13:52PM +0200, Iustin Pop wrote:
> From: Iustin Pop 
> 
> For some reason, this test doesn't pass on Debian unstable. I don't know if it
> passes on other versions of QuickCheck, but upon investigation, I think it was
> always broken, but the use of stableCover hides it.
> 
> The problem is that the test wants min 4% coverage of the enumerated actions 
> as
> applyingFilter, among which is also Continue. But according to that function's
> code and comments, Continue can never be the result of applyingFilter, which
> means that this test passes randomly only then stableCover generates 2/10 True
> cases (which can happen, since frequency AFAIK doesn't guarantee absolute
> numbers); this makes the test pass as our required percent is very low (2/10 >
> 1.3/10 which is what we require). Removing the 'stableCover' call and using
> instead 'cover' shows correctly that Cover is generated in 0% of cases (i.e.
> never). Therefore, let's remove Cover from the list of required actions.
> 
> Additionally, slightly improve the test: instead of using '==>' and discarding
> post-generation the unsuited tests, directly filter them during generation,
> potentially making the test slightly faster.
> 
> Signed-off-by: Iustin Pop 

LGTM (and today I learned...)

Thanks,
Brian.

Re: [PATCH master 1/8] Fixup cabal dependencies relations

['Viktor Bachraty' via ganeti-devel]

LGTM

On Thu, Jun 9, 2016 at 10:13 PM, Iustin Pop  wrote:

> From: Iustin Pop 
>
> In Haskell world (at least as far as I know), dependencies of type "<=
> A.B.C"
> are rarely used, because incrementing the fourth version component (i.e. D
> in
> A.B.C.D) doesn't change the API, hence this form is overly restrictive.
>
> Fixup the cabal dependencies by changing all "<= A.B.C" into "< A.B.(C+1)".
> This allows building/configuring on latest Debian unstable.
>
> Signed-off-by: Iustin Pop 
> ---
>  cabal/ganeti.template.cabal | 12 ++--
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/cabal/ganeti.template.cabal b/cabal/ganeti.template.cabal
> index b619a85..bf8aeed 100644
> --- a/cabal/ganeti.template.cabal
> +++ b/cabal/ganeti.template.cabal
> @@ -54,7 +54,7 @@ library
>  , transformers  >= 0.3.0.0
>  , unix  >= 2.5.1.0
>
> -, attoparsec>= 0.10.1.1   && <= 0.13.0.1
> +, attoparsec>= 0.10.1.1   && < 0.13.1
>  , base64-bytestring >= 1.0.0.1&& < 1.1
>  , case-insensitive  >= 0.4.0.1&& < 1.3
>  , Crypto>= 4.2.4  && < 4.3
> @@ -62,7 +62,7 @@ library
>  , hinotify  >= 0.3.2  && < 0.4
>  , hslogger  >= 1.1.4  && < 1.3
>  , json  >= 0.5&& < 1.0
> -, lens  >= 3.10   && <= 4.13.1
> +, lens  >= 3.10   && < 4.13.2
>  , lifted-base   >= 0.2.0.3&& < 0.3
>  , monad-control >= 0.3.1.3&& < 1.1
>  , MonadCatchIO-transformers >= 0.3.0.0&& < 0.4
> @@ -71,8 +71,8 @@ library
>  , regex-pcre>= 0.94.2 && < 0.95
>  , temporary >= 1.1.2.3&& < 1.3
>  , transformers-base >= 0.4.1  && < 0.5
> -, utf8-string   >= 0.3.7  && <= 1.0.1.1
> -, zlib  >= 0.5.3.3&& <= 0.6.1.1
> +, utf8-string   >= 0.3.7  && < 1.0.2
> +, zlib  >= 0.5.3.3&& < 0.6.2
>
>  -- Executables:
>  -- , happy
> @@ -81,8 +81,8 @@ library
>
>if flag(htest)
>  build-depends:
> -HUnit >= 1.2.4.2&& <= 1.3.1.0
> -  , QuickCheck>= 2.4.2  && <= 2.8.2
> +HUnit >= 1.2.4.2&& < 1.3.2
> +  , QuickCheck>= 2.4.2  && < 2.8.3
>, test-framework>= 0.6&& < 0.9
>, test-framework-hunit  >= 0.2.7  && < 0.4
>, test-framework-quickcheck2>= 0.2.12.1   && < 0.4
> --
> 2.8.1
>
>

Re: [PATCH master 3/3] Fix KVM pinning tests to not depend on the local machine

['Viktor Bachraty' via ganeti-devel]

On Fri, Jun 10, 2016 at 10:02 AM, Iustin Pop  wrote:

> 2016-06-10 10:58 GMT+02:00 Viktor Bachraty :
>
>>
>> On Thu, Jun 9, 2016 at 11:33 PM, Iustin Pop  wrote:
>>
>>> From: Iustin Pop 
>>>
>>> Commit 8b2ec2f added unittests for KVM pinning, but it introduced a
>>> non-obvious
>>> local dependency in the tests: the CPU_PINNING_OFF calls work by looking
>>> at the
>>> (current) machine's core count, and pinning to all those CPUs. In order
>>> to make
>>> this work independently from the test machine, we must also mock the
>>> result of
>>> process.cpu_count(). Do this by using a core count that is very much
>>> unlikely
>>> to ever be present in the real world.
>>>
>>> Signed-off-by: Iustin Pop 
>>> ---
>>>  test/py/ganeti.hypervisor.hv_kvm_unittest.py | 11 +++
>>>  1 file changed, 7 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/test/py/ganeti.hypervisor.hv_kvm_unittest.py b/test/py/
>>> ganeti.hypervisor.hv_kvm_unittest.py
>>> index c7a53b5..55ffb9b 100755
>>> --- a/test/py/ganeti.hypervisor.hv_kvm_unittest.py
>>> +++ b/test/py/ganeti.hypervisor.hv_kvm_unittest.py
>>> @@ -37,6 +37,7 @@ import socket
>>>  import os
>>>  import struct
>>>  import re
>>> +from contextlib import nested
>>>
>>>  from ganeti import serializer
>>>  from ganeti import constants
>>> @@ -636,12 +637,13 @@ class TestKvmCpuPinning(testutils.GanetiTestCase):
>>>  cpu_mask = self.params['cpu_mask']
>>>  worker_cpu_mask = self.params['worker_cpu_mask']
>>>  hypervisor = hv_kvm.KVMHypervisor()
>>> -with mock.patch('psutil.Process', return_value=mock_process):
>>> +with nested(mock.patch('psutil.Process', return_value=mock_process),
>>> +mock.patch('psutil.cpu_count', return_value=1237)):
>>>
>>
>> 'nested' is deprecated in Python 2.7 but unfortunately 2.6 does not
>> support comma separated contexts as 2.7 does.
>>
>
> Yep, saw that. However, nested still works (no warnings) in 2.6 as well,
> so it should be good for now.
>
>
>> All Debian versions newer than Squeeze should have 2.7 already. I believe
>> once master goes stable, we can safely drop support for Python <2.7, what
>>  do you think?
>>
>
> It is possible, but if we want to do that, I think we should do it across
> the board. E.g. on Haskell side we have a number of workaround for old
> compilers/library versions, and we could do some cleanup if we remove that,
> but we don't need to hurry.
>

> So declaring that some version (e.g. 2.19, or 2.20 - that is a round
> number) is only supported on (Jessie+, or 2015+, or something) would be a
> good move.
>

I tend see too much backwards compatibility more as a burden than a
feature, but agree that we should do declare/decide about it at release
time, not in a small patch.
LGTM.

thanks for your patches,
viktor


>
> thanks,
> iustin
>
>hypervisor._ExecuteCpuAffinity('test_instance', cpu_mask,
>>> worker_cpu_mask)
>>>
>>>  self.assertEqual(mock_process.set_cpu_affinity.call_count, 1)
>>>  self.assertEqual(mock_process.set_cpu_affinity.call_args_list[0],
>>> - mock.call(range(0,12)))
>>> + mock.call(range(0,1237)))
>>>
>>>def testCpuPinningPerVcpu(self):
>>>  mock_process = mock.MagicMock()
>>> @@ -659,11 +661,12 @@ class TestKvmCpuPinning(testutils.GanetiTestCase):
>>>  def get_mock_process(unused_pid):
>>>return mock_process
>>>
>>> -with mock.patch('psutil.Process', get_mock_process):
>>> +with nested(mock.patch('psutil.Process', get_mock_process),
>>> +mock.patch('psutil.cpu_count', return_value=1237)):
>>>hypervisor._ExecuteCpuAffinity('test_instance', cpu_mask,
>>> worker_cpu_mask)
>>>self.assertEqual(mock_process.set_cpu_affinity.call_count, 7)
>>>self.assertEqual(mock_process.set_cpu_affinity.call_args_list[0],
>>> -   mock.call(range(0,12)))
>>> +   mock.call(range(0,1237)))
>>>self.assertEqual(mock_process.set_cpu_affinity.call_args_list[6],
>>> mock.call([15, 16, 17]))
>>>
>>> --
>>> 2.8.1
>>>
>>>
>>
>

Re: [PATCH master 2/3] Don't use inexistent mock.assert_called_once()

['Viktor Bachraty' via ganeti-devel]

LGTM, thanks for catching this bug!

On Thu, Jun 9, 2016 at 11:33 PM, Iustin Pop  wrote:

> From: Iustin Pop 
>
> Old versions of mock were not strict, thus allowing to call any method on
> mocked objects, without complaining. More recent mock versions are
> stricter,
> and have uncovered this latent testing bug: assert_called_once() doesn't
> exist,
> only assert_called_once_with(…). Fixup the code to call that instead.
>
> A funny and ranty explanation of this is at
>
> http://engineeringblog.yelp.com/2015/02/assert_called_once-threat-or-menace.html
> .
>
> Signed-off-by: Iustin Pop 
> ---
>  test/py/cmdlib/test_unittest.py| 2 +-
>  test/py/ganeti.utils.retry_unittest.py | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/test/py/cmdlib/test_unittest.py
> b/test/py/cmdlib/test_unittest.py
> index f93f99d..16cc092 100644
> --- a/test/py/cmdlib/test_unittest.py
> +++ b/test/py/cmdlib/test_unittest.py
> @@ -85,7 +85,7 @@ class TestLUTestDelay(CmdlibTestCase):
>
>  self.ExecOpCode(op)
>
> -self.rpc.call_test_delay.assert_called_once()
> +self.rpc.call_test_delay.assert_called_once_with([self.master_uuid],
> DELAY_DURATION)
>
>def testFailingRpc(self):
>  op = opcodes.OpTestDelay(duration=DELAY_DURATION,
> diff --git a/test/py/ganeti.utils.retry_unittest.py b/test/py/
> ganeti.utils.retry_unittest.py
> index 93638cd..8a53760 100755
> --- a/test/py/ganeti.utils.retry_unittest.py
> +++ b/test/py/ganeti.utils.retry_unittest.py
> @@ -215,7 +215,7 @@ class
> TestRetryByNumberOfTimes(testutils.GanetiTestCase):
>def testSuccessOnFirst(self):
>  test_fn = mock.Mock()
>  utils.RetryByNumberOfTimes(5, 0, Exception, test_fn)
> -test_fn.assert_called_once()
> +test_fn.assert_called_once_with()
>
>def testSuccessOnFirstWithArgs(self):
>  test_fn = mock.Mock()
> --
> 2.8.1
>
>

Re: [PATCH master 1/3] Support python-mock versions later than about 1.1

['Viktor Bachraty' via ganeti-devel]

LGTM

On Thu, Jun 9, 2016 at 11:33 PM, Iustin Pop  wrote:

> From: Iustin Pop 
>
> It seems the two alternatives for python-mock's way of patching objects
> are not
> enough; more recent versions have patch.object, so let's support that as
> well.
> I think this is post 1.0.1, but not entirely sure.
>
> Additionally, move the import and check for patcher at top level, so
> they're
> not executed each time patch_object is called.
>
> Side-note: it seems there are many ways we call patch object in the code,
> some
> cleanup would be in order but that's left for another time.
>
> Signed-off-by: Iustin Pop 
> ---
>  test/py/testutils/__init__.py | 33 +++--
>  1 file changed, 19 insertions(+), 14 deletions(-)
>
> diff --git a/test/py/testutils/__init__.py b/test/py/testutils/__init__.py
> index 27ca425..f2e63b5 100644
> --- a/test/py/testutils/__init__.py
> +++ b/test/py/testutils/__init__.py
> @@ -37,6 +37,23 @@ import tempfile
>  import unittest
>  import logging
>
> +# Unified patch_object for various versions of Python Mock.
> +#
> +# Different Python Mock versions provide incompatible versions of
> patching an
> +# object. More recent versions use _patch_object, older ones used
> patch_object.
> +# This unifies the different variations.
> +import mock
> +
> +try:
> +  # pylint: disable=W0212
> +  _patcher = mock._patch_object
> +except AttributeError:
> +  # pylint: disable=E1101
> +  try:
> +_patcher = mock.patch_object
> +  except AttributeError:
> +_patcher = mock.patch.object
> +
>  from ganeti import utils
>
>
> @@ -235,20 +252,8 @@ class GanetiTestCase(unittest.TestCase):
>
>
>  def patch_object(*args, **kwargs):
> -  """Unified patch_object for various versions of Python Mock.
> -
> -  Different Python Mock versions provide incompatible versions of
> patching an
> -  object. More recent versions use _patch_object, older ones used
> patch_object.
> -  This function unifies the different variations.
> -
> -  """
> -  import mock
> -  try:
> -# pylint: disable=W0212
> -return mock._patch_object(*args, **kwargs)
> -  except AttributeError:
> -# pylint: disable=E1101
> -return mock.patch_object(*args, **kwargs)
> +  """Unified patch_object for various versions of Python Mock."""
> +  return _patcher(*args, **kwargs)
>
>
>  def UnifyValueType(data):
> --
> 2.8.1
>
>

Re: [PATCH master 3/3] Fix KVM pinning tests to not depend on the local machine

['Iustin Pop' via ganeti-devel]

2016-06-10 10:58 GMT+02:00 Viktor Bachraty :

>
> On Thu, Jun 9, 2016 at 11:33 PM, Iustin Pop  wrote:
>
>> From: Iustin Pop 
>>
>> Commit 8b2ec2f added unittests for KVM pinning, but it introduced a
>> non-obvious
>> local dependency in the tests: the CPU_PINNING_OFF calls work by looking
>> at the
>> (current) machine's core count, and pinning to all those CPUs. In order
>> to make
>> this work independently from the test machine, we must also mock the
>> result of
>> process.cpu_count(). Do this by using a core count that is very much
>> unlikely
>> to ever be present in the real world.
>>
>> Signed-off-by: Iustin Pop 
>> ---
>>  test/py/ganeti.hypervisor.hv_kvm_unittest.py | 11 +++
>>  1 file changed, 7 insertions(+), 4 deletions(-)
>>
>> diff --git a/test/py/ganeti.hypervisor.hv_kvm_unittest.py b/test/py/
>> ganeti.hypervisor.hv_kvm_unittest.py
>> index c7a53b5..55ffb9b 100755
>> --- a/test/py/ganeti.hypervisor.hv_kvm_unittest.py
>> +++ b/test/py/ganeti.hypervisor.hv_kvm_unittest.py
>> @@ -37,6 +37,7 @@ import socket
>>  import os
>>  import struct
>>  import re
>> +from contextlib import nested
>>
>>  from ganeti import serializer
>>  from ganeti import constants
>> @@ -636,12 +637,13 @@ class TestKvmCpuPinning(testutils.GanetiTestCase):
>>  cpu_mask = self.params['cpu_mask']
>>  worker_cpu_mask = self.params['worker_cpu_mask']
>>  hypervisor = hv_kvm.KVMHypervisor()
>> -with mock.patch('psutil.Process', return_value=mock_process):
>> +with nested(mock.patch('psutil.Process', return_value=mock_process),
>> +mock.patch('psutil.cpu_count', return_value=1237)):
>>
>
> 'nested' is deprecated in Python 2.7 but unfortunately 2.6 does not
> support comma separated contexts as 2.7 does.
>

Yep, saw that. However, nested still works (no warnings) in 2.6 as well, so
it should be good for now.


> All Debian versions newer than Squeeze should have 2.7 already. I believe
> once master goes stable, we can safely drop support for Python <2.7, what
>  do you think?
>

It is possible, but if we want to do that, I think we should do it across
the board. E.g. on Haskell side we have a number of workaround for old
compilers/library versions, and we could do some cleanup if we remove that,
but we don't need to hurry.

So declaring that some version (e.g. 2.19, or 2.20 - that is a round
number) is only supported on (Jessie+, or 2015+, or something) would be a
good move.

thanks,
iustin

   hypervisor._ExecuteCpuAffinity('test_instance', cpu_mask,
>> worker_cpu_mask)
>>
>>  self.assertEqual(mock_process.set_cpu_affinity.call_count, 1)
>>  self.assertEqual(mock_process.set_cpu_affinity.call_args_list[0],
>> - mock.call(range(0,12)))
>> + mock.call(range(0,1237)))
>>
>>def testCpuPinningPerVcpu(self):
>>  mock_process = mock.MagicMock()
>> @@ -659,11 +661,12 @@ class TestKvmCpuPinning(testutils.GanetiTestCase):
>>  def get_mock_process(unused_pid):
>>return mock_process
>>
>> -with mock.patch('psutil.Process', get_mock_process):
>> +with nested(mock.patch('psutil.Process', get_mock_process),
>> +mock.patch('psutil.cpu_count', return_value=1237)):
>>hypervisor._ExecuteCpuAffinity('test_instance', cpu_mask,
>> worker_cpu_mask)
>>self.assertEqual(mock_process.set_cpu_affinity.call_count, 7)
>>self.assertEqual(mock_process.set_cpu_affinity.call_args_list[0],
>> -   mock.call(range(0,12)))
>> +   mock.call(range(0,1237)))
>>self.assertEqual(mock_process.set_cpu_affinity.call_args_list[6],
>> mock.call([15, 16, 17]))
>>
>> --
>> 2.8.1
>>
>>
>

Re: [PATCH master 3/3] Fix KVM pinning tests to not depend on the local machine

['Viktor Bachraty' via ganeti-devel]

On Thu, Jun 9, 2016 at 11:33 PM, Iustin Pop  wrote:

> From: Iustin Pop 
>
> Commit 8b2ec2f added unittests for KVM pinning, but it introduced a
> non-obvious
> local dependency in the tests: the CPU_PINNING_OFF calls work by looking
> at the
> (current) machine's core count, and pinning to all those CPUs. In order to
> make
> this work independently from the test machine, we must also mock the
> result of
> process.cpu_count(). Do this by using a core count that is very much
> unlikely
> to ever be present in the real world.
>
> Signed-off-by: Iustin Pop 
> ---
>  test/py/ganeti.hypervisor.hv_kvm_unittest.py | 11 +++
>  1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/test/py/ganeti.hypervisor.hv_kvm_unittest.py b/test/py/
> ganeti.hypervisor.hv_kvm_unittest.py
> index c7a53b5..55ffb9b 100755
> --- a/test/py/ganeti.hypervisor.hv_kvm_unittest.py
> +++ b/test/py/ganeti.hypervisor.hv_kvm_unittest.py
> @@ -37,6 +37,7 @@ import socket
>  import os
>  import struct
>  import re
> +from contextlib import nested
>
>  from ganeti import serializer
>  from ganeti import constants
> @@ -636,12 +637,13 @@ class TestKvmCpuPinning(testutils.GanetiTestCase):
>  cpu_mask = self.params['cpu_mask']
>  worker_cpu_mask = self.params['worker_cpu_mask']
>  hypervisor = hv_kvm.KVMHypervisor()
> -with mock.patch('psutil.Process', return_value=mock_process):
> +with nested(mock.patch('psutil.Process', return_value=mock_process),
> +mock.patch('psutil.cpu_count', return_value=1237)):
>

'nested' is deprecated in Python 2.7 but unfortunately 2.6 does not support
comma separated contexts as 2.7 does. All Debian versions newer than
Squeeze should have 2.7 already. I believe once master goes stable, we can
safely drop support for Python <2.7, what  do you think?

   hypervisor._ExecuteCpuAffinity('test_instance', cpu_mask,
> worker_cpu_mask)
>
>  self.assertEqual(mock_process.set_cpu_affinity.call_count, 1)
>  self.assertEqual(mock_process.set_cpu_affinity.call_args_list[0],
> - mock.call(range(0,12)))
> + mock.call(range(0,1237)))
>
>def testCpuPinningPerVcpu(self):
>  mock_process = mock.MagicMock()
> @@ -659,11 +661,12 @@ class TestKvmCpuPinning(testutils.GanetiTestCase):
>  def get_mock_process(unused_pid):
>return mock_process
>
> -with mock.patch('psutil.Process', get_mock_process):
> +with nested(mock.patch('psutil.Process', get_mock_process),
> +mock.patch('psutil.cpu_count', return_value=1237)):
>hypervisor._ExecuteCpuAffinity('test_instance', cpu_mask,
> worker_cpu_mask)
>self.assertEqual(mock_process.set_cpu_affinity.call_count, 7)
>self.assertEqual(mock_process.set_cpu_affinity.call_args_list[0],
> -   mock.call(range(0,12)))
> +   mock.call(range(0,1237)))
>self.assertEqual(mock_process.set_cpu_affinity.call_args_list[6],
> mock.call([15, 16, 17]))
>
> --
> 2.8.1
>
>