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

[xigang]

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

This improves performance and correctness at scale by minimizing the time a read lock is held on the cache, reducing blockage of CacheController delta processing. It also helps lower workqueue processing time per object/key.

Which issue(s) this PR is related to:

Fixes # #130767

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Job controller uses controller UID index for pod lookups.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

[k8s-ci-robot]

This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

Instructions 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-sigs/prow repository.

[xigang]

/ok-to-test

[xigang]

/cc @mimowo @tenzen-y @kannon92

PTAL.

[xigang]

/assign @tenzen-y @kannon92

[mimowo]

@xigang do you have some measurements or experiments which would show the performance improvement?

[mimowo]

Please also post some release note and fix linter

[mimowo]

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

[xigang]

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.

[mimowo]

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.

[xigang]

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}

[mimowo]

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

[xigang]

Let’s wait and see what others suggest first.

[wojtek-t]

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.

[xigang]

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

[mimowo]

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

[atiratree]

+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

[xigang]

do you have some measurements or experiments which would show the performance improvement?

This improves performance and correctness at scale by minimizing the time a read lock is held on the cache, reducing blockage of CacheController delta processing. It also helps lower workqueue processing time per object/key.

We did some benchmarking comparing indexing with simply listing the pods from the Store/Cache.

For 10k pods in store, using Index it took ~30 µs (vs) ~320 µs

For 500k pods in store, using Index it took ~35 µs (vs) 18.080762ms

We can tell that, listing from store using Index is 500X times better at higher scale.

Following are the results:

=== RUN   TestGetPodsForDaemonSetUsesIndexer
    daemon_controller_test.go:922: ByIndex() fetched 100 pods in 30.252µs
    daemon_controller_test.go:930: List() fetched 10000 pods in 320.047µs

=== RUN   TestGetPodsForDaemonSetUsesIndexer
    daemon_controller_test.go:922: ByIndex() fetched 100 pods in 37.842µs
    daemon_controller_test.go:930: List() fetched 500000 pods in 18.080762ms

Optimizing this list call will provide the following benefits:

• Reduce Read Lock Duration on the InformerCache, allowing writes to proceed faster and reducing chances of cache staleness at scale.
• Lower Work Queue Processing Time, enabling faster convergence to the desired state
• Decreased Queue Wait Time,, ensuring items are dispatched and processed more efficiently

You can refer to this PR: #130961

[xigang]

Please also post some release note and fix linter

@mimowo Done.

[xigang]

@mimowo For approve

[mimowo]

nit: ctx is unused here.

[xigang]

Nice catch!👍🏻

[mimowo]

nit, revert unnecessary formatting changes

[xigang]

Done.

[mimowo]

/hold
To clarify #132305 (comment)

[mimowo]

/unhold
/lgtm
/approve
as suggested by @wojtek-t

[k8s-ci-robot]

LGTM label has been added.

Git tree hash: 607be3aa28f793e5a026f7f158e94b11b49b9c6d

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mimowo, xigang

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• pkg/controller/job/OWNERS [mimowo]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment