pillar/assignableadapters: clear error strings

for usb collisions:

1. create two usb adapters with collision
2. change the usbaddr of one of the adapters

now the collision should be gone, but
as the error string for both adapters has been set, it was
not cleared

for parentassigngrp cycles:

1. create two usb adapters that have each other as parentassigngrp
2. change the parentassignrp of one of the adapters

now the cycle should be gone, but
as the error string for both adapters has been set, it was
not cleared

Also don't overwrite errors and be able to clear specific error types

Signed-off-by: Christoph Ostarek <christoph@zededa.com>
(cherry picked from commit d1e13b8)

pkg/pillar/cmd/domainmgr/domainmgr.go

@@ -1381,8 +1381,8 @@ func doAssignIoAdaptersToDomain(ctx *domainContext, config types.DomainConfig,
  status.DomainName)
  }
  // Also checked in reserveAdapters. Check here in case there was a late error.
- if ib.Error != "" {
- return errors.New(ib.Error)
+ if !ib.Error.Empty() {
+ return fmt.Errorf(ib.Error.String())
  }
  if ib.UsbAddr != "" {
  log.Functionf("Assigning %s (%s) to %s",
@@ -2084,9 +2084,9 @@ func reserveAdapters(ctx *domainContext, config types.DomainConfig) *types.Error
  adapter.Type, adapter.Name, ibp.Phylabel)
  return &description
  }
- if ibp.Error != "" {
+ if !ibp.Error.Empty() {
  description.Error = fmt.Sprintf("adapter %d %s phylabel %s has error: %s",
- adapter.Type, adapter.Name, ibp.Phylabel, ibp.Error)
+ adapter.Type, adapter.Name, ibp.Phylabel, ibp.Error.String())
  return &description
  }
  }
@@ -2804,11 +2804,9 @@ func handlePhysicalIOAdapterListImpl(ctxArg interface{}, key string,
  // Fill in PCIlong, macaddr, unique
  _, err := checkAndFillIoBundle(ib)
  if err != nil {
- ib.Error = err.Error()
- ib.ErrorTime = time.Now()
+ ib.Error.Append(err)
  } else {
- ib.Error = ""
- ib.ErrorTime = time.Time{}
+ ib.Error.Clear()
  }
  // We assume AddOrUpdateIoBundle will preserve any
  // existing IsPort/IsPCIBack/UsedByUUID
@@ -2844,17 +2842,15 @@ func handlePhysicalIOAdapterListImpl(ctxArg interface{}, key string,
  if err != nil {
  err = fmt.Errorf("setupVCAN: %w", err)
  log.Error(err)
- ib.Error = err.Error()
- ib.ErrorTime = time.Now()
+ ib.Error.Append(err)
  }
  } else if ib.Type == types.IoCAN {
  // Initialize physical CAN device
  err := setupCAN(ib)
  if err != nil {
  err = fmt.Errorf("setupCAN: %w", err)
  log.Error(err)
- ib.Error = err.Error()
- ib.ErrorTime = time.Now()
+ ib.Error.Append(err)
  }
  }
  }
@@ -2894,11 +2890,9 @@ func handlePhysicalIOAdapterListImpl(ctxArg interface{}, key string,
  // Fill in PCIlong, macaddr, unique
  _, err := checkAndFillIoBundle(ib)
  if err != nil {
- ib.Error = err.Error()
- ib.ErrorTime = time.Now()
+ ib.Error.Append(err)
  } else {
- ib.Error = ""
- ib.ErrorTime = time.Time{}
+ ib.Error.Clear()
  }
  currentIbPtr := aa.LookupIoBundlePhylabel(phyAdapter.Phylabel)
  if currentIbPtr == nil || currentIbPtr.HasAdapterChanged(log, phyAdapter) {
@@ -3059,12 +3053,10 @@ func updatePortAndPciBackIoBundle(ctx *domainContext, ib *types.IoBundle) (chang
  changed, err := updatePortAndPciBackIoMember(ctx, ib, isPort, keepInHost)
  anyChanged = anyChanged || changed
  if err != nil {
- ib.Error = err.Error()
- ib.ErrorTime = time.Now()
+ ib.Error.Append(err)
  log.Error(err)
  } else {
- ib.Error = ""
- ib.ErrorTime = time.Time{}
+ ib.Error.Clear()
  }
  }
  return anyChanged
@@ -3137,9 +3129,9 @@ func updatePortAndPciBackIoMember(ctx *domainContext, ib *types.IoBundle, isPort
  }
 
  if !ib.KeepInHost && !ib.IsPCIBack {
- if ib.Error != "" {
+ if !ib.Error.Empty() {
  log.Warningf("Not assigning %s (%s) to pciback due to error: %s at %s",
- ib.Phylabel, ib.PciLong, ib.Error, ib.ErrorTime)
+ ib.Phylabel, ib.PciLong, ib.Error.String(), ib.Error.ErrorTime())
  } else if ctx.deviceNetworkStatus.Testing && ib.Type.IsNet() {
  log.Noticef("Not assigning %s (%s) to pciback due to Testing",
  ib.Phylabel, ib.PciLong)

pkg/pillar/cmd/zedagent/reportinfo.go

@@ -529,12 +529,12 @@ func PublishDeviceInfoToZedCloud(ctx *zedagentContext, dest destinationBitset) {
  } else if ib.KeepInHost {
  reportAA.UsedByBaseOS = true
  }
- if ib.Error != "" {
+ if !ib.Error.Empty() {
  errInfo := new(info.ErrorInfo)
- errInfo.Description = ib.Error
+ errInfo.Description = ib.Error.String()
  errInfo.Severity = info.Severity_SEVERITY_ERROR
- if !ib.ErrorTime.IsZero() {
- protoTime, err := ptypes.TimestampProto(ib.ErrorTime)
+ if !ib.Error.ErrorTime().IsZero() {
+ protoTime, err := ptypes.TimestampProto(ib.Error.ErrorTime())
  if err == nil {
  errInfo.Timestamp = protoTime
  }

pkg/pillar/types/assignableadapters.go

@@ -12,6 +12,7 @@ package types
 // file on boot.
 
 import (
+ "errors"
  "fmt"
  "os"
  "reflect"
@@ -31,6 +32,73 @@ type AssignableAdapters struct {
  IoBundleList []IoBundle
 }
 
+type ioBundleError struct {
+ Errors []error
+ TimeOfError time.Time
+}
+
+func (iobe *ioBundleError) ErrorTime() time.Time {
+ return iobe.TimeOfError
+}
+
+func (iobe *ioBundleError) String() string {
+ if len(iobe.Errors) == 0 {
+ return ""
+ }
+ errorStrings := make([]string, 0, len(iobe.Errors))
+ for _, err := range iobe.Errors {
+ errorStrings = append(errorStrings, err.Error())
+ }
+ return strings.Join(errorStrings, "; ")
+}
+
+func (iobe *ioBundleError) Append(err error) {
+ if iobe.Errors == nil {
+ iobe.Errors = make([]error, 0, 1)
+ }
+
+ iobe.Errors = append(iobe.Errors, err)
+
+ iobe.TimeOfError = time.Now()
+}
+
+func (iobe *ioBundleError) Empty() bool {
+ if iobe.Errors == nil || len(iobe.Errors) == 0 {
+ return true
+ }
+
+ return false
+}
+
+func (iobe *ioBundleError) hasError(e error) bool {
+ for _, err := range iobe.Errors {
+ if reflect.TypeOf(e) == reflect.TypeOf(err) {
+ return true
+ }
+ }
+
+ return false
+}
+
+func (iobe *ioBundleError) removeByType(e error) {
+ toRemoveIndices := []int{}
+ for i, err := range iobe.Errors {
+ if reflect.TypeOf(e) == reflect.TypeOf(err) {
+ toRemoveIndices = append(toRemoveIndices, i)
+ }
+ }
+
+ for i := len(toRemoveIndices) - 1; i >= 0; i-- {
+ toRemove := toRemoveIndices[i]
+ iobe.Errors = append(iobe.Errors[:toRemove], iobe.Errors[toRemove+1:]...)
+ }
+}
+
+func (iobe *ioBundleError) Clear() {
+ iobe.Errors = make([]error, 0)
+ iobe.TimeOfError = time.Time{}
+}
+
 // IoBundle has one entry per individual receptacle with a reference
 // to a group name. Those sharing a group name needs to be assigned
 // together.
@@ -98,8 +166,7 @@ type IoBundle struct {
  // Do not put device under pciBack, instead keep it in dom0 as long as it is not assigned to any application.
  // In other words, this does not prevent assignments but keeps unassigned devices visible to EVE.
  KeepInHost bool
- Error string
- ErrorTime time.Time
+ Error ioBundleError
 
  // Only used in PhyIoNetEthPF
  Vfs sriov.VFList
@@ -417,7 +484,6 @@ func (aa *AssignableAdapters) LookupIoBundleLogicallabel(label string) *IoBundle
 // LookupIoBundleGroup returns an empty slice if not found
 // Returns pointers into aa
 func (aa *AssignableAdapters) LookupIoBundleGroup(group string) []*IoBundle {
-
  var list []*IoBundle
  if group == "" {
  return list
@@ -437,7 +503,6 @@ func (aa *AssignableAdapters) LookupIoBundleGroup(group string) []*IoBundle {
 // a member phylabel, logicallabel, or a group
 // Returns pointers into aa
 func (aa *AssignableAdapters) LookupIoBundleAny(name string) []*IoBundle {
-
  list := aa.LookupIoBundleGroup(name)
  if len(list) != 0 {
  return list
@@ -467,29 +532,46 @@ func (aa *AssignableAdapters) LookupIoBundleIfName(ifname string) *IoBundle {
  return nil
 }
 
-// CheckParentAssigngrp finds dependency loops between ioBundles
+var errOwnParent = errors.New("IOBundle cannot be it's own parent")
+
+var errParentAssigngrpMismatch = errors.New("IOBundle with parentassigngrp mismatch found")
+
+var errEmptyAssigngrpWithParent = errors.New("IOBundle with empty assigngrp cannot have a parent")
+
+var errCycleDetected = errors.New("Cycle detected, please check provided parentassigngrp/assigngrp")
+
+// CheckParentAssigngrp finds dependency loops between ioBundles and sets/clears the error
 func (aa *AssignableAdapters) CheckParentAssigngrp() bool {
  assigngrp2parent := make(map[string]string)
 
+ for i := range aa.IoBundleList {
+ ioBundle := &aa.IoBundleList[i]
+ for _, parentAssigngrpErr := range []error{
+ errOwnParent,
+ errParentAssigngrpMismatch,
+ errEmptyAssigngrpWithParent,
+ errCycleDetected,
+ } {
+ ioBundle.Error.removeByType(parentAssigngrpErr)
+ }
+ }
+
  var cycleDetectedAssigngrp string
  for i := range aa.IoBundleList {
  ioBundle := &aa.IoBundleList[i]
 
  if ioBundle.AssignmentGroup == ioBundle.ParentAssignmentGroup && ioBundle.AssignmentGroup != "" {
- ioBundle.Error = "IOBundle cannot be it's own parent"
- ioBundle.ErrorTime = time.Now()
+ ioBundle.Error.Append(errOwnParent)
  return true
  }
  parentassigngrp, ok := assigngrp2parent[ioBundle.AssignmentGroup]
  if ok && parentassigngrp != ioBundle.ParentAssignmentGroup {
- ioBundle.Error = "IOBundle with parentassigngrp mismatch found"
- ioBundle.ErrorTime = time.Now()
+ ioBundle.Error.Append(errParentAssigngrpMismatch)
  return true
  }
 
  if ioBundle.AssignmentGroup == "" && ioBundle.ParentAssignmentGroup != "" {
- ioBundle.Error = "IOBundle with empty assigngrp cannot have a parent"
- ioBundle.ErrorTime = time.Now()
+ ioBundle.Error.Append(errEmptyAssigngrpWithParent)
  return true
  }
  assigngrp2parent[ioBundle.AssignmentGroup] = ioBundle.ParentAssignmentGroup
@@ -523,17 +605,55 @@ func (aa *AssignableAdapters) CheckParentAssigngrp() bool {
  for i := range aa.IoBundleList {
  ioBundle := &aa.IoBundleList[i]
  if ioBundle.AssignmentGroup == cycleDetectedAssigngrp {
- ioBundle.Error = "Cycle detected, please check provided parentassigngrp/assigngrp"
- ioBundle.ErrorTime = time.Now()
+ ioBundle.Error.Append(errCycleDetected)
  }
  }
 
  return true
 }
 
-// CheckBadUSBBundles sets ib.Error/ErrorTime if bundle collides in regards of USB
+type ioBundleCollision struct {
+ phylabel string
+ usbaddr string
+ usbproduct string
+ pcilong string
+ assigngrp string
+}
+
+func (i ioBundleCollision) String() string {
+ return fmt.Sprintf("phylabel %s - usbaddr: %s usbproduct: %s pcilong: %s assigngrp: %s", i.phylabel, i.usbaddr, i.usbproduct, i.pcilong, i.assigngrp)
+}
+
+type ioBundleCollisionErr struct {
+ collisions []ioBundleCollision
+}
+
+func (i ioBundleCollisionErr) Error() string {
+ collisionErrStrPrefix := "ioBundle collision:"
+
+ collisionStrs := make([]string, 0, len(i.collisions))
+ for _, collision := range i.collisions {
+ collisionStrs = append(collisionStrs, collision.String())
+ }
+ collisionErrStrBody := strings.Join(collisionStrs, "||")
+
+ return fmt.Sprintf("%s||%s||", collisionErrStrPrefix, collisionErrStrBody)
+}
+
+func newIoBundleCollisionErr() ioBundleCollisionErr {
+ return ioBundleCollisionErr{
+ collisions: []ioBundleCollision{},
+ }
+}
+
+// CheckBadUSBBundles sets and clears ib.Error/ErrorTime if bundle collides in regards of USB
 func (aa *AssignableAdapters) CheckBadUSBBundles() {
  usbProductsAddressMap := make(map[[4]string][]*IoBundle)
+ for i := range aa.IoBundleList {
+ ioBundle := &aa.IoBundleList[i]
+ ioBundle.Error.removeByType(ioBundleCollisionErr{})
+ }
+
  for i := range aa.IoBundleList {
  ioBundle := &aa.IoBundleList[i]
  if ioBundle.UsbAddr == "" && ioBundle.UsbProduct == "" && ioBundle.PciLong == "" {
@@ -552,15 +672,19 @@ func (aa *AssignableAdapters) CheckBadUSBBundles() {
  continue
  }
 
- errStr := "ioBundle collision:||"
+ collisionErr := newIoBundleCollisionErr()
 
  for _, bundle := range bundles {
- errStr += fmt.Sprintf("phylabel %s - usbaddr: %s usbproduct: %s pcilong: %s assigngrp: %s||",
- bundle.Phylabel, bundle.UsbAddr, bundle.UsbProduct, bundle.PciLong, bundle.AssignmentGroup)
+ collisionErr.collisions = append(collisionErr.collisions, ioBundleCollision{
+ phylabel: bundle.Phylabel,
+ usbaddr: bundle.UsbAddr,
+ usbproduct: bundle.UsbProduct,
+ pcilong: bundle.PciLong,
+ assigngrp: bundle.AssignmentGroup,
+ })
  }
  for _, bundle := range bundles {
- bundle.Error = errStr
- bundle.ErrorTime = time.Now()
+ bundle.Error.Append(collisionErr)
  }
  }
 }
@@ -590,8 +714,7 @@ func (aa *AssignableAdapters) CheckBadAssignmentGroups(log *base.LogObject, PCIS
  err := fmt.Errorf("CheckBadAssignmentGroup: %s same PCI controller as %s; pci long %s vs %s",
  ib2.Ifname, ib.Ifname, ib2.PciLong, ib.PciLong)
  log.Error(err)
- ib.Error = err.Error()
- ib.ErrorTime = time.Now()
+ ib.Error.Append(err)
  changed = true
  }
  }