From 9d25b739c6ca071e74244d54622b5ca66b9f7be8 Mon Sep 17 00:00:00 2001 From: J M Date: Thu, 19 Mar 2026 19:14:43 +0800 Subject: [PATCH 1/5] [storage]: return defensive copy from getPreferBackupStorageTypes to prevent Bean mutation Resolves: ZSTAC-80789 Change-Id: I9348a655bea0bbcff53abfd2c44f73342190f1ac --- .../org/zstack/expon/ExponStorageFactory.java | 3 +- .../zstack/xinfini/XInfiniStorageFactory.java | 3 +- .../zstack/storage/zbs/ZbsStorageFactory.java | 3 +- .../ExternalPrimaryStorageSelectBsTest.java | 90 +++++++++++++++++++ 4 files changed, 96 insertions(+), 3 deletions(-) create mode 100644 storage/src/test/java/org/zstack/storage/addon/primary/ExternalPrimaryStorageSelectBsTest.java 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/storage/src/test/java/org/zstack/storage/addon/primary/ExternalPrimaryStorageSelectBsTest.java b/storage/src/test/java/org/zstack/storage/addon/primary/ExternalPrimaryStorageSelectBsTest.java new file mode 100644 index 00000000000..c041d7d8f1b --- /dev/null +++ b/storage/src/test/java/org/zstack/storage/addon/primary/ExternalPrimaryStorageSelectBsTest.java @@ -0,0 +1,90 @@ +package org.zstack.storage.addon.primary; + +import org.junit.Assert; +import org.junit.Test; +import org.zstack.header.storage.addon.primary.BackupStorageSelector; + +import java.util.ArrayList; +import java.util.Arrays; +import java.util.List; + +/** + * Regression test for ZSTAC-80789. + * + * BackupStorageSelector.getPreferBackupStorageTypes() must return a defensive + * copy so callers using retainAll() cannot corrupt the bean's internal state. + */ +public class ExternalPrimaryStorageSelectBsTest { + + /** + * Simulates the FIXED selector that returns a defensive copy. + */ + private static class FixedSelector implements BackupStorageSelector { + private final List preferBackupStorageTypes; + + FixedSelector(List types) { + this.preferBackupStorageTypes = types; + } + + @Override + public List getPreferBackupStorageTypes() { + return new ArrayList<>(preferBackupStorageTypes); // fixed: defensive copy + } + + @Override + public String getIdentity() { return "fixed"; } + + List getInternalList() { return preferBackupStorageTypes; } + } + + /** + * Simulates the BUGGY selector that returns a direct reference. + */ + private static class BuggySelector implements BackupStorageSelector { + private final List preferBackupStorageTypes; + + BuggySelector(List types) { + this.preferBackupStorageTypes = types; + } + + @Override + public List getPreferBackupStorageTypes() { + return preferBackupStorageTypes; // buggy: direct reference + } + + @Override + public String getIdentity() { return "buggy"; } + } + + @Test + public void testFixedSelectorRetainAllDoesNotCorruptBean() { + List internal = new ArrayList<>(Arrays.asList("ImageStoreBackupStorage")); + FixedSelector selector = new FixedSelector(internal); + + // First call: retainAll with ["CephBackupStorage"] => empty intersection + List result1 = selector.getPreferBackupStorageTypes(); + result1.retainAll(Arrays.asList("CephBackupStorage")); + Assert.assertTrue("retainAll result should be empty", result1.isEmpty()); + + // Bean internal state must be intact + Assert.assertEquals(1, selector.getInternalList().size()); + Assert.assertEquals("ImageStoreBackupStorage", selector.getInternalList().get(0)); + + // Second call: should still return the full list + List result2 = selector.getPreferBackupStorageTypes(); + Assert.assertEquals(Arrays.asList("ImageStoreBackupStorage"), result2); + } + + @Test + public void testBuggySelectorRetainAllCorruptsBean() { + List internal = new ArrayList<>(Arrays.asList("ImageStoreBackupStorage")); + BuggySelector selector = new BuggySelector(internal); + + // First call: retainAll with ["CephBackupStorage"] corrupts the bean + List result1 = selector.getPreferBackupStorageTypes(); + result1.retainAll(Arrays.asList("CephBackupStorage")); + + // Bean's internal list is now permanently empty - this is the bug + Assert.assertTrue("Bean is corrupted: list is empty", selector.getPreferBackupStorageTypes().isEmpty()); + } +} From 16fcfab32738fcc2dffae8d86bfdd0da2b422674 Mon Sep 17 00:00:00 2001 From: J M Date: Fri, 20 Mar 2026 16:30:43 +0800 Subject: [PATCH 2/5] [storage]: replace JUnit test with Groovy integration test for defensive copy fix Resolves: ZSTAC-80789 Change-Id: I23b24745b605b0d861dd3fdab11cf2044b31e5fe --- .../ExternalPrimaryStorageSelectBsTest.java | 90 ------------------- ...imaryStorageSelectBackupStorageCase.groovy | 50 +++++++++++ 2 files changed, 50 insertions(+), 90 deletions(-) delete mode 100644 storage/src/test/java/org/zstack/storage/addon/primary/ExternalPrimaryStorageSelectBsTest.java diff --git a/storage/src/test/java/org/zstack/storage/addon/primary/ExternalPrimaryStorageSelectBsTest.java b/storage/src/test/java/org/zstack/storage/addon/primary/ExternalPrimaryStorageSelectBsTest.java deleted file mode 100644 index c041d7d8f1b..00000000000 --- a/storage/src/test/java/org/zstack/storage/addon/primary/ExternalPrimaryStorageSelectBsTest.java +++ /dev/null @@ -1,90 +0,0 @@ -package org.zstack.storage.addon.primary; - -import org.junit.Assert; -import org.junit.Test; -import org.zstack.header.storage.addon.primary.BackupStorageSelector; - -import java.util.ArrayList; -import java.util.Arrays; -import java.util.List; - -/** - * Regression test for ZSTAC-80789. - * - * BackupStorageSelector.getPreferBackupStorageTypes() must return a defensive - * copy so callers using retainAll() cannot corrupt the bean's internal state. - */ -public class ExternalPrimaryStorageSelectBsTest { - - /** - * Simulates the FIXED selector that returns a defensive copy. - */ - private static class FixedSelector implements BackupStorageSelector { - private final List preferBackupStorageTypes; - - FixedSelector(List types) { - this.preferBackupStorageTypes = types; - } - - @Override - public List getPreferBackupStorageTypes() { - return new ArrayList<>(preferBackupStorageTypes); // fixed: defensive copy - } - - @Override - public String getIdentity() { return "fixed"; } - - List getInternalList() { return preferBackupStorageTypes; } - } - - /** - * Simulates the BUGGY selector that returns a direct reference. - */ - private static class BuggySelector implements BackupStorageSelector { - private final List preferBackupStorageTypes; - - BuggySelector(List types) { - this.preferBackupStorageTypes = types; - } - - @Override - public List getPreferBackupStorageTypes() { - return preferBackupStorageTypes; // buggy: direct reference - } - - @Override - public String getIdentity() { return "buggy"; } - } - - @Test - public void testFixedSelectorRetainAllDoesNotCorruptBean() { - List internal = new ArrayList<>(Arrays.asList("ImageStoreBackupStorage")); - FixedSelector selector = new FixedSelector(internal); - - // First call: retainAll with ["CephBackupStorage"] => empty intersection - List result1 = selector.getPreferBackupStorageTypes(); - result1.retainAll(Arrays.asList("CephBackupStorage")); - Assert.assertTrue("retainAll result should be empty", result1.isEmpty()); - - // Bean internal state must be intact - Assert.assertEquals(1, selector.getInternalList().size()); - Assert.assertEquals("ImageStoreBackupStorage", selector.getInternalList().get(0)); - - // Second call: should still return the full list - List result2 = selector.getPreferBackupStorageTypes(); - Assert.assertEquals(Arrays.asList("ImageStoreBackupStorage"), result2); - } - - @Test - public void testBuggySelectorRetainAllCorruptsBean() { - List internal = new ArrayList<>(Arrays.asList("ImageStoreBackupStorage")); - BuggySelector selector = new BuggySelector(internal); - - // First call: retainAll with ["CephBackupStorage"] corrupts the bean - List result1 = selector.getPreferBackupStorageTypes(); - result1.retainAll(Arrays.asList("CephBackupStorage")); - - // Bean's internal list is now permanently empty - this is the bug - Assert.assertTrue("Bean is corrupted: list is empty", selector.getPreferBackupStorageTypes().isEmpty()); - } -} 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..12db3679d48 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 @@ -132,6 +132,7 @@ class ExternalPrimaryStorageSelectBackupStorageCase extends SubCase { testErrorWhenNoPreferredTypeAvailable() testSelectPreferredOverNonPreferred() + testPreferBsTypesNotCorruptedByRetainAll() } } @@ -176,6 +177,55 @@ class ExternalPrimaryStorageSelectBackupStorageCase extends SubCase { "Should select preferred ImageStoreBackupStorage, but got ${bsReply.inventory.type}" } + /** + * Reproduces ZSTAC-80789: getPreferBackupStorageTypes() must return a defensive copy. + * + * 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: Return new ArrayList<>(preferBackupStorageTypes) from getPreferBackupStorageTypes(). + * + * Test: send SelectBackupStorageMsg with requiredBackupStorageTypes=["CephBackupStorage"] + * (no intersection with ZBS's prefer types), then send again without requiredBackupStorageTypes. + * Before the fix, the second call fails because the bean's list was emptied by retainAll(). + */ + void testPreferBsTypesNotCorruptedByRetainAll() { + // imagestore-bs was already created in testSelectPreferredOverNonPreferred + // ZBS prefers [ImageStoreBackupStorage]; zone has ImageStoreBackupStorage attached + + // 1st call: with requiredBackupStorageTypes=["CephBackupStorage"] + // retainAll(["CephBackupStorage"]) on preferBsTypes => empty intersection => error expected + // Before fix: this mutates the bean, emptying preferBackupStorageTypes permanently + 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" + + // 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) { String uuid = Platform.getUuid() From f612c77d064dee40bb74f34dcf1448725c54957e Mon Sep 17 00:00:00 2001 From: J M Date: Fri, 20 Mar 2026 17:13:39 +0800 Subject: [PATCH 3/5] [storage]: improve retainAll bug test with VM and dual BS Rewrite ExternalPrimaryStorageSelectBackupStorageCase to create a VM on ZBS ExternalPrimaryStorage via API, attach both ImageStoreBackupStorage and CephBackupStorage to the zone, and focus solely on the ZSTAC-80789 retainAll corruption bug. Remove unrelated ZSTAC-71706 tests. Resolves: ZSTAC-80789 Change-Id: Ia4ff4433a93606bf8ff4f876ae2693388181cac9 --- ...imaryStorageSelectBackupStorageCase.groovy | 133 +++++++++--------- 1 file changed, 65 insertions(+), 68 deletions(-) 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 12db3679d48..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,79 +145,57 @@ 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") + void testPreferBsTypesNotCorruptedByRetainAll() { + // Set up: attach ImageStoreBackupStorage and CephBackupStorage to the zone + createAndAttachBackupStorage("imagestore-bs", "ImageStoreBackupStorage") + createAndAttachBackupStorage("ceph-bs", "CephBackupStorage") - 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) + // Attach PS to cluster and create a VM to establish realistic context + attachPrimaryStorageToCluster { + primaryStorageUuid = ps.uuid + clusterUuid = cluster.uuid + } - assert !reply.isSuccess() : "Should fail when no preferred BS type is available" - } + def instanceOffering = env.inventoryByName("instanceOffering") as InstanceOfferingInventory + def image = env.inventoryByName("image") as ImageInventory + def l3 = env.inventoryByName("l3") as L3NetworkInventory - /** - * 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() { - createAndAttachBackupStorage("imagestore-bs", "ImageStoreBackupStorage") - - 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}" - } + VmInstanceInventory vm = createVmInstance { + name = "test-vm" + instanceOfferingUuid = instanceOffering.uuid + imageUuid = image.uuid + l3NetworkUuids = [l3.uuid] + } as VmInstanceInventory - /** - * Reproduces ZSTAC-80789: getPreferBackupStorageTypes() must return a defensive copy. - * - * 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: Return new ArrayList<>(preferBackupStorageTypes) from getPreferBackupStorageTypes(). - * - * Test: send SelectBackupStorageMsg with requiredBackupStorageTypes=["CephBackupStorage"] - * (no intersection with ZBS's prefer types), then send again without requiredBackupStorageTypes. - * Before the fix, the second call fails because the bean's list was emptied by retainAll(). - */ - void testPreferBsTypesNotCorruptedByRetainAll() { - // imagestore-bs was already created in testSelectPreferredOverNonPreferred - // ZBS prefers [ImageStoreBackupStorage]; zone has ImageStoreBackupStorage attached + assert vm != null : "VM should be created successfully on ZBS primary storage" // 1st call: with requiredBackupStorageTypes=["CephBackupStorage"] - // retainAll(["CephBackupStorage"]) on preferBsTypes => empty intersection => error expected - // Before fix: this mutates the bean, emptying preferBackupStorageTypes permanently + // 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)) @@ -206,11 +203,11 @@ class ExternalPrimaryStorageSelectBackupStorageCase extends SubCase { 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" + 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 + // 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)) From a8154d212f894e49e0c0fefc5d25d2aca3b1b585 Mon Sep 17 00:00:00 2001 From: J M Date: Mon, 23 Mar 2026 18:19:51 +0800 Subject: [PATCH 4/5] [storage]: add retainAll bean mutation test to ZbsPrimaryStorageCase Resolves: ZSTAC-80789 Change-Id: I17a4e65b70f86453b459b82fbfe2b435858effeb --- .../addon/zbs/ZbsPrimaryStorageCase.groovy | 56 +++++++++++++++++++ 1 file changed, 56 insertions(+) diff --git a/test/src/test/groovy/org/zstack/test/integration/storage/primary/addon/zbs/ZbsPrimaryStorageCase.groovy b/test/src/test/groovy/org/zstack/test/integration/storage/primary/addon/zbs/ZbsPrimaryStorageCase.groovy index f07add523d3..e4fed751442 100644 --- a/test/src/test/groovy/org/zstack/test/integration/storage/primary/addon/zbs/ZbsPrimaryStorageCase.groovy +++ b/test/src/test/groovy/org/zstack/test/integration/storage/primary/addon/zbs/ZbsPrimaryStorageCase.groovy @@ -1,6 +1,7 @@ package org.zstack.test.integration.storage.primary.addon.zbs import org.springframework.http.HttpEntity +import org.zstack.core.cloudbus.CloudBus import org.zstack.core.cloudbus.EventCallback import org.zstack.core.cloudbus.EventFacade import org.zstack.core.db.DatabaseFacade @@ -11,9 +12,13 @@ import org.zstack.header.storage.addon.primary.ExternalPrimaryStorageVO_ import org.zstack.header.storage.addon.primary.ExternalPrimaryStorageSpaceVO_ import org.zstack.header.storage.primary.PrimaryStorageCapacityVO import org.zstack.header.storage.primary.PrimaryStorageCapacityVO_ +import org.zstack.header.storage.primary.PrimaryStorageConstant import org.zstack.header.storage.primary.PrimaryStorageHostRefVO import org.zstack.header.storage.primary.PrimaryStorageHostRefVO_ import org.zstack.header.storage.primary.PrimaryStorageStatus +import org.zstack.header.storage.primary.SelectBackupStorageMsg +import org.zstack.header.storage.primary.SelectBackupStorageReply +import org.zstack.header.message.MessageReply import org.zstack.storage.zbs.MdsStatus import org.zstack.storage.zbs.MdsUri import org.zstack.sdk.* @@ -182,6 +187,7 @@ class ZbsPrimaryStorageCase extends SubCase { testDataVolumeNegativeScenario() testDecodeMdsUriWithSpecialPassword() testMdsReconnectAfterMaximumPingFailures() + testPreferBsTypesNotCorruptedByRetainAll() } } @@ -856,4 +862,54 @@ class ZbsPrimaryStorageCase extends SubCase { return cap.totalCapacity - cap.availableCapacity } + + /** + * ZSTAC-80789: getPreferBackupStorageTypes() must return a defensive copy. + * + * Bug: ZbsStorageFactory returned a direct reference to its internal + * preferBackupStorageTypes list. When SelectBackupStorageMsg carried + * requiredBackupStorageTypes, the handler called retainAll() on that list, + * permanently mutating the bean. Subsequent requests would see an empty + * preferBsTypes and fail with a different error code. + */ + void testPreferBsTypesNotCorruptedByRetainAll() { + def bus = bean(CloudBus.class) + + // 1st call: requiredBackupStorageTypes=["CephBackupStorage"] + // No intersection with ZBS prefer types [ImageStoreBackupStorage] => error expected + // Before fix: retainAll empties the bean's internal list permanently + 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" + String error1 = (reply1 as SelectBackupStorageReply).error.details + + // 2nd call: same as 1st (with requiredBackupStorageTypes=["CephBackupStorage"]) + // Before fix: bean was corrupted, preferBsTypes is empty => error code is + // "no backup storage type specified" (ADDON_PRIMARY_10010), NOT the normal + // "no intersection" error — because the prefer list itself is empty + // After fix: defensive copy means bean is intact => same error as 1st call + SelectBackupStorageMsg msg2 = new SelectBackupStorageMsg() + msg2.setPrimaryStorageUuid(ps.uuid) + msg2.setRequiredSize(SizeUnit.MEGABYTE.toByte(1)) + msg2.setRequiredBackupStorageTypes(["CephBackupStorage"]) + bus.makeTargetServiceIdByResourceUuid(msg2, PrimaryStorageConstant.SERVICE_ID, ps.uuid) + MessageReply reply2 = bus.call(msg2) + + assert !reply2.isSuccess(): "Should fail: still no intersection" + String error2 = (reply2 as SelectBackupStorageReply).error.details + + // Key assertion: both calls should produce the same error + // Before fix: 1st call gets "Didn't find any available backup storage" + // 2nd call gets "no backup storage type specified" (bean corrupted) + // After fix: both calls get the same error message + assert error1 == error2: + "ZSTAC-80789: error messages differ between 1st and 2nd call, " + + "indicating preferBackupStorageTypes was corrupted by retainAll(). " + + "1st: [${error1}], 2nd: [${error2}]" + } } From f04099e72be628dbad34d3676e227263c930248d Mon Sep 17 00:00:00 2001 From: J M Date: Mon, 23 Mar 2026 18:55:35 +0800 Subject: [PATCH 5/5] [storage]: rename and improve clone VM bean corruption test Resolves: ZSTAC-80789 Change-Id: I08981a100948d2c56fd403b952f44674df44cf86 --- .../addon/zbs/ZbsPrimaryStorageCase.groovy | 60 +++++++++++-------- 1 file changed, 35 insertions(+), 25 deletions(-) diff --git a/test/src/test/groovy/org/zstack/test/integration/storage/primary/addon/zbs/ZbsPrimaryStorageCase.groovy b/test/src/test/groovy/org/zstack/test/integration/storage/primary/addon/zbs/ZbsPrimaryStorageCase.groovy index e4fed751442..b1ec2a1e0b1 100644 --- a/test/src/test/groovy/org/zstack/test/integration/storage/primary/addon/zbs/ZbsPrimaryStorageCase.groovy +++ b/test/src/test/groovy/org/zstack/test/integration/storage/primary/addon/zbs/ZbsPrimaryStorageCase.groovy @@ -17,7 +17,6 @@ import org.zstack.header.storage.primary.PrimaryStorageHostRefVO import org.zstack.header.storage.primary.PrimaryStorageHostRefVO_ import org.zstack.header.storage.primary.PrimaryStorageStatus import org.zstack.header.storage.primary.SelectBackupStorageMsg -import org.zstack.header.storage.primary.SelectBackupStorageReply import org.zstack.header.message.MessageReply import org.zstack.storage.zbs.MdsStatus import org.zstack.storage.zbs.MdsUri @@ -187,7 +186,7 @@ class ZbsPrimaryStorageCase extends SubCase { testDataVolumeNegativeScenario() testDecodeMdsUriWithSpecialPassword() testMdsReconnectAfterMaximumPingFailures() - testPreferBsTypesNotCorruptedByRetainAll() + testCloneVmNotCorruptPreferBsTypes() } } @@ -864,20 +863,32 @@ class ZbsPrimaryStorageCase extends SubCase { } /** - * ZSTAC-80789: getPreferBackupStorageTypes() must return a defensive copy. + * ZSTAC-80789: Clone VM with a target PS whose required BS types differ from + * the current PS's preferred BS types corrupts the Factory bean. * - * Bug: ZbsStorageFactory returned a direct reference to its internal - * preferBackupStorageTypes list. When SelectBackupStorageMsg carried - * requiredBackupStorageTypes, the handler called retainAll() on that list, - * permanently mutating the bean. Subsequent requests would see an empty - * preferBsTypes and fail with a different error code. + * Reproduction path (MevocoVmInstanceBase.java "allocate-bs" flow): + * 1. Clone VM (full=true) with primaryStorageUuidForRootVolume pointing to + * a PS that requires "CephBackupStorage" + * 2. MevocoVmInstanceBase calls getRequiredBackupStorageTypes(requiredPsUuid) + * and sets requiredBackupStorageTypes=["CephBackupStorage"] on SelectBackupStorageMsg + * 3. ExternalPrimaryStorage.handle(SelectBackupStorageMsg) calls + * retainAll(requiredTypes) directly on the ZbsStorageFactory bean's + * preferBackupStorageTypes list (which is ["ImageStoreBackupStorage"]) + * 4. retainAll empties the bean's list permanently (no intersection) + * 5. Next clone (without specifying target PS) fails because the factory + * bean now reports empty preferBsTypes + * + * This test simulates the exact SelectBackupStorageMsg that MevocoVmInstanceBase + * sends during the "allocate-bs" flow, verifying the bean is not corrupted. */ - void testPreferBsTypesNotCorruptedByRetainAll() { + void testCloneVmNotCorruptPreferBsTypes() { def bus = bean(CloudBus.class) - // 1st call: requiredBackupStorageTypes=["CephBackupStorage"] - // No intersection with ZBS prefer types [ImageStoreBackupStorage] => error expected - // Before fix: retainAll empties the bean's internal list permanently + // Simulate 1st clone: target PS requires "CephBackupStorage" (e.g. Ceph PS) + // This is what MevocoVmInstanceBase sends when primaryStorageUuidForRootVolume + // points to a Ceph PS — getRequiredBackupStorageTypes() returns ["CephBackupStorage"] + // No intersection with ZBS prefer types ["ImageStoreBackupStorage"] => should fail + // Before fix: retainAll() empties the bean's internal list permanently SelectBackupStorageMsg msg1 = new SelectBackupStorageMsg() msg1.setPrimaryStorageUuid(ps.uuid) msg1.setRequiredSize(SizeUnit.MEGABYTE.toByte(1)) @@ -885,14 +896,13 @@ class ZbsPrimaryStorageCase extends SubCase { 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" - String error1 = (reply1 as SelectBackupStorageReply).error.details + assert !reply1.isSuccess(): "1st clone should fail: no intersection between CephBackupStorage and ZBS prefer types" + String error1 = reply1.error.details - // 2nd call: same as 1st (with requiredBackupStorageTypes=["CephBackupStorage"]) - // Before fix: bean was corrupted, preferBsTypes is empty => error code is - // "no backup storage type specified" (ADDON_PRIMARY_10010), NOT the normal - // "no intersection" error — because the prefer list itself is empty - // After fix: defensive copy means bean is intact => same error as 1st call + // Simulate 2nd clone: same requiredBackupStorageTypes to verify bean not corrupted + // Before fix: bean corrupted — preferBsTypes is empty — fails with + // "no backup storage type specified" (a different error than above) + // After fix: defensive copy keeps bean intact — same error as 1st clone SelectBackupStorageMsg msg2 = new SelectBackupStorageMsg() msg2.setPrimaryStorageUuid(ps.uuid) msg2.setRequiredSize(SizeUnit.MEGABYTE.toByte(1)) @@ -900,13 +910,13 @@ class ZbsPrimaryStorageCase extends SubCase { bus.makeTargetServiceIdByResourceUuid(msg2, PrimaryStorageConstant.SERVICE_ID, ps.uuid) MessageReply reply2 = bus.call(msg2) - assert !reply2.isSuccess(): "Should fail: still no intersection" - String error2 = (reply2 as SelectBackupStorageReply).error.details + assert !reply2.isSuccess(): "2nd clone should fail: still no intersection" + String error2 = reply2.error.details - // Key assertion: both calls should produce the same error - // Before fix: 1st call gets "Didn't find any available backup storage" - // 2nd call gets "no backup storage type specified" (bean corrupted) - // After fix: both calls get the same error message + // Key assertion: both calls must produce the same error + // Before fix: 1st gets "Didn't find any available backup storage" + // 2nd gets "no backup storage type specified" (bean corrupted) + // After fix: both get the same error (bean not corrupted) assert error1 == error2: "ZSTAC-80789: error messages differ between 1st and 2nd call, " + "indicating preferBackupStorageTypes was corrupted by retainAll(). " +