diff --git a/plugin/expon/src/main/java/org/zstack/expon/ExponStorageFactory.java b/plugin/expon/src/main/java/org/zstack/expon/ExponStorageFactory.java index 4086058d198..db194c9a415 100644 --- a/plugin/expon/src/main/java/org/zstack/expon/ExponStorageFactory.java +++ b/plugin/expon/src/main/java/org/zstack/expon/ExponStorageFactory.java @@ -8,6 +8,7 @@ import org.zstack.header.volume.VolumeInventory; import org.zstack.header.volume.VolumeProtocol; +import java.util.ArrayList; import java.util.LinkedHashMap; import java.util.List; import java.util.Map; @@ -48,7 +49,7 @@ public String getIdentity() { @Override public List getPreferBackupStorageTypes() { - return preferBackupStorageTypes; + return new ArrayList<>(preferBackupStorageTypes); } public void setPreferBackupStorageTypes(List preferBackupStorageTypes) { diff --git a/plugin/xinfini/src/main/java/org/zstack/xinfini/XInfiniStorageFactory.java b/plugin/xinfini/src/main/java/org/zstack/xinfini/XInfiniStorageFactory.java index 70d33ea08d0..c3f98670bb6 100644 --- a/plugin/xinfini/src/main/java/org/zstack/xinfini/XInfiniStorageFactory.java +++ b/plugin/xinfini/src/main/java/org/zstack/xinfini/XInfiniStorageFactory.java @@ -11,6 +11,7 @@ import org.zstack.header.xinfini.XInfiniConstants; import org.zstack.storage.addon.primary.ExternalPrimaryStorageFactory; +import java.util.ArrayList; import java.util.LinkedHashMap; import java.util.List; @@ -55,7 +56,7 @@ public String getIdentity() { @Override public List getPreferBackupStorageTypes() { - return preferBackupStorageTypes; + return new ArrayList<>(preferBackupStorageTypes); } public void setPreferBackupStorageTypes(List preferBackupStorageTypes) { diff --git a/plugin/zbs/src/main/java/org/zstack/storage/zbs/ZbsStorageFactory.java b/plugin/zbs/src/main/java/org/zstack/storage/zbs/ZbsStorageFactory.java index 5b7c491814e..9df6cd4c97a 100644 --- a/plugin/zbs/src/main/java/org/zstack/storage/zbs/ZbsStorageFactory.java +++ b/plugin/zbs/src/main/java/org/zstack/storage/zbs/ZbsStorageFactory.java @@ -11,6 +11,7 @@ import org.zstack.utils.ssh.Ssh; import org.zstack.utils.ssh.SshResult; +import java.util.ArrayList; import java.util.List; import static org.zstack.core.Platform.operr; @@ -93,7 +94,7 @@ public void setPreferBackupStorageTypes(List preferBackupStorageTypes) { @Override public List getPreferBackupStorageTypes() { - return preferBackupStorageTypes; + return new ArrayList<>(preferBackupStorageTypes); } @Override diff --git a/test/src/test/groovy/org/zstack/test/integration/storage/primary/addon/ExternalPrimaryStorageSelectBackupStorageCase.groovy b/test/src/test/groovy/org/zstack/test/integration/storage/primary/addon/ExternalPrimaryStorageSelectBackupStorageCase.groovy index 9419db12215..b1161c3ae3f 100644 --- a/test/src/test/groovy/org/zstack/test/integration/storage/primary/addon/ExternalPrimaryStorageSelectBackupStorageCase.groovy +++ b/test/src/test/groovy/org/zstack/test/integration/storage/primary/addon/ExternalPrimaryStorageSelectBackupStorageCase.groovy @@ -13,7 +13,12 @@ import org.zstack.header.storage.backup.BackupStorageZoneRefVO_ import org.zstack.header.storage.primary.PrimaryStorageConstant import org.zstack.header.storage.primary.SelectBackupStorageMsg import org.zstack.header.storage.primary.SelectBackupStorageReply +import org.zstack.sdk.ClusterInventory +import org.zstack.sdk.ImageInventory +import org.zstack.sdk.InstanceOfferingInventory +import org.zstack.sdk.L3NetworkInventory import org.zstack.sdk.PrimaryStorageInventory +import org.zstack.sdk.VmInstanceInventory import org.zstack.sdk.ZoneInventory import org.zstack.test.integration.storage.StorageTest import org.zstack.testlib.EnvSpec @@ -21,21 +26,29 @@ import org.zstack.testlib.SubCase import org.zstack.utils.data.SizeUnit /** - * ZSTAC-71706: ExternalPrimaryStorage backup storage selection in mixed environment. + * ZSTAC-80789: getPreferBackupStorageTypes() must return a defensive copy. * - * Bug: List.indexOf() returns -1 for types not in preferBsTypes, - * causing ascending sort to place non-preferred types (e.g. VCenterBackupStorage) - * before preferred types in the sorted result. + * Bug: ZbsStorageFactory/ExponStorageFactory/XInfiniStorageFactory returned a direct + * reference to their internal preferBackupStorageTypes list. When SelectBackupStorageMsg + * carried requiredBackupStorageTypes, the handler called retainAll() on the returned list, + * permanently mutating the bean's internal state. Subsequent requests without + * requiredBackupStorageTypes would then see an empty preferBsTypes and fail. * - * Fix: Filter out non-preferred backup storage types before sorting by preference. + * Fix: Return new ArrayList<>(preferBackupStorageTypes) from getPreferBackupStorageTypes(). * - * This case sets up a ZBS ExternalPrimaryStorage (preferBsTypes = [ImageStoreBackupStorage]) - * with multiple backup storage types attached to the zone, then sends SelectBackupStorageMsg - * via CloudBus to verify the handler selects the correct preferred backup storage. + * This case sets up a ZBS ExternalPrimaryStorage with both ImageStoreBackupStorage and + * CephBackupStorage attached to the zone, creates a VM on the ZBS PS, then simulates + * two SelectBackupStorageMsg calls (as would happen during clone VM in premium): + * 1st with requiredBackupStorageTypes=["CephBackupStorage"] (triggers retainAll), + * 2nd without requiredBackupStorageTypes (verifies bean is not corrupted). + * + * Note: SelectBackupStorageMsg is sent via CloudBus because the sender (clone VM flow) + * is in the premium module and not available in open-source integration tests. */ class ExternalPrimaryStorageSelectBackupStorageCase extends SubCase { EnvSpec env ZoneInventory zone + ClusterInventory cluster PrimaryStorageInventory ps DatabaseFacade dbf CloudBus bus @@ -60,6 +73,12 @@ class ExternalPrimaryStorageSelectBackupStorageCase extends SubCase { @Override void environment() { env = makeEnv { + instanceOffering { + name = "instanceOffering" + memory = SizeUnit.GIGABYTE.toByte(8) + cpu = 4 + } + sftpBackupStorage { name = "sftp" url = "/sftp" @@ -126,54 +145,82 @@ class ExternalPrimaryStorageSelectBackupStorageCase extends SubCase { void test() { env.create { zone = env.inventoryByName("zone") as ZoneInventory + cluster = env.inventoryByName("cluster") as ClusterInventory ps = env.inventoryByName("zbs-ps") as PrimaryStorageInventory dbf = bean(DatabaseFacade.class) bus = bean(CloudBus.class) - testErrorWhenNoPreferredTypeAvailable() - testSelectPreferredOverNonPreferred() + testPreferBsTypesNotCorruptedByRetainAll() } } /** - * When only non-preferred backup storage types exist in the zone, - * the selection should return an error (no matching preferred types). - * Zone has SftpBackupStorage (from env) and VCenterBackupStorage, - * neither of which is in zbs's preferBsTypes [ImageStoreBackupStorage]. + * Reproduces ZSTAC-80789: retainAll corrupts bean's preferBackupStorageTypes. + * + * Scenario (simulates clone VM on ZBS with mixed BS types): + * 1. Set up zone with ImageStoreBackupStorage (ZBS preferred) and CephBackupStorage + * 2. Create a VM on the ZBS primary storage + * 3. Send SelectBackupStorageMsg with requiredBackupStorageTypes=["CephBackupStorage"] + * - This triggers retainAll(["CephBackupStorage"]) on preferBsTypes + * - Before fix: mutates the bean's list, emptying it permanently + * - Expected: fails (no intersection), but bean should remain intact + * 4. Send SelectBackupStorageMsg without requiredBackupStorageTypes + * - Before fix: fails because preferBsTypes was permanently emptied + * - After fix: succeeds, selects ImageStoreBackupStorage */ - void testErrorWhenNoPreferredTypeAvailable() { - createAndAttachBackupStorage("vcenter-bs", "VCenterBackupStorage") - - SelectBackupStorageMsg msg = new SelectBackupStorageMsg() - msg.setPrimaryStorageUuid(ps.uuid) - msg.setRequiredSize(SizeUnit.MEGABYTE.toByte(1)) - bus.makeTargetServiceIdByResourceUuid(msg, PrimaryStorageConstant.SERVICE_ID, ps.uuid) - MessageReply reply = bus.call(msg) - - assert !reply.isSuccess() : "Should fail when no preferred BS type is available" - } - - /** - * Reproduces ZSTAC-71706: zone has both ImageStoreBackupStorage (preferred) - * and VCenterBackupStorage (non-preferred, created in previous test). - * Before the fix, indexOf() returns -1 for VCenterBackupStorage causing - * it to sort before ImageStoreBackupStorage. After the fix, non-preferred - * types are filtered out entirely, and ImageStoreBackupStorage is correctly selected. - */ - void testSelectPreferredOverNonPreferred() { + void testPreferBsTypesNotCorruptedByRetainAll() { + // Set up: attach ImageStoreBackupStorage and CephBackupStorage to the zone createAndAttachBackupStorage("imagestore-bs", "ImageStoreBackupStorage") + createAndAttachBackupStorage("ceph-bs", "CephBackupStorage") + + // Attach PS to cluster and create a VM to establish realistic context + attachPrimaryStorageToCluster { + primaryStorageUuid = ps.uuid + clusterUuid = cluster.uuid + } - SelectBackupStorageMsg msg = new SelectBackupStorageMsg() - msg.setPrimaryStorageUuid(ps.uuid) - msg.setRequiredSize(SizeUnit.MEGABYTE.toByte(1)) - bus.makeTargetServiceIdByResourceUuid(msg, PrimaryStorageConstant.SERVICE_ID, ps.uuid) - MessageReply reply = bus.call(msg) - - assert reply.isSuccess() : "SelectBackupStorageMsg should succeed" - SelectBackupStorageReply bsReply = reply as SelectBackupStorageReply - assert bsReply.inventory != null - assert bsReply.inventory.type == "ImageStoreBackupStorage" : - "Should select preferred ImageStoreBackupStorage, but got ${bsReply.inventory.type}" + def instanceOffering = env.inventoryByName("instanceOffering") as InstanceOfferingInventory + def image = env.inventoryByName("image") as ImageInventory + def l3 = env.inventoryByName("l3") as L3NetworkInventory + + VmInstanceInventory vm = createVmInstance { + name = "test-vm" + instanceOfferingUuid = instanceOffering.uuid + imageUuid = image.uuid + l3NetworkUuids = [l3.uuid] + } as VmInstanceInventory + + assert vm != null : "VM should be created successfully on ZBS primary storage" + + // 1st call: with requiredBackupStorageTypes=["CephBackupStorage"] + // retainAll(["CephBackupStorage"]) on preferBsTypes [ImageStoreBackupStorage] + // => empty intersection => error expected + // Before fix: this permanently empties the bean's preferBackupStorageTypes + SelectBackupStorageMsg msg1 = new SelectBackupStorageMsg() + msg1.setPrimaryStorageUuid(ps.uuid) + msg1.setRequiredSize(SizeUnit.MEGABYTE.toByte(1)) + msg1.setRequiredBackupStorageTypes(["CephBackupStorage"]) + bus.makeTargetServiceIdByResourceUuid(msg1, PrimaryStorageConstant.SERVICE_ID, ps.uuid) + MessageReply reply1 = bus.call(msg1) + + assert !reply1.isSuccess() : "Should fail: no intersection between CephBackupStorage and ZBS prefer types [ImageStoreBackupStorage]" + + // 2nd call: without requiredBackupStorageTypes + // Before fix: preferBsTypes was permanently emptied by the 1st call's retainAll => fails + // After fix: defensive copy means bean is intact => succeeds and selects ImageStoreBackupStorage + SelectBackupStorageMsg msg2 = new SelectBackupStorageMsg() + msg2.setPrimaryStorageUuid(ps.uuid) + msg2.setRequiredSize(SizeUnit.MEGABYTE.toByte(1)) + bus.makeTargetServiceIdByResourceUuid(msg2, PrimaryStorageConstant.SERVICE_ID, ps.uuid) + MessageReply reply2 = bus.call(msg2) + + assert reply2.isSuccess() : + "ZSTAC-80789: second SelectBackupStorageMsg should succeed but failed - " + + "preferBackupStorageTypes was corrupted by previous retainAll()" + SelectBackupStorageReply bsReply2 = reply2 as SelectBackupStorageReply + assert bsReply2.inventory != null + assert bsReply2.inventory.type == "ImageStoreBackupStorage" : + "Should select ImageStoreBackupStorage, but got ${bsReply2.inventory.type}" } private void createAndAttachBackupStorage(String name, String type) {