Handle non-directory entries in /sys/class/net #52

ganawaj · 2024-12-24T02:23:55Z

Problem:

If non-directory entries are in /sys/class/net - the driver fails to enumerate interface macs.

Solution:

Before attempting to open /sys/class/net/[interface]/address, checks if /sys/class/net/[interface] is a directory. If it is not, ignore it and continue enumerating the interfaces

Related Issue:

harvester/harvester#7232

Test plan:

All test pass running make ci, built and pushed and tested in cluster with the following logs:

time="2024-12-24T02:13:22Z" level=info msg="Current node macs: [96:61:c4:bd:44:df 86:39:76:2a:5b:0e d2:87:e6:c1:44:f6 0a:05:df:c4:a8:a3 2e:a7:96:bc:b8:66 36:5c:6c:53:f5:3d a6:b0:1e:7e:87:35 76:89:f8:8b:f0:72 4a:88:eb:2a:8a:29 22:46:26:ab:9c:8f c6:6e:ce:5f:61:d0 d6:fb:8e:0b:2e:91 5a:b2:a1:09:74:7f 8a:ec:e4:13:74:84 8a:ec:71:12:15:74 0e:2a:0b:b2:f6:30 32:5c:8b:85:54:11 fe:20:bf:f1:a9:eb]"
time="2024-12-24T02:13:22Z" level=info msg="Compared iface: {10.4.4.133 76:70:fd:ad:9c:63 default [10.4.4.133 fe80::7470:fdff:fead:9c63] eth0 domain, guest-agent, multus-status 1}, mac: 76:70:fd:ad:9c:63"
time="2024-12-24T02:13:22Z" level=info msg="Compared iface: { da:bb:e8:a6:7e:a9  [] bond0 guest-agent 0}, mac: da:bb:e8:a6:7e:a9"
time="2024-12-24T02:13:22Z" level=info msg="Compared iface: { 56:b6:6a:ae:d5:5d  [] dummy0 guest-agent 0}, mac: 56:b6:6a:ae:d5:5d"
time="2024-12-24T02:13:22Z" level=info msg="Compared iface: {fdae:41e4:649b:9303:f6b1:f698:b096:210 00:00:00:00:00:00  [fdae:41e4:649b:9303:f6b1:f698:b096:210] siderolink domain, guest-agent 0}, mac: 00:00:00:00:00:00"
time="2024-12-24T02:13:22Z" level=info msg="Compared iface: {fe80::98f9:93ff:fe46:a73e 9a:f9:93:46:a7:3e  [fe80::98f9:93ff:fe46:a73e] cilium_net guest-agent 0}, mac: 9a:f9:93:46:a7:3e"
time="2024-12-24T02:13:22Z" level=info msg="Compared iface: {10.244.0.220 9e:96:8e:ff:70:f7  [10.244.0.220 fe80::9c96:8eff:feff:70f7] cilium_host guest-agent 0}, mac: 9e:96:8e:ff:70:f7"
time="2024-12-24T02:13:22Z" level=info msg="Compared iface: {fe80::e0af:67ff:fe16:bfb5 e2:af:67:16:bf:b5  [fe80::e0af:67ff:fe16:bfb5] cilium_vxlan guest-agent 0}, mac: e2:af:67:16:bf:b5"
time="2024-12-24T02:13:22Z" level=info msg="Compared iface: {fe80::e456:ceff:fe25:8bd5 e6:56:ce:25:8b:d5  [fe80::e456:ceff:fe25:8bd5] lxc_health guest-agent 0}, mac: e6:56:ce:25:8b:d5"
time="2024-12-24T02:13:22Z" level=info msg="Compared iface: {fe80::182d:2ff:fe35:d7bd 1a:2d:02:35:d7:bd  [fe80::182d:2ff:fe35:d7bd] lxc337c32147b15 guest-agent 0}, mac: 1a:2d:02:35:d7:bd"
time="2024-12-24T02:13:22Z" level=info msg="Compared iface: {fe80::d048:42ff:fe1f:edca d2:48:42:1f:ed:ca  [fe80::d048:42ff:fe1f:edca] lxc15d6f7b00d91 guest-agent 0}, mac: d2:48:42:1f:ed:ca"
time="2024-12-24T02:13:22Z" level=info msg="Compared iface: {fe80::b4b8:ebff:feb7:58e9 b6:b8:eb:b7:58:e9  [fe80::b4b8:ebff:feb7:58e9] lxcc18f02aa2727 guest-agent 0}, mac: b6:b8:eb:b7:58:e9"
time="2024-12-24T02:13:22Z" level=info msg="Compared iface: {fe80::a42b:acff:feb1:f388 a6:2b:ac:b1:f3:88  [fe80::a42b:acff:feb1:f388] lxc3d45b2854b27 guest-agent 0}, mac: a6:2b:ac:b1:f3:88"
time="2024-12-24T02:13:22Z" level=info msg="Current node macs: [96:61:c4:bd:44:df 86:39:76:2a:5b:0e d2:87:e6:c1:44:f6 0a:05:df:c4:a8:a3 2e:a7:96:bc:b8:66 36:5c:6c:53:f5:3d a6:b0:1e:7e:87:35 76:89:f8:8b:f0:72 4a:88:eb:2a:8a:29 22:46:26:ab:9c:8f c6:6e:ce:5f:61:d0 d6:fb:8e:0b:2e:91 5a:b2:a1:09:74:7f 8a:ec:e4:13:74:84 8a:ec:71:12:15:74 0e:2a:0b:b2:f6:30 32:5c:8b:85:54:11 fe:20:bf:f1:a9:eb]"
time="2024-12-24T02:13:22Z" level=info msg="Compared iface: {10.4.4.131 36:5c:6c:53:f5:3d default [10.4.4.131 fe80::345c:6cff:fe53:f53d] eth0 domain, guest-agent, multus-status 1}, mac: 36:5c:6c:53:f5:3d"
time="2024-12-24T02:13:22Z" level=info msg="Discovered Harvester VM node ID" node_id_discovered=pool-xdy-auth-k8s1-c623a657 node_id_original=talos-t4r-bk9

Vicente-Cheng

thanks for the enhancement!

ihcsim · 2025-01-07T01:08:28Z

pkg/sysfsnet/interfaces_linux.go

+		if !info.IsDir() {
+			return "", nil
+		}


What if we do this check earlier, and call readMACFromFile() only if it's a dir?

Will this work?

diff --git a/pkg/sysfsnet/interfaces_linux.go b/pkg/sysfsnet/interfaces_linux.go index f4a295f..138b788 100644 --- a/pkg/sysfsnet/interfaces_linux.go +++ b/pkg/sysfsnet/interfaces_linux.go @@ -39,6 +39,9 @@ func Interfaces() ([]Interface, error) { } for _, dentry := range dents { + if !dentry.IsDir() { + continue + } hwText, err := readMACFromFile(filepath.Join(sysClassNet, dentry.Name(), "address")) if os.IsNotExist(err) { continue

I agree this is much cleaner - I'll make the change and test locally

Thanks - looks like we are very close to merging this. Let me know if you need anything.

Looks good now with that change, I did have to call os.Stat() first becuase os.DirEntry(dentry) resolves to a symlink and IsDir()/FileInfo doesn't check the target

ihcsim · 2025-01-17T21:12:48Z

pkg/sysfsnet/interfaces_linux.go

+		// os.Stat to follow symlinks and return the info of the target
+		info, err := os.Stat(entryPath)
+		if err != nil {
+			continue


I think we should at least log err as a warning before continue. That may be other reasons why os.Stat(entryPath) could fail, and we don't want it to always fail silently.

@ganawaj would you mind improving that?

ihcsim · 2025-01-17T21:15:54Z

pkg/sysfsnet/interfaces_test.go

@@ -45,4 +50,9 @@ func TestInterfaces(t *testing.T) {
 	if !reflect.DeepEqual(want, got) {
 		t.Fatalf("want MACs=%v, got MACs=%v", want, got)
 	}
+
+	// Ensure non-directory entry is ignored
+	if _, exists := got[""]; exists {


I think got[""] is too broad. Can you add some test string to the bonding_masters file and then make sure they aren't present in got.

Vicente-Cheng

LGTM, ensure the @ihcsim's suggestion could be addressed.

Vicente-Cheng · 2025-02-04T01:48:58Z

pkg/sysfsnet/interfaces_linux.go

+		// os.Stat to follow symlinks and return the info of the target
+		info, err := os.Stat(entryPath)
+		if err != nil {
+			continue


@ganawaj would you mind improving that?

ihcsim · 2025-02-04T22:56:02Z

@ganawaj Thanks again for your help on this PR. Since this one is quite important, I am going to cherry-pick your commits into another PR (#55), to make sure it gets in before our upcoming code freeze. Feel free to review it at #55. Let me know if you have any questions.

@Vicente-Cheng @WebberHuang1118 PTAL at #55. Thanks.

ganawaj · 2025-02-04T23:13:13Z

@ganawaj Thanks again for your help on this PR. Since this one is quite important, I am going to cherry-pick your commits into another PR (#55), to make sure it gets in before our upcoming code freeze. Feel free to review it at #55. Let me know if you have any questions.

@Vicente-Cheng @WebberHuang1118 PTAL at #55. Thanks.

Sounds good - apologize for the delay on this. Had the commits a couple days ago after testing but hadn't pushed it yet - but if everything looks good feel free to close this one in favor of #55

ihcsim · 2025-02-04T23:15:23Z

pkg/sysfsnet/interfaces_linux.go

@@ -23,7 +25,9 @@ func Interfaces() ([]Interface, error) {
 	ifaces := make([]Interface, 0, 16)

 	readMACFromFile := func(s string) (string, error) {
+


Nit: can we remove the extra line break? Thanks.

ihcsim · 2025-02-04T23:15:29Z

pkg/sysfsnet/interfaces_linux.go

 		f, err := os.Open(s)
+


Nit: can we remove the extra line break? Thanks.

ihcsim · 2025-02-04T23:15:35Z

pkg/sysfsnet/interfaces_linux.go

@@ -39,7 +43,23 @@ func Interfaces() ([]Interface, error) {
 	}

 	for _, dentry := range dents {
-		hwText, err := readMACFromFile(filepath.Join(sysClassNet, dentry.Name(), "address"))
+


Nit: can we remove the extra line break? Thanks.

ihcsim · 2025-02-04T23:15:51Z

pkg/sysfsnet/interfaces_test.go

 	}

 	if !reflect.DeepEqual(want, got) {
 		t.Fatalf("want MACs=%v, got MACs=%v", want, got)
 	}
+


Nit: can we remove the extra line break? Thanks.

patch and test interface names

7eccfc1

innobead requested review from Vicente-Cheng and WebberHuang1118 December 24, 2024 02:25

Vicente-Cheng requested a review from ihcsim December 25, 2024 02:20

Vicente-Cheng approved these changes Jan 3, 2025

View reviewed changes

ihcsim reviewed Jan 7, 2025

View reviewed changes

ganawaj added 2 commits January 17, 2025 11:22

simplify check for dir

4baee3a

move parent check outside of readMACFromFile

a146b48

ihcsim reviewed Jan 17, 2025

View reviewed changes

Vicente-Cheng self-requested a review January 20, 2025 02:48

ganawaj added 2 commits January 24, 2025 10:55

add comments and readjust tests

b99847b

add comments and readjust tests

a2904de

Vicente-Cheng approved these changes Feb 4, 2025

View reviewed changes

ihcsim reviewed Feb 4, 2025

View reviewed changes

ganawaj added 2 commits February 4, 2025 17:20

test invalids do not show in final list

c1cd51f

fmt to remove line breaks

f9ce5d9

ihcsim approved these changes Feb 4, 2025

View reviewed changes

ihcsim merged commit d257036 into harvester:master Feb 4, 2025
4 checks passed

ihcsim mentioned this pull request Feb 5, 2025

[BUG] Harvester CSI fails to enumerate network interfaces when /sys/class/net contains non-directory entries harvester/harvester#7232

Closed

ganawaj deleted the patch-interface-names branch March 30, 2025 19:07

		@@ -23,7 +25,9 @@ func Interfaces() ([]Interface, error) {
		ifaces := make([]Interface, 0, 16)

		readMACFromFile := func(s string) (string, error) {

Handle non-directory entries in /sys/class/net #52

Handle non-directory entries in /sys/class/net #52

Uh oh!

Conversation

ganawaj commented Dec 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Vicente-Cheng left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ihcsim Jan 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Vicente-Cheng left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ihcsim commented Feb 4, 2025

Uh oh!

ganawaj commented Feb 4, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ganawaj commented Dec 24, 2024 •

edited

Loading

ihcsim Jan 17, 2025 •

edited

Loading