Skip to content

Converge the two owner-reference GVK recovery implementations #6140

Description

@cheyang

Problem

Two independent implementations of "recover the owner GroupVersionKind when TypeMeta is empty" are landing at the same time, and they do not agree on semantics.

#6138 adds a local helper in pkg/utils/dataset_runtime.go that fills the two fields independently:

func datasetControllerOwnerReference(dataset *datav1alpha1.Dataset) metav1.OwnerReference {
	kind := dataset.GetObjectKind().GroupVersionKind().Kind
	if len(kind) == 0 {
		kind = datav1alpha1.Datasetkind
	}
	apiVersion := dataset.APIVersion
	if len(apiVersion) == 0 {
		apiVersion = datav1alpha1.GroupVersion.String()
	}
	...
}

#6139 adds a scheme-based recovery to the shared helper in pkg/utils/transformer/owner_reference.go, gated on gvk.Empty():

gvk := obj.GetObjectKind().GroupVersionKind()
if gvk.Empty() {
	if resolved, err := apiutil.GVKForObject(obj, fluidScheme); err == nil {
		gvk = resolved
	}
}

The two code paths do not overlap — datasetControllerOwnerReference does not go through the transformer — so neither fix covers the other's callers.

Why the difference matters

gvk.Empty() is true only when Group, Version and Kind are all empty. An object whose TypeMeta is partially populated therefore skips recovery entirely and still produces a malformed reference. Verified with a unit test against #6139's head:

input resulting owner reference
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.

The per-field approach in #6138 handles this correctly. So the narrower, single-purpose helper is currently the more robust of the two, which is the wrong way round for a shared utility.

Proposal

  1. Make GenerateOwnerReferenceFromObject fill Kind and APIVersion independently rather than gating the whole recovery on gvk.Empty(), keeping the scheme lookup as the source of truth for both fields.
  2. Surface a diagnostic when apiutil.GVKForObject fails. Today the error is dropped (if resolved, err := ...; err == nil) and an unregistered type degrades silently to {Kind: "", APIVersion: ""} with no log line — confirmed zero log records on that path.
  3. Have datasetControllerOwnerReference delegate to the shared helper so there is one implementation, keeping the well-known Dataset values only as a last-resort fallback if the scheme lookup is unavailable.
  4. Add regression coverage for the partial-TypeMeta shapes, which no existing test exercises.

Scope

Follow-up only — not a blocker for either PR. Neither gap is reachable from the current call sites, since all 16 production callers pass concrete registered v1alpha1 structs. This is about the contract of a helper that now advertises itself as recovering the GVK, and about not leaving two divergent copies of that logic in the tree.

Related: #6138, #6139

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions