Skip to content

fix: implement ThinEngine.CheckRuntimeReady to check actual runtime state #6062

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 4 commits into
base: master
Choose a base branch
from
Open

fix: implement ThinEngine.CheckRuntimeReady to check actual runtime state #6062

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
075c0ec
fix: implement ThinEngine.CheckRuntimeReady to check actual runtime s…
adityaupasani2 Jun 25, 2026
b79642c
fix: implement ThinEngine.CheckRuntimeReady to check actual runtime …
adityaupasani2 Jun 26, 2026
9448a59
test: fix CheckRuntimeReady fuse tests to match always-ready contract
adityaupasani2 Jun 30, 2026
92860f5
fix: remove duplicate logging in CheckRuntimeReady
adityaupasani2 Jul 1, 2026
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
159 changes: 159 additions & 0 deletions pkg/ddc/thin/health_check_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -605,4 +605,163 @@ var _ = Describe("ThinEngine Health Check", Label("pkg.ddc.thin.health_check_tes
Expect(count).To(BeEmpty())
})
})

@cheyang cheyang Jun 29, 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.

Once the failing tests are cleaned up, a small It() that builds a ThinRuntime with Spec.Worker.Enabled = false and asserts CheckRuntimeReady() returns true would document the intentional short-circuit in CheckWorkersReady and guard against future regressions.

Describe("CheckRuntimeReady", func() {
Context("when workers and fuse are both ready", func() {
It("returns true", func() {
healthyFuse := &appsv1.DaemonSet{
Comment on lines +609 to +612
ObjectMeta: metav1.ObjectMeta{
Name: healthCheckTestHbase + "-fuse",
Namespace: healthCheckTestNamespace,
},
Status: appsv1.DaemonSetStatus{
NumberUnavailable: 0,
NumberReady: 1,
NumberAvailable: 1,
},
}
healthyWorker := &appsv1.StatefulSet{
ObjectMeta: metav1.ObjectMeta{
Name: healthCheckTestHbase + "-worker",
Namespace: healthCheckTestNamespace,
},
Status: appsv1.StatefulSetStatus{
Replicas: 1,
ReadyReplicas: 1,
AvailableReplicas: 1,
},
}
runtimeObj := &datav1alpha1.ThinRuntime{
ObjectMeta: metav1.ObjectMeta{
Name: healthCheckTestHbase,
Namespace: healthCheckTestNamespace,
},
Spec: datav1alpha1.ThinRuntimeSpec{
Replicas: 1,
Worker: datav1alpha1.ThinCompTemplateSpec{
Enabled: true,
},
},
}
datasetObj := &datav1alpha1.Dataset{
ObjectMeta: metav1.ObjectMeta{
Name: healthCheckTestHbase,
Namespace: healthCheckTestNamespace,
},
}
c := fake.NewFakeClientWithScheme(testScheme, healthyFuse, healthyWorker, runtimeObj, datasetObj)
runtimeInfo, err := base.BuildRuntimeInfo(healthCheckTestHbase, healthCheckTestNamespace, common.ThinRuntime)
Expect(err).NotTo(HaveOccurred())
engine := ThinEngine{
Client: c,
Log: fake.NullLogger(),
namespace: healthCheckTestNamespace,
name: healthCheckTestHbase,
runtime: runtimeObj,
Helper: ctrl.BuildHelper(runtimeInfo, c, fake.NullLogger()),
}

ready := engine.CheckRuntimeReady()
Expect(ready).To(BeTrue())
})
})

Context("when workers are not ready", func() {
It("returns false", func() {
healthyFuse := &appsv1.DaemonSet{
ObjectMeta: metav1.ObjectMeta{
Name: healthCheckTestHbase + "-fuse",
Namespace: healthCheckTestNamespace,
},
Status: appsv1.DaemonSetStatus{
NumberUnavailable: 0,
NumberReady: 1,
NumberAvailable: 1,
},
}
notReadyWorker := &appsv1.StatefulSet{
ObjectMeta: metav1.ObjectMeta{
Name: healthCheckTestHbase + "-worker",
Namespace: healthCheckTestNamespace,
},
Spec: appsv1.StatefulSetSpec{
Replicas: ptr.To[int32](1),
},
Status: appsv1.StatefulSetStatus{
Replicas: 1,
ReadyReplicas: 0,
AvailableReplicas: 0,
},
}
runtimeObj := &datav1alpha1.ThinRuntime{
ObjectMeta: metav1.ObjectMeta{
Name: healthCheckTestHbase,
Namespace: healthCheckTestNamespace,
},
Spec: datav1alpha1.ThinRuntimeSpec{
Replicas: 1,
Worker: datav1alpha1.ThinCompTemplateSpec{
Enabled: true,
},
},
}
datasetObj := &datav1alpha1.Dataset{
ObjectMeta: metav1.ObjectMeta{
Name: healthCheckTestHbase,
Namespace: healthCheckTestNamespace,
},
}
c := fake.NewFakeClientWithScheme(testScheme, healthyFuse, notReadyWorker, runtimeObj, datasetObj)
runtimeInfo, err := base.BuildRuntimeInfo(healthCheckTestHbase, healthCheckTestNamespace, common.ThinRuntime)
Expect(err).NotTo(HaveOccurred())
engine := ThinEngine{
Client: c,
Log: fake.NullLogger(),
namespace: healthCheckTestNamespace,
name: healthCheckTestHbase,
runtime: runtimeObj,
Helper: ctrl.BuildHelper(runtimeInfo, c, fake.NullLogger()),
}

ready := engine.CheckRuntimeReady()
Expect(ready).To(BeFalse())
})
})

@cheyang cheyang Jun 29, 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 architectural fix in runtime_info.go is right and matches what fluid does elsewhere (pkg/ctrl/fuse.go hard-codes fuse ready=true). The problem is this test file: your PR comment says the two fuse tests were removed, but Context("when fuse is not ready") at line 730 and Context("when fuse DaemonSet does not exist") at line 772 are still present in the diff. Both build a ThinRuntime with Spec: datav1alpha1.ThinRuntimeSpec{} (no Worker.Enabled), so isWorkerEnable() returns false, CheckWorkersReady() short-circuits to (true, nil), and CheckRuntimeReady() returns true. The assertions expect false. That is exactly what the CI log shows: Expected <bool>: true to be false at health_check_test.go:770 and :802. Delete the two contexts, or flip them to Expect(ready).To(BeTrue()) and rename them to document the intentional 'fuse is always ready' behavior.

Context("when worker is disabled (fuse-only runtime)", func() {
It("returns true because fluid treats fuse as always-ready", func() {
// ThinRuntime with no worker enabled: isWorkerEnable() returns false,
// so CheckWorkersReady() short-circuits to (true, nil).
// Fuse is intentionally excluded from CheckRuntimeReady because
// fluid assumes fuse components are always ready (pkg/ctrl/fuse.go).
runtimeObj := &datav1alpha1.ThinRuntime{
Comment on lines +731 to +737
ObjectMeta: metav1.ObjectMeta{
Name: healthCheckTestSpark,
Namespace: healthCheckTestNamespace,
},
Spec: datav1alpha1.ThinRuntimeSpec{},
}
datasetObj := &datav1alpha1.Dataset{
ObjectMeta: metav1.ObjectMeta{
Name: healthCheckTestSpark,
Namespace: healthCheckTestNamespace,
},
}
c := fake.NewFakeClientWithScheme(testScheme, runtimeObj, datasetObj)
runtimeInfo, err := base.BuildRuntimeInfo(healthCheckTestSpark, healthCheckTestNamespace, common.ThinRuntime)
Expect(err).NotTo(HaveOccurred())
engine := ThinEngine{
Client: c,
Log: fake.NullLogger(),
namespace: healthCheckTestNamespace,
name: healthCheckTestSpark,
runtime: runtimeObj,
Helper: ctrl.BuildHelper(runtimeInfo, c, fake.NullLogger()),
}

ready := engine.CheckRuntimeReady()
Expect(ready).To(BeTrue())
})
})
})
})
11 changes: 9 additions & 2 deletions pkg/ddc/thin/runtime_info.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,16 @@ import (
"github.com/fluid-cloudnative/fluid/pkg/utils/testutil"
)

// CheckRuntimeReady checks if the ThinRuntime is ready to serve data operations.
// Unlike Alluxio/Jindo which probe a master pod, ThinRuntime has no master component,
// so 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 (t *ThinEngine) CheckRuntimeReady() (ready bool) {
Comment on lines +25 to 29

@cheyang cheyang Jul 2, 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.

ThinRuntime is typically fuse-only in production — Spec.Worker.Enabled defaults to false and most users never set it. In that common case, isWorkerEnable() returns false, CheckWorkersReady() short-circuits to (true, nil), and CheckRuntimeReady() here returns true unconditionally — before anything about the runtime has actually been verified.

That doesn't really match what callers of CheckRuntimeReady() expect. Downstream gates (DataLoad, DataMigrate) will proceed as if the runtime is fully set up, even when the fuse DaemonSet hasn't been created yet or the mount-profile ConfigMap isn't ready.

Two ways to close this:

  1. Keep the "fuse always ready" invariant, but still verify the fuse DaemonSet object exists / the mount-profile ConfigMap has been generated — instead of blindly returning true for the worker-less case.
  2. Probe the fuse DS the way Helper.CheckAndSyncFuseStatus does. That's closer to how Alluxio probes its master, and fits the "no master, no worker, only fuse" reality of ThinRuntime.

The doc comment "readiness is determined by worker availability" also frames worker-mode as the primary path — for ThinRuntime it's the exception. Worth being explicit that with Worker.Enabled=false (the common case), this currently returns true unconditionally.

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

@cheyang cheyang Jun 25, 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.

Alluxio and Jindo implement CheckRuntimeReady() by probing the master pod's responsiveness (fileUtils.Ready()). ThinRuntime has no master pod so the approach is necessarily different, but a brief comment explaining why worker+fuse status is used instead would help future readers.

if err != nil {
return false

@cheyang cheyang Jun 25, 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.

Consider adding a t.Log.Info("runtime not ready", ...) statement before returning false, matching the pattern used in AlluxioEngine and JindoEngine. This will help operators debug readiness issues.

@cheyang cheyang Jun 26, 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.

Consider adding t.Log.Info("runtime not ready", "workerReady", workerReady, "err", err) before returning false, matching the pattern used in AlluxioEngine and JindoEngine. This will help operators debug readiness issues.

}
return workerReady
}

// getRuntimeInfo gets runtime info
Expand Down
Loading