<fix>[volume]: support thirdPartySoftwarePackage#3318
<fix>[volume]: support thirdPartySoftwarePackage#3318zstack-robot-2 wants to merge 1 commit intofeature-zsv-5.0.0-zmigratefrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Walkthrough本 PR 在备份存储域新增多种消息/回复类型并扩展 Ceph 插件以支持文件上传/下载/删除/进度查询、取消与升级包部署，同时在 BackupStorageBase 加入本地消息分发分支；新增多项 SDK 接口、zMigrate 接口、测试辅助方法与 Ceph 模拟器，并扩展许可枚举。 Changes
Sequence Diagram(s)sequenceDiagram
participant SDK as "SDK Action"
participant API as "API Server"
participant Bus as "Message Bus / BackupStorageBase"
participant Ceph as "CephBackupStorageBase"
participant Agent as "Backup Storage Agent"
SDK->>API: 发起上传/下载/删除/升级 等 REST 请求
API->>Bus: 构造 NeedReplyMessage / CancelMessage 并发送
Bus->>Ceph: BackupStorageBase 转发到 CephBackupStorageBase 处理
Ceph->>Agent: 构造 AgentCommand (Download/Upload/Delete/Progress/Upgrade/Cancel)
Agent-->>Ceph: 返回 AgentResponse / 进度/状态
Ceph-->>Bus: 映射为 MessageReply 并回传
Bus-->>API: API 返回结果给调用方
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In
`@plugin/ceph/src/main/java/org/zstack/storage/ceph/backup/CephBackupStorageBase.java`:
- Around line 2174-2206: The DeleteFilesCmd sent via
CephBackupStorageMonBase.httpCall lacks the AgentCommand base fields; before
calling monBase.httpCall in handle(DeleteFilesOnBackupStorageHostMsg) set the
command's base fields (e.g. cmd.fsid = getSelf().getFsid(); cmd.uuid =
mon.getUuid()) so the agent receives fsid/uuid, and apply the same fix to the
other httpCall usages in this class (the other commands used in the ranges
referenced) — ensure each AgentCommand subclass populates fsid and uuid (or the
equivalent base fields) from getSelf() and the mon before calling
monBase.httpCall(...).
In `@sdk/src/main/java/org/zstack/sdk/zmigrate/api/GetZMigrateImagesAction.java`:
- Around line 47-58: In makeResult, rename the abbreviated variables to full
words to improve readability: change parameter res to apiResult, local ret to
result, and local value to resultValue (or similar descriptive names); update
the references inside the method (including the error check, assignment
ret.error, and the getResult call
org.zstack.sdk.zmigrate.api.GetZMigrateImagesResult) so the logic and
null-coalescing behavior remain identical; apply the same renaming pattern to
the other occurrences noted (around the other makeResult-like blocks at the
indicated regions) so all short names (res/ret/value/info) become descriptive
across GetZMigrateImagesAction.
In `@testlib/src/main/java/org/zstack/testlib/CephBackupStorageSpec.groovy`:
- Around line 233-260: The simulator handlers for
CephBackupStorageBase.FILE_UPLOAD_PATH and
CephBackupStorageBase.FILE_UPLOAD_PROGRESS_PATH are returning generic
AgentResponse which mismatches real agents; change the FILE_UPLOAD_PATH handler
to return a CephBackupStorageBase.UploadFileResponse and set its
directUploadPath (and any other expected fields), and change the
FILE_UPLOAD_PROGRESS_PATH handler to return a
CephBackupStorageBase.GetDownloadFileProgressResponse and populate the
progress-related fields so tests see the same shape as real responses; keep the
other simulator handlers as-is.
🧹 Nitpick comments (4)
sdk/src/main/java/org/zstack/sdk/softwarePackage/header/UploadSoftwarePackageToBackupStorageResult.java (1)
3-3: 冗余的同包导入。
SoftwarePackageInventory与当前类在同一个包org.zstack.sdk.softwarePackage.header中，无需显式导入。♻️ 建议移除冗余导入
package org.zstack.sdk.softwarePackage.header; -import org.zstack.sdk.softwarePackage.header.SoftwarePackageInventory; - public class UploadSoftwarePackageToBackupStorageResult {sdk/src/main/java/org/zstack/sdk/zmigrate/api/GetZMigrateGatewayVmInstancesResult.java (1)
14-20:List类型应添加泛型参数以增强类型安全。当前
gatewayVmInstances使用原始类型java.util.List，缺少泛型参数。虽然此模式在 SDK 中广泛采用（整个 SDK 代码库都遵循此约定），但添加泛型参数（如List<String>或List<?>）可以提高编译时类型检查的严格性。建议与其他 SDK 类统一进行泛型参数升级。sdk/src/main/java/org/zstack/sdk/softwarePackage/header/UploadAndExecuteSoftwareUpgradePackageResult.java (1)
4-4: 冗余的同包导入语句
SoftwarePackageInventory与当前类在同一个包org.zstack.sdk.softwarePackage.header中，Java 不需要显式导入同包类。♻️ 建议移除冗余导入
package org.zstack.sdk.softwarePackage.header; - -import org.zstack.sdk.softwarePackage.header.SoftwarePackageInventory; public class UploadAndExecuteSoftwareUpgradePackageResult {header/src/main/java/org/zstack/header/storage/backup/SoftwareUpgradePackageDeployMsg.java (1)
11-11: 可选建议：考虑使用 Integer 包装类型替代 int。
targetHostSshPort使用int原始类型，默认值为 0。如果需要区分"未设置"和"显式设置为 0"的情况，建议使用Integer类型，以便通过null检测未设置状态。♻️ 可选改进
- private int targetHostSshPort; + private Integer targetHostSshPort;同时需要修改 getter/setter 的返回类型：
- public int getTargetHostSshPort() { + public Integer getTargetHostSshPort() { return targetHostSshPort; } - public void setTargetHostSshPort(int targetHostSshPort) { + public void setTargetHostSshPort(Integer targetHostSshPort) { this.targetHostSshPort = targetHostSshPort; }
| @Override | ||
| protected void handle(final DeleteFilesOnBackupStorageHostMsg msg) { | ||
| DeleteFilesOnBackupStorageHostReply reply = new DeleteFilesOnBackupStorageHostReply(); | ||
| if (CoreGlobalProperty.UNIT_TEST_ON) { | ||
| bus.reply(msg, reply); | ||
| return; | ||
| } | ||
|
|
||
| CephBackupStorageMonVO mon = getSelf().getMons().stream() | ||
| .filter(m -> m.getUuid().equals(msg.getBackupStorageHostUuid())).findAny().orElse(null); | ||
| if (mon == null) { | ||
| reply.setError(operr("failed to find mon with uuid [%s]", msg.getBackupStorageHostUuid())); | ||
| bus.reply(msg, reply); | ||
| return; | ||
| } | ||
|
|
||
| DeleteFilesCmd cmd = new DeleteFilesCmd(); | ||
| cmd.filesPath = msg.getFilesPath(); | ||
|
|
||
| CephBackupStorageMonBase monBase = new CephBackupStorageMonBase(mon); | ||
| monBase.httpCall(DELETE_FILE_PATH, cmd, DeleteFilesResponse.class, new ReturnValueCompletion<DeleteFilesResponse>(msg) { | ||
| @Override | ||
| public void fail(ErrorCode err) { | ||
| reply.setError(err); | ||
| bus.reply(msg, reply); | ||
| } | ||
|
|
||
| @Override | ||
| public void success(DeleteFilesResponse rsp) { | ||
| bus.reply(msg, reply); | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
为指定 mon 的 httpCall 补全 AgentCommand 基础字段。
这三处直接使用 CephBackupStorageMonBase.httpCall(...)，不会像 HttpCaller 一样自动填充 fsid/uuid。若 agent 依赖该字段，会导致请求失败或定位错误。建议在发送前显式赋值。
✅ 建议修复
DeleteFilesCmd cmd = new DeleteFilesCmd();
+cmd.setUuid(self.getUuid());
+cmd.setFsid(getSelf().getFsid());
cmd.filesPath = msg.getFilesPath(); GetDownloadFileProgressCmd cmd = new GetDownloadFileProgressCmd();
+cmd.setUuid(self.getUuid());
+cmd.setFsid(getSelf().getFsid());
cmd.taskUuid = msg.getTaskUuid(); SoftwareUpgradePackageCmd cmd = new SoftwareUpgradePackageCmd();
+cmd.setUuid(self.getUuid());
+cmd.setFsid(getSelf().getFsid());
cmd.upgradePackagePath = msg.getUpgradePackagePath();Also applies to: 2208-2250, 2252-2291
🤖 Prompt for AI Agents
In
`@plugin/ceph/src/main/java/org/zstack/storage/ceph/backup/CephBackupStorageBase.java`
around lines 2174 - 2206, The DeleteFilesCmd sent via
CephBackupStorageMonBase.httpCall lacks the AgentCommand base fields; before
calling monBase.httpCall in handle(DeleteFilesOnBackupStorageHostMsg) set the
command's base fields (e.g. cmd.fsid = getSelf().getFsid(); cmd.uuid =
mon.getUuid()) so the agent receives fsid/uuid, and apply the same fix to the
other httpCall usages in this class (the other commands used in the ranges
referenced) — ensure each AgentCommand subclass populates fsid and uuid (or the
equivalent base fields) from getSelf() and the mon before calling
monBase.httpCall(...).
There was a problem hiding this comment.
Comment from tao.gan:
删除物理机上文件，不需要补全
There was a problem hiding this comment.
@MatheMatrix 感谢您的说明！
理解了，如果删除文件的 agent 端点确实不依赖这些字段，那么 DeleteFilesCmd 可以不补全。
不过注意到同一批代码中，GetDownloadFileProgressCmd (Line 2208-2250) 和 SoftwareUpgradePackageCmd (Line 2252-2291) 也采用了相同的直接调用 monBase.httpCall() 的方式，没有经过 HttpCaller 的 prepareCmd() 环节。
请确认：
- 这两个命令的 agent 端点是否也不需要
fsid/uuid字段？ - 如果需要，建议在这两处补全字段，保持与代码库中其他
AgentCommand的一致性
如果这三个操作的 agent 实现都明确不依赖这些字段，可以考虑在代码注释中说明原因，方便后续维护。
| private Result makeResult(ApiResult res) { | ||
| Result ret = new Result(); | ||
| if (res.error != null) { | ||
| ret.error = res.error; | ||
| return ret; | ||
| } | ||
|
|
||
| org.zstack.sdk.zmigrate.api.GetZMigrateImagesResult value = res.getResult(org.zstack.sdk.zmigrate.api.GetZMigrateImagesResult.class); | ||
| ret.value = value == null ? new org.zstack.sdk.zmigrate.api.GetZMigrateImagesResult() : value; | ||
|
|
||
| return ret; | ||
| } |
There was a problem hiding this comment.
避免使用缩写变量名以符合命名规范。
ret/res/info 可读性较弱，建议改为完整单词。
♻️ 建议命名修正
- private Result makeResult(ApiResult res) {
- Result ret = new Result();
- if (res.error != null) {
- ret.error = res.error;
- return ret;
+ private Result makeResult(ApiResult apiResult) {
+ Result result = new Result();
+ if (apiResult.error != null) {
+ result.error = apiResult.error;
+ return result;
}
- org.zstack.sdk.zmigrate.api.GetZMigrateImagesResult value = res.getResult(org.zstack.sdk.zmigrate.api.GetZMigrateImagesResult.class);
- ret.value = value == null ? new org.zstack.sdk.zmigrate.api.GetZMigrateImagesResult() : value;
-
- return ret;
+ org.zstack.sdk.zmigrate.api.GetZMigrateImagesResult value = apiResult.getResult(org.zstack.sdk.zmigrate.api.GetZMigrateImagesResult.class);
+ result.value = value == null ? new org.zstack.sdk.zmigrate.api.GetZMigrateImagesResult() : value;
+
+ return result;
}
@@
- public void complete(ApiResult res) {
- completion.complete(makeResult(res));
+ public void complete(ApiResult apiResult) {
+ completion.complete(makeResult(apiResult));
}
@@
- RestInfo info = new RestInfo();
- info.httpMethod = "GET";
- info.path = "/zmigrate/images";
- info.needSession = true;
- info.needPoll = false;
- info.parameterName = "";
- return info;
+ RestInfo restInfo = new RestInfo();
+ restInfo.httpMethod = "GET";
+ restInfo.path = "/zmigrate/images";
+ restInfo.needSession = true;
+ restInfo.needPoll = false;
+ restInfo.parameterName = "";
+ return restInfo;
}Also applies to: 66-69, 82-89
🤖 Prompt for AI Agents
In `@sdk/src/main/java/org/zstack/sdk/zmigrate/api/GetZMigrateImagesAction.java`
around lines 47 - 58, In makeResult, rename the abbreviated variables to full
words to improve readability: change parameter res to apiResult, local ret to
result, and local value to resultValue (or similar descriptive names); update
the references inside the method (including the error check, assignment
ret.error, and the getResult call
org.zstack.sdk.zmigrate.api.GetZMigrateImagesResult) so the logic and
null-coalescing behavior remain identical; apply the same renaming pattern to
the other occurrences noted (around the other makeResult-like blocks at the
indicated regions) so all short names (res/ret/value/info) become descriptive
across GetZMigrateImagesAction.
| simulator(CephBackupStorageBase.FILE_DOWNLOAD_PATH) { HttpEntity<String> entity -> | ||
| def rsp = new CephBackupStorageBase.DownloadFileResponse() | ||
| rsp.md5sum = "1234567890" | ||
| rsp.size = 3L * 1024 * 1024 * 1024 | ||
| rsp.unzipInstallPath = "/root/zstone-software-package/unzipInstallPath" | ||
| rsp.filesSize = [:] | ||
| rsp.filesSize.put("/root/zstone-software-package/unzipInstallPath/Gateway_Linux_Server_(Treker).zmigrate.fast.10.1.106.qcow2", 1024L * 1024 * 1024) | ||
| rsp.filesSize.put("/root/zstone-software-package/unzipInstallPath/BootImage_for_Linux_(TrekerLite)_10.1.106.qcow2", 1024L * 1024 * 1024) | ||
| rsp.filesSize.put("/root/zstone-software-package/unzipInstallPath/BootImage_for_Windows_(TrekerLite)_10.1.106.qcow2", 1024L * 1024 * 1024) | ||
| return rsp | ||
| } | ||
|
|
||
| simulator(CephBackupStorageBase.FILE_UPLOAD_PATH) { HttpEntity<String> entity -> | ||
| def rsp = new CephBackupStorageBase.AgentResponse() | ||
| return rsp | ||
| } | ||
| simulator(CephBackupStorageBase.FILE_UPLOAD_PROGRESS_PATH) { HttpEntity<String> entity -> | ||
| def rsp = new CephBackupStorageBase.AgentResponse() | ||
| return rsp | ||
| } | ||
| simulator(CephBackupStorageBase.DELETE_FILE_PATH) { HttpEntity<String> entity -> | ||
| def rsp = new CephBackupStorageBase.AgentResponse() | ||
| return rsp | ||
| } | ||
| simulator(CephBackupStorageBase.SOFTWARE_UPGRADE_PACKAGE_DEPLOY_PATH) { HttpEntity<String> entity -> | ||
| def rsp = new CephBackupStorageBase.AgentResponse() | ||
| return rsp | ||
| } |
There was a problem hiding this comment.
模拟器返回类型与真实响应不一致。
FILE_UPLOAD_PATH 期望 UploadFileResponse（含 directUploadPath），FILE_UPLOAD_PROGRESS_PATH 期望 GetDownloadFileProgressResponse（含进度字段）。当前返回 AgentResponse 会导致测试中字段为空，容易掩盖问题。
✅ 建议修复
simulator(CephBackupStorageBase.FILE_UPLOAD_PATH) { HttpEntity<String> entity ->
- def rsp = new CephBackupStorageBase.AgentResponse()
+ def rsp = new CephBackupStorageBase.UploadFileResponse()
+ rsp.directUploadPath = "/tmp/direct-upload"
return rsp
}
simulator(CephBackupStorageBase.FILE_UPLOAD_PROGRESS_PATH) { HttpEntity<String> entity ->
- def rsp = new CephBackupStorageBase.AgentResponse()
+ def rsp = new CephBackupStorageBase.GetDownloadFileProgressResponse()
+ rsp.completed = false
+ rsp.progress = 0
+ rsp.size = 0L
+ rsp.actualSize = 0L
+ rsp.downloadSize = 0L
return rsp
}🤖 Prompt for AI Agents
In `@testlib/src/main/java/org/zstack/testlib/CephBackupStorageSpec.groovy` around
lines 233 - 260, The simulator handlers for
CephBackupStorageBase.FILE_UPLOAD_PATH and
CephBackupStorageBase.FILE_UPLOAD_PROGRESS_PATH are returning generic
AgentResponse which mismatches real agents; change the FILE_UPLOAD_PATH handler
to return a CephBackupStorageBase.UploadFileResponse and set its
directUploadPath (and any other expected fields), and change the
FILE_UPLOAD_PROGRESS_PATH handler to return a
CephBackupStorageBase.GetDownloadFileProgressResponse and populate the
progress-related fields so tests see the same shape as real responses; keep the
other simulator handlers as-is.
MatheMatrix
force-pushed
the
sync/tao.gan/zmigrate-ZSV-1000@@3
branch
from
19b4a85 to
9906a04
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
`@sdk/src/main/java/org/zstack/sdk/zmigrate/api/GetZMigrateGatewayVmInstancesResult.java`:
- Around line 6-17: The setters in GetZMigrateGatewayVmInstancesResult use
parameter names identical to the fields (managementVmInstanceUuid,
gatewayVmInstances); rename the parameters (e.g., managementVmInstanceUuidParam
or value, gatewayVmInstancesParam) inside setManagementVmInstanceUuid and
setGatewayVmInstances and assign them to this.managementVmInstanceUuid and
this.gatewayVmInstances respectively to avoid name collision and follow naming
conventions.
In `@testlib/src/main/java/org/zstack/testlib/ApiHelper.groovy`:
- Around line 38617-38641: The cleanUpgradeSoftwarePackage method in ApiHelper
(and similar methods) appears to be manually edited in generated code; revert
any manual changes and regenerate this file from the upstream code generator so
the changes come from the generator rather than hand edits; ensure the generator
source that emits methods like cleanUpgradeSoftwarePackage, the ApiPathTracker
usage and Test.apiPaths population is updated/synced and then re-run the
generator to produce a consistent ApiHelper.groovy output.
🧹 Nitpick comments (5)
sdk/src/main/java/org/zstack/sdk/zmigrate/api/GetZMigrateImagesResult.java (1)
6-6: 建议为Map添加泛型参数以增强类型安全。使用原始类型
java.util.Map会产生编译器警告，并且缺乏类型安全性。建议指定泛型参数，例如Map<String, Object>或更具体的类型。♻️ 建议的修改
- public java.util.Map images; + public java.util.Map<String, Object> images;同时更新 getter 和 setter 的签名：
- public void setImages(java.util.Map images) { + public void setImages(java.util.Map<String, Object> images) { this.images = images; } - public java.util.Map getImages() { + public java.util.Map<String, Object> getImages() { return this.images; }sdk/src/main/java/org/zstack/sdk/softwarePackage/header/UploadSoftwarePackageToBackupStorageResult.java (1)
3-3: 冗余的同包导入
SoftwarePackageInventory与当前类在同一个包org.zstack.sdk.softwarePackage.header中，无需显式导入。♻️ 建议移除冗余导入
package org.zstack.sdk.softwarePackage.header; -import org.zstack.sdk.softwarePackage.header.SoftwarePackageInventory; - public class UploadSoftwarePackageToBackupStorageResult {sdk/src/main/java/org/zstack/sdk/softwarePackage/header/CleanUpgradeSoftwarePackageAction.java (1)
34-38: 原始类型List缺少泛型参数
systemTags和userTags使用了原始的java.util.List类型，建议添加泛型参数以提高类型安全性。不过考虑到这与项目中其他 SDK Action 类的风格保持一致，可作为后续统一优化的改进点。♻️ 建议添加泛型参数
`@Param`(required = false) - public java.util.List systemTags; + public java.util.List<String> systemTags; `@Param`(required = false) - public java.util.List userTags; + public java.util.List<String> userTags;sdk/src/main/java/org/zstack/sdk/softwarePackage/header/UploadSoftwarePackageToBackupStorageAction.java (1)
46-53: 原始类型List缺少泛型参数
tagUuids、systemTags和userTags使用了原始的java.util.List类型。与前一个文件相同的建议，可考虑添加泛型参数。♻️ 建议添加泛型参数
`@Param`(required = false, nonempty = false, nullElements = false, emptyString = true, noTrim = false) - public java.util.List tagUuids; + public java.util.List<String> tagUuids; `@Param`(required = false) - public java.util.List systemTags; + public java.util.List<String> systemTags; `@Param`(required = false) - public java.util.List userTags; + public java.util.List<String> userTags;sdk/src/main/java/org/zstack/sdk/zmigrate/api/GetZMigrateGatewayVmInstancesResult.java (1)
14-19: 为List增加泛型以提升类型安全当前使用了原始类型
List，建议补充具体泛型类型。根据字段名称gatewayVmInstances，该列表很可能存储 VM 实例标识符（如 String UUID）或对象。如无法确定具体类型，至少应使用List<?>作为类型安全的最小保障。♻️ 建议修改
- public java.util.List gatewayVmInstances; - public void setGatewayVmInstances(java.util.List gatewayVmInstances) { + public java.util.List<?> gatewayVmInstances; + public void setGatewayVmInstances(java.util.List<?> gatewayVmInstances) { this.gatewayVmInstances = gatewayVmInstances; } - public java.util.List getGatewayVmInstances() { + public java.util.List<?> getGatewayVmInstances() { return this.gatewayVmInstances; }
| public java.lang.String managementVmInstanceUuid; | ||
| public void setManagementVmInstanceUuid(java.lang.String managementVmInstanceUuid) { | ||
| this.managementVmInstanceUuid = managementVmInstanceUuid; | ||
| } | ||
| public java.lang.String getManagementVmInstanceUuid() { | ||
| return this.managementVmInstanceUuid; | ||
| } | ||
|
|
||
| public java.util.List gatewayVmInstances; | ||
| public void setGatewayVmInstances(java.util.List gatewayVmInstances) { | ||
| this.gatewayVmInstances = gatewayVmInstances; | ||
| } |
There was a problem hiding this comment.
参数名与成员变量同名，违反命名规范
setter 参数与字段同名，按规范需要避免，建议改成不同的参数名以减少歧义。
🔧 建议修改
- public void setManagementVmInstanceUuid(java.lang.String managementVmInstanceUuid) {
- this.managementVmInstanceUuid = managementVmInstanceUuid;
+ public void setManagementVmInstanceUuid(java.lang.String managementVmInstanceUuidValue) {
+ this.managementVmInstanceUuid = managementVmInstanceUuidValue;
}
@@
- public void setGatewayVmInstances(java.util.List gatewayVmInstances) {
- this.gatewayVmInstances = gatewayVmInstances;
+ public void setGatewayVmInstances(java.util.List gatewayVmInstancesValue) {
+ this.gatewayVmInstances = gatewayVmInstancesValue;
}As per coding guidelines: 避免在父子类或同一代码块中出现相同名字的成员或局部变量，防止混淆。
🤖 Prompt for AI Agents
In
`@sdk/src/main/java/org/zstack/sdk/zmigrate/api/GetZMigrateGatewayVmInstancesResult.java`
around lines 6 - 17, The setters in GetZMigrateGatewayVmInstancesResult use
parameter names identical to the fields (managementVmInstanceUuid,
gatewayVmInstances); rename the parameters (e.g., managementVmInstanceUuidParam
or value, gatewayVmInstancesParam) inside setManagementVmInstanceUuid and
setGatewayVmInstances and assign them to this.managementVmInstanceUuid and
this.gatewayVmInstances respectively to avoid name collision and follow naming
conventions.
ddb9e36 to
9c184d2
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@sdk/src/main/java/org/zstack/sdk/softwarePackage/header/CleanUpgradeSoftwarePackageAction.java`:
- Around line 94-102: The REST path in
CleanUpgradeSoftwarePackageAction.getRestInfo currently uses the singular
"/software-package/upgrade/{uuid}" and should be changed to the plural form to
match the module convention and UploadAndExecuteSoftwareUpgradePackageAction;
update the RestInfo.path value in getRestInfo of class
CleanUpgradeSoftwarePackageAction to "/software-packages/upgrade/{uuid}" so API
naming is consistent across actions.
🧹 Nitpick comments (3)
sdk/src/main/java/org/zstack/sdk/zmigrate/api/GetZMigrateGatewayVmInstancesAction.java (1)
28-33: 建议为标签列表添加泛型，提升类型安全。使用原始 List 会丢失编译期类型检查，建议改为 List。
♻️ 建议修改
- public java.util.List systemTags; + public java.util.List<String> systemTags; - public java.util.List userTags; + public java.util.List<String> userTags;sdk/src/main/java/org/zstack/sdk/softwarePackage/header/UploadSoftwarePackageToBackupStorageAction.java (1)
46-53: List 字段使用了原始类型。
tagUuids、systemTags和userTags字段使用了原始List类型而非泛型类型。由于这是自动生成的 SDK 代码，此问题可忽略，但如果手动维护，建议使用List<String>以提高类型安全性。sdk/src/main/java/org/zstack/sdk/softwarePackage/header/CleanUpgradeSoftwarePackageAction.java (1)
28-29: uuid 参数缺少 maxLength 约束。
uuid字段标记为required = true，但未指定maxLength。其他类似的 Action 类中，UUID 字段通常会有长度限制（如 32 或 36 字符）。建议确认是否需要添加长度约束以保持一致性。
| protected RestInfo getRestInfo() { | ||
| RestInfo info = new RestInfo(); | ||
| info.httpMethod = "DELETE"; | ||
| info.path = "/software-package/upgrade/{uuid}"; | ||
| info.needSession = true; | ||
| info.needPoll = true; | ||
| info.parameterName = ""; | ||
| return info; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# 搜索服务端 API 定义中的 software-package 相关路径
echo "=== 搜索服务端 API 定义 ==="
rg -n "software-package|upgrade" --type java -g '!sdk/**' -i | grep -E "(RestRequest|path)" | head -20
# 搜索类似的清理/操作端点
echo -e "\n=== 搜索类似的清理操作端点 ==="
rg -n "@RestRequest.*DELETE" --type java -A 2 | head -40
# 查找 header 文件中的 API 定义
echo -e "\n=== 查找 software-package 相关的 header 定义 ==="
find . -path "*/header/*" -name "*Software*" -type f | head -20Repository: MatheMatrix/zstack
Length of output: 3124
🏁 Script executed:
#!/bin/bash
# 查看完整的 CleanUpgradeSoftwarePackageAction 所在目录结构
echo "=== CleanUpgradeSoftwarePackageAction 文件所在目录 ==="
find . -name "CleanUpgradeSoftwarePackageAction.java" -type f
# 查找对应的消息类（server-side）
echo -e "\n=== 查找对应的服务端消息类 ==="
find . -name "*CleanUpgradeSoftwarePackage*" -type f | grep -v sdk | grep -v target
# 查找 UploadSoftwarePackageToBackupStorageAction 对比
echo -e "\n=== 查找上传操作进行对比 ==="
find . -name "*UploadSoftwarePackageToBackupStorage*" -type fRepository: MatheMatrix/zstack
Length of output: 460
🏁 Script executed:
#!/bin/bash
# 检查 UploadSoftwarePackageToBackupStorageAction 的实际路径
echo "=== UploadSoftwarePackageToBackupStorageAction 路径 ==="
grep -n "path.*software" ./sdk/src/main/java/org/zstack/sdk/softwarePackage/header/UploadSoftwarePackageToBackupStorageAction.java
# 检查 CleanUpgradeSoftwarePackageAction 的路径
echo -e "\n=== CleanUpgradeSoftwarePackageAction 路径 ==="
grep -n "path.*software" ./sdk/src/main/java/org/zstack/sdk/softwarePackage/header/CleanUpgradeSoftwarePackageAction.java
# 搜索所有 software 相关的 API 消息定义（server-side）
echo -e "\n=== 搜索所有 software 相关的服务端 API 消息 ==="
find . -path "*/header/*" -type f -name "*.java" ! -path "*/sdk/*" | xargs grep -l -i "software.*msg\|software.*message" 2>/dev/null | head -10
# 检查是否有 CleanUpgrade 相关的服务端定义
echo -e "\n=== 查找 CleanUpgrade 服务端定义 ==="
find . -path "*/header/*" -type f -name "*.java" ! -path "*/sdk/*" | xargs grep -l "CleanUpgrade\|CleanUpgradeSoftware" 2>/dev/nullRepository: MatheMatrix/zstack
Length of output: 546
🏁 Script executed:
#!/bin/bash
# 对比所有软件包相关操作的 REST 路径
echo "=== 所有 softwarePackage 相关操作的 REST 路径 ==="
grep -r "path = " ./sdk/src/main/java/org/zstack/sdk/softwarePackage/header/ --include="*.java" | grep -E "Action.java" | sed 's/.*path = "//;s/".*//'
# 查找其他 upgrade 相关的路径模式
echo -e "\n=== 其他 upgrade 相关的路径 ==="
rg "path.*upgrade" --type java -A 1 -B 1 | head -30Repository: MatheMatrix/zstack
Length of output: 1356
🏁 Script executed:
#!/bin/bash
# 查找服务端的 API 消息类定义，看是否存在对应的 CleanUpgrade 定义
echo "=== 在 header 文件夹中查找任何 upgrade 相关的 API 消息 ==="
find ./header -type f -name "*Upgrade*.java" | grep -E "Msg|Message" | head -20
# 查看 SoftwareUpgradePackageDeployMsg 的内容
echo -e "\n=== 查看 SoftwareUpgradePackageDeployMsg 内容 ==="
head -20 ./header/src/main/java/org/zstack/header/storage/backup/SoftwareUpgradePackageDeployMsg.java
# 查找任何包含 clean 或 remove upgrade 的定义
echo -e "\n=== 查找清理升级包的服务端定义 ==="
rg -n "CleanUpgrade|RemoveUpgrade|DeleteUpgrade" --type java | grep -v sdk | head -20Repository: MatheMatrix/zstack
Length of output: 969
🏁 Script executed:
#!/bin/bash
# 检查 UploadAndExecuteSoftwareUpgradePackageAction 是否是类似的操作
echo "=== UploadAndExecuteSoftwareUpgradePackageAction 完整路径 ==="
sed -n '94,104p' ./sdk/src/main/java/org/zstack/sdk/softwarePackage/header/UploadAndExecuteSoftwareUpgradePackageAction.java
# 查看所有 software-package 开头的路径
echo -e "\n=== 所有以 /software-package 开头的路径（singular）==="
grep -r "path = \"/software-package" ./sdk/src/main/java/org/zstack/sdk/softwarePackage/ | sed 's/.*path = "//;s/".*//' | sort -u
# 查看所有 /software-packages 开头的路径（plural）
echo -e "\n=== 所有以 /software-packages 开头的路径（plural）==="
grep -r "path = \"/software-packages" ./sdk/src/main/java/org/zstack/sdk/softwarePackage/ | sed 's/.*path = "//;s/".*//' | sort -uRepository: MatheMatrix/zstack
Length of output: 962
REST 路径命名不一致。
当前路径 /software-package/upgrade/{uuid} 使用了单数形式，但同一模块中 UploadAndExecuteSoftwareUpgradePackageAction 使用的是 /software-packages/backup-storage/{uuid}/upgrade（复数形式）。
根据编码规范："path: 针对资源使用复数形式"，建议统一使用复数形式 /software-packages/upgrade/{uuid} 以保持 API 一致性。
建议修改
protected RestInfo getRestInfo() {
RestInfo info = new RestInfo();
info.httpMethod = "DELETE";
- info.path = "/software-package/upgrade/{uuid}";
+ info.path = "/software-packages/upgrade/{uuid}";
info.needSession = true;
info.needPoll = true;
info.parameterName = "";
return info;
}🤖 Prompt for AI Agents
In
`@sdk/src/main/java/org/zstack/sdk/softwarePackage/header/CleanUpgradeSoftwarePackageAction.java`
around lines 94 - 102, The REST path in
CleanUpgradeSoftwarePackageAction.getRestInfo currently uses the singular
"/software-package/upgrade/{uuid}" and should be changed to the plural form to
match the module convention and UploadAndExecuteSoftwareUpgradePackageAction;
update the RestInfo.path value in getRestInfo of class
CleanUpgradeSoftwarePackageAction to "/software-packages/upgrade/{uuid}" so API
naming is consistent across actions.
9cb785d to
ed7009d
Compare
|
Comment from ye.zou: Code Review Summary (AI-Assisted)Verdict: CONDITIONAL - 修复 Critical 问题后可合并 Critical (3)
Major (3)
Minor
Review by AI Assistant (Claude) - 请作者确认实现文件中的路径验证和消息处理逻辑 |
|
Comment from tao.gan: 1 SSH密码明文传输 2 缺少输入验证 url 有 scheme 限制 installPath 在 api 中已做校验 3 批量删除无保护 底层删除文件使用 linux.rm_dir_force ，有白名单 4 消息路由注册 1）正常的使用方式 5 CancelDownloadFileOnBackupStorageHostMsg 取消的 job 是 api id，不需要 taskUuid 6 Commit message type 和 volume 是自动生成的 |
MatheMatrix
force-pushed
the
sync/tao.gan/zmigrate-ZSV-1000@@3
branch
4 times, most recently
from
9dd9ec8 to
104ce68
Compare
|
Comment from ye.zou: Code Review — zstack !9141Reviewer: AI Code Review Bot | Verdict: REQUEST CHANGES [CRITICAL] Command injection via SSH parameters to agentFile: SSH credentials and script paths passed to the Python agent without validation. The agent then interpolates them into Fix: Add input validation before building the command:
[CRITICAL] Path traversal in file download/uploadFile:
Fix: Validate installPath — must be absolute, must not contain [HIGH] Null safety on
|
|
Comment from tao.gan: 2 Path traversal in file download/upload |
MatheMatrix
force-pushed
the
sync/tao.gan/zmigrate-ZSV-1000@@3
branch
3 times, most recently
from
f64341a to
4fac778
Compare
|
Comment from ye.zou: Code Review: zstackio/zstack MR !9141Feature: thirdPartySoftwarePackage support 🔴 Critical (必须修复)1. NPE 风险 — backupStorageHostUuid 可能为 null mons.stream().filter(m -> m.getUuid().equals(msg.getBackupStorageHostUuid())).findAny().orElse(null);如果 2. URL scheme 空字符串未校验 if (scheme == null) { ... }
cmd.urlScheme = scheme; // scheme 可能是 ""建议: 3. SSH 密码 @nologging 注解缺少 type 🟡 Major (强烈建议修复)4. 新 Message 类缺少输入验证
5. 大文件操作 timeout 策略 6. Response vs Reply 字段命名不一致 🔵 Minor
🤖 AI Code Review |
|
Comment from tao.gan: 1 NPE 风险 已修改 2 URL scheme 空字符串未校 已修改 3 SSH 密码 @nologging 注解缺少 type 现有用法，不需要增加 type 4 新 Message 类缺少输入验证 为内部消息，这类消息的参数在api中已校验 5 大文件操作 timeout 策略 上传文件超时时间由 agent 处理，不需要 控制面设置超时 6 Response vs Reply 同一改为 directUploadUrl 7 表示消息目前进 重载 此函数的子类支持处理此逻辑。 8 多处 运行case中不执行相关逻辑的通过做法
已修改 |
APIImpact Resolves: ZSV-1000 Change-Id: I656979726c736267776c7262716969726874716d
MatheMatrix
force-pushed
the
sync/tao.gan/zmigrate-ZSV-1000@@3
branch
from
4fac778 to
78a5433
Compare
|
Comment from gitlab: 自上次添加REVIEWED标签(2026-02-12 22:53:09.000Z)后, 有新的COMMIT更新(2026-02-14 10:18:03.007Z), 所以移除了REVIEWED标签 |
|
Comment from ye.zou: Code Review: zstackio/zstack MR !9141Branch: 🔴 CRITICAL (必须修复)1. CephBackupStorageBase.java:2149 — NPE 风险 // 修复建议：
if (rsp.handleMon != null) {
reply.setBackupStorageHostUuid(rsp.handleMon.getMonUuid());
}2. CephBackupStorageBase.java:2183 — SSH 密码明文传输 3. SoftwareUpgradePackageCmd — 缺少输入校验
4. CephBackupStorageBase.java:2138 — URL Scheme 未白名单化 🟡 MAJOR (强烈建议)
🔵 MINOR
✅ 做得好
Review by AI Code Reviewer |
5 participants
APIImpact
Resolves: ZSV-1
Change-Id: I656979726c736267776c7262716969726874716d
sync from gitlab !9141