Copilot commented on code in PR #14203:
URL: https://github.com/apache/cloudstack/pull/14203#discussion_r4313092396
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtTakeBackupCommandWrapper.java:
##########
@@ -136,6 +151,33 @@ private String stripMarkerLines(String stdout) {
return sb.toString();
}
+ /**
+ * Find nasbackup.sh's {@code BACKUP_SIZE_TOTAL=<bytes>} marker line.
Unlike the old
+ * position/shape-based parsing this replaced, it doesn't need to know or
guess which of
+ * nasbackup.sh's code paths actually ran.
+ * <p>
+ * The marker is only metadata: a missing/unparseable marker does not mean
the backup (which
+ * already exited 0) failed, so this logs a warning and reports an unknown
size (0) rather
+ * than throwing — the caller's generic RuntimeException handler would
otherwise turn an
+ * on-disk-successful backup into a reported failure.
+ */
+ private long extractBackupSize(String rawStdout) {
+ if (rawStdout != null) {
+ for (String line : rawStdout.split("\n", -1)) {
+ if (line.startsWith(BACKUP_SIZE_MARKER_PREFIX)) {
+ try {
+ return
Long.parseLong(line.substring(BACKUP_SIZE_MARKER_PREFIX.length()).trim());
+ } catch (NumberFormatException e) {
+ logger.warn("nasbackup.sh reported an unparseable {}
marker: {}", BACKUP_SIZE_MARKER_PREFIX, line);
+ return 0L;
+ }
+ }
+ }
+ }
+ logger.warn("nasbackup.sh did not report a {} marker in its output;
backup succeeded but its size is unknown", BACKUP_SIZE_MARKER_PREFIX);
+ return 0L;
+ }
Review Comment:
With this change, a successful backup that runs an older `nasbackup.sh` (or
any environment where the marker is missing) will always report size `0`, which
can silently break quota/reporting and make troubleshooting harder during
rolling upgrades. Consider a backward-compatible fallback: if the marker is
missing, attempt the legacy size parsing for known old output shapes (and only
return 0 if both marker and fallback parsing fail).
##########
plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/NASBackupProvider.java:
##########
@@ -383,6 +383,22 @@ protected boolean
allVolumesOnCheckpointCapableStorage(VirtualMachine vm) {
return true;
}
+ /**
+ * True when any of the given volumes sits on a pool of the given type.
+ */
+ private boolean hasVolumeOnPoolType(List<VolumeVO> volumes,
Storage.StoragePoolType poolType) {
+ if (volumes == null) {
+ return false;
+ }
+ for (VolumeVO volume : volumes) {
+ StoragePoolVO pool =
primaryDataStoreDao.findById(volume.getPoolId());
+ if (pool != null && poolType.equals(pool.getPoolType())) {
+ return true;
+ }
+ }
+ return false;
+ }
Review Comment:
This introduces a per-volume DAO lookup (`findById`) which is an N+1 pattern
and runs on every backup attempt (since `vmVolumes` is now always fetched). To
keep this cheap at scale (many data disks), consider resolving pool types in
bulk (e.g., fetch all pools for the unique `poolId`s once) or changing the
volume query to include pool type information.
##########
test/integration/plugins/storpool/sp_util.py:
##########
@@ -926,6 +951,19 @@ def get_pool(cls, zone):
return
return sp_pools
+ @classmethod
+ def get_nfs_pool(cls, zone):
+ """Return the NFS primary storage entries configured for this zone in
+ the Marvin cfg file. NFS entries don't set "provider" (unlike
+ StorPool/RBD), so they're identified by their "nfs://" url instead."""
+ storage_pools = zone.primaryStorages
+ nfs_pools = []
+ for storage in storage_pools:
+ url = storage['url'] or ""
+ if not storage['provider'] and
str(url).lower().startswith("nfs://"):
Review Comment:
`get_nfs_pool()` assumes `storage['provider']` (and `storage['url']`) keys
always exist, but the docstring says NFS entries “don't set provider”, which
can also mean the key is absent. This will raise `KeyError` and break the
mixed-storage suite setup. Use safe accessors (e.g., `storage.get('provider')`
/ `storage.get('url', '')`) for robust detection of NFS entries.
##########
test/integration/plugins/storpool/TestNasBackupStorPool.py:
##########
@@ -0,0 +1,712 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+"""
+Smoke tests for NAS backup and recovery on StorPool primary storage.
+
+Assumes the environment already has NAS backup configured for the zone
+(a NAS backup repository and an imported backup offering).
+"""
+
+import math
+import time
+import uuid
+
+from marvin.cloudstackTestCase import cloudstackTestCase
+from marvin.codes import FAILED, KVM
+from marvin.lib.base import (Account,
+ Backup,
+ BackupOffering,
+ BackupRepository,
+ DiskOffering,
+ SecurityGroup,
+ ServiceOffering,
+ StoragePool,
+ VirtualMachine,
+ Volume)
+from marvin.lib.common import (get_domain,
+ get_template,
+ list_disk_offering,
+ list_networks,
+ list_service_offering,
+ list_storage_pools,
+ list_zones)
+from marvin.lib.utils import cleanup_resources, get_hypervisor_type
+from nose.plugins.attrib import attr
+from storpool import spapi
+
+from sp_util import StorPoolHelper, TestData
+
+
+def _template_size_gb(template):
+ size_bytes = getattr(template, "size", None)
+ if not size_bytes:
+ return 1
+ return max(1, int(math.ceil(float(size_bytes) / (1024 ** 3))))
+
+
+class TestNasBackupStorPool(cloudstackTestCase):
+
+ @classmethod
+ def setUpClass(cls):
+ super(TestNasBackupStorPool, cls).setUpClass()
+ try:
+ cls.setUpCloudStack()
+ except Exception:
+ cls.cleanUpCloudStack()
+ raise
+
+ @classmethod
+ def setUpCloudStack(cls):
+ cls._setup_common()
+ if cls.unsupportedHypervisor:
+ return
+
+ cls.vm = cls._create_test_vm("SP-NasBkp-%s" % uuid.uuid4(),
cls.disk_offering.id)
+ cls._verify_setup_volumes(cls.vm)
+
+ @classmethod
+ def _setup_common(cls):
+ """Build everything shared by the StorPool-only and mixed-storage NAS
+ backup suites: account, template, network, backup offering, StorPool
+ primary storage/service/disk offering. Does not create the test VM,
+ so subclasses can plug in a different disk offering for it."""
+ config = cls.getClsConfig()
+ StorPoolHelper.logger = cls
+
+ zone_cfg = config.zones[0]
+ assert zone_cfg is not None
Review Comment:
This test suite uses bare `assert` in setup. In Python, `assert` statements
are skipped when optimizations are enabled (`python -O`), which can lead to
confusing downstream failures. Prefer `unittest` assertions (e.g.,
`cls.assertIsNotNone(...)` / `self.assertTrue(...)`) so the checks always
execute and produce clearer failure messages.
##########
test/integration/plugins/storpool/TestNasBackupStorPool.py:
##########
@@ -0,0 +1,712 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+"""
+Smoke tests for NAS backup and recovery on StorPool primary storage.
+
+Assumes the environment already has NAS backup configured for the zone
+(a NAS backup repository and an imported backup offering).
+"""
+
+import math
+import time
+import uuid
+
+from marvin.cloudstackTestCase import cloudstackTestCase
+from marvin.codes import FAILED, KVM
+from marvin.lib.base import (Account,
+ Backup,
+ BackupOffering,
+ BackupRepository,
+ DiskOffering,
+ SecurityGroup,
+ ServiceOffering,
+ StoragePool,
+ VirtualMachine,
+ Volume)
+from marvin.lib.common import (get_domain,
+ get_template,
+ list_disk_offering,
+ list_networks,
+ list_service_offering,
+ list_storage_pools,
+ list_zones)
+from marvin.lib.utils import cleanup_resources, get_hypervisor_type
+from nose.plugins.attrib import attr
+from storpool import spapi
+
+from sp_util import StorPoolHelper, TestData
+
+
+def _template_size_gb(template):
+ size_bytes = getattr(template, "size", None)
+ if not size_bytes:
+ return 1
+ return max(1, int(math.ceil(float(size_bytes) / (1024 ** 3))))
+
+
+class TestNasBackupStorPool(cloudstackTestCase):
+
+ @classmethod
+ def setUpClass(cls):
+ super(TestNasBackupStorPool, cls).setUpClass()
+ try:
+ cls.setUpCloudStack()
+ except Exception:
+ cls.cleanUpCloudStack()
+ raise
+
+ @classmethod
+ def setUpCloudStack(cls):
+ cls._setup_common()
+ if cls.unsupportedHypervisor:
+ return
+
+ cls.vm = cls._create_test_vm("SP-NasBkp-%s" % uuid.uuid4(),
cls.disk_offering.id)
+ cls._verify_setup_volumes(cls.vm)
+
+ @classmethod
+ def _setup_common(cls):
+ """Build everything shared by the StorPool-only and mixed-storage NAS
+ backup suites: account, template, network, backup offering, StorPool
+ primary storage/service/disk offering. Does not create the test VM,
+ so subclasses can plug in a different disk offering for it."""
+ config = cls.getClsConfig()
+ StorPoolHelper.logger = cls
+
+ zone_cfg = config.zones[0]
+ assert zone_cfg is not None
+ cls.zone_cfg = zone_cfg
+
+ testClient = super(TestNasBackupStorPool, cls).getClsTestClient()
+ cls.apiclient = testClient.getApiClient()
+ cls._cleanup = []
+ cls.unsupportedHypervisor = False
+ cls.skip_reason = None
+ cls.nfs_storage_pool = None
+ cls.nfs_tag = None
+
+ td = TestData()
+ cls.testdata = td.testdata
+ cls.helper = StorPoolHelper()
+
+ cls.services = testClient.getParsedTestDataConfig()
+ cls.domain = get_domain(cls.apiclient)
+ cls.zone = list_zones(cls.apiclient, name=zone_cfg.name)[0]
+ assert cls.zone is not None
Review Comment:
This test suite uses bare `assert` in setup. In Python, `assert` statements
are skipped when optimizations are enabled (`python -O`), which can lead to
confusing downstream failures. Prefer `unittest` assertions (e.g.,
`cls.assertIsNotNone(...)` / `self.assertTrue(...)`) so the checks always
execute and produce clearer failure messages.
##########
test/integration/plugins/storpool/sp_util.py:
##########
@@ -682,26 +683,50 @@ def destroy_vm(self, apiclient, virtualmachineid):
apiclient.destroyVirtualMachine(cmd)
@classmethod
- def check_storpool_volume_size(cls, volume, spapi):
- name = volume.path.split("/")[3]
+ def get_storpool_volume_by_path(cls, spapi, path):
+ """
+ Resolve a CloudStack volume path (/dev/storpool-byid/<globalId>) to a
+ StorPool volume.
+ """
+ if not path:
+ raise Exception("Volume path is empty")
+ parts = path.rstrip("/").split("/")
+ if len(parts) < 4:
+ raise Exception("Unexpected StorPool volume path: %s" % path)
+ name = "~" + parts[3]
try:
- spvolume = spapi.volumeList(volumeName = "~" + name)
- if spvolume[0].size != volume.size:
- raise Exception("Storpool volume size is not the same as
CloudStack db size")
- except spapi.ApiError as err:
- raise Exception(err)
+ volumes = spapi.volumeList(volumeName=name)
+ except Exception as err:
+ raise Exception("StorPool volume not found for path %s: %s" %
(path, err))
+ if not volumes:
+ raise Exception("StorPool volume not found for path %s" % path)
+ return volumes[0]
+
+ @classmethod
+ def check_storpool_volume_size(cls, volume, spapi):
+ spvolume = cls.get_storpool_volume_by_path(spapi, volume.path)
+ if spvolume.size != volume.size:
+ raise Exception("Storpool volume size is not the same as
CloudStack db size")
@classmethod
def check_storpool_volume_iops(cls, spapi, volume,):
- name = volume.path.split("/")[3]
- try:
- spvolume = spapi.volumeList(volumeName = "~" + name)
- logging.debug(spvolume[0].iops)
- logging.debug(volume.maxiops)
- if spvolume[0].iops != volume.maxiops:
- raise Exception("Storpool volume size is not the same as
CloudStack db size")
- except spapi.ApiError as err:
- raise Exception(err)
+ spvolume = cls.get_storpool_volume_by_path(spapi, volume.path)
+ logging.debug(spvolume.iops)
+ logging.debug(volume.maxiops)
+ if spvolume.iops != volume.maxiops:
+ raise Exception("Storpool volume size is not the same as
CloudStack db size")
Review Comment:
This exception message is emitted from `check_storpool_volume_iops` on an
IOPS mismatch, but it says “volume size”. Update it to reference IOPS (and
consider standardizing “StorPool” capitalization) so failures are diagnosable.
--
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]
