[pve-devel] [RFC/PATCH qemu] fix #3084: fall back to open-iscsi initiatorname

[pve-devel] [RFC/PATCH qemu] fix #3084: fall back to open-iscsi initiatorname

[Fabian Ebner]

Fixes vma restore when the target is an iSCSI storage which expects that
initiatorname. Also avoids the need to always explicitly set the initiatorname
in PVE code, thus fixing moving efidisks from and to such iSCSI storages.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---

The obvious alternative is to add the option in vma itself. But I'd like to
hear some opinions about this approach first. One downside is that the old
default behavior (i.e. using iqn.2008-11.org.linux-kvmXXX) is overwritten as
long as an initiatorname can be read from /etc/iscsi/initiatorname.iscsi, but
as PVE only reads the initiatorname from there, relying on that behavior seems
to be incompatible with our tooling anyways.

 ...all-back-to-open-iscsi-initiatorname.patch | 71 +++++++++++++++++++
 debian/patches/series                         |  1 +
 2 files changed, 72 insertions(+)
 create mode 100644 debian/patches/pve/0057-PVE-fall-back-to-open-iscsi-initiatorname.patch

diff --git a/debian/patches/pve/0057-PVE-fall-back-to-open-iscsi-initiatorname.patch b/debian/patches/pve/0057-PVE-fall-back-to-open-iscsi-initiatorname.patch
new file mode 100644
index 0000000..9f0e822
--- /dev/null
+++ b/debian/patches/pve/0057-PVE-fall-back-to-open-iscsi-initiatorname.patch
@@ -0,0 +1,71 @@
+From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
+From: Fabian Ebner <f.ebner@proxmox.com>
+Date: Tue, 17 Nov 2020 10:51:05 +0100
+Subject: [PATCH] PVE: fall back to open-iscsi initiatorname
+
+When no explicit option is given, try reading the initiator name from
+/etc/iscsi/initiatorname.iscsi and only use the generic fallback, i.e.
+iqn.2008-11.org.linux-kvmXXX, as a third alternative.
+
+This avoids the need to add an explicit option for vma and to explicitly set it
+for each call to qemu that deals with iSCSI disks, while still allowing to set
+the option if a different name is needed.
+
+According to RFC 3720, an initiator name is at most 223 bytes long, so the
+4 KiB buffer is big enough, even if many whitespaces are used.
+
+Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
+---
+ block/iscsi.c | 30 ++++++++++++++++++++++++++++++
+ 1 file changed, 30 insertions(+)
+
+diff --git a/block/iscsi.c b/block/iscsi.c
+index bd2122a3a4..56437d8b99 100644
+--- a/block/iscsi.c
++++ b/block/iscsi.c
+@@ -1374,12 +1374,42 @@ static char *get_initiator_name(QemuOpts *opts)
+     const char *name;
+     char *iscsi_name;
+     UuidInfo *uuid_info;
++    FILE *name_fh;
+ 
+     name = qemu_opt_get(opts, "initiator-name");
+     if (name) {
+         return g_strdup(name);
+     }
+ 
++    name_fh = fopen("/etc/iscsi/initiatorname.iscsi", "r");
++    if (name_fh) {
++        const char *key = "InitiatorName";
++        char buffer[4096];
++        char *line;
++
++        while ((line = fgets(buffer, sizeof(buffer), name_fh))) {
++            line = g_strstrip(line);
++            if (!strncmp(line, key, strlen(key))) {
++                line = strchr(line, '=');
++                if (!line || strlen(line) == 1) {
++                    continue;
++                }
++                line++;
++                g_strstrip(line);
++                if (!strlen(line)) {
++                    continue;
++                }
++                name = line;
++                break;
++            }
++        }
++        fclose(name_fh);
++
++        if (name) {
++            return g_strdup(name);
++        }
++    }
++
+     uuid_info = qmp_query_uuid(NULL);
+     if (strcmp(uuid_info->UUID, UUID_NONE) == 0) {
+         name = qemu_get_vm_name();
+-- 
+2.20.1
+
diff --git a/debian/patches/series b/debian/patches/series
index 0b4ba60..604352b 100644
--- a/debian/patches/series
+++ b/debian/patches/series
@@ -57,3 +57,4 @@ pve/0053-PVE-fix-and-clean-up-error-handling-for-create_backu.patch
 pve/0054-migration-block-dirty-bitmap-fix-larger-granularity-.patch
 pve/0055-PVE-Migrate-dirty-bitmap-state-via-savevm.patch
 pve/0056-migration-block-dirty-bitmap-migrate-other-bitmaps-e.patch
+pve/0057-PVE-fall-back-to-open-iscsi-initiatorname.patch
-- 
2.20.1

Re: [pve-devel] [RFC/PATCH qemu] fix #3084: fall back to open-iscsi initiatorname

[Dominik Csapak]

idea sounds good, patch looks sane (compiled it, but did not test it 
though, since i have no iscsi around atm)

the only thing that 'could' go wrong afaics, is that we maybe
have some codepaths were we only use the default initiatorname
and some setups could not do that anymore

but this would mean they could not start a vm in the first place...
so not an issue

Reviewed-By: Dominik Csapak <d.csapak@proxmox.com>

On 11/17/20 12:38 PM, Fabian Ebner wrote:
> Fixes vma restore when the target is an iSCSI storage which expects that
> initiatorname. Also avoids the need to always explicitly set the initiatorname
> in PVE code, thus fixing moving efidisks from and to such iSCSI storages.
> 
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---
> 
> The obvious alternative is to add the option in vma itself. But I'd like to
> hear some opinions about this approach first. One downside is that the old
> default behavior (i.e. using iqn.2008-11.org.linux-kvmXXX) is overwritten as
> long as an initiatorname can be read from /etc/iscsi/initiatorname.iscsi, but
> as PVE only reads the initiatorname from there, relying on that behavior seems
> to be incompatible with our tooling anyways.
> 
>   ...all-back-to-open-iscsi-initiatorname.patch | 71 +++++++++++++++++++
>   debian/patches/series                         |  1 +
>   2 files changed, 72 insertions(+)
>   create mode 100644 debian/patches/pve/0057-PVE-fall-back-to-open-iscsi-initiatorname.patch
> 
> diff --git a/debian/patches/pve/0057-PVE-fall-back-to-open-iscsi-initiatorname.patch b/debian/patches/pve/0057-PVE-fall-back-to-open-iscsi-initiatorname.patch
> new file mode 100644
> index 0000000..9f0e822
> --- /dev/null
> +++ b/debian/patches/pve/0057-PVE-fall-back-to-open-iscsi-initiatorname.patch
> @@ -0,0 +1,71 @@
> +From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
> +From: Fabian Ebner <f.ebner@proxmox.com>
> +Date: Tue, 17 Nov 2020 10:51:05 +0100
> +Subject: [PATCH] PVE: fall back to open-iscsi initiatorname
> +
> +When no explicit option is given, try reading the initiator name from
> +/etc/iscsi/initiatorname.iscsi and only use the generic fallback, i.e.
> +iqn.2008-11.org.linux-kvmXXX, as a third alternative.
> +
> +This avoids the need to add an explicit option for vma and to explicitly set it
> +for each call to qemu that deals with iSCSI disks, while still allowing to set
> +the option if a different name is needed.
> +
> +According to RFC 3720, an initiator name is at most 223 bytes long, so the
> +4 KiB buffer is big enough, even if many whitespaces are used.
> +
> +Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> +---
> + block/iscsi.c | 30 ++++++++++++++++++++++++++++++
> + 1 file changed, 30 insertions(+)
> +
> +diff --git a/block/iscsi.c b/block/iscsi.c
> +index bd2122a3a4..56437d8b99 100644
> +--- a/block/iscsi.c
> ++++ b/block/iscsi.c
> +@@ -1374,12 +1374,42 @@ static char *get_initiator_name(QemuOpts *opts)
> +     const char *name;
> +     char *iscsi_name;
> +     UuidInfo *uuid_info;
> ++    FILE *name_fh;
> +
> +     name = qemu_opt_get(opts, "initiator-name");
> +     if (name) {
> +         return g_strdup(name);
> +     }
> +
> ++    name_fh = fopen("/etc/iscsi/initiatorname.iscsi", "r");
> ++    if (name_fh) {
> ++        const char *key = "InitiatorName";
> ++        char buffer[4096];
> ++        char *line;
> ++
> ++        while ((line = fgets(buffer, sizeof(buffer), name_fh))) {
> ++            line = g_strstrip(line);
> ++            if (!strncmp(line, key, strlen(key))) {
> ++                line = strchr(line, '=');
> ++                if (!line || strlen(line) == 1) {
> ++                    continue;
> ++                }
> ++                line++;
> ++                g_strstrip(line);
> ++                if (!strlen(line)) {
> ++                    continue;
> ++                }
> ++                name = line;
> ++                break;
> ++            }
> ++        }
> ++        fclose(name_fh);
> ++
> ++        if (name) {
> ++            return g_strdup(name);
> ++        }
> ++    }
> ++
> +     uuid_info = qmp_query_uuid(NULL);
> +     if (strcmp(uuid_info->UUID, UUID_NONE) == 0) {
> +         name = qemu_get_vm_name();
> +--
> +2.20.1
> +
> diff --git a/debian/patches/series b/debian/patches/series
> index 0b4ba60..604352b 100644
> --- a/debian/patches/series
> +++ b/debian/patches/series
> @@ -57,3 +57,4 @@ pve/0053-PVE-fix-and-clean-up-error-handling-for-create_backu.patch
>   pve/0054-migration-block-dirty-bitmap-fix-larger-granularity-.patch
>   pve/0055-PVE-Migrate-dirty-bitmap-state-via-savevm.patch
>   pve/0056-migration-block-dirty-bitmap-migrate-other-bitmaps-e.patch
> +pve/0057-PVE-fall-back-to-open-iscsi-initiatorname.patch
> 

[pve-devel] applied: [RFC/PATCH qemu] fix #3084: fall back to open-iscsi initiatorname

[Thomas Lamprecht]

On 17.11.20 12:38, Fabian Ebner wrote:
> Fixes vma restore when the target is an iSCSI storage which expects that
> initiatorname. Also avoids the need to always explicitly set the initiatorname
> in PVE code, thus fixing moving efidisks from and to such iSCSI storages.
> 
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---
> 
> The obvious alternative is to add the option in vma itself. But I'd like to
> hear some opinions about this approach first. One downside is that the old
> default behavior (i.e. using iqn.2008-11.org.linux-kvmXXX) is overwritten as
> long as an initiatorname can be read from /etc/iscsi/initiatorname.iscsi, but
> as PVE only reads the initiatorname from there, relying on that behavior seems
> to be incompatible with our tooling anyways.
> 
>  ...all-back-to-open-iscsi-initiatorname.patch | 71 +++++++++++++++++++
>  debian/patches/series                         |  1 +
>  2 files changed, 72 insertions(+)
>  create mode 100644 debian/patches/pve/0057-PVE-fall-back-to-open-iscsi-initiatorname.patch
> 
>

applied, thanks!

Had to resolve a trivial merge-conflict as there was a 0057 patch applied in
between, so this is now 0058.