Skip to content

Commit

Permalink
pillar/assignableadapters: clear error strings
Browse files Browse the repository at this point in the history 
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 <[email protected]>
(cherry picked from commit d1e13b8)
  • Loading branch information
@christoph-zededa @eriknordmark
christoph-zededa authored and eriknordmark committed Sep 16, 2024
1 parent 37cce08 commit 77c2f13
Show file tree
Hide file tree
Showing 4 changed files with 330 additions and 63 deletions.
36 changes: 14 additions & 22 deletions pkg/pillar/cmd/domainmgr/domainmgr.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down Expand Up @@ -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
}
}
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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)
}
}
}
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand Down
8 changes: 4 additions & 4 deletions pkg/pillar/cmd/zedagent/reportinfo.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down
165 changes: 144 additions & 21 deletions pkg/pillar/types/assignableadapters.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ package types
// file on boot.

import (
"errors"
"fmt"
"os"
"reflect"
Expand All @@ -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.
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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 == "" {
Expand All @@ -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)
}
}
}
Expand Down Expand Up @@ -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
}
}
Expand Down
Loading

0 comments on commit 77c2f13

Please sign in to comment.