Skip to content

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

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+41 −0
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 31 additions & 0 deletions pkg/nfd-topology-updater/nfd-topology-updater.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import (
"net/url"
"os"
"path/filepath"
"strings"

"golang.org/x/net/context"

Expand All @@ -42,6 +43,7 @@ import (
"sigs.k8s.io/node-feature-discovery/pkg/resourcemonitor"
"sigs.k8s.io/node-feature-discovery/pkg/topologypolicy"
"sigs.k8s.io/node-feature-discovery/pkg/utils"
"sigs.k8s.io/node-feature-discovery/pkg/utils/hostpath"
"sigs.k8s.io/node-feature-discovery/pkg/utils/kubeconf"
"sigs.k8s.io/node-feature-discovery/pkg/version"
"sigs.k8s.io/yaml"
Expand Down Expand Up @@ -337,6 +339,32 @@ func (w *nfdTopologyUpdater) updateNodeResourceTopology(zoneInfo v1alpha2.ZoneLi
return nil
}

// Discover E/P cores
func discoverCpuCores() v1alpha2.AttributeList {
attrList := v1alpha2.AttributeList{}

cpusPathGlob := hostpath.SysfsDir.Path("sys/devices/cpu_*/cpus")
cpuPaths, err := filepath.Glob(cpusPathGlob)
if err != nil {
klog.ErrorS(err, "error reading cpu entries", "cpusPathGlob", cpusPathGlob)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what if the path doesn't exist? I would expect to be a relatively common occurrence on non-hybrid archs (including older Intel archs). So I think we want

  1. to swallow the error if the error IsNotFound or similar; can happen, it's ok
  2. to complain very loudly if it's an actual read error: this is more likely something serious

return attrList
}

for _, entry := range cpuPaths {
cpus, err := os.ReadFile(entry)
if err != nil {
klog.ErrorS(err, "error reading cpu entry file", "entry", entry)
} else {
attrList = append(attrList, v1alpha2.AttributeInfo{
Name: filepath.Base(filepath.Dir(entry)),
Value: strings.TrimSpace(string(cpus)),
Comment on lines +359 to +360
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Attributes are free text, there's no schema.
But as best practice we should strive to have as close as possible to a real schema: the possible names and values should be defined as stricly as possible and as practical. Now: we are passing through the kernel API, which is usually very stable. But still: what are the possible entries for both name and value? let's at least document that so users of this API have a easier time.

})
}
}

return attrList
}

func (w *nfdTopologyUpdater) updateNRTTopologyManagerInfo(nrt *v1alpha2.NodeResourceTopology) error {
policy, scope, err := w.detectTopologyPolicyAndScope()
if err != nil {
Expand All @@ -349,6 +377,9 @@ func (w *nfdTopologyUpdater) updateNRTTopologyManagerInfo(nrt *v1alpha2.NodeReso
updateAttributes(&nrt.Attributes, tmAttributes)
nrt.TopologyPolicies = deprecatedTopologyPolicies

attrList := discoverCpuCores()
updateAttributes(&nrt.Attributes, attrList)

Comment on lines +380 to +382
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PTAL #1964 (comment)
rather than constrain ourselves to the key/value attribute list, I'd explore the option to add new zones describing the E/P split and relationship

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, I guess we want to keep using attributes. I'm not convinced this is the best approach, but I can live with this

return nil
}

Expand Down
10 changes: 10 additions & 0 deletions source/cpu/cpu.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,16 @@ func (s *cpuSource) GetLabels() (source.FeatureLabels, error) {
labels["coprocessor.nx_gzip"] = v
}

_, err := os.ReadFile(hostpath.SysfsDir.Path("sys/devices/cpu_atom/cpus"))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we want to access the filesystem in this function? it seems we are only operating in memory up till this change

if err == nil {
labels["cpu_atom"] = true
}

_, err = os.ReadFile(hostpath.SysfsDir.Path("sys/devices/cpu_core/cpus"))
if err == nil {
labels["cpu_core"] = true
}

return labels, nil
}

Expand Down