Add UT for pkg/agent/ipassigner/ip_assigner_linux #5402

rajnkamr · 2023-08-17T14:14:11Z

for #4142
Added UT for pkg/agent/ipassigner/ip_assigner_linux.go

pkg/agent/ipassigner/ip_assigner_linux.go

pkg/agent/ipassigner/ip_assigner_linux_test.go

tnqn · 2024-02-09T10:53:47Z

pkg/agent/ipassigner/ip_assigner_linux.go

+	netlinkAdd         = netlink.LinkAdd
+	netlinkDel         = netlink.LinkDel
+	ensureRPF          = util.EnsureRPFilterOnInterface
+	netlinkSetUp       = netlink.LinkSetUp
+	netInterfaceByName = net.InterfaceByName
+	netlinkAddrAdd     = netlink.AddrAdd
+	netlinkAddrDel     = netlink.AddrDel
+	netlinkAddrList    = netlink.AddrList


assignee heavily relies on netlink and there is an interface defined for it: pkg/agent/util/netlink.Interface. Instead of replacing each call repeatedly, assignee should hold the interface, and we just replace it with a fake implementation in unit test. pkg/agent/route/route_linux.go can be an example.

We have following 4 types supported in netlink interface, we can use mocked apis for these
netlink.AddrList
netlink.AddrDel
netlink.AddrAdd
netlink.LinkSetUp
However others like netlink.LinkAdd, netlink.LinkDel, net.InterfaceByName are not supported

netlink.LinkAdd, netlink.LinkDel can be added to the interface

This comment hasn't been resolved.

Had Tried earlier replacing while holding the interface for assignee, it might not work unlike pkg/agent/route/route_linux.go as in route_linux.go, it is using netlink of type utilnetlink.Interface which matches type of interface pkg/agent/util/netlink/netlink_linux.go while replacing

// Client takes care of routing container packets in host network, coordinating ip route, ip rule, iptables and ipset. type Client struct { netlink utilnetlink.Interface

In assign function, it is directly using external netlink pkg

func (as *assignee) assign(ip net.IP, subnetInfo *crdv1b1.SubnetInfo) error { // If there is a real link, add the IP to its address list. if as.link != nil { addr := getIPNet(ip, subnetInfo) if err := netlink.AddrAdd(as.link, &netlink.Addr{IPNet: addr}); err != nil {

mockNetlink.EXPECT().AddrAdd(dummyDevice.Attrs().Name, &netlink.Addr{IPNet: expectedIPNet}).Return(nil)
For example above expectation are never met wrt interface defined in pkg/agent/util/netlink/netlink_linux.go ! similarly for all other netlink apis in ipassigner is used directly from netlink.

tnqn · 2024-02-09T11:51:28Z

pkg/agent/ipassigner/ip_assigner_linux.go

@@ -528,6 +544,7 @@ func (a *ipAssigner) addVLANAssignee(link netlink.Link, vlan int32) (*assignee,
 		logicalInterface: iface,
 		link:             link,
 		ips:              sets.New[string](),
+		advertiseFn:      advertiseFnc,


Why setting it to a dummy function?

To replace advertise from test, specially for non vlan case

It doesn't explain why it's set to a dummy function here. This is not test code.

To mock advertise( replace defer and restore strategy for other apis was followed as mentioned in netlink, but not for advertise), advertise function is added in assignee struct, line 73 , advertiseFn func(ip net.IP, externalInterface *net.Interface), since vlan and non vlan use the structure assigne, a dummy function shld be added to avoid invalid memory address or nil pointer dereference

This is wrong. both vlan and non-vlan interfaces need to advertise.

advertise api syntax changed, now it is rebased

pkg/agent/ipassigner/ip_assigner_linux_test.go

github-actions · 2024-07-08T00:04:24Z

This PR is stale because it has been open 90 days with no activity. Remove stale label or comment, or this will be closed in 90 days

tnqn · 2024-07-08T06:37:58Z

pkg/agent/ipassigner/ip_assigner_linux.go

@@ -528,6 +544,7 @@ func (a *ipAssigner) addVLANAssignee(link netlink.Link, vlan int32) (*assignee,
 		logicalInterface: iface,
 		link:             link,
 		ips:              sets.New[string](),
+		advertiseFn:      advertiseFnc,


This is wrong. both vlan and non-vlan interfaces need to advertise.

tnqn · 2024-07-08T06:38:18Z

pkg/agent/ipassigner/ip_assigner_linux.go

+	netlinkAdd         = netlink.LinkAdd
+	netlinkDel         = netlink.LinkDel
+	ensureRPF          = util.EnsureRPFilterOnInterface
+	netlinkSetUp       = netlink.LinkSetUp
+	netInterfaceByName = net.InterfaceByName
+	netlinkAddrAdd     = netlink.AddrAdd
+	netlinkAddrDel     = netlink.AddrDel
+	netlinkAddrList    = netlink.AddrList


This comment hasn't been resolved.

tnqn · 2024-07-08T06:39:16Z

pkg/agent/ipassigner/ip_assigner_linux_test.go

+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"github.com/vishvananda/netlink"
+	gomock "go.uber.org/mock/gomock"


the alias is not needed

tnqn · 2024-07-08T06:43:51Z

pkg/agent/ipassigner/ip_assigner_linux_test.go

+	}
+}
+
+func TestIPAssigner_UnAssignIP(t *testing.T) {


Suggested change

func TestIPAssigner_UnAssignIP(t *testing.T) {

func TestIPAssigner_UnassignIP(t *testing.T) {

Signed-off-by: Rajnish Kumar <rajnishk1@vmware.com>

rajnkamr added the area/test Issues or PRs related to unit and integration tests. label Aug 24, 2023

rajnkamr force-pushed the ipassigner branch 2 times, most recently from 96f7a5f to c730c77 Compare September 29, 2023 09:29

rajnkamr force-pushed the ipassigner branch from 765269f to b18f057 Compare October 9, 2023 06:12

rajnkamr marked this pull request as ready for review October 10, 2023 10:05

rajnkamr added this to the Antrea v1.14 release milestone Oct 11, 2023

rajnkamr requested a review from luolanzone October 16, 2023 06:15

tnqn reviewed Oct 17, 2023

View reviewed changes

pkg/agent/ipassigner/ip_assigner_linux.go Outdated Show resolved Hide resolved

pkg/agent/ipassigner/ip_assigner_linux_test.go Outdated Show resolved Hide resolved

luolanzone modified the milestones: Antrea v1.14 release, Antrea v1.15 release Oct 18, 2023

luolanzone removed this from the Antrea v1.15 release milestone Jan 8, 2024

rajnkamr marked this pull request as draft January 10, 2024 04:13

rajnkamr force-pushed the ipassigner branch 2 times, most recently from e7c5d4c to b2d889c Compare January 15, 2024 06:03

rajnkamr force-pushed the ipassigner branch from b2d889c to 505b050 Compare January 22, 2024 11:59

rajnkamr force-pushed the ipassigner branch 3 times, most recently from b800266 to 591707d Compare February 5, 2024 05:38

rajnkamr force-pushed the ipassigner branch from 591707d to e089225 Compare February 8, 2024 05:07

rajnkamr marked this pull request as ready for review February 8, 2024 06:33

tnqn reviewed Feb 9, 2024

View reviewed changes

rajnkamr force-pushed the ipassigner branch 2 times, most recently from 47d1b56 to d5b8495 Compare March 29, 2024 08:49

rajnkamr force-pushed the ipassigner branch from 245a177 to 4112403 Compare April 8, 2024 15:23

github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 8, 2024

rajnkamr force-pushed the ipassigner branch from 4112403 to 8438b38 Compare July 8, 2024 05:58

tnqn reviewed Jul 8, 2024

View reviewed changes

github-actions bot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 9, 2024

IPAssigner Vlan Unit Test

1aea270

Signed-off-by: Rajnish Kumar <rajnishk1@vmware.com>

IpAssigner vlan & nonvlan UT

149e206

Signed-off-by: Rajnish Kumar <rajnishk1@vmware.com>

rajnkamr force-pushed the ipassigner branch from 8438b38 to 149e206 Compare July 19, 2024 10:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add UT for pkg/agent/ipassigner/ip_assigner_linux #5402

Add UT for pkg/agent/ipassigner/ip_assigner_linux #5402

rajnkamr commented Aug 17, 2023 •

edited

Loading

tnqn Feb 9, 2024

rajnkamr Mar 5, 2024

tnqn Mar 5, 2024

tnqn Jul 8, 2024

rajnkamr Jul 9, 2024

tnqn Feb 9, 2024

rajnkamr Mar 5, 2024

tnqn Mar 5, 2024

rajnkamr Mar 6, 2024

tnqn Jul 8, 2024

rajnkamr Jul 19, 2024

github-actions bot commented Jul 8, 2024

tnqn Jul 8, 2024

tnqn Jul 8, 2024

tnqn Jul 8, 2024

tnqn Jul 8, 2024

	func TestIPAssigner_UnAssignIP(t *testing.T) {
	func TestIPAssigner_UnassignIP(t *testing.T) {

Add UT for pkg/agent/ipassigner/ip_assigner_linux #5402

Are you sure you want to change the base?

Add UT for pkg/agent/ipassigner/ip_assigner_linux #5402

Conversation

rajnkamr commented Aug 17, 2023 • edited Loading

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

github-actions bot commented Jul 8, 2024

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

rajnkamr commented Aug 17, 2023 •

edited

Loading