Skip to content

fix(jindo): fall back to dataset mounts when DataLoad has no target #6156

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
pujitha24 wants to merge 2 commits into
base: master
Choose a base branch
from
Open

fix(jindo): fall back to dataset mounts when DataLoad has no target #6156

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f2e5f7b
fix(jindo): fall back to dataset mounts when DataLoad has no target
pujitha24 Aug 4, 2026
153ba4b
revert(jindo): back out no-target mount fallback per review
pujitha24 Aug 11, 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
28 changes: 21 additions & 7 deletions pkg/ddc/jindo/load_data.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,13 +160,27 @@ func (e *JindoEngine) genDataLoadValue(image string, runtime *datav1alpha1.Jindo
}

targetPaths := []cdataload.TargetPath{}
for _, target := range dataload.Spec.Target {
fluidNative := utils.IsTargetPathUnderFluidNativeMounts(target.Path, *targetDataset)
targetPaths = append(targetPaths, cdataload.TargetPath{
Path: target.Path,
Replicas: target.Replicas,
FluidNative: fluidNative,
})
if len(dataload.Spec.Target) > 0 {
for _, target := range dataload.Spec.Target {
fluidNative := utils.IsTargetPathUnderFluidNativeMounts(target.Path, *targetDataset)
targetPaths = append(targetPaths, cdataload.TargetPath{
Path: target.Path,
Replicas: target.Replicas,
FluidNative: fluidNative,
})
}
} else {

@cheyang cheyang Aug 11, 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.

fixes #4439 doesn't hold with this scope, so the issue would stay broken after merge.

That report is about a JindoRuntime, and GetDefaultEngineImpl() (pkg/utils/jindo/jindo.go:37) returns jindocache unless the operator runs with JINDO_ENGINE_TYPE=jindo or jindofsx. pkg/ddc/jindo is the legacy JindoFS engine on smartdata:3.8.0, so a default install never executes this branch. jindocache and jindofsx carry the same empty-target shape in their own load_data.go.

I understand wanting to keep the diff minimal and scoped, and normally I'd agree. Here the scope is what makes the change unable to affect the environment in the report.

Either retarget the change to jindocache, or drop fixes #4439 from the description so the issue doesn't get auto-closed by a change that cannot affect it.

// No explicit target is specified, fall back to loading all mount points of the dataset,
// otherwise the generated targetPaths would be empty and the dataload would be a no-op.

@cheyang cheyang Aug 11, 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.

This premise doesn't hold for this engine, so the change is solving a problem that isn't present here.

DataLoadInfo.TargetPaths is declared json:"targetPaths,omitempty" (pkg/dataload/value.go:50), so an empty slice is dropped from the generated values file rather than written as an empty list. helm install -f <values> <chart> then falls back to the chart's own default, and charts/fluid-dataloader/jindo/values.yaml:41 documents that default as (path: "/", replicas: 1, fluidNative: false).

I rendered the real chart both ways to check:

master (no targetPaths key)  ->  DATA_PATH="/"            PATH_REPLICAS="1"
this PR (per-mount paths)    ->  DATA_PATH="/mnt0:/mnt1"  PATH_REPLICAS="1:1"

So a no-target Jindo DataLoad already loads the whole dataset here, and the generated targetPaths is never the empty list this comment assumes.

#4439 describes something different, though: a DataLoad pod that failed, not one that loaded nothing. I think the chart default is still the right thing to suspect there, but on jindocache rather than on this engine. See the review body for that reasoning.

for _, mount := range targetDataset.Spec.Mounts {
path := utils.UFSPathBuilder{}.GenUFSPathInUnifiedNamespace(mount)

@cheyang cheyang Aug 11, 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 paths this derives aren't addressable in the JindoFS namespace, so the change turns a working DataLoad into a failing one.

GenUFSPathInUnifiedNamespace encodes Alluxio's convention, where each mount really does get its own /{name} subtree in the unified namespace. The Jindo engine has no equivalent mapping. transform.go:202 hardcodes jfsNamespace := "jindo", every mount then overwrites the same jfs.namespaces.jindo.<mode>.uri key (lines 227, 230, 245), and the per-mount accumulation is commented out at line 220. jfs://jindo/ is therefore rooted directly at one UFS URI, and with several mounts only the last one survives.

The generated job runs the script from charts/fluid-dataloader/jindo/templates/configmap.yaml, whose checkPathExistence exits 1 on a missing path. Running that script verbatim against a stub CLI, with the namespace root holding the bucket's real contents:

DATA_PATH='/'            exit=0  loads=1   jindo jfs -load ... jfs://jindo/
DATA_PATH='/mnt0:/mnt1'  exit=1  loads=0   dataLoad failed because some paths not exist.
DATA_PATH='/spark:/hive' exit=1  loads=0   dataLoad failed because some paths not exist.

The one shape where this is harmless is a single mount with path: "/", which is also the shape where it changes nothing.

If you want an explicit default here instead of relying on the chart, a single {Path: "/", Replicas: 1} matches both what the chart documents and what JindoFS can address.

The per-mount idea itself is more defensible in jindocache, which builds one cacheset per mount rather than a single namespace. If #4439 is what you're targeting, that is where this change belongs.

fluidNative := utils.IsTargetPathUnderFluidNativeMounts(path, *targetDataset)
targetPaths = append(targetPaths, cdataload.TargetPath{
Path: path,
Replicas: 1,

@cheyang cheyang Aug 11, 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.

nit: Replicas: 1 is redundant. The chart already applies default 1 .replicas when it builds PATH_REPLICAS (charts/fluid-dataloader/jindo/templates/job.yaml:83). Harmless, just noting it.

FluidNative: fluidNative,
})
}
}
dataloadInfo.TargetPaths = targetPaths
options := map[string]string{}
Expand Down
83 changes: 83 additions & 0 deletions pkg/ddc/jindo/load_data_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -546,6 +546,89 @@ func Test_genDataLoadValue(t *testing.T) {
},
},
},
"test case with multiple mounts and no explicit target": {
image: "fluid:v0.0.1",
targetDataset: &datav1alpha1.Dataset{
ObjectMeta: metav1.ObjectMeta{
Name: "test-dataset",
Namespace: "fluid",
},
Spec: datav1alpha1.DatasetSpec{
Mounts: []datav1alpha1.Mount{
{
Name: "spark",
MountPoint: "local://mnt/data0",
Path: "/mnt0",

@cheyang cheyang Aug 11, 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.

Both fixtures set an absolute path, so this case never reaches the behavior the PR description describes. GenUFSPathInUnifiedNamespace returns early via filepath.IsAbs(mount.Path), which leaves the /{mount.Name} fallback uncovered.

I checked by mutation: replacing utils.UFSPathBuilder{}.GenUFSPathInUnifiedNamespace(mount) with a plain mount.Path still leaves Test_genDataLoadValue green. Codecov reports these lines as covered, which is true at the line level but doesn't pin the behavior.

The local:// mount points are a second problem. transform.go only handles hdfs://, s3:// and oss://, and continues past anything else, so a local://-only Dataset ends up with no namespace URI at all and this fixture describes a Dataset the Jindo engine can't serve. An oss:// or hdfs:// mount point, plus one mount with no path, would exercise the real code.

A case that asserts the rendered DATA_PATH would also have caught the issue above, since the struct-level assertion can't see that the paths are unreachable.

},
{
Name: "hive",
MountPoint: "local://mnt/data1",
Path: "/mnt1",
},
},
},
},
dataload: &datav1alpha1.DataLoad{
ObjectMeta: metav1.ObjectMeta{
Name: "test-dataload",
Namespace: "fluid",
},
Spec: datav1alpha1.DataLoadSpec{
Dataset: datav1alpha1.TargetDataset{
Name: "test-dataset",
Namespace: "fluid",
},
SchedulerName: "scheduler-test",
},
},
runtime: &datav1alpha1.JindoRuntime{
Spec: datav1alpha1.JindoRuntimeSpec{
TieredStore: datav1alpha1.TieredStore{
Levels: []datav1alpha1.Level{
{
MediumType: "MEM",
},
},
},
HadoopConfig: "principal=root",
},
},
want: &cdataload.DataLoadValue{
Name: "test-dataload",
OwnerDatasetId: "fluid-test-dataset",
Owner: &common.OwnerReference{
Kind: "DataLoad",
APIVersion: "data.fluid.io/v1alpha1",
Enabled: true,
Name: "test-dataload",
BlockOwnerDeletion: false,
Controller: true,
},
DataLoadInfo: cdataload.DataLoadInfo{
BackoffLimit: 3,
Image: "fluid:v0.0.1",
TargetDataset: "test-dataset",
SchedulerName: "scheduler-test",
TargetPaths: []cdataload.TargetPath{
{
Path: "/mnt0",
Replicas: 1,
FluidNative: true,
},
{
Path: "/mnt1",
Replicas: 1,
FluidNative: true,
},
},
ImagePullSecrets: []corev1.LocalObjectReference{},
Options: map[string]string{
"loadMemorydata": "true",
"hdfsConfig": "principal=root",
},
},
},
},
}
engine := JindoEngine{
namespace: "fluid",
Expand Down
Loading