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

add topology option #354

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
30 changes: 21 additions & 9 deletions pkg/context/context.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,10 @@ type Context struct {
PathOverrides option.PathOverrides
snapshotUnpackedPath string
alert option.Alerter
err error
Err error
DisableNodeCaches bool
DisableNodeAreas bool
DisableNodeDistances bool
Comment on lines +28 to +30
Copy link
Collaborator

Choose a reason for hiding this comment

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

do we need this (kinda unprecedented) level of detail or can we just have a single option controlling everything?

}

// WithContext returns an option.Option that contains a pre-existing Context
Expand Down Expand Up @@ -83,12 +86,18 @@ func New(opts ...*option.Option) *Context {
ctx.PathOverrides = merged.PathOverrides
}

if merged.TopologyOptions != nil {
ctx.DisableNodeCaches = merged.TopologyOptions.DisableNodeCaches
ctx.DisableNodeAreas = merged.TopologyOptions.DisableNodeAreas
ctx.DisableNodeDistances = merged.TopologyOptions.DisableNodeDistances
}

// New is not allowed to return error - it would break the established API.
// so the only way out is to actually do the checks here and record the error,
// and return it later, at the earliest possible occasion, in Setup()
if ctx.SnapshotPath != "" && ctx.Chroot != option.DefaultChroot {
// The env/client code supplied a value, but we are will overwrite it when unpacking shapshots!
ctx.err = fmt.Errorf("Conflicting options: chroot %q and snapshot path %q", ctx.Chroot, ctx.SnapshotPath)
ctx.Err = fmt.Errorf("Conflicting options: chroot %q and snapshot path %q", ctx.Chroot, ctx.SnapshotPath)
}
return ctx
}
Expand All @@ -102,11 +111,14 @@ func FromEnv() *Context {
snapRootVal := option.EnvOrDefaultSnapshotRoot()
snapExclusiveVal := option.EnvOrDefaultSnapshotExclusive()
return &Context{
Chroot: chrootVal,
EnableTools: enableTools,
SnapshotPath: snapPathVal,
SnapshotRoot: snapRootVal,
SnapshotExclusive: snapExclusiveVal,
Chroot: chrootVal,
EnableTools: enableTools,
SnapshotPath: snapPathVal,
SnapshotRoot: snapRootVal,
SnapshotExclusive: snapExclusiveVal,
DisableNodeCaches: option.EnvOrDefaultDisableNodeCaches(),
DisableNodeAreas: option.EnvOrDefaultDisableNodeAreas(),
DisableNodeDistances: option.EnvOrDefaultDisableNodeDistances(),
Comment on lines +119 to +121
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm inclined to say to keep consistent with other options are handled even though we should later perhaps refactor. But I'd be consistent meantime.

}
}

Expand All @@ -131,8 +143,8 @@ func (ctx *Context) Do(fn func() error) error {
// You should call `Setup` just once. It is safe to call `Setup` if you don't make
// use of optional extra features - `Setup` will do nothing.
func (ctx *Context) Setup() error {
if ctx.err != nil {
return ctx.err
if ctx.Err != nil {
return ctx.Err
}
if ctx.SnapshotPath == "" {
// nothing to do!
Expand Down
4 changes: 4 additions & 0 deletions pkg/memory/memory_cache_linux.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,10 @@ import (
)

func CachesForNode(ctx *context.Context, nodeID int) ([]*Cache, error) {
if ctx.DisableNodeCaches {
ctx.Warn("Node caches disabled. Returning empty cache list.")
return nil, nil
}
// The /sys/devices/node/nodeX directory contains a subdirectory called
// 'cpuX' for each logical processor assigned to the node. Each of those
// subdirectories containers a 'cache' subdirectory which contains a number
Expand Down
4 changes: 4 additions & 0 deletions pkg/memory/memory_linux.go
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,10 @@ func (i *Info) load() error {
}

func AreaForNode(ctx *context.Context, nodeID int) (*Area, error) {
if ctx.DisableNodeAreas {
ctx.Warn("Node areas disabled. Returning nil area.")
return nil, nil
}
paths := linuxpath.New(ctx)
path := filepath.Join(
paths.SysDevicesSystemNode,
Expand Down
65 changes: 58 additions & 7 deletions pkg/option/option.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,13 +17,16 @@ const (
)

const (
envKeyChroot = "GHW_CHROOT"
envKeyDisableWarnings = "GHW_DISABLE_WARNINGS"
envKeyDisableTools = "GHW_DISABLE_TOOLS"
envKeySnapshotPath = "GHW_SNAPSHOT_PATH"
envKeySnapshotRoot = "GHW_SNAPSHOT_ROOT"
envKeySnapshotExclusive = "GHW_SNAPSHOT_EXCLUSIVE"
envKeySnapshotPreserve = "GHW_SNAPSHOT_PRESERVE"
envKeyChroot = "GHW_CHROOT"
envKeyDisableWarnings = "GHW_DISABLE_WARNINGS"
envKeyDisableTools = "GHW_DISABLE_TOOLS"
envKeySnapshotPath = "GHW_SNAPSHOT_PATH"
envKeySnapshotRoot = "GHW_SNAPSHOT_ROOT"
envKeySnapshotExclusive = "GHW_SNAPSHOT_EXCLUSIVE"
envKeySnapshotPreserve = "GHW_SNAPSHOT_PRESERVE"
envKeyDisableNodeCaches = "GHW_DISABLE_NODE_CACHES"
envKeyDisableNodeAreas = "GHW_DISABLE_NODE_AREAS"
envKeyDisableNodeDistances = "GHW_DISABLE_NODE_DISTANCES"
)

// Alerter emits warnings about undesirable but recoverable errors.
Expand Down Expand Up @@ -140,6 +143,9 @@ type Option struct {
// during a call to the `context.WithContext` function. Only used internally.
// This is an interface to get around recursive package import issues.
Context interface{}

// TopologyOptions contains options for handling of ghw topology
TopologyOptions *TopologyOptions
}

// SnapshotOptions contains options for handling of ghw snapshots
Expand All @@ -163,6 +169,16 @@ type SnapshotOptions struct {
Exclusive bool
}

// TopologyOptions contains options for handling of ghw topology
type TopologyOptions struct {
// DisableNodeCaches disables the collection of node caches
DisableNodeCaches bool
// DisableNodeAreas disables the collection of node memory areas
DisableNodeAreas bool
// DisableNodeDistances disables the collection of node distances
DisableNodeDistances bool
}

// WithChroot allows to override the root directory ghw uses.
func WithChroot(dir string) *Option {
return &Option{Chroot: &dir}
Expand Down Expand Up @@ -233,6 +249,9 @@ func Merge(opts ...*Option) *Option {
if opt.Context != nil {
merged.Context = opt.Context
}
if opt.TopologyOptions != nil {
merged.TopologyOptions = opt.TopologyOptions
}
}
// Set the default value if missing from mergeOpts
if merged.Chroot == nil {
Expand All @@ -254,5 +273,37 @@ func Merge(opts ...*Option) *Option {
enabled := EnvOrDefaultTools()
merged.EnableTools = &enabled
}
if merged.TopologyOptions == nil {
merged.TopologyOptions = &TopologyOptions{
DisableNodeCaches: EnvOrDefaultDisableNodeCaches(),
DisableNodeAreas: EnvOrDefaultDisableNodeAreas(),
DisableNodeDistances: EnvOrDefaultDisableNodeDistances(),
}
}

return merged
}

// EnvOrDefaultDisableNodeCaches ...
func EnvOrDefaultDisableNodeCaches() bool {
if _, exists := os.LookupEnv(envKeyDisableNodeCaches); exists {
return true
}
return false
}

// EnvOrDefaultDisableNodeCaches ...
func EnvOrDefaultDisableNodeAreas() bool {
if _, exists := os.LookupEnv(envKeyDisableNodeAreas); exists {
return true
}
return false
}

// EnvOrDefaultDisableNodeDistances ...
func EnvOrDefaultDisableNodeDistances() bool {
if _, exists := os.LookupEnv(envKeyDisableNodeDistances); exists {
return true
}
return false
}
9 changes: 9 additions & 0 deletions pkg/topology/topology_linux.go
Original file line number Diff line number Diff line change
Expand Up @@ -46,32 +46,37 @@ func topologyNodes(ctx *context.Context) []*Node {
nodeID, err := strconv.Atoi(filename[4:])
if err != nil {
ctx.Warn("failed to determine node ID: %s\n", err)
ctx.Err = err
Copy link
Collaborator

Choose a reason for hiding this comment

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

this usecase makes sense, but I'd rather add a new function or enhance the Warn helper to also set err, without exposing the field (just yet)

Copy link
Author

Choose a reason for hiding this comment

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

sorry i did not get your point, could you give a example code please?

return nodes
}
node.ID = nodeID
cores, err := cpu.CoresForNode(ctx, nodeID)
if err != nil {
ctx.Warn("failed to determine cores for node: %s\n", err)
ctx.Err = err
return nodes
}
node.Cores = cores
caches, err := memory.CachesForNode(ctx, nodeID)
if err != nil {
ctx.Warn("failed to determine caches for node: %s\n", err)
ctx.Err = err
return nodes
}
node.Caches = caches

distances, err := distancesForNode(ctx, nodeID)
if err != nil {
ctx.Warn("failed to determine node distances for node: %s\n", err)
ctx.Err = err
return nodes
}
node.Distances = distances

area, err := memory.AreaForNode(ctx, nodeID)
if err != nil {
ctx.Warn("failed to determine memory area for node: %s\n", err)
ctx.Err = err
return nodes
}
node.Memory = area
Expand All @@ -82,6 +87,10 @@ func topologyNodes(ctx *context.Context) []*Node {
}

func distancesForNode(ctx *context.Context, nodeID int) ([]int, error) {
if ctx.DisableNodeDistances {
ctx.Warn("Node distances disabled. Returning empty distances list.")
return nil, nil
}
paths := linuxpath.New(ctx)
path := filepath.Join(
paths.SysDevicesSystemNode,
Expand Down
Loading