Skip to content

Conversation

@klmp200
Copy link

@klmp200 klmp200 commented Oct 31, 2025

Check for usage status before deleting a linstor volume and raise an appropriate error if this happens

@klmp200 klmp200 requested review from Nambrok and Wescoeur October 31, 2025 12:02
Check for usage status before deleting a linstor volume and raise an appropriate error if this happens

Signed-off-by: Antoine Bartuccio <[email protected]>
@klmp200 klmp200 force-pushed the linstor_protect_in_use_volume branch from 2e28b7a to a86786c Compare October 31, 2025 12:05
Comment on lines +694 to +698
def is_volume_in_use(volume_info: Dict) -> bool:
for node in volume_info["nodes"].values():
if node["in-use"]:
return True
return False

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could inline it, else it takes a lot of space for what it does.

def is_volume_in_use(volume_info: Dict) -> bool:
    return any(node["in-use"] for node in volume_info["nodes"].values())

Comment on lines +685 to +706
def get_volume_info(volume_uuid: str) -> Dict:
for volume in self.get_resources_info().values():
if volume["uuid"] == volume_uuid:
return volume
raise LinstorVolumeManagerError(
f"Could not find info about volume `{volume_uuid}`",
LinstorVolumeManagerError.ERR_VOLUME_DESTROY
)

def is_volume_in_use(volume_info: Dict) -> bool:
for node in volume_info["nodes"].values():
if node["in-use"]:
return True
return False

self._ensure_volume_exists(volume_uuid)
self.ensure_volume_is_not_locked(volume_uuid)
if is_volume_in_use(get_volume_info(volume_uuid)):
raise LinstorVolumeManagerError(
f"Could not destroy volume `{volume_uuid}` as it is currently in use",
LinstorVolumeManagerError.ERR_VOLUME_DESTROY
)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additionally, having single-use nested functions introduces unnecessary noise IMHO.

Don’t take this suggestion at face value, it's mostly meant to illustrate the idea.

Suggested change
def get_volume_info(volume_uuid: str) -> Dict:
for volume in self.get_resources_info().values():
if volume["uuid"] == volume_uuid:
return volume
raise LinstorVolumeManagerError(
f"Could not find info about volume `{volume_uuid}`",
LinstorVolumeManagerError.ERR_VOLUME_DESTROY
)
def is_volume_in_use(volume_info: Dict) -> bool:
for node in volume_info["nodes"].values():
if node["in-use"]:
return True
return False
self._ensure_volume_exists(volume_uuid)
self.ensure_volume_is_not_locked(volume_uuid)
if is_volume_in_use(get_volume_info(volume_uuid)):
raise LinstorVolumeManagerError(
f"Could not destroy volume `{volume_uuid}` as it is currently in use",
LinstorVolumeManagerError.ERR_VOLUME_DESTROY
)
self._ensure_volume_exists(volume_uuid)
self.ensure_volume_is_not_locked(volume_uuid)
volume_info = next(
(volume for volume in self.get_resources_info().values()
if volume["uuid"] == volume_uuid), None
)
if not volume_info:
raise LinstorVolumeManagerError(
f"Could not find info about volume `{volume_uuid}`",
LinstorVolumeManagerError.ERR_VOLUME_DESTROY
)
if any(node["in-use"] for node in volume_info["nodes"].values()):
raise LinstorVolumeManagerError(
f"Could not destroy volume `{volume_uuid}` as it is currently in use",
LinstorVolumeManagerError.ERR_VOLUME_DESTROY
)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants