Skip to content

fix(primaryStorage): rollback persisted records on controller build failure #3868

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.

Sign up for GitHub

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

Open
zstack-robot-1 wants to merge 1 commit into
base: 5.5.6
Choose a base branch
from
+45 −1
Open

fix(primaryStorage): rollback persisted records on controller build failure #3868

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
16 changes: 15 additions & 1 deletion storage/src/main/java/org/zstack/storage/addon/primary/ExternalPrimaryStorageFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -320,7 +320,21 @@ public PrimaryStorageInventory createPrimaryStorage(PrimaryStorageVO vo, APIAddP
dbf.persist(lvo);
dbf.persist(ref);

saveControllerIfNeed(lvo);
try {
saveControllerIfNeed(lvo);
} catch (Throwable t) {
logger.warn(String.format("failed to build controller for primary storage[uuid:%s, identity:%s], rolling back persisted records",
lvo.getUuid(), identity), t);
try {
dbf.remove(ref);
dbf.remove(lvo);
} catch (Throwable cleanupEx) {
logger.warn(String.format("failed to roll back persisted records for primary storage[uuid:%s]", lvo.getUuid()), cleanupEx);
}
controllers.remove(lvo.getUuid());
nodes.remove(lvo.getUuid());
throw t;
}
Comment on lines +323 to +337
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Apr 27, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

这次补偿只覆盖“当前新增”的脏记录，修复前已经落库的坏数据仍会继续打挂服务。

Line 324 失败时这里只回滚本次 createPrimaryStorage() 刚写入的记录，但 buildPsController()（Line 180-187）和 nodeLeft() 触发的重建路径（Line 1077）仍会直接消费库里已有的 ExternalPrimaryStorageVO。如果现场在修复前已经因为同类 malformed JSON 留下过脏记录，升级后启动/切主时还是会在这些路径上反复抛异常，QueryPrimaryStorage 依旧可能不可用。建议补一个启动期的隔离/跳过坏记录，或者增加显式的数据修复逻辑。

return lvo.toInventory();
}

Expand Down
30 changes: 30 additions & 0 deletions ...groovy/org/zstack/test/integration/storage/primary/addon/zbs/ZbsPrimaryStorageCase.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,6 +179,7 @@ class ZbsPrimaryStorageCase extends SubCase {
testMdsPing()
testCheckHostStorageConnection()
testNegativeScenario()
testAddExternalPrimaryStorageWithMalformedJsonShouldRollback()
testDataVolumeNegativeScenario()
testDecodeMdsUriWithSpecialPassword()
testMdsReconnectAfterMaximumPingFailures()
Expand Down Expand Up @@ -725,6 +726,35 @@ class ZbsPrimaryStorageCase extends SubCase {
}
}

// AddExternalPrimaryStorage 收到非法 JSON config 导致 buildControllerSvc 抛异常时，
// ExternalPrimaryStorageVO/PrimaryStorageVO/PrimaryStorageOutputProtocolRefVO 不应残留在 DB 中。
// 否则 buildPsController 会被脏 VO 持续打挂，QueryPrimaryStorage 永久 503。
void testAddExternalPrimaryStorageWithMalformedJsonShouldRollback() {
long psCountBefore = Q.New(ExternalPrimaryStorageVO.class).count()

// malformed JSON — JSONObjectUtil.toObject(config, Config.class) 会抛 RuntimeException
expect(AssertionError.class) {
addExternalPrimaryStorage {
zoneUuid = zone.uuid
name = "zbs-bad-json"
identity = "zbs"
defaultOutputProtocol = "CBD"
config = "{this is not valid json"
url = ""
}
}

// 失败后不应在 DB 中遗留任何 zbs-bad-json 相关记录
assert Q.New(ExternalPrimaryStorageVO.class).count() == psCountBefore
assert !Q.New(ExternalPrimaryStorageVO.class)
.eq(ExternalPrimaryStorageVO_.name, "zbs-bad-json")
.isExists()

// 失败之后 QueryPrimaryStorage 仍可正常返回（不被脏数据打挂）
Comment on lines +729 to +753
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Apr 27, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

请把新增注释改成英文。

Line 729-753 新增的是中文注释，不符合仓库规范。

可直接替换的英文注释 
-    // AddExternalPrimaryStorage 收到非法 JSON config 导致 buildControllerSvc 抛异常时，
-    // ExternalPrimaryStorageVO/PrimaryStorageVO/PrimaryStorageOutputProtocolRefVO 不应残留在 DB 中。
-    // 否则 buildPsController 会被脏 VO 持续打挂，QueryPrimaryStorage 永久 503。
+    // When AddExternalPrimaryStorage receives malformed JSON config and buildControllerSvc throws,
+    // ExternalPrimaryStorageVO / PrimaryStorageVO / PrimaryStorageOutputProtocolRefVO must not remain in the database.
+    // Otherwise buildPsController keeps failing on the dirty VO and QueryPrimaryStorage can return 503 permanently.
@@
-        // malformed JSON — JSONObjectUtil.toObject(config, Config.class) 会抛 RuntimeException
+        // Malformed JSON: JSONObjectUtil.toObject(config, Config.class) should throw RuntimeException.
@@
-        // 失败后不应在 DB 中遗留任何 zbs-bad-json 相关记录
+        // No zbs-bad-json related records should remain in the database after the failure.
@@
-        // 失败之后 QueryPrimaryStorage 仍可正常返回（不被脏数据打挂）
+        // QueryPrimaryStorage should still return successfully after the failure.
As per coding guidelines `代码里不应当有有中文，包括报错、注释等都应当使用正确的、无拼写错误的英文来写`
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// AddExternalPrimaryStorage 收到非法 JSON config 导致 buildControllerSvc 抛异常时，
// ExternalPrimaryStorageVO/PrimaryStorageVO/PrimaryStorageOutputProtocolRefVO 不应残留在 DB 中。
// 否则 buildPsController 会被脏 VO 持续打挂，QueryPrimaryStorage 永久 503。
void testAddExternalPrimaryStorageWithMalformedJsonShouldRollback() {
long psCountBefore = Q.New(ExternalPrimaryStorageVO.class).count()
// malformed JSON — JSONObjectUtil.toObject(config, Config.class) 会抛 RuntimeException
expect(AssertionError.class) {
addExternalPrimaryStorage {
zoneUuid = zone.uuid
name = "zbs-bad-json"
identity = "zbs"
defaultOutputProtocol = "CBD"
config = "{this is not valid json"
url = ""
}
}
// 失败后不应在 DB 中遗留任何 zbs-bad-json 相关记录
assert Q.New(ExternalPrimaryStorageVO.class).count() == psCountBefore
assert !Q.New(ExternalPrimaryStorageVO.class)
.eq(ExternalPrimaryStorageVO_.name, "zbs-bad-json")
.isExists()
// 失败之后 QueryPrimaryStorage 仍可正常返回（不被脏数据打挂）
// When AddExternalPrimaryStorage receives malformed JSON config and buildControllerSvc throws,
// ExternalPrimaryStorageVO / PrimaryStorageVO / PrimaryStorageOutputProtocolRefVO must not remain in the database.
// Otherwise buildPsController keeps failing on the dirty VO and QueryPrimaryStorage can return 503 permanently.
void testAddExternalPrimaryStorageWithMalformedJsonShouldRollback() {
long psCountBefore = Q.New(ExternalPrimaryStorageVO.class).count()
// Malformed JSON: JSONObjectUtil.toObject(config, Config.class) should throw RuntimeException.
expect(AssertionError.class) {
addExternalPrimaryStorage {
zoneUuid = zone.uuid
name = "zbs-bad-json"
identity = "zbs"
defaultOutputProtocol = "CBD"
config = "{this is not valid json"
url = ""
}
}
// No zbs-bad-json related records should remain in the database after the failure.
assert Q.New(ExternalPrimaryStorageVO.class).count() == psCountBefore
assert !Q.New(ExternalPrimaryStorageVO.class)
.eq(ExternalPrimaryStorageVO_.name, "zbs-bad-json")
.isExists()
// QueryPrimaryStorage should still return successfully after the failure.
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In
`@test/src/test/groovy/org/zstack/test/integration/storage/primary/addon/zbs/ZbsPrimaryStorageCase.groovy`
around lines 729 - 753, Replace the Chinese comment block above the test method
testAddExternalPrimaryStorageWithMalformedJsonShouldRollback with an English
comment that explains: this test verifies AddExternalPrimaryStorage rolls back
when config contains malformed JSON (JSONObjectUtil.toObject throws), and no
ExternalPrimaryStorageVO/PrimaryStorageVO/PrimaryStorageOutputProtocolRefVO
records remain to avoid dirty VO causing buildPsController failures and
QueryPrimaryStorage 503s; update any inline Chinese in the same comment region
to concise, correct English while keeping references to ExternalPrimaryStorageVO
and the failure expectations intact.

def psList = queryPrimaryStorage {} as List
assert psList != null
Comment on lines +733 to +755
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Apr 27, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

这个回归用例还没有覆盖基表和协议映射表的回滚。

现在只校验了 ExternalPrimaryStorageVO。如果后面有人只删掉子表、遗漏 PrimaryStorageVO 或 PrimaryStorageOutputProtocolRefVO，这个用例仍然会通过，但数据库里还是会残留脏数据。既然本次修复显式补偿了这些记录，建议把相关表的 count 前后也一起断言。

可参考的补充断言 
 void testAddExternalPrimaryStorageWithMalformedJsonShouldRollback() {
     long psCountBefore = Q.New(ExternalPrimaryStorageVO.class).count()
+    long primaryStorageCountBefore = Q.New(org.zstack.header.storage.primary.PrimaryStorageVO.class).count()
+    long protocolRefCountBefore = Q.New(org.zstack.header.storage.primary.PrimaryStorageOutputProtocolRefVO.class).count()

     expect(AssertionError.class) {
         addExternalPrimaryStorage {
             zoneUuid = zone.uuid
             name = "zbs-bad-json"
@@
     }

     assert Q.New(ExternalPrimaryStorageVO.class).count() == psCountBefore
+    assert Q.New(org.zstack.header.storage.primary.PrimaryStorageVO.class).count() == primaryStorageCountBefore
+    assert Q.New(org.zstack.header.storage.primary.PrimaryStorageOutputProtocolRefVO.class).count() == protocolRefCountBefore
     assert !Q.New(ExternalPrimaryStorageVO.class)
             .eq(ExternalPrimaryStorageVO_.name, "zbs-bad-json")
             .isExists()
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In
`@test/src/test/groovy/org/zstack/test/integration/storage/primary/addon/zbs/ZbsPrimaryStorageCase.groovy`
around lines 733 - 755, The test only checks ExternalPrimaryStorageVO but misses
asserting rollback for PrimaryStorageVO and PrimaryStorageOutputProtocolRefVO;
capture counts for PrimaryStorageVO and PrimaryStorageOutputProtocolRefVO (using
Q.New(PrimaryStorageVO.class).count() and
Q.New(PrimaryStorageOutputProtocolRefVO.class).count()) before the failing
addExternalPrimaryStorage call, then after the expect(AssertionError.class)
block assert those counts are unchanged and also assert no rows exist with name
"zbs-bad-json" in PrimaryStorageVO and no protocol refs tied to that primary
storage in PrimaryStorageOutputProtocolRefVO, so failures don't leave residual
rows across those related tables.

}

void testDataVolumeNegativeScenario() {
env.simulator(ZbsStorageController.CREATE_VOLUME_PATH) { HttpEntity<String> e, EnvSpec spec ->
def rsp = new ZbsStorageController.CreateVolumeRsp()
Expand Down