Skip to content

fix(Cubelet): use correct Cloud Hypervisor disk API endpoint#337

Merged
kinwin-ustc merged 1 commit into
TencentCloud:masterfrom
luzhixing12345:fix-reqwithctx
May 27, 2026
Merged

fix(Cubelet): use correct Cloud Hypervisor disk API endpoint#337
kinwin-ustc merged 1 commit into
TencentCloud:masterfrom
luzhixing12345:fix-reqwithctx

Conversation

@luzhixing12345
Copy link
Copy Markdown
Contributor

Fix Cubelet disk hotplug requests to use the requested Cloud Hypervisor endpoint instead of always calling vm.add-disk. This makes DelDisk correctly call vm.remove-device.

Also switch to http.NewRequestWithContext so caller timeout/cancellation propagates to the HTTP request.

Signed-off-by: luzhixing12345 <luzhixing12345@163.com>
@luzhixing12345 luzhixing12345 requested a review from fslongjin as a code owner May 22, 2026 06:32
@kinwin-ustc
Copy link
Copy Markdown
Collaborator

/cubebot review

@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 23, 2026

Claude finished @kinwin-ustc's task in 52s —— View job


Code Review Checklist

  • Read the PR diff and understand the changes
  • Analyze correctness of the fix
  • Check for edge cases, regressions, and potential issues
  • Post review feedback

Review Feedback

This is a focused, well-scoped bug fix with 3 interconnected changes in Cubelet/pkg/apis/shimapi/disk.go:

1. Actually using the path parameter in chRequest (line 96)

The core fix. Previously chRequest ignored its path parameter and hardcoded the URL to "add-disk", which meant DelDisk was calling vm.add-disk instead of vm.remove-device. The old code:

url := chAPIPrefix + "add-disk" // always vm.add-disk

Now correctly:

url := chAPIPrefix + path

2. Passing correct endpoint from DelDisk (line 87)

Changed from csc.chRequest(ctx, ipath, requestBody) to csc.chRequest(ctx, "remove-device", requestBody). Previously ipath held the display-friendly "vm.remove-device" which would have produced http://localhost/api/v1/vm.vm.remove-device — a double prefix — if the underlying chRequest had actually used the path parameter. This would have been a second bug. The fix correctly passes just the endpoint segment "remove-device".

3. Context propagation via http.NewRequestWithContext (line 98)

Good defensive change. Callers that pass a cancel/timeout context will now have it propagate to the HTTP request lifecycle, preventing hung requests. Without this, a cancelled caller context would leave the HTTP request running until completion or transport timeout.

4. Dynamic log field (line 107)

The "func" log field now uses the dynamic path instead of the hardcoded "AddDisk". This is more accurate and consistent. Minor note: if anyone has log-based monitoring filtering on "func": "AddDisk", they'll need to update their queries.

No issues found. The changes are correct, minimal, and the diff is easy to verify. The only observation worth noting:

  • The ipath variables in both AddDisk (line 54) and DelDisk (line 86) are now only used in error messages as human-readable endpoint names. Consider renaming them (e.g., endpointName) to avoid confusion, since they no longer represent a URL path. This is cosmetic and optional.

@luzhixing12345
Copy link
Copy Markdown
Contributor Author

Hi, may I ask when this PR might be merged?

@kinwin-ustc kinwin-ustc merged commit 5f8d81a into TencentCloud:master May 27, 2026
7 of 8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants