pillar/assignableadapters: clear error strings #4238

christoph-zededa · 2024-09-11T13:40:51Z

create two usb adapters with collision
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

christoph-zededa · 2024-09-11T13:41:11Z

This only happens when the manifest has been changed; should I backport this?

pkg/pillar/types/assignableadapters.go

OhmSpectator · 2024-09-11T13:54:06Z

This only happens when the manifest has been changed; should I backport this?

I would say yes... It's a bug fix...

OhmSpectator

Looks good. I think it should be backported.

OhmSpectator · 2024-09-11T13:58:56Z

Waiting for https://api.github.com/repos/lf-edge/eve/actions/workflows/build.yml/runs?head_sha=01880429c1dd5e1dbc2b1da62c894b29a3032dba&status=success to return a non-empty list and rerun the tests.

christoph-zededa · 2024-09-11T16:53:21Z

I think the same issue exists for the cycle detection of parentassigngrp - I will have a look tomorrow.

OhmSpectator · 2024-09-11T17:13:46Z

I think the same issue exists for the cycle detection of parentassigngrp - I will have a look tomorrow.

Ok, but we can still merge it in different PRs, if you want

pkg/pillar/types/assignableadapters.go

pkg/pillar/cmd/domainmgr/domainmgr.go

OhmSpectator

In general, looks fine. Minor comments only.

OhmSpectator · 2024-09-12T14:29:51Z

Oh, I expected the go tests to pass =D

christoph-zededa · 2024-09-12T14:32:06Z

Oh, I expected the go tests to pass =D

fix for it is:

diff --git a/pkg/pillar/types/assignableadapters_test.go b/pkg/pillar/types/assignableadapters_test.go
index 82ddab7a1..f19071f36 100644
--- a/pkg/pillar/types/assignableadapters_test.go
+++ b/pkg/pillar/types/assignableadapters_test.go
@@ -494,8 +494,10 @@ func alternativeCheckBadUSBBundlesImpl(bundles []IoBundle) {
                                }
                        }
 
-                       bundles[i].Error.Append(fmt.Errorf(errStr))
-                       bundles[j].Error.Append(fmt.Errorf(errStr))
+                       if errStr != "" {
+                               bundles[i].Error.Append(fmt.Errorf(errStr))
+                               bundles[j].Error.Append(fmt.Errorf(errStr))
+                       }
                }
        }
 }

I will fixup ...

OhmSpectator

Looks good.

eriknordmark · 2024-09-13T09:10:09Z

pkg/pillar/types/assignableadapters.go

+type ownParentError struct{}
+
+func (o ownParentError) Error() string {
+	return "IOBundle cannot be it's own parent"
+}
+


Is this approach the recommended golang approach to define error strings?
I think the old/original standard packages just define a well-known string for the exported errors.

In the standard library I see for example:

var EOF = errors.New("EOF")

for most of the newly introduced errors, this approach would work (I guess), but not for the collision error, because the error string is different depending with what it is colliding.

var me3 = errors.New("myErr3") var me4 = errors.New("myErr4") errors.Is(me3, me3)

evaluates to false, because it is evaluating the members.

An alternative approach would be to use errors.Is and have global instances of ownParentError, parentAssigngrpMismatchError, emptyAssigngrpWithParentError and cycleDetectedError and then create
the ioBundleCollisionErr with a

func (i ioBundleCollisisonErr) Is(target err) bool { return reflect.TypeOf(i) == reflect.TypeOf(m) }

But are those the same error if they are different collisions?

I have to think about it a bit more.

But are those the same error if they are different collisions?

I have to think about it a bit more.

But the Is() implementation can consider the fields in the error struct as well...

But the Is() implementation can consider the fields in the error struct as well...

I think that is the default implementation anyways.

I am now using errors.New and according to the documentation:

func New(text string) error
New returns an error that formats as the given text. Each call to New
returns a distinct error value even if the text is identical.

so that works and the collisionError still uses different error strings, but because I check with reflect.TypeOf they are detected as the same.

eriknordmark · 2024-09-13T09:16:44Z

pkg/pillar/types/assignableadapters.go

@@ -533,6 +659,7 @@ func (aa *AssignableAdapters) CheckBadUSBBundles() {
 			continue
 		}

+		ioBundle.Error.removeByType(ioBundleCollisionErr{})


My understanding (which could be wrong) is that when there is a collision the collision error is added to both of the colliding bundles. Is that correct?

If it is, then wouldn't you want to remove all the collisions before scanning for new collisions i.e., do the removal in a separate loop?

the collision error is added to both of the colliding bundles

That is correct.

do the removal in a separate loop?

I think it is not a problem doing it this way, because the cycleDetectedError is added in a separate loop.
But I will change it, because indeed it makes it more readable.

eriknordmark · 2024-09-13T09:17:51Z

pkg/pillar/types/assignableadapters.go

+		collisions: []ioBundleCollision{},
+	}
+}
+
 // CheckBadUSBBundles sets ib.Error/ErrorTime if bundle collides in regards of USB


Can you update the comment to say that this re-evaluates/updates the set of errors (is it only collision errors) across all of the bundles?
THe current comment doesn't indicate that it will clear existing errors.

now: // CheckBadUSBBundles sets and clears ib.Error/ErrorTime if bundle collides in regards of USB

eriknordmark · 2024-09-13T09:19:29Z

pkg/pillar/types/assignableadapters.go

+func (c cycleDetectedError) Error() string {
+	return "Cycle detected, please check provided parentassigngrp/assigngrp"
+}
+
 // CheckParentAssigngrp finds dependency loops between ioBundles


Can you update the comment to say that it will re-evaluate/update (clear and set any new) errors relating to the parent assigngrp. Current comment implies it doesn't clear any.

now: // CheckParentAssigngrp finds dependency loops between ioBundles and sets/clears the error

eriknordmark

For some reason a number of eden tests fail and the log shows the state of an app instance flip-flopping between INSTALLED, UNKNOWN, and INITIAL.

I can't see how the iobundle code can do that, but didn't see it on earlier PRs.

OhmSpectator · 2024-09-13T13:30:42Z

@christoph-zededa rebase the branch, please... Otherwise, it will show the linter issue. I'll try to fix it now

christoph-zededa · 2024-09-13T15:22:43Z

Seems this is the issue:

2024-09-13T15:10:27.875945177Z;pillar;panic: cannot handle unexported field at {types.AssignableAdapters}.IoBundleList[0].Error.errors:
2024-09-13T15:10:27.875951972Z;pillar;	"github.com/lf-edge/eve/pkg/pillar/types".ioBundleError
2024-09-13T15:10:27.875976544Z;pillar;consider using a custom Comparer; if you control the implementation of type, you can also consider using an Exporter, AllowUnexported, o
r cmpopts.IgnoreUnexported
2024-09-13T15:10:27.875977919Z;pillar;
2024-09-13T15:10:27.875986675Z;pillar;goroutine 36220 [running]:
2024-09-13T15:10:27.875987181Z;pillar;github.com/google/go-cmp/cmp.validator.apply({{}}, 0xc000ee2d20, {0x26f3460?, 0xc00125a0f8?, 0x0?}, {0x26f3460?, 0xc0014498f8?, 0xc00125
a0f8?})
2024-09-13T15:10:27.875998276Z;pillar;	/pillar/vendor/github.com/google/go-cmp/cmp/options.go:248 +0x3aa
2024-09-13T15:10:27.877006718Z;pillar;github.com/google/go-cmp/cmp.(*state).tryOptions(0xc000ee2d20, {0x30f8150, 0x26f3460}, {0x26f3460?, 0xc00125a0f8?, 0xc00210d7d0?}, {0x26
f3460?, 0xc0014498f8?, 0xc00210d750?})
2024-09-13T15:10:27.87701065Z;pillar;	/pillar/vendor/github.com/google/go-cmp/cmp/compare.go:306 +0x11a
2024-09-13T15:10:27.877411058Z;pillar;github.com/google/go-cmp/cmp.(*state).compareAny(0xc000ee2d20, {0x30cfd10, 0xc002db6600?})
2024-09-13T15:10:27.877414473Z;pillar;	/pillar/vendor/github.com/google/go-cmp/cmp/compare.go:261 +0x525
2024-09-13T15:10:27.877693073Z;pillar;github.com/google/go-cmp/cmp.(*state).compareStruct(0xc000ee2d20, {0x30f8150, 0x2915ee0}, {0x2915ee0?, 0xc00125a0f8?, 0x10bcda5?}, {0x29
15ee0?, 0xc0014498f8?, 0xc00210dae8?})
2024-09-13T15:10:27.87769625Z;pillar;	/pillar/vendor/github.com/google/go-cmp/cmp/compare.go:414 +0x58b
2024-09-13T15:10:27.877718656Z;pillar;github.com/google/go-cmp/cmp.(*state).compareAny(0xc000ee2d20, {0x30cfd10, 0xc002db6500?})
2024-09-13T15:10:27.877719001Z;pillar;	/pillar/vendor/github.com/google/go-cmp/cmp/compare.go:289 +0xd65
2024-09-13T15:10:27.878080884Z;pillar;github.com/google/go-cmp/cmp.(*state).compareStruct(0xc000ee2d20, {0x30f8150, 0x2b4ec80}, {0x2b4ec80?, 0xc00125a000?, 0x27f65c0?}, {0x2b
4ec80?, 0xc001449800?, 0x10bfb39?})
2024-09-13T15:10:27.87808408Z;pillar;	/pillar/vendor/github.com/google/go-cmp/cmp/compare.go:414 +0x58b
2024-09-13T15:10:27.878097241Z;pillar;github.com/google/go-cmp/cmp.(*state).compareAny(0xc000ee2d20, {0x30cfce0, 0xc002dacae0?})
2024-09-13T15:10:27.878097689Z;pillar;	/pillar/vendor/github.com/google/go-cmp/cmp/compare.go:289 +0xd65
2024-09-13T15:10:27.878367966Z;pillar;github.com/google/go-cmp/cmp.(*state).statelessCompare(0xc000ee2d20, {0x30cfce0?, 0xc002dacae0?})
2024-09-13T15:10:27.878370888Z;pillar;	/pillar/vendor/github.com/google/go-cmp/cmp/compare.go:232 +0x85
2024-09-13T15:10:27.878456493Z;pillar;github.com/google/go-cmp/cmp.(*state).compareSlice.func2(0x10b9f45?, 0x0?)
2024-09-13T15:10:27.878459066Z;pillar;	/pillar/vendor/github.com/google/go-cmp/cmp/compare.go:480 +0x5f
2024-09-13T15:10:27.878534069Z;pillar;github.com/google/go-cmp/cmp/internal/diff.Difference(0x3, 0x3, 0xc00210e378)
2024-09-13T15:10:27.878536377Z;pillar;	/pillar/vendor/github.com/google/go-cmp/cmp/internal/diff/diff.go:244 +0x5fe
2024-09-13T15:10:27.878659423Z;pillar;github.com/google/go-cmp/cmp.(*state).compareSlice(0xc000ee2d20, {0x30f8150, 0x26faba0}, {0x26faba0?, 0xc000c86ce8?, 0xc00210e468?}, {0x
26faba0?, 0xc002d49508?, 0xc00210e478?})
2024-09-13T15:10:27.878724899Z;pillar;	/pillar/vendor/github.com/google/go-cmp/cmp/compare.go:479 +0x9b8
2024-09-13T15:10:27.878794989Z;pillar;github.com/google/go-cmp/cmp.(*state).compareAny(0xc000ee2d20, {0x30cfd10, 0xc002db6400?})
2024-09-13T15:10:27.878850443Z;pillar;	/pillar/vendor/github.com/google/go-cmp/cmp/compare.go:291 +0xc4c
2024-09-13T15:10:27.878957923Z;pillar;github.com/google/go-cmp/cmp.(*state).compareStruct(0xc000ee2d20, {0x30f8150, 0x2a515a0}, {0x2a515a0?, 0xc000c86ce0?, 0x0?}, {0x2a515a0?
, 0xc002d49500?, 0xc002d49500?})
2024-09-13T15:10:27.879015036Z;pillar;	/pillar/vendor/github.com/google/go-cmp/cmp/compare.go:414 +0x58b
2024-09-13T15:10:27.879094488Z;pillar;github.com/google/go-cmp/cmp.(*state).compareAny(0xc000ee2d20, {0x30ccc50, 0xc0022c4e00?})
2024-09-13T15:10:27.879151874Z;pillar;	/pillar/vendor/github.com/google/go-cmp/cmp/compare.go:289 +0xd65
2024-09-13T15:10:27.879240235Z;pillar;github.com/google/go-cmp/cmp.Equal({0x2a515a0, 0xc000c86ce0}, {0x2a515a0, 0xc002d49500}, {0x0?, 0xc002d77900?, 0x25e?})
2024-09-13T15:10:27.879242749Z;pillar;	/pillar/vendor/github.com/google/go-cmp/cmp/compare.go:97 +0x69
2024-09-13T15:10:27.87931925Z;pillar;github.com/lf-edge/eve/pkg/pillar/pubsub.(*PublicationImpl).Publish(0xc000f69b80, {0x2b92616, 0x6}, {0x2a515a0?, 0xc002d49480})
2024-09-13T15:10:27.879388223Z;pillar;	/pillar/pubsub/publish.go:115 +0x315
2024-09-13T15:10:27.879470586Z;pillar;github.com/lf-edge/eve/pkg/pillar/cmd/domainmgr.(*domainContext).publishAssignableAdapters(0xc00026b180)
2024-09-13T15:10:27.879472733Z;pillar;	/pillar/cmd/domainmgr/domainmgr.go:138 +0xbc
2024-09-13T15:10:27.879581413Z;pillar;github.com/lf-edge/eve/pkg/pillar/cmd/domainmgr.reserveAdapters(_, {{{0xd7, 0x7d, 0xc9, 0x9f, 0x81, 0x2a, 0x4d, 0x4, 0xab, ...}, ...}, .
..})
2024-09-13T15:10:27.87966029Z;pillar;	/pillar/cmd/domainmgr/domainmgr.go:2209 +0x3ec
2024-09-13T15:10:27.879753909Z;pillar;github.com/lf-edge/eve/pkg/pillar/cmd/domainmgr.doActivate(_, {{{0xd7, 0x7d, 0xc9, 0x9f, 0x81, 0x2a, 0x4d, 0x4, 0xab, ...}, ...}, ...}, 
...)
2024-09-13T15:10:27.87981818Z;pillar;	/pillar/cmd/domainmgr/domainmgr.go:1550 +0x1f8
2024-09-13T15:10:27.879897426Z;pillar;github.com/lf-edge/eve/pkg/pillar/cmd/domainmgr.handleCreate(0xc000b3df00?, {0xc001a96720?, 0x24?}, 0xc0027e8840)
2024-09-13T15:10:27.879977064Z;pillar;	/pillar/cmd/domainmgr/domainmgr.go:1358 +0x854
2024-09-13T15:10:27.880032699Z;pillar;github.com/lf-edge/eve/pkg/pillar/cmd/domainmgr.runHandler(0xc00026b180, {0xc001a96720, 0x24}, 0xc0027574a0, 0xc002757500)
2024-09-13T15:10:27.880090109Z;pillar;	/pillar/cmd/domainmgr/domainmgr.go:891 +0x5b7
2024-09-13T15:10:27.880092289Z;pillar;created by github.com/lf-edge/eve/pkg/pillar/cmd/domainmgr.handleDomainCreate
2024-09-13T15:10:27.880222581Z;pillar;	/pillar/cmd/domainmgr/domainmgr.go:829 +0x3ec

OhmSpectator · 2024-09-13T15:31:12Z

Seems this is the issue:

Why was it not caught by the unit test?..

christoph-zededa · 2024-09-13T15:38:28Z

Why was it not caught by the unit test?..

There was no unit test; now -> https://github.com/lf-edge/eve/pull/4238/files#diff-12fc573707d1d64e810b0b070ad566c6f912c07d10bd1891f13b5d7f9ef95274R902

OhmSpectator · 2024-09-13T15:52:53Z

Why was it not caught by the unit test?..

There was no unit test; now -> https://github.com/lf-edge/eve/pull/4238/files#diff-12fc573707d1d64e810b0b070ad566c6f912c07d10bd1891f13b5d7f9ef95274R902

Argh... The issue was not the error itself but the structure that contained this error got it.
It's still strange no linter/analyzer got it. I thought CodeQL was able to catch such stuff.

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>

OhmSpectator · 2024-09-14T09:31:40Z

So, @christoph-zededa, now let's do the backports =)

christoph-zededa requested a review from eriknordmark as a code owner September 11, 2024 13:40

christoph-zededa force-pushed the clear_err_on_usb_collision branch from 1eb1e11 to 06869fa Compare September 11, 2024 13:45

OhmSpectator reviewed Sep 11, 2024

View reviewed changes

pkg/pillar/types/assignableadapters.go Outdated Show resolved Hide resolved

christoph-zededa force-pushed the clear_err_on_usb_collision branch from 06869fa to 0188042 Compare September 11, 2024 13:55

OhmSpectator approved these changes Sep 11, 2024

View reviewed changes

OhmSpectator added bug Something isn't working stable Should be backported to stable release(s) and removed bug Something isn't working labels Sep 11, 2024

christoph-zededa force-pushed the clear_err_on_usb_collision branch from 0188042 to 0755f2f Compare September 11, 2024 15:29

OhmSpectator approved these changes Sep 11, 2024

View reviewed changes

christoph-zededa marked this pull request as draft September 11, 2024 16:52

christoph-zededa force-pushed the clear_err_on_usb_collision branch from 0755f2f to fa8b1a1 Compare September 12, 2024 09:42

christoph-zededa marked this pull request as ready for review September 12, 2024 10:51

christoph-zededa requested review from milan-zededa and rouming as code owners September 12, 2024 13:09

OhmSpectator reviewed Sep 12, 2024

View reviewed changes

pkg/pillar/types/assignableadapters.go Outdated Show resolved Hide resolved

github-actions bot requested review from jsfakian, OhmSpectator and uncleDecart September 12, 2024 13:09

christoph-zededa force-pushed the clear_err_on_usb_collision branch 2 times, most recently from 01f9ca3 to 7eccd17 Compare September 12, 2024 13:16

OhmSpectator reviewed Sep 12, 2024

View reviewed changes

pkg/pillar/cmd/domainmgr/domainmgr.go Outdated Show resolved Hide resolved

christoph-zededa force-pushed the clear_err_on_usb_collision branch from 7eccd17 to 72f7847 Compare September 12, 2024 13:38

github-actions bot requested a review from OhmSpectator September 12, 2024 13:38

christoph-zededa force-pushed the clear_err_on_usb_collision branch from 72f7847 to 80f1910 Compare September 12, 2024 14:02

OhmSpectator approved these changes Sep 12, 2024

View reviewed changes

OhmSpectator self-requested a review September 12, 2024 14:29

christoph-zededa force-pushed the clear_err_on_usb_collision branch 3 times, most recently from f02dc67 to 502a7ea Compare September 12, 2024 15:05

OhmSpectator approved these changes Sep 12, 2024

View reviewed changes

eriknordmark reviewed Sep 13, 2024

View reviewed changes

christoph-zededa force-pushed the clear_err_on_usb_collision branch from 502a7ea to 5f90fb9 Compare September 13, 2024 13:25

github-actions bot requested review from eriknordmark and OhmSpectator September 13, 2024 13:26

christoph-zededa force-pushed the clear_err_on_usb_collision branch from 5f90fb9 to d999e90 Compare September 13, 2024 13:33

christoph-zededa force-pushed the clear_err_on_usb_collision branch from d999e90 to 977b46d Compare September 13, 2024 15:38

OhmSpectator approved these changes Sep 13, 2024

View reviewed changes

christoph-zededa force-pushed the clear_err_on_usb_collision branch from 977b46d to 28a08bb Compare September 13, 2024 16:13

github-actions bot requested a review from OhmSpectator September 13, 2024 16:13

OhmSpectator approved these changes Sep 13, 2024

View reviewed changes

eriknordmark merged commit d1e13b8 into lf-edge:master Sep 13, 2024
39 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pillar/assignableadapters: clear error strings #4238

pillar/assignableadapters: clear error strings #4238

christoph-zededa commented Sep 11, 2024

christoph-zededa commented Sep 11, 2024

OhmSpectator commented Sep 11, 2024

OhmSpectator left a comment

OhmSpectator commented Sep 11, 2024

christoph-zededa commented Sep 11, 2024

OhmSpectator commented Sep 11, 2024

OhmSpectator left a comment

OhmSpectator commented Sep 12, 2024

christoph-zededa commented Sep 12, 2024

OhmSpectator left a comment

eriknordmark Sep 13, 2024

christoph-zededa Sep 13, 2024

OhmSpectator Sep 13, 2024

christoph-zededa Sep 13, 2024

eriknordmark Sep 13, 2024

christoph-zededa Sep 13, 2024

eriknordmark Sep 13, 2024

christoph-zededa Sep 13, 2024

eriknordmark Sep 13, 2024

christoph-zededa Sep 13, 2024

eriknordmark left a comment

OhmSpectator commented Sep 13, 2024

christoph-zededa commented Sep 13, 2024

OhmSpectator commented Sep 13, 2024

christoph-zededa commented Sep 13, 2024 •

edited

Loading

OhmSpectator commented Sep 13, 2024

OhmSpectator commented Sep 14, 2024

pillar/assignableadapters: clear error strings #4238

pillar/assignableadapters: clear error strings #4238

Conversation

christoph-zededa commented Sep 11, 2024

christoph-zededa commented Sep 11, 2024

OhmSpectator commented Sep 11, 2024

OhmSpectator left a comment

Choose a reason for hiding this comment

OhmSpectator commented Sep 11, 2024

christoph-zededa commented Sep 11, 2024

OhmSpectator commented Sep 11, 2024

OhmSpectator left a comment

Choose a reason for hiding this comment

OhmSpectator commented Sep 12, 2024

christoph-zededa commented Sep 12, 2024

OhmSpectator left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eriknordmark left a comment

Choose a reason for hiding this comment

OhmSpectator commented Sep 13, 2024

christoph-zededa commented Sep 13, 2024

OhmSpectator commented Sep 13, 2024

christoph-zededa commented Sep 13, 2024 • edited Loading

OhmSpectator commented Sep 13, 2024

OhmSpectator commented Sep 14, 2024

christoph-zededa commented Sep 13, 2024 •

edited

Loading