Skip to content

Job controller optimization: reduce work duration time & minimize cache locking #132305

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jun 18, 2025
+33 −3
Merged

Job controller optimization: reduce work duration time & minimize cache locking #132305

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 33 additions & 3 deletions pkg/controller/job/job_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,9 @@ type Controller struct {
// A store of pods, populated by the podController
podStore corelisters.PodLister

// podIndexer allows looking up pods by ControllerRef UID
podIndexer cache.Indexer

// Jobs that need to be updated
queue workqueue.TypedRateLimitingInterface[string]

Expand Down Expand Up @@ -223,6 +226,12 @@ func newControllerWithClock(ctx context.Context, podInformer coreinformers.PodIn
jm.podStore = podInformer.Lister()
jm.podStoreSynced = podInformer.Informer().HasSynced

err := controller.AddPodControllerUIDIndexer(podInformer.Informer())
if err != nil {
return nil, fmt.Errorf("adding Pod controller UID indexer: %w", err)
}
jm.podIndexer = podInformer.Informer().GetIndexer()

jm.updateStatusHandler = jm.updateJobStatus
jm.patchJobHandler = jm.patchJob
jm.syncHandler = jm.syncJob
Expand Down Expand Up @@ -758,9 +767,9 @@ func (jm *Controller) getPodsForJob(ctx context.Context, j *batch.Job) ([]*v1.Po
if err != nil {
return nil, fmt.Errorf("couldn't convert Job selector: %v", err)
}
// List all pods to include those that don't match the selector anymore
// but have a ControllerRef pointing to this controller.
pods, err := jm.podStore.Pods(j.Namespace).List(labels.Everything())

// list all pods managed by this Job using the pod indexer
pods, err := jm.getJobPodsByIndexer(j)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -799,6 +808,27 @@ func (jm *Controller) getPodsForJob(ctx context.Context, j *batch.Job) ([]*v1.Po
return pods, err
}

// getJobPodsByIndexer returns the set of pods that this Job should manage.
func (jm *Controller) getJobPodsByIndexer(j *batch.Job) ([]*v1.Pod, error) {
podsForJob := []*v1.Pod{}
for _, key := range []string{string(j.UID), controller.OrphanPodIndexKey} {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How the orphan pods get populated in the index, can you provide some reference?

Copy link
Member Author

@xigang xigang Jun 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Orphan pods are automatically indexed through the AddPodControllerUIDIndexer function in pkg/controller/controller_utils.go:1092-1105.

When a pod has no ControllerRef, it gets indexed under OrphanPodIndexKey instead of a specific controller UID.

This allows controllers to discover and potentially adopt pods that match their selectors but temporarily lack owner references.

Reference:
Similar pattern used by StatefulSet, ReplicaSet, and DaemonSet controllers.

Copy link
Contributor

@mimowo mimowo Jun 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting, thank you for explaining.

Does it mean that we now list orphaned Pods from all namespaces?

If so, then it seems this may actually hurt performance of some environments with many namespaces and orphaned Pods (for example when Pods are managed by external systems directly), as before we would only list Pods from a single namespace.

@hakuna-matatah @xigang Is such a situation covered by the benchmarks, or is it not a concern for us?

Maybe as a mitigation we could filter early the Pods by the namespace as we iterate over them anyway, wdyt @soltysh @wojtek-t @atiratree ? If we change the semantics of the function to return also Pods from other namespaces, then we need to assume that the downstream code can cope with it. I'm not sure how well tested such scenarios are, so extra filtering by namespace seems useful anyway to maintain semantics.

Or maybe, even better there could be different keys for orphans in the indexer depending on namespace?

As a follow up I argue we should commonize the functions (probably in pkg/controller/controller_utils.go), as this seems a non-trivial code duplication.

Copy link
Member Author

@xigang xigang Jun 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To distinguish orphan Pods based on namespace, the index can be built like this:

// AddPodControllerUIDIndexer adds an indexer for Pod's controllerRef.UID to the given PodInformer.
// This indexer is used to efficiently look up pods by their ControllerRef.UID
func AddPodControllerUIDIndexer(podInformer cache.SharedIndexInformer) error {
	if _, exists := podInformer.GetIndexer().GetIndexers()[PodControllerUIDIndex]; exists {
		// indexer already exists, do nothing
		return nil
	}
	return podInformer.AddIndexers(cache.Indexers{
		PodControllerUIDIndex: func(obj interface{}) ([]string, error) {
			pod, ok := obj.(*v1.Pod)
			if !ok {
				return nil, nil
			}
			// Get the ControllerRef of the Pod to check if it's managed by a controller
			if ref := metav1.GetControllerOf(pod); ref != nil {
				return []string{string(ref.UID)}, nil
			}
			// If the Pod has no controller (i.e., it's orphaned), index it with namespace-specific OrphanPodIndexKey
			// This helps identify orphan pods for reconciliation and adoption by controllers within specific namespaces
			return []string{OrphanPodIndexKey + "/" + pod.Namespace}, nil
		},
	})
}

However, this approach requires changes to the code of other controllers, such as ReplicaSet and DaemonSet.

Before:

uidKeys := []string{string(rs.UID), controller.OrphanPodIndexKey}

After:

uidKeys := []string{string(rs.UID), controller.OrphanPodIndexKey + "/" + rs.Namespace}

Copy link
Contributor

@mimowo mimowo Jun 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, to have a peace of mind that we are not worsening the scenario I would suggest to go this way, and commonized the function in controller_utils, but let's see what others say.

I'm ok to merge the PR as is if we follow up, but keep it open yet for visibility.

I'm also ok to turn this PR to also adjust the previous controllers and expose the utility function in controllers_utils

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let’s wait and see what others suggest first.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not super worried about too many orphaned pods, but I also like the change to the indexing suggested above. Given it's purely in-memory indexing we can change that at any point in time.

So let's follow-up on changing the value to also include the namespace - I would suggest merging the PR as is, and then switching all controllers to the new index in one shot.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I can take care of the follow-up work on the new index.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sgtm. I would also like the following up to ensure that there is no risk for the downstream code to now deal with pods coming from other namespaces. the code may or may not be ready for it

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, sounds like a nice improvement for all the controllers.

A small suggestion; we could make a function for constructing the orphan key

controller.OrphanPodIndexKey + "/" + rs.Namespace

pods, err := jm.podIndexer.ByIndex(controller.PodControllerUIDIndex, key)
if err != nil {
return nil, err
}

for _, obj := range pods {
pod, ok := obj.(*v1.Pod)
if !ok {
utilruntime.HandleError(fmt.Errorf("unexpected object type in pod indexer: %v", obj))
continue
}
podsForJob = append(podsForJob, pod)
}
}
return podsForJob, nil
}

// syncJob will sync the job with the given key if it has had its expectations fulfilled, meaning
// it did not expect to see any more of its pods created or deleted. This function is not meant to be invoked
// concurrently with the same key.
Expand Down