[libvirt] [PATCH 0/2] Fix problem generating partition names for multipath device
Originally posted as an RFC: http://www.redhat.com/archives/libvir-list/2015-December/msg00200.html I've made further adjustments/changes and actually tested in a multipath environment configured with the issue. For these patches, rather than making a "general" flag for the pool, I chose to add an attribute to the <source> 'device'. This way it was much more clear what the attribute was affecting. Additionally, it turned out that the issue was showing up for both 'user_friendly_names' and for custom 'alias' name entries in the /etc/multipath.conf file so rather than being a user_friendly_name type flag - I just chose to specific state what it was - a way to not have the partition separator flag "p" added to the generated name. Also it was a bit more difficult to describe the use of the flag especially when it was so specific to one backing store type. John Ferlan (2): conf: Add storage pool device attribute part_separator storage: Add new flag for libvirt_parthelper docs/formatstorage.html.in | 30 ++++++++++++++++++++-- docs/schemas/storagepool.rng | 5 ++++ src/conf/storage_conf.c | 26 +++++++++++++++++-- src/conf/storage_conf.h | 3 ++- src/storage/parthelper.c | 15 ++++++++--- src/storage/storage_backend_disk.c | 11 +++++++- .../pool-disk-device-nopartsep.xml | 14 ++++++++++ .../pool-disk-device-nopartsep.xml | 14 ++++++++++ tests/storagepoolxml2xmltest.c | 1 + 9 files changed, 110 insertions(+), 9 deletions(-) create mode 100644 tests/storagepoolxml2xmlin/pool-disk-device-nopartsep.xml create mode 100644 tests/storagepoolxml2xmlout/pool-disk-device-nopartsep.xml -- 2.5.0
Add a new storage pool source device attribute 'part_separator=[yes|no]' in order to allow a 'disk' storage pool using a device mapper multipath device to not add the "p" partition separator to the generated device name when libvirt_parthelper is run. This will allow libvirt to find device mapper multipath devices which were configured in /etc/multipath.conf to use 'user_friendly_names' or custom 'alias' names for the LUN. Signed-off-by: John Ferlan <jferlan@redhat.com> --- docs/formatstorage.html.in | 30 ++++++++++++++++++++-- docs/schemas/storagepool.rng | 5 ++++ src/conf/storage_conf.c | 26 +++++++++++++++++-- src/conf/storage_conf.h | 3 ++- .../pool-disk-device-nopartsep.xml | 14 ++++++++++ .../pool-disk-device-nopartsep.xml | 14 ++++++++++ tests/storagepoolxml2xmltest.c | 1 + 7 files changed, 88 insertions(+), 5 deletions(-) create mode 100644 tests/storagepoolxml2xmlin/pool-disk-device-nopartsep.xml create mode 100644 tests/storagepoolxml2xmlout/pool-disk-device-nopartsep.xml diff --git a/docs/formatstorage.html.in b/docs/formatstorage.html.in index a60e05e..4965a4c 100644 --- a/docs/formatstorage.html.in +++ b/docs/formatstorage.html.in @@ -90,6 +90,14 @@ <pre> ... <source> + <device path='/dev/mapper/mpatha' part_separator='no'/> + <format type='gpt'/> + </source> + ...</pre> + + <pre> + ... + <source> <adapter type='scsi_host' name='scsi_host1'/> </source> ...</pre> @@ -118,10 +126,28 @@ (pool types <code>fs</code>, <code>logical</code>, <code>disk</code>, <code>iscsi</code>, <code>zfs</code>). May be repeated multiple times depending on backend driver. Contains - a single attribute <code>path</code> which is either the fully + a required attribute <code>path</code> which is either the fully qualified path to the block device node or for <code>iscsi</code> the iSCSI Qualified Name (IQN). - <span class="since">Since 0.4.1</span></dd> + <span class="since">Since 0.4.1</span> + <p>An optional attribute <code>part_separator</code> for each + <code>path</code> may be supplied. Valid values for the attribute + may be either "yes" or "no". This attribute is to be used for a + <code>disk</code> pool type using a <code>path</code> to a + device mapper multipath device configured to utilize either + 'user_friendly_names' or a custom 'alias' name in the + /etc/multipath.conf. The attribute directs libvirt to not + generate device volume names with the partition character "p". + By default, when libvirt generates the partition names for + device mapper multipath devices it will add a "p" path separator + to the device name before adding the partition number. For example, + a <code>device path</code> of '/dev/mapper/mpatha' libvirt would + generate partition names of '/dev/mapper/mpathap1', + '/dev/mapper/mpathap2', etc. for each partition found. With + this attribute set to "no", libvirt will not append the "p" to + the name unless it ends with a number thus generating names + of '/dev/mapper/mpatha1', '/dev/mapper/mpatha2', etc. + <span class="since">Since 1.3.1</span></p></dd> <dt><code>dir</code></dt> <dd>Provides the source for pools backed by directories (pool types <code>dir</code>, <code>netfs</code>, <code>gluster</code>), diff --git a/docs/schemas/storagepool.rng b/docs/schemas/storagepool.rng index 3a61f04..49d212f 100644 --- a/docs/schemas/storagepool.rng +++ b/docs/schemas/storagepool.rng @@ -278,6 +278,11 @@ <empty/> <ref name='devextents'/> </choice> + <optional> + <attribute name="part_separator"> + <ref name="virYesNo"/> + </attribute> + </optional> </element> </define> diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 7c81bab..3657dfd 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -1,7 +1,7 @@ /* * storage_conf.c: config handling for storage driver * - * Copyright (C) 2006-2014 Red Hat, Inc. + * Copyright (C) 2006-2016 Red Hat, Inc. * Copyright (C) 2006-2008 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -528,6 +528,7 @@ virStoragePoolDefParseSource(xmlXPathContextPtr ctxt, goto cleanup; for (i = 0; i < nsource; i++) { + char *partsep; virStoragePoolSourceDevice dev = { .path = NULL }; dev.path = virXMLPropString(nodeset[i], "path"); @@ -537,10 +538,25 @@ virStoragePoolDefParseSource(xmlXPathContextPtr ctxt, goto cleanup; } + partsep = virXMLPropString(nodeset[i], "part_separator"); + if (partsep) { + dev.part_separator = virTristateBoolTypeFromString(partsep); + if (dev.part_separator <= 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("invalid part_separator setting '%s'"), + partsep); + virStoragePoolSourceDeviceClear(&dev); + VIR_FREE(partsep); + goto cleanup; + } + VIR_FREE(partsep); + } + if (VIR_APPEND_ELEMENT(source->devices, source->ndevice, dev) < 0) { virStoragePoolSourceDeviceClear(&dev); goto cleanup; } + } source->dir = virXPathString("string(./dir/@path)", ctxt); @@ -1051,8 +1067,14 @@ virStoragePoolSourceFormat(virBufferPtr buf, virBufferAdjustIndent(buf, -2); virBufferAddLit(buf, "</device>\n"); } else { - virBufferEscapeString(buf, "<device path='%s'/>\n", + virBufferEscapeString(buf, "<device path='%s'", src->devices[i].path); + if (src->devices[i].part_separator != + VIR_TRISTATE_SWITCH_ABSENT) { + virBufferAsprintf(buf, " part_separator='%s'", + virTristateBoolTypeToString(src->devices[i].part_separator)); + } + virBufferAddLit(buf, "/>\n"); } } } diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h index ec59c17..f1dc62b 100644 --- a/src/conf/storage_conf.h +++ b/src/conf/storage_conf.h @@ -1,7 +1,7 @@ /* * storage_conf.h: config handling for storage driver * - * Copyright (C) 2006-2008, 2010-2014 Red Hat, Inc. + * Copyright (C) 2006-2008, 2010-2016 Red Hat, Inc. * Copyright (C) 2006-2008 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -157,6 +157,7 @@ struct _virStoragePoolSourceDevice { virStoragePoolSourceDeviceExtentPtr freeExtents; char *path; int format; /* Pool specific source format */ + int part_separator; /* enum virTristateSwitch */ /* When the source device is a physical disk, * the geometry data is needed diff --git a/tests/storagepoolxml2xmlin/pool-disk-device-nopartsep.xml b/tests/storagepoolxml2xmlin/pool-disk-device-nopartsep.xml new file mode 100644 index 0000000..71b381e --- /dev/null +++ b/tests/storagepoolxml2xmlin/pool-disk-device-nopartsep.xml @@ -0,0 +1,14 @@ +<pool type='disk'> + <name>multipath</name> + <uuid>70a7eb15-6c34-ee9c-bf57-69e8e5ff3fb2</uuid> + <capacity unit='bytes'>0</capacity> + <allocation unit='bytes'>0</allocation> + <available unit='bytes'>0</available> + <source> + <device path='/dev/mapper/mpatha' part_separator='no'/> + <format type='gpt'/> + </source> + <target> + <path>/dev</path> + </target> +</pool> diff --git a/tests/storagepoolxml2xmlout/pool-disk-device-nopartsep.xml b/tests/storagepoolxml2xmlout/pool-disk-device-nopartsep.xml new file mode 100644 index 0000000..71b381e --- /dev/null +++ b/tests/storagepoolxml2xmlout/pool-disk-device-nopartsep.xml @@ -0,0 +1,14 @@ +<pool type='disk'> + <name>multipath</name> + <uuid>70a7eb15-6c34-ee9c-bf57-69e8e5ff3fb2</uuid> + <capacity unit='bytes'>0</capacity> + <allocation unit='bytes'>0</allocation> + <available unit='bytes'>0</available> + <source> + <device path='/dev/mapper/mpatha' part_separator='no'/> + <format type='gpt'/> + </source> + <target> + <path>/dev</path> + </target> +</pool> diff --git a/tests/storagepoolxml2xmltest.c b/tests/storagepoolxml2xmltest.c index b03c4b0..41d6987 100644 --- a/tests/storagepoolxml2xmltest.c +++ b/tests/storagepoolxml2xmltest.c @@ -80,6 +80,7 @@ mymain(void) DO_TEST("pool-logical-nopath"); DO_TEST("pool-logical-create"); DO_TEST("pool-disk"); + DO_TEST("pool-disk-device-nopartsep"); DO_TEST("pool-iscsi"); DO_TEST("pool-iscsi-auth"); DO_TEST("pool-netfs"); -- 2.5.0
https://bugzilla.redhat.com/show_bug.cgi?id=1265694 In order to be able to process disk storage pool's using a multipath device to handle the partitions, libvirt_parthelper will need a way to not automatically add a partition separator "p" to the generated device name for each partition found. This is designed to mimic the multipath features known as 'user_friendly_names' and custom 'alias' name. If the part_separator attribute is set to "no", then generation of the multipath partition name will not include the "p" partition separator unless the source device path name ends with a number. The generated partition names that get passed back to libvirt are processed in order to find the device mapper multipath (dm-#) path device. For example, device path "/dev/mapper/mpatha" would create partitions "/dev/mapper/mpatha1", "/dev/mapper/mpatha2", etc. instead of "/dev/mapper/mpathap1", "/dev/mapper/mpathap2", etc. If the device path ends with a number "/dev/mapper/mpatha1", then the algorithm to generate names "/dev/mapper/mpatha1p1", "/dev/mapper/mpatha1p2", etc. would be utilized. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/parthelper.c | 15 ++++++++++++--- src/storage/storage_backend_disk.c | 11 ++++++++++- 2 files changed, 22 insertions(+), 4 deletions(-) diff --git a/src/storage/parthelper.c b/src/storage/parthelper.c index 8de32fd..d1df068 100644 --- a/src/storage/parthelper.c +++ b/src/storage/parthelper.c @@ -10,7 +10,7 @@ * in a reliable fashion if merely after a list of partitions & sizes, * though it is fine for creating partitions. * - * Copyright (C) 2007-2008, 2010, 2013 Red Hat, Inc. + * Copyright (C) 2007-2008, 2010, 2013, 2016 Red Hat, Inc. * Copyright (C) 2007-2008 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -70,6 +70,7 @@ int main(int argc, char **argv) const char *path; char *canonical_path; const char *partsep; + bool devmap_nopartsep = false; if (setlocale(LC_ALL, "") == NULL || bindtextdomain(PACKAGE, LOCALEDIR) == NULL || @@ -80,14 +81,22 @@ int main(int argc, char **argv) if (argc == 3 && STREQ(argv[2], "-g")) { cmd = DISK_GEOMETRY; + } else if (argc == 3 && STREQ(argv[2], "-p")) { + devmap_nopartsep = true; } else if (argc != 2) { - fprintf(stderr, _("syntax: %s DEVICE [-g]\n"), argv[0]); + fprintf(stderr, _("syntax: %s DEVICE [-g]|[-p]\n"), argv[0]); return 1; } path = argv[1]; if (virIsDevMapperDevice(path)) { - partsep = "p"; + /* The 'devmap_nopartsep' option will not append the partsep of "p" + * onto the name unless the 'path' ends in a number + */ + if (c_isdigit(path[strlen(path)-1]) || !devmap_nopartsep) + partsep = "p"; + else + partsep = ""; if (VIR_STRDUP_QUIET(canonical_path, path) < 0) return 2; } else { diff --git a/src/storage/storage_backend_disk.c b/src/storage/storage_backend_disk.c index a83e340..3e0395d 100644 --- a/src/storage/storage_backend_disk.c +++ b/src/storage/storage_backend_disk.c @@ -1,7 +1,7 @@ /* * storage_backend_disk.c: storage backend for disk handling * - * Copyright (C) 2007-2014 Red Hat, Inc. + * Copyright (C) 2007-2016 Red Hat, Inc. * Copyright (C) 2007-2008 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -325,6 +325,15 @@ virStorageBackendDiskReadPartitions(virStoragePoolObjPtr pool, pool->def->source.devices[0].path, NULL); + /* Check for the presence of the part_separator='no'. Pass this + * along to the libvirt_parthelper as option '-p'. This will cause + * libvirt_parthelper to not append the "p" partition separator to + * the generated device name, unless the name ends with a number. + */ + if (pool->def->source.devices[0].part_separator == + VIR_TRISTATE_BOOL_NO) + virCommandAddArg(cmd, "-p"); + /* If a volume is passed, virStorageBackendDiskMakeVol only updates the * pool allocation for that single volume. */ -- 2.5.0
On 08.01.2016 18:09, John Ferlan wrote:
Originally posted as an RFC:
http://www.redhat.com/archives/libvir-list/2015-December/msg00200.html
It hasn't gotten much attention, has it? Sorry.
I've made further adjustments/changes and actually tested in a multipath environment configured with the issue.
For these patches, rather than making a "general" flag for the pool, I chose to add an attribute to the <source> 'device'. This way it was much more clear what the attribute was affecting.
Additionally, it turned out that the issue was showing up for both 'user_friendly_names' and for custom 'alias' name entries in the /etc/multipath.conf file so rather than being a user_friendly_name type flag - I just chose to specific state what it was - a way to not have the partition separator flag "p" added to the generated name. Also it was a bit more difficult to describe the use of the flag especially when it was so specific to one backing store type.
John Ferlan (2): conf: Add storage pool device attribute part_separator storage: Add new flag for libvirt_parthelper
docs/formatstorage.html.in | 30 ++++++++++++++++++++-- docs/schemas/storagepool.rng | 5 ++++ src/conf/storage_conf.c | 26 +++++++++++++++++-- src/conf/storage_conf.h | 3 ++- src/storage/parthelper.c | 15 ++++++++--- src/storage/storage_backend_disk.c | 11 +++++++- .../pool-disk-device-nopartsep.xml | 14 ++++++++++ .../pool-disk-device-nopartsep.xml | 14 ++++++++++ tests/storagepoolxml2xmltest.c | 1 + 9 files changed, 110 insertions(+), 9 deletions(-) create mode 100644 tests/storagepoolxml2xmlin/pool-disk-device-nopartsep.xml create mode 100644 tests/storagepoolxml2xmlout/pool-disk-device-nopartsep.xml
ACK to both. But I'd push it after the release. Michal
On 01/15/2016 10:06 AM, Michal Privoznik wrote:
On 08.01.2016 18:09, John Ferlan wrote:
Originally posted as an RFC:
http://www.redhat.com/archives/libvir-list/2015-December/msg00200.html
It hasn't gotten much attention, has it? Sorry.
I've made further adjustments/changes and actually tested in a multipath environment configured with the issue.
For these patches, rather than making a "general" flag for the pool, I chose to add an attribute to the <source> 'device'. This way it was much more clear what the attribute was affecting.
Additionally, it turned out that the issue was showing up for both 'user_friendly_names' and for custom 'alias' name entries in the /etc/multipath.conf file so rather than being a user_friendly_name type flag - I just chose to specific state what it was - a way to not have the partition separator flag "p" added to the generated name. Also it was a bit more difficult to describe the use of the flag especially when it was so specific to one backing store type.
John Ferlan (2): conf: Add storage pool device attribute part_separator storage: Add new flag for libvirt_parthelper
docs/formatstorage.html.in | 30 ++++++++++++++++++++-- docs/schemas/storagepool.rng | 5 ++++ src/conf/storage_conf.c | 26 +++++++++++++++++-- src/conf/storage_conf.h | 3 ++- src/storage/parthelper.c | 15 ++++++++--- src/storage/storage_backend_disk.c | 11 +++++++- .../pool-disk-device-nopartsep.xml | 14 ++++++++++ .../pool-disk-device-nopartsep.xml | 14 ++++++++++ tests/storagepoolxml2xmltest.c | 1 + 9 files changed, 110 insertions(+), 9 deletions(-) create mode 100644 tests/storagepoolxml2xmlin/pool-disk-device-nopartsep.xml create mode 100644 tests/storagepoolxml2xmlout/pool-disk-device-nopartsep.xml
ACK to both. But I'd push it after the release.
Now pushed - thanks. John
participants (2)
-
John Ferlan -
Michal Privoznik