feat: add batch install biz #29
Conversation
|
Warning Rate limit exceeded@gaosaroma has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 37 minutes and 46 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
WalkthroughThis pull request introduces enhancements to the Koupleless runtime system, focusing on batch installation of business modules. The changes span multiple files, introducing new constants, updating response structures, and modifying controller and tunnel implementations. The primary objectives include adding support for batch business module installation, improving error handling, and refactoring runtime information management by removing the Changes
Sequence DiagramsequenceDiagram
participant Client
participant MqttTunnel
participant ArkService
participant Controller
Client->>MqttTunnel: Batch Install Business Modules
MqttTunnel->>ArkService: BatchInstallBiz Request
ArkService->>Controller: Process Installation
Controller-->>ArkService: Installation Response
ArkService-->>MqttTunnel: Batch Install Response
MqttTunnel->>Client: Notify Installation Status
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (2)
module_tunnels/koupleless_http_tunnel/ark_service/ark_service.go (2)
Line range hint
22-38: HTTP status handling lacking.
Currently, you only check for network errors (err) or nil responses. Consider verifyingresp.StatusCodefor non-200 statuses:resp, err := h.client.R(). SetContext(ctx). SetBody(req). Post(fmt.Sprintf("http://%s:%d/installBiz", baseIP, arkletPort)) if err != nil { ... } + if resp.StatusCode() != 200 { + err = fmt.Errorf("unexpected status code: %d", resp.StatusCode()) + logger.Error(err, "installBiz returned non-200 response") + return + } if err = json.Unmarshal(resp.Body(), &response); err != nil { ... }
Line range hint
49-66: Similarly handle non-200 responses in UninstallBiz.
Applying the same status code checks here strengthens error handling consistency:resp, err := h.client.R(). SetContext(ctx). SetBody(req). Post(fmt.Sprintf("http://%s:%d/uninstallBiz", baseIP, arkletPort)) if err != nil { ... } + if resp.StatusCode() != 200 { + err = fmt.Errorf("unexpected status code: %d", resp.StatusCode()) + logger.Error(err, "uninstallBiz returned non-200 response") + return + } if err = json.Unmarshal(resp.Body(), &response); err != nil { ... }
🧹 Nitpick comments (7)
controller/module_deployment_controller/module_deployment_controller_test.go (1)
10-10: Consider using a typed channel and expanding test coverage.This channel is declared with
interface{}, which offers no compile-time type checks. If the usage is known and consistent, consider defining a typed channel (e.g.,chan struct{}orchan someType) to reduce the risk of sending/receiving unexpected data types. Also, there are no assertions verifying that thedeploymentAddHandleranddeploymentUpdateHandleroperations produce the intended results. Consider adding validation logic to ensure these handlers operate as expected.controller/module_deployment_controller/module_deployment_controller.go (1)
105-108: Avoid discarding the original error context.Reassigning a new error message (
errors.New("failed to list deployments")) overwrites the original error details (e.g., stack trace, message). Consider wrapping or logging the original error instead of discarding it.Below is an example of how you might wrap the original error:
err = moduleDeploymentController.cache.List(ctx, depList, &client.ListOptions{ LabelSelector: deploymentSelector, }) -if err != nil { - err = errors.New("failed to list deployments") +if err != nil { + wrappedErr := fmt.Errorf("failed to list deployments: %w", err) return }module_tunnels/koupleless_mqtt_tunnel/mqtt_tunnel.go (4)
18-18: Inconsistent logging library usage.
You already importzaploggerand frequently usezlogger; consider standardizing your logging approach to maintain consistency.
63-106: Reduce repetitive subscription code.
You’re subscribing to multiple MQTT topics with largely identical logic. As a good-to-have, consider consolidating this repetition into a small loop or helper function:func (mqttTunnel *MqttTunnel) RegisterNode(nodeInfo vkModel.NodeInfo) error { zlogger := zaplogger.FromContext(mqttTunnel.ctx) nodeID := utils2.ExtractNodeIDFromNodeName(nodeInfo.Metadata.Name) - var err error - topic := fmt.Sprintf(model.BaseHealthTopic, mqttTunnel.env, nodeID) - err = mqttTunnel.mqttClient.Sub(topic, mqtt.Qos1, mqttTunnel.healthMsgCallback) - if err != nil { - zlogger.Error(err, fmt.Sprintf("Error when registering node: %s for topic: %s", nodeID, topic)) - return err - } - - topic = fmt.Sprintf(model.BaseSimpleBizTopic, mqttTunnel.env, nodeID) - err = mqttTunnel.mqttClient.Sub(topic, mqtt.Qos1, mqttTunnel.bizMsgCallback) - if err != nil { - zlogger.Error(err, fmt.Sprintf("Error when registering node: %s for topic: %s", nodeID, topic)) - return err - } - - ... - + topics := []struct { + format string + cb paho.MessageHandler + }{ + {model.BaseHealthTopic, mqttTunnel.healthMsgCallback}, + {model.BaseSimpleBizTopic, mqttTunnel.bizMsgCallback}, + {model.BaseAllBizTopic, mqttTunnel.allBizMsgCallback}, + {model.BaseBizOperationResponseTopic, mqttTunnel.bizOperationResponseCallback}, + {model.BaseBatchInstallBizResponseTopic, mqttTunnel.batchInstallBizResponseCallback}, + } + for _, t := range topics { + topic := fmt.Sprintf(t.format, mqttTunnel.env, nodeID) + err := mqttTunnel.mqttClient.Sub(topic, mqtt.Qos1, t.cb) + if err != nil { + zlogger.Error(err, fmt.Sprintf("Error when registering node: %s for topic: %s", nodeID, topic)) + return err + } + } mqttTunnel.Lock() defer mqttTunnel.Unlock() return nil }
221-221: Verify logging placeholders.
Thelog.G(mqttTunnel.ctx).Info("queried baseline {} ", baselineBizs)usage with{}might not expand as intended. Confirm the correct formatting placeholder or switch to azloggercall for consistency.
418-420: Consider enumerating possible biz states.
While this helper is succinct, you might eventually include other “active” states. Using an enum or a map-based lookup could be more future-proof.module_tunnels/koupleless_http_tunnel/http_tunnel.go (1)
Line range hint
76-86: Consider validating the provided node metadata and handling errors more robustly.While
RegisterNodenow returns an error, the function body always returnsnil. Evaluate whether any validation failures or exceptional states (e.g., inability to set up the node network info) should propagate an actual error instead of silently succeeding.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
common/model/consts.go(2 hunks)common/model/model.go(1 hunks)controller/module_deployment_controller/module_deployment_controller.go(2 hunks)controller/module_deployment_controller/module_deployment_controller_test.go(1 hunks)controller/module_deployment_controller/runtime_info_store.go(0 hunks)controller/module_deployment_controller/runtime_info_store_test.go(0 hunks)module_tunnels/koupleless_http_tunnel/ark_service/ark_service.go(3 hunks)module_tunnels/koupleless_http_tunnel/ark_service/model.go(1 hunks)module_tunnels/koupleless_http_tunnel/http_tunnel.go(2 hunks)module_tunnels/koupleless_mqtt_tunnel/mqtt_tunnel.go(4 hunks)suite/http/mock_http_base.go(2 hunks)
💤 Files with no reviewable changes (2)
- controller/module_deployment_controller/runtime_info_store_test.go
- controller/module_deployment_controller/runtime_info_store.go
🧰 Additional context used
🪛 golangci-lint (1.62.2)
suite/http/mock_http_base.go
241-241: undefined: ark.ArkClientResponse
(typecheck)
268-268: undefined: ark.ArkClientResponse
(typecheck)
🔇 Additional comments (10)
controller/module_deployment_controller/module_deployment_controller.go (1)
52-53: Be cautious with theupdateTokenchannel usage.Since
updateTokenis a buffered channel with capacity 1 and is used as a simple lock mechanism (one writer at a time), multiple concurrent calls toupdateDeploymentReplicasmay be queued. If this is the intended design, consider documenting it explicitly. Alternatively, explore standard concurrency primitives such as mutexes or semaphores for clarity.Run the following script to confirm all references to
updateTokenare properly serialized or synchronized:✅ Verification successful
Let me gather more context about the usage of
updateTokento understand the synchronization pattern better.
The
updateTokenchannel is properly used as a mutex-like synchronization mechanismThe code inspection reveals that
updateTokenis correctly implemented as a synchronization primitive:
- It's initialized with a buffer size of 1 in the constructor and immediately filled with a token in
SetupWithManager- The
updateDeploymentReplicasmethod properly uses it as a mutex by:
- Acquiring the lock:
<-moduleDeploymentController.updateToken- Releasing the lock in a deferred function:
moduleDeploymentController.updateToken <- nilThis pattern ensures that only one goroutine can execute the critical section at a time, with others being blocked until the token becomes available. The implementation is thread-safe and follows a common Go pattern for using channels as mutexes.
While a
sync.Mutexwould be more idiomatic for this use case, the current implementation is correct and safe.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Searching for references to updateToken in the codebase to confirm usage. rg "updateToken"Length of output: 737
Script:
#!/bin/bash # Get the full context of updateToken usage rg "updateToken" -B 5 -A 5Length of output: 5961
module_tunnels/koupleless_http_tunnel/ark_service/model.go (1)
16-21: Adopting generics for ArkResponse is a clean and flexible design.
Using a type parameter to defineDataenhances reusability and readability. This approach aligns well with Go's generics and makes the response type more adaptable.module_tunnels/koupleless_mqtt_tunnel/mqtt_tunnel.go (2)
21-21: Import statement recognized & validated.
Thestringsimport appears correct and consistent with your usage.
364-388: EnsureonAllBizStatusArrivedis non-nil.
Before invokingonAllBizStatusArrived, confirm it’s set. Although this might be guaranteed elsewhere, adding a safeguard can prevent panics if the callback is ever nil.common/model/consts.go (2)
52-53: New command for batch operations recognized.
TheCommandBatchInstallBizconstant is consistent with the naming of existing commands.
70-71: Additional MQTT topic for batch operations looks good.
The naming aligns with existing patterns for p2p mode topics.common/model/model.go (2)
33-37: Generic response adoption is consistent.
SwitchingResponsetoArkResponse[ark.ArkResponseData]matches recent changes, ensuring uniform response handling across the codebase.
39-41: New BatchInstallBizResponse struct is well-defined.
The naming is clear, and this struct follows the established response pattern.module_tunnels/koupleless_http_tunnel/ark_service/ark_service.go (1)
8-8: Import usage recognized.
Importingarkis valid for aligning with the new generic response approach.suite/http/mock_http_base.go (1)
269-272:⚠️ Potential issue
ark.ArkClientResponseis also undefined here.As above, update or define
ark.ArkClientResponseto reference the correct struct in the proper package.Proposed fix (assuming the intended struct exists in
ark_service):- ArkClientResponse: ark.ArkClientResponse{ + ArkClientResponse: ark_service.ArkClientResponse{ Code: "SUCCESS", Message: "", BizInfos: nil, },Likely invalid or redundant comment.
| func (mqttTunnel *MqttTunnel) convertToBizStatusData(msg model.ArkMqttMsg[model.BatchInstallBizResponse]) []vkModel.BizStatusData { | ||
| var result []vkModel.BizStatusData | ||
| arkBatchInstallResponse := msg.Data.Response.Data | ||
| for _, bizResponse := range arkBatchInstallResponse.BizUrlToResponse { | ||
| bizInfo := bizResponse.BizInfos[0] | ||
| bizName := bizInfo.BizName | ||
| bizVersion := bizInfo.BizVersion | ||
| bizState := strings.ToUpper(bizInfo.BizState) | ||
| changeTime := time.UnixMilli(msg.PublishTimestamp) | ||
| message := bizResponse.Message | ||
| var reason string | ||
| if mqttTunnel.isActivatedBiz(bizInfo.BizState) { | ||
| reason = fmt.Sprintf("%s:%s %s succeed", bizName, bizVersion, bizState) | ||
| } else { | ||
| reason = fmt.Sprintf("%s:%s %s failed", bizName, bizVersion, bizState) | ||
| } | ||
| result = append(result, vkModel.BizStatusData{ | ||
| Key: utils.GetBizIdentity(bizName, bizVersion), | ||
| Name: bizName, | ||
| State: bizState, | ||
| ChangeTime: changeTime, | ||
| Reason: reason, | ||
| Message: message, | ||
| }) | ||
| } | ||
| return result | ||
| } |
There was a problem hiding this comment.
Potential index out-of-bounds.
At line 394, you access bizResponse.BizInfos[0]. If BizInfos is empty, this triggers a panic. Consider validating the length before indexing:
for _, bizResponse := range arkBatchInstallResponse.BizUrlToResponse {
- bizInfo := bizResponse.BizInfos[0]
+ if len(bizResponse.BizInfos) == 0 {
+ zlogger.Warn("No BizInfos found in BizUrlToResponse, skipping...")
+ continue
+ }
+ bizInfo := bizResponse.BizInfos[0]
...
}📝 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.
| func (mqttTunnel *MqttTunnel) convertToBizStatusData(msg model.ArkMqttMsg[model.BatchInstallBizResponse]) []vkModel.BizStatusData { | |
| var result []vkModel.BizStatusData | |
| arkBatchInstallResponse := msg.Data.Response.Data | |
| for _, bizResponse := range arkBatchInstallResponse.BizUrlToResponse { | |
| bizInfo := bizResponse.BizInfos[0] | |
| bizName := bizInfo.BizName | |
| bizVersion := bizInfo.BizVersion | |
| bizState := strings.ToUpper(bizInfo.BizState) | |
| changeTime := time.UnixMilli(msg.PublishTimestamp) | |
| message := bizResponse.Message | |
| var reason string | |
| if mqttTunnel.isActivatedBiz(bizInfo.BizState) { | |
| reason = fmt.Sprintf("%s:%s %s succeed", bizName, bizVersion, bizState) | |
| } else { | |
| reason = fmt.Sprintf("%s:%s %s failed", bizName, bizVersion, bizState) | |
| } | |
| result = append(result, vkModel.BizStatusData{ | |
| Key: utils.GetBizIdentity(bizName, bizVersion), | |
| Name: bizName, | |
| State: bizState, | |
| ChangeTime: changeTime, | |
| Reason: reason, | |
| Message: message, | |
| }) | |
| } | |
| return result | |
| } | |
| func (mqttTunnel *MqttTunnel) convertToBizStatusData(msg model.ArkMqttMsg[model.BatchInstallBizResponse]) []vkModel.BizStatusData { | |
| var result []vkModel.BizStatusData | |
| arkBatchInstallResponse := msg.Data.Response.Data | |
| for _, bizResponse := range arkBatchInstallResponse.BizUrlToResponse { | |
| if len(bizResponse.BizInfos) == 0 { | |
| zlogger.Warn("No BizInfos found in BizUrlToResponse, skipping...") | |
| continue | |
| } | |
| bizInfo := bizResponse.BizInfos[0] | |
| bizName := bizInfo.BizName | |
| bizVersion := bizInfo.BizVersion | |
| bizState := strings.ToUpper(bizInfo.BizState) | |
| changeTime := time.UnixMilli(msg.PublishTimestamp) | |
| message := bizResponse.Message | |
| var reason string | |
| if mqttTunnel.isActivatedBiz(bizInfo.BizState) { | |
| reason = fmt.Sprintf("%s:%s %s succeed", bizName, bizVersion, bizState) | |
| } else { | |
| reason = fmt.Sprintf("%s:%s %s failed", bizName, bizVersion, bizState) | |
| } | |
| result = append(result, vkModel.BizStatusData{ | |
| Key: utils.GetBizIdentity(bizName, bizVersion), | |
| Name: bizName, | |
| State: bizState, | |
| ChangeTime: changeTime, | |
| Reason: reason, | |
| Message: message, | |
| }) | |
| } | |
| return result | |
| } |
| ArkClientResponse: ark.ArkClientResponse{ | ||
| Code: "SUCCESS", | ||
| Message: "", | ||
| BizInfos: nil, | ||
| }, |
There was a problem hiding this comment.
ark.ArkClientResponse is undefined.
It appears that the ark package does not export a type named ArkClientResponse. You may need to reference the correct struct in either the ark_service or another relevant package, or define ArkClientResponse in the ark package if that's the intended usage.
Proposed fix (assuming the intended struct exists in ark_service):
- ArkClientResponse: ark.ArkClientResponse{
+ ArkClientResponse: ark_service.ArkClientResponse{
Code: "SUCCESS",
Message: "",
BizInfos: nil,
},Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 golangci-lint (1.62.2)
241-241: undefined: ark.ArkClientResponse
(typecheck)
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
common/model/model.go(1 hunks)
🧰 Additional context used
🪛 GitHub Check: unit-test
common/model/model.go
[failure] 42-42:
undefined: ark.ArkBatchInstallResponse
🪛 GitHub Actions: lvjing2 pushed module-controller code
common/model/model.go
[error] 42-42: undefined: ark.ArkBatchInstallResponse - Type reference not found
🔇 Additional comments (1)
common/model/model.go (1)
34-38: LGTM! Well-structured type enhancement.The update to use a generic type parameter
ark.ArkResponseDataimproves type safety and makes the response structure more explicit.
| type BatchInstallBizResponse struct { | ||
| Command string `json:"command"` // Operation command executed | ||
| Response ark_service.ArkResponse[ark.ArkBatchInstallResponse] `json:"response"` // Response from ark service |
There was a problem hiding this comment.
💡 Codebase verification
Dependency issue confirmed - ArkBatchInstallResponse type not available
The search results show that the code is using github.com/koupleless/arkctl v0.2.3, but the ArkBatchInstallResponse type is not found in the current codebase. However, the type is being used in multiple locations:
common/model/model.go: In the struct definitionmodule_tunnels/koupleless_mqtt_tunnel/mqtt_tunnel.go: Being used to process response data
Since:
- The type is not found in the current version of dependencies
- The code is already using this type in multiple places
- The PR description mentions a dependency on Feat: add batch install biz handler runtime#231
This confirms that this PR has an unmet dependency that needs to be resolved first.
Please:
- Wait for Feat: add batch install biz handler runtime#231 to be merged
- Update the dependency version in go.mod to include the new type
- Document the structure of
ArkBatchInstallResponsein your code comments
🔗 Analysis chain
Verify dependency and add documentation for batch install response.
The pipeline failure indicates that ark.ArkBatchInstallResponse is undefined. This is likely because this PR depends on koupleless/runtime#231.
Please ensure:
- The dependency PR (Feat: add batch install biz handler runtime#231) is merged first
- Add documentation describing the structure and fields of
ArkBatchInstallResponse
Run this script to verify the dependency:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the required type is defined in the runtime repository
# Expected: Find the type definition in the dependent PR
# Check if the type exists in current branch
rg -g '*.go' "type ArkBatchInstallResponse"
# Check go.mod for runtime dependency version
cat go.mod | grep "koupleless/runtime"
Length of output: 84
Script:
#!/bin/bash
# Let's try a broader search to understand the context better
# Search for any reference to BatchInstallResponse
rg -g '*.go' "BatchInstallResponse"
# Check for ark service related types
rg -g '*.go' "type.*Response.*ark"
# Look for imports related to ark
rg -g '*.go' 'import.*".*ark.*"'
# Check go.mod content to understand dependencies
cat go.mod
Length of output: 4321
🧰 Tools
🪛 GitHub Check: unit-test
[failure] 42-42:
undefined: ark.ArkBatchInstallResponse
🪛 GitHub Actions: lvjing2 pushed module-controller code
[error] 42-42: undefined: ark.ArkBatchInstallResponse - Type reference not found
# Conflicts: # go.mod # go.sum
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
suite/mqtt/mock_mqtt_base.go (2)
234-242: Use descriptive messages or constants for success codes, and handle possible errors injson.Marshal.
Your new generic response structure andArkClientResponseare helpful for flexible data returns. However, consider using named constants (e.g.,ark.SUCCESS) instead of"SUCCESS"strings for maintainability, and setting a more descriptive message reflecting the uninstall operation’s outcome. Additionally, eachjson.Marshalcall currently discards potential errors, which could mask serialization issues.
300-308: Extract repeated response-building logic into a helper function.
Lines 300-308 inSetBizStatelargely replicate the same structure as lines 234-242 inprocessUnInstallBiz. Consider creating a shared utility or constructor to buildark_service.ArkResponse[ark.ArkResponseData]objects. This approach follows the DRY principle and simplifies future maintenance.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
suite/http/mock_http_base.go(8 hunks)suite/http/module_http_lifecycle_test.go(2 hunks)suite/mqtt/mock_mqtt_base.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- suite/mqtt/mock_mqtt_base.go
- suite/http/mock_http_base.go
🧰 Additional context used
🪛 GitHub Actions: gaosaroma pushed module-controller code
suite/http/module_http_lifecycle_test.go
[error] 54-54: Test failure: 'publish a module pod and it should be running' timed out after 20.000s. Expected false to be true
| @@ -38,7 +38,7 @@ var _ = Describe("Module Lifecycle Test", func() { | |||
| } | |||
| } | |||
| return err == nil && vnodeReady | |||
| }, time.Second*50, time.Second).Should(BeTrue()) | |||
| }, time.Second*20, time.Second).Should(BeTrue()) | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potential flakiness due to reduced timeout
Cutting down the timeout from 50 seconds to 20 seconds may introduce test flakiness in environments with slower responses. Consider reinstating a longer timeout or using an adaptive approach.
Apply this diff to revert the timeout to 30 seconds for improved stability:
- }, time.Second*20, time.Second).Should(BeTrue())
+ }, time.Second*30, time.Second).Should(BeTrue())📝 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.
| }, time.Second*20, time.Second).Should(BeTrue()) | |
| }, time.Second*30, time.Second).Should(BeTrue()) |
| @@ -51,7 +51,7 @@ | |||
| Name: mockModulePod.Name, | |||
| }, podFromKubernetes) | |||
| return err == nil && podFromKubernetes.Status.Phase == v1.PodRunning | |||
| }, time.Second*50, time.Second).Should(BeTrue()) | |||
| }, time.Second*20, time.Second).Should(BeTrue()) | |||
There was a problem hiding this comment.
Pipeline failure suggests test timing issues
This test has consistently timed out after 20 seconds, indicating that the environment might require more time to start up. Extending the timeout should help resolve these failures.
Apply this diff to restore the timeout to 40 seconds:
- }, time.Second*20, time.Second).Should(BeTrue())
+ }, time.Second*40, time.Second).Should(BeTrue())📝 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.
| }, time.Second*20, time.Second).Should(BeTrue()) | |
| }, time.Second*40, time.Second).Should(BeTrue()) |
🧰 Tools
🪛 GitHub Actions: gaosaroma pushed module-controller code
[error] 54-54: Test failure: 'publish a module pod and it should be running' timed out after 20.000s. Expected false to be true
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
module_tunnels/koupleless_http_tunnel/ark_service/ark_service.go (1)
42-46: Improve error messages using fmt.ErrorfThe error handling has been improved by adding HTTP status code checks. However, the error message construction can be enhanced using
fmt.Errorffor better readability and performance.Apply this diff to improve the error messages:
-err = errors.New(fmt.Sprintf("response status: %d", resp.StatusCode())) +err = fmt.Errorf("response status: %d", resp.StatusCode())Also applies to: 75-79, 108-112, 141-145
🧰 Tools
🪛 golangci-lint (1.62.2)
43-43: S1028: should use fmt.Errorf(...) instead of errors.New(fmt.Sprintf(...))
(gosimple)
module_tunnels/koupleless_http_tunnel/http_tunnel.go (1)
Line range hint
76-86: Consider returning meaningful errors in RegisterNodeThe method now returns an error but always returns nil. Consider returning meaningful errors when node registration fails.
For example, return an error if the node ID is empty or if there's a registration conflict.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
module_tunnels/koupleless_http_tunnel/ark_service/ark_service.go(7 hunks)module_tunnels/koupleless_http_tunnel/http_tunnel.go(3 hunks)
🧰 Additional context used
🪛 golangci-lint (1.62.2)
module_tunnels/koupleless_http_tunnel/ark_service/ark_service.go
43-43: S1028: should use fmt.Errorf(...) instead of errors.New(fmt.Sprintf(...))
(gosimple)
76-76: S1028: should use fmt.Errorf(...) instead of errors.New(fmt.Sprintf(...))
(gosimple)
109-109: S1028: should use fmt.Errorf(...) instead of errors.New(fmt.Sprintf(...))
(gosimple)
module_tunnels/koupleless_http_tunnel/http_tunnel.go
252-252: printf: fmt.Sprintf format %s has arg data.Response of wrong type github.com/koupleless/module_controller/module_tunnels/koupleless_http_tunnel/ark_service.ArkResponse[github.com/koupleless/arkctl/v1/service/ark.ArkResponseData]
(govet)
🪛 GitHub Check: unit-test
module_tunnels/koupleless_http_tunnel/http_tunnel.go
[failure] 252-252:
fmt.Sprintf format %s has arg data.Response of wrong type github.com/koupleless/module_controller/module_tunnels/koupleless_http_tunnel/ark_service.ArkResponse[github.com/koupleless/arkctl/v1/service/ark.ArkResponseData]
🪛 GitHub Actions: gaosaroma pushed module-controller code
module_tunnels/koupleless_http_tunnel/http_tunnel.go
[error] 252-252: Format string error: fmt.Sprintf format %s has argument 'data.Response' of incorrect type 'ArkResponse[ArkResponseData]'
🔇 Additional comments (1)
module_tunnels/koupleless_http_tunnel/ark_service/ark_service.go (1)
23-23: LGTM: Generic type parameter improves type safetyThe return type change to
ArkResponse[ark.ArkResponseData]improves type safety and makes the API contract more explicit.Also applies to: 56-56
| @@ -226,24 +227,32 @@ | |||
| return | |||
| } | |||
| if h.onQueryAllBizDataArrived != nil { | |||
| fmt.Println("allBizMsgCallback!") | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove debug print statement
The fmt.Println statement appears to be a debug log that should be replaced with proper structured logging using the existing logger.
Apply this diff:
-fmt.Println("allBizMsgCallback!")
+logger := zaplogger.FromContext(h.ctx)
+logger.Debug("Processing all biz callback")Committable suggestion skipped: line range outside the PR's diff.
| func (httpTunnel *HttpTunnel) bizOperationResponseCallback(nodeID string, data model.BizOperationResponse) { | ||
| logger := zaplogger.FromContext(httpTunnel.ctx) | ||
| nodeName := utils2.FormatNodeName(nodeID, httpTunnel.env) | ||
| if data.Response.Code == "SUCCESS" { | ||
| if data.Command == model.CommandInstallBiz { | ||
| // not update here, update in all biz response callback | ||
| logger.Info("install biz success: ", data.BizName, data.BizVersion) | ||
| httpTunnel.onOneBizDataArrived(nodeName, vkModel.BizStatusData{ | ||
| Key: utils.GetBizIdentity(data.BizName, data.BizVersion), | ||
| Name: data.BizName, | ||
| State: string(vkModel.BizStateActivated), | ||
| ChangeTime: time.Now(), | ||
| Reason: fmt.Sprintf("%s:%s %s succeed", data.BizName, data.BizVersion, data.Command), | ||
| Message: data.Response.Data.Message, | ||
| }) | ||
| return | ||
| } else { | ||
| logger.Error(nil, fmt.Sprintf("biz operation failed: %s\n", data.Response)) | ||
| } | ||
| } else { | ||
| // operation failed, log | ||
| logger.Error(nil, "biz operation failed: %s\n%s\n%s", data.Response.Message, data.Response.ErrorStackTrace, data.Response.Data.Message) | ||
| } | ||
|
|
||
| h.onOneBizDataArrived(utils2.FormatNodeName(nodeID, h.env), vkModel.BizStatusData{ | ||
| httpTunnel.onOneBizDataArrived(nodeName, vkModel.BizStatusData{ |
There was a problem hiding this comment.
Fix format string error and improve error handling logic
There are several issues in this code segment:
- The format string error in line 252 is causing pipeline failures.
- The success condition only handles
CommandInstallBiz, leaving other successful operations unhandled.
Apply this diff to fix the issues:
if data.Response.Code == "SUCCESS" {
if data.Command == model.CommandInstallBiz {
logger.Info("install biz success: ", data.BizName, data.BizVersion)
httpTunnel.onOneBizDataArrived(nodeName, vkModel.BizStatusData{
Key: utils.GetBizIdentity(data.BizName, data.BizVersion),
Name: data.BizName,
State: string(vkModel.BizStateActivated),
ChangeTime: time.Now(),
Reason: fmt.Sprintf("%s:%s %s succeed", data.BizName, data.BizVersion, data.Command),
Message: data.Response.Data.Message,
})
return
- } else {
- logger.Error(nil, fmt.Sprintf("biz operation failed: %s\n", data.Response))
}
+ if data.Command == model.CommandUnInstallBiz {
+ logger.Info("uninstall biz success: ", data.BizName, data.BizVersion)
+ httpTunnel.onOneBizDataArrived(nodeName, vkModel.BizStatusData{
+ Key: utils.GetBizIdentity(data.BizName, data.BizVersion),
+ Name: data.BizName,
+ State: string(vkModel.BizStateDeactivated),
+ ChangeTime: time.Now(),
+ Reason: fmt.Sprintf("%s:%s %s succeed", data.BizName, data.BizVersion, data.Command),
+ Message: data.Response.Data.Message,
+ })
+ return
+ }
+} else {
+ logger.Error(nil, "biz operation failed",
+ "bizName", data.BizName,
+ "bizVersion", data.BizVersion,
+ "command", data.Command,
+ "code", data.Response.Code,
+ "message", data.Response.Message)
}📝 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.
| func (httpTunnel *HttpTunnel) bizOperationResponseCallback(nodeID string, data model.BizOperationResponse) { | |
| logger := zaplogger.FromContext(httpTunnel.ctx) | |
| nodeName := utils2.FormatNodeName(nodeID, httpTunnel.env) | |
| if data.Response.Code == "SUCCESS" { | |
| if data.Command == model.CommandInstallBiz { | |
| // not update here, update in all biz response callback | |
| logger.Info("install biz success: ", data.BizName, data.BizVersion) | |
| httpTunnel.onOneBizDataArrived(nodeName, vkModel.BizStatusData{ | |
| Key: utils.GetBizIdentity(data.BizName, data.BizVersion), | |
| Name: data.BizName, | |
| State: string(vkModel.BizStateActivated), | |
| ChangeTime: time.Now(), | |
| Reason: fmt.Sprintf("%s:%s %s succeed", data.BizName, data.BizVersion, data.Command), | |
| Message: data.Response.Data.Message, | |
| }) | |
| return | |
| } else { | |
| logger.Error(nil, fmt.Sprintf("biz operation failed: %s\n", data.Response)) | |
| } | |
| } else { | |
| // operation failed, log | |
| logger.Error(nil, "biz operation failed: %s\n%s\n%s", data.Response.Message, data.Response.ErrorStackTrace, data.Response.Data.Message) | |
| } | |
| h.onOneBizDataArrived(utils2.FormatNodeName(nodeID, h.env), vkModel.BizStatusData{ | |
| httpTunnel.onOneBizDataArrived(nodeName, vkModel.BizStatusData{ | |
| func (httpTunnel *HttpTunnel) bizOperationResponseCallback(nodeID string, data model.BizOperationResponse) { | |
| logger := zaplogger.FromContext(httpTunnel.ctx) | |
| nodeName := utils2.FormatNodeName(nodeID, httpTunnel.env) | |
| if data.Response.Code == "SUCCESS" { | |
| if data.Command == model.CommandInstallBiz { | |
| logger.Info("install biz success: ", data.BizName, data.BizVersion) | |
| httpTunnel.onOneBizDataArrived(nodeName, vkModel.BizStatusData{ | |
| Key: utils.GetBizIdentity(data.BizName, data.BizVersion), | |
| Name: data.BizName, | |
| State: string(vkModel.BizStateActivated), | |
| ChangeTime: time.Now(), | |
| Reason: fmt.Sprintf("%s:%s %s succeed", data.BizName, data.BizVersion, data.Command), | |
| Message: data.Response.Data.Message, | |
| }) | |
| return | |
| } | |
| if data.Command == model.CommandUnInstallBiz { | |
| logger.Info("uninstall biz success: ", data.BizName, data.BizVersion) | |
| httpTunnel.onOneBizDataArrived(nodeName, vkModel.BizStatusData{ | |
| Key: utils.GetBizIdentity(data.BizName, data.BizVersion), | |
| Name: data.BizName, | |
| State: string(vkModel.BizStateDeactivated), | |
| ChangeTime: time.Now(), | |
| Reason: fmt.Sprintf("%s:%s %s succeed", data.BizName, data.BizVersion, data.Command), | |
| Message: data.Response.Data.Message, | |
| }) | |
| return | |
| } | |
| } else { | |
| logger.Error(nil, "biz operation failed", | |
| "bizName", data.BizName, | |
| "bizVersion", data.BizVersion, | |
| "command", data.Command, | |
| "code", data.Response.Code, | |
| "message", data.Response.Message) | |
| } | |
| httpTunnel.onOneBizDataArrived(nodeName, vkModel.BizStatusData{ |
🧰 Tools
🪛 GitHub Check: unit-test
[failure] 252-252:
fmt.Sprintf format %s has arg data.Response of wrong type github.com/koupleless/module_controller/module_tunnels/koupleless_http_tunnel/ark_service.ArkResponse[github.com/koupleless/arkctl/v1/service/ark.ArkResponseData]
🪛 golangci-lint (1.62.2)
252-252: printf: fmt.Sprintf format %s has arg data.Response of wrong type github.com/koupleless/module_controller/module_tunnels/koupleless_http_tunnel/ark_service.ArkResponse[github.com/koupleless/arkctl/v1/service/ark.ArkResponseData]
(govet)
🪛 GitHub Actions: gaosaroma pushed module-controller code
[error] 252-252: Format string error: fmt.Sprintf format %s has argument 'data.Response' of incorrect type 'ArkResponse[ArkResponseData]'
fix: koupleless/koupleless#361
modifications:
依赖这三个 PR：
koupleless/arkctl#14
koupleless/runtime#231
koupleless/virtual-kubelet#34
Summary by CodeRabbit
New Features
Improvements
Refactoring
RuntimeInfoStoreand related runtime storage management.Chores