diff --git a/pkg/pillar/cmd/domainmgr/domainmgr.go b/pkg/pillar/cmd/domainmgr/domainmgr.go index ec1e8b6c441..421dc7d150a 100644 --- a/pkg/pillar/cmd/domainmgr/domainmgr.go +++ b/pkg/pillar/cmd/domainmgr/domainmgr.go @@ -1442,8 +1442,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", @@ -2163,9 +2163,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 } } @@ -2888,11 +2888,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 @@ -2928,8 +2926,7 @@ 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 @@ -2937,8 +2934,7 @@ func handlePhysicalIOAdapterListImpl(ctxArg interface{}, key string, if err != nil { err = fmt.Errorf("setupCAN: %w", err) log.Error(err) - ib.Error = err.Error() - ib.ErrorTime = time.Now() + ib.Error.Append(err) } } } @@ -2978,11 +2974,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) { @@ -3157,12 +3151,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 @@ -3235,9 +3227,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) diff --git a/pkg/pillar/cmd/zedagent/reportinfo.go b/pkg/pillar/cmd/zedagent/reportinfo.go index 04311a65550..a5ad028146f 100644 --- a/pkg/pillar/cmd/zedagent/reportinfo.go +++ b/pkg/pillar/cmd/zedagent/reportinfo.go @@ -530,12 +530,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 } diff --git a/pkg/pillar/types/assignableadapters.go b/pkg/pillar/types/assignableadapters.go index 33b467c8a31..17f46703ee3 100644 --- a/pkg/pillar/types/assignableadapters.go +++ b/pkg/pillar/types/assignableadapters.go @@ -29,6 +29,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. @@ -96,8 +163,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 @@ -410,7 +476,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 @@ -430,7 +495,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 @@ -460,6 +524,30 @@ func (aa *AssignableAdapters) LookupIoBundleIfName(ifname string) *IoBundle { return nil } +type ownParentError struct{} + +func (o ownParentError) Error() string { + return "IOBundle cannot be it's own parent" +} + +type parentAssigngrpMismatchError struct{} + +func (p parentAssigngrpMismatchError) Error() string { + return "IOBundle with parentassigngrp mismatch found" +} + +type emptyAssigngrpWithParentError struct{} + +func (e emptyAssigngrpWithParentError) Error() string { + return "IOBundle with empty assigngrp cannot have a parent" +} + +type cycleDetectedError struct{} + +func (c cycleDetectedError) Error() string { + return "Cycle detected, please check provided parentassigngrp/assigngrp" +} + // CheckParentAssigngrp finds dependency loops between ioBundles func (aa *AssignableAdapters) CheckParentAssigngrp() bool { assigngrp2parent := make(map[string]string) @@ -468,21 +556,26 @@ func (aa *AssignableAdapters) CheckParentAssigngrp() bool { for i := range aa.IoBundleList { ioBundle := &aa.IoBundleList[i] + for _, parentAssigngrpErr := range []error{ + ownParentError{}, + parentAssigngrpMismatchError{}, + emptyAssigngrpWithParentError{}, + cycleDetectedError{}, + } { + ioBundle.Error.removeByType(parentAssigngrpErr) + } if ioBundle.AssignmentGroup == ioBundle.ParentAssignmentGroup && ioBundle.AssignmentGroup != "" { - ioBundle.Error = "IOBundle cannot be it's own parent" - ioBundle.ErrorTime = time.Now() + ioBundle.Error.Append(ownParentError{}) 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(parentAssigngrpMismatchError{}) return true } if ioBundle.AssignmentGroup == "" && ioBundle.ParentAssignmentGroup != "" { - ioBundle.Error = "IOBundle with empty assigngrp cannot have a parent" - ioBundle.ErrorTime = time.Now() + ioBundle.Error.Append(emptyAssigngrpWithParentError{}) return true } assigngrp2parent[ioBundle.AssignmentGroup] = ioBundle.ParentAssignmentGroup @@ -516,14 +609,47 @@ 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(cycleDetectedError{}) } } return true } +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 ib.Error/ErrorTime if bundle collides in regards of USB func (aa *AssignableAdapters) CheckBadUSBBundles() { usbProductsAddressMap := make(map[[4]string][]*IoBundle) @@ -533,6 +659,7 @@ func (aa *AssignableAdapters) CheckBadUSBBundles() { continue } + ioBundle.Error.removeByType(ioBundleCollisionErr{}) id := [4]string{ioBundle.UsbAddr, ioBundle.UsbProduct, ioBundle.PciLong, ioBundle.AssignmentGroup} if usbProductsAddressMap[id] == nil { usbProductsAddressMap[id] = make([]*IoBundle, 0) @@ -545,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) } } } @@ -583,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 } } diff --git a/pkg/pillar/types/assignableadapters_test.go b/pkg/pillar/types/assignableadapters_test.go index 41930f3110c..f19071f3693 100644 --- a/pkg/pillar/types/assignableadapters_test.go +++ b/pkg/pillar/types/assignableadapters_test.go @@ -353,10 +353,10 @@ var aa2 = AssignableAdapters{ // Same indices as above var aa2Errors = []string{ - "CheckBadAssignmentGroup: eth3 same PCI controller as eth0; pci long 0000:f2:00.1 vs 0000:f2:00.0", - "CheckBadAssignmentGroup: eth3 same PCI controller as eth1; pci long 0000:f2:00.1 vs 0000:f2:00.0", - "CheckBadAssignmentGroup: eth3 same PCI controller as eth2; pci long 0000:f2:00.1 vs 0000:f2:00.0", - "CheckBadAssignmentGroup: eth2 same PCI controller as eth3; pci long 0000:f2:00.0 vs 0000:f2:00.1", + "CheckBadAssignmentGroup: eth2 same PCI controller as eth0; pci long 0000:f2:00.0 vs 0000:f2:00.0; CheckBadAssignmentGroup: eth3 same PCI controller as eth0; pci long 0000:f2:00.1 vs 0000:f2:00.0", + "CheckBadAssignmentGroup: eth2 same PCI controller as eth1; pci long 0000:f2:00.0 vs 0000:f2:00.0; CheckBadAssignmentGroup: eth3 same PCI controller as eth1; pci long 0000:f2:00.1 vs 0000:f2:00.0", + "CheckBadAssignmentGroup: eth0 same PCI controller as eth2; pci long 0000:f2:00.0 vs 0000:f2:00.0; CheckBadAssignmentGroup: eth1 same PCI controller as eth2; pci long 0000:f2:00.0 vs 0000:f2:00.0; CheckBadAssignmentGroup: eth3 same PCI controller as eth2; pci long 0000:f2:00.1 vs 0000:f2:00.0", + "CheckBadAssignmentGroup: eth0 same PCI controller as eth3; pci long 0000:f2:00.0 vs 0000:f2:00.1; CheckBadAssignmentGroup: eth1 same PCI controller as eth3; pci long 0000:f2:00.0 vs 0000:f2:00.1; CheckBadAssignmentGroup: eth2 same PCI controller as eth3; pci long 0000:f2:00.0 vs 0000:f2:00.1", "", "", "", @@ -384,7 +384,7 @@ func TestCheckBadAssignmentGroups(t *testing.T) { assert.Equal(t, len(aa2.IoBundleList), len(aa2Errors)) for i, ib := range aa2.IoBundleList { t.Logf("Running test case TestCheckBadAssignmentGroups[%d]", i) - assert.Equal(t, aa2Errors[i], ib.Error) + assert.Equal(t, aa2Errors[i], ib.Error.String()) } } @@ -494,14 +494,91 @@ func alternativeCheckBadUSBBundlesImpl(bundles []IoBundle) { } } - bundles[i].Error = errStr - bundles[j].Error = errStr + if errStr != "" { + bundles[i].Error.Append(fmt.Errorf(errStr)) + bundles[j].Error.Append(fmt.Errorf(errStr)) + } } } } -func FuzzCheckBadUSBBundles(f *testing.F) { +func TestClearingCycleErrors(t *testing.T) { + t.Parallel() + + aa := AssignableAdapters{} + bundles := make([]IoBundle, 2) + + bundles[0].Phylabel = "usb1" + bundles[1].Phylabel = "usb2" + + bundles[0].UsbAddr = "1:1" + bundles[1].UsbAddr = "1:2" + + bundles[0].AssignmentGroup = "a1" + bundles[1].AssignmentGroup = "a2" + + bundles[0].ParentAssignmentGroup = "a2" + bundles[1].ParentAssignmentGroup = "a1" + + aa.IoBundleList = bundles + + aa.CheckParentAssigngrp() + + errFound := func() bool { + found := false + for _, ioBundle := range aa.IoBundleList { + if ioBundle.Error.String() != "" { + found = true + } + } + return found + } + + if !errFound() { + t.Fatalf("no error found although there is a cycle: %+v", aa.IoBundleList) + } + + aa.IoBundleList[1].ParentAssignmentGroup = "p2" + aa.CheckParentAssigngrp() + if errFound() { + t.Fatalf("error found although there is no cycle anymore: %+v", aa.IoBundleList) + } +} + +func TestClearingUSBCollision(t *testing.T) { + t.Parallel() + aa := AssignableAdapters{} + bundles := make([]IoBundle, 2) + + bundles[0].Phylabel = "usb1" + bundles[1].Phylabel = "usb2" + + bundles[0].UsbAddr = "1:1" + bundles[1].UsbAddr = bundles[0].UsbAddr + aa.IoBundleList = bundles + + aa.CheckBadUSBBundles() + + for _, ioBundle := range aa.IoBundleList { + t.Logf("%s / %s", ioBundle.Phylabel, ioBundle.Error.String()) + if ioBundle.Error.String() == "" { + t.Fatalf("expected collision for ioBundle %s", ioBundle.Phylabel) + } + } + + aa.IoBundleList[0].UsbAddr = "1:2" + aa.IoBundleList[0].Error.Clear() + + aa.CheckBadUSBBundles() + for _, ioBundle := range aa.IoBundleList { + t.Logf("%s / %s", ioBundle.Phylabel, ioBundle.Error.String()) + if ioBundle.Error.String() != "" { + t.Fatalf("expected no collision for ioBundle %s", ioBundle.Phylabel) + } + } +} +func FuzzCheckBadUSBBundles(f *testing.F) { f.Fuzz(func(t *testing.T, // ioBundle 1 pciLong1 string, @@ -552,10 +629,10 @@ func FuzzCheckBadUSBBundles(f *testing.F) { failed := false for i := 0; i < len(bundles); i++ { - if bundles[i].Error != "" && alternativeCheckBundles[i].Error != "" { + if bundles[i].Error.String() != "" && alternativeCheckBundles[i].Error.String() != "" { continue } - if bundles[i].Error == "" && alternativeCheckBundles[i].Error == "" { + if bundles[i].Error.String() == "" && alternativeCheckBundles[i].Error.String() == "" { continue } @@ -565,7 +642,7 @@ func FuzzCheckBadUSBBundles(f *testing.F) { if failed { for i := 0; i < len(bundles); i++ { t.Logf("'%s' '%s' '%s' : '%s' <-> '%s'", bundles[i].PciLong, bundles[i].UsbAddr, bundles[i].UsbProduct, - bundles[i].Error, alternativeCheckBundles[i].Error) + bundles[i].Error.String(), alternativeCheckBundles[i].Error.String()) } t.Fatal("fail - check log") } @@ -593,7 +670,7 @@ func TestCheckBadParentAssigngrp(t *testing.T) { errorSet := false for _, ioBundle := range aa.IoBundleList { - if ioBundle.Error == "IOBundle with parentassigngrp mismatch found" { + if ioBundle.Error.String() == "IOBundle with parentassigngrp mismatch found" { errorSet = true break } @@ -625,7 +702,7 @@ func TestCheckBadParentAssigngrpLoop(t *testing.T) { for _, ioBundle := range aa.IoBundleList { if ioBundle.Phylabel == "2" { - if ioBundle.Error != "IOBundle cannot be it's own parent" { + if ioBundle.Error.String() != "IOBundle cannot be it's own parent" { t.Fatal("wrong error message") } } @@ -648,7 +725,7 @@ func TestCheckBadParentAssigngrpLoop(t *testing.T) { errorSet := false for _, ioBundle := range aa.IoBundleList { - if ioBundle.Error == "Cycle detected, please check provided parentassigngrp/assigngrp" { + if ioBundle.Error.String() == "Cycle detected, please check provided parentassigngrp/assigngrp" { errorSet = true break } @@ -746,10 +823,77 @@ func TestCheckBadUSBBundles(t *testing.T) { aa.CheckBadUSBBundles() for i, bundleWithErr := range testCase.bundleWithError { - if bundles[i].Error != bundleWithErr.expectedError { + if bundles[i].Error.String() != bundleWithErr.expectedError { t.Fatalf("bundle %s expected error \n'%s', got error \n'%s'", - bundleWithErr.bundle.Phylabel, bundleWithErr.expectedError, bundles[i].Error) + bundleWithErr.bundle.Phylabel, bundleWithErr.expectedError, bundles[i].Error.String()) } } } } + +type ( + testErr1 struct{} + testErr2 struct{} + testErr3 struct { + error + } + testErr4 struct { + error + } +) + +func (testErr1) Error() string { + return "err1" +} + +func (testErr2) Error() string { + return "err2" +} + +func TestIoBundleError(t *testing.T) { + iobe := ioBundleError{} + + iobe.Append(testErr1{}) + + if !iobe.hasError(testErr1{}) { + t.Fatal("has not error testErr1") + } + if iobe.hasError(testErr2{}) { + t.Fatal("has error testErr2, but shouldn't") + } + + if iobe.String() != "err1" { + t.Fatalf("expected error string to be 'err1', but got '%s'", iobe.String()) + } + + iobe.Append(testErr2{}) + + if iobe.String() != "err1; err2" { + t.Fatalf("expected error string to be 'err1; err2', but got '%s'", iobe.String()) + } + + iobe.Append(testErr1{}) + + iobe.removeByType(testErr1{}) + + if iobe.String() != "err2" { + t.Fatalf("expected error string to be 'err2', but got '%s'", iobe.String()) + } + if !iobe.hasError(testErr2{}) { + t.Fatal("has not error testErr2") + } + + err3 := testErr3{fmt.Errorf("err3")} + err4 := testErr4{fmt.Errorf("err4")} + iobe.Append(err3) + iobe.Append(err4) + + if iobe.String() != "err2; err3; err4" { + t.Fatalf("expected error string to be 'err2; err3; err4', but got '%s'", iobe.String()) + } + + iobe.removeByType(testErr3{}) + if iobe.String() != "err2; err4" { + t.Fatalf("expected error string to be 'err2; err4', but got '%s'", iobe.String()) + } +}