Please remove the numbers from these comments

typos in the string, please also use a %s to pull in the actual name of the policy as stored in topologymanager.PolicySingleNumaNode instead of writing it out yourself.

fixed in latest commit

remove == true

I think part of the problem here is that this new flag probably belongs on the topologymanager as a "topology-manager-policy-option", not the CPUManager (for which we don't have any infrastructure yet). Let's leave it as is for the time being until we have time to evaluate if it makes sense moving this flag to the topologymanager in a future release (which may just have the opposite problem for the 2nd check going on which is CPUManager specific).

Why are you making a copy of the numaBits slice before passing it? Is it necessary for some reason?

Initially, i had some difficulty in calling SocketInNumaNodes since it is variadic function. Intent was to convert numBits into slice and then pass.
However, fixed in latest commit.

I agree it's nice to avoid else statements when possible, but I think this is fine for now. We can revisit this as a follow-up.

Is it really necessary to make a copy of the numaNodes slice before passing it in here?

fixed it in latest commit

Given the name it seems this should take a hint as a paramater, not a bitmask.

fixed function input parameter

This seems like a strange addition to an interface called Store. I never actually liked the name Store to begin with but it was part of the original design before I got involved. I feel fairly strongly that this new function needs to live elsewhere.

This is the interface which is passed to CPUManager from where i can deduce anything about Topology manager allowing me to fetch topology manager policy and perform validation checks in policy options for compatibility.

OK. I took a closer look at this.

I think the issue is that we should not have an interface called Store in the topologymanager, and instead should call this interface something more generic (I don't have a good suggestion off the top of my head though). Let's leave it in here for now as you have done, and do a follow-up PR to rename this interface to something more appropriate.

The way to avoid an if/else and make this a little cleaner here would be to introduce a new function as

func (p *staticPolicy) getAlignedCPUs(numaAffinity bitmask.BitMask) {
	alignedCPUs := cpuset.NewCPUSet()
	if p.options.AlignBySocket {
		socketBits := p.topology.CPUDetails.SocketsInNUMANodes(numaAffinity.GetBits()...).ToSliceNoSort()
		for _, socketID := range socketBits {
			alignedCPUs = alignedCPUs.Union(allocatableCPUs.Intersection(p.topology.CPUDetails.CPUsInSockets(socketID)))
		}
		return alignedCPUs
	}

	for _, numaNodeID := range numaBits {
		alignedCPUs = alignedCPUs.Union(allocatableCPUs.Intersection(p.topology.CPUDetails.CPUsInNUMANodes(numaNodeID)))
	}
	
	return alignedCPUs
}

And then at the callsite here, just do:

alignedCPUs := p.getAlignedCPUs(numaAffinity)

It's odd to put the defer in the middle of a for loop like this. Defers are function scoped and will only be run once this entire function exits. You are essentially adding a new function to run at function exit each time around the loop. Is this your intention here?

You will likely need to add a wrapping function around runStaticPolicyTestCase() called something like runStaticPolicyTestCaseWithFeatureGates() that sets up the necessary feature gates around the test.

fixed in latest commit

just call the variable tm if you don't want to spell it out completely. topoManager is awkward.

fixed in latest commit

I'm not a huge fan of introducing yet another NewFakeManager() function. What if we want a FakeManager with a hint and a policy set? We need to come up with something more generic.

Ideally there should be a generic FakeManager(). I added since we already have one withAffinity, however underlying structure is same. I can track it for beta to improve FakeManager()

Does this ever get exercised anyhwere? It looks like you just always set the policy to nil in all test cases.

Fixed in latest commit

Yes, this is correct. When setting this flag, think of a socket as a super-NUMA-node for the purpose of alignment. So long as the set of bits in the NUMA mask all point to the same socket, then alignment should be preferred.

Again -- it's weird to have this here in the middle of the function like this. If you want it set during this test, jut put it at the top of the function, not in the middle.

Fixed in latest commit

At some point it would be good to add unit tests in pkg/kubelet/cm that can test across all of the hint-providers (with their various options) instead of just testing each of them in isolation like this.

Will track this item for beta.
I think testcases under pkg/kubelet/cm are definitely required and should be tracked via separate issue

Can you add one more test case that ensures this is not available if CPUManagerPolicyBetaOptions is set.

Fixed!

Is it sufficient to just check Size() == 1? What if minAffinitySize is greater than the number of numa nodes on a socket?

Should this really be something like:

minSockets := (minAffinitySize + numaNodesPerSocket - 1) / numaNodesPerSocket
return p.topology.CPUDetails.SocketsInNUMANodes(numaNodesBitMask...).Size() == minSockets

We should also add a test case for this.

Can you give an example topology where above will apply?

It is independent of the topology. It depends on the request size. Imagine a 2 socket machine with 4 NUMA nodes per socket and 20 CPUs per NUMA node. If a request comes in for 81 CPUs, this requires at least 5 NUMA nodes (meaning minAffinity will be 5) and (by definition) at least 2 sockets.

The thought process i had is:
If allocation need to spread across two sockets, CPUs by definition are not socket aligned. Assumption is by socket alignment, we mean CPUs from one socket are preferred for allocation over cross-socket allocation.
In 2 socket architecture, With above logic, if allocation spreads two sockets then all hints become preferred and the whole point of finding optimized hint may become moot.

In my view, considering minSockets may be relevant for architecture where there are more than 2 sockets(viz 4 socket or 6 sockets etc) where we want the CPUs to be localized to let say 2 sockets.

Intent is to have simplicity, in sense that CPU's will be considered aligned if they can come from one-socket. In my opinion, Making align-by-socket to consider CPUs as aligned within minAffinitySockets(2 or more ) may bring complexity whose performance returns may not be much. Since anyway allocation is anyway spreading across sockets

I could be wrong and open to suggestions.

If allocation need to spread across two sockets, CPUs by definition are not socket aligned.

This is true, but I don't see how that is relevant here. The point of the align-by-socket flag is not to try and force alignment to a single socket, but rather to generate hints at the socket boundary rather than the NUMA boundary.

If an allocation request has so many CPUs that it must always pull from more than one socket to satisfy its allocation (meaning, the topology dictates this, not the current allocation to other pods), then we need to generate a preferred hint that is representative of this.

Not doing so would make this flag inconsistent with its purpose -- i.e. to generate hints as if we were working at socket boundaries instead of NUMA boundaries.

It's the reason we need to do this loop on the other side (instead of just assuming things always come from one socket):

	if p.options.AlignBySocket {
		socketBits := p.topology.CPUDetails.SocketsInNUMANodes(numaBits...).ToSliceNoSort()
		for _, socketID := range socketBits {
			alignedCPUs = alignedCPUs.Union(allocatableCPUs.Intersection(p.topology.CPUDetails.CPUsInSockets(socketID)))
		}
		return alignedCPUs
	}

Fixed in latest commit

nit: line break here

Fixed in latest commit

I would just remove the panic here and ignore the error. If an erorr is returned here as part of test declaration we have bigger issues.

Fixed in latest commit

nit: line break after this

Fixed in latest commit

Remove policy from here

Fixed in latest commit

Technically this can never happen because we gate calling this function by p.options.AlignBySocket which will error out at startup if p.topology.NumNUMANodes > p.topology.NumSockets.

I thin it's fine to leave it here for completeness though.

Yeah! Prefer to leave it there, In future other functions might leverage isHintSocketAligned and we may run into div by 0 issues

Does this mean that if the AlignBySocket option is enabled, the number of NUMA nodes in the final CPU allocation may not be minimal (although there may be CPU allocations with fewer NUMA nodes, and within a socket)?

Yes. It may not be minimal.

This option makes it so that allocation decisions are made at the socket boundary, rather than the NUMA node boundary. All CPUs will come from the same socket, but not necessarily the same NUMA node.

Would it be better to also consider the number of NUMA nodes if the AlignBySocket option is enabled? First the CPUs should be in a socket, then the minimum number of NUMA nodes.

That is not guarantees by the API (because if you are using this setting, you should not be caring about NUMA alignment, but only socket alignment), but under the hood, CPUs will in fact be handed out on one NUMA node first before moving onto the next.

This ensures all CPUs in the same socket are considered aligned:

And this (depending on the setting of DistributeCPUsAcrossNUMA) will make sure to pull those CPUs from a minimal set of NUMA nodes:

Maybe I got it. Does this mean that the final allocated CPUs in the Allocate() method may be different from the TopologyHint returned by the GetTopologyHints() method and merged by the Topology Manager?

Assume NUMA node 0 and NUMA node 1 are on the same socket and they both have 2 CPUs. AlignBySocket is enabled while DistributeCPUsAcrossNUMA is not enabled. An incoming container requests 2 CPUs.

1. In the GetTopologyHints() method, the returned TopologyHints are {{01, true}, {10, true}, {11, true}}
2. In Topology Manager, the merged result is {11, true}
3. In the Allocate() method, the final allocated CPUs to the container are both taken from NUMA node 0.

Is my understanding correct?