-
Notifications
You must be signed in to change notification settings - Fork 32
topology-aware: try picking resources by hints first #545
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
topology-aware: try picking resources by hints first #545
Conversation
a7edac8
to
00a510a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With a very quick pass on this, looks sane to me 😅 With a potentially "might not be a good idea" I'd suggest to start with an off-by-default configuration option.
Spotted one typo in one commit message With multiple devices allocated to a since container
00a510a
to
6b88f9e
Compare
Yes, definitely. I'll put this behind an annotation so that it will be in effect only for containers annotated for it. And only take of the draft status once that commit is in place.
Thanks for spotting that! Fixed. |
Set defaults for agent config in values. This makes it easy to enable pod resource API by passing this to helm install --set config.agent.podResourceAPI=true Signed-off-by: Krisztian Litkey <[email protected]>
Allow checking if a topology hint is based on pod resource API. Signed-off-by: Krisztian Litkey <[email protected]>
Give more priority for topology hints than earlier, putting them right below annotated affinities. This will give hints priority over memory pinning tightness, which is preferable when a container allocates multiple devices with different memory locality. Hints now have precedence over annotated memory type. This might be a bit questionable, since hints are implied while memory type annotations are explicit. We probably can live with this for the time being. Hints can be selectively dis- abled per pod or container to restore the earlier behavior. Signed-off-by: Krisztian Litkey <[email protected]>
If a container asks for at least as many exsclusive CPUs as it has pod resource API hints, try allocating CPUs by hints first. With multiple devices allocated to a single container, this can help in cases where the collective locality of devices forces allocation high in the pool tree, where we should prefer CPUs with locality to one of the devices and avoid other CPUs. For instance, devices with locality to NUMA node #0 and #3, or 'half of' sockets #0 and #1, and a request for 2 CPUs, we end up in the root pool. But we should only prefer allocating CPUs with locality to NUMA nodes #0 or #3 and avoiding any CPU with locality to node #1 or #3. Signed-off-by: Krisztian Litkey <[email protected]>
If a container has pod resource API hints, try allocating from and pinning to memory from nodes which hints indicate locality to. Signed-off-by: Krisztian Litkey <[email protected]>
Only try to pick resources by hints, if a container is annotated for it using 'pick-resources-by-hints.resource-policy.nri.io'. Signed-off-by: Krisztian Litkey <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If NRT is now enabled by default on the agent, I wonder if we still need the Helm based gate for toggling it. Since exposing resources via NRT seems to be the expected default (at least in Helm deployments), maybe we can simplify things by removing that extra switch, unless we're still concerned about generating large CRs? Just a thought.
but otherwise LGTM, as you already mentioned the annotation name is something that can be improved but I can't really suggest anything better since naming is always hard. |
Signed-off-by: Krisztian Litkey <[email protected]>
5b0e265
to
7ea0bb5
Compare
I think NRT has already been on by default in the configuration. This was just implicit by leaving it out of the default configuration in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you.
This PR updates how topology hints are taken into account during resource allocation, especially when there are multiple devices with different HW locality allocated to a single container. In particular the PR
Notes: This behavioral change is currently not put behind a config option or an annotation yet. I intend to do that however, by pushing an additional commit.