From 591a1bababf9ac1446a13e75ab3cae2df40b9619 Mon Sep 17 00:00:00 2001 From: Francesco Romani Date: Tue, 16 Feb 2021 16:33:03 +0100 Subject: [PATCH 1/2] option: add WithDisableTools option When reading network interface information from a snapshot, invoking ethtool is useless at best, misleading at worst. We start allowing the users which want to consume a snapshot to disable the ethtool invocation. The default is to keep the existing behaviour and call ethtool if available. Signed-off-by: Francesco Romani --- README.md | 12 ++++++++++++ alias.go | 1 + pkg/context/context.go | 7 +++++++ pkg/net/net_linux.go | 12 ++++++++---- pkg/option/option.go | 29 +++++++++++++++++++++++++++++ pkg/option/option_test.go | 26 +++++++++++++++++++++++++- 6 files changed, 82 insertions(+), 5 deletions(-) diff --git a/README.md b/README.md index 12ccc8f7..509534cd 100644 --- a/README.md +++ b/README.md @@ -1278,6 +1278,18 @@ memory: total_usable_bytes: 25263415296 ``` +## Calling external programs + +By default ghw may call external programs, for example `ethtool` to learn about hardware capabilities. +In some rare circumstances it may be useful to opt out from this behaviour and rely only on the data +provided by pseudo-filesystems, like sysfs. +The most common use case is when we want to consume a snapshot from ghw. In these cases the information +provided by tools will be most likely inconsistent with the data from the snapshot - they will run on +a different host! +To make ghw not calling external tools, set the environs variable `GHW_DISABLE_TOOLS` to any value, +or, programmatically, check the `WithDisableTools` function. +The default behaviour of ghw is to call external tools when available. + ## Developers Contributions to `ghw` are welcomed! Fork the repo on GitHub and submit a pull diff --git a/alias.go b/alias.go index 125067c9..965d80be 100644 --- a/alias.go +++ b/alias.go @@ -30,6 +30,7 @@ var ( WithNullAlterter = option.WithNullAlerter // match the existing environ variable to minimize surprises WithDisableWarnings = option.WithNullAlerter + WithDisableTools = option.WithDisableTools ) type SnapshotOptions = option.SnapshotOptions diff --git a/pkg/context/context.go b/pkg/context/context.go index d59a0853..255d5ebf 100644 --- a/pkg/context/context.go +++ b/pkg/context/context.go @@ -15,6 +15,7 @@ import ( // context when calling internal discovery methods type Context struct { Chroot string + EnableTools bool SnapshotPath string SnapshotRoot string SnapshotExclusive bool @@ -42,6 +43,10 @@ func New(opts ...*option.Option) *Context { ctx.alert = merged.Alerter } + if merged.EnableTools != nil { + ctx.EnableTools = *merged.EnableTools + } + return ctx } @@ -49,11 +54,13 @@ func New(opts ...*option.Option) *Context { // default options values func FromEnv() *Context { chrootVal := option.EnvOrDefaultChroot() + enableTools := option.EnvOrDefaultTools() snapPathVal := option.EnvOrDefaultSnapshotPath() snapRootVal := option.EnvOrDefaultSnapshotRoot() snapExclusiveVal := option.EnvOrDefaultSnapshotExclusive() return &Context{ Chroot: chrootVal, + EnableTools: enableTools, SnapshotPath: snapPathVal, SnapshotRoot: snapRootVal, SnapshotExclusive: snapExclusiveVal, diff --git a/pkg/net/net_linux.go b/pkg/net/net_linux.go index 9f270db7..1b338dfa 100644 --- a/pkg/net/net_linux.go +++ b/pkg/net/net_linux.go @@ -37,10 +37,14 @@ func nics(ctx *context.Context) []*NIC { return nics } - etInstalled := ethtoolInstalled() - if !etInstalled { - ctx.Warn(_WARN_ETHTOOL_NOT_INSTALLED) + etAvailable := ctx.EnableTools + if etAvailable { + if etInstalled := ethtoolInstalled(); !etInstalled { + ctx.Warn(_WARN_ETHTOOL_NOT_INSTALLED) + etAvailable = false + } } + for _, file := range files { filename := file.Name() // Ignore loopback... @@ -62,7 +66,7 @@ func nics(ctx *context.Context) []*NIC { mac := netDeviceMacAddress(paths, filename) nic.MacAddress = mac - if etInstalled { + if etAvailable { nic.Capabilities = netDeviceCapabilities(ctx, filename) } else { nic.Capabilities = []*NICCapability{} diff --git a/pkg/option/option.go b/pkg/option/option.go index ed8fecfa..0af8b4cb 100644 --- a/pkg/option/option.go +++ b/pkg/option/option.go @@ -17,6 +17,7 @@ const ( defaultChroot = "/" envKeyChroot = "GHW_CHROOT" envKeyDisableWarnings = "GHW_DISABLE_WARNINGS" + envKeyDisableTools = "GHW_DISABLE_TOOLS" envKeySnapshotPath = "GHW_SNAPSHOT_PATH" envKeySnapshotRoot = "GHW_SNAPSHOT_ROOT" envKeySnapshotExclusive = "GHW_SNAPSHOT_EXCLUSIVE" @@ -95,6 +96,17 @@ func EnvOrDefaultSnapshotPreserve() bool { return false } +// EnvOrDefaultTools return true if ghw should use external tools to augment the data collected +// from sysfs. Most users want to do this most of time, so this is enabled by default. +// Users consuming snapshots may want to opt out, thus they can set the GHW_DISABLE_TOOLS +// environs variable to any value to make ghw skip calling external tools even if they are available. +func EnvOrDefaultTools() bool { + if _, exists := os.LookupEnv(envKeyDisableTools); exists { + return false + } + return true +} + // Option is used to represent optionally-configured settings. Each field is a // pointer to some concrete value so that we can tell when something has been // set or left unset. @@ -113,6 +125,10 @@ type Option struct { // Alerter contains the target for ghw warnings Alerter Alerter + + // EnableTools optionally request ghw to not call any external program to learn + // about the hardware. The default is to use such tools if available. + EnableTools *bool } // SnapshotOptions contains options for handling of ghw snapshots @@ -161,6 +177,12 @@ func WithNullAlerter() *Option { } } +// WithDisableTools sets enables or prohibts ghw to call external tools to discover hardware capabilities. +func WithDisableTools() *Option { + false_ := false + return &Option{EnableTools: &false_} +} + // There is intentionally no Option related to GHW_SNAPSHOT_PRESERVE because we see that as // a debug/troubleshoot aid more something users wants to do regularly. // Hence we allow that only via the environment variable for the time being. @@ -177,6 +199,9 @@ func Merge(opts ...*Option) *Option { if opt.Alerter != nil { merged.Alerter = opt.Alerter } + if opt.EnableTools != nil { + merged.EnableTools = opt.EnableTools + } } // Set the default value if missing from mergeOpts if merged.Chroot == nil { @@ -194,5 +219,9 @@ func Merge(opts ...*Option) *Option { Exclusive: EnvOrDefaultSnapshotExclusive(), } } + if merged.EnableTools == nil { + enabled := EnvOrDefaultTools() + merged.EnableTools = &enabled + } return merged } diff --git a/pkg/option/option_test.go b/pkg/option/option_test.go index 25718518..62a7e0c4 100644 --- a/pkg/option/option_test.go +++ b/pkg/option/option_test.go @@ -28,7 +28,8 @@ func TestOption(t *testing.T) { option.WithChroot("/my/chroot/dir/2"), }, merged: &option.Option{ - Chroot: stringPtr("/my/chroot/dir/2"), + Chroot: stringPtr("/my/chroot/dir/2"), + EnableTools: boolPtr(true), }, }, { @@ -106,6 +107,17 @@ func TestOption(t *testing.T) { }, }, }, + { + name: "chroot and disabling tools", + opts: []*option.Option{ + option.WithChroot("/my/chroot/dir"), + option.WithDisableTools(), + }, + merged: &option.Option{ + Chroot: stringPtr("/my/chroot/dir"), + EnableTools: boolPtr(false), + }, + }, } for _, optTCase := range optTCases { t.Run(optTCase.name, func(t *testing.T) { @@ -121,6 +133,10 @@ func stringPtr(s string) *string { return &s } +func boolPtr(b bool) *bool { + return &b +} + func optionEqual(a, b *option.Option) (string, bool) { if a == nil || b == nil { return "top-level", false @@ -139,6 +155,14 @@ func optionEqual(a, b *option.Option) (string, bool) { } return optionSnapshotEqual(a.Snapshot, b.Snapshot) } + if a.EnableTools != nil { + if b.EnableTools == nil { + return "enabletools ptr", false + } + if *a.EnableTools != *b.EnableTools { + return "enabletools value", false + } + } return "", true } From 1717e36f4cc68d8dadb2eeb985bee48182980ba9 Mon Sep 17 00:00:00 2001 From: Francesco Romani Date: Tue, 16 Feb 2021 17:21:50 +0100 Subject: [PATCH 2/2] snapshot: clone data pertaining the net ifaces We split the discovery of the list of the files we want to collect for snapshotting purposes because the list depends on the host specific details; we want to filter out the virtual devices - they are not about hardware after all. hence, we need to do some runtime discovery. Signed-off-by: Francesco Romani --- pkg/snapshot/clonetree.go | 9 +++++ pkg/snapshot/clonetree_net.go | 62 +++++++++++++++++++++++++++++++++++ 2 files changed, 71 insertions(+) create mode 100644 pkg/snapshot/clonetree_net.go diff --git a/pkg/snapshot/clonetree.go b/pkg/snapshot/clonetree.go index 691c2559..01c22bb7 100644 --- a/pkg/snapshot/clonetree.go +++ b/pkg/snapshot/clonetree.go @@ -46,6 +46,15 @@ func CloneTreeInto(scratchDir string) error { // ghw cares about. The intended usage of this function is to validate a clone tree, // checking that the content matches the expectations. func ExpectedCloneContent() []string { + fileSpecs := ExpectedCloneStaticContent() + fileSpecs = append(fileSpecs, ExpectedCloneNetContent()...) + return fileSpecs +} + +// ExpectedCloneStaticContent return a slice of glob patterns which represent the pseudofiles +// ghw cares about, and which are independent from host specific topology or configuration, +// thus are safely represented by a static slice - e.g. they don't need to be discovered at runtime. +func ExpectedCloneStaticContent() []string { return []string{ "/etc/mtab", "/proc/cpuinfo", diff --git a/pkg/snapshot/clonetree_net.go b/pkg/snapshot/clonetree_net.go new file mode 100644 index 00000000..2f11214f --- /dev/null +++ b/pkg/snapshot/clonetree_net.go @@ -0,0 +1,62 @@ +// +// Use and distribution licensed under the Apache license version 2. +// +// See the COPYING file in the root project directory for full text. +// + +package snapshot + +import ( + "io/ioutil" + "os" + "path/filepath" + "strings" +) + +const ( + sysClassNet = "/sys/class/net" +) + +// ExpectedCloneNetContent returns a slice of strings pertaning the network interfaces ghw +// cares about. We cannot use a static list because we want to filter away the virtual devices, +// which ghw doesn't concerns itself about. So we need to do some runtime discovery. +// Additionally, we want to make sure to clone the backing device data. +func ExpectedCloneNetContent() []string { + var fileSpecs []string + ifaceEntries := []string{ + "addr_assign_type", + // intentionally avoid to clone "address" to avoid to leak any host-idenfifiable data. + } + entries, err := ioutil.ReadDir(sysClassNet) + if err != nil { + // we should not import context, hence we can't Warn() + return fileSpecs + } + for _, entry := range entries { + netName := entry.Name() + netPath := filepath.Join(sysClassNet, netName) + dest, err := os.Readlink(netPath) + if err != nil { + continue + } + if strings.Contains(dest, "devices/virtual/net") { + // there is no point in cloning data for virtual devices, + // becahse ghw concerns itself with HardWare. + continue + } + + // so, first copy the symlink itself + fileSpecs = append(fileSpecs, netPath) + + // now we have to clone the content of the actual network interface + // data related (and found into a subdir of) the backing hardware + // device + netIface := filepath.Clean(filepath.Join(sysClassNet, dest)) + for _, ifaceEntry := range ifaceEntries { + fileSpecs = append(fileSpecs, filepath.Join(netIface, ifaceEntry)) + } + + } + + return fileSpecs +}