From 9d25b739c6ca071e74244d54622b5ca66b9f7be8 Mon Sep 17 00:00:00 2001 From: J M Date: Thu, 19 Mar 2026 19:14:43 +0800 Subject: [PATCH 1/3] [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/3] [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/3] [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))