Skip to content
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.

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

Exposing TopologyManagerScope as a separate field #22

Closed
PiotrProkop opened this issue Nov 23, 2022 · 5 comments
Closed

Exposing TopologyManagerScope as a separate field #22

PiotrProkop opened this issue Nov 23, 2022 · 5 comments
Milestone

Comments

@PiotrProkop
Copy link
Contributor

Is there any reason why TopologyManagerScope was not exposed as separate field?
I would like to implement a Scoring Scheduler plugin that will favor nodes that requires least NUMA nodes to satisfy POD's resource requirements and I would like to do it when using restricted or best-effort policy.

@PiotrProkop
Copy link
Contributor Author

@swatisehgal @fromanirh can you take a look?

@ffromani ffromani added this to the v1beta1 milestone Nov 23, 2022
@ffromani
Copy link
Contributor

Hi @PiotrProkop . I will dig in the history to check the reasoning at time. In general I think this is a very fair ask to evaluate. The only drawback (of sorts) is adding the scope will probably fall in the v1beta1 bucket.
At this stage we're planning the content to evaluate to move to v1beta, there's no deadline yet.

@PiotrProkop
Copy link
Contributor Author

Thanks!

@swatisehgal
Copy link
Contributor

Hi @PiotrProkop . I will dig in the history to check the reasoning at time. In general I think this is a very fair ask to evaluate. The only drawback (of sorts) is adding the scope will probably fall in the v1beta1 bucket. At this stage we're planning the content to evaluate to move to v1beta, there's no deadline yet.

IIRC, it was because when we started working on this, Topology Manager scope didn't even exist! We designed the API such that topologyPolicies would take in only exisiting Topology Manager policies (none, restricted, single-numa-nodde). Later, when the feature was added we decided to continue using the same field and coupled the policy with the scope :(

I agree it is a fair ask to decouple policy from scope. It probably makes sense to figure out a way for us to represent the topologyPolicies in a more generic way to not only explicitly capture the Topology Manager scope but also the newly introduced policy options. Was thinking on the lines of a map so we have a kubelet flag as the key and value capturing the configured kubelet config value.

@ffromani
Copy link
Contributor

ffromani commented Feb 1, 2023

we are addressing this need using top-level Attributes added in v1alpha2: please see #24 and #25

@ffromani ffromani closed this as completed Feb 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants