fix: implement VineyardEngine.CheckRuntimeReady to check worker read…#6106
fix: implement VineyardEngine.CheckRuntimeReady to check worker read…#6106adityaupasani2 wants to merge 1 commit into
Conversation
…iness CheckRuntimeReady() was an unimplemented stub that unconditionally returned true, causing data operations to be dispatched against a VineyardRuntime even when workers were not ready. Implement it using CheckWorkersReady() — the same helper used by CheckRuntimeHealthy(). Fuse readiness is intentionally excluded because fluid treats fuse as always-ready by design (see pkg/ctrl/fuse.go). Also adds tests covering workers ready (returns true) and workers not ready (returns false). Fixes fluid-cloudnative#6105 Signed-off-by: Aditya Upasani <adityaupasani29@gmail.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Hi @adityaupasani2. Thanks for your PR. I'm waiting for a fluid-cloudnative member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements the CheckRuntimeReady method for the VineyardEngine in pkg/ddc/vineyard/runtime_info.go, which now checks if the vineyard workers are ready instead of always returning true. It also adds corresponding unit tests in pkg/ddc/vineyard/runtime_info_test.go to verify this behavior under different worker readiness states. There are no review comments, and I have no feedback to provide.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6106 +/- ##
=======================================
Coverage 64.76% 64.76%
=======================================
Files 485 485
Lines 33895 33898 +3
=======================================
+ Hits 21952 21955 +3
+ Misses 10220 10219 -1
- Partials 1723 1724 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
/copilot review |
| func (e *VineyardEngine) CheckRuntimeReady() (ready bool) { | ||
| //TODO implement me | ||
| return true | ||
| workerReady, err := e.CheckWorkersReady() |
There was a problem hiding this comment.
Delegating to CheckWorkersReady means every call to CheckRuntimeReady now goes through Helper.CheckAndSyncWorkerStatus, which patches the VineyardRuntime status (worker phase, conditions, counters) whenever it changes. Every peer engine's CheckRuntimeReady is side-effect-free today: Alluxio/Jindo use fileUtils.Ready() (read-only exec), JuiceFS uses GetRunningPodsOfStatefulSet (pure list), EFC combines both without touching status. This method is called from SetDataOperationInTargetDataset before every DataLoad/DataMigrate/etc., so under load it turns a preflight check into a write path with retry-on-conflict. Is that intentional? If the goal is only readiness, consider a read-only variant (e.g. GetRunningPodsOfStatefulSet like JuiceFS) so the data-op preflight stays cheap and side-effect-free.
| if err != nil { | ||
| return false | ||
| } | ||
| return workerReady |
There was a problem hiding this comment.
Worth calling out: CheckWorkersReady returns true for both RuntimePhaseReady and RuntimePhasePartialReady (see pkg/ctrl/worker.go:143-151). So CheckRuntimeReady will report ready even when only a subset of Vineyard workers are up. That is consistent with how JuiceFS and CheckRuntimeHealthy treat worker readiness, but it means the docstring above is slightly optimistic ('worker availability' -> 'at least one worker replica available'). Consider tightening the comment so future readers do not assume all workers are up.
| // CheckRuntimeReady checks if the VineyardRuntime is ready to serve data operations. | ||
| // Readiness is determined by worker availability. Fuse components are intentionally | ||
| // excluded because fluid treats fuse as always-ready by design (see pkg/ctrl/fuse.go). | ||
| func (e *VineyardEngine) CheckRuntimeReady() (ready bool) { |
There was a problem hiding this comment.
Vineyard's runtime has a real master component (etcd, see master.go/CheckMasterReady) that stores object metadata, and CheckRuntimeHealthy in this same package explicitly checks master + worker + fuse in that order. Peer engines diverge on this: EFC checks master and worker; JuiceFS checks worker only; Alluxio/Jindo check master only. What's the reasoning for worker-only here? If the invariant 'workers cannot become ready until master is ready' holds in the setup path, please note that in the docstring; otherwise consider following EFC's pattern (master first, then workers) to avoid data operations racing an unhealthy master.
| }, | ||
| expectedReady: false, | ||
| }, | ||
| } |
There was a problem hiding this comment.
The two test cases cover ready=true and ready=false, but the if err != nil { return false } branch on line 27 of runtime_info.go is untested. A simple third case where the worker StatefulSet is absent from the fake client (or where BuildRuntimeInfo yields a helper that surfaces an error) would close the coverage gap and lock in the intended fail-closed behavior.
2 participants
Ⅰ. Describe what this PR does
VineyardEngine.CheckRuntimeReady()inpkg/ddc/vineyard/runtime_info.gowas an unimplemented stub that unconditionally returnedtrue:This caused data operations (DataLoad, DataMigrate, DataProcess) to be dispatched against a VineyardRuntime even when workers were not actually ready.
Implements it using
CheckWorkersReady()— the same helper already used byCheckRuntimeHealthy(). Fuse readiness is intentionally excluded because fluid treats fuse as always-ready by design (pkg/ctrl/fuse.go: "fluid assumes fuse components are always ready") —checkFuseHealthy()delegates toCheckAndSyncFuseStatuswhich unconditionally returnsready = true. This matches the approach taken for ThinEngine in #6062.Ⅱ. Does this pull request fix one issue?
Fixes #6105
Ⅲ. List the added test cases
Added
TestVineyardEngineCheckRuntimeReadyinruntime_info_test.gowith two cases:truefalseⅣ. Describe how to verify it
go test ./pkg/ddc/vineyard/... -run TestVineyardEngineCheckRuntimeReady -vⅤ. Special notes for reviews
Same pattern and rationale as #6062 (ThinEngine fix). The only difference is
VineyardEngineusesVineyardRuntimeinstead ofThinRuntime.