Repository navigation
fix(utils): recover group for group-less apiVersion in owner reference - #6206
Conversation
Motivation: GenerateOwnerReferenceFromObject only consulted the scheme when Kind or Version was empty. An object with apiVersion "v1alpha1" parses with Kind and Version set but an empty Group, so the OwnerReference was emitted without the data.fluid.io group, which owner-based watches and GC resolve by group and kind. Approach: also trigger scheme recovery when Group is empty and, if the version is already set, fill in only the group. Validation: go test ./pkg/utils/transformer/ passes; the new group-less Dataset case fails without the change and passes with it. golangci-lint run ./pkg/utils/transformer/... reports 0 issues. No live cluster run. Report: fluid-cloudnative#6205 Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com> Assisted-by: claude-sonnet-5-5 (via Claude Code)
|
Hi @pujitha24. Thanks for your PR. I'm waiting for a fluid-cloudnative member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
There was a problem hiding this comment.
🟢 Approval recommended
The narrow fix addresses the reported issue and includes targeted regression coverage, with no blocking issues identified.
0 open findings
What changed in this PR
Fixes #6205 by recovering missing API groups in owner references for Fluid objects.
Changes:
- Triggers scheme recovery when the group is missing, preserving an existing version.
- Adds a regression test for a Dataset with
APIVersion: "v1alpha1".
| File | Description |
|---|---|
| pkg/utils/transformer/owner_reference.go | Recovers missing groups from the Fluid scheme. |
| pkg/utils/transformer/owner_reference_test.go | Tests group recovery for a version-only API value. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
cheyang
left a comment
cheyang
left a comment
There was a problem hiding this comment.
Both reviewers verified the premise of #6205 independently on the base commit and it holds: an object with a group-less apiVersion (e.g. TypeMeta{Kind: "Dataset", APIVersion: "v1alpha1"}) produced an ownerReference without the data.fluid.io group — accepted by the API server on write, but silently dropped by the real owner-based watch (envtest with EnqueueRequestForOwner: 0 reconcile requests on base, 1 on this head). The fix is minimal and lands at the right layer: all contracts pass on head 0fa32294 (group fill for every fluid owner type, the kind-missing variant, the consumer path through datasetControllerOwnerReference, and no-output-change guards for core types), with the fix reverting red and restoring green on both harnesses. No blockers and no majors from either side — the cross-review was full convergence, with the only recorded disagreement being none at all. Reviewer C (Copilot) also raised no findings on this head. What remains is small: the added table entry covers only the kind-set shape of the new branch (the kind-missing + group-less combination, and a group-less case in TestDatasetControllerOwnerReference, are verified on the review branches but not yet in the PR's suite — both tests are ready to lift), the header comment at owner_reference.go:42 is now one noun short, and the widened trigger logs a harmless-but-misleading error for fully-typed core-group objects (output verified unchanged). One open question for the author: no current production path constructs the group-less shape, so if you have a concrete producer in mind, naming it would sharpen the motivation. CI beyond DCO/SonarCloud had not reported at review time — please wait for tide and the unit-test workflow before merging.
Next round
[MUST] (none — no blocking findings)
[SHOULD] Lift the kind-missing + group-less table entry (and a group-less case in TestDatasetControllerOwnerReference) from docs/verification/ownerref-group-recovery/ or docs/verification/ownerref-groupless-group/ into the PR's own tests
[SHOULD] Update the owner_reference.go:42 header comment to mention that the group also falls back now
[SHOULD] If touched again, log the recovery failure only when the emitted reference is actually still incomplete, so core-group types with complete TypeMeta don't log a misleading error
[Filed] (none)
Non-blocking observations (4) — not part of this round's ask; fix now, follow up, or file an issue as you see fit
- pkg/utils/transformer/owner_reference.go:45 — Widened recovery trigger emits a misleading error log for fully-typed core-group objects [minor]
- pkg/utils/transformer/owner_reference.go:45 — Is there a known production path that produces a group-less TypeMeta for this helper? [minor] — pre-existing, not introduced by this PR
- pkg/utils/transformer/owner_reference_test.go:251 — No PR test for the group-less apiVersion + missing-Kind shape, and no group-less case in the consumer-side... [minor]
- pkg/utils/transformer/owner_reference.go:42 — Function header comment understates the recovery scope after this change [nit]
Verified review delta
- F2-core-type-recovery-log-noise: open (initial-pr)
- F4-groupless-reachability: open (preexisting)
- F1-groupless-nokind-coverage: open (initial-pr)
- F3-header-comment-group-fallback: open (initial-pr)
Code: clear. CI: pending; attribution: unknown. Required CI checks still apply.
… incomplete 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 #6206 +/- ##
=======================================
Coverage 65.60% 65.61%
=======================================
Files 486 486
Lines 34354 34361 +7
=======================================
+ Hits 22539 22546 +7
Misses 10061 10061
Partials 1754 1754 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…wnerReference Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com>
|
cheyang
left a comment
cheyang
left a comment
There was a problem hiding this comment.
Follow-up round on 0daf2ba (delta 0fa3229..0daf2ba): the two new commits implement exactly the round-1 minor suggestions — the scheme-lookup error log now fires only when the reference is actually incomplete (core-group objects with a legitimate empty group no longer log a misleading error), the header comment covers the group fallback, and the missing group-less/kind-less cases are now covered in both owner_reference_test.go and the dataset controller suite. All six verification contracts pass on the new head (group-less and kind-less recovery, watch group-match unit and envtest enqueue with the production EnqueueRequestForOwner wiring, core-type and fully-typed regression guards), and pkg/utils/transformer plus pkg/utils suites are green; the pkg/utils/helm failure is base-identical and environmental (helm binary not on PATH). LGTM — merging once the required CI reports green on 0daf2ba.
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cheyang The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
This is approved with green CI and no outstanding comments as far as I can tell — ready whenever you have a moment. Happy to rebase first if you'd like it freshened. |
cheyang
left a comment
cheyang
left a comment
There was a problem hiding this comment.
Round 2 on head 0daf2ba (delta 0fa32294..0daf2ba, two commits) lands all three items from the previous review, and each is verified differentially on both harness branches rather than by inspection:
- Log noise fixed (
75851117): the recovery-failure error now fires only when the emitted reference is actually incomplete. Two independent log-capture contracts (ConfigMap/v1 and Pod/v1) are red on the previous head and green on this one — and green on the base, confirming the noise was born and died inside this PR — while a guard confirms genuinely incomplete references still log. Output stays byte-identical throughout. - Test gaps closed (
0daf2ba0): the kind-missing + group-less entry inowner_reference_test.goand the group-less case inTestDatasetControllerOwnerReferenceare now in the PR's own suite. Grafted onto the base, both fail with the exact issue-#6205 signature (apiVersionleft asv1alpha1) — genuine differential tests, not decoration. - Header comment now names the group alongside kind and version.
All round-1 contracts re-ran green on the new head, including the envtest owner-watch integration (0 enqueues on base → 1 on head), so the delta introduces no regression; the only behavioral change vs the previous head is whether one log.Error fires, and no caller depends on it. CI has fully green on 0daf2ba — unittest, lint, staticcheck, builds, kind-e2e and backward-compat across five k8s versions, CodeQL, SonarCloud, codecov, DCO and tide — closing the CI gate that was pending last round. The two reviewers were in full agreement on every verdict, and Reviewer C (Copilot) raised no findings on this head.
From the review side this is merge-ready; the merge decision itself goes to the maintainer.
Next round
[MUST] (none — no blocking findings)
[SHOULD] Lift the two log-gating contract tests (with the zap-buffer log-capture helper) from verify/ownerref-group-recovery-claude into the PR's own suite, so the suppression is locked in-tree — author's discretion
[SHOULD] If one exists, name the concrete producer of the group-less TypeMeta in the PR description to sharpen the motivation (pre-existing question; otherwise this remains contract hardening)
[Filed] (none)
Non-blocking observations (1) — not part of this round's ask; fix now, follow up, or file an issue as you see fit
- pkg/utils/transformer/owner_reference_test.go:300 — The PR's own suite does not lock the new log-gating behavior [nit]
Verified review delta
- F2-core-type-recovery-log-noise: fixed (initial-pr)
- F1-groupless-nokind-coverage: fixed (initial-pr)
- F3-header-comment-group-fallback: fixed (initial-pr)
- F5-log-gating-not-in-pr-tests: open (initial-pr)
Code: clear. CI: success; attribution: baseline. Required CI checks still apply.
|
@pujitha24 Thank you for your contribution. Looks forward to your next PR. |
|
On the producer question: I don't have a concrete production caller that builds a group-less TypeMeta today. The shape came up while reading the helper against #6205, so this is hardening the helper's contract and not a fix for a path I've seen break. I'm happy to have the description say that plainly if you'd rather it not read as more than that. I'm leaving the log-gating tests out of this PR for now. The behavior is covered by your out-of-tree checks, and I didn't want to add a zap-capture helper to the suite at this stage. If you'd prefer them in-tree before merge, say so and I'll add them. |
Ⅰ. Describe what this PR does
GenerateOwnerReferenceFromObjectnow also recovers the group from the Fluid scheme when the object's apiVersion has no group (e.g.v1alpha1), so the generated OwnerReference isdata.fluid.io/v1alpha1rather thanv1alpha1.Ⅱ. Does this pull request fix one issue?
fixes #6205
Ⅲ. List the added test cases (unit test/integration test) if any, please explain if no tests are needed.
New table entry in
pkg/utils/transformer/owner_reference_test.gofor a Dataset withAPIVersion: "v1alpha1". It fails without the fix and passes with it.Ⅳ. Describe how to verify it
go test ./pkg/utils/transformer/andgolangci-lint run ./pkg/utils/transformer/.... Both passed locally. I did not run a live cluster; the change is covered by the unit test only.Ⅴ. Special notes for reviews
Only the group is filled in when the version is already set. All production callers pass Fluid types registered in the scheme.
Fixes #6205