Opened 3 years ago

Closed 3 years ago

#3517 closed defect (fixed)

unitest IfaceMgrTest.detectIfaces failed on RedHat 6.4

Reported by: wlodekwencel Owned by: marcin
Priority: low Milestone: Kea0.9
Component: Unclassified Version: git
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: Core Feature Depending on Ticket:
Estimated Difficulty: 0 Add Hours to Ticket: 3
Total Hours: 3 Internal?: no

Description

Some of the interfaces are reported twice

[ RUN ] IfaceMgrTest?.detectIfaces
Checking interface lo
Checking interface eth0
Checking interface eth1
Checking interface eth2
Checking interface eth3
Checking interface eth4
Checking interface eth5
Checking interface virbr0
Checking interface virbr0
iface_mgr_unittest.cc:2669: Failure
Value of: checkIfIndex(*i, ifptr)

Actual: false

Expected: true
iface_mgr_unittest.cc:2672: Failure
Value of: checkIfFlags(*i, ifptr)

Actual: false

Expected: true
Checking interface virbr0-nic
Checking interface macvtap0
Checking interface macvtap1
Checking interface vnet0
Checking interface macvtap2
Checking interface virbr1
Checking interface virbr1
iface_mgr_unittest.cc:2669: Failure
Value of: checkIfIndex(*i, ifptr)

Actual: false

Expected: true
iface_mgr_unittest.cc:2672: Failure
Value of: checkIfFlags(*i, ifptr)

Actual: false

Expected: true
Checking interface virbr1-nic
Checking interface macvtap5
Checking interface macvtap6
Checking interface macvtap3
Checking interface macvtap4
Checking interface macvtap7
Checking interface macvtap8
Checking interface lo
Checking interface eth0
Checking interface eth4
Checking interface virbr0
Checking interface virbr1
Checking interface lo
Checking interface eth0
Checking interface eth0
Checking interface eth4
Checking interface macvtap0
Checking interface vnet0
Checking interface macvtap5
Checking interface macvtap3
Checking interface macvtap4
Checking interface macvtap7
Checking interface macvtap8
[ FAILED ] IfaceMgrTest?.detectIfaces (1 ms)

Please check projects: RedHat_6.4_64 and RedHat_6.4_64_branch_testing on Jenkins CI.

Subtickets

Change History (6)

comment:1 Changed 3 years ago by tomek

  • Milestone changed from Kea-proposed to Kea0.9
  • Priority changed from medium to low
  • Version set to git

I'm not sure, but I think we decided to remove it in 0.9. If I mis-remembered, please move this to 1.0.

comment:2 Changed 3 years ago by marcin

  • Owner set to marcin
  • Status changed from new to accepted

comment:3 Changed 3 years ago by marcin

  • Add Hours to Ticket changed from 0 to 3
  • Owner changed from marcin to UnAssigned
  • Status changed from accepted to reviewing
  • Total Hours changed from 0 to 3

It appeared to be a bogus test. Plus, it had nothing to do with the KVM running on the system under test, except that KVM creates pairs of virtual interfaces which have similar names: virbr0 and virbr0-nic. The test detected the interfaces using getifaddrs and then iterated over them trying to find a match with the interfaces detected by the IfaceMgr. The logic which was matching the interfaces was wrong because it didn't compare the full interface name but rather a part of it. As a result, the virbr0 and virbr0-nic were matched by the test and obviously the test failed when it was comparing properties of these two distinct interfaces.

Here is a proposed ChangeLog for this work:

XYZ.	[bug]		marcin
	Corrected a IfaceMgr::detectIfaces unit test which reported false
	positives for specific network configurations.
	(Trac #3517, git abcd)

comment:4 Changed 3 years ago by tomek

  • Owner changed from UnAssigned to tomek

comment:5 Changed 3 years ago by tomek

  • Owner changed from tomek to marcin

I reviewed the code and I'm a bit embarrassed. I tend to agree that your description of the unit test code as 'crappy' was a fair assessment :)

Your changes are good, but I have 2 minor requests.

  1. Please add one line comment before the test that explain what it does.
  1. There is a disabled test detectIfaces_linux. It's a leftover from around 3 years ago when we used ifconfig output parsing as a way to verify if netlink returns proper data. Can you remove that test and close #1529 as won't fix?

I verified that the code builds and unit-tests pass on Ubuntu 13.10 x64. My system did not have interfaces with the same prefix (e.g. eth0 and eth0-1), but the code should handle that now.

In changelog, please use test name (something users seen failing) rather the tested method name (which requires deep code knowledge to be sure that it's the same test).

I don't need to see this ticket again.

comment:6 Changed 3 years ago by marcin

  • Resolution set to fixed
  • Status changed from reviewing to closed

Review comments addressed as requested.

Merged with commit 9affa1b2210f5cc9d7a99724e5d5c8979409cefd

Note: See TracTickets for help on using tickets.