From df27327f147da30fb6200968b3b678848c4aa583 Mon Sep 17 00:00:00 2001 From: Markus Lehtonen Date: Wed, 3 Mar 2021 17:36:43 +0200 Subject: [PATCH] source/usb: implement FeatureSource Separate feature discovery and creation of feature labels in the usb source. Move usb_utils from source/internal to the source/usb package. Change the implementation of the UsbID custom rule to utilize the FeatureSource interface of the usb source. Also, add minimalist unit test. --- source/custom/rules/usb_id_rule.go | 36 +++++---- source/usb/usb.go | 75 ++++++++++++------- source/usb/usb_test.go | 36 +++++++++ .../{internal/usb_utils.go => usb/utils.go} | 66 +++++++--------- 4 files changed, 127 insertions(+), 86 deletions(-) create mode 100644 source/usb/usb_test.go rename source/{internal/usb_utils.go => usb/utils.go} (62%) diff --git a/source/custom/rules/usb_id_rule.go b/source/custom/rules/usb_id_rule.go index 104ec594e..058ec357b 100644 --- a/source/custom/rules/usb_id_rule.go +++ b/source/custom/rules/usb_id_rule.go @@ -18,7 +18,10 @@ package rules import ( "fmt" - usbutils "sigs.k8s.io/node-feature-discovery/source/internal" + + "sigs.k8s.io/node-feature-discovery/pkg/api/feature" + "sigs.k8s.io/node-feature-discovery/source" + "sigs.k8s.io/node-feature-discovery/source/usb" ) // Rule that matches on the following USB device attributes: @@ -38,44 +41,39 @@ type UsbIDRule struct { // Match USB devices on provided USB device attributes func (r *UsbIDRule) Match() (bool, error) { - devAttr := map[string]bool{} - for _, attr := range []string{"class", "vendor", "device", "serial"} { - devAttr[attr] = true - } - allDevs, err := usbutils.DetectUsb(devAttr) - if err != nil { - return false, fmt.Errorf("failed to detect USB devices: %s", err.Error()) + devs, ok := source.GetFeatureSource("usb").GetFeatures().Instances[usb.DeviceFeature] + if !ok { + return false, fmt.Errorf("usb device information not available") } - for _, classDevs := range allDevs { - for _, dev := range classDevs { - // match rule on a single device - if r.matchDevOnRule(dev) { - return true, nil - } + for _, dev := range devs.Elements { + // match rule on a single device + if r.matchDevOnRule(dev) { + return true, nil } } return false, nil } -func (r *UsbIDRule) matchDevOnRule(dev usbutils.UsbDeviceInfo) bool { +func (r *UsbIDRule) matchDevOnRule(dev feature.InstanceFeature) bool { if len(r.Class) == 0 && len(r.Vendor) == 0 && len(r.Device) == 0 { return false } - if len(r.Class) > 0 && !in(dev["class"], r.Class) { + attrs := dev.Attributes + if len(r.Class) > 0 && !in(attrs["class"], r.Class) { return false } - if len(r.Vendor) > 0 && !in(dev["vendor"], r.Vendor) { + if len(r.Vendor) > 0 && !in(attrs["vendor"], r.Vendor) { return false } - if len(r.Device) > 0 && !in(dev["device"], r.Device) { + if len(r.Device) > 0 && !in(attrs["device"], r.Device) { return false } - if len(r.Serial) > 0 && !in(dev["serial"], r.Serial) { + if len(r.Serial) > 0 && !in(attrs["serial"], r.Serial) { return false } diff --git a/source/usb/usb.go b/source/usb/usb.go index a86cf15d4..036409967 100644 --- a/source/usb/usb.go +++ b/source/usb/usb.go @@ -22,12 +22,15 @@ import ( "k8s.io/klog/v2" + "sigs.k8s.io/node-feature-discovery/pkg/api/feature" + "sigs.k8s.io/node-feature-discovery/pkg/utils" "sigs.k8s.io/node-feature-discovery/source" - usbutils "sigs.k8s.io/node-feature-discovery/source/internal" ) const Name = "usb" +const DeviceFeature = "device" + type Config struct { DeviceClassWhitelist []string `json:"deviceClassWhitelist,omitempty"` DeviceLabelFields []string `json:"deviceLabelFields,omitempty"` @@ -46,12 +49,14 @@ func newDefaultConfig() *Config { // usbSource implements the LabelSource and ConfigurableSource interfaces. type usbSource struct { - config *Config + config *Config + features *feature.DomainFeatures } // Singleton source instance var ( - src usbSource + src = usbSource{config: newDefaultConfig()} + _ source.FeatureSource = &src _ source.LabelSource = &src _ source.ConfigurableSource = &src ) @@ -80,7 +85,8 @@ func (s *usbSource) Priority() int { return 0 } // GetLabels method of the LabelSource interface func (s *usbSource) GetLabels() (source.FeatureLabels, error) { - features := source.FeatureLabels{} + labels := source.FeatureLabels{} + features := s.GetFeatures() // Construct a device label format, a sorted list of valid attributes deviceLabelFields := []string{} @@ -89,7 +95,7 @@ func (s *usbSource) GetLabels() (source.FeatureLabels, error) { configLabelFields[field] = true } - for _, attr := range usbutils.DefaultUsbDevAttrs { + for _, attr := range devAttrs { if _, ok := configLabelFields[attr]; ok { deviceLabelFields = append(deviceLabelFields, attr) delete(configLabelFields, attr) @@ -100,42 +106,55 @@ func (s *usbSource) GetLabels() (source.FeatureLabels, error) { for key := range configLabelFields { keys = append(keys, key) } - klog.Warningf("invalid fields '%v' in deviceLabelFields, ignoring...", keys) + klog.Warningf("invalid fields (%s) in deviceLabelFields, ignoring...", strings.Join(keys, ", ")) } if len(deviceLabelFields) == 0 { klog.Warningf("no valid fields in deviceLabelFields defined, using the defaults") deviceLabelFields = []string{"vendor", "device"} } - // Read configured or default labels. Attributes set to 'true' are considered must-have. - deviceAttrs := map[string]bool{} - for _, label := range deviceLabelFields { - deviceAttrs[label] = true - } - - devs, err := usbutils.DetectUsb(deviceAttrs) - if err != nil { - return nil, fmt.Errorf("failed to detect USB devices: %s", err.Error()) - } - // Iterate over all device classes - for class, classDevs := range devs { + for _, dev := range features.Instances[DeviceFeature].Elements { + attrs := dev.Attributes + class := attrs["class"] for _, white := range s.config.DeviceClassWhitelist { - if strings.HasPrefix(class, strings.ToLower(white)) { - for _, dev := range classDevs { - devLabel := "" - for i, attr := range deviceLabelFields { - devLabel += dev[attr] - if i < len(deviceLabelFields)-1 { - devLabel += "_" - } + if strings.HasPrefix(string(class), strings.ToLower(white)) { + devLabel := "" + for i, attr := range deviceLabelFields { + devLabel += attrs[attr] + if i < len(deviceLabelFields)-1 { + devLabel += "_" } - features[devLabel+".present"] = true } + labels[devLabel+".present"] = true + break } } } - return features, nil + return labels, nil +} + +// Discover method of the FeatureSource interface +func (s *usbSource) Discover() error { + s.features = feature.NewDomainFeatures() + + devs, err := detectUsb() + if err != nil { + return fmt.Errorf("failed to detect USB devices: %s", err.Error()) + } + s.features.Instances[DeviceFeature] = feature.NewInstanceFeatures(devs) + + utils.KlogDump(3, "discovered usb features:", " ", s.features) + + return nil +} + +// GetFeatures method of the FeatureSource Interface +func (s *usbSource) GetFeatures() *feature.DomainFeatures { + if s.features == nil { + s.features = feature.NewDomainFeatures() + } + return s.features } func init() { diff --git a/source/usb/usb_test.go b/source/usb/usb_test.go new file mode 100644 index 000000000..460402536 --- /dev/null +++ b/source/usb/usb_test.go @@ -0,0 +1,36 @@ +/* +Copyright 2021 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package usb + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "sigs.k8s.io/node-feature-discovery/pkg/api/feature" +) + +func TestUsbSource(t *testing.T) { + assert.Equal(t, src.Name(), Name) + + // Check that GetLabels works with empty features + src.features = feature.NewDomainFeatures() + l, err := src.GetLabels() + + assert.Nil(t, err, err) + assert.Empty(t, l) + +} diff --git a/source/internal/usb_utils.go b/source/usb/utils.go similarity index 62% rename from source/internal/usb_utils.go rename to source/usb/utils.go index 2b80178fb..cce6674a1 100644 --- a/source/internal/usb_utils.go +++ b/source/usb/utils.go @@ -14,7 +14,7 @@ See the License for the specific language governing permissions and limitations under the License. */ -package busutils +package usb import ( "fmt" @@ -24,12 +24,11 @@ import ( "strings" "k8s.io/klog/v2" + + "sigs.k8s.io/node-feature-discovery/pkg/api/feature" ) -type UsbDeviceInfo map[string]string -type UsbClassMap map[string]UsbDeviceInfo - -var DefaultUsbDevAttrs = []string{"class", "vendor", "device", "serial"} +var devAttrs = []string{"class", "vendor", "device", "serial"} // The USB device sysfs files do not have terribly user friendly names, map // these for consistency with the PCI matcher. @@ -58,26 +57,26 @@ func readSingleUsbAttribute(devPath string, attrName string) (string, error) { } // Read information of one USB device -func readUsbDevInfo(devPath string, deviceAttrSpec map[string]bool) (UsbClassMap, error) { - classmap := UsbClassMap{} - info := UsbDeviceInfo{} +func readUsbDevInfo(devPath string) ([]feature.InstanceFeature, error) { + instances := make([]feature.InstanceFeature, 0) + attrs := make(map[string]string) - for attr := range deviceAttrSpec { + for _, attr := range devAttrs { attrVal, _ := readSingleUsbAttribute(devPath, attr) if len(attrVal) > 0 { - info[attr] = attrVal + attrs[attr] = attrVal } } // USB devices encode their class information either at the device or the interface level. If the device class // is set, return as-is. - if info["class"] != "00" { - classmap[info["class"]] = info + if attrs["class"] != "00" { + instances = append(instances, *feature.NewInstanceFeature(attrs)) } else { // Otherwise, if a 00 is presented at the device level, descend to the interface level. interfaces, err := filepath.Glob(devPath + "/*/bInterfaceClass") if err != nil { - return classmap, err + return nil, err } // A device may, notably, have multiple interfaces with mixed classes, so we create a unique device for each @@ -86,54 +85,43 @@ func readUsbDevInfo(devPath string, deviceAttrSpec map[string]bool) (UsbClassMap // Determine the interface class attrVal, err := readSingleUsbSysfsAttribute(intf) if err != nil { - return classmap, err + return nil, err } - attr := UsbDeviceInfo{} - for k, v := range info { - attr[k] = v + subdevAttrs := make(map[string]string, len(attrs)) + for k, v := range attrs { + subdevAttrs[k] = v } + subdevAttrs["class"] = attrVal - attr["class"] = attrVal - classmap[attrVal] = attr + instances = append(instances, *feature.NewInstanceFeature(subdevAttrs)) } } - return classmap, nil + return instances, nil } -// List available USB devices and retrieve device attributes. -// deviceAttrSpec is a map which specifies which attributes to retrieve. -// a false value for a specific attribute marks the attribute as optional. -// a true value for a specific attribute marks the attribute as mandatory. -// "class" attribute is considered mandatory. -// DetectUsb() will fail if the retrieval of a mandatory attribute fails. -func DetectUsb(deviceAttrSpec map[string]bool) (map[string][]UsbDeviceInfo, error) { +// detectUsb detects available USB devices and retrieves their device attributes. +func detectUsb() ([]feature.InstanceFeature, error) { // Unlike PCI, the USB sysfs interface includes entries not just for // devices. We work around this by globbing anything that includes a // valid product ID. - const devicePath = "/sys/bus/usb/devices/*/idProduct" - devInfo := make(map[string][]UsbDeviceInfo) - - devices, err := filepath.Glob(devicePath) + const devPathGlob = "/sys/bus/usb/devices/*/idProduct" + devPaths, err := filepath.Glob(devPathGlob) if err != nil { return nil, err } - // "class" is a mandatory attribute, inject it to spec if needed. - deviceAttrSpec["class"] = true - // Iterate over devices - for _, device := range devices { - devMap, err := readUsbDevInfo(filepath.Dir(device), deviceAttrSpec) + devInfo := make([]feature.InstanceFeature, 0) + for _, devPath := range devPaths { + devs, err := readUsbDevInfo(filepath.Dir(devPath)) if err != nil { klog.Error(err) continue } - for class, info := range devMap { - devInfo[class] = append(devInfo[class], info) - } + devInfo = append(devInfo, devs...) } return devInfo, nil