Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

<fix>[trash]: fix storage trash capacity calculation #1572

Open
wants to merge 1 commit into
base: 4.8.24
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions conf/springConfigXml/gc.xml
Original file line number Diff line number Diff line change
Expand Up @@ -35,4 +35,10 @@
<zstack:extension interface="org.zstack.header.storage.snapshot.VolumeSnapshotAfterDeleteExtensionPoint"/>
</zstack:plugin>
</bean>

<bean id="DefaultPrimaryStorageTrashLifeCycleExtension" class="org.zstack.core.trash.DefaultPrimaryStorageTrashLifeCycleExtension" >
<zstack:plugin>
<zstack:extension interface="org.zstack.core.trash.StorageTrashLifeCycleExtensionPoint" order="-1"/>
</zstack:plugin>
</bean>
</beans>
8 changes: 7 additions & 1 deletion conf/springConfigXml/localStorage.xml
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@
<zstack:extension interface="org.zstack.header.storage.snapshot.AfterTakeLiveSnapshotsOnVolumes"/>
<zstack:extension interface="org.zstack.compute.vm.VmCapabilitiesExtensionPoint"/>
<zstack:extension interface="org.zstack.header.storage.primary.PrimaryStorageDetachExtensionPoint"/>
<zstack:extension interface="org.zstack.core.trash.CreateRecycleExtensionPoint"/>
<zstack:extension interface="org.zstack.header.volume.AfterInstantiateVolumeExtensionPoint"/>
</zstack:plugin>
</bean>
Expand Down Expand Up @@ -141,4 +140,11 @@
<zstack:extension interface="org.zstack.header.managementnode.ManagementNodeReadyExtensionPoint" order="-2"/>
</zstack:plugin>
</bean>

<bean id="LocalPrimaryStorageTrashLifeCycleExtension" class="org.zstack.storage.primary.local.LocalPrimaryStorageTrashLifeCycleExtension">
<zstack:plugin>
<zstack:extension interface="org.zstack.core.trash.StorageTrashLifeCycleExtensionPoint"/>
</zstack:plugin>
</bean>

</beans>

This file was deleted.

Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
package org.zstack.core.trash;

import org.springframework.beans.factory.annotation.Autowired;
import org.zstack.core.cloudbus.CloudBus;
import org.zstack.core.db.DatabaseFacade;
import org.zstack.header.core.trash.InstallPathRecycleInventory;
import org.zstack.header.core.trash.InstallPathRecycleVO;
import org.zstack.header.storage.primary.AllocatePrimaryStorageSpaceMsg;
import org.zstack.header.storage.primary.PrimaryStorageConstant;
import org.zstack.header.storage.primary.PrimaryStorageVO;
import org.zstack.header.storage.primary.ReleasePrimaryStorageSpaceMsg;
import org.zstack.utils.Utils;
import org.zstack.utils.logging.CLogger;

import java.util.Collections;
import java.util.List;
import java.util.Map;
import java.util.stream.Collectors;

public class DefaultPrimaryStorageTrashLifeCycleExtension implements StorageTrashLifeCycleExtensionPoint {
@Autowired
private DatabaseFacade dbf;

@Autowired
private CloudBus bus;

private final static CLogger logger = Utils.getLogger(DefaultPrimaryStorageTrashLifeCycleExtension.class);

protected boolean isPrimaryStorage(String storageType) {
return PrimaryStorageVO.class.getSimpleName().equals(storageType);
}

@Override
public void beforeCreateTrash(InstallPathRecycleVO vo) {
}

@Override
public List<Long> afterCreateTrash(List<InstallPathRecycleInventory> inventoryList) {
if (inventoryList.isEmpty()) {
return Collections.EMPTY_LIST;
}
Map<String, Long> storageTrashMap = inventoryList.stream().filter(i -> isPrimaryStorage(i.getStorageType()))
.collect(Collectors.groupingBy(InstallPathRecycleInventory::getStorageUuid,
Collectors.summingLong(InstallPathRecycleInventory::getSize)));
List<AllocatePrimaryStorageSpaceMsg> amsgs = storageTrashMap.entrySet().stream().map(e -> {
String storageUuid = e.getKey();
Long totalTrashSize = e.getValue();
AllocatePrimaryStorageSpaceMsg amsg = new AllocatePrimaryStorageSpaceMsg();
amsg.setForce(true);
amsg.setRequiredPrimaryStorageUuid(storageUuid);
amsg.setNoOverProvisioning(true);
amsg.setRequiredInstallUri(inventoryList.get(0).getInstallPath());
amsg.setSize(totalTrashSize);
bus.makeTargetServiceIdByResourceUuid(amsg, PrimaryStorageConstant.SERVICE_ID, amsg.getRequiredPrimaryStorageUuid());
return amsg;
}).collect(Collectors.toList());
bus.send(amsgs);
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

缺少对 bus.send(amsgs) 的错误处理

在调用 bus.send(amsgs) 时,没有处理可能发生的异常或错误。如果消息发送失败,可能会导致容量分配不正确。建议添加错误处理,确保在发送失败时进行适当的处理或重试。

return inventoryList.stream().map(InstallPathRecycleInventory::getTrashId).collect(Collectors.toList());
}

@Override
public List<Long> beforeRemoveTrash(List<InstallPathRecycleInventory> inventoryList) {
if (inventoryList.isEmpty()) {
return Collections.EMPTY_LIST;
}
Map<String, Long> storageTrashMap = inventoryList.stream().filter(i -> isPrimaryStorage(i.getStorageType()))
.collect(Collectors.groupingBy(InstallPathRecycleInventory::getStorageUuid,
Collectors.summingLong(InstallPathRecycleInventory::getSize)));
List<ReleasePrimaryStorageSpaceMsg> rmsgs = storageTrashMap.entrySet().stream().map(e -> {
String storageUuid = e.getKey();
Long totalTrashSize = e.getValue();
ReleasePrimaryStorageSpaceMsg amsg = new ReleasePrimaryStorageSpaceMsg();
amsg.setPrimaryStorageUuid(storageUuid);
amsg.setNoOverProvisioning(true);
amsg.setAllocatedInstallUrl(inventoryList.get(0).getInstallPath());
amsg.setDiskSize(totalTrashSize);
bus.makeTargetServiceIdByResourceUuid(amsg, PrimaryStorageConstant.SERVICE_ID, amsg.getPrimaryStorageUuid());
return amsg;
}).collect(Collectors.toList());
bus.send(rmsgs);
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

缺少对 bus.send(rmsgs) 的错误处理

同样地,bus.send(rmsgs) 调用缺少错误处理。如果释放容量的消息发送失败,可能导致容量统计不准确。建议添加错误处理,确保系统能正确响应消息发送失败的情况。

return inventoryList.stream().map(InstallPathRecycleInventory::getTrashId).collect(Collectors.toList());
}


}
53 changes: 45 additions & 8 deletions core/src/main/java/org/zstack/core/trash/StorageRecycleImpl.java
Original file line number Diff line number Diff line change
Expand Up @@ -34,14 +34,13 @@
import org.zstack.header.vo.ResourceVO;
import org.zstack.header.volume.*;
import org.zstack.utils.CollectionDSL;
import org.zstack.utils.CollectionUtils;
import org.zstack.utils.DebugUtils;
import org.zstack.utils.Utils;
import org.zstack.utils.gson.JSONObjectUtil;
import org.zstack.utils.logging.CLogger;

import java.util.ArrayList;
import java.util.Iterator;
import java.util.List;
import java.util.*;
import java.util.concurrent.TimeUnit;

import static org.zstack.core.Platform.inerr;
Expand Down Expand Up @@ -80,10 +79,10 @@ private InstallPathRecycleInventory createRecycleFromVolume(TrashType type, bool
vo.setStorageUuid(vol.getPrimaryStorageUuid());
vo.setStorageType(PrimaryStorageVO.class.getSimpleName());
vo.setTrashType(type.toString());
vo.setSize(vol.getSize());
vo.setSize(vol.getActualSize());

for (CreateRecycleExtensionPoint ext : pluginRgty.getExtensionList(CreateRecycleExtensionPoint.class)) {
ext.setHostUuid(vo, vol.getPrimaryStorageUuid());
for (StorageTrashLifeCycleExtensionPoint ext : pluginRgty.getExtensionList(StorageTrashLifeCycleExtensionPoint.class)) {
ext.beforeCreateTrash(vo);
Comment on lines +84 to +85
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

建议为扩展点方法添加异常处理

在调用 ext.beforeCreateTrash(vo) 时,如果扩展点实现抛出异常,可能导致循环中断,影响后续逻辑。建议在调用扩展点方法时添加异常处理,防止单个扩展点的错误影响整体流程。

Also applies to: 120-121

}

vo = dbf.persistAndRefresh(vo);
Expand Down Expand Up @@ -118,8 +117,8 @@ private InstallPathRecycleInventory createRecycleFromVolumeSnapshot(TrashType ty
vo.setTrashType(type.toString());
vo.setSize(snapshot.getSize());

for (CreateRecycleExtensionPoint ext : pluginRgty.getExtensionList(CreateRecycleExtensionPoint.class)) {
ext.setHostUuid(vo, snapshot.getPrimaryStorageUuid());
for (StorageTrashLifeCycleExtensionPoint ext : pluginRgty.getExtensionList(StorageTrashLifeCycleExtensionPoint.class)) {
ext.beforeCreateTrash(vo);
}

vo = dbf.persistAndRefresh(vo);
Expand All @@ -139,6 +138,44 @@ public InstallPathRecycleInventory createTrash(TrashType type, boolean isFolder,
}
}

public void decreaseCapacityAfterCreateTrash(List<Long> trashIds) {
if (trashIds.isEmpty()) {
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

建议在方法开始处添加对 trashIds 的空值检查

当前代码假设 trashIds 不为 null,但未进行空值检查。如果 trashIds 为空,会导致 NullPointerException。建议在方法开头添加对 trashIds 是否为 null 的检查,确保代码健壮性。

Also applies to: 161-161

return;
}
List<InstallPathRecycleVO> trashVOs = Q.New(InstallPathRecycleVO.class)
.in(InstallPathRecycleVO_.trashId, trashIds)
.list();
if (trashVOs.isEmpty()) {
return;
}

List<InstallPathRecycleInventory> trashList = InstallPathRecycleInventory.valueOf(trashVOs);

CollectionUtils.safeForEach(pluginRgty.getExtensionList(StorageTrashLifeCycleExtensionPoint.class), ext -> {
List<Long> trash = ext.afterCreateTrash(trashList);
trashList.removeIf(t -> trash.contains(t.getTrashId()));
});
}

public void increaseCapacityBeforeRemoveTrash(List<Long> trashIds) {
if (trashIds.isEmpty()) {
return;
}
List<InstallPathRecycleVO> trashVOs = Q.New(InstallPathRecycleVO.class)
.in(InstallPathRecycleVO_.trashId, trashIds)
.list();
if (trashVOs.isEmpty()) {
return;
}

List<InstallPathRecycleInventory> trashList = InstallPathRecycleInventory.valueOf(trashVOs);

CollectionUtils.safeForEach(pluginRgty.getExtensionList(StorageTrashLifeCycleExtensionPoint.class), ext -> {
List<Long> trash = ext.beforeRemoveTrash(trashList);
trashList.removeIf(t -> trash.contains(t.getTrashId()));
});
}

@Override
public List<InstallPathRecycleInventory> getTrashList(String storageUuid) {
return getTrashList(storageUuid, CollectionDSL.list(TrashType.values()));
Expand Down
4 changes: 4 additions & 0 deletions core/src/main/java/org/zstack/core/trash/StorageTrash.java
Original file line number Diff line number Diff line change
Expand Up @@ -20,5 +20,9 @@ public interface StorageTrash {
List<String> findTrashInstallPath(String installPath, String storageUuid);
Long getTrashId(String storageUuid, String installPath);

void decreaseCapacityAfterCreateTrash(List<Long> trashIds);

void increaseCapacityBeforeRemoveTrash(List<Long> trashIds);

void removeFromDb(Long trashId);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
package org.zstack.core.trash;

import org.zstack.header.core.trash.InstallPathRecycleInventory;
import org.zstack.header.core.trash.InstallPathRecycleVO;

import java.util.List;

/**
* Created by mingjian.deng on 2019/10/29.
*/
public interface StorageTrashLifeCycleExtensionPoint {
void beforeCreateTrash(InstallPathRecycleVO vo);

List<Long> afterCreateTrash(List<InstallPathRecycleInventory> inventoryList);

List<Long> beforeRemoveTrash(List<InstallPathRecycleInventory> inventoryList);
}
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
public class SyncVolumeSizeOnPrimaryStorageReply extends MessageReply {
private long actualSize;
private long size;
private boolean withInternalSnapshot;
private boolean supportInternalSnapshot;

public long getSize() {
return size;
Expand All @@ -26,11 +26,11 @@ public void setActualSize(long actualSize) {
this.actualSize = actualSize;
}

public void setWithInternalSnapshot(boolean withInternalSnapshot) {
this.withInternalSnapshot = withInternalSnapshot;
public void setSupportInternalSnapshot(boolean supportInternalSnapshot) {
this.supportInternalSnapshot = supportInternalSnapshot;
}

public boolean isWithInternalSnapshot() {
return withInternalSnapshot;
public boolean isSupportInternalSnapshot() {
return supportInternalSnapshot;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
public class SyncVolumeSizeReply extends MessageReply {
private long actualSize;
private long size;
private Long independentSnapshotSize;

public long getSize() {
return size;
Expand All @@ -24,4 +25,12 @@ public long getActualSize() {
public void setActualSize(long actualSize) {
this.actualSize = actualSize;
}

public Long getIndependentSnapshotSize() {
return independentSnapshotSize;
}

public void setIndependentSnapshotSize(Long independentSnapshotSize) {
this.independentSnapshotSize = independentSnapshotSize;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -1834,11 +1834,7 @@ public void run(MessageReply reply) {
chain.done(new FlowDoneHandler(completion) {
@Override
public void handle(Map data) {
IncreasePrimaryStorageCapacityMsg imsg = new IncreasePrimaryStorageCapacityMsg();
imsg.setPrimaryStorageUuid(self.getUuid());
imsg.setDiskSize(inv.getSize());
bus.makeTargetServiceIdByResourceUuid(imsg, PrimaryStorageConstant.SERVICE_ID, self.getUuid());
bus.send(imsg);
trash.increaseCapacityBeforeRemoveTrash(Collections.singletonList(trashId));
logger.info(String.format("Returned space[size:%s] to PS %s after volume migration", inv.getSize(), self.getUuid()));
trash.removeFromDb(trashId);

Expand Down Expand Up @@ -1929,11 +1925,7 @@ public void run(MessageReply reply) {
chain.done(new FlowDoneHandler(coml) {
@Override
public void handle(Map data) {
IncreasePrimaryStorageCapacityMsg imsg = new IncreasePrimaryStorageCapacityMsg();
imsg.setPrimaryStorageUuid(self.getUuid());
imsg.setDiskSize(inv.getSize());
bus.makeTargetServiceIdByResourceUuid(imsg, PrimaryStorageConstant.SERVICE_ID, self.getUuid());
bus.send(imsg);
trash.increaseCapacityBeforeRemoveTrash(Collections.singletonList(inv.getTrashId()));
logger.info(String.format("Returned space[size:%s] to PS %s after volume migration", inv.getSize(), self.getUuid()));

results.add(new TrashCleanupResult(inv.getResourceUuid(), inv.getTrashId(), inv.getSize()));
Expand Down Expand Up @@ -3296,7 +3288,7 @@ public void success(GetVolumeSizeRsp rsp) {
.findValue();
reply.setActualSize(asize);
reply.setSize(rsp.size);
reply.setWithInternalSnapshot(true);
reply.setSupportInternalSnapshot(true);
bus.reply(msg, reply);
}

Expand Down
Loading