feat(tikv): add worker ref#6624
Conversation
Signed-off-by: liubo02 <liubo02@pingcap.com>
[LGTM Timeline notifier]Timeline:
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #6624 +/- ##
==========================================
- Coverage 39.53% 39.42% -0.12%
==========================================
Files 351 351
Lines 20659 20722 +63
==========================================
+ Hits 8167 8169 +2
- Misses 12492 12553 +61
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR adds support for referencing remote workers in TiKV configurations, allowing TiKV instances to delegate work to separate worker groups for coprocessor, compactor, and worker operations. This is a feature specifically designed for the "nextgen" architecture.
Key Changes
- Added new API types for worker references (CoprocessorReference, CompactorReference, WorkerReference) to the TiKV spec
- Implemented TiKV configuration generation to connect to remote worker services via internal ClusterIP services
- Created an internal service for TiKVWorkerGroup to enable service-based communication (in addition to the existing headless service)
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| api/core/v1alpha1/tikv_types.go | Defines RemoteWorkers field and reference types for TiKV template spec |
| api/core/v1alpha1/zz_generated.deepcopy.go | Auto-generated deepcopy methods for new worker reference types |
| manifests/crd/core.pingcap.com_tikvs.yaml | CRD schema additions for remoteWorkers field in TiKV spec |
| manifests/crd/core.pingcap.com_tikvgroups.yaml | CRD schema additions for remoteWorkers field in TiKVGroup spec |
| pkg/configs/tikv/config.go | Configures TiKV to connect to remote worker URLs based on references |
| pkg/apiutil/core/v1alpha1/tikv_worker.go | Helper functions to generate worker service URLs with appropriate paths |
| pkg/apiutil/core/v1alpha1/group.go | Added InternalServiceName helper for generating worker service names |
| pkg/controllers/tikvworkergroup/tasks/svc.go | Creates both headless and internal services for TiKV worker groups |
| tests/e2e/framework/desc/options.go | Added EnableTiKVWorkers option for E2E tests |
| tests/e2e/framework/desc/patch.go | Applies worker configuration patches when option is enabled |
| tests/e2e/data/tikv.go | Test helper to configure all three worker references for TiKV |
| tests/e2e/suite/cluster/tls.go | Enables worker configuration in TLS test scenarios |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
api/core/v1alpha1/tikv_types.go
Outdated
| PreStop *TiKVPreStop `json:"preStop,omitempty"` | ||
|
|
||
| // RemoteWorkers defines remote workers used by this tikv | ||
| // It's only worked for nextgen |
There was a problem hiding this comment.
Grammatical error in the comment. It should be "It only works for nextgen" instead of "It's only worked for nextgen".
| // It's only worked for nextgen | |
| // It only works for nextgen |
| } | ||
| name := tikv.Spec.RemoteWorkers.Coprocessor.Name | ||
| c.KVEngine.RemoteCoprocessorAddr = coreutil.TiKVWorkerCoprocessorURL(name, tikv.Namespace, tls) | ||
| c.KVEngine.RemoteWorkerAddr = coreutil.TiKVWorkerCoprocessorURL(name, tikv.Namespace, tls) |
There was a problem hiding this comment.
The RemoteWorkerAddr is incorrectly set to TiKVWorkerCoprocessorURL which includes a "/coprocessor" path suffix. When both Coprocessor and Worker references point to the same worker group, RemoteWorkerAddr should either use a different URL path or the base URL without the "/coprocessor" suffix. This assignment appears redundant with line 108 and may cause the worker configuration to be incorrect.
| c.KVEngine.RemoteWorkerAddr = coreutil.TiKVWorkerCoprocessorURL(name, tikv.Namespace, tls) |
| c.KVEngine = &KVEngine{} | ||
| } | ||
| name := tikv.Spec.RemoteWorkers.Worker.Name | ||
| c.KVEngine.RemoteWorkerAddr = coreutil.TiKVWorkerCoprocessorURL(name, tikv.Namespace, tls) |
There was a problem hiding this comment.
The RemoteWorkerAddr is set using TiKVWorkerCoprocessorURL which includes a "/coprocessor" path suffix. This appears incorrect for a Worker reference configuration. The function should either be renamed or a separate TiKVWorkerURL function should be created to provide the appropriate endpoint for worker configuration.
| c.KVEngine.RemoteWorkerAddr = coreutil.TiKVWorkerCoprocessorURL(name, tikv.Namespace, tls) | |
| c.KVEngine.RemoteWorkerAddr = coreutil.TiKVWorkerURL(name, tikv.Namespace, tls) |
| Ports: []corev1.ServicePort{ | ||
| { | ||
| Name: v1alpha1.TiKVWorkerPortNameAPI, | ||
| Port: v1alpha1.DefaultTiKVWorkerPortAPI, |
There was a problem hiding this comment.
The internal service uses a hardcoded default port instead of respecting custom port configuration. This is inconsistent with other components like PDGroup and TiDBGroup, which use dynamic port helper functions. The port should be retrieved using coreutil.TiKVWorkerGroupAPIPort(wg) to respect custom port configurations if specified.
| Port: v1alpha1.DefaultTiKVWorkerPortAPI, | |
| Port: coreutil.TiKVWorkerGroupAPIPort(wg), |
| return task.Fail().With(fmt.Sprintf("can't create internal service of tikv worker: %v", err)) | ||
| } | ||
|
|
||
| return task.Complete().With("services of tikv worker has been applied") |
There was a problem hiding this comment.
The success message has a grammatical error. It should be "services of tikv worker have been applied" (plural "have" to agree with plural "services").
| return task.Complete().With("services of tikv worker has been applied") | |
| return task.Complete().With("services of tikv worker have been applied") |
Signed-off-by: liubo02 <liubo02@pingcap.com>
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: fgksgf, srstack The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/test pull-e2e |
Support ref worker in tikv