Copilot commented on code in PR #14287: URL: https://github.com/apache/cloudstack/pull/14287#discussion_r4258264532
########## scripts/vm/hypervisor/xenserver/xcpserver83/NFSSR.py: ########## @@ -28,6 +28,13 @@ from lock import Lock import cleanup +try: + # sm >= 3.2.12-18 (e.g. XCP-ng 8.3), where LOCK_TYPE_SR moved to lock.py + from lock import LOCK_TYPE_SR +except ImportError: + # older sm (XenServer and XCP-ng releases prior to the 8.3 SM refactor) + LOCK_TYPE_SR = vhdutil.LOCK_TYPE_SR Review Comment: In the fallback branch, `vhdutil` is referenced without being imported in this block. If `vhdutil` is not imported earlier in the file, this will raise `NameError` at import time. Make the fallback branch self-contained by importing `vhdutil` there (or importing `LOCK_TYPE_SR` directly from the legacy module where it exists). ########## scripts/vm/hypervisor/xenserver/xenserver84/cloud-plugin-storage: ########## @@ -41,6 +39,26 @@ import random import cloudstack_pluginlib as lib import logging +try: + # sm >= 3.2.12-18 (e.g. XCP-ng 8.3), where lvhdutil was folded into + # constants/lvmcowutil and vhdutil became a class + from constants import VG_PREFIX, VG_LOCATION + from lvmcowutil import LvmCowUtil + from vhdutil import VhdUtil + _vhdutil = VhdUtil() + lvhdutil_extractUuid = LvmCowUtil.extractUuid + vhdutil_getParent = _vhdutil.getParent + vhdutil_getVHDInfoLVM = _vhdutil.getInfoFromLVM +except ImportError: + # older sm (XenServer and XCP-ng releases prior to the 8.3 SM refactor) + import vhdutil + import lvhdutil + VG_PREFIX = lvhdutil.VG_PREFIX + VG_LOCATION = lvhdutil.VG_LOCATION + lvhdutil_extractUuid = lvhdutil.extractUuid + vhdutil_getParent = vhdutil.getParent + vhdutil_getVHDInfoLVM = vhdutil.getVHDInfoLVM Review Comment: This SM-compatibility shim is duplicated (also in `xenserver84/vmopsSnapshot`). To reduce the risk of drift (e.g., function mappings diverging across scripts), consider extracting this into a shared helper module under the same directory and importing it from both scripts (exporting `VG_PREFIX/VG_LOCATION` and the `vhdutil_*`/`lvhdutil_*` callables). This should make future SM changes easier to handle in one place. ########## scripts/vm/hypervisor/xenserver/xenserver84/vmopsSnapshot: ########## @@ -41,6 +39,30 @@ import random import cloudstack_pluginlib as lib import logging +try: + # sm >= 3.2.12-18 (e.g. XCP-ng 8.3), where lvhdutil was folded into + # constants/lvmcowutil and vhdutil became a class + from constants import VG_PREFIX, VG_LOCATION + from lvmcowutil import LvmCowUtil + from vhdutil import VhdUtil Review Comment: The PR description is still the template and doesn’t explain the behavioral change or validation. Please update the PR description to state that this introduces runtime compatibility for the XCP-ng SM refactor (vhdutil/lvhdutil + LOCK_TYPE_SR moves), and include how it was tested (at least one “old SM” and one “new SM” environment/version). -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
