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

['Viktor Bachraty' via ganeti-devel]

LGTM, thanks for tracing this down!

On Thursday, May 26, 2016 at 3:56:14 PM UTC+1, Brian Foley 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  
> --- 
>  src/Ganeti/Config.hs | 37 ++--- 
>  1 file changed, 26 insertions(+), 11 deletions(-) 
>
> diff --git a/src/Ganeti/Config.hs b/src/Ganeti/Config.hs 
> index cf2f885..5b40694 100644 
> --- a/src/Ganeti/Config.hs 
> +++ b/src/Ganeti/Config.hs 
> @@ -90,7 +90,7 @@ import qualified Data.ByteString as BS 
>  import qualified Data.ByteString.UTF8 as UTF8 
>  import qualified Data.Foldable as F 
>  import Data.List (foldl', nub) 
> -import Data.Maybe (fromMaybe) 
> +import Data.Maybe (fromMaybe, mapMaybe) 
>  import Data.Monoid 
>  import qualified Data.Map as M 
>  import qualified Data.Set as S 
> @@ -158,21 +158,36 @@ instNodes :: ConfigData -> Instance -> S.Set String 
>  instNodes cfg inst = maybe id S.insert (instPrimaryNode inst) 
>$ instDiskNodes cfg inst 
>   
> --- | Computes the secondary nodes of an instance. Since this is valid 
> --- only for DRBD, we call directly 'instDiskNodes', skipping over the 
> --- extra primary insert. 
> -instSecondaryNodes :: ConfigData -> Instance -> S.Set String 
> -instSecondaryNodes cfg inst = 
> -  maybe id S.delete (instPrimaryNode inst) $ instDiskNodes cfg inst 
> +-- | 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 
> +_ -> Nothing 
>   
>  -- | Get instances of a given node. 
>  -- The node is specified through its UUID. 
> +-- The secondary calculation is expensive and frequently called, so 
> optimise 
> +-- this to allocate fewer temporary values 
>  getNodeInstances :: ConfigData -> String -> ([Instance], [Instance]) 
>  getNodeInstances cfg nname = 
> -let all_inst = M.elems . fromContainer . configInstances $ cfg 
> -pri_inst = filter ((== Just nname) . instPrimaryNode) all_inst 
> -sec_inst = filter ((nname `S.member`) . instSecondaryNodes cfg) 
> all_inst 
> -in (pri_inst, sec_inst) 
> +let all_insts = M.elems . fromContainer . configInstances $ cfg 
> +all_disks = fromContainer . configDisks $ cfg 
> + 
> +pri_inst = filter ((== Just nname) . instPrimaryNode) $ all_insts 
> + 
> +find_disk :: String -> Maybe Disk 
> +find_disk d_uuid = M.lookup (UTF8.fromString d_uuid) all_disks 
> + 
> +inst_disks :: [(Instance, [Disk])] 
> +inst_disks = [(i, mapMaybe find_disk $ instDisks i) | i <- 
> all_insts] 
> + 
> +sec_insts :: [Instance] 
> +sec_insts = [inst | 
> +(inst, disks) <- inst_disks, 
> +s_uuid <- mapMaybe computeDiskSecondaryNode disks, 
> +s_uuid == nname] 
> +in (pri_inst, sec_insts) 
>   
>  -- | Computes the role of a node. 
>  getNodeRole :: ConfigData -> Node -> NodeRole 
> -- 
> 2.8.0.rc3.226.g39d4020 
>
>

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

['Brian Foley' via ganeti-devel]

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 
---
 src/Ganeti/Config.hs | 37 ++---
 1 file changed, 26 insertions(+), 11 deletions(-)

diff --git a/src/Ganeti/Config.hs b/src/Ganeti/Config.hs
index cf2f885..5b40694 100644
--- a/src/Ganeti/Config.hs
+++ b/src/Ganeti/Config.hs
@@ -90,7 +90,7 @@ import qualified Data.ByteString as BS
 import qualified Data.ByteString.UTF8 as UTF8
 import qualified Data.Foldable as F
 import Data.List (foldl', nub)
-import Data.Maybe (fromMaybe)
+import Data.Maybe (fromMaybe, mapMaybe)
 import Data.Monoid
 import qualified Data.Map as M
 import qualified Data.Set as S
@@ -158,21 +158,36 @@ instNodes :: ConfigData -> Instance -> S.Set String
 instNodes cfg inst = maybe id S.insert (instPrimaryNode inst)
   $ instDiskNodes cfg inst
 
--- | Computes the secondary nodes of an instance. Since this is valid
--- only for DRBD, we call directly 'instDiskNodes', skipping over the
--- extra primary insert.
-instSecondaryNodes :: ConfigData -> Instance -> S.Set String
-instSecondaryNodes cfg inst =
-  maybe id S.delete (instPrimaryNode inst) $ instDiskNodes cfg inst
+-- | 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
+_ -> Nothing
 
 -- | Get instances of a given node.
 -- The node is specified through its UUID.
+-- The secondary calculation is expensive and frequently called, so optimise
+-- this to allocate fewer temporary values
 getNodeInstances :: ConfigData -> String -> ([Instance], [Instance])
 getNodeInstances cfg nname =
-let all_inst = M.elems . fromContainer . configInstances $ cfg
-pri_inst = filter ((== Just nname) . instPrimaryNode) all_inst
-sec_inst = filter ((nname `S.member`) . instSecondaryNodes cfg) 
all_inst
-in (pri_inst, sec_inst)
+let all_insts = M.elems . fromContainer . configInstances $ cfg
+all_disks = fromContainer . configDisks $ cfg
+
+pri_inst = filter ((== Just nname) . instPrimaryNode) $ all_insts
+
+find_disk :: String -> Maybe Disk
+find_disk d_uuid = M.lookup (UTF8.fromString d_uuid) all_disks
+
+inst_disks :: [(Instance, [Disk])]
+inst_disks = [(i, mapMaybe find_disk $ instDisks i) | i <- all_insts]
+
+sec_insts :: [Instance]
+sec_insts = [inst |
+(inst, disks) <- inst_disks,
+s_uuid <- mapMaybe computeDiskSecondaryNode disks,
+s_uuid == nname]
+in (pri_inst, sec_insts)
 
 -- | Computes the role of a node.
 getNodeRole :: ConfigData -> Node -> NodeRole
-- 
2.8.0.rc3.226.g39d4020

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 :

> 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 
>
> 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 instance-debootstrap] Fix sfdisk invocation for util-linux 2.26+

['Brian Foley' via ganeti-devel]

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 

LGTM. Iustin, do you still have push privileges, or would you like me to do it?

Cheers,
Brian.
> ---
>  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" < +  # Create one big partition, and make it bootable.
> +  local ARGS
> +  # Some versions of sfdisk need manual specification of head/sectors
> +  # for devices such as drbd which don't report geometry.
> +  if sfdisk --help | grep -q -e '--cylinders'; then
> +ARGS="-H 64 -S 32 -u S --Linux"
> +  fi
> +  sfdisk $ARGS --quiet "$1" <  ${PARTITION_ALIGNMENT},,L,*
>  EOF
>  }
> -- 
> 2.8.0.rc3.226.g39d4020
> 

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