fix(jindo): fall back to dataset mounts when DataLoad has no target - #6156
fix(jindo): fall back to dataset mounts when DataLoad has no target#6156pujitha24 wants to merge 2 commits into
Conversation
Motivation: When a Jindo DataLoad is created without spec.target, and the target Dataset has multiple mount points, JindoEngine.genDataLoadValue builds TargetPaths solely from dataload.Spec.Target. With that field empty, TargetPaths ends up empty regardless of how many mounts the dataset has, so the resulting DataLoad job does not load any data. Approach: When dataload.Spec.Target is empty, fall back to deriving TargetPaths from Dataset.Spec.Mounts, one entry per mount, defaulting Replicas to 1. Each mount's path is computed with the existing utils.UFSPathBuilder.GenUFSPathInUnifiedNamespace helper (already used by other engines, e.g. alluxio) so that mounts without an explicit path are handled the same way the rest of the codebase handles them. Validation: - go build ./... - go test ./pkg/ddc/jindo/ -run "Test_genDataLoadValue|TestGenerateDataLoadValueFile" -v (all cases pass, including a new case covering a DataLoad with no target on a Dataset with multiple mounts) Note: the wider `go test ./pkg/ddc/jindo/...` has pre-existing, unrelated failures on this environment (TestDestroyWorker, and a gomonkey/Go-runtime crash in the operations subpackage) that are present on master without this change too (confirmed via git stash), so they are not related to this fix. This change only touches the Jindo engine, matching the scope of the reported issue. JindoCache and JindoFsx have the same empty-target gap in their own load_data.go but are left untouched here to keep the change minimal. Report: fluid-cloudnative#4439 Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #6156 +/- ##
=======================================
Coverage 65.13% 65.13%
=======================================
Files 485 485
Lines 34039 34039
=======================================
Hits 22171 22171
Misses 10127 10127
Partials 1741 1741 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
cheyang
left a comment
cheyang
left a comment
There was a problem hiding this comment.
Thanks for the detailed writeup, and for flagging the pre-existing failures. I checked TestDestroyWorker at the merge base with this change absent and it fails the same way, so that one is definitely not yours.
I'm requesting changes, but I want to be clear up front that I think you're chasing a real bug. #4439 has been open since December 2024 and is still unfixed. My problem is that this patch targets a different engine than the report, and rests on a mechanism that doesn't hold, so it wouldn't fix the report while introducing a new failure.
Two separate points.
First, #4439 is about a JindoRuntime. GetDefaultEngineImpl() (pkg/utils/jindo/jindo.go:37) returns jindocache unless the operator runs with JINDO_ENGINE_TYPE=jindo or jindofsx, and this PR only changes pkg/ddc/jindo, the legacy JindoFS engine. A default install never reaches this code.
Second, the reported symptom is a DataLoad pod that failed, not one that silently loaded nothing. An empty spec.target does not produce an empty DATA_PATH: TargetPaths is json:"targetPaths,omitempty" (pkg/dataload/value.go:50), so the empty slice never reaches the generated values file and helm falls back to the chart default path: "/". On pkg/ddc/jindo that default works, and this PR replaces it with per-mount paths that are not addressable in the single jindo namespace, so the job fails where it previously succeeded.
Where I would look for the real cause of #4439: jindocache's dataload script runs jindo fs -ls jindo://$targetPath, so the same chart default / becomes jindo:///. jindocache addresses data through one cacheset per mount (cacheset.xml, with <name> from mount.Name and <path> from the mount point, built in pkg/ddc/jindocache/transform.go), and there is no jindo:/// root. That would produce exactly the dataLoad failed because some paths not exist. from the report. I have not confirmed this on a live cluster, so please treat it as a lead rather than a diagnosis.
That also means your instinct fits jindocache better than it fits this engine. jindocache really does have a per-mount cacheset, so deriving one target per mount can make sense there. The same code cannot work in pkg/ddc/jindo, which collapses every mount into one namespace over one UFS URI.
The jindo side needs no cluster to reproduce. I rendered the real dataloader chart and ran the repo's own dataload script against a stub CLI. Reproduction and captured output: https://github.com/cheyang/fluid/tree/verify/pr6156-jindo-dataload-no-target/docs/verification/pr6156-jindo-dataload-no-target
| } | ||
| } else { | ||
| // 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. |
There was a problem hiding this comment.
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.
| // 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. | ||
| for _, mount := range targetDataset.Spec.Mounts { | ||
| path := utils.UFSPathBuilder{}.GenUFSPathInUnifiedNamespace(mount) |
There was a problem hiding this comment.
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: fluidNative, | ||
| }) | ||
| } | ||
| } else { |
There was a problem hiding this comment.
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.
| { | ||
| Name: "spark", | ||
| MountPoint: "local://mnt/data0", | ||
| Path: "/mnt0", |
There was a problem hiding this comment.
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.
| fluidNative := utils.IsTargetPathUnderFluidNativeMounts(path, *targetDataset) | ||
| targetPaths = append(targetPaths, cdataload.TargetPath{ | ||
| Path: path, | ||
| Replicas: 1, |
There was a problem hiding this comment.
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.
cheyang's review on fluid-cloudnative#6156 showed the empty-TargetPaths premise doesn't hold on this engine (omitempty + chart default already load the whole dataset) and that per-mount paths aren't addressable in pkg/ddc/jindo's single JindoFS namespace, so the change would break multi-mount DataLoads. Revert load_data.go and its test back to master; the actual fix for fluid-cloudnative#4439 (default installs run jindocache, not this engine) would need a separate, jindocache-targeted change. Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com>
|
|
You're right on both points. I reverted I haven't done the jindocache-side investigation, so I can't confirm your |
2 participants
Ⅰ. Describe what this PR does
This PR originally added a fallback in
JindoEngine.genDataLoadValuethat derivedTargetPathsfromDataset.Spec.Mountswhen a JindoDataLoadhad no explicitspec.target, on the assumption that an emptyTargetPathsmade the DataLoad job a no-op.@cheyang's review showed that assumption is wrong for this engine:
TargetPathsisjson:"targetPaths,omitempty", so an empty slice is dropped from the generated values file and helm falls back to the chart's own default (path: "/"), which already loads the whole dataset. The per-mount paths this PR generated aren't addressable inpkg/ddc/jindo's singlejindonamespace either (transform.gohardcodesjfsNamespace := "jindo"and every mount overwrites the same URI key), so with more than one mount the change would turn a working DataLoad into a failing one.Given that, this PR now reverts
pkg/ddc/jindo/load_data.goand its test back to the pre-PR behavior — the diff is a no-op relative tomaster. It's left open pending a decision on whether to pursue a fix onjindocacheinstead (the engine#4439's default install actually runs, and where the per-mount idea is more applicable given its per-mount cacheset model), which needs separate investigation this PR hasn't done.Ⅱ. Does this pull request fix one issue?
Not fixing #4439 with this PR. The report is about a
JindoRuntime, andGetDefaultEngineImpl()returnsjindocacheby default, so a change topkg/ddc/jindo(the legacy JindoFS engine, only used whenJINDO_ENGINE_TYPE=jindoorjindofsx) doesn't affect the reported install.Ⅲ. List the added test cases (unit test/integration test) if any, please explain if no tests are needed.
None — the code and test changes have been reverted, so no new behavior is added.
Ⅳ. Describe how to verify it
go test ./pkg/ddc/jindo/ -run "Test_genDataLoadValue|TestGenerateDataLoadValueFile" -vpasses against the reverted code (same as it does onmaster).Ⅴ. Special notes for reviews
This PR is currently a no-op diff against
master, kept open for visibility while we figure out whether to retarget the fix tojindocacheor close this out. See review discussion for the full reasoning.AI assistance: this change was drafted with Claude Code.