Re: On string data types

['Iustin Pop' via ganeti-devel]

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

> Hi Iustin,
>
> I'm half inclined to think that if we're going to change the type of UUID
> we should do it 'properly', ie change to a strict, unpacked 16-byte array
> that implements Eq, Show, Read (and maybe Hashable too) and use it
> consistently everywhere.
>

16-byte - does that mean you favour Data.Text over ByteString? I was
thinking that way as well (due to ease of using one underlying type
throughout), but we will pay 2N compared to bytes on disk. And at least for
UUIDs, we don't do any string-related processing on them (substrings,
casing, etc.) so we could save some memory there.

On the other hand, Data.ByteString.Short is a newer addition, so using it
might have some small logistical drawbacks, plus that (very rarely) we do
have conversions.

Hmm…

I'd also be tempted to introduce a type that lets us distinguish between
> UUIDs for different Ganeti object types, eg Disks, Nodes, Instances, NICs
> etc.
>

Interesting. On one hand, they are the same thing (UUIDs), and we do
process them together at one point (although badly - in getAllIDs). On the
other hand, they do refer to different things :)

Were you thinking of documentation only (type aliases), or hard separation
(newtypes)?

Efficiency-wise, yeah, we do have the problem where we don't have a suite
> of microbenchmarks at the moment, and we don't really know which functions
> are critical to overall performance. As you say, it's entirely possible
> that improving performance in one area might just cause problems somewhere
> else.
>
> Our testing approach at the moment is to take a relatively heavy workload
> -- a parallel 4 node evacuate on a 80 node/1200 instance cluster -- and to
> see what problems that exposes, but that's far from perfect.
>
> For what it's worth, I found that getNodeInstances shows up high in the
> profile when doing a RAPI /2/nodes requests, and it would also matter for
>  ReqNodeInstances queries to confd. Otherwise it doesn't seem to be
> something that's heavily used.
>

Ack to the above.

I think I agree with you: we shouldn't make any large-scale changes on
> anything but master because there's too much potential to introduce
> performance regressions and we don't have a good way to measure them.
> Instead, we're focusing on point fixes, and trying to get 2.16 released,
> since it has a bunch of random efficiency improvements relative to 2.15,
> improves KVM support, and allows configurable SSH key types/sizes.
>

Cool. Let's see where the discussion regarding Data.Text goes (anybody else
wants to chime in), and we can proceed (in master) along those lines.

thanks for the feedback!
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

[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

['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

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

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: [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

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: [PATCH stable-2.16 1/2] Tune getNodeInstances DRBD secondary computation

['Iustin Pop' via ganeti-devel]

2016-05-30 11:34 GMT+02:00 Brian Foley :

> On Sun, May 29, 2016 at 12:01:39PM +0200, Iustin Pop wrote:
> > On Thu, May 26, 2016 at 03:56:10PM +0100, Ganeti Development List wrote:
> > > This is exercised by the luxi QueryInstances call when the sinst_cnt
> and
> > > sint_list fields are used. This uses a lot of CPU and does a lot of
> > > short-lived heap allocation on clusters with many instances.
> > >
> > > The reimplementation allocates fewer temporary values, and does fewer
> > > object lookups by UUID. The net effect is to reduce heap use from
> ~3.2GB
> > > to ~1.5GB, and CPU use from ~1200ms to ~770ms in a test harness using
> > > a config with 1000 DRBD instances on 80 nodes.
> > >
> > > Signed-off-by: Brian Foley 
> >
> > Hi Brian,
> >
> > > +-- | Computes the secondary node UUID for a DRBD disk
> > > +computeDiskSecondaryNode :: Disk -> Maybe String
> > > +computeDiskSecondaryNode dsk =
> > > +  case diskLogicalId dsk of
> > > +Just (LIDDrbd8 _nodeA nodeB _ _ _ _) -> Just nodeB
> >
> > Are you sure this snippet is correct? If I remember correctly and things
> > haven't changed, disks do not record primary/secondary roles, they only
> > hold the nodes. I.e. nodeA ≠ primary, it's simple one of the nodes of
> > the disk, similary with nodeB. Hence the naming A/B, instead of Pri/Sec.
>
> Ah blast. No, I'm not sure. When diagnosing DRBD issues on live clusters
> nodeA always seemed to be the primary, so I assumed that this invariant
> was enforced by the code. I'll submit a patch to fix this shortly.
>

nodeA is the primary at the moment of instance creation; afterwards, live
migration/failover and change secondary can change this, but it's 50% which
is which.

thanks!
iustin

Re: [PATCH instance-debootstrap 0/4] Cleanups and simple tests

['Iustin Pop' via ganeti-devel]

2016-05-30 11:51 GMT+02:00 Brian Foley :

> On Sun, May 29, 2016 at 02:38:22AM +0200, Iustin Pop wrote:
> > From: Iustin Pop 
> >
> > A few cleanups, and adding a test suite to be run without Ganeti, i.e. on
> > standalone systems. However it has downsides, see that patch's
> description.
> >
> > Note: I haven't tested this patch series under Ganeti itself, but the
> only
> > non-test modification is the suite change (wheezy→jessie), so this
> should work
> > as before.
> >
> > If these are LGTM'ed and pushed, I'll followup with release patch (0.16).
> >
> > Iustin Pop (4):
> >   Don't mark common.sh as executable
> >   Check for required programs at configure time
> >   Switch default suite to jessie and bump min disk size
> >   Add a test for exercising the scripts
>
> LGTM. I'll fix the nit and push the patchset.
>

thanks!
iustin

UUID references (was: Tune getNodeInstances DRBD secondary) computation

['Iustin Pop' via ganeti-devel]

On Thu, May 26, 2016 at 03:56:10PM +0100, Ganeti Development List wrote:
> This is exercised by the luxi QueryInstances call when the sinst_cnt and
> sint_list fields are used. This uses a lot of CPU and does a lot of
> short-lived heap allocation on clusters with many instances.
> 
> The reimplementation allocates fewer temporary values, and does fewer
> object lookups by UUID. The net effect is to reduce heap use from ~3.2GB
> to ~1.5GB, and CPU use from ~1200ms to ~770ms in a test harness using
> a config with 1000 DRBD instances on 80 nodes.

Hi Brian & all,

I was surprised to see that much heap use and wall time for this code,
so I looked at this yesterday (thanks for the config file/test
hardness!).

Profiling shows that overall, the allocation for this test harness is
split half-half between config loading (which is a separate issue) and
the UTF8.fromString calls for converting between the disk IDs (stored as
String in the instance) and the keys for configDisks (which are
ByteStrings).

Writing a simple 20-line hack to change the instance disks to be
ByteStrings shows that the runtime of just "map (snd . getNodeInstances
cfg) all_nodes" goes from (on my machine) 200ms to ~45 ms., i.e. a 5×
decrease in runtime, with the getNodeInstances doing very few
allocations.

Is there a reason not to store UUIDs everywhere as ByteStrings? My
quick and dirty patch seems to pass all unittests.

Or alternatively: is anyone already working on making UUIDs uniform? If
not, I'll work on making my patch clean and ready for submit.

thanks,
iustin

Re: UUID references (was: Tune getNodeInstances DRBD secondary) computation

['Iustin Pop' via ganeti-devel]

On Mon, May 30, 2016 at 11:54:45AM +0100, Viktor Bachraty wrote:
> Hi Iustin,
> 
> Thanks for helping out with the optimizations. AFAIK, Klaus had a plan to
> introduce a type for UUIDs (that could have been backed by a ByteString)
> instead of using just a String before the project was handed over to us.

OK, good to have confirmation that this was planned, it means there are
no obvious known issues with it.

> If
> this could be done in a not too invasive change and it would reduce
> allocations as well, that would be awesome.

Cool, will do then. I'll see how invasive it gets and I'll send it
against stable-2.16 (if not too invasive) or master otherwise.

iustin

> On Mon, May 30, 2016 at 11:40 AM, 'Iustin Pop' via ganeti-devel <
> ganeti-devel@googlegroups.com> wrote:
> 
> > On Thu, May 26, 2016 at 03:56:10PM +0100, Ganeti Development List wrote:
> > > This is exercised by the luxi QueryInstances call when the sinst_cnt and
> > > sint_list fields are used. This uses a lot of CPU and does a lot of
> > > short-lived heap allocation on clusters with many instances.
> > >
> > > The reimplementation allocates fewer temporary values, and does fewer
> > > object lookups by UUID. The net effect is to reduce heap use from ~3.2GB
> > > to ~1.5GB, and CPU use from ~1200ms to ~770ms in a test harness using
> > > a config with 1000 DRBD instances on 80 nodes.
> >
> > Hi Brian & all,
> >
> > I was surprised to see that much heap use and wall time for this code,
> > so I looked at this yesterday (thanks for the config file/test
> > hardness!).
> >
> > Profiling shows that overall, the allocation for this test harness is
> > split half-half between config loading (which is a separate issue) and
> > the UTF8.fromString calls for converting between the disk IDs (stored as
> > String in the instance) and the keys for configDisks (which are
> > ByteStrings).
> >
> > Writing a simple 20-line hack to change the instance disks to be
> > ByteStrings shows that the runtime of just "map (snd . getNodeInstances
> > cfg) all_nodes" goes from (on my machine) 200ms to ~45 ms., i.e. a 5×
> > decrease in runtime, with the getNodeInstances doing very few
> > allocations.
> >
> > Is there a reason not to store UUIDs everywhere as ByteStrings? My
> > quick and dirty patch seems to pass all unittests.
> >
> > Or alternatively: is anyone already working on making UUIDs uniform? If
> > not, I'll work on making my patch clean and ready for submit.
> >
> > thanks,
> > iustin
> >

Re: UUID references (was: Tune getNodeInstances DRBD secondary) computation

['Iustin Pop' via ganeti-devel]

On Mon, May 30, 2016 at 12:18:37PM +0100, Brian Foley wrote:
> On Mon, May 30, 2016 at 12:40:49PM +0200, Iustin Pop wrote:
> > On Thu, May 26, 2016 at 03:56:10PM +0100, Ganeti Development List wrote:
> > > This is exercised by the luxi QueryInstances call when the sinst_cnt and
> > > sint_list fields are used. This uses a lot of CPU and does a lot of
> > > short-lived heap allocation on clusters with many instances.
> > > 
> > > The reimplementation allocates fewer temporary values, and does fewer
> > > object lookups by UUID. The net effect is to reduce heap use from ~3.2GB
> > > to ~1.5GB, and CPU use from ~1200ms to ~770ms in a test harness using
> > > a config with 1000 DRBD instances on 80 nodes.
> > 
> > Hi Brian & all,
> > 
> > I was surprised to see that much heap use and wall time for this code,
> > so I looked at this yesterday (thanks for the config file/test
> > hardness!).
> > 
> > Profiling shows that overall, the allocation for this test harness is
> > split half-half between config loading (which is a separate issue) and
> > the UTF8.fromString calls for converting between the disk IDs (stored as
> > String in the instance) and the keys for configDisks (which are
> > ByteStrings).
> >
> > Writing a simple 20-line hack to change the instance disks to be
> > ByteStrings shows that the runtime of just "map (snd . getNodeInstances
> > cfg) all_nodes" goes from (on my machine) 200ms to ~45 ms., i.e. a 5×
> > decrease in runtime, with the getNodeInstances doing very few
> > allocations.
> >
> > Is there a reason not to store UUIDs everywhere as ByteStrings? My
> > quick and dirty patch seems to pass all unittests.
> 
> That's a huge improvement, and it would be great if you could get it to
> work. The problem I had is that there's a lot of places that use
> getItem'/getItem, and these currently require a String arg (because they
> can take a name, a name prefix, or a UUID), so you end up doing a lot of
> conversions back from ByteString to String. :(

Yes, saw that, and from a cursory look I believe it can be worked
around. Testing overall performance might be difficult though. Are there
nowadays large-scale performance tests? Or just individual benchmarks?

> Note also that there's a caveat with ByteStrings: they use pinned memory
> and therefore can cause lots of heap fragmentation, and I suspect this
> might be an issue in our daemons.
> 
> Data.ByteString.Short is more compact and doesn't used pinned memory. I
> wonder if this could be used instead.

That's interesting, I didn't know that. Although many of the Container
types (or all?) already use ByteString as keys, so I suspect there are
already problems with it.

OK, will look into this, and see what I can come up with.

thanks!
iustin

Re: [PATCH stable-2.16 1/3] Fix optimisation: Correctly extract secondary node.

['Iustin Pop' via ganeti-devel]

On Mon, May 30, 2016 at 03:47:40PM +0100, Brian Foley wrote:
> On Mon, May 30, 2016 at 03:33:50PM +0200, Iustin Pop wrote:
> > On Mon, May 30, 2016 at 02:10:14PM +0100, Ganeti Development List wrote:
> > > It is possible for either nodeA or nodeB in a DRBD disk to be the
> > > primary or the secondary. Patch 9825767ad incorrectly assumed nodeB
> > > was the secondary.  Correct this by passing the actual primary to
> > > computeDiskSecondaryNode so it can decide.
> > > 
> > >  -- | Get instances of a given node.
> > > @@ -184,9 +184,10 @@ getNodeInstances cfg nname =
> > >  
> > >  sec_insts :: [Instance]
> > >  sec_insts = [inst |
> > > -(inst, disks) <- inst_disks,
> > > -s_uuid <- mapMaybe computeDiskSecondaryNode disks,
> > > -s_uuid == nname]
> > > +  (inst, disks) <- inst_disks,
> > > +  s_uuid <- mapMaybe (\d -> maybe Nothing
> > > +(computeDiskSecondaryNode d) $ instPrimaryNode inst) disks,
> > 
> > Style comment: I had to read twice here, as the use of 'maybe' is a bit
> > strange; it looks like you use maybe to deal with the fact that
> > 'instPrimaryNode inst' is a Maybe type, and then applying
> > computeDiskSecondaryNode on top of it to return again a Maybe type; i.e.
> > you don't change the types, just do some post-processing on the result
> > of instPrimaryNode.
> > 
> > This to me sounds like a Functor application (not tested):
> > 
> >   s_uuid <- mapMaybe (\d -> computeDiskSecondaryNode d `fmap` 
> > instPrimaryNode inst) disks
> 
> Not quite. That functor application would return a Maybe Maybe String, and
> mapMaybe would squash [Maybe Maybe String] down to [Maybe String].
> 
> I think what I wanted without realising it was monadic bind:
> mapMaybe (\d -> (instPrimaryNode inst) >>= (computeDiskSecondaryNode d)) disks

Ah, you're right! Sorry :)

iustin

Re: [PATCH stable-2.16 1/3] Fix optimisation: Correctly extract secondary node.

['Iustin Pop' via ganeti-devel]

On Mon, May 30, 2016 at 02:10:14PM +0100, Ganeti Development List wrote:
> It is possible for either nodeA or nodeB in a DRBD disk to be the
> primary or the secondary. Patch 9825767ad incorrectly assumed nodeB
> was the secondary.  Correct this by passing the actual primary to
> computeDiskSecondaryNode so it can decide.
> 
> Signed-off-by: Brian Foley 
> ---
>  src/Ganeti/Config.hs | 13 +++--
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/src/Ganeti/Config.hs b/src/Ganeti/Config.hs
> index 5b40694..5ada94b 100644
> --- a/src/Ganeti/Config.hs
> +++ b/src/Ganeti/Config.hs
> @@ -159,10 +159,10 @@ instNodes cfg inst = maybe id S.insert (instPrimaryNode 
> inst)
>$ instDiskNodes cfg inst
>  
>  -- | Computes the secondary node UUID for a DRBD disk
> -computeDiskSecondaryNode :: Disk -> Maybe String
> -computeDiskSecondaryNode dsk =
> +computeDiskSecondaryNode :: Disk -> String -> Maybe String
> +computeDiskSecondaryNode dsk primary =
>case diskLogicalId dsk of
> -Just (LIDDrbd8 _nodeA nodeB _ _ _ _) -> Just nodeB
> +Just (LIDDrbd8 a b _ _ _ _) -> Just $ if primary == a then b else a
>  _ -> Nothing
>  
>  -- | Get instances of a given node.
> @@ -184,9 +184,10 @@ getNodeInstances cfg nname =
>  
>  sec_insts :: [Instance]
>  sec_insts = [inst |
> -(inst, disks) <- inst_disks,
> -s_uuid <- mapMaybe computeDiskSecondaryNode disks,
> -s_uuid == nname]
> +  (inst, disks) <- inst_disks,
> +  s_uuid <- mapMaybe (\d -> maybe Nothing
> +(computeDiskSecondaryNode d) $ instPrimaryNode inst) disks,

Style comment: I had to read twice here, as the use of 'maybe' is a bit
strange; it looks like you use maybe to deal with the fact that
'instPrimaryNode inst' is a Maybe type, and then applying
computeDiskSecondaryNode on top of it to return again a Maybe type; i.e.
you don't change the types, just do some post-processing on the result
of instPrimaryNode.

This to me sounds like a Functor application (not tested):

  s_uuid <- mapMaybe (\d -> computeDiskSecondaryNode d `fmap` instPrimaryNode 
inst) disks

LGTM either way, as the code does the same thing.

thanks,
iustin

Re: [PATCH stable-2.16 1/2] Tune getNodeInstances DRBD secondary computation

['Iustin Pop' via ganeti-devel]

On Thu, May 26, 2016 at 03:56:10PM +0100, Ganeti Development List wrote:
> This is exercised by the luxi QueryInstances call when the sinst_cnt and
> sint_list fields are used. This uses a lot of CPU and does a lot of
> short-lived heap allocation on clusters with many instances.
> 
> The reimplementation allocates fewer temporary values, and does fewer
> object lookups by UUID. The net effect is to reduce heap use from ~3.2GB
> to ~1.5GB, and CPU use from ~1200ms to ~770ms in a test harness using
> a config with 1000 DRBD instances on 80 nodes.
> 
> Signed-off-by: Brian Foley 

Hi Brian,

> +-- | Computes the secondary node UUID for a DRBD disk
> +computeDiskSecondaryNode :: Disk -> Maybe String
> +computeDiskSecondaryNode dsk =
> +  case diskLogicalId dsk of
> +Just (LIDDrbd8 _nodeA nodeB _ _ _ _) -> Just nodeB

Are you sure this snippet is correct? If I remember correctly and things
haven't changed, disks do not record primary/secondary roles, they only
hold the nodes. I.e. nodeA ≠ primary, it's simple one of the nodes of
the disk, similary with nodeB. Hence the naming A/B, instead of Pri/Sec.

Only the instance knows its current primary node, and thus you need that
to be able to determine which is the disk's secondary node.

regards,
iustin

[PATCH instance-debootstrap] Fix sfdisk invocation for util-linux 2.26+

['Iustin Pop' via ganeti-devel]

sfdisk changed in 2.26; it stopped supporting the geometry options (-C, -H,
-S), deprecated -L and -u. However, we can't just drop these options, as
pre-2.26 needs them for devices that don't report geometry (e.g. plain files).

This patch changes the invocation of sfdisk based on what the `--help' output
contains, as parsing the version number seems a bit more involved. Tested
manually (not from Ganeti) with both 2.20, 2.25 and 2.28. The resulting
partitioned disks have similarly-sized partitions, but the geometry information
is different (as expected).

This was reported in, and will fix, Ubuntu Launchpad bug LP#1577346.

Signed-off-by: Iustin Pop 
---
 common.sh.in | 13 -
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/common.sh.in b/common.sh.in
index dd786af..bd7b260 100644
--- a/common.sh.in
+++ b/common.sh.in
@@ -87,11 +87,14 @@ get_api10_arguments() {
 }
 
 format_disk0() {
-  # Create one big partition, and make it bootable
-  # some versions of sfdisk need manual specification of
-  # head/sectors for devices such as drbd which don't
-  # report geometry
-  sfdisk -H 64 -S 32 -u S --quiet --Linux "$1" <

Re: [PATCH instance-debootstrap] Fix sfdisk invocation for util-linux 2.26+

['Iustin Pop' via ganeti-devel]

2016-05-26 12:37 GMT+02:00 Brian Foley <bpfo...@google.com>:

> On Thu, May 26, 2016 at 12:26:28PM +0200, 'Iustin Pop' via ganeti-devel
> wrote:
> > sfdisk changed in 2.26; it stopped supporting the geometry options (-C,
> -H,
> > -S), deprecated -L and -u. However, we can't just drop these options, as
> > pre-2.26 needs them for devices that don't report geometry (e.g. plain
> files).
> >
> > This patch changes the invocation of sfdisk based on what the `--help'
> output
> > contains, as parsing the version number seems a bit more involved. Tested
> > manually (not from Ganeti) with both 2.20, 2.25 and 2.28. The resulting
> > partitioned disks have similarly-sized partitions, but the geometry
> information
> > is different (as expected).
> >
> > This was reported in, and will fix, Ubuntu Launchpad bug LP#1577346.
> >
> > Signed-off-by: Iustin Pop <ius...@google.com>
>
> LGTM. Iustin, do you still have push privileges, or would you like me to
> do it?
>

I don't anymore, so I please push it. I saw another small issue regarding
dependencies, so I'll send another patch in the next days, after which a
new release would be good.

thanks!
iustin

Re: [PATCH stable-2.15 1/2] KVM: handle gracefully too old psutil versions

['Iustin Pop' via ganeti-devel]

On 21 June 2016 at 16:28, Brian Foley  wrote:

> On Sat, Jun 18, 2016 at 04:53:25AM +0200, Iustin Pop wrote:
> > On 2016-06-15 10:23:57, Brian Foley wrote:
> > > Additionally, 0.5.0 had a psutil.Process.{get,set}_cpu_affinity() API,
> > > which we use in the kvm code. In 2.0.0 API was renamed to
> cpu_affinity()
> > > and the old API deprecated, and in 3.0 the old {get,set} API was
> removed.
> > >
> > > So we currently need to restrict psutils to 2.x, but maybe it's just
> > > easier to add an __attr__ check for the new affinity API, but perhaps
> > > this should be done in a separate patch.
> >
> > Just to see if I understand: we don't support 3.x+ due to the
> > cpu_affinity API only?
>
> I think so yes. That's the only thing I saw when I looked through the
> psutil
> version history a few months ago.
>
> Thinking a little more about what I wrote above, since we require psutil
> >= 2.0
> for kvm anyway, then it would be perfectly fine to replace the use of the
> 0.5.0
> affinity API in the kvm code with the >= 2.0 API. I don't know if this on
> its
> own would be sufficient to get it to work with psutil >= 3.0, but it would
> be
> a step in the right direction. We can look at this another time though.
>

Yes, that's why I asked—it was kind of surprising to see the use of 0.5.0
API but not actually supporting 0.5.0 :)

thanks,
iustin

Re: [PATCH stable-2.15] KVM: handle gracefully too old/too new psutil versions

['Iustin Pop' via ganeti-devel]

On 21 June 2016 at 16:29, Brian Foley  wrote:

> On Sat, Jun 18, 2016 at 04:54:51AM +0200, Iustin Pop wrote:
> > From: Iustin Pop 
> >
> > My previous pylint cleanups were done without psutil installed; as soon
> > I installed it, pylint showed that the wheezy's version of psutil is too
> > old (0.5.1), not having the cpu_count() function which was introduced in
> > 2.0.0. Furthermore, thanks to Brian, it turns out that too new versions
> > are also unsupported.
> >
> > This change adds a simple way to detect this and to degrade gracefully
> > by throwing an appropriate exception instead of an unclear one at
> > runtime. Tested with wheezy's 0.5.0 and sid's 4.1.1 versions. It also
> > updates the INSTALL file to note the supported versions and that
> > [2.0.0,2.2.0) had issues with FH leaks.
> >
> > Signed-off-by: Iustin Pop 
>
> LGTM, and sorry for the delay in reviewing.
>

No worries, thanks!

Re: [PATCH stable-2.17] Prevent watcher from holding lock on verify group

['Iustin Pop' via ganeti-devel]

On 17 June 2016 at 10:36, Federico Pareschi  wrote:

> Yes, this is definitely the case.
> We have ideas to implement something in the future to obviate this
> problem, although we're still considering exactly how to implement it.
> This is a short-term quick fix to solve some blocker issues that show up
> as a consequence of it.
>

Ack, thanks for the info!

iustin


> On 17 June 2016 at 17:57, Iustin Pop  wrote:
>
>> 2016-06-17 9:46 GMT-07:00 Federico Pareschi :
>>
>>> When a ganeti-watcher runs on the nodegroup, it submits the verify-group
>>> job. If there is another job in the queue that is taking some locks that
>>> stop the verify-group job (like an instance creation) then the whole
>>> ganeti-watcher is blocked and has to wait for that job to finish.
>>>
>>> We have a case where we need the ganeti-watcher's periodic check to
>>> restart some downed instances, but the ganeti-watcher itself gets stuck on
>>> some other jobs and those downed instances don't get brought back up on
>>> time (And this causes problems to us).
>>>
>>> There really is no reason to hold a lock to the watcher state file when
>>> submitting the verify disk job anyway.
>>>
>>
>> All this makes sense. My question is rather, if we end up with multiple
>> watchers basically running (waiting) concurrently, can we end up with
>> multiple (redundant) verify-group jobs?
>>
>> Sorry if I misunderstand the situation.
>>
>> iustin
>>
>> On 17 June 2016 at 17:18, Iustin Pop  wrote:
>>>
 2016-06-17 8:31 GMT-07:00 'Federico Morg Pareschi' via ganeti-devel <
 ganeti-devel@googlegroups.com>:

> The ganeti-watcher holds the group file lock for too long, until after
> the execution of a group-verify-disk job. This locks for a long time if
> there are other jobs already running and blocking the verify from
> executing. When the lock is held, another ganeti-watcher run cannot be
> scheduled, so this prevents the ganeti-watcher from running for several
> minutes.
>
> With this commit, the lock is released before running the VerifyDisks
> operation, so even if the submitted job gets stuck in the Job Queue, a
> subsequient ganeti-watcher run would still happen.
>

 Quick question: what prevents a runaway case where VerifyDisks is
 blocking for hours and we have many watchers all running and submitting
 their own VerifyDisks?

 thanks,
 iustin

>>>
>>>
>>
>

Re: [PATCH stable-2.17] Prevent watcher from holding lock on verify group

['Iustin Pop' via ganeti-devel]

2016-06-17 9:46 GMT-07:00 Federico Pareschi :

> When a ganeti-watcher runs on the nodegroup, it submits the verify-group
> job. If there is another job in the queue that is taking some locks that
> stop the verify-group job (like an instance creation) then the whole
> ganeti-watcher is blocked and has to wait for that job to finish.
>
> We have a case where we need the ganeti-watcher's periodic check to
> restart some downed instances, but the ganeti-watcher itself gets stuck on
> some other jobs and those downed instances don't get brought back up on
> time (And this causes problems to us).
>
> There really is no reason to hold a lock to the watcher state file when
> submitting the verify disk job anyway.
>

All this makes sense. My question is rather, if we end up with multiple
watchers basically running (waiting) concurrently, can we end up with
multiple (redundant) verify-group jobs?

Sorry if I misunderstand the situation.

iustin

On 17 June 2016 at 17:18, Iustin Pop  wrote:
>
>> 2016-06-17 8:31 GMT-07:00 'Federico Morg Pareschi' via ganeti-devel <
>> ganeti-devel@googlegroups.com>:
>>
>>> The ganeti-watcher holds the group file lock for too long, until after
>>> the execution of a group-verify-disk job. This locks for a long time if
>>> there are other jobs already running and blocking the verify from
>>> executing. When the lock is held, another ganeti-watcher run cannot be
>>> scheduled, so this prevents the ganeti-watcher from running for several
>>> minutes.
>>>
>>> With this commit, the lock is released before running the VerifyDisks
>>> operation, so even if the submitted job gets stuck in the Job Queue, a
>>> subsequient ganeti-watcher run would still happen.
>>>
>>
>> Quick question: what prevents a runaway case where VerifyDisks is
>> blocking for hours and we have many watchers all running and submitting
>> their own VerifyDisks?
>>
>> thanks,
>> iustin
>>
>
>

Re: [PATCH master 0/6] New balancing options implementation

['Iustin Pop' via ganeti-devel]

On 23 June 2016 at 17:08, Oleg Ponomarev <onponoma...@gmail.com> wrote:

> Hi Iustin, Daniil,
>
> The reason for Daniil not to reply immediately is his GSoC midterm
> evaluation coming soon. As the implementation represents his work during
> the first month of GSoC, it's necessary to share it with the community at
> this point. We have discussed your comments on design document and Daniil
> took them into account. Still, I don't understand why Daniil decided not to
> spend 5-10 minutes to reply in the design document discussion thread.
>

Oh, no worries, I just wanted to know if Daniil acknowledged the comments
or not.

Anyway, comments are welcome here and the discussion is still open:)
>

Sounds good.

And thanks Daniil for the commits.
>

Of course, looking forward to see this implemented!

thanks!
iustin

On Thu, Jun 23, 2016 at 5:49 PM 'Iustin Pop' via ganeti-devel <
> ganeti-devel@googlegroups.com> wrote:
>
>> On 23 June 2016 at 16:45, <mele...@gmail.com> wrote:
>>
>>> From: Daniil Leshchev <mele...@gmail.com>
>>>
>>> The patchset introduces new command line options
>>> (--long-solution-threshold" and --avoid-long-solutions").
>>> That gives an ability for HBal to avoid balancing solutions,
>>> that take significant amount of time.
>>>
>>
>> Daniil, I've replied to your design doc changes, but I haven't seen a
>> reply. That discussion would be useful before implementing this :)
>>
>> regards,
>> iustin
>>
>

Re: [PATCH master 0/6] New balancing options implementation

['Iustin Pop' via ganeti-devel]

On 23 June 2016 at 17:42, Даниил Лещёв  wrote:

> Hi, Iustin
>
>
>> Oh, no worries, I just wanted to know if Daniil acknowledged the comments
>> or not.
>>
>> Anyway, comments are welcome here and the discussion is still open:)
>>>
>>
>>
> The only reason why I didn't reply to your comments is that I wanted to
> show patchset in order to discuss ideas in design document in more detailed
> way.
> Hope, that was not a big mistake.
>

Oh no, not a mistake at all.


> I'm also going to rewrite patch for design document (append information
> about bandwidth tags).
>

I would slightly prefer if we discuss it over plain email (without
patches), to see what you think about how complex the network model needs
to be, and whether a static "time X" vs. semi-dynamic (based on the
instance disk size) approach is best.

Maybe there was some more information back at the start of the project? (I
only started watching the mailing list again recently).

thanks!
iustin

Re: [PATCH master 0/6] New balancing options implementation

['Iustin Pop' via ganeti-devel]

On 23 June 2016 at 16:45,  wrote:

> From: Daniil Leshchev 
>
> The patchset introduces new command line options
> (--long-solution-threshold" and --avoid-long-solutions").
> That gives an ability for HBal to avoid balancing solutions,
> that take significant amount of time.
>

Daniil, I've replied to your design doc changes, but I haven't seen a
reply. That discussion would be useful before implementing this :)

regards,
iustin

Re: [PATCH master 0/6] New balancing options implementation

['Iustin Pop' via ganeti-devel]

On 23 June 2016 at 18:32, Даниил Лещёв  wrote:

>
>> I would slightly prefer if we discuss it over plain email (without
>> patches), to see what you think about how complex the network model needs
>> to be, and whether a static "time X" vs. semi-dynamic (based on the
>> instance disk size) approach is best.
>>
>> Maybe there was some more information back at the start of the project?
>> (I only started watching the mailing list again recently).
>>
>> The initial plan was to implement "static" solutions, based on instance
> disk size and then make it "dynamic" by using information about network
> speed from data collectors.
>

Ack.

At the moment, we have "semi-dynamic" solution, I think. The new tags may
> specify network speed in cluster (and between different parts of cluster).
>

I'll look at the patches, but if I read correctly—these are currently
stored as tags. Would it make more sense to have them as proper values in
the objects, so that (in the future) they can be used by other parts of the
code? Just a thought.

I am assuming that this speed remains constant since the network usually
> configured once and locally (for example in server rack).
>

That makes sense.


> I think, with such assumption, the network speed stays almost constant and
> the time estimations for balancing solutions become predictable.
>
> I suggest to use the new options for discarding solutions, that takes long
> time and slightly changes the state of the cluster.
> In my mind the time to perform disk replication is directly depends on the
> network bandwidth.
>

Hmm, depends. On a gigabyte or 10G network and with mechanical harddrives,
the time will depend more on disk load.

thanks,
iustin

Re: [PATCH stable-2.17] Prevent watcher from holding lock on verify group

['Iustin Pop' via ganeti-devel]

2016-06-17 8:31 GMT-07:00 'Federico Morg Pareschi' via ganeti-devel <
ganeti-devel@googlegroups.com>:

> The ganeti-watcher holds the group file lock for too long, until after
> the execution of a group-verify-disk job. This locks for a long time if
> there are other jobs already running and blocking the verify from
> executing. When the lock is held, another ganeti-watcher run cannot be
> scheduled, so this prevents the ganeti-watcher from running for several
> minutes.
>
> With this commit, the lock is released before running the VerifyDisks
> operation, so even if the submitted job gets stuck in the Job Queue, a
> subsequient ganeti-watcher run would still happen.
>

Quick question: what prevents a runaway case where VerifyDisks is blocking
for hours and we have many watchers all running and submitting their own
VerifyDisks?

thanks,
iustin

Re: [PATCH stable-2.16] Fix typos in gnt-cluster man page

['Iustin Pop' via ganeti-devel]

On Fri, Aug 12, 2016 at 03:03:18PM +0100, Ganeti Development List wrote:
> Luckily, nothing that changes the meaning anywhere.
> 
> Signed-off-by: Brian Foley 

LGTM, thanks.

Re: [PATCH stable-2.16] Hide errors for expected inotify failures in unittest

['Iustin Pop' via ganeti-devel]

On 12 August 2016 at 15:04, 'Brian Foley' via ganeti-devel <
ganeti-devel@googlegroups.com> wrote:

> This makes it a little easier to eyeball the output of make py-tests.
>

Ooh, nice, this is a very old bug, thanks!

+logger = logging.getLogger('pyinotify')
> +logger.propagate = False
>  self.assertRaises(errors.InotifyError, handler.enable)
>  self.assertRaises(errors.InotifyError, handler.enable)
>  handler.disable()
>  self.assertRaises(errors.InotifyError, handler.enable)
> +logger.propagate = True
>

Do you want to put a try/finally around the asserts that ensure propagate
is restored?

iustin

Re: [PATCH stable-2.16] Hide errors for expected inotify failures in unittest

['Iustin Pop' via ganeti-devel]

On 12 August 2016 at 15:38, Brian Foley  wrote:

> On Fri, Aug 12, 2016 at 03:13:53PM +0200, Iustin Pop wrote:
> >On 12 August 2016 at 15:04, 'Brian Foley' via ganeti-devel
> ><[1]ganeti-devel@googlegroups.com> wrote:
> >
> >  This makes it a little easier to eyeball the output of make
> >  py-tests.
> >
> >Ooh, nice, this is a very old bug, thanks!
> >
> >  +logger = logging.getLogger('pyinotify')
> >  +logger.propagate = False
> >   self.assertRaises(errors.InotifyError, handler.enable)
> >   self.assertRaises(errors.InotifyError, handler.enable)
> >   handler.disable()
> >   self.assertRaises(errors.InotifyError, handler.enable)
> >  +logger.propagate = True
> >
> >Do you want to put a try/finally around the asserts that ensure
> >propagate is restored?
> >iustin
>
> Good point. There are paths in pyinotify's add_watch that can throw
> exceptions
> (although I don't think we use them). Submitted with your change.
>

Excellent, LGTM :)

iustin

Re: [PATCH master 2/6] Add bandwidth tags and bandwidth map fields into Node

['Iustin Pop' via ganeti-devel]

On 28 June 2016 at 11:28, Даниил Лещёв  wrote:
>
> > The bandwidths map store data about network speed
>> > between current node and given Node by it's bandwidth tags.
>>
>> Filling this up will take some space in a large cluster. Is it really
>> necessary to store this by nodes (which makes it O(n²) in terms of
>> nodes), or simply in terms of node group wide-bandwidth, plus
>> inter-node group bandwidth?
>>
>
> I think, I definitely said wrong.
> At the moment bandwidth map store information about network speed between
> current node and some bandwidth tag (not another node).
>

I see.


> So at the best case (all nodes in cluster have the same bandwidth tag) you
> store O(n) per cluster and in the worst case O(n^2) (all nodes have
> different bandwidth tags).
> If you only have group-wide bandwidth tags you will store O(n*g) per
> cluster, where n is the number of nodes and g is the number of groups (And
> I think it is the most common case).
>
> Should I use some single place for storing network speed information in
> order to avoid data duplication?
>

I have to main concerns here.

1. I still believe it's wrong to model this on a per-node basis, and that
it should be rather two things: bandwidth available inside a node group
(between any two arbitrary nodes), and bandwidth available between any two
node groups. However, your badwidth tag concept is nice. Hmm…

2. Storing the information somewhere else then nodes would indeed be useful.

So let's discuss the main point: how do you or anybody else think that
bandwidth should be represented? Per node, per node group, etc.?

thanks!
iustin

Re: [PATCH master 2/6] Add bandwidth tags and bandwidth map fields into Node

['Iustin Pop' via ganeti-devel]

On 28 June 2016 at 16:16, Даниил Лещёв  wrote:
>
> I have to main concerns here.
>>
>> 1. I still believe it's wrong to model this on a per-node basis, and that
>> it should be rather two things: bandwidth available inside a node group
>> (between any two arbitrary nodes), and bandwidth available between any two
>> node groups. However, your badwidth tag concept is nice. Hmm…
>>
>
> But with such approach we lose an opportunity to specify network speed
> between two specific nodes.
> Or it is really rare case and we don't need such flexibility?
>

That's what I believe, yes. The concept of node groups was introduced do
model (approximately) high-bandwidth connectivity, with inter-node-group
bandwidth being potentially smaller/more contented, but still symmetrical.
I don't know of any deployments that had per-node different bandwidth, but
maybe some other people have such cases?

CC-ing a couple of other people to see their input as well.

thanks,
iustin

Re: [PATCH master 0/6] New balancing options implementation

['Iustin Pop' via ganeti-devel]

On 24 June 2016 at 14:36, Oleg Ponomarev  wrote:

> Hi Iustin,
>
> > I'll look at the patches, but if I read correctly—these are currently
> stored as tags. Would it make more sense to have
> > them as proper values in the objects, so that (in the future) they can
> be used by other parts of the code? Just a thought.
>
> Do you have any ideas about how network bandwidth might be used in Ganeti
> itself?
> At my first glance, this information might be useful in HTools only. And
> in this case, node tags is the common way to pass the information. It's the
> same mechanism as used in HTools to obtain location, migration, desired
> location and some other information.
>

I don't have very strong examples, but the reason I mention this is that I
regard network capacity in a nodegroup as a physical characteristic of the
hardware, not something that is htools-specific, so modelling it explicitly
might have some value. For example, it might make sense to say that the
number of concurrent disk replaces on a given node, or the number of
concurrent instance migrations, might depend on the network bandwidth, such
that Ganeti would automatically adjust the concurrency of such jobs per
node, without needing external control.

That is however far-fetched, so I'm not proposing any change to the code
per se; I was asking to see what others think of this.

regards,
iustin

On Fri, Jun 24, 2016 at 3:17 PM Iustin Pop  wrote:
>
>> On 23 June 2016 at 18:32, Даниил Лещёв  wrote:
>>
>>>
 I would slightly prefer if we discuss it over plain email (without
 patches), to see what you think about how complex the network model needs
 to be, and whether a static "time X" vs. semi-dynamic (based on the
 instance disk size) approach is best.

 Maybe there was some more information back at the start of the project?
 (I only started watching the mailing list again recently).

 The initial plan was to implement "static" solutions, based on instance
>>> disk size and then make it "dynamic" by using information about network
>>> speed from data collectors.
>>>
>>
>> Ack.
>>
>> At the moment, we have "semi-dynamic" solution, I think. The new tags may
>>> specify network speed in cluster (and between different parts of cluster).
>>>
>>
>> I'll look at the patches, but if I read correctly—these are currently
>> stored as tags. Would it make more sense to have them as proper values in
>> the objects, so that (in the future) they can be used by other parts of the
>> code? Just a thought.
>>
>> I am assuming that this speed remains constant since the network usually
>>> configured once and locally (for example in server rack).
>>>
>>
>> That makes sense.
>>
>>
>>> I think, with such assumption, the network speed stays almost constant
>>> and the time estimations for balancing solutions become predictable.
>>>
>>> I suggest to use the new options for discarding solutions, that takes
>>> long time and slightly changes the state of the cluster.
>>> In my mind the time to perform disk replication is directly depends on
>>> the network bandwidth.
>>>
>>
>> Hmm, depends. On a gigabyte or 10G network and with mechanical
>> harddrives, the time will depend more on disk load.
>>
>> thanks,
>> iustin
>>
>

Re: [PATCH stable-2.16 2/2] Make executeRpcCall only compute rpcCallData once

['Iustin Pop' via ganeti-devel]

On 5 July 2016 at 11:44, 'Brian Foley' via ganeti-devel <
ganeti-devel@googlegroups.com> wrote:

> On Mon, Jul 04, 2016 at 08:32:12PM +0200, Iustin Pop wrote:
> > On 2016-07-04 16:57:24, Ganeti Development List wrote:
> > > This is important for distMCsAsyncTask, because currently every time
> > > config.data is updated, wconfd generates a separate copy of the Base64
> > > encoded, zlib compressed config.data payload string for the
> > > /upload_file_single call sent to each master candidate's noded.
> > >
> > > This patch causes it to generate one copy of the string and send that
> > > in parallel to all MCs.
> > >
> > > Signed-off-by: Brian Foley 
> >
> > LGTM, with the mention that the 3 different names for executing a rpc
> > call are too close together. I don't have good suggestions though, so
> > good as is.
> >
> > thanks!
> > iustin
>
> I agree, the names aren't wonderful and I've probably made them worse. I'll
> clean them up in a later patch.
>
> How about sendSingleRpc, sendMultipleRpcs, sendRpcsUsingData?
>

I though about such naming too but it's still misleading. All calls send
multiple RPCs, the different is whether it's the same payload or not.

What about sendSameRpc, sendVaryingRpcs, sendRpcsUsingData (in the latter
case it's up to the caller whether it's the same or not).

iustin

Re: [PATCH stable-2.16] Fix some trivial pep8/pylint errors

['Iustin Pop' via ganeti-devel]

On 7 July 2016 at 13:05, 'Brian Foley' via ganeti-devel <
ganeti-devel@googlegroups.com> wrote:

> Whitespace and an unused variable.
>

LGTM ,thanks.

iustin

Re: RFC: Releasing Ganeti 2.15.3

['Iustin Pop' via ganeti-devel]

On 29 September 2016 at 19:21, 'Brian Foley' via ganeti-devel <
ganeti-devel@googlegroups.com> wrote:

> Hi all,
>
> since December 2015 quite a large number of commits have been made to the
> stable-2.15 branch (87 to be exact). These cover the gamut from performance
> improvements, to compatibility fixes, to error handling, and even some
> minor
> feature improvements. There should be no compatibility breaks though.
>
> You can see the full list with git log --oneline v2.15.2..stable-2.15
>
> I've tried to categorise them all below. I'd like us to do a (final?) patch
> release of 2.15.3, as the patches includes an important fix for socat 1.7.3
> compatibility that's affected quite a few users.
>
> Does anyone else have anything they'd really like to see fixed in 2.15, or
> does
> anyone have any objections to including any of the below in a point
> release?
>

As far as my investigations on
https://code.google.com/p/ganeti/issues/detail?id=1185 went, it seems that
the location setup tags completely broke htools on clusters with non-DRBD
instances. It's a trivial breakage, not a design one, so fixing it
shouldn't be too invasive.

Day work has been crazy busy so I didn't manage to finish up a patch, but I
think this should be fixed in 2.15 stable. When were you planning the
release for?

thanks,
iustin

Re: RFC: Releasing Ganeti 2.15.3

['Iustin Pop' via ganeti-devel]

On 30 September 2016 at 12:30, 'Brian Foley' via ganeti-devel <
ganeti-devel@googlegroups.com> wrote:

> On Fri, Sep 30, 2016 at 11:47:24AM +0200, Iustin Pop wrote:
> >On 29 September 2016 at 19:21, 'Brian Foley' via ganeti-devel
> ><[1]ganeti-devel@googlegroups.com> wrote:
> >
> >  Hi all,
> >  since December 2015 quite a large number of commits have been made
> >  to the
> >  stable-2.15 branch (87 to be exact). These cover the gamut from
> >  performance
> >  improvements, to compatibility fixes, to error handling, and even
> >  some minor
> >  feature improvements. There should be no compatibility breaks
> >  though.
> >  You can see the full list with git log --oneline
> >  v2.15.2..stable-2.15
> >  I've tried to categorise them all below. I'd like us to do a
> >  (final?) patch
> >  release of 2.15.3, as the patches includes an important fix for
> >  socat 1.7.3
> >  compatibility that's affected quite a few users.
> >  Does anyone else have anything they'd really like to see fixed in
> >  2.15, or does
> >  anyone have any objections to including any of the below in a point
> >  release?
> >
> >As far as my investigations on
> >[2]https://code.google.com/p/ganeti/issues/detail?id=1185 went, it
> >seems that the location setup tags completely broke htools on clusters
> >with non-DRBD instances. It's a trivial breakage, not a design one, so
> >fixing it shouldn't be too invasive.
> >Day work has been crazy busy so I didn't manage to finish up a patch,
> >but I think this should be fixed in 2.15 stable. When were you
> planning
> >the release for?
>
> Hi Iustin,
>
> I completely forgot about #1185. Yeah, it would definitely be worth fixing
> this if it's not too hard.
>
> I was hoping to get a release done in the next 2-3 weeks if we could, but
> I've no fixed schedule in mind and I'll go with whatever suits people.
>

Ah, OK, that should very doable. I thought you're planning for the next few
days; I'll try to send a patch then sometimes next week.

thanks!
iustin

Re: [PATCH stable-2.15 00/37] Cleanup for pylint 1.6.4

['Iustin Pop' via ganeti-devel]

On 5 December 2016 at 11:35, 'Brian Foley' via ganeti-devel <
ganeti-devel@googlegroups.com> wrote:

> Ganeti's python code passes the pylint checks in the version of pylint
> included with Debian Jessie. Unfortunately this is a really old pylint
> (0.26 from 2012) and the latest stable pylint (1.6.4 from 2016) has a
> lot of additional checks. Ganeti produces many many warnings and errors
> when run against 1.6.4.
>
> This patchset is a first cut at fixing these, while still maintaining
>  compatibility with pylint 0.26.
>

Quick question: is there a reason to keep that compat, as opposed to
switching the "blessed" (and required) pylint version to 1.6.4?

(If this is addressed in the patch series, sorry, didn't read the patches)

iustin

Re: [PATCH stable-2.15] Fix coexistence of location tags and non-DRBD instances

['Iustin Pop' via ganeti-devel]

On 4 December 2016 at 18:44, Brian Foley  wrote:

> On Fri, Dec 02, 2016 at 11:03:55PM +0100, Iustin Pop wrote:
> > From: Iustin Pop 
> >
> > This addresses issue 1185, “hbal: IntMap.!: key -1 is not an element of
> > the map”. The issue is that the location tags code presumed all
> > instances have a primary and a secondary (i.e., they are DRBD).
> >
> > The fix is to set the location score for non-DRBD instances to zero,
> > since this is what I understand the correct value: such an instance
> > cannot be optimized by moving, so its score is "perfect" (zero).
> >
> > Signed-off-by: Iustin Pop 
>
> LGTM, and thank you. I'm going to do a merge forward of the recent (and a
> batch of upcoming) 2.15 patches in the next few days, and this is a nice
> fix
> to include.
>

Note:  someone with edit rights will need to update the issue once this is
committed (or released). Thanks for the review!

iustin

Re: [PATCH stable-2.15 00/37] Cleanup for pylint 1.6.4

['Iustin Pop' via ganeti-devel]

On 5 December 2016 at 13:04, 'Brian Foley' via ganeti-devel <
ganeti-devel@googlegroups.com> wrote:

> On Mon, Dec 05, 2016 at 12:01:14PM +0100, Iustin Pop wrote:
> >Quick question: is there a reason to keep that compat, as opposed to
> >switching the "blessed" (and required) pylint version to 1.6.4?
> >(If this is addressed in the patch series, sorry, didn't read the
> >patches)
> >iustin
>
> Hi Iustin,
>
> no, there isn't any very good reason, I was just playing it safe, and kinda
> assumed that make dist or the debian build process might run pylint, but it
> appears tht neither do.
>

As long as we require a fixed version, it's not really feasible to do so.
Similarly how -Werror is only enabled in developer builds, I guess.

In that case, I'll move us to requiring 1.6.4, and remove a couple of the
> compatibility workarounds.
>

Yes please, that sounds cleaner.

thanks,
iustin

Re: [PATCH stable-2.15 13/37] Fix pylint >1.4 pycurl no-member warnings

['Iustin Pop' via ganeti-devel]

On 5 December 2016 at 14:56, 'Brian Foley' via ganeti-devel <
ganeti-devel@googlegroups.com> wrote:

> On Mon, Dec 05, 2016 at 01:43:57PM +, Federico Pareschi wrote:
> >  to avoid arbitrary code injection.
> >
> >Is this safe? Should we be looking more into this or is it something
> >that does not affect us?
>
> The attack vector is someone writing their own pycurl.so with malicious
> code
> in it, puting that on the python search path, and using that to perform
> arbitrary actions as the user running pylint when pylint loads pycurl.
>
> I think this is a legitimate concern for developers running pylint as
> themselves and potentially accepting arbitary patches from the internet
> without
> checking what's in them, but it shouldn't be much of a problem for a
> sandboxed
> buildbot.
>

Even for developers, random patches can't easily ship a .so, so while this
is a valid concern for some cases, I think it's less likely for accepting
source diffs (that show no binary files) and linting them.

I wonder if it's possible to write a git config that rejects binary deltas…

The only alternatives I can think of are to explicitly disable the warning
> for
> any ganeti modules that use pycurl directly, or to explicitly add disables
> to
> each use of pycurl.foo but I don't much like those.
>

I tend to agree, this seems a worthwhile tradeoff.

iustin

Re: [PATCH stable-2.15] Fix for incorrect parsing of DRBD versions

['Iustin Pop' via ganeti-devel]

On Thu, Dec 01, 2016 at 11:38:35AM +, Ganeti Development List wrote:
> Following issue #1194, this patch allows Ganeti to correctly
> parse drbd versions that also include a dash in their k-fix
> version component.

This means 8.4.8-1 and 8.4.8.1 will be treated the same. Is this the
correct behaviour?

> Signed-off-by: Federico Morg Pareschi 
> ---
>  lib/storage/drbd_info.py| 17 +++--
>  test/py/ganeti.storage.drbd_unittest.py | 10 ++
>  2 files changed, 25 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/storage/drbd_info.py b/lib/storage/drbd_info.py
> index 99605f1..469ed7f 100644
> --- a/lib/storage/drbd_info.py
> +++ b/lib/storage/drbd_info.py
> @@ -164,13 +164,15 @@ class DRBD8Info(object):
>  
>"""
>  
> -  _VERSION_RE = re.compile(r"^version: (\d+)\.(\d+)\.(\d+)(?:\.(\d+))?"
> +  _VERSION_RE = re.compile(r"^version: (\d+)\.(\d+)\.(\d+)(?:[.-](\d+))?"
> r" \(api:(\d+)/proto:(\d+)(?:-(\d+))?\)")
>_VALID_LINE_RE = re.compile("^ *([0-9]+): cs:([^ ]+).*$")
> +  _K_FIX_DASH_SEPARATOR_RE = re.compile(r"^version: 
> (\d+)\.(\d+)\.(\d+)(?:-)")
>  
> +  def _GetKFixSeparator(self, lines):
> +"""Check, in case of a K-fix version, if the separator is a dash or 
> dot."""
> +
> +first_line = lines[0].strip()
> +match = self._K_FIX_DASH_SEPARATOR_RE.match(first_line)
> +if match is None:
> +  return "."
> +else:
> +  return "-"

This seems to be done in two steps. Would it be simpler to have
K_FIX_DASH_SEPARATOR itself extract the separator, instead of it having
to match - and if not, return . vs -?

You could even get rid of _K_FIX_DASH_SEPARATOR_RE, and extract the
separator from the _VERSION_RE, after changing the RE to have the
separator a capturing group.

thanks,
iustin

Re: [PATCH stable-2.15] Fix for incorrect parsing of DRBD versions

['Iustin Pop' via ganeti-devel]

On Thu, Dec 01, 2016 at 04:09:56AM -0800, Ganeti Development List wrote:
> 
> On Thursday, December 1, 2016 at 11:53:43 AM UTC, Iustin Pop wrote:
> >
> > On Thu, Dec 01, 2016 at 11:38:35AM +, Ganeti Development List wrote: 
> > > Following issue #1194, this patch allows Ganeti to correctly 
> > > parse drbd versions that also include a dash in their k-fix 
> > > version component. 
> >
> > This means 8.4.8-1 and 8.4.8.1 will be treated the same. Is this the 
> > correct behaviour? 
> >
> 
> Yes, you are correct. I'm not sure to be honest. I've been looking around 
> the drbd and 
> also the debian packaging site but I'm not 100% sure what logic follows 
> their 'k-fix' 
> numbering scheme. In drbd, from what I can see, they always use x.y.z-k 
> except 
> in one particular (old) version where they do x.y.z.k. 
> 
> Honestly, though, as somebody correctly pointed out on the ganeti mailing 
> list, does it
> really matter? Shouldn't we mostly only care about major and minor version 
> as that 
> should be what might break compatibility? All in all I'd say we can treat 
> these as 
> interchangeable but if somebody disagrees or knows more about this, I'd be 
> happy to 
> fix it.


No, it doesn't really matter, I was just asking to confirm this is the
plan. I'd suggest updating the commit message to record this explicit
decision (the current one will not be very clear in 2 years whether it
was intended or not).

iustin

Re: Status of migrated issues?

['Iustin Pop' via ganeti-devel]

On 29 January 2018 at 11:08, 'Federico Pareschi' via ganeti-devel <
ganeti-devel@googlegroups.com> wrote:

> Sorry, this email had completely flown under my radar.
>

No worries.

The issues have all been migrated to the github repository, however there
> is a difference between 'fixed' and 'released' in the old issue tracker on
> code.google.com. By importing these issues in github, these states turned
> into tags. I assume 'released' meant the issue could be closed, but 'fixed'
> simply added the tag but the issue stayed open.
>

Sorry, not sure I understand here. Do I read that code.google.com had two
"fixed" state - one fix committed ("fixed"), and one fix committed and also
released ("released")? If so, and if Github only has fixed, I propose to
drop the released state.

I had been thinking about going through all of them and closing the ones
> marked as fixed but I wanted to review them first and is a lot of
> time/effort that I haven't invested yet (verifying the fix + closing the
> issue rather than just automatically closing all of them).
>

I would suggest to simply fix all of them, based on:

- we should trust the code.google.com "fixed" tag;
- there are a lot of very old issues that are for sure fixed, and verifying
all of them is time better spent on fixing new issues

If some issue is still open, and if users still hit it, let's crowdsource
the effort of reopening (against current versions) to the users :)

iustin

On Fri, 26 Jan 2018 at 19:53, Iustin Pop  wrote:
>
>> On 2018-01-14 02:15:41, Iustin Pop wrote:
>> > Hi all,
>> >
>> > I did a quick look at the GitHub issue tracker today, and it seems that
>> > the migrator did not close the migrated issues that were closed.
>> >
>> > More specifically, a search of:
>> >
>> > is:issue is:open label:Status:Fixed
>> >
>> > Shows 98 such open issues. If I'm not mistaken, "Status:Fixed" is a
>> > googlecode imported label, which tells me that the migrator had some
>> > issue (ha) importing the bug list.
>>
>> Friendly ping? Mostly for my own curiosity.
>>
>> iustin
>>
>