ioBundle->NetworkPortStatus matching tightening for IsPort() #4460

andrewd-zededa · 2024-12-04T16:19:01Z

Check all three identifiers: ifname, usbaddr, pciaddr
Some io types can have an empty string in one of these fields.
Expand matching conditions to ensure ioBundles are not incorrectly matched to a different NetworkPort.

While modifying the parameters to types.IsPort() I discovered an unused isPort() which referenced a lock that appears to be used nowhere else and removed both.

pkg/pillar/types/dns.go

eriknordmark

LGTM but are there other functions which compare on ifname which should also compare on all three fields?

pkg/pillar/types/dns.go

andrewd-zededa · 2024-12-09T23:52:38Z

LGTM but are there other functions which compare on ifname which should also compare on all three fields?

@eriknordmark there are a series of other helper functions in types/dns.go (possibly elsewhere too): IsL3Port(), IsMgmtPort(), GetPortCost(), ...and all of these appear to only check Ifname for equivalence of the device. At the moment I'm not sure how many of these are relevant for device passthrough specifically.

They all follow the same pattern of

		if us.IfName != ifname {
			continue
		}

Maybe this should be split out to some form of a IsSameDevice() call (or some better name).

eriknordmark · 2024-12-10T09:30:33Z

@eriknordmark there are a series of other helper functions in types/dns.go (possibly elsewhere too): IsL3Port(), IsMgmtPort(), GetPortCost(), ...and all of these appear to only check Ifname for equivalence of the device. At the moment I'm not sure how many of these are relevant for device passthrough specifically.

Maybe this should be split out to some form of a IsSameDevice() call (or some better name).

You'd actually need to create some Lookup function if you want to break out the comparison of the different identifiers.

Looking at the functions you listed:

IsMgmtPort() is only called for ifnames which are in DeviceNetworkStatus.Ports. But based on Milan's comment we can have modems which do not have an ifname, so makes sense to add the args there.
IsL3Port() is no longer used hence can be deleted.
GetPortCost() has the same considerations as IsMgmtPort.

eriknordmark · 2024-12-10T09:34:42Z

But I don't understand if we can have empty ifname arguments to these functions. There are other functions in dns.go which build a list of ifnames and then use that to get a list of IP addresses, and that will produce the wrong thing if a modem interface without an ifname is an actual Port which has an IP address. So I'm a bit confused - need to re-read Milan's earlier comment.

andrewd-zededa · 2024-12-10T14:41:02Z

@eriknordmark Milan's comment references propagatePhyioAttrsToPort() which calls handleMissingIfname() so the NetworkPortConfig seems like it should be safe but somehow NetworkPortStatus had an empty field.

andrewd-zededa · 2024-12-10T16:02:28Z

Latest pushes include USBProd plumbing through NetworkPortConfig/Status removal of unused IsL3Port() and spreading the expanded check of usbaddr, usbprod, pciaddr to IsMgmtPort() and GetPortCost()

pkg/pillar/types/dns.go

andrewd-zededa · 2024-12-10T23:54:14Z

@eriknordmark @milan-zededa when this is approved/merged I need to backport to one or more stable/lts branches.

rene · 2024-12-11T14:00:45Z

@andrewd-zededa , please, update the commit message to add USBProd as well and mention why you have removed the isPort() function (not used anywhere)...

rene

LGTM... kicking off tests...

rucoder · 2024-12-11T14:02:08Z

LGTM... kicking off tests...

@rene still not clear whether @andrewd-zededa has tested on real device

rene · 2024-12-11T14:06:07Z

LGTM... kicking off tests...

@rene still not clear whether @andrewd-zededa has tested on real device

ok, let's run the tests at least, he still needs to update the commit message, so when he pushes the final version we can review it again....

Check all four identifiers: ifname, usbaddr, usbprod, pciaddr Some io types can have an empty string in one of these fields. Expand matching conditions to ensure ioBundles are not incorrectly matched to a different NetworkPort. Removed unused code isPort() in domainmgr and IsL3Port(). Signed-off-by: Andrew Durbin <andrewd@zededa.com>

andrewd-zededa · 2024-12-11T14:40:49Z

@rene updated commit message with your requests

eriknordmark

I don't know if this is all we need - I'm concerned that we do not set ifname for all WWAN ports which would make them not be used in zedcloud/send.go. But hard to tell how much of an issue this is when we erroneously think COM ports should be used by TCP/IP. So it makes sense to merge this and keep on testing.

andrewd-zededa · 2024-12-11T23:08:05Z

I'm reviewing the eden test failures and here are the first three failures all in the Log/Info/Metric lim test, unable to ssh. Not sure why yet

smoke ext4 true
FAIL: ../lim/testdata/log_test.txt:16: command failure
[background] bash ssh.sh: signal: killed
[stdout]
time="2024-12-11T21:11:25Z" level=fatal msg="command ssh failed: exit status 255"
Connection timed out during banner exchange
Connection to 127.0.0.1 port 2222 timed out
smoke ext4 false
FAIL: ../lim/testdata/log_test.txt:16: command failure
[background] bash ssh.sh: exit status 0
[stdout]
time="2024-12-11T21:11:30Z" level=fatal msg="command ssh failed: exit status 255"
Connection timed out during banner exchange
Connection to 127.0.0.1 port 2222 timed out
smoke zfs true
FAIL: ../lim/testdata/log_test.txt:16: command failure
[background] bash ssh.sh: exit status 0
[stdout]
time="2024-12-11T21:11:01Z" level=fatal msg="command ssh failed: exit status 255"
Connection timed out during banner exchange
Connection to 127.0.0.1 port 2222 timed out

OhmSpectator · 2024-12-12T15:16:05Z

@uncleDecart, do you have any ideas on why the test failed? =)

uncleDecart · 2024-12-12T18:50:19Z

@uncleDecart, do you have any ideas on why the test failed? =)

@OhmSpectator, @andrewd-zededa does it run locally? Could be that EVE crashes and then you can't ssh

eriknordmark · 2024-12-12T19:23:53Z

FWIW I kicked off ztests on old of the old test machines in the lab and that seems to be fine.

But I also did a qemu run on my laptop using
make rootfs installer-raw run-installer-raw run-target
and after that virtual device is onboarded to the controller it gets stuck.

tail -F /run/diag.out shows among other things
ERROR: App test-ztest-ubunu uuid 91a07a2f-2606-4a50-8067-84a9300e21b9 state DOWNLOADING error: Found error in content tree Ztest-xenial-amd64 attached to volume test-ztest-ubunu_0_m_1: No IP management port addresses with cost <= 0

cat /run/nim/DeviceNetworkStatus/* |jq shows an error
"LastError": "link not up for interface eth0 (down)",

And ip link has eth0 but not the actual hardware interface keth0 (eth0 is a bridge so we can attach e.g., switch network instances to eth0, while keth0 is the actual hardware device driver).

So something is seriously amiss here, even though I can update a running physical device to the image from this PR and it runs fine.

eriknordmark · 2024-12-12T19:30:48Z

pkg/pillar/types/dns.go

-func IsL3Port(dns DeviceNetworkStatus, ifname string) bool {
-	for _, us := range dns.Ports {
-		if us.IfName != ifname {
+		if us.USBAddr != usbaddr {


I wonder whether we need to treat empty strings as wildcards in some of these comparisons.

Turns out that is at least required for PCIAddr in IsPort since the comparison is between the model aka SystemAdapter, which might have an empty PCIAddr and the discovered PCIAddr on the running system, which is not empty.

eriknordmark · 2024-12-12T19:32:45Z

pkg/pillar/types/dns.go

@@ -732,7 +751,7 @@ func GetPortCost(dns DeviceNetworkStatus, ifname string) uint8 {

 func GetPort(dns DeviceNetworkStatus, ifname string) *NetworkPortStatus {
 	for _, us := range dns.Ports {
-		if us.IfName != ifname {
+		if ifname != "" && us.IfName != ifname {


Do we have callers which pass an empty string as the ifname argument? If they do this will match on the first management port which might not be what we want.

getLocalAddrListImpl() only calls it with a non empty ifname.

Could VerifyAllIntf() pass an empty string?

andrewd-zededa · 2024-12-12T23:45:45Z

@eriknordmark @uncleDecart @mikem-zed @milan-zededa @OhmSpectator @rene

Maybe a simpler and quicker to test fix would be to guard against the ib.Type == types.IoCom

In the scope of domainmgr.go where it calls types.IsPort() change it to

if types.IsPort(ctx.deviceNetworkStatus, ib.Ifname) && (ib.Type != types.IoCom){
    isPort = true
    keepInHost = true
}

Or something similar.

eriknordmark · 2024-12-13T09:34:19Z

pkg/pillar/types/dns.go

 	for _, us := range dns.Ports {
-		if us.IfName != ifname {
+		if ifname != "" && us.IfName != ifname {


Why this change? It causes the complete breakage since a lookup of e.g. COM1 or audio0 (which do not have an ifname by definition) will match the first item in dns.Ports. If that first item is a network port like eth0, then IsPort will incorrectly return true.

OhmSpectator · 2024-12-13T15:49:56Z

Do we still need this PR after merging #4482 ?

eriknordmark · 2024-12-13T16:37:12Z

Superceeded by #4482

github-actions bot requested a review from eriknordmark December 4, 2024 16:19

milan-zededa reviewed Dec 4, 2024

View reviewed changes

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

andrewd-zededa force-pushed the iobundle_ifname_port branch from 9d96dc3 to cf08c8e Compare December 9, 2024 21:05

github-actions bot requested review from jsfakian, milan-zededa, OhmSpectator and uncleDecart December 9, 2024 21:06

andrewd-zededa changed the title ~~Guard against empty ifname argument.~~ ioBundle->NetworkPortStatus matching tightening for IsPort() Dec 9, 2024

eriknordmark reviewed Dec 9, 2024

View reviewed changes

christoph-zededa reviewed Dec 9, 2024

View reviewed changes

pkg/pillar/types/dns.go Show resolved Hide resolved

andrewd-zededa marked this pull request as ready for review December 10, 2024 00:02

andrewd-zededa force-pushed the iobundle_ifname_port branch from cf08c8e to 25f7bf7 Compare December 10, 2024 15:57

andrewd-zededa requested a review from rouming as a code owner December 10, 2024 15:57

github-actions bot requested a review from eriknordmark December 10, 2024 15:57

andrewd-zededa force-pushed the iobundle_ifname_port branch from 25f7bf7 to f6e6db6 Compare December 10, 2024 16:00

andrewd-zededa requested a review from christoph-zededa December 10, 2024 16:07

milan-zededa reviewed Dec 10, 2024

View reviewed changes

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

andrewd-zededa force-pushed the iobundle_ifname_port branch from f6e6db6 to a68769d Compare December 10, 2024 22:54

github-actions bot requested a review from milan-zededa December 10, 2024 22:54

rene added the stable Should be backported to stable release(s) label Dec 11, 2024

rene approved these changes Dec 11, 2024

View reviewed changes

andrewd-zededa force-pushed the iobundle_ifname_port branch from a68769d to 6fc578e Compare December 11, 2024 14:40

OhmSpectator approved these changes Dec 11, 2024

View reviewed changes

eriknordmark approved these changes Dec 11, 2024

View reviewed changes

eriknordmark reviewed Dec 12, 2024

View reviewed changes

andrewd-zededa mentioned this pull request Dec 13, 2024

Skip marking Com port as a zedrouterport #4482

Merged

eriknordmark reviewed Dec 13, 2024

View reviewed changes

eriknordmark closed this Dec 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ioBundle->NetworkPortStatus matching tightening for IsPort() #4460

ioBundle->NetworkPortStatus matching tightening for IsPort() #4460

andrewd-zededa commented Dec 4, 2024 •

edited

Loading

eriknordmark left a comment

andrewd-zededa commented Dec 9, 2024 •

edited

Loading

eriknordmark commented Dec 10, 2024

eriknordmark commented Dec 10, 2024

andrewd-zededa commented Dec 10, 2024

andrewd-zededa commented Dec 10, 2024

andrewd-zededa commented Dec 10, 2024

rene commented Dec 11, 2024

rene left a comment

rucoder commented Dec 11, 2024

rene commented Dec 11, 2024

andrewd-zededa commented Dec 11, 2024

eriknordmark left a comment

andrewd-zededa commented Dec 11, 2024

OhmSpectator commented Dec 12, 2024

uncleDecart commented Dec 12, 2024

eriknordmark commented Dec 12, 2024

eriknordmark Dec 12, 2024

eriknordmark Dec 12, 2024

eriknordmark Dec 12, 2024

andrewd-zededa Dec 12, 2024

andrewd-zededa commented Dec 12, 2024 •

edited

Loading

eriknordmark Dec 13, 2024

OhmSpectator commented Dec 13, 2024

eriknordmark commented Dec 13, 2024

ioBundle->NetworkPortStatus matching tightening for IsPort() #4460

ioBundle->NetworkPortStatus matching tightening for IsPort() #4460

Conversation

andrewd-zededa commented Dec 4, 2024 • edited Loading

eriknordmark left a comment

Choose a reason for hiding this comment

andrewd-zededa commented Dec 9, 2024 • edited Loading

eriknordmark commented Dec 10, 2024

eriknordmark commented Dec 10, 2024

andrewd-zededa commented Dec 10, 2024

andrewd-zededa commented Dec 10, 2024

andrewd-zededa commented Dec 10, 2024

rene commented Dec 11, 2024

rene left a comment

Choose a reason for hiding this comment

rucoder commented Dec 11, 2024

rene commented Dec 11, 2024

andrewd-zededa commented Dec 11, 2024

eriknordmark left a comment

Choose a reason for hiding this comment

andrewd-zededa commented Dec 11, 2024

OhmSpectator commented Dec 12, 2024

uncleDecart commented Dec 12, 2024

eriknordmark commented Dec 12, 2024

eriknordmark Dec 12, 2024

Choose a reason for hiding this comment

eriknordmark Dec 12, 2024

Choose a reason for hiding this comment

eriknordmark Dec 12, 2024

Choose a reason for hiding this comment

andrewd-zededa Dec 12, 2024

Choose a reason for hiding this comment

andrewd-zededa commented Dec 12, 2024 • edited Loading

eriknordmark Dec 13, 2024

Choose a reason for hiding this comment

OhmSpectator commented Dec 13, 2024

eriknordmark commented Dec 13, 2024

andrewd-zededa commented Dec 4, 2024 •

edited

Loading

andrewd-zededa commented Dec 9, 2024 •

edited

Loading

andrewd-zededa commented Dec 12, 2024 •

edited

Loading