fix: http tunnel in cluster mode#44
Conversation
WalkthroughAdds an env-var presence check after generating clientID and logs the generated clientID if the env var is absent; populates NodeState in health->node-status translation; bumps virtual-kubelet dependency; minor import reorder in an HTTP tunnel file. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Main as Module Controller (main)
participant OS as Env Vars
participant Log as Logger
Main->>Main: Generate random clientID
Main->>OS: os.LookupEnv(EnvKeyOfClientID)
alt Env var missing
Main->>Log: Infof("Using generated clientID: %s", clientID)
else Env var present
Note over Main: No extra log
end
Main->>Main: Continue initialization
sequenceDiagram
autonumber
participant Health as HealthData
participant Utils as ConvertHealthDataToNodeStatus
participant VK as vkModel.NodeStatusData
Health->>Utils: pass health data
Utils->>VK: populate fields including NodeState = ToUpper(MasterBizInfo.BizState)
Utils->>VK: return NodeStatusData
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/module-controller/main.go (1)
276-278: Fix typo in user-facing error“deteched” → “detected”.
- return "", fmt.Errorf("multiple kubelet proxy services deteched in namespace %s, expected only one", namespace) + return "", fmt.Errorf("multiple kubelet proxy services detected in namespace %s, expected only one", namespace)
🧹 Nitpick comments (1)
cmd/module-controller/main.go (1)
88-91: Handle empty CLIENT_ID and make the log actionable; avoid eager UUID generationTreat empty value as unset, generate UUID lazily, and guide operators to persist it.
- clientID := utils.GetEnv(model.EnvKeyOfClientID, uuid.New().String()) - if _, ok := os.LookupEnv(model.EnvKeyOfClientID); !ok { - log.L.Infof("Randomly generated client ID: %s", clientID) - } + var clientID string + if clientIDEnv, ok := os.LookupEnv(model.EnvKeyOfClientID); !ok || clientIDEnv == "" { + clientID = uuid.New().String() + log.L.Infof("%s not set; generated client ID: %s. Set %s to persist across restarts.", model.EnvKeyOfClientID, clientID, model.EnvKeyOfClientID) + } else { + clientID = clientIDEnv + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cmd/module-controller/main.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
cmd/module-controller/main.go (1)
common/model/consts.go (1)
EnvKeyOfClientID(24-24)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: unit-test
- GitHub Check: lint
|
等待 koupleless/virtual-kubelet#46 通过后更新依赖 |
BetaCat0
force-pushed
the
fix/http-tunnel-cluster-mode
branch
from
e5e3c9b to
5077d10
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
common/utils/utils.go (2)
201-204: Bug: wrong error checked for NotFound after deleteUses
err(from Get) instead ofdeleteErr. This will misreport NotFound deletes as errors.- if deleteErr != nil && !apiErrors.IsNotFound(err) { + if deleteErr != nil && !apiErrors.IsNotFound(deleteErr) { logger.Error(deleteErr, "delete base node failed") }
216-220: Pass the actual parse error to logger
Error(nil, ...)drops the cause. Passerr.- if err != nil { - zaplogger.GetLogger().Error(nil, fmt.Sprintf("failed to parse port %s from node info", portStr)) + if err != nil { + zaplogger.GetLogger().Error(err, fmt.Sprintf("failed to parse port %s from node info", portStr)) port = 1238 }
🧹 Nitpick comments (2)
common/utils/utils.go (2)
138-139: Fix typo and clarify intent for PodKeyMinor: “fille” → “fill”. If PodKey is intentionally deferred, keep it commented; otherwise set it explicitly.
- // fille PodKey when using + // fill PodKey when used
197-197: Log context function name is misleadingThe function is OnBaseUnreachable but the context tag says OnNodeNotReady.
- logger := zaplogger.FromContext(ctx).WithValues("nodeName", nodeName, "func", "OnNodeNotReady") + logger := zaplogger.FromContext(ctx).WithValues("nodeName", nodeName, "func", "OnBaseUnreachable")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (4)
cmd/module-controller/main.go(1 hunks)common/utils/utils.go(3 hunks)go.mod(1 hunks)module_tunnels/koupleless_http_tunnel/http_tunnel.go(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- go.mod
- module_tunnels/koupleless_http_tunnel/http_tunnel.go
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/module-controller/main.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: unit-test
🔇 Additional comments (1)
common/utils/utils.go (1)
10-11: OK: zaplogger importImport is used below (FromContext/GetLogger). No action needed.
| return vkModel.NodeStatusData{ | ||
| Resources: resourceMap, | ||
| NodeState: vkModel.NodeState(strings.ToUpper(data.MasterBizInfo.BizState)), | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Harden NodeState derivation; avoid empty/unknown values leaking through
If BizState is empty, we emit an empty NodeState. Recommend a safe fallback and normalization. Also, if MasterBizInfo is a pointer in arkctl, add a nil‑check (verify type).
Apply:
- NodeState: vkModel.NodeState(strings.ToUpper(data.MasterBizInfo.BizState)),
+ NodeState: func() vkModel.NodeState {
+ v := strings.ToUpper(data.MasterBizInfo.BizState)
+ if v == "" {
+ return vkModel.NodeStateDeactivated
+ }
+ return vkModel.NodeState(v)
+ }(),To confirm the allowed values and whether Unknown exists in vkModel, and whether MasterBizInfo is a pointer in arkctl:
🌐 Web query:
What are the defined values for vkModel.NodeState in github.com/koupleless/virtual-kubelet/model, and does a NodeStateUnknown constant exist? Also, in github.com/koupleless/arkctl (v1/service/ark), is HealthData.MasterBizInfo a pointer type?
💡 Result:
-
vkModel.NodeState (github.com/koupleless/virtual-kubelet/model) has two defined values: NodeStateActivated = "ACTIVATED" and NodeStateDeactivated = "DEACTIVATED". There is no NodeStateUnknown constant. [1]
-
In github.com/koupleless/arkctl (v1/service/ark) HealthData.MasterBizInfo is a value field of type MasterBizInfo (not a pointer). [2]
Sources:
[1] pkg.go.dev: github.com/koupleless/virtual-kubelet/model (NodeState). (pkg.go.dev)
[2] pkg.go.dev: github.com/koupleless/arkctl/v1/service/ark (HealthData, MasterBizInfo). (pkg.go.dev)
Citations:
- 1: https://pkg.go.dev/github.com/koupleless/virtual-kubelet/model
- 2: https://pkg.go.dev/github.com/koupleless/arkctl/v1/service/ark?utm_source=openai
Harden NodeState derivation — default empty BizState to vkModel.NodeStateDeactivated (no nil-check needed)
vkModel only defines ACTIVATED/DEACTIVATED (no Unknown). HealthData.MasterBizInfo is a value (not a pointer), so check for empty BizState and fall back to NodeStateDeactivated.
File: common/utils/utils.go Lines: 114-117
- NodeState: vkModel.NodeState(strings.ToUpper(data.MasterBizInfo.BizState)),
+ NodeState: func() vkModel.NodeState {
+ v := strings.ToUpper(data.MasterBizInfo.BizState)
+ if v == "" {
+ return vkModel.NodeStateDeactivated
+ }
+ return vkModel.NodeState(v)
+ }(),📝 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.
| return vkModel.NodeStatusData{ | |
| Resources: resourceMap, | |
| NodeState: vkModel.NodeState(strings.ToUpper(data.MasterBizInfo.BizState)), | |
| } | |
| return vkModel.NodeStatusData{ | |
| Resources: resourceMap, | |
| NodeState: func() vkModel.NodeState { | |
| v := strings.ToUpper(data.MasterBizInfo.BizState) | |
| if v == "" { | |
| return vkModel.NodeStateDeactivated | |
| } | |
| return vkModel.NodeState(v) | |
| }(), | |
| } |
🤖 Prompt for AI Agents
In common/utils/utils.go around lines 114-117, the NodeState is derived directly
from data.MasterBizInfo.BizState which can be an empty string; change the logic
to check if BizState == "" and if so set NodeState to
vkModel.NodeStateDeactivated, otherwise set NodeState to
vkModel.NodeState(strings.ToUpper(data.MasterBizInfo.BizState)); keep the rest
of the returned vkModel.NodeStatusData unchanged.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #44 +/- ##
==========================================
+ Coverage 67.11% 67.13% +0.02%
==========================================
Files 12 12
Lines 1426 1427 +1
==========================================
+ Hits 957 958 +1
Misses 403 403
Partials 66 66
🚀 New features to boost your workflow:
|
2 participants
see koupleless/koupleless#423
Summary by CodeRabbit
New Features
Chores