Feat/enterprise readiness #2753
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…g/observability: metrics hooks and KEDA/OTel/Prometheus helm templates; multi-cluster: Karmada CPP; volcano: job template; docs and samples; unit tests for tenancy
… adapter; feat(ai-proxy): batch trigger metric; tests: add basic observability and tenancy tests; ci: add GitHub Actions unit test workflow
jpjamespeterson
requested review from
Xunzhuo,
2456868764,
CH3CHO,
cr7258,
rinfx,
wydream,
SpecialYang and
johnlanni
as code owners
|
|
企业级功能增强：多集群、自动扩缩容、可观测性与多租户支持变更概述
变更文件
时序图sequenceDiagram
participant HC as Helm Chart
participant KS as KedaScaler
participant VS as VolcanoScheduler
participant KM as KarmadaSync
participant TM as TenantManager
participant OM as ObservabilityMetrics
participant OT as ObservabilityTracing
HC->>KS: Enable KEDA via Helm values
KS->>KS: Build with -tags keda
KS->>KS: Create/Update ScaledObject
HC->>VS: Enable Volcano via Helm values
VS->>VS: Build with -tags volcano
VS->>VS: Schedule Batch Job
HC->>KM: Enable Karmada via Helm values
KM->>KM: Build with -tags karmada
KM->>KM: Sync ConfigMap/CRD
HC->>TM: Set tenantNamespaces via Helm
TM->>TM: Validate and check namespace access
HC->>OM: Enable Prometheus via Helm
OM->>OM: Record token usage and latency
HC->>OT: Enable OTel Collector via Helm
OT->>OT: Setup tracing with OTLP exporter
💡 小贴士与 lingma-agents 交流的方式📜 直接回复评论
📜 在代码行处标记
📜 在讨论中提问
|
![lingma-agents[bot]](https://avatars.githubusercontent.com/in/1198481?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔎 代码评审报告
🎯 评审意见概览
| 严重度 | 数量 | 说明 |
|---|---|---|
| 🔴 Blocker | 0 | 阻断性问题，需立即修复。例如：系统崩溃、关键功能不可用或严重安全漏洞。 |
| 🟠 Critical | 0 | 严重问题，高优先级修复。例如：核心功能异常或性能瓶颈影响用户体验。 |
| 🟡 Major | 2 | 主要问题，建议修复。例如：非核心功能缺陷或代码维护性较差。 |
| 🟢 Minor | 0 | 次要问题，酬情优化。例如：代码格式不规范或注释缺失。 |
总计: 2 个问题
📋 评审意见详情
💡 代码实现建议
以下是文件级别的代码建议，聚焦于代码的可读性、可维护性和潜在问题。
🔹 pkg/autoscaler/keda_scaler.go (1 💬)
- KEDA自动伸缩器应允许配置Prometheus服务器地址，而不是硬编码。 (L43-L47)
🔹 pkg/karmada/sync.go (1 💬)
- Karmada同步器应允许配置集群亲和性，而不是硬编码为所有集群。 (L47-L49)
🚀 架构设计建议
以下是对代码架构和设计的综合分析，聚焦于跨文件交互、系统一致性和潜在优化空间。
🔍1. 多租户隔离逻辑不完整，存在越权访问风险
pkg/ingress/kube/gateway/istio/controller.go 中实现了基于命名空间的租户隔离逻辑，但该逻辑仅在 List 和 Reconcile 方法中部分应用。例如，在 List 方法中对 Gateway 和 VirtualService 进行了过滤，但未对其他资源类型（如 HTTPRoute、TCPRoute 等）进行相同处理。此外，Reconcile 方法中的过滤逻辑虽然覆盖了多种资源，但未与 List 方法保持一致，可能导致数据泄露或越权访问。建议统一租户隔离策略，并确保所有相关资源类型都受到保护。
📌 关键代码
// Tenant namespace filtering
allowed := []string{}
if c.client != nil && c.client.RESTConfig() != nil { // placeholder: plumb allowed list from options or env
// For now, allow the requested namespace only when specified
if namespace != "" {
allowed = []string{namespace}
}
}
tenantMgr := &tenancy.TenantManager{}
switch typ {
case gvk.Gateway:
items := filterNamespace(c.state.Gateway, namespace)
if len(allowed) == 0 {
return items
}
var res []config.Config
for _, it := range items {
if tenantMgr.AllowedNamespace(it.Namespace, allowed) {
res = append(res, it)
}
}
return res
case gvk.VirtualService:
items := filterNamespace(c.state.VirtualService, namespace)
if len(allowed) == 0 {
return items
}
var res []config.Config
for _, it := range items {
if tenantMgr.AllowedNamespace(it.Namespace, allowed) {
res = append(res, it)
}
}
return res
default:
return nil
}// Apply namespace filtering on inputs if allowed namespaces defined
if len(allowed) > 0 {
filter := func(in []config.Config) []config.Config {
var out []config.Config
for _, it := range in {
if tenantMgr.AllowedNamespace(it.Namespace, allowed) {
out = append(out, it)
}
}
return out
}
gateway = filter(gateway)
httpRoute = filter(httpRoute)
tcpRoute = filter(tcpRoute)
tlsRoute = filter(tlsRoute)
}可能导致不同租户间的数据泄露或越权访问，影响系统的安全性和合规性。
🔍2. 构建标签依赖管理不当，可能引发编译或运行时错误
项目引入了多个可选功能模块（如 KEDA、Karmada、Volcano），并通过 Go 构建标签（build tags）控制其启用状态。然而，pkg/autoscaler/keda_scaler.go、pkg/karmada/sync.go 和 pkg/volcano/scheduler.go 等文件直接依赖于这些外部库，而没有适当的抽象层或接口隔离。这可能导致在不启用相应构建标签的情况下编译失败，或者在运行时因缺少依赖而导致程序崩溃。建议通过定义通用接口并为每个模块提供实现的方式来解耦依赖关系。
📌 关键代码
//go:build keda
package autoscaler
import (
context "context"
"fmt"
kedav1alpha1 "github.com/kedacore/keda/v2/apis/keda/v1alpha1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"sigs.k8s.io/controller-runtime/pkg/client"
)//go:build karmada
package karmada
import (
context "context"
policyv1alpha1 "github.com/karmada-io/api/policy/v1alpha1"
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"sigs.k8s.io/controller-runtime/pkg/client"
)//go:build volcano
package volcano
import (
context "context"
batchv1alpha1 "volcano.sh/apis/pkg/apis/batch/v1alpha1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"sigs.k8s.io/controller-runtime/pkg/client"
)可能导致编译失败或运行时错误，增加系统维护成本和部署复杂度。
🔍3. 监控指标命名不一致，影响可观测性
在 pkg/observability/metrics.go 中定义的 Prometheus 指标名称与 plugins/wasm-go/extensions/ai-proxy/metrics.go 中定义的指标名称存在不一致。例如，pkg/observability/metrics.go 使用 'higress_ai_token_usage_total' 和 'higress_ai_model_latency_seconds'，而 plugins/wasm-go/extensions/ai-proxy/metrics.go 使用 'higress_ai_token_usage_total' 和 'higress_ai_model_latency_ms_p50' 等。这种不一致性会使得监控和告警规则难以统一管理，降低系统的可观测性。建议统一指标命名规范，并确保所有模块使用相同的命名方式。
📌 关键代码
TokenUsage: prometheus.NewCounter(prometheus.CounterOpts{
Namespace: "higress",
Subsystem: "ai",
Name: "token_usage_total",
Help: "Total number of LLM tokens processed by Higress.",
}),
ModelLatency: prometheus.NewHistogram(prometheus.HistogramOpts{
Namespace: "higress",
Subsystem: "ai",
Name: "model_latency_seconds",
Help: "Latency of LLM requests handled by Higress.",
Buckets: prometheus.DefBuckets,
}),const (
metricTokenUsage = "higress_ai_token_usage_total"
metricLatencyMilliP50 = "higress_ai_model_latency_ms_p50"
metricLatencyMilliP95 = "higress_ai_model_latency_ms_p95"
metricLatencyMilliP999 = "higress_ai_model_latency_ms_p999"
ctxStartTimeKey = "ai_proxy_start_time"
)导致监控和告警规则混乱，影响问题排查效率和系统稳定性评估。
审查详情
📒 文件清单 （32 个文件）
✅ 新增: 26 个文件
📝 变更: 6 个文件
✅ 新增文件:
.github/workflows/test.ymldocs/autoscaling.mddocs/multicluster.mddocs/observability.mddocs/tenancy.mddocs/volcano.mdhelm/core/templates/karmada-cpp.yamlhelm/core/templates/keda-scaledobject.yamlhelm/core/templates/otel-collector.yamlhelm/core/templates/prometheus.yamlhelm/core/templates/volcano-job.yamlpkg/autoscaler/keda_scaler.gopkg/autoscaler/keda_stub.gopkg/gateway/controller.gopkg/karmada/sync.gopkg/karmada/sync_stub.gopkg/observability/metrics.gopkg/observability/tracing.gopkg/tenancy/manager.gopkg/volcano/scheduler.gopkg/volcano/scheduler_stub.goplugins/wasm-go/extensions/ai-proxy/metrics.gosamples/keda/scaledobject.yamltest/karmada/sample_cpp.yamltest/observability/metrics_test.gotest/tenancy/manager_test.go
📝 变更文件:
README.mdgo.modhelm/core/templates/controller-deployment.yamlhelm/core/values.yamlpkg/ingress/kube/gateway/istio/controller.goplugins/wasm-go/extensions/ai-proxy/main.go
💡 小贴士
与 lingma-agents 交流的方式
📜 直接回复评论
直接回复本条评论，lingma-agents 将自动处理您的请求。例如：
-
在当前代码中添加详细的注释说明。
-
请详细介绍一下你说的 LRU 改造方案，并使用伪代码加以说明。
📜 在代码行处标记
在文件的特定位置创建评论并 @lingma-agents。例如：
-
@lingma-agents 分析这个方法的性能瓶颈并提供优化建议。
-
@lingma-agents 对这个方法生成优化代码。
📜 在讨论中提问
在任何讨论中 @lingma-agents 来获取帮助。例如：
-
@lingma-agents 请总结上述讨论并提出解决方案。
-
@lingma-agents 请根据讨论内容生成优化代码。
Comment on lines
+43
to
+47
| Metadata: map[string]string{ | ||
| "serverAddress": "http://prometheus-server.monitoring.svc.cluster.local", | ||
| "metricName": metricName, | ||
| "threshold": fmt.Sprintf("%d", targetValue), | ||
| }, |
There was a problem hiding this comment.
KEDA自动伸缩器应允许配置Prometheus服务器地址，而不是硬编码。
🟡 Major | 🧹 Code Smells
📋 问题详情
当前代码在ScaleByLLMMetrics方法中硬编码了Prometheus服务器地址，这使得配置不够灵活。应该允许通过参数或环境变量来配置服务器地址。
💡 解决方案
应该允许通过参数或环境变量来配置Prometheus服务器地址，而不是硬编码。这样可以提高配置的灵活性。
- Metadata: map[string]string{
- "serverAddress": "http://prometheus-server.monitoring.svc.cluster.local",
- "metricName": metricName,
- "threshold": fmt.Sprintf("%d", targetValue),
- },
+ Metadata: map[string]string{
+ "serverAddress": os.Getenv("PROMETHEUS_SERVER_ADDRESS"),
+ "metricName": metricName,
+ "threshold": fmt.Sprintf("%d", targetValue),
+ },您的反馈对我们很重要！(建议右键在新标签页中打开以下链接)
Comment on lines
+47
to
+49
| Placement: &policyv1alpha1.Placement{ // all clusters by default | ||
| ClusterAffinity: &policyv1alpha1.ClusterAffinity{}, | ||
| }, |
There was a problem hiding this comment.
Karmada同步器应允许配置集群亲和性，而不是硬编码为所有集群。
🟡 Major | 🧹 Code Smells
📋 问题详情
当前代码在SyncConfigMap和SyncCRD方法中硬编码了集群亲和性为所有集群，这可能不符合实际需求。应该允许通过参数或配置来指定集群亲和性。
💡 解决方案
应该允许通过参数或配置来指定集群亲和性，而不是硬编码为所有集群。这样可以提高配置的灵活性。
- Placement: &policyv1alpha1.Placement{ // all clusters by default
- ClusterAffinity: &policyv1alpha1.ClusterAffinity{},
- },
+ Placement: &policyv1alpha1.Placement{
+ ClusterAffinity: &policyv1alpha1.ClusterAffinity{
+ ClusterNames: []string{os.Getenv("KARMADA_CLUSTER_NAMES")},
+ },
+ },您的反馈对我们很重要！(建议右键在新标签页中打开以下链接)
|
@jpjamespeterson Could you provide some background information on this feature? Also, please sign the CLA |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Ⅰ. Describe what this PR did
Ⅱ. Does this pull request fix one issue?
Ⅲ. Why don't you add test cases (unit test/integration test)?
Ⅳ. Describe how to verify it
Ⅴ. Special notes for reviews