Bug 1381111

Hi Guys
after my discussion with frank zdraski
i have tried to make a try to contribute some code to triple-o ironic.

i have a working solution which takes out 2 parameters as part of the ironic node properties.
storage_gb    - number in GB for storage partition on the root disk
storage_journal_gb - number in GB for storage_journal on the Root Disk.

the solution is basically Similar to epheral Disk feature which already exists as part of triple-o.

i believe this is the right way to add the ability to add this partitions and later on as part of the already existing osd puppet post deploy just mention those partitions in the storage-environemnt.yaml to use this partitions (or only one if journal is suppose to be on a differnt location).

In Short it was already tested and working so i can share a portion of the code and see together if this is a make sense solution ....

The solution touches only 4 files:
	modified:   etc/ironic/ironic.conf.sample
	modified:   ironic/drivers/modules/agent_client.py
	modified:   ironic/drivers/modules/deploy_utils.py
	modified:   ironic/drivers/modules/iscsi_deploy.py


the man change is here: (See diff)

diff --git a/ironic/drivers/modules/deploy_utils.py b/ironic/drivers/modules/deploy_utils.py
index 93c79a0..03e2595 100644
--- a/ironic/drivers/modules/deploy_utils.py
+++ b/ironic/drivers/modules/deploy_utils.py
@@ -251,7 +251,7 @@ def get_disk_identifier(dev):
     return disk_identifier[0]
 
 
-def make_partitions(dev, root_mb, swap_mb, ephemeral_mb,
+def make_partitions(dev, root_mb,  swap_mb, storage_mb, storage_journal_mb, ephemeral_mb,
                     configdrive_mb, node_uuid, commit=True,
                     boot_option="netboot", boot_mode="bios"):
     """Partition the disk device.
@@ -264,6 +264,10 @@ def make_partitions(dev, root_mb, swap_mb, ephemeral_mb,
         no partition will be created.
     :param ephemeral_mb: Size of the ephemeral partition in mebibytes (MiB).
         If 0, no partition will be created.
+    :param storage_mb: Size of the storage partition in mebibytes (MiB).
+        If 0, no partition will be created.
+    :param storage_journal_mb: Size of the storage journal partition in mebibytes (MiB).
+        If 0, no partition will be created.
     :param configdrive_mb: Size of the configdrive partition in
         mebibytes (MiB). If 0, no partition will be created.
     :param commit: True/False. Default for this setting is True. If False
@@ -298,6 +302,17 @@ def make_partitions(dev, root_mb, swap_mb, ephemeral_mb,
                   {'dev': dev, 'size': ephemeral_mb, 'node': node_uuid})
         part_num = dp.add_partition(ephemeral_mb)
         part_dict['ephemeral'] = part_template % part_num
+
+    if storage_mb:
+        LOG.debug("Add storage partition (%(size)d / (%(journal_size)d MB) disk/journal to device: %(dev)s "
+                  "for node %(node)s",
+                  {'dev': dev, 'size': storage_mb ,'journal_size': storage_journal_mb, 'node': node_uuid})
+        part_num = dp.add_partition(storage_mb)
+        part_dict['storage'] = part_template % part_num
+        if storage_journal_mb:
+            part_num = dp.add_partition(storage_journal_mb)
+            part_dict['storage_journal'] = part_template % part_num
+
     if swap_mb:
         LOG.debug("Add Swap partition (%(size)d MB) to device: %(dev)s "
                   "for node %(node)s",
@@ -321,6 +336,7 @@ def make_partitions(dev, root_mb, swap_mb, ephemeral_mb,
                                                    boot_mode == "bios"))
     part_dict['root'] = part_template % part_num
 
+
     if commit:
         # write to the disk
         dp.commit()
@@ -577,7 +593,7 @@ def _get_configdrive(configdrive, node_uuid):
         return (configdrive_mb, configdrive_file.name)
 
 
-def work_on_disk(dev, root_mb, swap_mb, ephemeral_mb, ephemeral_format,
+def work_on_disk(dev, root_mb, swap_mb,storage_mb , storage_journal_mb, storage_format, ephemeral_mb, ephemeral_format,
                  image_path, node_uuid, preserve_ephemeral=False,
                  configdrive=None, boot_option="netboot",
                  boot_mode="bios"):
@@ -622,7 +638,7 @@ def work_on_disk(dev, root_mb, swap_mb, ephemeral_mb, ephemeral_format,
             configdrive_mb, configdrive_file = _get_configdrive(configdrive,
                                                                 node_uuid)
 
-        part_dict = make_partitions(dev, root_mb, swap_mb, ephemeral_mb,
+        part_dict = make_partitions(dev, root_mb, swap_mb, storage_mb ,storage_journal_mb , ephemeral_mb,
                                     configdrive_mb, node_uuid,
                                     commit=commit,
                                     boot_option=boot_option,
@@ -632,6 +648,8 @@ def work_on_disk(dev, root_mb, swap_mb, ephemeral_mb, ephemeral_format,
                  {'dev': dev, "node": node_uuid})
 
         ephemeral_part = part_dict.get('ephemeral')
+        storage_part = part_dict.get('storage')
+        storage_journal_part = part_dict.get('storage_journal')
         swap_part = part_dict.get('swap')
         configdrive_part = part_dict.get('configdrive')
         root_part = part_dict.get('root')
@@ -640,7 +658,7 @@ def work_on_disk(dev, root_mb, swap_mb, ephemeral_mb, ephemeral_format,
             raise exception.InstanceDeployFailure(
                 _("Root device '%s' not found") % root_part)
 
-        for part in ('swap', 'ephemeral', 'configdrive',
+        for part in ('storage_journal', 'stoarge', 'swap', 'ephemeral', 'configdrive',
                      'efi system partition'):
             part_device = part_dict.get(part)
             LOG.debug("Checking for %(part)s device (%(dev)s) on node "
@@ -686,6 +704,17 @@ def work_on_disk(dev, root_mb, swap_mb, ephemeral_mb, ephemeral_format,
                      "formatted for node %(node)s"),
                  {'ephemeral': ephemeral_part, 'node': node_uuid})
 
+    if storage_part:
+        utils.mkfs(storage_format, storage_part, "storage")
+        LOG.info(_LI("storage partition %(storage)s successfully "
+                     "formatted %(storage_format) for node %(node)s"),
+                 {'storage': storage_part, 'storage_format' : storage_format, 'node': node_uuid})
+        if storage_journal_part:
+            utils.mkfs(storage_format, storage_journal_part, "storage_journal")
+            LOG.info(_LI("storage journal partition %(storage_journal)s successfully "
+                         "formatted %(storage_format) for node %(node)s"),
+                     {'storage_journal': storage_journal_part, 'storage_format' : storage_format, 'node': node_uuid})
+
     uuids_to_return = {
         'root uuid': root_part,
         'efi system partition uuid': part_dict.get('efi system partition')
@@ -705,7 +734,7 @@ def work_on_disk(dev, root_mb, swap_mb, ephemeral_mb, ephemeral_format,
 
 def deploy_partition_image(
         address, port, iqn, lun, image_path,
-        root_mb, swap_mb, ephemeral_mb, ephemeral_format, node_uuid,
+        root_mb, swap_mb, ephemeral_mb, storage_mb, storage_journal_mb, storage_format , ephemeral_format, node_uuid,
         preserve_ephemeral=False, configdrive=None,
         boot_option="netboot", boot_mode="bios"):
     """All-in-one function to deploy a partition image to a node.
@@ -717,6 +746,10 @@ def deploy_partition_image(
     :param image_path: Path for the instance's disk image.
     :param root_mb: Size of the root partition in megabytes.
     :param swap_mb: Size of the swap partition in megabytes.
+    :param storage_mb: Size of the storage partition in megabytes.
+    :param storage_journal_mb: Size of the storage journal partition in megabytes.
+    :param storage_format: The type of file system to format the storage
+        partition.
     :param ephemeral_mb: Size of the ephemeral partition in megabytes. If 0,
         no ephemeral partition will be created.
     :param ephemeral_format: The type of file system to format the ephemeral
@@ -739,6 +772,8 @@ def deploy_partition_image(
               exist.
     """
     image_mb = get_image_mb(image_path)
+#   storage_mb = 900000            #will be configurable via ironic node properties - different for each system
+#   storage_journal_mb = 1024      #Journal is defaulted to 1Giga
     if image_mb > root_mb:
         msg = (_('Root partition is too small for requested image. Image '
                  'virtual size: %(image_mb)d MB, Root size: %(root_mb)d MB')
@@ -747,7 +782,7 @@ def deploy_partition_image(
 
     with _iscsi_setup_and_handle_errors(address, port, iqn, lun) as dev:
         uuid_dict_returned = work_on_disk(
-            dev, root_mb, swap_mb, ephemeral_mb, ephemeral_format, image_path,
+            dev, root_mb, swap_mb, storage_mb , storage_journal_mb, storage_format, ephemeral_mb, ephemeral_format, image_path,
             node_uuid, preserve_ephemeral=preserve_ephemeral,
             configdrive=configdrive, boot_option=boot_option,
             boot_mode=boot_mode)
@@ -1277,6 +1312,40 @@ def get_boot_option(node):
     return capabilities.get('boot_option', 'netboot').lower()
 
 
+def get_storage_option(node):
+    """Gets the storage_gb configuration option.
+
+    :param node: A single Node.
+    :raises: InvalidParameterValue if the capabilities string is not a
+         dict or is malformed.
+    :returns: A string representing the storage option configuration. Defaults to
+        '0' , the default means do not install ceph on Root Disk.
+    """
+    properties = node.properties
+    storage_gb = properties['storage_gb']
+    if storage_gb is None:
+        storage_gb = 0
+    LOG.debug('Deploy storage_gb is %(storage_gb)s for %(node)s.',
+              {'storage_gb': storage_gb, 'node': node.uuid})
+    return storage_gb
+
+def get_storage_journal_option(node):
+    """Gets the storage_journal_gb configuration option.
+
+    :param node: A single Node.
+    :raises: InvalidParameterValue if the capabilities string is not a
+         dict or is malformed.
+    :returns: A number representing the storage journal option configuration in gb. Defaults to
+        '0' , the default means do not install journal on Root Disk.
+    """
+    properties = node.properties
+    storage_journal_gb = properties['storage_journal_gb']
+    if storage_journal_gb is None:
+        storage_journal_gb = 0
+    LOG.debug('Deploy storage_journal_gb is %(storage_journal_gb)s for %(node)s.',
+              {'storage_journal_gb': storage_journal_gb, 'node': node.uuid})
+    return storage_journal_gb
+
 def prepare_cleaning_ports(task):
     """Prepare the Ironic ports of the node for cleaning.

i would like to go over this possible solution with you guys and HLD document if it's ok from your side ? please let me know if this is a valid option? and if yes how to do it
Regards

Hi!

We do have advanced partitioning planned for Ironic, but it won't happen before Pike (OSP 12). In the meanwhile, there are two ways to work around it:
1. Use whole-disk images. This is what I personally recommend, then you can build any layout you want into the image itself. Yolanda is working on its support in TripleO for OSP 11, but you can do it manually right now as well.
2. Use a post-deployment script to adjust partitioning. This may not be convenient, and I'm not the best person to talk about it. But I'm aware that people were doing it for Ceph already.

Kobi Ginon, you patch brings Ceph-specific assumptions in Ironic, thus it won't be acceptable upstream. So, I'm afraid we won't be able to accept it either or support in any form.

*** Bug 1379607 has been marked as a duplicate of this bug. ***

*** Bug 1300405 has been marked as a duplicate of this bug. ***

(In reply to Dmitry Tantsur from comment #5)
> Hi!
> 
> We do have advanced partitioning planned for Ironic, but it won't happen
> before Pike (OSP 12). In the meanwhile, there are two ways to work around it:
> 1. Use whole-disk images. This is what I personally recommend, then you can
> build any layout you want into the image itself. Yolanda is working on its
> support in TripleO for OSP 11, but you can do it manually right now as well.
> 2. Use a post-deployment script to adjust partitioning. This may not be
> convenient, and I'm not the best person to talk about it. But I'm aware that
> people were doing it for Ceph already.
> 
> Kobi Ginon, you patch brings Ceph-specific assumptions in Ironic, thus it
> won't be acceptable upstream. So, I'm afraid we won't be able to accept it
> either or support in any form.

Hi Dmitry
i made the changes to the code as a generic approach not specific to ceph
Opened a Blueprint in ironic
https://blueprints.launchpad.net/ironic/+spec/generic-image-partition-aproach

Hi Kobi, as an alternative you can start using whole disk images now, and adjust the volumes with the size you need, so you can have extra free disk space for ceph:

http://teknoarticles.blogspot.com.es/2016/12/start-using-whole-disk-images-with.html
http://teknoarticles.blogspot.com.es/2016/12/how-to-encrypt-your-home-with-guestfs.html

There is a pending article i need to write, about how to expand the filesystem after deployment, to consume the remaining disk space.

Hi Yollanda
thanks for the alternative,
we actually already have a working solution 
which is generic and was suggested to the community
https://blueprints.launchpad.net/ironic/+spec/generic-image-partition-aproach

i delivered ironic code changes to the related master branch,
while saying that - i do not have time to go further with it on the community currently , maybe later.

Regards

FWIW, we should be able to use any partition created on the root disk as long as the disk uses a GPT label.

We could also create only the root partition on the disk, leave the remaining space unpartitioned and let puppet-ceph deal with creation of the additional partition. Lucas, what do you think would work best?

Hi,

> we actually already have a working solution 
> which is generic and was suggested to the community
> https://blueprints.launchpad.net/ironic/+spec/generic-image-partition-aproach
> 

Thanks for bringing it upstream, one thing to note is that in Ironic upstream we do not use blueprints anymore [0], so I would suggest you to clean up the spec [1] you guys also put up so we can start reviewing it.

[0] http://docs.openstack.org/developer/ironic/dev/code-contribution-guide.html#adding-new-features

[1] https://review.openstack.org/#/c/412078/

> i delivered ironic code changes to the related master branch,
> while saying that - i do not have time to go further with it on the
> community currently , maybe later.
> 

That's unfortunate. Working on a free software project is not just about dumping code into a repository, it requires time and discussion before deciding on things. I like the idea of having better ways to configure the way disks are configured at deploy time and I'm up to review the future changes to the spec when you get more time to work on it.

Cheers,
Lucas

(In reply to Giulio Fidente from comment #11)
> FWIW, we should be able to use any partition created on the root disk as
> long as the disk uses a GPT label.
> 
> We could also create only the root partition on the disk, leave the
> remaining space unpartitioned and let puppet-ceph deal with creation of the
> additional partition. Lucas, what do you think would work best?

As far as i saw in the code
the ironic is just creating the OS part as the last partition
and then after reboot the growpart utility
is expanding the Root disk to take over the rest of the Available space.

This seems to be the design i think and deployers probably expect that behaviour to stay as is.

(In reply to Lucas Alvares Gomes from comment #12)
> Hi,
> 
> > we actually already have a working solution 
> > which is generic and was suggested to the community
> > https://blueprints.launchpad.net/ironic/+spec/generic-image-partition-aproach
> > 
> 
> Thanks for bringing it upstream, one thing to note is that in Ironic
> upstream we do not use blueprints anymore [0], so I would suggest you to
> clean up the spec [1] you guys also put up so we can start reviewing it.
> 
> [0]
> http://docs.openstack.org/developer/ironic/dev/code-contribution-guide.
> html#adding-new-features
> 
> [1] https://review.openstack.org/#/c/412078/
> 
> > i delivered ironic code changes to the related master branch,
> > while saying that - i do not have time to go further with it on the
> > community currently , maybe later.
> > 
> 
> That's unfortunate. Working on a free software project is not just about
> dumping code into a repository, it requires time and discussion before
> deciding on things. I like the idea of having better ways to configure the
> way disks are configured at deploy time and I'm up to review the future
> changes to the spec when you get more time to work on it.
> 
> Cheers,
> Lucas
Sorry lucas for that, i'll get back to it ASAP BTW i started with a bug and then i was told this is an Enhancment request 
thx

(In reply to Lucas Alvares Gomes from comment #12)
> Hi,
> 
> > we actually already have a working solution 
> > which is generic and was suggested to the community
> > https://blueprints.launchpad.net/ironic/+spec/generic-image-partition-aproach
> > 
> 
> Thanks for bringing it upstream, one thing to note is that in Ironic
> upstream we do not use blueprints anymore [0], so I would suggest you to
> clean up the spec [1] you guys also put up so we can start reviewing it.
> 
> [0]
> http://docs.openstack.org/developer/ironic/dev/code-contribution-guide.
> html#adding-new-features
> 
> [1] https://review.openstack.org/#/c/412078/
> 
> > i delivered ironic code changes to the related master branch,
> > while saying that - i do not have time to go further with it on the
> > community currently , maybe later.
> > 
> 
> That's unfortunate. Working on a free software project is not just about
> dumping code into a repository, it requires time and discussion before
> deciding on things. I like the idea of having better ways to configure the
> way disks are configured at deploy time and I'm up to review the future
> changes to the spec when you get more time to work on it.
> 
> Cheers,
> Lucas
Sorry lucas for that, i'll get back to it ASAP BTW i started with a bug and then i was told this is an Enhancement request 
thx

This work depends on the ironic deployment steps.

Hi! We're thinking of using the ansible deploy interface (https://bugzilla.redhat.com/show_bug.cgi?id=1479814) to handle this kind of customizations. Yolanda, you seem to be looking at a similar request already, right?

Hi Dmitry, in that case, whole disk images look as a good solution. Why involve ansible deploy driver there? Is something that needs to be automated, and cannot be achieved by an image customization converting to whole disk?

Closing this feature request. 
If this feature is still required, 
please re-open it while providing an updated business justification.

Ramon, this is a very substantial work item. Could you (re-)evaluate its priority for OSP?

*** Bug 1644671 has been marked as a duplicate of this bug. ***

removing from osp17, capacity, clarity of requirement, size of effort, can't commit

The LVM partitioning capabilities of RFE bug #1964179 meets the needs of this RFE, so we're closing this as a duplicate. There may need to be a new RFE raised with more focused requirements which can build off the whole-disk capability.

*** This bug has been marked as a duplicate of bug 1964179 ***

The needinfo request[s] on this closed bug have been removed as they have been unresolved for 120 days