From 137df269a461a9fa5780f0516ae6147fe6244480 Mon Sep 17 00:00:00 2001 From: Brian M Date: Tue, 23 Jul 2024 12:00:33 -0700 Subject: [PATCH 1/3] Allow extent to be deleted even if underlying storage is missing --- src/middlewared/middlewared/plugins/iscsi_/extents.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/middlewared/middlewared/plugins/iscsi_/extents.py b/src/middlewared/middlewared/plugins/iscsi_/extents.py index df04f3ce7e2a5..9325bf7cd9a67 100755 --- a/src/middlewared/middlewared/plugins/iscsi_/extents.py +++ b/src/middlewared/middlewared/plugins/iscsi_/extents.py @@ -205,7 +205,9 @@ async def do_delete(self, audit_callback, id_, remove, force): # This change is being made in conjunction with threads_num being specified in scst.conf if data['type'] == 'DISK' and data['path'].startswith('zvol/'): zvolname = zvol_path_to_name(os.path.join('/dev', data['path'])) - await self.middleware.call('zfs.dataset.update', zvolname, {'properties': {'volthreading': {'value': 'on'}}}) + # Only try to set volthreading if the volume still exists. + if await self.middleware.call('pool.dataset.query', [['name', '=', zvolname], ['type', '=', 'VOLUME']]): + await self.middleware.call('zfs.dataset.update', zvolname, {'properties': {'volthreading': {'value': 'on'}}}) try: return await self.middleware.call( @@ -473,7 +475,8 @@ def save(self, data, schema_name, verrors, old=None): async def remove_extent_file(self, data): if data['type'] == 'FILE': try: - os.unlink(data['path']) + if os.path.exists(data['path']): + os.unlink(data['path']) except Exception as e: return e From de0b4834297a110b659d579bf5702d2a54a47b43 Mon Sep 17 00:00:00 2001 From: Brian M Date: Tue, 23 Jul 2024 12:01:47 -0700 Subject: [PATCH 2/3] Add tests wrt iSCSI extent deletion and wrt volthreading --- tests/api2/test_261_iscsi_cmd.py | 54 +++++++++++++++++++++++++++++--- 1 file changed, 49 insertions(+), 5 deletions(-) diff --git a/tests/api2/test_261_iscsi_cmd.py b/tests/api2/test_261_iscsi_cmd.py index 7a77a4ae12753..4fb7ac70245b5 100644 --- a/tests/api2/test_261_iscsi_cmd.py +++ b/tests/api2/test_261_iscsi_cmd.py @@ -16,10 +16,10 @@ portal, read_capacity16, target, target_extent_associate, verify_capacity, verify_luns) -from middlewared.service_exception import ValidationError, ValidationErrors +from middlewared.service_exception import InstanceNotFound, ValidationError, ValidationErrors from middlewared.test.integration.assets.iscsi import target_login_test from middlewared.test.integration.assets.pool import dataset, snapshot -from middlewared.test.integration.utils import call +from middlewared.test.integration.utils import call, ssh from middlewared.test.integration.utils.client import truenas_server from pyscsi.pyscsi.scsi_sense import sense_ascq_dict from pytest_dependency import depends @@ -156,7 +156,7 @@ def file_extent(pool_name, dataset_name, file_name, filesize=MB_512, extent_name @contextlib.contextmanager -def zvol_dataset(zvol, volsize=MB_512): +def zvol_dataset(zvol, volsize=MB_512, recursive=False, force=False): payload = { 'name': zvol, 'type': 'VOLUME', @@ -168,7 +168,10 @@ def zvol_dataset(zvol, volsize=MB_512): try: yield dataset_config finally: - call('pool.dataset.delete', dataset_config['id']) + try: + call('pool.dataset.delete', dataset_config['id'], {'recursive': recursive, 'force': force}) + except InstanceNotFound: + pass def modify_extent(ident, payload): @@ -211,6 +214,10 @@ def get_client_count(): return call('iscsi.global.client_count') +def get_volthreading(zvolid): + return call('zfs.dataset.query', [['id', '=', zvolid]], {'get': True})['properties']['volthreading']['value'] + + def verify_client_count(count, retries=10): """Verify that the client count is the expected value, but include some retries to allow things to settle if necessary.""" @@ -236,7 +243,10 @@ def zvol_extent(zvol, extent_name='zvol_extent'): try: yield extent_config finally: - call('iscsi.extent.delete', extent_config['id'], True, True) + try: + call('iscsi.extent.delete', extent_config['id'], True, True) + except InstanceNotFound: + pass @contextlib.contextmanager @@ -2602,6 +2612,40 @@ def test_33_no_lun_zero(): verify_luns(s, [100, 101]) +def test_34_zvol_extent_volthreading(): + """ + Ensure that volthreading is on for regular zvols and off when they are being + used an iSCSI extent. + """ + zvol_name = f"zvol_volthreading_test{digit}" + zvol = f'{pool_name}/{zvol_name}' + with zvol_dataset(zvol, MB_100, True, True): + assert get_volthreading(zvol) == 'on' + with zvol_extent(zvol, extent_name='zvolextent1'): + assert get_volthreading(zvol) == 'off' + assert get_volthreading(zvol) == 'on' + + +@pytest.mark.parametrize('extent_type', ["FILE", "VOLUME"]) +def test_35_delete_extent_no_dataset(extent_type): + """ + Verify that even if a dataset that contains an extent has been deleted from + the command line, can still use the webui/API to delete the extent. + """ + dataset_name = f'iscsids_{extent_type}_{digit}' + with dataset(dataset_name) as dspath: + DESTROY_CMD = f'zfs destroy -r {dspath}' + match extent_type: + case 'FILE': + with file_extent(pool_name, dataset_name, 'testfile', extent_name='fileextent1'): + ssh(DESTROY_CMD) + case 'VOLUME': + zvol = f'{dspath}/zvol{digit}' + with zvol_dataset(zvol, MB_100, True, True): + with zvol_extent(zvol, extent_name='zvolextent1'): + ssh(DESTROY_CMD) + + def test_99_teardown(request): # Disable iSCSI service depends(request, ["iscsi_cmd_00"]) From 7834fdda4af9dca155927c07fda43e30b38981b1 Mon Sep 17 00:00:00 2001 From: Brian M Date: Tue, 23 Jul 2024 14:51:07 -0700 Subject: [PATCH 3/3] Address review --- src/middlewared/middlewared/plugins/iscsi_/extents.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/middlewared/middlewared/plugins/iscsi_/extents.py b/src/middlewared/middlewared/plugins/iscsi_/extents.py index 9325bf7cd9a67..c7186baf65d58 100755 --- a/src/middlewared/middlewared/plugins/iscsi_/extents.py +++ b/src/middlewared/middlewared/plugins/iscsi_/extents.py @@ -475,8 +475,9 @@ def save(self, data, schema_name, verrors, old=None): async def remove_extent_file(self, data): if data['type'] == 'FILE': try: - if os.path.exists(data['path']): - os.unlink(data['path']) + os.unlink(data['path']) + except FileNotFoundError: + pass except Exception as e: return e