Skip to content

A pod group's job identity is one member's UID, so losing a member reads as a different job #14533

Description

@thc1006

GenericJob.Object() is the framework's handle on "which job is this". For most integrations it is the job object itself, so Object().GetUID() identifies it. For a pod group it is whichever member Load happened to pick, and that changes when the group's membership changes.

What happened:

(*Pod).Load takes the group path when the reconcile key carries a group/ namespace prefix, and picks the group's representative like this:

if len(p.list.Items) > 0 {
p.isFound = true
p.pod = p.list.Items[0]
key.Name = p.pod.Name
}

	if len(p.list.Items) > 0 {
		p.isFound = true
		p.pod = p.list.Items[0]
		key.Name = p.pod.Name
	}

(*Pod).Object() returns &p.pod, so Object().GetUID() is that one member's UID. p.list comes from a List over the group-name index and is never sorted, so the representative is whatever the index returns first. Two reads of the same group can report two different UIDs.

This is not a theoretical window for the stop path. (*Pod).Stop deletes the group's pods:

// Delete the pod if it is not already deleted.
if err == nil {
if err := c.Delete(ctx, podInGroup.Object()); client.IgnoreNotFound(err) != nil {
return stoppedNow, err
}
}

so a stop is itself a membership change, and a read taken after it can pick a different representative than the read taken before it.

What you expected to happen:

Object().GetUID() identifies the job, so a caller can use it to tell "the same job" from "a job deleted and recreated under the same name". A pod group that loses or gains a member is still the same group, and should still answer the same.

How to reproduce it (as minimally and precisely as possible):

No cluster needed. Put this in pkg/controller/jobs/pod and run it:

func TestGroupIdentityIsMemberDerived(t *testing.T) {
	ctx, _ := utiltesting.ContextWithLog(t)

	mk := func(name, uid string) *corev1.Pod {
		return &corev1.Pod{ObjectMeta: metav1.ObjectMeta{
			Name:      name,
			Namespace: "ns",
			UID:       types.UID(uid),
			Labels:    map[string]string{podconstants.GroupNameLabel: "grp"},
		}}
	}
	a, b := mk("pod-a", "uid-a"), mk("pod-b", "uid-b")

	cl := utiltesting.NewClientBuilder().
		WithObjects(a, b).
		WithIndex(&corev1.Pod{}, PodGroupNameCacheKey, IndexPodGroupName).
		Build()

	load := func() types.UID {
		p := &Pod{excessPodExpectations: expectations.NewStore("test")}
		key := types.NamespacedName{Name: "grp", Namespace: "group/ns"}
		if _, err := p.Load(ctx, cl, &key); err != nil {
			t.Fatalf("Load: %v", err)
		}
		return p.Object().GetUID()
	}

	first := load()
	victim := a
	if first == b.UID {
		victim = b
	}
	if err := cl.Delete(ctx, victim); err != nil {
		t.Fatalf("Delete: %v", err)
	}
	second := load()

	t.Logf("group %q identity moved %s -> %s after %s left", "grp", first, second, victim.Name)
	if first == second {
		t.Fatalf("identity unchanged")
	}
}
    group "grp" identity moved uid-a -> uid-b after pod-a left
--- PASS

Anything else we need to know?:

I found this while adding a guard in #14441 that compares job.Object().GetUID() across a reload, to tell an original job from one deleted and recreated under the same name. That comparison is exact for a single-object job and cannot be for a pod group: a group that only lost a member reads as replaced, and the guard then skips the job's own finalization. I have kept that guard in the PR and noted the limitation in a comment there rather than widening its scope.

The same member-derived UID is read elsewhere in the framework, and those sites are worth an audit rather than an accusation — I have not shown any of them to be broken for a group:

  • reconciler.go, the orphaned-workload owner comparison behind FinishOrphanedWorkloads
  • reconciler.go, the kueue.x-k8s.io/job-uid label written on the Workload

Related work on identity in this area, both distinct from this: #13960 refuses adopting and finalizing foreign Workloads by pod group name, and #13802 validates owner-reference UIDs in FindAncestorJobManagedByKueue. Neither changes Load.

To be clear about how far this is verified: the mechanism is reproduced by the test above and the deletion in Stop is in the code, but I have not observed a resulting failure on a live cluster. If a stable group identity is wanted, the owner references the Workload already carries for every member look like a better basis than Items[0].

Environment:

  • Kubernetes version (use kubectl version): not applicable, the reproducer uses the controller-runtime fake client
  • Kueue version (use git describe --tags --dirty --always): main at 6d6df4c, also reproduced on v0.20.0-devel-327-gd1a0dabb0
  • Cloud provider or hardware configuration: n/a
  • OS (e.g: cat /etc/os-release): n/a
  • Kernel (e.g. uname -a): n/a
  • Install tools: n/a
  • Others: plain Pod (pod group) integration

/kind bug
/area integrations

This report was prepared with AI assistance; the reproducer was run before filing.

Metadata

Metadata

Assignees

Labels

area/integrationsWorkload integrationskind/bugCategorizes issue or PR as related to a bug.

Type

No type

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions