<fix>[storage]: exclude deleted volumes from recalculate and sync physical capacity on reconnect

docs/cache/fix-artifacts/ZSTAC-80937/phase-5.json

@@ -0,0 +1,32 @@
+{
+  "phase": 5,
+  "issue": "ZSTAC-80937",
+  "title": "TDD GREEN - Fix PS availableCapacity drift on syncVolumeSize",
+  "status": "GREEN",
+  "data": {
+    "root_cause": "VolumeBase.syncVolumeVolumeSize() updates VolumeVO.size from the agent-reported value but does not send DecreasePrimaryStorageCapacityMsg/IncreasePrimaryStorageCapacityMsg to adjust the primary storage availableCapacity. Over time, every syncVolumeSize that reports a different size causes availableCapacity to drift from reality, inflating the storage pool allocation rate.",
+    "bug_location": "storage/src/main/java/org/zstack/storage/volume/VolumeBase.java:syncVolumeVolumeSize()",
+    "solution": "In syncVolumeVolumeSize(), capture oldSize before updating. After dbf.updateAndRefresh(), compare newSize vs oldSize: if size grew, send DecreasePrimaryStorageCapacityMsg with the diff; if size shrank, send IncreasePrimaryStorageCapacityMsg with the abs diff. This keeps PS availableCapacity in sync with actual volume size changes.",
+    "files_modified": [
+      "storage/src/main/java/org/zstack/storage/volume/VolumeBase.java"
+    ],
+    "tdd_cycle": {
+      "phase": "GREEN",
+      "test_file": "test/src/test/groovy/org/zstack/test/integration/storage/primary/nfs/capacity/NfsSyncVolumeSizeCapacityCase.groovy",
+      "test_class": "NfsSyncVolumeSizeCapacityCase",
+      "test_method": "testSyncVolumeSizeAdjustsCapacity",
+      "expected_result": "PASS - availableCapacity decreases by 10GB after syncVolumeSize reports a larger volume size",
+      "reproduction_steps": [
+        "1. Create a VM with a root volume on NFS primary storage",
+        "2. Record baseline PS availableCapacity",
+        "3. Mock NFS GET_VOLUME_SIZE_PATH to return volume size + 10GB",
+        "4. Call APISyncVolumeSizeMsg on the root volume",
+        "5. Assert VolumeVO.size is updated (passes)",
+        "6. Assert PS availableCapacity decreased by 10GB (passes with fix)"
+      ]
+    },
+    "fix_reverted": false,
+    "fix_file": "storage/src/main/java/org/zstack/storage/volume/VolumeBase.java",
+    "fix_description": "After updating VolumeVO.size in syncVolumeVolumeSize(), compare newSize vs oldSize and send DecreasePrimaryStorageCapacityMsg (size grew) or IncreasePrimaryStorageCapacityMsg (size shrank) to keep PS availableCapacity in sync."
+  }
+}

storage/pom.xml

@@ -54,6 +54,11 @@
             <artifactId>longjob</artifactId>
             <version>${project.version}</version>
         </dependency>
+        <dependency>
+            <groupId>junit</groupId>
+            <artifactId>junit</artifactId>
+            <scope>test</scope>
+        </dependency>
     </dependencies>
 
     <build>

storage/src/main/java/org/zstack/storage/addon/primary/ExternalPrimaryStorage.java

@@ -55,6 +55,7 @@
 import org.zstack.utils.gson.JSONObjectUtil;
 import org.zstack.utils.logging.CLogger;
 
+import java.util.ArrayList;
 import java.util.Comparator;
 import java.util.LinkedHashMap;
 import java.util.List;
@@ -409,7 +410,7 @@ private void handle(final SelectBackupStorageMsg msg) {
 
 
         BackupStorageSelector selector = pluginRgty.getExtensionFromMap(externalVO.getIdentity(), BackupStorageSelector.class);
-        List<String> preferBsTypes = selector.getPreferBackupStorageTypes();
+        List<String> preferBsTypes = new ArrayList<>(selector.getPreferBackupStorageTypes());
         if (!CollectionUtils.isEmpty(msg.getRequiredBackupStorageTypes())) {
             preferBsTypes.retainAll(msg.getRequiredBackupStorageTypes());
         }

storage/src/main/java/org/zstack/storage/primary/PrimaryStorageBase.java

@@ -655,10 +655,10 @@ public void success() {
                         changeStatus(PrimaryStorageStatus.Connected);
                         logger.debug(String.format("successfully connected primary storage[uuid:%s]", self.getUuid()));
 
-                        RecalculatePrimaryStorageCapacityMsg rmsg = new RecalculatePrimaryStorageCapacityMsg();
-                        rmsg.setPrimaryStorageUuid(self.getUuid());
-                        bus.makeLocalServiceId(rmsg, PrimaryStorageConstant.SERVICE_ID);
-                        bus.send(rmsg);
+                        SyncPrimaryStorageCapacityMsg smsg = new SyncPrimaryStorageCapacityMsg();
+                        smsg.setPrimaryStorageUuid(self.getUuid());
+                        bus.makeLocalServiceId(smsg, PrimaryStorageConstant.SERVICE_ID);
+                        bus.send(smsg);
 
                         tracker.track(self.getUuid());
 

storage/src/main/java/org/zstack/storage/primary/PrimaryStorageCapacityRecalculator.java

@@ -75,7 +75,7 @@ public void run() {
                             " group by vol.primaryStorageUuid";
                     TypedQuery<Tuple> q = dbf.getEntityManager().createQuery(sql, Tuple.class);
                     q.setParameter("psUuids", psUuids);
-                    List<VolumeStatus> needCountVolumeStates = asList(VolumeStatus.Creating, VolumeStatus.Ready, VolumeStatus.Deleted);
+                    List<VolumeStatus> needCountVolumeStates = asList(VolumeStatus.Creating, VolumeStatus.Ready);
                     q.setParameter("volStatus", needCountVolumeStates);
                     List<Tuple> ts = q.getResultList();
 

storage/src/main/java/org/zstack/storage/volume/VolumeBase.java

@@ -2184,7 +2184,11 @@ public void run(MessageReply reply) {
 
                 refreshVO();
                 SyncVolumeSizeOnPrimaryStorageReply r = reply.castReply();
-                self.setSize(r.getSize());
+
+                long oldSize = self.getSize();
+                long newSize = r.getSize();
+
+                self.setSize(newSize);
 
                 if (!r.isWithInternalSnapshot()) {
                     // the actual size = volume actual size + all snapshot size
@@ -2196,6 +2200,23 @@ public void run(MessageReply reply) {
 
                 self = dbf.updateAndRefresh(self);
 
+                if (newSize != oldSize && self.getPrimaryStorageUuid() != null) {
+                    long diff = newSize - oldSize;
+                    if (diff > 0) {
+                        DecreasePrimaryStorageCapacityMsg dmsg = new DecreasePrimaryStorageCapacityMsg();
+                        dmsg.setPrimaryStorageUuid(self.getPrimaryStorageUuid());
+                        dmsg.setDiskSize(diff);
+                        bus.makeTargetServiceIdByResourceUuid(dmsg, PrimaryStorageConstant.SERVICE_ID, self.getPrimaryStorageUuid());
+                        bus.send(dmsg);
+                    } else {
+                        IncreasePrimaryStorageCapacityMsg imsg = new IncreasePrimaryStorageCapacityMsg();
+                        imsg.setPrimaryStorageUuid(self.getPrimaryStorageUuid());
+                        imsg.setDiskSize(-diff);
+                        bus.makeTargetServiceIdByResourceUuid(imsg, PrimaryStorageConstant.SERVICE_ID, self.getPrimaryStorageUuid());
+                        bus.send(imsg);
+                    }
+                }
+
                 VolumeSize size = new VolumeSize();
                 size.actualSize = self.getActualSize();
                 size.size = self.getSize();

storage/src/test/java/org/zstack/storage/addon/primary/ExternalPrimaryStorageSelectBsTest.java

@@ -0,0 +1,99 @@
+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 implementations (e.g. XInfiniStorageFactory, ExponStorageFactory)
+ * are Spring beans whose getPreferBackupStorageTypes() returns a direct reference to an
+ * internal field. If the caller mutates that list (e.g. via retainAll), the bean's state
+ * is permanently corrupted, causing all subsequent SelectBackupStorageMsg handling to fail.
+ *
+ * The fix in ExternalPrimaryStorage wraps the returned list with {@code new ArrayList<>(...)}.
+ * This test verifies that pattern.
+ */
+public class ExternalPrimaryStorageSelectBsTest {
+
+    /**
+     * Simulates a BackupStorageSelector that returns its internal field directly,
+     * just like XInfiniStorageFactory and ExponStorageFactory do.
+     */
+    private static class FakeBackupStorageSelector implements BackupStorageSelector {
+        private List<String> preferBackupStorageTypes;
+
+        FakeBackupStorageSelector(List<String> types) {
+            this.preferBackupStorageTypes = types;
+        }
+
+        @Override
+        public List<String> getPreferBackupStorageTypes() {
+            return preferBackupStorageTypes;
+        }
+
+        @Override
+        public String getIdentity() {
+            return "fake";
+        }
+    }
+
+    @Test
+    public void testDefensiveCopyPreventsBeaMutation() {
+        // Setup: simulate a Spring bean with mutable internal state
+        List<String> beanInternalList = new ArrayList<>(Arrays.asList("ImageStore", "Ceph", "FusionStor"));
+        FakeBackupStorageSelector selector = new FakeBackupStorageSelector(beanInternalList);
+
+        List<String> originalTypes = new ArrayList<>(beanInternalList);
+
+        // First call: simulate handle(SelectBackupStorageMsg) with requiredBackupStorageTypes = ["ImageStore"]
+        // The fix: defensive copy before retainAll
+        List<String> preferBsTypes = new ArrayList<>(selector.getPreferBackupStorageTypes());
+        List<String> requiredTypes = Arrays.asList("ImageStore");
+        preferBsTypes.retainAll(requiredTypes);
+
+        // After retainAll, the working copy should be filtered
+        Assert.assertEquals(Arrays.asList("ImageStore"), preferBsTypes);
+
+        // The bean's internal state must remain unchanged
+        Assert.assertEquals(originalTypes, selector.getPreferBackupStorageTypes());
+        Assert.assertEquals(3, selector.getPreferBackupStorageTypes().size());
+
+        // Second call: simulate another handle(SelectBackupStorageMsg) with requiredBackupStorageTypes = ["Ceph"]
+        List<String> preferBsTypes2 = new ArrayList<>(selector.getPreferBackupStorageTypes());
+        List<String> requiredTypes2 = Arrays.asList("Ceph");
+        preferBsTypes2.retainAll(requiredTypes2);
+
+        Assert.assertEquals(Arrays.asList("Ceph"), preferBsTypes2);
+
+        // Bean's internal state still unchanged after two calls
+        Assert.assertEquals(originalTypes, selector.getPreferBackupStorageTypes());
+    }
+
+    @Test
+    public void testWithoutDefensiveCopyBeanIsMutated() {
+        // This test demonstrates the original bug: without defensive copy, retainAll mutates the bean
+        List<String> beanInternalList = new ArrayList<>(Arrays.asList("ImageStore", "Ceph", "FusionStor"));
+        FakeBackupStorageSelector selector = new FakeBackupStorageSelector(beanInternalList);
+
+        // Without defensive copy (the old buggy code path)
+        List<String> preferBsTypes = selector.getPreferBackupStorageTypes();
+        preferBsTypes.retainAll(Arrays.asList("ImageStore"));
+
+        // The bean's internal list is now corrupted
+        Assert.assertEquals(1, selector.getPreferBackupStorageTypes().size());
+        Assert.assertEquals(Arrays.asList("ImageStore"), selector.getPreferBackupStorageTypes());
+
+        // Second call with "Ceph" now fails because the bean only has "ImageStore" left
+        List<String> preferBsTypes2 = selector.getPreferBackupStorageTypes();
+        preferBsTypes2.retainAll(Arrays.asList("Ceph"));
+
+        // The list is now empty - this is the bug that caused clone VM to fail
+        Assert.assertTrue(selector.getPreferBackupStorageTypes().isEmpty());
+    }
+}