[rhel6-branch] [PATCH] Limit the maximum swap size to 10 % of disk space

[Vratislav Podzimek]

On e.g. virtual machines with smalls disks and high amount of
memory it doesn't make sense to create large swap space leaving
only a little for the system and user data.

Also change comment to a docstring and document parameters.

Related: rhbz#878907

Signed-off-by: Vratislav Podzimek 
---
 installclass.py |  8 +++++++-
 iutil.py        | 28 ++++++++++++++++++++++++----
 kickstart.py    | 18 ++++++++++++++++--
 3 files changed, 47 insertions(+), 7 deletions(-)

diff --git a/installclass.py b/installclass.py
index a417762..fb07b03 100644
--- a/installclass.py
+++ b/installclass.py
@@ -191,7 +191,13 @@ class BaseInstallClass(object):
         if bootreq:
             autorequests.extend(bootreq)

-        (minswap, maxswap) = iutil.swapSuggestion()
+        disk_space = 0
+        for disk in storage.disks:
+            if not storage.clearPartDisks \
+                    or disk.name in storage.clearPartDisks:
+                disk_space += disk.size
+
+        (minswap, maxswap) = iutil.swapSuggestion(disk_space=disk_space)
         autorequests.append(PartSpec(fstype="swap", size=minswap, maxSize=maxswap,
                                      grow=True, asVol=True))

diff --git a/iutil.py b/iutil.py
index 3e3f903..3a9a583 100644
--- a/iutil.py
+++ b/iutil.py
@@ -40,6 +40,9 @@ import logging
 log = logging.getLogger("anaconda")
 program_log = logging.getLogger("program")

+# maximum ratio of swap size to disk size (10%)
+MAX_SWAP_DISK_RATIO = 0.1
+
 #Python reimplementation of the shell tee process, so we can
 #feed the pipe output into two places at the same time
 class tee(threading.Thread):
@@ -450,10 +453,17 @@ def memInstalled():

return long(mem)

-## Suggest the size of the swap partition that will be created.
-# @param quiet Should size information be logged?
-# @return A tuple of the minimum and maximum swap size, in megabytes.
-def swapSuggestion(quiet=0, hibernation=False):
+def swapSuggestion(quiet=0, hibernation=False, disk_space=None):
+    """
+    Suggest the size of the swap partition that will be created.
+
+    @param quiet: Should size information be logged?
+    @param hibernation: Suggest size of the swap partition to support hibernation?
+    @param disk_space: Total disk space available
+    @return: A tuple of the minimum and maximum swap size, in megabytes.
+
+    """
+
     mem = memInstalled()/1024
     mem = ((mem/16)+1)*16
     if not quiet:
@@ -479,6 +489,16 @@ def swapSuggestion(quiet=0, hibernation=False):
         else:
             log.info("Ignoring --hibernation option on systems with 64 GB of RAM or more")

+    if disk_space is not None:
+        max_swap = int(disk_space * MAX_SWAP_DISK_RATIO)
+        if swap > max_swap:
+            log.info("Suggested swap size (%(swap)d M) exceeds %(percent)d %% of "
+                     "disk space, using %(percent)d %% of disk space (%(size)d M) "
+                     "instead." % {"percent": MAX_SWAP_DISK_RATIO*100,
+                                   "swap": swap,
+                                   "size": max_swap})
+            swap = max_swap
+
     if not quiet:
    log.info("Swap attempt of %sM", swap)

diff --git a/kickstart.py b/kickstart.py
index 493a7c6..2d5144f 100644
--- a/kickstart.py
+++ b/kickstart.py
@@ -478,11 +478,18 @@ class LogVolData(commands.logvol.RHEL6_LogVolData):

storage.doAutoPart = False

+        disk_space = 0
+        for disk in storage.disks:
+            if not storage.clearPartDisks \
+                    or disk.name in storage.clearPartDisks:
+                disk_space += disk.size
+
         if self.mountpoint == "swap":
             type = "swap"
             self.mountpoint = ""
             if self.recommended or self.hibernation:
-                (self.size, self.maxSizeMB) = iutil.swapSuggestion(hibernation=self.hibernation)
+                (self.size, self.maxSizeMB) = \
+                    iutil.swapSuggestion(hibernation=self.hibernation, disk_space=disk_space)
                 self.grow = True
         else:
             if self.fstype != "":
@@ -749,11 +756,18 @@ class PartitionData(commands.partition.RHEL6_PartData):
             if self.disk == "":
                 raise KickstartValueError, formatErrorMsg(self.lineno, msg="Specified BIOS disk %s cannot be determined" % self.onbiosdisk)

+        disk_space = 0
+        for disk in storage.disks:
+            if not storage.clearPartDisks \
+                    or disk.name in storage.clearPartDisks:
+                disk_space += disk.size
+
         if self.mountpoint == "swap":
             type = "swap"
             self.mountpoint = ""
             if self.recommended or self.hibernation:
-                (self.size, self.maxSizeMB) = iutil.swapSuggestion(hibernation=self.hibernation)
+                (self.size, self.maxSizeMB) = \
+                    iutil.swapSuggestion(hibernation=self.hibernation, disk_space=disk_space)
                 self.grow = True

# if people want to specify no mountpoint for some reason, let them

[David Lehman]

On Thu, 2012-12-06 at 14:00 +0100, Vratislav Podzimek wrote:
> On e.g. virtual machines with smalls disks and high amount of
> memory it doesn't make sense to create large swap space leaving
> only a little for the system and user data.

The only concern I have is that the set of available disks can change
and the swap suggestion is only calculated once IIRC. That is why in my
version I did the correction/capping at the time of partition
allocation. Still it wouldn't be too difficult to recalculate the swap
suggestion when leaving the clearpart_disks screen.

Dave
> Also change comment to a docstring and document parameters.
>
> Related: rhbz#878907
> Signed-off-by: Vratislav Podzimek
>
> installclass.py |  8 +++++++-
>  iutil.py        | 28 ++++++++++++++++++++++++----
>  kickstart.py    | 18 ++++++++++++++++--
>  3 files changed, 47 insertions(+), 7 deletions(-)
>
> diff --git a/installclass.py b/installclass.py
> index a417762..fb07b03 100644
> --- a/installclass.py
> +++ b/installclass.py
> @@ -191,7 +191,13 @@ class BaseInstallClass(object):
>          if bootreq:
>              autorequests.extend(bootreq)
>    (minswap, maxswap) = iutil.swapSuggestion()
>    disk_space = 0
>    for disk in storage.disks:
>        if not storage.clearPartDisks \
>                or disk.name in storage.clearPartDisks:
>            disk_space += disk.size
>    (minswap, maxswap) = iutil.swapSuggestion(disk_space=disk_space)
>    autorequests.append(PartSpec(fstype="swap", size=minswap, maxSize=maxswap,
>                                 grow=True, asVol=True))
>
> diff --git a/iutil.py b/iutil.py
> index 3e3f903..3a9a583 100644
> --- a/iutil.py
> +++ b/iutil.py
> @@ -40,6 +40,9 @@ import logging
>  log = logging.getLogger("anaconda")
>  program_log = logging.getLogger("program")
>
> +# maximum ratio of swap size to disk size (10%)
> +MAX_SWAP_DISK_RATIO = 0.1
>
> #Python reimplementation of the shell tee process, so we can
>  #feed the pipe output into two places at the same time
>  class tee(threading.Thread):
> @@ -450,10 +453,17 @@ def memInstalled():
>  return long(mem)
>
> -## Suggest the size of the swap partition that will be created.
> -# @param quiet Should size information be logged?
> -# @return A tuple of the minimum and maximum swap size, in megabytes.
> -def swapSuggestion(quiet=0, hibernation=False):
> +def swapSuggestion(quiet=0, hibernation=False, disk_space=None):
> """
> Suggest the size of the swap partition that will be created.
> @param quiet: Should size information be logged?
> @param hibernation: Suggest size of the swap partition to support hibernation?
> @param disk_space: Total disk space available
> @return: A tuple of the minimum and maximum swap size, in megabytes.
> """
> mem = memInstalled()/1024
>    mem = ((mem/16)+1)*16
>    if not quiet:
>
> @@ -479,6 +489,16 @@ def swapSuggestion(quiet=0, hibernation=False):
>          else:
>              log.info("Ignoring --hibernation option on systems with 64 GB of RAM or more")
> if disk_space is not None:
>    max_swap = int(disk_space * MAX_SWAP_DISK_RATIO)
>    if swap > max_swap:
>        log.info("Suggested swap size (%(swap)d M) exceeds %(percent)d %% of "
>                 "disk space, using %(percent)d %% of disk space (%(size)d M) "
>                 "instead." % {"percent": MAX_SWAP_DISK_RATIO*100,
>                               "swap": swap,
>                               "size": max_swap})
>        swap = max_swap
> if not quiet:
>  log.info("Swap attempt of %sM", swap)
>
> diff --git a/kickstart.py b/kickstart.py
> index 493a7c6..2d5144f 100644
> --- a/kickstart.py
> +++ b/kickstart.py
> @@ -478,11 +478,18 @@ class LogVolData(commands.logvol.RHEL6_LogVolData):
>      storage.doAutoPart = False
>
>    disk_space = 0
>    for disk in storage.disks:
>        if not storage.clearPartDisks \
>                or disk.name in storage.clearPartDisks:
>            disk_space += disk.size
>    if self.mountpoint == "swap":
>        type = "swap"
>        self.mountpoint = ""
>        if self.recommended or self.hibernation:
>            (self.size, self.maxSizeMB) = iutil.swapSuggestion(hibernation=self.hibernation)
>            (self.size, self.maxSizeMB) = \
>                iutil.swapSuggestion(hibernation=self.hibernation, disk_space=disk_space)
>            self.grow = True
>    else:
>        if self.fstype != "":
>
> @@ -749,11 +756,18 @@ class PartitionData(commands.partition.RHEL6_PartData):
>              if self.disk == "":
>                  raise KickstartValueError, formatErrorMsg(self.lineno, msg="Specified BIOS disk %s cannot be determined" % self.onbiosdisk)
>    disk_space = 0
>    for disk in storage.disks:
>        if not storage.clearPartDisks \
>                or disk.name in storage.clearPartDisks:
>            disk_space += disk.size
>    if self.mountpoint == "swap":
>        type = "swap"
>        self.mountpoint = ""
>        if self.recommended or self.hibernation:
>            (self.size, self.maxSizeMB) = iutil.swapSuggestion(hibernation=self.hibernation)
>            (self.size, self.maxSizeMB) = \
>                iutil.swapSuggestion(hibernation=self.hibernation, disk_space=disk_space)
>            self.grow = True
>
>    # if people want to specify no mountpoint for some reason, let them

[Vratislav Podzimek]

On Thu, 2012-12-06 at 10:04 -0600, David Lehman wrote:
> On Thu, 2012-12-06 at 14:00 +0100, Vratislav Podzimek wrote:
> > On e.g. virtual machines with smalls disks and high amount of
> > memory it doesn't make sense to create large swap space leaving
> > only a little for the system and user data.
>
> The only concern I have is that the set of available disks can change
> and the swap suggestion is only calculated once IIRC. That is why in my
> version I did the correction/capping at the time of partition
> allocation. Still it wouldn't be too difficult to recalculate the swap
> suggestion when leaving the clearpart_disks screen.
>
> Dave

Yeah, I've tested the patch with manual installation and it doesn't work
because swap suggestion is calculated in an early phase where storage is
not initialized and thus results in 0 causing division by zero later.
I'm trying to modify the patch to recalculate the suggestion when
leaving the clearpart_disks screen.

[Chris Lumens]

> On e.g. virtual machines with smalls disks and high amount of
> memory it doesn't make sense to create large swap space leaving
> only a little for the system and user data.
>
> Also change comment to a docstring and document parameters.
>
> Related: rhbz#878907
>
> Signed-off-by: Vratislav Podzimek

Whatever change here, this absolutely will require both a release note
and an update to the relevant knowledge base/installation guide/etc.
pages as well.  We need to avoid getting back into the situation where
anaconda's swap algorithm and the documented swap algorithm differ.

- Chris