Skip to content

fix: implement VineyardEngine.CheckRuntimeReady to check worker read… #6106

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
adityaupasani2 wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix: implement VineyardEngine.CheckRuntimeReady to check worker read… #6106

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions pkg/ddc/vineyard/runtime_info.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,15 @@ import (
"github.com/fluid-cloudnative/fluid/pkg/utils/testutil"
)

// 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) {

@cheyang cheyang Jul 9, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

//TODO implement me
return true
workerReady, err := e.CheckWorkersReady()

@cheyang cheyang Jul 9, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@cheyang cheyang Jul 9, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

// getRuntimeInfo gets runtime info
Expand Down
86 changes: 86 additions & 0 deletions pkg/ddc/vineyard/runtime_info_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,14 @@ import (

"github.com/fluid-cloudnative/fluid/api/v1alpha1"
"github.com/fluid-cloudnative/fluid/pkg/common"
ctrlhelper "github.com/fluid-cloudnative/fluid/pkg/ctrl"
"github.com/fluid-cloudnative/fluid/pkg/ddc/base"
"github.com/fluid-cloudnative/fluid/pkg/utils/fake"
v1 "k8s.io/api/apps/v1"
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/utils/ptr"
"sigs.k8s.io/controller-runtime/pkg/client"
)

Expand Down Expand Up @@ -181,3 +183,87 @@ func TestGetRuntimeInfo(t *testing.T) {
}
}
}

func TestVineyardEngineCheckRuntimeReady(t *testing.T) {
testcases := []struct {
name string
workerSS *v1.StatefulSet
runtime *v1alpha1.VineyardRuntime
expectedReady bool
}{
{
name: "workers ready returns true",
workerSS: &v1.StatefulSet{
ObjectMeta: metav1.ObjectMeta{
Name: "hbase-worker",
Namespace: "fluid",
},
Spec: v1.StatefulSetSpec{
Replicas: ptr.To[int32](1),
},
Status: v1.StatefulSetStatus{
Replicas: 1,
ReadyReplicas: 1,
AvailableReplicas: 1,
},
},
runtime: &v1alpha1.VineyardRuntime{
ObjectMeta: metav1.ObjectMeta{Name: "hbase", Namespace: "fluid"},
Spec: v1alpha1.VineyardRuntimeSpec{
Worker: v1alpha1.VineyardCompTemplateSpec{Replicas: 1},
},
},
expectedReady: true,
},
{
name: "workers not ready returns false",
workerSS: &v1.StatefulSet{
ObjectMeta: metav1.ObjectMeta{
Name: "hbase-worker",
Namespace: "fluid",
},
Spec: v1.StatefulSetSpec{
Replicas: ptr.To[int32](1),
},
Status: v1.StatefulSetStatus{
Replicas: 1,
ReadyReplicas: 0,
AvailableReplicas: 0,
},
},
runtime: &v1alpha1.VineyardRuntime{
ObjectMeta: metav1.ObjectMeta{Name: "hbase", Namespace: "fluid"},
Spec: v1alpha1.VineyardRuntimeSpec{
Worker: v1alpha1.VineyardCompTemplateSpec{Replicas: 1},
},
},
expectedReady: false,
},
}

@cheyang cheyang Jul 9, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


for _, tc := range testcases {
t.Run(tc.name, func(t *testing.T) {
dataset := &v1alpha1.Dataset{
ObjectMeta: metav1.ObjectMeta{Name: "hbase", Namespace: "fluid"},
}
objs := []runtime.Object{tc.workerSS, tc.runtime, dataset}
fakeClient := fake.NewFakeClientWithScheme(testScheme, objs...)
runtimeInfo, err := base.BuildRuntimeInfo("hbase", "fluid", common.VineyardRuntime)
if err != nil {
t.Fatalf("failed to build runtime info: %v", err)
}
engine := &VineyardEngine{
Client: fakeClient,
Log: fake.NullLogger(),
namespace: "fluid",
name: "hbase",
runtime: tc.runtime,
Helper: ctrlhelper.BuildHelper(runtimeInfo, fakeClient, fake.NullLogger()),
}
ready := engine.CheckRuntimeReady()
if ready != tc.expectedReady {
t.Errorf("expected ready=%v, got ready=%v", tc.expectedReady, ready)
}
})
}
}
Loading