Allow to set/get socket timeout option for default handle

[ArthurChiao]

Ref: #613

Signed-off-by: arthurchiao [email protected]

[brb]

Hey @aboch , could you take a look?

[aboch]

Why do we need to make this a pointer?
Can't we still set the socket TO in init()?

[ArthurChiao]

Hi @aboch thanks for reviewing!

Not sure if I understand your comment correctly: if pkgHandle was initialized as &Handle{} as previously, the pkgHandle.sockets would be an empty map, and we can not set TO values for these sockets.

[aboch]

Thanks @ArturChiao now I remember.

With your proposed change, any binary depending on vishvananda/netlink will open 3 nl sockets, even when no netlink request is actually being executed (https://github.com/vishvananda/netlink/blob/v1.1.0/nl/nl_linux.go#L34).

Current behavior of this library has been: Open the nl socket only when you execute a pkg method (AddAddr(), AddLink(),...).

The addition of the Handle thing was done in a way so that above behavior was not modified.

I know it is limiting, but have you considered setting a default TO in func (req *NetlinkRequest) Execute() at https://github.com/vishvananda/netlink/blob/v1.1.0/nl/nl_linux.go#L406 instead ?

At the end of the day, a minute timeout (the default you are proposing) should satisfy most if not all use-cases.

[ArthurChiao]

Thanks for the explanations! @aboch

Set TO for each socket we're going to use makes sense to me.
But I'm not sure if you would like to make the TO value configurable or not (e.g. hardcode 1 minute).

[ArthurChiao]

Refactored to set TO after we got a socket, please have a review, thanks! @aboch

[brb]

PTAL @aboch

[Ewocker]

I was trying to see what was the problem for the patch.
For test case TestAddrAddReplace the output of AddList originally is [127.0.0.2/24 lo] whereas with the patch there is an extra entry [127.0.0.1/8 lo 127.0.0.2/24 lo].

[ArthurChiao]

Hi @Ewocker, thanks for your reminder, I'll take a look at it!

[Ewocker]

Hi @Ewocker, thanks for your reminder, I'll take a look at it!

@ArthurChiao, asking out of curiosity, why was test TestAddrSubscribeWithOptions in original PR hitting the 1m TO? Also, why would the AddrAdd call create 127.0.0.1/8 in test TestAddrAddReplace ?

don't know too much about netlink, would really appreciate if you can answer my questions 😁

[ArthurChiao]

@Ewocker actually I can't reproduce it in my develop env :(

And I found that even without my changes, many tests would fail in my dev node (already with root privilege), but with different reasons, such as the following one:

$ go test -v -run TestAddrAdd
=== RUN   TestAddrAdd
    TestAddrAdd: addr_test.go:90: Address flags not set properly, got=128, expected=140
--- FAIL: TestAddrAdd (0.00s)
=== RUN   TestAddrAddReplace
--- PASS: TestAddrAddReplace (0.00s)
FAIL
exit status 1
FAIL    github.com/vishvananda/netlink  0.004s

and this one:

--- FAIL: TestSEG6RouteAddDel (0.00s)
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
        panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x38 pc=0x64863e]

goroutine 230 [running]:
testing.tRunner.func1.1(0x693980, 0x8ece40)
        /usr/local/go/src/testing/testing.go:940 +0x2f5
testing.tRunner.func1(0xc000305440)
        /usr/local/go/src/testing/testing.go:943 +0x3f9
panic(0x693980, 0x8ece40)
        /usr/local/go/src/runtime/panic.go:969 +0x166
github.com/vishvananda/netlink.TestSEG6RouteAddDel(0xc000305440)
        /home/arthurchiao/go/src/github.com/vishvananda/netlink/route_test.go:1176 +0xa9e

Haven't dug into these problems.

[brb]

Thanks, LGTM!

[nr17]

@Ewocker actually I can't reproduce it in my develop env :(

And I found that even without my changes, many tests would fail in my dev node (already with root privilege), but with different reasons, such as the following one:

$ go test -v -run TestAddrAdd
=== RUN   TestAddrAdd
    TestAddrAdd: addr_test.go:90: Address flags not set properly, got=128, expected=140
--- FAIL: TestAddrAdd (0.00s)
=== RUN   TestAddrAddReplace
--- PASS: TestAddrAddReplace (0.00s)
FAIL
exit status 1
FAIL    github.com/vishvananda/netlink  0.004s

and this one:

--- FAIL: TestSEG6RouteAddDel (0.00s)
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
        panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x38 pc=0x64863e]

goroutine 230 [running]:
testing.tRunner.func1.1(0x693980, 0x8ece40)
        /usr/local/go/src/testing/testing.go:940 +0x2f5
testing.tRunner.func1(0xc000305440)
        /usr/local/go/src/testing/testing.go:943 +0x3f9
panic(0x693980, 0x8ece40)
        /usr/local/go/src/runtime/panic.go:969 +0x166
github.com/vishvananda/netlink.TestSEG6RouteAddDel(0xc000305440)
        /home/arthurchiao/go/src/github.com/vishvananda/netlink/route_test.go:1176 +0xa9e

Haven't dug into these problems.

The Makefile (line#22) runs 4 tests in parallel. Tests setup and cleanup the new ns using setUpNetlinkTest.(netlink/netlink_test.go). Although the ns is supposed to be thread-local by making use of runtime.LockOSThread(), if the test goroutines running in parallel share the same underlying OS thread, they will interfere with each other.