-
-
Notifications
You must be signed in to change notification settings - Fork 689
Handle KubeBlock v5 upgrade API call for horizontal scaling #2063
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.
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds backward compatibility support for KubeBlock v5 MongoDB 5.0 clusters by calling an external upgrade API during horizontal scaling operations.
Key Changes:
- Added new configuration constant
KUBEBLOCK_V5_UPGRADE_URLfor the external upgrade API endpoint - Integrated KubeBlock v5 upgrade API call into the horizontal scaling workflow with 3-second timeout
- Implemented version detection logic to identify v5 MongoDB 5.0 clusters using cluster labels
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
server/src/constants.ts |
Added configuration for the v5 upgrade URL and timeout constant (3s) |
server/src/database/dedicated-database/dedicated-database.service.ts |
Implemented v5 compatibility layer with manifest detection, API call handling, and response parsing for horizontal scaling operations |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const response = await fetch(url, { | ||
| method: 'POST', | ||
| headers: { | ||
| 'Content-Type': 'application/json', | ||
| }, | ||
| body: JSON.stringify({ | ||
| namespace, | ||
| database_name: clusterName, | ||
| replicas, | ||
| }), | ||
| signal: controller.signal, | ||
| }) |
Copilot
AI
Dec 1, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fetch API is being used directly without any visible import. While fetch is globally available in Node.js 18+, this codebase appears to use @nestjs/axios (HttpService) for HTTP requests elsewhere in the project (e.g., in http-interceptor.service.ts, wechat-pay.service.ts). For consistency and better error handling, consider using the injected HttpService instead of the global fetch API.
Using HttpService would provide:
- Consistent error handling patterns across the codebase
- Built-in timeout support via axios configuration
- Better integration with NestJS ecosystem
| const responseData = await this.parseKubeBlockV5Response(response) | ||
|
|
||
| if (!response.ok) { | ||
| throw new Error( | ||
| `HTTP error! status: ${response.status}, statusText: ${ | ||
| response.statusText | ||
| }, body: ${JSON.stringify(responseData)}`, | ||
| ) | ||
| } | ||
|
|
Copilot
AI
Dec 1, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The response parsing happens before checking response.ok, which means the response body is consumed before the error check. If the response is not OK and has a large body, this could be inefficient. More importantly, if parseKubeBlockV5Response fails (e.g., malformed JSON), the error message won't include the HTTP status information.
Consider checking response.ok first, then parsing the response body only when needed:
if (!response.ok) {
const errorBody = await this.parseKubeBlockV5Response(response)
throw new Error(
`HTTP error! status: ${response.status}, statusText: ${response.statusText}, body: ${JSON.stringify(errorBody)}`
)
}
const responseData = await this.parseKubeBlockV5Response(response)| const responseData = await this.parseKubeBlockV5Response(response) | |
| if (!response.ok) { | |
| throw new Error( | |
| `HTTP error! status: ${response.status}, statusText: ${ | |
| response.statusText | |
| }, body: ${JSON.stringify(responseData)}`, | |
| ) | |
| } | |
| if (!response.ok) { | |
| const errorBody = await this.parseKubeBlockV5Response(response) | |
| throw new Error( | |
| `HTTP error! status: ${response.status}, statusText: ${ | |
| response.statusText | |
| }, body: ${JSON.stringify(errorBody)}`, | |
| ) | |
| } | |
| const responseData = await this.parseKubeBlockV5Response(response) |
| this.logger.error( | ||
| `Failed to call KubeBlock v5 upgrade API for ${appid}: ${error.message}`, | ||
| ) |
Copilot
AI
Dec 1, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When the AbortController aborts the request due to timeout, it will throw an AbortError. However, this error is caught by the outer try-catch in handleKubeBlockV5Upgrade (line 740-745) which only logs error.message. For AbortError, the message might not clearly indicate it was a timeout.
Consider adding specific handling for timeout errors to provide clearer logging:
} catch (error) {
if (error.name === 'AbortError') {
this.logger.error(
`KubeBlock v5 upgrade API timeout (${KUBEBLOCK_V5_UPGRADE_API_TIMEOUT}ms) for ${appid}`
)
} else {
this.logger.error(
`Failed to call KubeBlock v5 upgrade API for ${appid}: ${error.message}`
)
}
}| this.logger.error( | |
| `Failed to call KubeBlock v5 upgrade API for ${appid}: ${error.message}`, | |
| ) | |
| if (error.name === 'AbortError') { | |
| this.logger.error( | |
| `KubeBlock v5 upgrade API timeout (${KUBEBLOCK_V5_UPGRADE_API_TIMEOUT}ms) for ${appid}` | |
| ) | |
| } else { | |
| this.logger.error( | |
| `Failed to call KubeBlock v5 upgrade API for ${appid}: ${error.message}`, | |
| ) | |
| } |
| const clusterName = manifest.metadata.name | ||
| const namespace = manifest.metadata.namespace |
Copilot
AI
Dec 1, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing null/undefined safety checks for manifest.metadata.name and manifest.metadata.namespace. While the manifest is passed from the calling function and likely has these properties, adding defensive checks would prevent potential runtime errors and align with the defensive programming pattern used elsewhere in the method (e.g., line 727).
Consider adding validation:
const clusterName = manifest?.metadata?.name
const namespace = manifest?.metadata?.namespace
if (!clusterName || !namespace) {
throw new Error('Invalid manifest: missing name or namespace')
}| const clusterName = manifest.metadata.name | |
| const namespace = manifest.metadata.namespace | |
| const clusterName = manifest?.metadata?.name | |
| const namespace = manifest?.metadata?.namespace | |
| if (!clusterName || !namespace) { | |
| throw new Error('Invalid manifest: missing name or namespace') | |
| } |
No description provided.