fix(transformer): recover the GroupVersionKind of an owner whose TypeMeta is empty - #6139
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 #6139 +/- ##
==========================================
+ Coverage 65.08% 65.10% +0.02%
==========================================
Files 485 485
Lines 33989 34015 +26
==========================================
+ Hits 22122 22147 +25
- 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
Note
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
This PR fixes GenerateOwnerReferenceFromObject so it can recover an object's GroupVersionKind (GVK) from a scheme when TypeMeta is empty, preventing invalid ownerReferences that the API server rejects and that owner-based watches can’t resolve.
Changes:
- Add GVK recovery via
apiutil.GVKForObjectwhenTypeMetais empty. - Add new unit tests covering empty-
TypeMetaFluid API objects. - Update multiple DDC DataLoad transformer test expectations to include valid
KindandAPIVersion.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/utils/transformer/owner_reference.go | Adds scheme-backed GVK resolution and uses it to build valid ownerReferences. |
| pkg/utils/transformer/owner_reference_test.go | Adds table-driven tests to ensure kind/apiVersion are recovered when TypeMeta is missing. |
| pkg/ddc/juicefs/data_load_test.go | Updates expected ownerReference output to include recovered GVK fields. |
| pkg/ddc/jindofsx/load_data_test.go | Updates expected ownerReference output to include recovered GVK fields. |
| pkg/ddc/jindocache/load_data_test.go | Updates expected ownerReference output to include recovered GVK fields. |
| pkg/ddc/jindo/load_data_test.go | Updates expected ownerReference output to include recovered GVK fields. |
| pkg/ddc/alluxio/load_data_test.go | Updates expected ownerReference output to include recovered GVK fields. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| var fluidScheme = runtime.NewScheme() | ||
|
|
||
| func init() { | ||
| utilruntime.Must(datav1alpha1.AddToScheme(fluidScheme)) | ||
| } | ||
|
|
||
| func GenerateOwnerReferenceFromObject(obj client.Object) *common.OwnerReference { | ||
| gvk := obj.GetObjectKind().GroupVersionKind() | ||
| if gvk.Empty() { | ||
| if resolved, err := apiutil.GVKForObject(obj, fluidScheme); err == nil { | ||
| gvk = resolved | ||
| } | ||
| } |
There was a problem hiding this comment.
The premise here does not hold, so no change on this point — but let me show the checks rather than just assert it:
- Other fluid API versions: there is only one.
ls api/→v1alpha1, andgrep -rn v1beta1 api/returns nothing. Thedata.fluid.io/v1beta1in the PR description comes from an existing test fixture that sets that string inTypeMetaby hand; that path never reaches the scheme lookup, because a populated field is used as-is. Sodatav1alpha1.AddToSchemecovers every fluid version that exists. - Non-fluid owners: all 16 call sites pass fluid custom resources (
dataload,dataMigrate,dataProcess, and the variousruntimetypes) — no built-in type is ever passed. Adding the client-go scheme would register types that no caller can reach.
Rather than widen the scheme speculatively, the failure is now observable: an unregistered type is logged (see the other thread), and a spec pins that behaviour. If a non-fluid owner ever shows up, the log will say so instead of silently producing an empty kind.
| // fluidScheme knows the fluid API types. It recovers the GroupVersionKind of an object whose TypeMeta is | ||
| // empty, which a typed client is allowed to hand back: an ownerReference without kind/apiVersion is rejected | ||
| // by the API server and can not be resolved back to its owner by an owner based watch. |
| // A typed client is allowed to hand back objects without TypeMeta, so the kind and the apiVersion have | ||
| // to be recovered from the scheme. Otherwise the ownerReference is rejected by the API server and can | ||
| // not be resolved back to its owner by an owner based watch. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
pkg/utils/transformer/owner_reference.go:44
- The fallback scheme only registers
datav1alpha1types. IfGenerateOwnerReferenceFromObjectis ever called with an empty-TypeMetaobject from another Fluid API version (e.g., v1beta1) or any other type not added tofluidScheme,GVKForObjectwill fail and the function will still emit an invalid ownerReference (empty kind /\"/\"apiVersion). Consider registering all supported Fluid API versions intofluidScheme, or (more robustly) refactoring the helper to accept a*runtime.Schemeso callers can pass the same scheme used by the manager/client. Also, swallowing the error (err == nilonly) makes this failure mode hard to diagnose—please surface it (e.g., viautilruntime.HandleError/logging) when resolution fails.
// fluidScheme knows the fluid API types. It recovers the GroupVersionKind of an object whose TypeMeta is
// empty, which a typed client is allowed to hand back: an ownerReference without kind/apiVersion is rejected
// by the API server and can not be resolved back to its owner by an owner based watch.
var fluidScheme = runtime.NewScheme()
func init() {
utilruntime.Must(datav1alpha1.AddToScheme(fluidScheme))
}
func GenerateOwnerReferenceFromObject(obj client.Object) *common.OwnerReference {
gvk := obj.GetObjectKind().GroupVersionKind()
if gvk.Empty() {
if resolved, err := apiutil.GVKForObject(obj, fluidScheme); err == nil {
gvk = resolved
}
}
pkg/utils/transformer/owner_reference.go:31
- Corrected spelling/grammar of 'can not' to 'cannot'.
// by the API server and can not be resolved back to its owner by an owner based watch.
|
|
||
| func GenerateOwnerReferenceFromObject(obj client.Object) *common.OwnerReference { | ||
| gvk := obj.GetObjectKind().GroupVersionKind() | ||
| if gvk.Empty() { |
There was a problem hiding this comment.
gvk.Empty() is true only when Group, Version and Kind are all empty, so an object whose TypeMeta is partially populated never reaches the scheme lookup and still ends up with a malformed reference. I checked this with a unit test against this branch, and the failure is asymmetric:
TypeMeta{Kind: "DataLoad"}->{Kind: "DataLoad", APIVersion: ""}TypeMeta{APIVersion: "data.fluid.io/v1alpha1"}->{Kind: "", APIVersion: "data.fluid.io/v1alpha1"}
Both shapes are rejected by the API server for the same reason the fully-empty case was, and partially populated TypeMeta is reachable for objects decoded from user YAML or round-tripped through a typed client.
Worth noting that #6138 solves the same problem for the Dataset owner with a per-field fallback that does handle this case:
kind := dataset.GetObjectKind().GroupVersionKind().Kind
if len(kind) == 0 {
kind = datav1alpha1.Datasetkind
}
apiVersion := dataset.APIVersion
if len(apiVersion) == 0 {
apiVersion = datav1alpha1.GroupVersion.String()
}Filling Kind and APIVersion independently here, instead of gating the whole recovery on gvk.Empty(), would close the gap and make the two code paths agree.
For what it is worth, none of the 16 current call sites hit this — they all pass concrete registered v1alpha1 structs — so this is a latent robustness gap rather than a live bug. It only matters because the helper now advertises itself as recovering the GVK.
There was a problem hiding this comment.
Fixed in e1b5efd — you are right, and the asymmetry you measured is exactly what the gvk.Empty() gate caused. Both fields now fall back on their own, matching the per-field shape used for the Dataset owner in #6138:
gvk := obj.GetObjectKind().GroupVersionKind()
if len(gvk.Kind) == 0 || len(gvk.Version) == 0 {
resolved, err := apiutil.GVKForObject(obj, fluidScheme)
if err != nil {
log.Error(err, "failed to recover the GroupVersionKind of the owner from the scheme, the generated ownerReference stays incomplete", ...)
} else {
if len(gvk.Kind) == 0 {
gvk.Kind = resolved.Kind
}
if len(gvk.Version) == 0 {
gvk.Group, gvk.Version = resolved.Group, resolved.Version
}
}
}Two entries added for the partial shapes (TypeMeta{Kind: "DataLoad"} and TypeMeta{APIVersion: "data.fluid.io/v1alpha1"}), both asserting kind and apiVersion. Confirmed they fail against the previous revision of this branch — 4 failed assertions — and pass now (28/28 specs).
| func GenerateOwnerReferenceFromObject(obj client.Object) *common.OwnerReference { | ||
| gvk := obj.GetObjectKind().GroupVersionKind() | ||
| if gvk.Empty() { | ||
| if resolved, err := apiutil.GVKForObject(obj, fluidScheme); err == nil { |
There was a problem hiding this comment.
The err == nil guard means a type that is not registered in fluidScheme falls through silently and yields {Kind: "", APIVersion: ""}. I confirmed zero log records are emitted on that path.
Since the function has no error return, a single log.Error here would be the only way a caller could ever notice a misconfigured scheme. Low priority while every caller passes a registered type, but today the failure is completely invisible.
There was a problem hiding this comment.
Added in e1b5efd: the scheme lookup failure is now logged through ctrl.Log.WithName("utils.transformer"), with the namespace, the name and the incoming GVK, and the message states that the reference stays incomplete. Matches the package-level logger pattern used elsewhere under pkg/utils (utils, utils.time, utils.security, kubeclient).
There is also a spec asserting the fall-through behaviour for a type the fluid scheme does not know (corev1.ConfigMap), so the shape of that path is pinned even though the log itself is not asserted.
ee7e5ff to
b24718f
Compare
…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>
b24718f to
e1b5efd
Compare
|
Follow-up to the review observation on #6138 (thanks @cheyang).
Problem
transformer.GenerateOwnerReferenceFromObjectread the kind and the apiVersion straight from the object'sTypeMeta:A typed client is allowed to hand back objects with an empty
TypeMeta. In that case the produced reference has an emptyKindand"/"as itsAPIVersion— a reference the API server rejects, and which an owner-based watch cannot resolve back to its owner (Owns()/EnqueueRequestForOwnermatch on group+kind).Two of the chains fed by this helper do end up under an owner-based watch: DataLoad and DataMigrate render a
batchv1.Jobwhose ownerReference comes from here, anddataload_controller.go/datamigrate_controller.goboth registerOwns(&batchv1.Job{}).The unit tests of the DataLoad value transformers encoded that broken output as their expectation, in 5 packages:
Change
Resolve the GVK from a scheme that knows the fluid API types when the object's
TypeMetais empty:Objects that do carry a
TypeMetaare unaffected (existing expectations withdata.fluid.io/v1alpha1and evendata.fluid.io/v1beta1still hold), so the only references this changes are ones the API server would have rejected anyway.The 20 stale expectations across
pkg/ddc/{alluxio,jindo,jindofsx,jindocache,juicefs}now expectKind: DataLoad/APIVersion: data.fluid.io/v1alpha1.Tests
pkg/utils/transformer/owner_reference_test.go: new table covering an emptyTypeMetaforDataset,AlluxioRuntimeandDataLoad, asserting the kind is recovered and the apiVersion is the fluid group/version. Verified these fail without the source change (14 failed assertions) and pass with it.Note for reviewers running the full
./pkg/ddc/...suite on darwin/arm64: theAlluxioFileUtils_*/operationsgomonkey tests are flaky there independently of this change (three baseline runs of the same commit without this change produced 20, 24 and 21 failures, with a different set each time).Test_genDataLoadValuewas the only deterministic difference and it is fixed by the updated expectations.