nfd-topology-updater: Detect E/P cores and expose through attributes

[ozhuraki]

Detect which CPUs are which types of the cores (P-cores or E-cores) and expose IDs through labels.

[k8s-ci-robot]

Hi @ozhuraki. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

[netlify]

✅ Deploy Preview for kubernetes-sigs-nfd ready!

Name	Link
🔨 Latest commit	2946157
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-nfd/deploys/67599acefb726600082bb9a9
😎 Deploy Preview	https://deploy-preview-1737--kubernetes-sigs-nfd.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[marquiz]

Thanks for the patch @ozhuraki. I have hard time seeing the practical usefulness of this this information. I mean how is it possible to meaningfully consume these properties – I think they definitely should not be exposed as built-in/default node labels.

A more useful strategy could be to add this information as attributes in the NodeResourceTopology object that the nfd-topology-updater handles. This way, the E/P core information could be consumed by a Kubernetes (topology-aware) scheduler extension.

ping @kad

[kad]

I have several concerns regarding this PR:

1. way of detecting cores. We should use /sys/devices/cpu_*/*cpus for getting information about P/E cores. not something else.
2. Those subdirs cpu_* are present only on hybrid CPUs, so if we have only one subdir /sys/devices/cpu/, it means only one type of the cores exists in the package, thus no need to expose those attributes at all. In other words: labels should be exposed only if CPU is hybrid.
3. Even on hybrid CPUs, those labels should not be by default populated, only when configuration requests that. I agree with Markus that those labels are not directly consumable, so extra step to enable them on hybrid CPUs is fine.
4. Exposing it as attributes in topology exporter is fine. There we can put them without configuration option if running on hybrid CPUs.
5. Names of the types of the cores should not be hard-coded, but taken from sysfs. e.g.

$ grep "" /sys/devices/cpu_*/*cpus
/sys/devices/cpu_atom/cpus:16-19
/sys/devices/cpu_core/cpus:0-15
$

so, the labels should have "foo..atom" and "foo...core" names. @marquiz please suggest some good prefix instead of "foo..."

[ozhuraki]

@marquiz @kad

Thanks for the helpful input. Updated, please take a look

[ozhuraki]

@marquiz

Thanks for the useful input! Updated, please take a look

[ozhuraki]

@marquiz @kad

Thanks for the help! Updated, please take a look

[marquiz]

Thanks for the update. A few more comments below.
/ok-to-test
/retitle nfd-topology-updater: Detect E/P cores and expose through attributes

[ozhuraki]

@marquiz

Thanks, updated, please take a look

[ozhuraki]

@marquiz

Thanks, updated, please take a look

[marquiz]

Thanks @ozhuraki for the update, I think we could merge this.
/assign @PiotrProkop

One general note about documentation that it would be nice to have e.g. a table listing all the information that we expose as attributes in the NRT. Maybe we should create an issue about this(?)

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: marquiz, ozhuraki

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [marquiz]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[marquiz]

/retest

[PiotrProkop]

One question and sorry if I misunderstand how hybrid cpus can be consumed in K8s. Wouldn't be more useful to just modify ResourceAggregator to advertise new resources for each cpu type in each Zone (NUMA node) by mapping generic k8s cpu resource to each cpu_type? But I may be wrong as I don't know how currently those E/P cores are assigned and consumed in Kubernetes.

[ozhuraki]

@PiotrProkop

There's nothing special about it, just one way of exposing such information.

We can also modify ResourceAggregator to advertise new resources for each cpu type too.

I will make an issue and will try to add it with a sepatate PR.

[PiotrProkop]

I just don't know how this information provided via attributes can be useful to custom scheduler without mapping to already used cpus. NRT object only exposes info about allocated/free cpus in given Zone without an info which cpu id is used and which is free.

[uniemimu]

Found typo

[ozhuraki]

@PiotrProkop

Thanks and sorry, missed you comment.

Yes, it's merely exposing an information for particuar kind of cores for workloads that just want particular kind of cores on hybrid cpus.

I made an issue #1964 to expose it through ResourceAggregator (so the custom scheduler could track allocated/free cores) and will try to add such PR too.

@uniemimu

Thanks, updated.

[PiotrProkop]

I still feel like this information should be then exposed via labels and just say that given node has e or p cores without the numbers. And then no special scheduler plugin is needed but I may be wrong on what's the usecase here. I'll ask @ffromani to chime in.

[ozhuraki]

@PiotrProkop

We initially had it through labels but later switched to annotations since labels were restrictive on exposing IDs.

I can additionaly expose via labels and just say that given node has E or P, as you suggest.

[ozhuraki]

@PiotrProkop

Added labels too, please take a look.

[k8s-ci-robot]

@ozhuraki: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-node-feature-discovery-build-image-cross-generic	2946157	link	true	/test pull-node-feature-discovery-build-image-cross-generic

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

[ffromani]

/cc

[ffromani]

got the ping, will review ASAP