fix(dataset): recreate the ThinRuntime of a reference dataset stuck in NotBound - #6138
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #6138 +/- ##
=======================================
Coverage 65.08% 65.09%
=======================================
Files 485 485
Lines 33989 34000 +11
=======================================
+ Hits 22122 22132 +10
- Misses 10126 10127 +1
Partials 1741 1741 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Fixes a wedge where reference Datasets could remain NotBound indefinitely when a leftover/terminating ThinRuntime was mistakenly treated as “existing”, combined with the dataset controller not watching ThinRuntime deletions to trigger a re-reconcile.
Changes:
- Treats a terminating
ThinRuntime(hasdeletionTimestamp) as an error inCreateRuntimeForReferenceDatasetIfNotExist, so the dataset reconcile will retry after the object is truly gone (instead of adopting/mutating a dying object and returning success). - Updates the dataset controller to
Owns(&ThinRuntime{})so changes/deletions of the auto-created runtime re-trigger reconciliation of the owningDataset. - Adds targeted tests for the terminating-runtime path and for ensuring the created
ThinRuntimehas a resolvable controllerOwnerReference; also fixes a pre-existing gomonkey patch flake by patching once and switching behavior via a variable.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/utils/dataset_runtime.go | Adds terminating-runtime detection and centralizes correct Dataset controller OwnerReference construction (with Kind/APIVersion fallback). |
| pkg/utils/dataset_runtime_test.go | Adds a ThinRuntimeTerminating test case and verifies the terminating object is not adopted/mutated. |
| pkg/controllers/v1alpha1/dataset/dataset_reconciler_test.go | Adds coverage that the created ThinRuntime has a watch-resolvable controller ownerRef and that terminating runtimes cause reconcile to error without adoption. |
| pkg/controllers/v1alpha1/dataset/dataset_controller.go | Watches owned ThinRuntime objects and adds the corresponding kubebuilder RBAC marker. |
| pkg/controllers/v1alpha1/dataset/dataset_controller_ut_test.go | Removes flaky double-patching by delegating the patched function’s return to a per-test variable. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
cheyang
left a comment
cheyang
left a comment
There was a problem hiding this comment.
Solid fix. The two-part root cause (adopting a terminating runtime + no watch on ThinRuntime deletion) is well-diagnosed, and the repair is minimal and targeted. The TypeMeta fallback in datasetControllerOwnerReference is a nice catch — without it the new Owns() watch would silently fail to map events back to the Dataset when the typed client hands back empty GVK.
Two observations, neither blocking:
-
pkg/utils/transformer/owner_reference.go(GenerateOwnerReferenceFromObject) has the same empty-TypeMeta vulnerability. If any of its callers rely onOwns()-style watches, they'd hit the same silent mapping failure. Worth a follow-up audit. -
The error string uses
thinRuntime(lowercase t) while the API kind isThinRuntime. Trivial, but consistent casing helps grep-ability in logs.
The gomonkey flake fix is well-justified by the 20k-iteration data. Nice cleanup.
…n NotBound A reference dataset (spec.mounts[0].mountPoint: dataset://<ns>/<name>) could stay in NotBound forever, without any ThinRuntime and without any error being surfaced. Two independent gaps combined to wedge it: 1. CreateRuntimeForReferenceDatasetIfNotExist treated a terminating ThinRuntime as an existing one. GetThinRuntime is a plain Get, so it also returns objects which already have a deletionTimestamp, e.g. a leftover runtime whose controller was scaled to 0 and could not remove its finalizer. The helper concluded that the runtime already exists, tried to adopt the dying object by overwriting its ownerReferences and returned success, so no runtime was ever created once the object finally disappeared. A runtime which is being deleted is now reported as an error (which is not a conflict error, so it is not swallowed by the surrounding retry.RetryOnConflict), letting the caller requeue until the object is really gone and a fresh runtime can be created. 2. The dataset controller did not watch the ThinRuntime it creates, so nothing re-triggered the dataset after the runtime vanished. The controller now Owns() the ThinRuntime. The owner based watch resolves a dependent through the kind and apiVersion of its ownerReference. Both were read from the dataset's TypeMeta, which a typed client may hand back empty, so they now fall back to the well-known values of the Dataset CRD. The chart role for the dataset controller already allows watching thinruntimes and the generated config/rbac/role.yaml is unchanged by the added kubebuilder marker. While verifying the change, the pre-existing spec "should requeue after the resync period when runtime scaleout returns an error" turned out to be flaky (~1% of runs, depending on binary layout): it reset the patch installed by BeforeEach and applied a second patch on the very same function, and gomonkey leaves the first patch in effect in a measurable share of such reset-then-reapply sequences (2217 out of 20000 iterations of an isolated probe). The spec then observed a successful scaleout and no requeue. It now switches behaviour through a variable which the single patch delegates to, instead of patching twice; 500 consecutive runs are green. Fixes fluid-cloudnative#6136 Signed-off-by: cheyang <cheyang@163.com>
36852b2 to
47077ff
Compare
|
|
Thanks — pushed (2) casing: the error string now says (1) In practice those flows work today, which is itself evidence that the objects those controllers read do carry a populated Worth noting for the record: with an empty So I'd treat the same fallback inside |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (4)
pkg/utils/dataset_runtime_test.go:194
- These assertions run after the table loop and depend on shared fake-client state accumulated across prior subtests, which makes the test order-dependent and harder to reason about (and can hide failures if earlier cases unintentionally mutate shared objects). Prefer moving this verification into the
ThinRuntimeTerminatingsubtest and/or building a fresh fake client perttso each case is isolated.
// The terminating runtime must be left untouched, especially it must not be adopted by the dataset.
terminatingRuntime, err := GetThinRuntime(fakeClient, "ThinRuntimeTerminating", "default")
if err != nil {
t.Fatalf("failed to get the terminating thinRuntime: %v", err)
}
if !HasDeletionTimestamp(terminatingRuntime.ObjectMeta) {
t.Errorf("expected the thinRuntime ThinRuntimeTerminating to be still terminating")
}
if len(terminatingRuntime.GetOwnerReferences()) != 0 {
t.Errorf("expected no ownerReference set on the terminating thinRuntime, but got %v", terminatingRuntime.GetOwnerReferences())
}
pkg/utils/dataset_runtime.go:57
- The helper derives
Kindfrom the object’s GVK but derivesAPIVersionfromdataset.APIVersion(TypeMeta). This can still produce an empty/mismatchedAPIVersioneven when the GVK is set (e.g., Kind is present but TypeMeta fields are not). To ensure the ownerReference is consistently resolvable byOwns(), derive bothKindandAPIVersionfrom the sameGroupVersionKind()when available (and only fall back to CRD constants if the GVK is empty).
kind := dataset.GetObjectKind().GroupVersionKind().Kind
if len(kind) == 0 {
kind = datav1alpha1.Datasetkind
}
apiVersion := dataset.APIVersion
if len(apiVersion) == 0 {
apiVersion = datav1alpha1.GroupVersion.String()
}
pkg/utils/dataset_runtime.go:65
- For controller ownerReferences, it’s generally safer to construct them via the standard helpers (e.g.,
metav1.NewControllerReforcontrollerutil.SetControllerReference) so fields likeBlockOwnerDeletionare set consistently and you match Kubernetes/controller-runtime conventions. This reduces subtle GC/deletion-behavior differences and avoids manually maintaining ownerRef semantics.
return metav1.OwnerReference{
Kind: kind,
APIVersion: apiVersion,
Name: dataset.GetName(),
UID: dataset.GetUID(),
Controller: ptr.To(true),
}
pkg/utils/dataset_runtime_test.go:111
- Change 'can not' to 'cannot' in the comment.
// A leftover runtime which is stuck in Terminating, e.g. because its controller is
// scaled to 0 and can not remove the finalizer. The finalizer is also required to keep
// the object in the fake client's tracker once a deletionTimestamp is set.
| } | ||
|
|
||
| runtimeToUpdate := runtime.DeepCopy() | ||
| runtimeToUpdate.SetOwnerReferences([]metav1.OwnerReference{ |
There was a problem hiding this comment.
SetOwnerReferences overwrites the slice rather than merging into it, so any owner reference already on the runtime is dropped when the reference dataset adopts it. I checked this with a unit test on this branch: seeding the ThinRuntime with an unrelated example.com/v1 SomeOtherKind owner and calling this function leaves exactly [{data.fluid.io/v1alpha1 Dataset ...}] — the foreign owner is gone.
The behaviour predates this PR, but the new Owns(&ThinRuntime{}) watch makes ownerReferences load-bearing for reconcile, so it seems worth tightening while you are here. Upserting the Dataset controller reference into the existing slice — replacing only a stale Dataset entry whose UID differs — would also cover the delete-and-recreate-same-name case, which currently leaves a stale-UID owner in place.
The existing ThinRuntimeExists fixture carries a single Dataset owner, so nothing covers the extra-owner shape today.
| datasetControllerOwnerReference(dataset), | ||
| }, | ||
| Labels: map[string]string{ | ||
| common.LabelAnnotationDatasetId: GetDatasetId(dataset.GetNamespace(), dataset.GetName(), string(dataset.GetUID())), |
There was a problem hiding this comment.
common.LabelAnnotationDatasetId is applied here on the create branch but not on the adopt branch above, so a ThinRuntime that predates the label — or any runtime that goes through adoption — never gets it. Verified on this branch: adopting a runtime with no labels leaves labels = map[], with the key wholly absent.
The label is read in a few places (pkg/utils/label.go, pkg/ddc/thin/referencedataset/volume.go, pkg/utils/kubeclient/configmap.go), so the gap is not cosmetic. Setting it next to the ownerReference in the adopt branch would make the two paths converge.
Separately, and out of scope for this PR: GetDatasetId only substitutes the UID once the <ns>-<name> string reaches 63 characters, so for ordinary short names the label carries no UID at all and is not unique across a delete/recreate.
| // 1.1 The runtime is being deleted, it can neither be adopted nor be re-created for now. | ||
| // Return an error (not a conflict error, so retry.RetryOnConflict won't swallow it) to | ||
| // let the caller requeue until the terminating runtime is really gone. | ||
| if HasDeletionTimestamp(runtime.ObjectMeta) { |
There was a problem hiding this comment.
Returning an error here makes CreateRuntimeForReferenceDatasetIfNotExist fail, and the caller in dataset_controller.go requeues before it reaches the phase update. So while the old ThinRuntime is still terminating, a reference dataset in NoneDatasetPhase stays at "" and never advances to NotBound. I reproduced this on this branch, and the control case without a terminating runtime does reach NotBound, which isolates the behaviour to this guard.
Given the PR is about a reference dataset stuck in NotBound, I wanted to confirm this is the intended trade-off — fail loudly and back off — rather than surfacing NotBound while the recreate is pending. Either reading seems defensible, I would just like it to be a deliberate choice.
| // Kind and APIVersion come from the dataset's TypeMeta, and fall back to the well-known values of the Dataset | ||
| // CRD when it is empty, which a typed client may hand back depending on how the object was read. The owner | ||
| // based watch of the dataset controller resolves a dependent through those two fields, so they must be set. | ||
| func datasetControllerOwnerReference(dataset *datav1alpha1.Dataset) metav1.OwnerReference { |
There was a problem hiding this comment.
#6139 adds the same kind/apiVersion fallback to pkg/utils/transformer/owner_reference.go via a scheme lookup. Once both land there are two independent implementations of "recover the owner GVK" with different semantics: this one fills the two fields independently, while the transformer gates the whole recovery on gvk.Empty() and therefore misses partially populated TypeMeta.
Worth converging on one. The per-field approach here is the more robust of the two, so folding it into the shared helper and having this call site delegate would remove the duplication without losing coverage. Not something to fix in this PR — I will open a follow-up issue.
…Meta is empty
GenerateOwnerReferenceFromObject read the kind and the apiVersion straight from the
object's TypeMeta, which a typed client is allowed to hand back empty. The resulting
reference carries an empty kind and "/" as its apiVersion, which the API server
rejects, and which an owner based watch can not resolve back to its owner. The unit
tests of the DataLoad value transformers encoded exactly that broken output as their
expectation.
Fall back to the GroupVersionKind known by the fluid scheme when the TypeMeta of the
object is empty, and fix the expectations of the affected tests. Objects which do
carry a TypeMeta are unaffected, so this only changes references which the API server
would have rejected anyway.
Two of the chains fed by this helper end up under an owner based watch: DataLoad and
DataMigrate render a batchv1.Job whose ownerReference comes from here, and both
controllers register Owns(&batchv1.Job{}).
Reported by @cheyang while reviewing fluid-cloudnative#6138.
Signed-off-by: cheyang <cheyang@163.com>
…Meta is incomplete GenerateOwnerReferenceFromObject read the kind and the apiVersion straight from the object's TypeMeta, which a typed client is allowed to hand back empty. The resulting reference carries an empty kind and "/" as its apiVersion, which the API server rejects, and which an owner based watch cannot resolve back to its owner. The unit tests of the DataLoad value transformers encoded exactly that broken output as their expectation. Recover the missing fields from the GroupVersionKind known by the fluid scheme. The kind and the apiVersion fall back on their own rather than only when the whole TypeMeta is empty, because a partially populated TypeMeta (only the kind, or only the apiVersion, as objects decoded from user YAML can carry) produces a reference which is just as malformed, and because that matches the per-field fallback used for the Dataset owner in fluid-cloudnative#6138. A type which is missing from the scheme is logged, since the function has no error return and the incomplete reference would be invisible otherwise. Objects which do carry a complete TypeMeta are unaffected, so this only changes references which the API server would have rejected anyway. The expectations of the affected tests are fixed accordingly. Two of the chains fed by this helper end up under an owner based watch: DataLoad and DataMigrate render a batchv1.Job whose ownerReference comes from here, and both controllers register Owns(&batchv1.Job{}). Reported by @cheyang while reviewing fluid-cloudnative#6138. Signed-off-by: cheyang <cheyang@163.com>
…Meta is incomplete (#6139) GenerateOwnerReferenceFromObject read the kind and the apiVersion straight from the object's TypeMeta, which a typed client is allowed to hand back empty. The resulting reference carries an empty kind and "/" as its apiVersion, which the API server rejects, and which an owner based watch cannot resolve back to its owner. The unit tests of the DataLoad value transformers encoded exactly that broken output as their expectation. Recover the missing fields from the GroupVersionKind known by the fluid scheme. The kind and the apiVersion fall back on their own rather than only when the whole TypeMeta is empty, because a partially populated TypeMeta (only the kind, or only the apiVersion, as objects decoded from user YAML can carry) produces a reference which is just as malformed, and because that matches the per-field fallback used for the Dataset owner in #6138. A type which is missing from the scheme is logged, since the function has no error return and the incomplete reference would be invisible otherwise. Objects which do carry a complete TypeMeta are unaffected, so this only changes references which the API server would have rejected anyway. The expectations of the affected tests are fixed accordingly. Two of the chains fed by this helper end up under an owner based watch: DataLoad and DataMigrate render a batchv1.Job whose ownerReference comes from here, and both controllers register Owns(&batchv1.Job{}). Reported by @cheyang while reviewing #6138. Signed-off-by: cheyang <cheyang@163.com>
Fixes #6136
The bug
A reference Dataset (
spec.mounts[0].mountPoint: dataset://<ns>/<name>) can stay inNotBoundforever, with no ThinRuntime in the namespace and no error surfaced anywhere. Observed on a live 3-node Kubernetes v1.36 cluster running Fluid from master;kubectl annotate dataset <ref-dataset> probe=1 --overwriterecreated the runtime and the Dataset becameBoundwithin seconds, i.e. the state was wedged, not slow. Full timeline and logs in #6136.Root cause and fix
1. A terminating ThinRuntime was treated as an existing one —
CreateRuntimeForReferenceDatasetIfNotExist(pkg/utils/dataset_runtime.go)GetThinRuntimeis a plainGet, so it also returns objects that already carry adeletionTimestamp— e.g. a leftover runtime whose controller had been scaled to 0 and therefore could not remove its finalizer. The helper concluded "the runtime already exists", tried to adopt the dying object by overwriting itsownerReferences, and returned success. Moments later the object really disappeared and nothing had been created.It now reports a terminating runtime as an error. The error is deliberately not a conflict error, so the surrounding
retry.RetryOnConflictdoes not swallow it and the caller (reconcileDatasetstep 3 →utils.RequeueIfError) requeues until the object is gone and a fresh runtime can be created. The dying object is neither adopted nor mutated.2. Nothing re-triggered the Dataset once the runtime vanished —
pkg/controllers/v1alpha1/dataset/dataset_controller.goSetupWithManageronly registeredFor(&Dataset{}), andreconcileDatasetends withNoRequeue(), so the deletion of the auto-created ThinRuntime produced neither an event nor a resync. The controller nowOwns(&datav1alpha1.ThinRuntime{}).The owner-based watch resolves a dependent through the
kindandapiVersionof its ownerReference. Both were read from the Dataset'sTypeMeta, which a typed client may hand back empty, so they now fall back to the well-known values of the Dataset CRD.RBAC: no file changes needed.
charts/fluid/fluid/templates/role/dataset/rbac.yamlalready grants the dataset-controllerget;list;watch;create;update;patch;deleteonthinruntimes, and the generatedconfig/rbac/role.yamlis byte-identical before/after the added kubebuilder marker (verified with the repo's pinned controller-gen v0.19.0), becausethinruntimesalready carries that verb set from other controllers' markers.Tests
pkg/utils/dataset_runtime_test.go: newThinRuntimeTerminatingcase (deletionTimestamp + finalizer so the fake client keeps the object) asserting the helper errors out and leaves the terminating object untouched, ownerReferences still empty.pkg/controllers/v1alpha1/dataset/dataset_reconciler_test.go: two specs — reconcile returns an error mentioningterminatingand does not adopt the dying runtime; and the created ThinRuntime carries a resolvable controller ownerReference thatOwns()can map back to the Dataset.(These packages need the repo's
LOCAL_FLAGS— without-gcflags="all=-N -l"the dataset package hits a pre-existing gomonkeySIGBUSon darwin/arm64.)One pre-existing test flake, diagnosed and fixed here
While verifying, the pre-existing spec "should requeue after the resync period when runtime scaleout returns an error" failed in ~1% of runs (3/248 with this change; 0/248 on the same commit without it — it is layout-sensitive, not logic-sensitive: that spec does not touch the reference-dataset path at all).
The spec reset the patch installed by
BeforeEachand applied a second gomonkey patch to the same function. An isolated probe of exactly that sequence shows gomonkey leaves the first patch in effect in 2217 out of 20000 iterations, in which case the spec observes a successful scaleout and hence no requeue — precisely the observed failure (RequeueAfter == 0). Note this direction matters: if the new patch had merely been skipped and the real function run, the fake client would have produced an error and the spec would have passed.The spec now switches behaviour through a variable that the single
BeforeEachpatch delegates to, so the function is patched exactly once. 500 consecutive runs green (previously 3 failures per ~250 runs with the same binary shape).