Merge ~rafaeldtinoco/ubuntu/+source/open-iscsi:lp1877617-bionic into ubuntu/+source/open-iscsi:ubuntu/bionic-devel

[Rafael David Tinoco]

BUG: https://bugs.launchpad.net/ubuntu/+source/open-iscsi/+bug/1877617
PPA: https://launchpad.net/~rafaeldtinoco/+archive/ubuntu/lp1877617

[Christian Ehrhardt  (paelzer)]

Changelog, Patch headers and the patch itself LGTM.
Since it does not change the default, but only provides a way to change the behavior it seems good for an SRU to me.

The addition of node.session.scan = auto to the conffile is fine.
But we have to consider people might upgrade with a custom iscsid.conf and NOT take the changes.
This will make the value unset.

The code reading it is:
+ __recinfo_int_o2(SESSION_SCAN, ri, r,
+ session.scan, IDBM_SHOW, "manual", "auto",
+ num, 1);

I was trying to check the code but I'm not entirely sure.
Target should be that if the user declines the conffile change then it should still behave like before (as if auto was set). Could you test that before pushing a SRU?

+1 to SRU this after the new version is in groovy and the above is checked.

review: Needs Information

[Rafael David Tinoco (rafaeldtinoco)]

(k)rafaeldtinoco@bionicvmdev:~$ TARGET_IQN=$(iscsi-iname)
(k)rafaeldtinoco@bionicvmdev:~$ INITIATOR_IQN=$(sudo awk -F = '/InitiatorName=/ {print $2}' /etc/iscsi/initiatorname.iscsi)

(k)rafaeldtinoco@bionicvmdev:~$ sudo targetcli /iscsi create $TARGET_IQN
Created target iqn.2005-03.org.open-iscsi:b1d5ef68d55e.
Created TPG 1.
Global pref auto_add_default_portal=true
Created default portal listening on all IPs (0.0.0.0), port 3260.

(k)rafaeldtinoco@bionicvmdev:~$ sudo targetcli /iscsi/$TARGET_IQN/tpg1/acls create $INITIATOR_IQN
Created Node ACL for iqn.1993-08.org.debian:01:c8d8c9ccc8

(k)rafaeldtinoco@bionicvmdev:~$ sudo targetcli /backstores/fileio create lun1 /lun1 1G
Created fileio lun1 with size 1073741824

(k)rafaeldtinoco@bionicvmdev:~$ sudo targetcli /iscsi/$TARGET_IQN/tpg1/luns create /backstores/fileio/lun1 1
Created LUN 1.
Created LUN 1->1 mapping in node ACL iqn.1993-08.org.debian:01:c8d8c9ccc8

(k)rafaeldtinoco@bionicvmdev:~$ sudo targetcli /backstores/fileio create lun2 /lun2 1G
Created fileio lun2 with size 1073741824

(k)rafaeldtinoco@bionicvmdev:~$ sudo targetcli /iscsi/$TARGET_IQN/tpg1/luns create /backstores/fileio/lun2 2
Created LUN 2.
Created LUN 2->2 mapping in node ACL iqn.1993-08.org.debian:01:c8d8c9ccc8

(k)rafaeldtinoco@bionicvmdev:~$ sudo iscsiadm -m node -p 127.0.0.1 -T $TARGET_IQN -o new
New iSCSI node [tcp:[hw=,ip=,net_if=,iscsi_if=default] 127.0.0.1,3260,-1 iqn.2005-03.org.open-iscsi:b1d5ef68d55e] added

(k)rafaeldtinoco@bionicvmdev:~$ sudo iscsiadm -m node -p 127.0.0.1 -T $TARGET_IQN --login
Logging in to [iface: default, target: iqn.2005-03.org.open-iscsi:b1d5ef68d55e, portal: 127.0.0.1,3260] (multiple)
Login to [iface: default, target: iqn.2005-03.org.open-iscsi:b1d5ef68d55e, portal: 127.0.0.1,3260] successful.

(k)rafaeldtinoco@bionicvmdev:~$ eval "DISKS=\$(sudo iscsiadm -m session -P3 | awk '/Attached scsi disk/ {print \$4}')"

(k)rafaeldtinoco@bionicvmdev:~$ echo $DISKS
sdd sdc

This is the same behavior as before and the package from PPA is being used:

(k)rafaeldtinoco@bionicvmdev:~$ apt-cache policy open-iscsi
open-iscsi:
  Installed: 2.0.874-5ubuntu2.10
  Candidate: 2.0.874-5ubuntu2.10
  Version table:
 *** 2.0.874-5ubuntu2.10 500
        500 http://ppa.launchpad.net/rafaeldtinoco/lp1877617/ubuntu bionic/main amd64 Packages
        100 /var/lib/dpkg/status
     2.0.874-5ubuntu2.9 500
        500 http://us.archive.ubuntu.com/ubuntu bionic-updates/main amd64 Packages
     2.0.874-5ubuntu2 500
        500 http://us.archive.ubuntu.com/ubuntu bionic/main amd64 Packages

so it is good to go.

review: Approve

[Rafael David Tinoco (rafaeldtinoco)]

(c)rafaeldtinoco@bionic:~/.../sources/ubuntu/open-iscsi$ git describe
upload/2.0.874-5ubuntu2.10

(c)rafaeldtinoco@bionic:~/.../sources/ubuntu/open-iscsi$ git push pkg upload/2.0.874-5ubuntu2.10
Counting objects: 12, done.
Delta compression using up to 8 threads.
Compressing objects: 100% (12/12), done.
Writing objects: 100% (12/12), 4.86 KiB | 1.21 MiB/s, done.
Total 12 (delta 7), reused 0 (delta 0)
remote: Checking connectivity: 12, done.
To ssh://git.launchpad.net/ubuntu/+source/open-iscsi
 * [new tag] upload/2.0.874-5ubuntu2.10 -> upload/2.0.874-5ubuntu2.10

(c)rafaeldtinoco@bionic:~/.../sources/ubuntu/open-iscsi$ dput ubuntu ../open-iscsi_2.0.874-5ubuntu2.10_source.changes
Checking signature on .changes
gpg: ../open-iscsi_2.0.874-5ubuntu2.10_source.changes: Valid signature from A93E0E0AD83C0D0F
Checking signature on .dsc
gpg: ../open-iscsi_2.0.874-5ubuntu2.10.dsc: Valid signature from A93E0E0AD83C0D0F
Uploading to ubuntu (via ftp to upload.ubuntu.com):
  Uploading open-iscsi_2.0.874-5ubuntu2.10.dsc: done.
  Uploading open-iscsi_2.0.874-5ubuntu2.10.debian.tar.xz: done.
  Uploading open-iscsi_2.0.874-5ubuntu2.10_source.buildinfo: done.
  Uploading open-iscsi_2.0.874-5ubuntu2.10_source.changes: done.
Successfully uploaded packages.

diff --git a/debian/changelog b/debian/changelog
index 28bb3f8..67521bb 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,12 @@
+open-iscsi (2.0.874-5ubuntu2.10) bionic; urgency=medium
+
+  [ Ben Swartzlander ]
+  * allow open-iscsi to disable auto-scan feature (LP: #1877617)
+    - d/p/lp1877617-Allow-disabling-auto-LUN-scans.patch
+    - d/p/lp1877617-Fix-manual-LUN-scans-feature.patch
+
+ -- Rafael David Tinoco <[e-mail]>  Mon, 11 May 2020 01:27:31 +0000
+
 open-iscsi (2.0.874-5ubuntu2.9) bionic; urgency=medium
 
   * Ship open-iscsi finalrd hook. LP: #1800681
diff --git a/debian/patches/lp1877617-Allow-disabling-auto-LUN-scans.patch b/debian/patches/lp1877617-Allow-disabling-auto-LUN-scans.patch
new file mode 100644
index 0000000..1f41157
--- /dev/null
+++ b/debian/patches/lp1877617-Allow-disabling-auto-LUN-scans.patch
@@ -0,0 +1,227 @@
+Description: Allow disabling auto LUN scans
+
+Existing behavior of auto scanning LUNs is problematic for some
+deployments, particularly in cases where we are:
+
+- Accessing different LUNs from the same target on different machines
+  and we don't want the other LUNs automatically exposed in other
+  systems.
+
+- LUNs are constantly being created and removed from the target by
+  another machine and we don't want our systems polluted by no longer
+  available logical units, since default udev rules don't remove them
+  automatically from the system once they have been added automatically.
+
+This is a little more problematic when working with multipaths as we end
+up with a lot of leftover device maps.
+
+This patch introduces a new configuration option at the session level
+called "scan", with "auto" and "manual" as acceptable values, that
+allows us to disable the autoscan in the following cases:
+
+- On iscsid start
+- On login
+- On AEN/AER messages reporting LUNs data has changed.
+
+For HW drivers all sessions will use the value defined in the
+configuration file.
+
+Default value for this new option is "auto" to maintain existing
+behavior.
+
+Author: Gorka Eguileor <[e-mail]>
+Origin: upstream, https://github.com/open-iscsi/open-iscsi/commit/5e32aea957
+Bug-Ubuntu: https://launchpad.net/bugs/1877617
+Reviewed-By: Rafael David Tinoco <[e-mail]>
+Last-Update: 2020-05-11
+
+--- a/etc/iscsid.conf
++++ b/etc/iscsid.conf
+@@ -306,3 +306,11 @@
+ # a task management function like an ABORT TASK or LOGICAL UNIT RESET, that
+ # it continue to respond to R2Ts. To enable this uncomment this line
+ # node.session.iscsi.FastAbort = No
++
++# To prevent doing automatic scans that would add unwanted luns to the system
++# we can disable them and have sessions only do manually requested scans.
++# Automatic scans are performed on startup, on login, and on AEN/AER reception
++# on devices supporting it.  For HW drivers all sessions will use the value
++# defined in the configuration file.  This configuration option is independent
++# of scsi_mod scan parameter. (The default behavior is auto):
++node.session.scan = auto
+--- a/usr/config.h
++++ b/usr/config.h
+@@ -190,6 +190,7 @@
+ 	int					queue_depth;
+ 	int					initial_login_retry_max;
+ 	int					nr_sessions;
++	int					scan;
+ 	struct iscsi_auth_config		auth;
+ 	struct iscsi_session_timeout_config	timeo;
+ 	struct iscsi_error_timeout_config	err_timeo;
+--- a/usr/idbm.c
++++ b/usr/idbm.c
+@@ -462,6 +462,9 @@
+ 		      session.iscsi.MaxOutstandingR2T, IDBM_SHOW, num, 1);
+ 	__recinfo_int(SESSION_ERL, ri, r,
+ 		      session.iscsi.ERL, IDBM_SHOW, num, 1);
++	__recinfo_int_o2(SESSION_SCAN, ri, r,
++			 session.scan, IDBM_SHOW, "manual", "auto",
++			 num, 1);
+ 
+ 	for (i = 0; i < ISCSI_CONN_MAX; i++) {
+ 		char key[NAME_MAXVAL];
+@@ -2490,7 +2493,7 @@
+ 		log_debug(5, "disc removal removing link %s %s %s %s",
+ 			  target, address, port, iface_id);
+ 
+-		memset(rec, 0, sizeof(*rec));	
++		memset(rec, 0, sizeof(*rec));
+ 		strlcpy(rec->name, target, TARGET_NAME_MAXLEN);
+ 		rec->tpgt = atoi(tpgt);
+ 		rec->conn[0].port = atoi(port);
+@@ -2726,6 +2729,14 @@
+ 	       sizeof(struct iscsi_slp_config));
+ }
+ 
++int
++idbm_session_autoscan(struct iscsi_session *session)
++{
++	if (session)
++		return session->nrec.session.scan;
++	return db->nrec.session.scan;
++}
++
+ struct user_param *idbm_alloc_user_param(char *name, char *value)
+ {
+ 	struct user_param *param;
+@@ -2981,6 +2992,7 @@
+ 	rec->session.info = NULL;
+ 	rec->session.sid = 0;
+ 	rec->session.multiple = 0;
++	rec->session.scan = DEF_INITIAL_SCAN;
+ 	idbm_setup_session_defaults(&rec->session.iscsi);
+ 
+ 	for (i=0; i<ISCSI_CONN_MAX; i++) {
+--- a/usr/idbm.h
++++ b/usr/idbm.h
+@@ -140,6 +140,7 @@
+ extern void idbm_sendtargets_defaults(struct iscsi_sendtargets_config *cfg);
+ extern void idbm_isns_defaults(struct iscsi_isns_config *cfg);
+ extern void idbm_slp_defaults(struct iscsi_slp_config *cfg);
++extern int idbm_session_autoscan(struct iscsi_session *session);
+ extern int idbm_discovery_read(discovery_rec_t *rec, int type, char *addr,
+ 				int port);
+ extern int idbm_rec_read(node_rec_t *out_rec, char *target_name,
+--- a/usr/idbm_fields.h
++++ b/usr/idbm_fields.h
+@@ -45,6 +45,7 @@
+ #define SESSION_MAX_CONNS	"node.session.iscsi.MaxConnections"
+ #define SESSION_MAX_R2T		"node.session.iscsi.MaxOutstandingR2T"
+ #define SESSION_ERL		"node.session.iscsi.ERL"
++#define SESSION_SCAN		"node.session.scan"
+ 
+ /* connections fields */
+ #define CONN_ADDR		"node.conn[%d].address"
+--- a/usr/initiator.c
++++ b/usr/initiator.c
+@@ -997,7 +997,7 @@
+ {
+ 	pid_t pid;
+ 
+-	pid = iscsi_sysfs_scan_host(hostno, 1);
++	pid = iscsi_sysfs_scan_host(hostno, 1, idbm_session_autoscan(session));
+ 	if (pid == 0) {
+ 		mgmt_ipc_write_rsp(qtask, ISCSI_SUCCESS);
+ 
+@@ -1201,7 +1201,8 @@
+ 			break;
+ 		}
+ 
+-		if (sshdr.asc == 0x3f && sshdr.ascq == 0x0e)
++		if (sshdr.asc == 0x3f && sshdr.ascq == 0x0e
++		    && idbm_session_autoscan(session))
+ 			session_scan_host(session, session->hostno, NULL);
+ 		break;
+ 	case ISCSI_ASYNC_MSG_REQUEST_LOGOUT:
+--- a/usr/iscsi_settings.h
++++ b/usr/iscsi_settings.h
+@@ -45,3 +45,6 @@
+ 
+ /* login retries */
+ #define DEF_INITIAL_LOGIN_RETRIES_MAX	4
++
++/* autoscan enabled */
++#define DEF_INITIAL_SCAN	1
+--- a/usr/iscsi_sysfs.c
++++ b/usr/iscsi_sysfs.c
+@@ -1882,12 +1882,15 @@
+ 			strlen(write_buf));
+ }
+ 
+-pid_t iscsi_sysfs_scan_host(int hostno, int async)
++pid_t iscsi_sysfs_scan_host(int hostno, int async, int full_scan)
+ {
+ 	char id[NAME_SIZE];
+-	char *write_buf = "- - -";
++	char write_buf[6] = "- - 0";
+ 	pid_t pid = 0;
+ 
++	if (full_scan)
++		write_buf[4] = '-';
++
+ 	if (async)
+ 		pid = fork();
+ 	if (pid == 0) {
+--- a/usr/iscsi_sysfs.h
++++ b/usr/iscsi_sysfs.h
+@@ -87,7 +87,7 @@
+ 				struct iscsi_session_operational_config *conf);
+ extern void iscsi_sysfs_get_negotiated_conn_conf(int sid,
+ 				struct iscsi_conn_operational_config *conf);
+-extern pid_t iscsi_sysfs_scan_host(int hostno, int async);
++extern pid_t iscsi_sysfs_scan_host(int hostno, int async, int full);
+ extern int iscsi_sysfs_get_session_state(char *state, int sid);
+ extern int iscsi_sysfs_get_host_state(char *state, int host_no);
+ extern int iscsi_sysfs_get_device_state(char *state, int host_no, int target,
+--- a/usr/iscsiadm.c
++++ b/usr/iscsiadm.c
+@@ -773,7 +773,7 @@
+ 	iscsi_sysfs_for_each_device(NULL, host_no, info->sid,
+ 				    iscsi_sysfs_rescan_device);
+ 	/* now scan for new devices */
+-	iscsi_sysfs_scan_host(host_no, 0);
++	iscsi_sysfs_scan_host(host_no, 0, 1);
+ 	return 0;
+ }
+ 
+--- a/usr/iscsid.c
++++ b/usr/iscsid.c
+@@ -216,7 +216,7 @@
+ 				  iscsi_err_to_str(err));
+ 			return 0;
+ 		}
+-		iscsi_sysfs_scan_host(host_no, 0);
++		iscsi_sysfs_scan_host(host_no, 0, idbm_session_autoscan(NULL));
+ 		return 0;
+ 	}
+ 
+--- a/usr/iscsistart.c
++++ b/usr/iscsistart.c
+@@ -140,6 +140,7 @@
+ 	rec->session.initial_login_retry_max = -1;
+ 	rec->conn[0].timeo.noop_out_interval = -1;
+ 	rec->conn[0].timeo.noop_out_timeout = -1;
++	rec->session.scan = -1;
+ 
+ 	list_for_each_entry(param, &user_params, list) {
+ 		/*
+@@ -183,6 +184,8 @@
+ 		rec->conn[0].timeo.noop_out_interval = 0;
+ 	if (rec->conn[0].timeo.noop_out_timeout == -1)
+ 		rec->conn[0].timeo.noop_out_timeout = 0;
++	if (rec->session.scan == -1)
++		rec->session.scan = DEF_INITIAL_SCAN;
+ 
+ 	return 0;
+ }
diff --git a/debian/patches/lp1877617-Fix-manual-LUN-scans-feature.patch b/debian/patches/lp1877617-Fix-manual-LUN-scans-feature.patch
new file mode 100644
index 0000000..82db80d
--- /dev/null
+++ b/debian/patches/lp1877617-Fix-manual-LUN-scans-feature.patch
@@ -0,0 +1,54 @@
+Description: Fix manual LUN scans feature
+
+The newly introduced feature to disable automatic scans should not be
+scanning *any* of the LUNs when the scan is set to manual, but it always
+scans for LUN0.
+
+This patch fixes this by skipping the sysfs call altogether, as it
+should have been doing from the start.
+
+Author: Gorka Eguileor <[e-mail]>
+Origin: upstream, https://github.com/open-iscsi/open-iscsi/commit/d5483b0df9
+Bug-Ubuntu: https://launchpad.net/bugs/1877617
+Reviewed-By: Rafael David Tinoco <[e-mail]>
+Last-Update: 2020-05-11
+
+--- open-iscsi-2.0.874.orig/usr/iscsi_sysfs.c
++++ open-iscsi-2.0.874/usr/iscsi_sysfs.c
+@@ -1882,18 +1882,19 @@ void iscsi_sysfs_rescan_device(void *dat
+ 			strlen(write_buf));
+ }
+ 
+-pid_t iscsi_sysfs_scan_host(int hostno, int async, int full_scan)
++pid_t iscsi_sysfs_scan_host(int hostno, int async, int autoscan)
+ {
+ 	char id[NAME_SIZE];
+-	char write_buf[6] = "- - 0";
++	char *write_buf = "- - -";
+ 	pid_t pid = 0;
+ 
+-	if (full_scan)
+-		write_buf[4] = '-';
+-
+ 	if (async)
+ 		pid = fork();
+-	if (pid == 0) {
++
++	if (pid >= 0 && !autoscan) {
++		if (pid)
++			log_debug(4, "host%d in manual scan mode, skipping scan", hostno);
++	} else if (pid == 0) {
+ 		/* child */
+ 		log_debug(4, "scanning host%d", hostno);
+ 
+--- open-iscsi-2.0.874.orig/usr/iscsi_sysfs.h
++++ open-iscsi-2.0.874/usr/iscsi_sysfs.h
+@@ -87,7 +87,7 @@ extern void iscsi_sysfs_get_negotiated_s
+ 				struct iscsi_session_operational_config *conf);
+ extern void iscsi_sysfs_get_negotiated_conn_conf(int sid,
+ 				struct iscsi_conn_operational_config *conf);
+-extern pid_t iscsi_sysfs_scan_host(int hostno, int async, int full);
++extern pid_t iscsi_sysfs_scan_host(int hostno, int async, int autoscan);
+ extern int iscsi_sysfs_get_session_state(char *state, int sid);
+ extern int iscsi_sysfs_get_host_state(char *state, int host_no);
+ extern int iscsi_sysfs_get_device_state(char *state, int host_no, int target,
diff --git a/debian/patches/series b/debian/patches/series
index 34608a6..6a070b6 100644
--- a/debian/patches/series
+++ b/debian/patches/series
@@ -13,3 +13,5 @@ security/Ensure-strings-from-peer-are-copied-correctly.patch
 security/Skip-useless-strcopy-and-validate-CIDR-length.patch
 security/Check-iscsiuio-ping-data-length-for-validity.patch
 iscid-conf-use-systemd.socket-patch
+lp1877617-Allow-disabling-auto-LUN-scans.patch
+lp1877617-Fix-manual-LUN-scans-feature.patch