<fix>[trash]: fix storage trash capacity calculation #1572
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Warning Rate limit exceeded@MatheMatrix has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 7 minutes and 17 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (17)
Walkthrough此次更改主要涉及对垃圾管理和存储容量计算的功能进行重构与增强。删除了 Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
MatheMatrix
force-pushed
the
sync/shan.wu/fix-ZSTAC-68260-2@@2
branch
from
d1d4711 to
b47133a
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (11)
storage/src/main/java/org/zstack/storage/primary/PrimaryStorageCapacityRecalculator.java (1)
146-168: 垃圾容量计算逻辑实现完整！代码实现符合需求，通过SQL聚合计算每个主存储的垃圾总大小，并正确更新容量统计。建议考虑以下优化：
- 将SQL查询语句抽取为常量，提高代码可维护性
- 对于较大的垃圾容量值，建议添加日志记录，便于问题排查
建议参考如下重构：
+ private static final String TRASH_SIZE_SQL = + "select sum(trash.size), trash.storageUuid" + + " from InstallPathRecycleVO trash" + + " where trash.storageUuid in (:psUuids)" + + " group by trash.storageUuid"; // calculate all trash size { - String sql = "select sum(trash.size), trash.storageUuid" + - " from InstallPathRecycleVO trash" + - " where trash.storageUuid in (:psUuids)" + - " group by trash.storageUuid"; + String sql = TRASH_SIZE_SQL; TypedQuery<Tuple> q = dbf.getEntityManager().createQuery(sql, Tuple.class); q.setParameter("psUuids", psUuids); List<Tuple> ts = q.getResultList(); for (Tuple t : ts) { if (t.get(0, Long.class) == null) { continue; } long cap = t.get(0, Long.class); String psUuid = t.get(1, String.class); + if (cap > 1024 * 1024 * 1024L) { // 1GB + logger.info(String.format("Found large trash size[%s] for primary storage[uuid:%s]", + cap, psUuid)); + } Long ncap = psCap.get(psUuid); ncap = ncap == null ? cap : ncap + cap; psCap.put(psUuid, ncap); } }core/src/main/java/org/zstack/core/trash/DefaultPrimaryStorageTrashLifeCycleExtension.java (1)
40-40: 建议使用Collections.emptyList()代替Collections.EMPTY_LIST
Collections.EMPTY_LIST是未泛型化的，会导致类型安全警告。建议使用Collections.emptyList()来获得类型安全的空列表。- return Collections.EMPTY_LIST; + return Collections.emptyList();Also applies to: 64-64
plugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalPrimaryStorageTrashLifeCycleExtension.java (1)
53-53: 建议使用Collections.emptyList()代替Collections.EMPTY_LIST为提高类型安全性，使用
Collections.emptyList()替换Collections.EMPTY_LIST。- return Collections.EMPTY_LIST; + return Collections.emptyList();Also applies to: 95-95
core/src/main/java/org/zstack/core/trash/StorageRecycleImpl.java (1)
43-43: 建议避免使用通配符导入在第43行，
import java.util.*;会导入不必要的类，建议只导入实际需要的类以提高代码可读性和维护性。- import java.util.*; + import java.util.List; + import java.util.Map; + import java.util.concurrent.TimeUnit;core/src/main/java/org/zstack/core/trash/StorageTrashLifeCycleExtensionPoint.java (1)
12-17: 方法设计建议：检查返回类型的一致性和必要性
- 方法
afterCreateTrash和beforeRemoveTrash返回List<Long>，而beforeCreateTrash返回void。需要确认这些方法的返回值是否符合预期，或者是否应保持一致。- 此外，建议为每个方法添加文档注释，明确其作用和参数含义，增强代码的可读性和可维护性。
storage/src/main/java/org/zstack/storage/volume/VolumeUtils.java (1)
27-35: 方法实现正确，建议添加方法文档方法实现采用了只读事务和高效的SQL查询，逻辑正确。建议添加JavaDoc说明方法的用途、参数和返回值。
建议添加如下文档:
+ /** + * 计算指定卷的所有快照总大小 + * @param volumeUuid 卷UUID + * @return 快照总大小，如果没有快照返回0 + */ @Transactional(readOnly = true) public static long calculateSnapshotSizeByVolume(String volumeUuid) {plugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageCapacityRecalculator.java (1)
90-111: 垃圾容量计算逻辑正确，建议提取通用查询方法新增的垃圾容量计算逻辑与现有的卷和快照容量计算逻辑结构相似。建议提取一个通用的查询方法以减少代码重复。
建议重构如下:
+ private void calculateResourceCapacity(String psUuid, List<String> huuids, + Map<String, Long> hostCap, + String tableName, + String hostUuidColumn) { + String sql = String.format("select sum(trash.size), trash.%s" + + " from %s trash" + + " where trash.storageUuid = :psUuid" + + " and trash.%s in :huuids" + + " group by trash.%s", + hostUuidColumn, tableName, hostUuidColumn, hostUuidColumn); + + TypedQuery<Tuple> query = dbf.getEntityManager().createQuery(sql, Tuple.class); + query.setParameter("psUuid", psUuid); + query.setParameter("huuids", huuids); + + List<Tuple> resultList = query.getResultList(); + for (Tuple t : resultList) { + if (t.get(0, Long.class) == null) { + continue; + } + + long cap = t.get(0, Long.class); + String huuid = t.get(1, String.class); + Long ncap = hostCap.get(huuid); + ncap = ncap == null ? cap : ncap + cap; + hostCap.put(huuid, ncap); + } + }test/src/test/groovy/org/zstack/test/integration/storage/snapshot/RevertVolumeFromSnapshotCase.groovy (1)
71-71: 测试用例逻辑正确，建议增加边界测试测试用例正确验证了垃圾容量计算逻辑，使用 actual_size - snapshot_size 的公式进行断言。建议添加一些边界情况的测试。
建议添加以下测试场景:
- 没有快照时的情况
- 快照大小为0的情况
- 快照大小等于卷大小的情况
Also applies to: 104-105
plugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageFactory.java (1)
76-77: 新增接口实现以增强卷管理功能类已实现了
AfterInstantiateVolumeExtensionPoint和CreateDataVolumeExtensionPoint接口，用于处理卷实例化和创建事件。不过需要注意，该类已实现了大量接口，建议考虑是否可以将部分功能拆分到独立的类中，以符合单一职责原则。
建议考虑:
- 将卷相关的功能抽取到单独的类中
- 使用组合而不是继承来重用代码
- 按功能模块拆分成多个较小的类
示例重构方案:
-public class LocalStorageFactory implements PrimaryStorageFactory, Component, - MarshalVmOperationFlowExtensionPoint, HostDeleteExtensionPoint, VmAttachVolumeExtensionPoint, - GetAttachableVolumeExtensionPoint, HostMaintenancePolicyExtensionPoint, AddExpandedQueryExtensionPoint, - VolumeGetAttachableVmExtensionPoint, RecoverDataVolumeExtensionPoint, RecoverVmExtensionPoint, - VmPreMigrationExtensionPoint, CreateTemplateFromVolumeSnapshotExtensionPoint, - HostAfterConnectedExtensionPoint, InstantiateDataVolumeOnCreationExtensionPoint, - PrimaryStorageAttachExtensionPoint, PostMarkRootVolumeAsSnapshotExtension, - AfterTakeLiveSnapshotsOnVolumes, VmCapabilitiesExtensionPoint, PrimaryStorageDetachExtensionPoint, - AfterInstantiateVolumeExtensionPoint, CreateDataVolumeExtensionPoint { +public class LocalStorageFactory implements PrimaryStorageFactory, Component { + private final LocalStorageVolumeManager volumeManager; + private final LocalStorageVmManager vmManager; + private final LocalStorageHostManager hostManager; + + public LocalStorageFactory() { + this.volumeManager = new LocalStorageVolumeManager(); + this.vmManager = new LocalStorageVmManager(); + this.hostManager = new LocalStorageHostManager(); + }storage/src/main/java/org/zstack/storage/primary/PrimaryStorageBase.java (2)
Line range hint
905-942: 发现消息类名拼写不一致在处理
CleanUpTrashOnPrimaryStorageMsg的handle方法中，存在两个不同的类名：CleanUpTrashOnPrimaryStorageMsg和CleanUpTrashOnPrimaryStroageMsg。请检查是否存在拼写错误，建议统一类名以避免潜在的异常和混淆。
Line range hint
556-558: 消息处理时可能存在拼写错误导致的类名不一致在
handleLocalMessage方法中，处理了CleanUpTrashOnPrimaryStorageMsg和CleanUpTrashOnPrimaryStroageMsg两种消息类型。Stroage似乎是Storage的拼写错误，请核实并纠正，以确保消息能够被正确处理。
🛑 Comments failed to post (4)
plugin/ceph/src/main/java/org/zstack/storage/ceph/primary/CephPrimaryStorageBase.java (2)
1837-1839: 🛠️ Refactor suggestion
确保增加容量和删除垃圾记录的操作原子性
在调用
trash.increaseCapacityBeforeRemoveTrash()后，如果在执行trash.removeFromDb()之前发生异常，可能导致容量已增加但垃圾记录未删除，造成数据不一致。建议将这两个操作放在一个事务中，确保操作的原子性。
1928-1930: 🛠️ Refactor suggestion
确保增加容量和删除垃圾记录的操作原子性
同样地，在增加容量后删除垃圾记录的过程中，如果发生异常，可能导致数据不一致。建议将
trash.increaseCapacityBeforeRemoveTrash()和trash.removeFromDb()放在一个事务中执行。storage/src/main/java/org/zstack/storage/volume/VolumeBase.java (2)
1258-1258:
⚠️ Potential issue可能的代码错误：应使用
setPrimaryStorageUuid方法在第 1258 行，调用了
dmsg.setUuid(self.getPrimaryStorageUuid());。通常，setUuid()方法用于设置消息的唯一标识符，而不是设置资源的 UUID。为了确保消息正确路由，应该使用setPrimaryStorageUuid()方法。建议修改为：
-dmsg.setUuid(self.getPrimaryStorageUuid()); +dmsg.setPrimaryStorageUuid(self.getPrimaryStorageUuid());📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.dmsg.setPrimaryStorageUuid(self.getPrimaryStorageUuid());
1266-1267:
⚠️ Potential issue未处理删除失败的情况
在第 1266 行，当删除卷失败时，仅记录了警告日志，但未对流程进行失败处理。这可能导致在主存储中未成功删除卷的情况下，流程仍然继续，可能引发数据不一致或资源泄漏。
建议在删除卷失败时，触发流程失败：
if (!reply.isSuccess()) { logger.warn(String.format("failed to delete volume[uuid:%s, name:%s], %s", self.getUuid(), self.getName(), reply.getError())); + trigger.fail(reply.getError()); }Committable suggestion skipped: line range outside the PR's diff.
MatheMatrix
force-pushed
the
sync/shan.wu/fix-ZSTAC-68260-2@@2
branch
from
b47133a to
0204ea7
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (7)
storage/src/main/java/org/zstack/storage/snapshot/VolumeSnapshotTreeBase.java (2)
2091-2091: 代码变更正确，建议增加数据验证！在设置 actualSize 时，建议添加非负数检查以确保数据的正确性。
建议添加如下验证:
-vol.setActualSize(actualSize); +vol.setActualSize(actualSize < 0 ? 0 : actualSize);
2090-2093: 建议增加关键操作日志记录在创建垃圾存储记录的关键步骤中，建议添加详细的日志记录，以便于问题排查。
建议添加如下日志:
VolumeInventory vol = VolumeInventory.valueOf(volume); vol.setActualSize(actualSize); vol.setInstallPath(oldVolumeInstallPath); vol.setPrimaryStorageUuid(getSelfInventory().getPrimaryStorageUuid()); +logger.info(String.format("creating trash for volume[uuid:%s] with actual size[%s], old install path[%s]", + vol.getUuid(), actualSize, oldVolumeInstallPath)); trashId = trash.createTrash(TrashType.RevertVolume, false, vol).getTrashId(); +logger.info(String.format("created trash[id:%s] for volume[uuid:%s]", trashId, vol.getUuid()));storage/src/main/java/org/zstack/storage/volume/VolumeBase.java (1)
2183-2193: 优化卷大小计算逻辑对卷大小计算逻辑进行了优化:
- 添加了对独立快照大小的计算
- 根据是否支持内部快照来决定实际大小的计算方式
- 更准确地反映了存储空间的使用情况
建议添加日志记录以便于问题排查。
VolumeSize size = new VolumeSize(); refreshVO(); SyncVolumeSizeOnPrimaryStorageReply r = reply.castReply(); self.setSize(r.getSize()); +logger.debug(String.format("Volume[uuid:%s] size synced, size:%s", self.getUuid(), r.getSize())); if (!r.isSupportInternalSnapshot()) { long snapshotSize = calculateSnapshotSize(); self.setActualSize(r.getActualSize() + snapshotSize); size.independentSnapshotSize = snapshotSize; + logger.debug(String.format("Volume[uuid:%s] actual size calculated with independent snapshots, size:%s", + self.getUuid(), self.getActualSize())); } else { self.setActualSize(r.getActualSize()); }core/src/main/java/org/zstack/core/trash/DefaultPrimaryStorageTrashLifeCycleExtension.java (3)
40-40: 建议使用Collections.emptyList()代替Collections.EMPTY_LIST
Collections.EMPTY_LIST返回的是原始类型的列表，可能导致类型安全问题。使用Collections.emptyList()可以获得类型安全的空列表。
64-64: 建议使用Collections.emptyList()代替Collections.EMPTY_LIST同上，使用
Collections.emptyList()来替换Collections.EMPTY_LIST，以提高类型安全性。
38-59: 建议抽取公共代码以消除重复
afterCreateTrash和beforeRemoveTrash方法中存在大量相似的代码逻辑。建议将共同的部分提取到一个私有方法中，减少代码重复，提高代码的可维护性。Also applies to: 62-82
core/src/main/java/org/zstack/core/trash/StorageRecycleImpl.java (1)
141-158: 建议抽取公共代码以消除重复
decreaseCapacityAfterCreateTrash和increaseCapacityBeforeRemoveTrash方法中的代码逻辑类似。建议提取公共部分到一个私有方法或工具类中，减少代码重复，提高代码的可维护性和可读性。Also applies to: 160-177
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
conf/springConfigXml/gc.xmlis excluded by!**/*.xmlconf/springConfigXml/localStorage.xmlis excluded by!**/*.xml
📒 Files selected for processing (17)
core/src/main/java/org/zstack/core/trash/CreateRecycleExtensionPoint.java(0 hunks)core/src/main/java/org/zstack/core/trash/DefaultPrimaryStorageTrashLifeCycleExtension.java(1 hunks)core/src/main/java/org/zstack/core/trash/StorageRecycleImpl.java(4 hunks)core/src/main/java/org/zstack/core/trash/StorageTrash.java(1 hunks)core/src/main/java/org/zstack/core/trash/StorageTrashLifeCycleExtensionPoint.java(1 hunks)header/src/main/java/org/zstack/header/storage/primary/SyncVolumeSizeOnPrimaryStorageReply.java(2 hunks)header/src/main/java/org/zstack/header/volume/SyncVolumeSizeReply.java(2 hunks)plugin/ceph/src/main/java/org/zstack/storage/ceph/primary/CephPrimaryStorageBase.java(3 hunks)plugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalPrimaryStorageTrashLifeCycleExtension.java(1 hunks)plugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageCapacityRecalculator.java(4 hunks)plugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageFactory.java(1 hunks)storage/src/main/java/org/zstack/storage/primary/PrimaryStorageBase.java(1 hunks)storage/src/main/java/org/zstack/storage/primary/PrimaryStorageCapacityRecalculator.java(2 hunks)storage/src/main/java/org/zstack/storage/snapshot/VolumeSnapshotTreeBase.java(1 hunks)storage/src/main/java/org/zstack/storage/volume/VolumeBase.java(4 hunks)storage/src/main/java/org/zstack/storage/volume/VolumeUtils.java(2 hunks)test/src/test/groovy/org/zstack/test/integration/storage/snapshot/RevertVolumeFromSnapshotCase.groovy(3 hunks)
💤 Files with no reviewable changes (1)
- core/src/main/java/org/zstack/core/trash/CreateRecycleExtensionPoint.java
🚧 Files skipped from review as they are similar to previous changes (9)
- header/src/main/java/org/zstack/header/volume/SyncVolumeSizeReply.java
- core/src/main/java/org/zstack/core/trash/StorageTrashLifeCycleExtensionPoint.java
- plugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageCapacityRecalculator.java
- header/src/main/java/org/zstack/header/storage/primary/SyncVolumeSizeOnPrimaryStorageReply.java
- storage/src/main/java/org/zstack/storage/volume/VolumeUtils.java
- core/src/main/java/org/zstack/core/trash/StorageTrash.java
- plugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalPrimaryStorageTrashLifeCycleExtension.java
- test/src/test/groovy/org/zstack/test/integration/storage/snapshot/RevertVolumeFromSnapshotCase.groovy
- plugin/ceph/src/main/java/org/zstack/storage/ceph/primary/CephPrimaryStorageBase.java
🔇 Additional comments (7)
plugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageFactory.java (1)
76-77: 新增接口实现以支持数据卷生命周期管理
LocalStorageFactory 类新增实现了以下接口:
AfterInstantiateVolumeExtensionPoint: 用于在数据卷实例化后执行相关操作CreateDataVolumeExtensionPoint: 用于支持数据卷的创建流程
这些接口的添加完善了本地存储对数据卷生命周期的管理能力。
storage/src/main/java/org/zstack/storage/volume/VolumeBase.java (2)
799-799: 新增独立快照大小字段
在回复消息中添加了独立快照大小字段,用于跟踪独立快照占用的存储空间。
3444-3444: 添加独立快照大小字段
在VolumeSize类中添加了independentSnapshotSize字段,用于记录独立快照的大小。
storage/src/main/java/org/zstack/storage/primary/PrimaryStorageCapacityRecalculator.java (1)
146-168: 代码正确地计算并包含了垃圾回收的容量
新增的代码正确地将垃圾回收的大小纳入了主存储容量的重新计算中，确保了容量信息的准确性。
core/src/main/java/org/zstack/core/trash/StorageRecycleImpl.java (1)
82-82: 正确使用 vol.getActualSize() 更新回收项大小
将回收项的大小设置为卷的实际大小，符合需求，确保了容量统计的准确性。
storage/src/main/java/org/zstack/storage/primary/PrimaryStorageBase.java (2)
Line range hint 988-993: 在删除垃圾记录时未更新容量信息
当删除失败并从数据库移除垃圾记录时，没有调用 increaseCapacityBeforeRemoveTrash 方法更新容量信息，可能导致容量统计不准确。请确认在这种情况下是否需要更新容量信息。
Line range hint 985-996: 删除卷链时遇到错误应考虑继续处理剩余路径
当前代码在删除某个路径发生错误时，会立即调用 c.allDone() 停止处理，可能跳过剩余路径的删除。建议确认是否应在遇到错误时继续尝试删除剩余的路径，以确保尽可能清理所有资源。
Comment on lines
+1231
to
+1272
| boolean deleteFromPs = deletionPolicy == VolumeDeletionPolicy.Direct; | ||
| boolean releaseCapFromPs = deleteFromPs && self.getPrimaryStorageUuid() != null; | ||
| if (deleteFromPs) { | ||
| for (VolumeBeforeExpungeExtensionPoint ext : pluginRgty.getExtensionList(VolumeBeforeExpungeExtensionPoint.class)) { | ||
| if (ext.skipExpungeVolume(self.toInventory())) { | ||
| deleteFromPs = false; | ||
| logger.debug(String.format("extension[%s] skip delete volume[uuid:%s] from primary storage", | ||
| ext.getClass().getSimpleName(), self.getUuid())); | ||
| } | ||
| }; | ||
|
|
||
| // some task will create a NotInstantiated volume but instantiated actually, means the volume is not ready to delete. | ||
| if (self.getStatus() != VolumeStatus.Ready || self.getPrimaryStorageUuid() == null) { | ||
| deleteFromPs = false; | ||
| logger.debug(String.format("volume[uuid:%s] is not ready or has no primary storage, skip delete volume from primary storage", self.getUuid())); | ||
| } | ||
| } | ||
|
|
||
|
|
||
| if (deleteFromPs) { | ||
| flow(new NoRollbackFlow() { | ||
| String __name__ = "delete-volume-from-primary-storage"; | ||
|
|
||
| @Override | ||
| public boolean skip(Map data) { | ||
| return pluginRgty.getExtensionList(VolumeBeforeExpungeExtensionPoint.class).stream() | ||
| .anyMatch(ext -> ext.skipExpungeVolume(self.toInventory())); | ||
| } | ||
|
|
||
| @Override | ||
| public void run(final FlowTrigger trigger, Map data) { | ||
| if (self.getStatus() == VolumeStatus.Ready && | ||
| self.getPrimaryStorageUuid() != null) { | ||
| DeleteVolumeOnPrimaryStorageMsg dmsg = new DeleteVolumeOnPrimaryStorageMsg(); | ||
| dmsg.setVolume(getSelfInventory()); | ||
| dmsg.setUuid(self.getPrimaryStorageUuid()); | ||
| bus.makeTargetServiceIdByResourceUuid(dmsg, PrimaryStorageConstant.SERVICE_ID, self.getPrimaryStorageUuid()); | ||
| logger.debug(String.format("Asking primary storage[uuid:%s] to remove data volume[uuid:%s]", self.getPrimaryStorageUuid(), | ||
| self.getUuid())); | ||
| bus.send(dmsg, new CloudBusCallBack(trigger) { | ||
| @Override | ||
| public void run(MessageReply reply) { | ||
| if (!reply.isSuccess()) { | ||
| logger.warn(String.format("failed to delete volume[uuid:%s, name:%s], %s", | ||
| self.getUuid(), self.getName(), reply.getError())); | ||
| } | ||
|
|
||
| trigger.next(); | ||
| DeleteVolumeOnPrimaryStorageMsg dmsg = new DeleteVolumeOnPrimaryStorageMsg(); | ||
| dmsg.setVolume(getSelfInventory()); | ||
| dmsg.setUuid(self.getPrimaryStorageUuid()); | ||
| bus.makeTargetServiceIdByResourceUuid(dmsg, PrimaryStorageConstant.SERVICE_ID, self.getPrimaryStorageUuid()); | ||
| logger.debug(String.format("Asking primary storage[uuid:%s] to remove data volume[uuid:%s]", self.getPrimaryStorageUuid(), | ||
| self.getUuid())); | ||
| bus.send(dmsg, new CloudBusCallBack(trigger) { | ||
| @Override | ||
| public void run(MessageReply reply) { | ||
| if (!reply.isSuccess()) { | ||
| logger.warn(String.format("failed to delete volume[uuid:%s, name:%s], %s", | ||
| self.getUuid(), self.getName(), reply.getError())); | ||
| } | ||
| }); | ||
| } else { | ||
| trigger.next(); | ||
| } | ||
|
|
||
| trigger.next(); | ||
| } | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
改进卷删除逻辑
优化了卷删除的处理流程:
- 明确了删除策略的判断条件
- 增加了对扩展点的支持
- 改进了错误处理机制
建议进一步完善错误处理:
- 添加更详细的错误信息
- 增加重试机制
boolean deleteFromPs = deletionPolicy == VolumeDeletionPolicy.Direct;
boolean releaseCapFromPs = deleteFromPs && self.getPrimaryStorageUuid() != null;
if (deleteFromPs) {
for (VolumeBeforeExpungeExtensionPoint ext : pluginRgty.getExtensionList(VolumeBeforeExpungeExtensionPoint.class)) {
if (ext.skipExpungeVolume(self.toInventory())) {
deleteFromPs = false;
+ logger.info(String.format("Volume[uuid:%s] deletion skipped by extension[%s]",
+ self.getUuid(), ext.getClass().getSimpleName()));
break;
}
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| boolean deleteFromPs = deletionPolicy == VolumeDeletionPolicy.Direct; | |
| boolean releaseCapFromPs = deleteFromPs && self.getPrimaryStorageUuid() != null; | |
| if (deleteFromPs) { | |
| for (VolumeBeforeExpungeExtensionPoint ext : pluginRgty.getExtensionList(VolumeBeforeExpungeExtensionPoint.class)) { | |
| if (ext.skipExpungeVolume(self.toInventory())) { | |
| deleteFromPs = false; | |
| logger.debug(String.format("extension[%s] skip delete volume[uuid:%s] from primary storage", | |
| ext.getClass().getSimpleName(), self.getUuid())); | |
| } | |
| }; | |
| // some task will create a NotInstantiated volume but instantiated actually, means the volume is not ready to delete. | |
| if (self.getStatus() != VolumeStatus.Ready || self.getPrimaryStorageUuid() == null) { | |
| deleteFromPs = false; | |
| logger.debug(String.format("volume[uuid:%s] is not ready or has no primary storage, skip delete volume from primary storage", self.getUuid())); | |
| } | |
| } | |
| if (deleteFromPs) { | |
| flow(new NoRollbackFlow() { | |
| String __name__ = "delete-volume-from-primary-storage"; | |
| @Override | |
| public boolean skip(Map data) { | |
| return pluginRgty.getExtensionList(VolumeBeforeExpungeExtensionPoint.class).stream() | |
| .anyMatch(ext -> ext.skipExpungeVolume(self.toInventory())); | |
| } | |
| @Override | |
| public void run(final FlowTrigger trigger, Map data) { | |
| if (self.getStatus() == VolumeStatus.Ready && | |
| self.getPrimaryStorageUuid() != null) { | |
| DeleteVolumeOnPrimaryStorageMsg dmsg = new DeleteVolumeOnPrimaryStorageMsg(); | |
| dmsg.setVolume(getSelfInventory()); | |
| dmsg.setUuid(self.getPrimaryStorageUuid()); | |
| bus.makeTargetServiceIdByResourceUuid(dmsg, PrimaryStorageConstant.SERVICE_ID, self.getPrimaryStorageUuid()); | |
| logger.debug(String.format("Asking primary storage[uuid:%s] to remove data volume[uuid:%s]", self.getPrimaryStorageUuid(), | |
| self.getUuid())); | |
| bus.send(dmsg, new CloudBusCallBack(trigger) { | |
| @Override | |
| public void run(MessageReply reply) { | |
| if (!reply.isSuccess()) { | |
| logger.warn(String.format("failed to delete volume[uuid:%s, name:%s], %s", | |
| self.getUuid(), self.getName(), reply.getError())); | |
| } | |
| trigger.next(); | |
| DeleteVolumeOnPrimaryStorageMsg dmsg = new DeleteVolumeOnPrimaryStorageMsg(); | |
| dmsg.setVolume(getSelfInventory()); | |
| dmsg.setUuid(self.getPrimaryStorageUuid()); | |
| bus.makeTargetServiceIdByResourceUuid(dmsg, PrimaryStorageConstant.SERVICE_ID, self.getPrimaryStorageUuid()); | |
| logger.debug(String.format("Asking primary storage[uuid:%s] to remove data volume[uuid:%s]", self.getPrimaryStorageUuid(), | |
| self.getUuid())); | |
| bus.send(dmsg, new CloudBusCallBack(trigger) { | |
| @Override | |
| public void run(MessageReply reply) { | |
| if (!reply.isSuccess()) { | |
| logger.warn(String.format("failed to delete volume[uuid:%s, name:%s], %s", | |
| self.getUuid(), self.getName(), reply.getError())); | |
| } | |
| }); | |
| } else { | |
| trigger.next(); | |
| } | |
| trigger.next(); | |
| } | |
| }); | |
| boolean deleteFromPs = deletionPolicy == VolumeDeletionPolicy.Direct; | |
| boolean releaseCapFromPs = deleteFromPs && self.getPrimaryStorageUuid() != null; | |
| if (deleteFromPs) { | |
| for (VolumeBeforeExpungeExtensionPoint ext : pluginRgty.getExtensionList(VolumeBeforeExpungeExtensionPoint.class)) { | |
| if (ext.skipExpungeVolume(self.toInventory())) { | |
| deleteFromPs = false; | |
| logger.info(String.format("Volume[uuid:%s] deletion skipped by extension[%s]", | |
| self.getUuid(), ext.getClass().getSimpleName())); | |
| break; | |
| } | |
| }; | |
| // some task will create a NotInstantiated volume but instantiated actually, means the volume is not ready to delete. | |
| if (self.getStatus() != VolumeStatus.Ready || self.getPrimaryStorageUuid() == null) { | |
| deleteFromPs = false; | |
| logger.debug(String.format("volume[uuid:%s] is not ready or has no primary storage, skip delete volume from primary storage", self.getUuid())); | |
| } | |
| } | |
| if (deleteFromPs) { | |
| flow(new NoRollbackFlow() { | |
| String __name__ = "delete-volume-from-primary-storage"; | |
| @Override | |
| public void run(final FlowTrigger trigger, Map data) { | |
| DeleteVolumeOnPrimaryStorageMsg dmsg = new DeleteVolumeOnPrimaryStorageMsg(); | |
| dmsg.setVolume(getSelfInventory()); | |
| dmsg.setUuid(self.getPrimaryStorageUuid()); | |
| bus.makeTargetServiceIdByResourceUuid(dmsg, PrimaryStorageConstant.SERVICE_ID, self.getPrimaryStorageUuid()); | |
| logger.debug(String.format("Asking primary storage[uuid:%s] to remove data volume[uuid:%s]", self.getPrimaryStorageUuid(), | |
| self.getUuid())); | |
| bus.send(dmsg, new CloudBusCallBack(trigger) { | |
| @Override | |
| public void run(MessageReply reply) { | |
| if (!reply.isSuccess()) { | |
| logger.warn(String.format("failed to delete volume[uuid:%s, name:%s], %s", | |
| self.getUuid(), self.getName(), reply.getError())); | |
| } | |
| trigger.next(); | |
| } | |
| }); |
| bus.makeTargetServiceIdByResourceUuid(amsg, PrimaryStorageConstant.SERVICE_ID, amsg.getRequiredPrimaryStorageUuid()); | ||
| return amsg; | ||
| }).collect(Collectors.toList()); | ||
| bus.send(amsgs); |
There was a problem hiding this comment.
缺少对 bus.send(amsgs) 的错误处理
在调用 bus.send(amsgs) 时，没有处理可能发生的异常或错误。如果消息发送失败，可能会导致容量分配不正确。建议添加错误处理，确保在发送失败时进行适当的处理或重试。
| bus.makeTargetServiceIdByResourceUuid(amsg, PrimaryStorageConstant.SERVICE_ID, amsg.getPrimaryStorageUuid()); | ||
| return amsg; | ||
| }).collect(Collectors.toList()); | ||
| bus.send(rmsgs); |
There was a problem hiding this comment.
缺少对 bus.send(rmsgs) 的错误处理
同样地，bus.send(rmsgs) 调用缺少错误处理。如果释放容量的消息发送失败，可能导致容量统计不准确。建议添加错误处理，确保系统能正确响应消息发送失败的情况。
Comment on lines
+84
to
+85
| for (StorageTrashLifeCycleExtensionPoint ext : pluginRgty.getExtensionList(StorageTrashLifeCycleExtensionPoint.class)) { | ||
| ext.beforeCreateTrash(vo); |
There was a problem hiding this comment.
建议为扩展点方法添加异常处理
在调用 ext.beforeCreateTrash(vo) 时，如果扩展点实现抛出异常，可能导致循环中断，影响后续逻辑。建议在调用扩展点方法时添加异常处理，防止单个扩展点的错误影响整体流程。
Also applies to: 120-121
| @@ -139,6 +138,44 @@ public InstallPathRecycleInventory createTrash(TrashType type, boolean isFolder, | |||
| } | |||
| } | |||
|
|
|||
| public void decreaseCapacityAfterCreateTrash(List<Long> trashIds) { | |||
| if (trashIds.isEmpty()) { | |||
There was a problem hiding this comment.
建议在方法开始处添加对 trashIds 的空值检查
当前代码假设 trashIds 不为 null，但未进行空值检查。如果 trashIds 为空，会导致 NullPointerException。建议在方法开头添加对 trashIds 是否为 null 的检查，确保代码健壮性。
Also applies to: 161-161
MatheMatrix
force-pushed
the
sync/shan.wu/fix-ZSTAC-68260-2@@2
branch
from
0204ea7 to
11c6284
Compare
1、when calculating the primary storage capacity, it is necessary to calculate the trash size. 2、When creating a volume trash, the size should be the actual size and necessary to reduce capacity. Resolves/Related: ZSTAC-68260 Change-Id: I62656562636d7777646e6f6b787769727a716f6d
MatheMatrix
force-pushed
the
sync/shan.wu/fix-ZSTAC-68260-2@@2
branch
from
11c6284 to
09850db
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
1、when calculating the primary storage capacity, it is necessary to calculate the trash size.
2、When creating a volume trash, the size should be the actual size and necessary to reduce capacity.
Resolves/Related: ZSTAC-68260
Change-Id: I62656562636d7777646e6f6b787769727a716f6d
sync from gitlab !7294