Is not obvious to me what do we gain by wrapping the static policy. Could you please elaborate a bit?

For Windows we want the new BestEffort policy to have the same logic as the existing static policy.
I figured that doing this would reduce a lot of code duplication.

I can update the comments here to reflect that or maybe I can have the kubelet arguments specify BestEffortPolicy and then create a staticPolicy instance here for Windows.

Do you all have a preference @ffromani @jsturtevant ?

Updating comments and wrapping seems to make sense to me. This will allow to adjust more easily if we learn of reasons to adjust.

I'll need to refresh my memory over what we discussed in the KEP before I can answer. Right now I can surely say comments is a great starting point.

ok, first: I've nothing against reusing the code like this. We didn't do often in the codebase, but why not?
The issue I see however is that from my interpretation of the KEP I would have expected different hint generation on windows based on https://github.com/kubernetes/enhancements/tree/master/keps/sig-windows/4885-windows-cpu-and-memory-affinity#windows-memory-considerations

I think we can settle this with a good set of testcases, at very least unit tests. which seems lacking to me?
Granted, the policy is actually a thin wrapper over the static policy, which has its own tests; the tests I recommend to add are to ensure and document the expected behavior on windows.

The tests should cover preferably also the topology manager merge hint process. It will be similar in spirit to what you are adding to internal_container_lifecycle_windows_test.go, but covering the generic topology manager flow.

There's likely not enough time to do this for this cycle. Plus I want to be practical: it's an alpha feature, disabled by default, and the early feedback from having some functionality in is important as well. So I don't think it is blocking, but I DO think this is beta-blocking (to discuss this topic in detail at least) and possibly we can need another alpha iteration to sort this out depending on the aforementioned feedback

The issue I see however is that from my interpretation of the KEP I would have expected different hint generation on windows based on https://github.com/kubernetes/enhancements/tree/master/keps/sig-windows/4885-windows-cpu-and-memory-affinity#windows-memory-considerations

I didn't see a difference in what we would provide for a hint. The major difference is in the semantic meaning of the hint, which on Windows we can't guarantee that it the hint will be respected, only make that suggestion. What kind of difference are you thinking?

There's likely not enough time to do this for this cycle. Plus I want to be practical: it's an alpha feature, disabled by default, and the early feedback from having some functionality in is important as well. So I don't think it is blocking, but I DO think this is beta-blocking (to discuss this topic in detail at least) and possibly we can need another alpha iteration to sort this out depending on the aforementioned feedback

I appreciate this sentiment. We do have a user we are working with that is willing to provide feedback but is looking for it to be in an Alpha release. From my understanding of the challenges on Linux, these types of features need some real feedback to get them dialed in, and I anticipate we will get feedback that we will need to incorporate back into the design.

The issue I see however is that from my interpretation of the KEP I would have expected different hint generation on windows based on https://github.com/kubernetes/enhancements/tree/master/keps/sig-windows/4885-windows-cpu-and-memory-affinity#windows-memory-considerations

I didn't see a difference in what we would provide for a hint. The major difference is in the semantic meaning of the hint, which on Windows we can't guarantee that it the hint will be respected, only make that suggestion. What kind of difference are you thinking?

I'm basing my expectations on what I saw so far on linux for the cpumanager policy. I would like to see if for example returning a different set of NUMA bitmasks or different values for Preferred field.

If we add a thin wrapper over the Static policy, we are saying that this policy should behave 1:1 like the static policy, and we are unnecessarily coupling the behavior. What I would rather see is a specification (and unit tests are a good vehicle for that) of the expected behavior, more precise than (yes, I'm oversimplifying here) "whatever Static policy does, but not enforcing".

Lacking specification is something that biten us already multiple already, so we should IMO go in the direction to have the spec spelled out more precisely, and the testcase well defined. If, after that, we can reuse the implementation, great.

If nothing else (which I can accept!) this should be discussed and elaborated in the KEP design details.
I'm happy to elaborate further in the next cycle. Again, more details in the KEP to elaborate more precisely the behavior can possibly be all we need.

There's likely not enough time to do this for this cycle. Plus I want to be practical: it's an alpha feature, disabled by default, and the early feedback from having some functionality in is important as well. So I don't think it is blocking, but I DO think this is beta-blocking (to discuss this topic in detail at least) and possibly we can need another alpha iteration to sort this out depending on the aforementioned feedback

I appreciate this sentiment. We do have a user we are working with that is willing to provide feedback but is looking for it to be in an Alpha release. From my understanding of the challenges on Linux, these types of features need some real feedback to get them dialed in, and I anticipate we will get feedback that we will need to incorporate back into the design.

Fine, so we will evaluate during the 1.33 cycle if we can move to beta (taking into account my oen feedback above) or if we need another alpha iteration.

The rest of the code overall LGTM, I'll have another pass ASAP but from my recollections I don't have anything major on my notes.

ok, first: I've nothing against reusing the code like this. We didn't do often in the codebase, but why not?
The issue I see however is that from my interpretation of the KEP I would have expected different hint generation on windows based on https://github.com/kubernetes/enhancements/tree/master/keps/sig-windows/4885-windows-cpu-and-memory-affinity#windows-memory-considerations

I think we can settle this with a good set of testcases, at very least unit tests. which seems lacking to me?
Granted, the policy is actually a thin wrapper over the static policy, which has its own tests; the tests I recommend to add are to ensure and document the expected behavior on windows.

The tests should cover preferably also the topology manager merge hint process. It will be similar in spirit to what you are adding to internal_container_lifecycle_windows_test.go, but covering the generic topology manager flow.

There's likely not enough time to do this for this cycle. Plus I want to be practical: it's an alpha feature, disabled by default, and the early feedback from having some functionality in is important as well. So I don't think it is blocking, but I DO think this is beta-blocking (to discuss this topic in detail at least) and possibly we can need another alpha iteration to sort this out depending on the aforementioned feedback