CA DRA: remove NodeInfo wrapping

[towca]

Which component are you using?

/area cluster-autoscaler
/area core-autoscaler

Problem description:

Before Cluster Autoscaler was integrated with DRA, ClusterSnapshot would directly store the scheduler-framework NodeInfo objects, and CA code would operate on these scheduler-framework objects.

In order to support DRA, CA had to start tracking ResourceSlices and ResourceClaims in addition to just Nodes and Pods. The scheduler NodeInfo type doesn't contain them, and back then it was just a struct, not an interface - so we couldn't directly extend it. Instead, we introduced an internal NodeInfo and PodInfo types - wrapping the scheduler structs, and adding CA-specific fields on top:

autoscaler/cluster-autoscaler/simulator/framework/infos.go

Line 47 in 91bae6b

type NodeInfo struct {

Scheduler NodeInfos are tracked in ClusterSnapshotStore:

autoscaler/cluster-autoscaler/simulator/clustersnapshot/store/delta.go

Line 54 in 91bae6b

type DeltaSnapshotStore struct {

ClusterSnapshotStore also contains dynamicresources/snapshot.Snapshot which tracks DRA objects:

autoscaler/cluster-autoscaler/simulator/dynamicresources/snapshot/snapshot.go

Line 48 in 91bae6b

type Snapshot struct {

Whenever scheduler plugins list (the scheduler version of) NodeInfos or DRA objects through the scheduler framework, they are listed directly from ClusterSnapshotStore and dynamicresources/snapshot.Snapshot with no memory allocations.

On the other hand, Cluster Autoscaler code always lists/gets the CA-specific version of NodeInfo. Since parts of the object live in different places (ClusterSnapshotStore and dynamicresources/snapshot.Snapshot), the internal NodeInfos always get computed on-the-fly, wrapping both parts:

autoscaler/cluster-autoscaler/simulator/clustersnapshot/predicate/predicate_snapshot.go

Line 54 in 91bae6b

func (s *PredicateSnapshot) GetNodeInfo(nodeName string) (*framework.NodeInfo, error) {

This results in allocating memory for the internal NodeInfo every time it's accessed from ClusterSnapshot. After we added parallelism to binpacking in #8729, it turned out after benchmarking that these memory allocations are now a performance bottleneck.

The scheduler-framework NodeInfo was changed to an interface in K8s 1.34, so we can now return the internal NodeInfos directly to scheduler code - as long as they implement the interface. Since the internal NodeInfo wraps the scheduler one, most of the interface methods can just be passed through to the wrapped scheduler struct.

Proposed solution:

High-level overview:

• Make internal NodeInfo implement the scheduler-framework NodeInfo interface, so that it can be directly returned and used by the scheduler framework.
• Change the "storage version" in ClusterSnapshotStore from scheduler NodeInfo/PodInfo to CA-internal NodeInfo/PodInfo. Have ClusterSnapshotStore directly provide the CA-internal objects to both CA and scheduler-framework without any wrapping/memory allocations.
• Modify ClusterSnapshotStore interface and implementations so that any time the internal NodeInfo/PodInfo objects are allocated during its methods, they have the CA-specific fields correctly set using DRA snapshot before storing.
• Modify ClusterSnapshot interface and its PredicateSnapshot implementation to delegate NodeInfo getting/listing to the embedded ClusterSnapshotStore.

Detailed plan:

• Implement the scheduler-framework NodeInfo interface for the CA-internal NodeInfo struct, passing most methods through to the embedded scheduler-framework NodeInfo implementation.
  • Handling PodInfo will be tricky - the internal NodeInfo.Pods() should return a list of internal PodInfo objects without allocating them on the fly. So we can't just pass this method through to the scheduler NodeInfo.Pods(). We have to actually embed the scheduler PodInfo in the internal PodInfo and implement the PodInfo interface, keep a list of internal PodInfos in the internal NodeInfo, and return that from the internal NodeInfo.Pods().
• Modify ClusterSnapshotStore interface and both implementations (BasicSnapshotStore and DeltaSnapshotStore):
  • Store the internal NodeInfos instead of the scheduler version in the implementations. The ClusterSnapshotStore.NodeInfos() method exposing the NodeInfos to scheduler code should keep working as-is, because the internal NodeInfo implements the correct interface.
  • AddSchedulerNodeInfo and RemoveSchedulerNodeInfo should be changed to operate on the internal NodeInfo, and renamed to something like ForceAddNodeInfo/ForceRemoveNodeInfo. The internal NodeInfo provided to ForceAddNodeInfo() should already have all the CA-specific parts filled in, so it can just be stored as-is.
  • Move the logic that allocates internal NodeInfo objects including all the CA-specific parts from on-the-fly in PredicateSnapshot.GetNodeInfo()/ListNodeInfos(), to once-per-loop in ClusterSnapshotStore.SetClusterState().
  • Change ForceAddPod() to allocate an internal PodInfo and include all the DRA-related information (taken from the DRA snapshot) before storing it in a NodeInfo.
• Modify ClusterSnapshot interface and implementation (PredicateSnapshot):
  • Move GetNodeInfo() and ListNodeInfos() methods from ClusterSnapshot to ClusterSnapshotStore. Implement the new methods in both ClusterSnapshotStore implementations by just getting/listing the stored internal NodeInfos without any wrapping.
  • Change AddNodeInfo() to call the new ClusterSnapshotStore.ForceAddNodeInfo() and pass the whole internal NodeInfo to it.
  • Change RemoveNodeInfo to call the new ClusterSnapshotStore.ForceRemoveNodeInfo().

Mental model changes

Currently, a good mental model for Cluster Snapshot and DRA is something like this:

• ClusterSnapshotStore is the single storage source-of-truth for Nodes and Pods.
• ClusterSnapshotStore.DraSnapshot() is the single storage source-of-truth for DRA objects.
• ClusterSnapshot.GetNodeInfo()/ListNodeInfos() correlates Nodes and Pods the DRA objects, and returns the CA-internal NodeInfo objects combining both of them. The result is "ephemeral": read-only, only valid until the next ClusterSnapshot mutation, shouldn't be stored.
• Any time ClusterSnapshot is mutated, ClusterSnapshotStore and the embedded ClusterSnapshotStore.DraSnapshot() are mutated independently. The previous results of GetNodeInfo()/ListNodeInfos() are now invalid, so we don't care about "synchronizing" them with the mutation. For each tracked object, we only change the single source of truth it's stored in.

The proposed changes make the mental model more complex:

• ClusterSnapshotStore.DraSnapshot() still stores the DRA objects and is still the source-of-truth conceptually, but it's not the only place where the DRA objects are stored.
• ClusterSnapshotStore stores fully-formed CA-internal NodeInfo objects - so Nodes, Pods, and DRA objects. It's still the single storage source-of-truth for Nodes and Pods.
• DRA objects are stored in 2 places now - ClusterSnapshotStore.DraSnapshot(), and the CA-internal NodeInfos tracked in ClusterSnapshotStore. We can't get away from having a separate DRA snapshot because there are objects to track that aren't bound to a particular Node or Pod. We also can't get away from having the DRA objects in the internal NodeInfos stored by ClusterSnapshotStore - either they're persisted there, or we're back to allocating new NodeInfo objects on the fly.
• We still don't care about synchronizing mutations to the ephemeral results of GetNodeInfo()/ListNodeInfos(), but we now need to synchronize any mutations between ClusterSnapshotStore.DraSnapshot() and the CA-internal NodeInfos tracked in ClusterSnapshotStore. In return, any time we get a NodeInfo from ClusterSnapshotStore, we can just return it with no extra logic.

I couldn't come up with a solution that doesn't make the mental model more complex at all, and at least in the proposed solution the new complexity is limited to ClusterSnapshot and ClusterSnapshotStore implementations. The expectations for the rest of the code base shouldn't change.

Additional context:

• Add csinode limit awareness in cluster-autoscaler #8721 adds more fields to the CA-internal NodeInfo, and a new CSISnapshot for filling them to ClusterSnapshotStore. It'll probably land before this issue is fixed, so the parts that mention "DRA" in the implementation plan above will become "DRA and CSINode". Both work the same way, so the same solution should be fine.

[Choraden]

/assign @Choraden

[mtrqq]

Couple comments to the proposal @towca

1. Move GetNodeInfo() and ListNodeInfos() methods from ClusterSnapshot to ClusterSnapshotStore. Implement the new methods in both ClusterSnapshotStore implementations by just getting/listing the stored internal NodeInfos without any wrapping.

   Do you still see value to keep GetNodeInfo() and ListNodeInfos() methods? To me they feel obsolete if we are already storing complete objects inside the snapshot structs. Previously they were added so that predicate snapshot can augment data provided from ClusterSnapshotStore with DRA data, now the results should map 1:1 to NodeInfos().List() / NodeInfos().Get().

2. Change ForceAddPod() to allocate an internal PodInfo and include all the DRA-related information (taken from the DRA snapshot) before storing it in a NodeInfo.

   Modifying ForceAddPod() in ClusterSnapshotStore seems to be breaking the existing pattern of handling the connection between DRA and nodes/pod on the predicate snapshot level and takes away the ability to disable the DRA enablement (which may be needed in case of temporary breakage mitigation). I would rather keep having those mutations happening in predicate snapshot and have a method override for it there - that way we'll also fix the problem in filter_out_expendable.go not caring about DRA objects.

3. We still don't care about synchronizing mutations to the ephemeral results of GetNodeInfo()/ListNodeInfos(), but we now need to synchronize any mutations between ClusterSnapshotStore.DraSnapshot() and the CA-internal NodeInfos tracked in ClusterSnapshotStore

   Once this change is done - the results of GetNodeInfo()/ListNodeInfos() would be no longer be ephemeral and will represent pointers to the actual objects stored in the state. I don't think we should introduce some mechanism to prevent that, but it would be good to review existing parts of the code and validate that they are not modifying NodeInfos inplace and do not actually depend on the snapshot returning ephemeral object. This may lead to obscure bugs being introduced into CA which are extremely hard to debug. Do you agree with that of I'm missing something?

4. Do we also want to get rid WrapSchedulerNodeInfo() method(s) as part of this change? It doesn't seem to be needed anymore after this change is implemented

[towca]

1. Do you still see value to keep GetNodeInfo() and ListNodeInfos() methods? To me they feel obsolete if we are already storing complete objects inside the snapshot structs. Previously they were added so that predicate snapshot can augment data provided from ClusterSnapshotStore with DRA data, now the results should map 1:1 to NodeInfos().List() / NodeInfos().Get().

NodeInfos() will only expose the scheduler-framework NodeInfo interface. CA code would then have to cast it to the internal type in order to access the CA-specific parts of it. The getters solve this problem, so IMO there's definitely still value in them.

2. Modifying ForceAddPod() in ClusterSnapshotStore seems to be breaking the existing pattern of handling the connection between DRA and nodes/pod on the predicate snapshot level and takes away the ability to disable the DRA enablement (which may be needed in case of temporary breakage mitigation). I would rather keep having those mutations happening in predicate snapshot and have a method override for it there - that way we'll also fix the problem in filter_out_expendable.go not caring about DRA objects.

Very good point, I agree this logic should live in PredicateSnapshot, it'll be identical for both stores. So then how about we change ClusterSnapshotStore.ForceAddPod(pod *apiv1.Pod, nodeName string) into something like ClusterSnapshotStore.StorePodInfo(podInfo *framework.PodInfo, nodeName string) (and similarly we'd introduce StoreNodeInfo() etc)? These new methods would only be used by the PredicateSnapshot, and we'd move the ForceAddPod() method to PredicateSnapshot.

ForceAddPod() will still be needed (at least initially), to handle the places that don't check scheduler predicates before adding a Pod to a Node in the snapshot. The problem with the filter_out_expendable.go logic you link is described in detail in #7683. Tl;dr is that it's not clear what CA should do for these Pods - not checking scheduler filters can lead to subtle problems with not properly counting a DRA device as allocated, and checking scheduler filters can lead to CA doing unnecessary scale-ups (which the logic is supposed to prevent in the first place). The problem is in deciding what CA should do - implementing whatever we decide should be fairly trivial after that, even without this proposal.

Btw @mtrqq, the ClusterSnapshotStore layer is becoming really redundant with all the recent changes around ClusterSnapshot. We could migrate the store layer to a PatchSet, and PredicateSnapshot would just directly operate on the PatchSets: one for NodeInfo, one for DRA, one for CSI - treating them very similarly. The NodeInfo PatchSet would still be special - the others would be "synchronized into" it, and it would be "nested" (Pods are nested under Nodes, which isn't true for the other PatchSets). But IMO that would be a huge improvement to readability/complexity (not to mention the potential performance improvements from moving to the different caching strategy in PatchSet).

3. Once this change is done - the results of GetNodeInfo()/ListNodeInfos() would be no longer be ephemeral and will represent pointers to the actual objects stored in the state. I don't think we should introduce some mechanism to prevent that, but it would be good to review existing parts of the code and validate that they are not modifying NodeInfos inplace and do not actually depend on the snapshot returning ephemeral object. This may lead to obscure bugs being introduced into CA which are extremely hard to debug. Do you agree with that of I'm missing something?

I think in practice, if some logic were to mutate the results of GetNodeInfo()/ListNodeInfos(), it would most likely mutate the underlying Node object (e.g. add a label to the Node), or the scheduler NodeInfo (e.g. add a Pod to the Node). Modifications to the CA-specific parts of NodeInfo seem much less likely. And modifications to the Node/scheduler NodeInfo parts would result in the bugs you describe even without the proposed changes, right?

Thanks for raising this though - I definitely agree we should review the existing callsites at least somewhat. But I think we're most likely not going to find anything breaking.

4. Do we also want to get rid WrapSchedulerNodeInfo() method(s) as part of this change? It doesn't seem to be needed anymore after this change is implemented

Yes, they should be obsolete after the proposed changes.

[mtrqq]

NodeInfos() will only expose the scheduler-framework NodeInfo interface. CA code would then have to cast it to the internal type in order to access the CA-specific parts of it. The getters solve this problem, so IMO there's definitely still value in them.

Ahh, thanks for clarification, I thought that they still return scheduler types, but return type of framework.NodeInfo != fwk.NodeInfo, this slightly confused me

Very good point, I agree this logic should live in PredicateSnapshot, it'll be identical for both stores. So then how about we change ClusterSnapshotStore.ForceAddPod(pod *apiv1.Pod, nodeName string) into something like ClusterSnapshotStore.StorePodInfo(podInfo *framework.PodInfo, nodeName string) (and similarly we'd introduce StoreNodeInfo() etc)? These new methods would only be used by the PredicateSnapshot, and we'd move the ForceAddPod() method to PredicateSnapshot.

Doing such a split seems beneficial to me and I like the naming, +1 here

Btw @mtrqq, the ClusterSnapshotStore layer is becoming really redundant with all the recent changes around ClusterSnapshot. We could migrate the store layer to a PatchSet, and PredicateSnapshot would just directly operate on the PatchSets: one for NodeInfo, one for DRA, one for CSI - treating them very similarly. The NodeInfo PatchSet would still be special - the others would be "synchronized into" it, and it would be "nested" (Pods are nested under Nodes, which isn't true for the other PatchSets). But IMO that would be a huge improvement to readability/complexity (not to mention the potential performance improvements from moving to the different caching strategy in PatchSet).

These were exactly my thoughts when I've been looking at the interface differences between ClusterSnapshotStore and ClusterSnapshot interfaces, even though I've been recently working with this part of the code base - I still needed to do the mental exercise to understand which type corresponds to which interface.

I think this is a great idea to have PredicateSnapshot to orchestrate with, while the storage layer would be handled by the patchsets. My only worry is that PatchSet doesn't provide a good way for users to influence its caching strategy, IMO - basic assumptions work for the most cases and it performs quite well for general scenarios, but may not be the best for some usage patterns. In more complex cases (e.g. havePodsWithAffinity / havePodsWithRequiredAntiAffinity / pvcNamespaceMap) - we'll need to handle them within the predicate snapshot (what I'd probably try to avoid as it's already doing quite a bit of work) or make a specialized patchset for node infos with minimal interface changes to handle such operations effectively

Also while doing such a transition - do we want to drop basic snapshot store completely? At this point it's the reference implementation which is only used in tests, but seems to be bringing unnecessary abstraction and complexity. I would say that it's somewhat useful if there was a flag in CA to switch between delta and basic snapshots, but there's no option like that as this point as delta snapshot is getting instantiated in all the cases (which is logical)

And modifications to the Node/scheduler NodeInfo parts would result in the bugs you describe even without the proposed changes, right?

Fair call, underlying objects would be modified regardless of our changes, those are stored as pointers and being carried over between multiple ListNodeInfos() while internal objects are ephemeral.

[Choraden]

NodeInfos() will only expose the scheduler-framework NodeInfo interface. CA code would then have to cast it to the internal type in order to access the CA-specific parts of it. The getters solve this problem, so IMO there's definitely still value in them.

Ahh, thanks for clarification, I thought that they still return scheduler types, but return type of framework.NodeInfo != fwk.NodeInfo, this slightly confused me

The problem I see is []framework.NodeInfo can't be returned as []fwk.NodeInfo (scheduler) before manual conversion that allocates memory:

type internalDeltaSnapshotData struct {
    ...
    nodeInfoList *[]framework.NodeInfo
}

// List returns list of all node infos.
func (snapshot *deltaSnapshotStoreNodeLister) List() ([]fwk.NodeInfo, error) {
	return snapshot.data.nodeInfoList, nil // <- This won't compile.
}

Would need to do something like:

func toSchedulerNodeInfoList(nodeInfos []*framework.NodeInfo) []fwk.NodeInfo {
	result := make([]fwk.NodeInfo, len(nodeInfos))
	for i := range nodeInfos {
		result[i] = nodeInfos[i]
	}
	return result
}

My question is how frequently is that fwk.Lister used and if we can afford to have that extra allocation there? @towca @mtrqq