[libvirt] [PATCH 0/2] Use static configuration method for iSCSI target discovery
https://bugzilla.redhat.com/show_bug.cgi?id=1356436 Details in patches. John Ferlan (2): util: Introduce virISCSINodeNew iscsi: Establish connection to target via static target login src/libvirt_private.syms | 1 + src/storage/storage_backend_iscsi.c | 9 ++++---- src/util/viriscsi.c | 45 +++++++++++++++++++++++++++++++++++++ src/util/viriscsi.h | 6 +++++ 4 files changed, 56 insertions(+), 5 deletions(-) -- 2.5.5
https://bugzilla.redhat.com/show_bug.cgi?id=1356436 According to RFC 3721 (https://www.ietf.org/rfc/rfc3721.txt), there are two ways to "discover" targets in/for the iSCSI environment. Discovery is the process which allows the initiator to find the targets to which it has access and at least one address at which each target may be accessed. The method currently implemented in libvirt using the virISCSIScanTargets API is known as "SendTargets" discovery. This method is more useful when the target IP Address and TCP port information are available, e.g. in libvirt terms the "portal". It returns a list of targets for the portal.
From that list, the target can be found. This operation can also fill an iSCSI node table into which iSCSI logins may occur. Commit id '56057900' altered that filling by adding the "--op nonpersistent" since it was not necessarily desired to perform that for non libvirt related targets.
The second method is "Static Configuration". This method not only needs the IP Address and TCP port (e.g. portal), but also the iSCSI target name. In libvirt terms this would be the device path field from the iSCSI pool <source> XML. This patch implements the second methodology using that required device path as the targetname. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/libvirt_private.syms | 1 + src/util/viriscsi.c | 45 +++++++++++++++++++++++++++++++++++++++++++++ src/util/viriscsi.h | 6 ++++++ 3 files changed, 52 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 3580a72..edef70b 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1714,6 +1714,7 @@ iptablesRemoveUdpOutput; virISCSIConnectionLogin; virISCSIConnectionLogout; virISCSIGetSession; +virISCSINodeNew; virISCSINodeUpdate; virISCSIRescanLUNs; virISCSIScanTargets; diff --git a/src/util/viriscsi.c b/src/util/viriscsi.c index e705517..8115d09 100644 --- a/src/util/viriscsi.c +++ b/src/util/viriscsi.c @@ -444,6 +444,51 @@ virISCSIScanTargets(const char *portal, return ret; } +/* + * virISCSINodeNew: + * @portal: address for iSCSI target + * @target: IQN and specific LUN target + * + * Usage of nonpersistent discovery in virISCSIScanTargets is useful primarily + * only when the target IQN is not known; however, since we have the target IQN + * usage of the "--op new" can be done. This avoids problems if "--op delete" + * had been used wiping out the static nodes determined by the scanning of + * all targets. + * + * NB: If already created, subsequent "--op new" commands do not return + * an error. + * + * Returns 0 on success, -1 w/ error message on error + */ +int +virISCSINodeNew(const char *portal, + const char *target) +{ + virCommandPtr cmd = NULL; + int status; + int ret = -1; + + cmd = virCommandNewArgList(ISCSIADM, + "--mode", "node", + "--portal", portal, + "--targetname", target, + "--op", "new", + NULL); + + /* Ignore non-zero status. */ + if (virCommandRun(cmd, &status) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed new node mode for target '%s'"), + target); + goto cleanup; + } + + ret = 0; + cleanup: + virCommandFree(cmd); + return ret; +} + int virISCSINodeUpdate(const char *portal, diff --git a/src/util/viriscsi.h b/src/util/viriscsi.h index 459249e..a44beea 100644 --- a/src/util/viriscsi.h +++ b/src/util/viriscsi.h @@ -54,6 +54,12 @@ virISCSIScanTargets(const char *portal, ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK; int +virISCSINodeNew(const char *portal, + const char *target) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) + ATTRIBUTE_RETURN_CHECK; + +int virISCSINodeUpdate(const char *portal, const char *target, const char *name, -- 2.5.5
On Mon, Jul 18, 2016 at 07:37:20AM -0400, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1356436
According to RFC 3721 (https://www.ietf.org/rfc/rfc3721.txt), there are two ways to "discover" targets in/for the iSCSI environment. Discovery is the process which allows the initiator to find the targets to which it has access and at least one address at which each target may be accessed.
The method currently implemented in libvirt using the virISCSIScanTargets API is known as "SendTargets" discovery. This method is more useful when the target IP Address and TCP port information are available, e.g. in libvirt terms the "portal". It returns a list of targets for the portal.
From that list, the target can be found. This operation can also fill an iSCSI node table into which iSCSI logins may occur. Commit id '56057900' altered that filling by adding the "--op nonpersistent" since it was not necessarily desired to perform that for non libvirt related targets.
The second method is "Static Configuration". This method not only needs the IP Address and TCP port (e.g. portal), but also the iSCSI target name. In libvirt terms this would be the device path field from the iSCSI pool <source> XML. This patch implements the second methodology using that required device path as the targetname.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/libvirt_private.syms | 1 + src/util/viriscsi.c | 45 +++++++++++++++++++++++++++++++++++++++++++++ src/util/viriscsi.h | 6 ++++++ 3 files changed, 52 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 3580a72..edef70b 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1714,6 +1714,7 @@ iptablesRemoveUdpOutput; virISCSIConnectionLogin; virISCSIConnectionLogout; virISCSIGetSession; +virISCSINodeNew; virISCSINodeUpdate; virISCSIRescanLUNs; virISCSIScanTargets; diff --git a/src/util/viriscsi.c b/src/util/viriscsi.c index e705517..8115d09 100644 --- a/src/util/viriscsi.c +++ b/src/util/viriscsi.c @@ -444,6 +444,51 @@ virISCSIScanTargets(const char *portal, return ret; }
+/* + * virISCSINodeNew: + * @portal: address for iSCSI target + * @target: IQN and specific LUN target + * + * Usage of nonpersistent discovery in virISCSIScanTargets is useful primarily + * only when the target IQN is not known; however, since we have the target IQN + * usage of the "--op new" can be done. This avoids problems if "--op delete" + * had been used wiping out the static nodes determined by the scanning of + * all targets. + * + * NB: If already created, subsequent "--op new" commands do not return + * an error.
If it does not return an error, do we need to ignore non-zero status? AFAIK the rest of the code ignoring exit codes was written before iscsiadm returned them properly: https://github.com/open-iscsi/open-iscsi/commit/2c839a2 Even this libvirt commit from Jan 2010 speaks of 'older versions of iscsiadm': http://libvirt.org/git/?p=libvirt.git;a=commitdiff;h=6aabcb5b Jan
On 07/26/2016 12:24 PM, Ján Tomko wrote:
On Mon, Jul 18, 2016 at 07:37:20AM -0400, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1356436
According to RFC 3721 (https://www.ietf.org/rfc/rfc3721.txt), there are two ways to "discover" targets in/for the iSCSI environment. Discovery is the process which allows the initiator to find the targets to which it has access and at least one address at which each target may be accessed.
The method currently implemented in libvirt using the virISCSIScanTargets API is known as "SendTargets" discovery. This method is more useful when the target IP Address and TCP port information are available, e.g. in libvirt terms the "portal". It returns a list of targets for the portal.
From that list, the target can be found. This operation can also fill an iSCSI node table into which iSCSI logins may occur. Commit id '56057900' altered that filling by adding the "--op nonpersistent" since it was not necessarily desired to perform that for non libvirt related targets.
The second method is "Static Configuration". This method not only needs the IP Address and TCP port (e.g. portal), but also the iSCSI target name. In libvirt terms this would be the device path field from the iSCSI pool <source> XML. This patch implements the second methodology using that required device path as the targetname.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/libvirt_private.syms | 1 + src/util/viriscsi.c | 45 +++++++++++++++++++++++++++++++++++++++++++++ src/util/viriscsi.h | 6 ++++++ 3 files changed, 52 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 3580a72..edef70b 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1714,6 +1714,7 @@ iptablesRemoveUdpOutput; virISCSIConnectionLogin; virISCSIConnectionLogout; virISCSIGetSession; +virISCSINodeNew; virISCSINodeUpdate; virISCSIRescanLUNs; virISCSIScanTargets; diff --git a/src/util/viriscsi.c b/src/util/viriscsi.c index e705517..8115d09 100644 --- a/src/util/viriscsi.c +++ b/src/util/viriscsi.c @@ -444,6 +444,51 @@ virISCSIScanTargets(const char *portal, return ret; }
+/* + * virISCSINodeNew: + * @portal: address for iSCSI target + * @target: IQN and specific LUN target + * + * Usage of nonpersistent discovery in virISCSIScanTargets is useful primarily + * only when the target IQN is not known; however, since we have the target IQN + * usage of the "--op new" can be done. This avoids problems if "--op delete" + * had been used wiping out the static nodes determined by the scanning of + * all targets. + * + * NB: If already created, subsequent "--op new" commands do not return + * an error.
If it does not return an error, do we need to ignore non-zero status?
I thought the point of the comment is that subsequent --op new commands won't cause an error "if" the node record was generated. IOW: It's ok to have multiple NodeNew commands and those won't cause an error. If there was some other failure, then I'd expect to get and report some error. But for this particular command the code isn't ignoring any error, so the code isn't ignoring errors... John
AFAIK the rest of the code ignoring exit codes was written before iscsiadm returned them properly: https://github.com/open-iscsi/open-iscsi/commit/2c839a2
Even this libvirt commit from Jan 2010 speaks of 'older versions of iscsiadm': http://libvirt.org/git/?p=libvirt.git;a=commitdiff;h=6aabcb5b
Jan
On Tue, Jul 26, 2016 at 10:25:41PM -0400, John Ferlan wrote:
On 07/26/2016 12:24 PM, Ján Tomko wrote:
On Mon, Jul 18, 2016 at 07:37:20AM -0400, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1356436
According to RFC 3721 (https://www.ietf.org/rfc/rfc3721.txt), there are two ways to "discover" targets in/for the iSCSI environment. Discovery is the process which allows the initiator to find the targets to which it has access and at least one address at which each target may be accessed.
The method currently implemented in libvirt using the virISCSIScanTargets API is known as "SendTargets" discovery. This method is more useful when the target IP Address and TCP port information are available, e.g. in libvirt terms the "portal". It returns a list of targets for the portal.
From that list, the target can be found. This operation can also fill an iSCSI node table into which iSCSI logins may occur. Commit id '56057900' altered that filling by adding the "--op nonpersistent" since it was not necessarily desired to perform that for non libvirt related targets.
The second method is "Static Configuration". This method not only needs the IP Address and TCP port (e.g. portal), but also the iSCSI target name. In libvirt terms this would be the device path field from the iSCSI pool <source> XML. This patch implements the second methodology using that required device path as the targetname.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/libvirt_private.syms | 1 + src/util/viriscsi.c | 45 +++++++++++++++++++++++++++++++++++++++++++++ src/util/viriscsi.h | 6 ++++++ 3 files changed, 52 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 3580a72..edef70b 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1714,6 +1714,7 @@ iptablesRemoveUdpOutput; virISCSIConnectionLogin; virISCSIConnectionLogout; virISCSIGetSession; +virISCSINodeNew; virISCSINodeUpdate; virISCSIRescanLUNs; virISCSIScanTargets; diff --git a/src/util/viriscsi.c b/src/util/viriscsi.c index e705517..8115d09 100644 --- a/src/util/viriscsi.c +++ b/src/util/viriscsi.c @@ -444,6 +444,51 @@ virISCSIScanTargets(const char *portal, return ret; }
+/* + * virISCSINodeNew: + * @portal: address for iSCSI target + * @target: IQN and specific LUN target + * + * Usage of nonpersistent discovery in virISCSIScanTargets is useful primarily + * only when the target IQN is not known; however, since we have the target IQN + * usage of the "--op new" can be done. This avoids problems if "--op delete" + * had been used wiping out the static nodes determined by the scanning of + * all targets. + * + * NB: If already created, subsequent "--op new" commands do not return + * an error.
If it does not return an error, do we need to ignore non-zero status?
I thought the point of the comment is that subsequent --op new commands won't cause an error "if" the node record was generated. IOW: It's ok to have multiple NodeNew commands and those won't cause an error. If there was some other failure, then I'd expect to get and report some error. But for this particular command the code isn't ignoring any error, so the code isn't ignoring errors...
Perhaps I should have written it below the code, not the above comment: + /* Ignore non-zero status. */ + if (virCommandRun(cmd, &status) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed new node mode for target '%s'"), + target); + goto cleanup; + } With a non-NULL second argument, virCommandRun ignores non-zero exit status and expects the caller to deal with it. If we don't need to be compatible with old broken iscsiadm and multiple --op new commands do not return an error, we should error out on non-zero status. Jan
John
AFAIK the rest of the code ignoring exit codes was written before iscsiadm returned them properly: https://github.com/open-iscsi/open-iscsi/commit/2c839a2
Even this libvirt commit from Jan 2010 speaks of 'older versions of iscsiadm': http://libvirt.org/git/?p=libvirt.git;a=commitdiff;h=6aabcb5b
Jan
On 07/27/2016 06:48 AM, Ján Tomko wrote:
On Tue, Jul 26, 2016 at 10:25:41PM -0400, John Ferlan wrote:
On 07/26/2016 12:24 PM, Ján Tomko wrote:
On Mon, Jul 18, 2016 at 07:37:20AM -0400, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1356436
According to RFC 3721 (https://www.ietf.org/rfc/rfc3721.txt), there are two ways to "discover" targets in/for the iSCSI environment. Discovery is the process which allows the initiator to find the targets to which it has access and at least one address at which each target may be accessed.
The method currently implemented in libvirt using the virISCSIScanTargets API is known as "SendTargets" discovery. This method is more useful when the target IP Address and TCP port information are available, e.g. in libvirt terms the "portal". It returns a list of targets for the portal.
From that list, the target can be found. This operation can also fill an iSCSI node table into which iSCSI logins may occur. Commit id '56057900' altered that filling by adding the "--op nonpersistent" since it was not necessarily desired to perform that for non libvirt related targets.
The second method is "Static Configuration". This method not only needs the IP Address and TCP port (e.g. portal), but also the iSCSI target name. In libvirt terms this would be the device path field from the iSCSI pool <source> XML. This patch implements the second methodology using that required device path as the targetname.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/libvirt_private.syms | 1 + src/util/viriscsi.c | 45 +++++++++++++++++++++++++++++++++++++++++++++ src/util/viriscsi.h | 6 ++++++ 3 files changed, 52 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 3580a72..edef70b 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1714,6 +1714,7 @@ iptablesRemoveUdpOutput; virISCSIConnectionLogin; virISCSIConnectionLogout; virISCSIGetSession; +virISCSINodeNew; virISCSINodeUpdate; virISCSIRescanLUNs; virISCSIScanTargets; diff --git a/src/util/viriscsi.c b/src/util/viriscsi.c index e705517..8115d09 100644 --- a/src/util/viriscsi.c +++ b/src/util/viriscsi.c @@ -444,6 +444,51 @@ virISCSIScanTargets(const char *portal, return ret; }
+/* + * virISCSINodeNew: + * @portal: address for iSCSI target + * @target: IQN and specific LUN target + * + * Usage of nonpersistent discovery in virISCSIScanTargets is useful primarily + * only when the target IQN is not known; however, since we have the target IQN + * usage of the "--op new" can be done. This avoids problems if "--op delete" + * had been used wiping out the static nodes determined by the scanning of + * all targets. + * + * NB: If already created, subsequent "--op new" commands do not return + * an error.
If it does not return an error, do we need to ignore non-zero status?
I thought the point of the comment is that subsequent --op new commands won't cause an error "if" the node record was generated. IOW: It's ok to have multiple NodeNew commands and those won't cause an error. If there was some other failure, then I'd expect to get and report some error. But for this particular command the code isn't ignoring any error, so the code isn't ignoring errors...
Perhaps I should have written it below the code, not the above comment:
+ /* Ignore non-zero status. */ + if (virCommandRun(cmd, &status) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed new node mode for target '%s'"), + target); + goto cleanup; + }
With a non-NULL second argument, virCommandRun ignores non-zero exit status and expects the caller to deal with it.
If we don't need to be compatible with old broken iscsiadm and multiple --op new commands do not return an error, we should error out on non-zero status.
Ahhhh OK - I see your point now. Of course none of this code deals with the exitstatus anyway yet, but I can add something before pushing - John
Jan
John
AFAIK the rest of the code ignoring exit codes was written before iscsiadm returned them properly: https://github.com/open-iscsi/open-iscsi/commit/2c839a2
Even this libvirt commit from Jan 2010 speaks of 'older versions of iscsiadm': http://libvirt.org/git/?p=libvirt.git;a=commitdiff;h=6aabcb5b
Jan
https://bugzilla.redhat.com/show_bug.cgi?id=1356436 Commit id '56057900' altered the discovery of iSCSI node targets by using the "--op nonpersistent". This caused issues for clean environments or if by chance a "-m node -o delete" was executed. Since each iSCSI Storage Pool has the required iSCSI target path, use that and the virISCSINodeNew API in order to generate the iSCSI node record. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend_iscsi.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/storage/storage_backend_iscsi.c b/src/storage/storage_backend_iscsi.c index bb33da2..84ad6f3 100644 --- a/src/storage/storage_backend_iscsi.c +++ b/src/storage/storage_backend_iscsi.c @@ -353,11 +353,10 @@ virStorageBackendISCSIStartPool(virConnectPtr conn, if ((session = virStorageBackendISCSISession(pool, true)) == NULL) { if ((portal = virStorageBackendISCSIPortal(&pool->def->source)) == NULL) goto cleanup; - /* - * iscsiadm doesn't let you login to a target, unless you've - * first issued a 'sendtargets' command to the portal :-( - */ - if (virISCSIScanTargets(portal, NULL, NULL) < 0) + + /* Create a static node record for the IQN target. Must be done + * in order for login to the target */ + if (virISCSINodeNew(portal, pool->def->source.devices[0].path) < 0) goto cleanup; if (virStorageBackendISCSISetAuth(portal, conn, &pool->def->source) < 0) -- 2.5.5
ping? Tks - John On 07/18/2016 07:37 AM, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1356436
Details in patches.
John Ferlan (2): util: Introduce virISCSINodeNew iscsi: Establish connection to target via static target login
src/libvirt_private.syms | 1 + src/storage/storage_backend_iscsi.c | 9 ++++---- src/util/viriscsi.c | 45 +++++++++++++++++++++++++++++++++++++ src/util/viriscsi.h | 6 +++++ 4 files changed, 56 insertions(+), 5 deletions(-)
On Mon, Jul 18, 2016 at 07:37:19AM -0400, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1356436
Details in patches.
John Ferlan (2): util: Introduce virISCSINodeNew iscsi: Establish connection to target via static target login
src/libvirt_private.syms | 1 + src/storage/storage_backend_iscsi.c | 9 ++++---- src/util/viriscsi.c | 45 +++++++++++++++++++++++++++++++++++++ src/util/viriscsi.h | 6 +++++ 4 files changed, 56 insertions(+), 5 deletions(-)
ACK series, but not ignoring errors would be nicer. Jan
participants (2)
-
John Ferlan -
Ján Tomko