Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Explicitly set the IP on secondary ENIs #271

Merged
merged 2 commits into from
Jan 7, 2019
Merged

Explicitly set the IP on secondary ENIs #271

merged 2 commits into from
Jan 7, 2019

Conversation

ewbankkit
Copy link
Contributor

@ewbankkit ewbankkit commented Dec 17, 2018

Fixes #219.

Verified on Amazon Linux 2 and stock CentOS 7.

$ cat /etc/centos-release
CentOS Linux release 7.5.1804 (Core)
$ uname -r
3.10.0-862.14.4.el7.x86_64

@md2k
Copy link

md2k commented Dec 21, 2018

@ewbankkit found issue when i did restart for aws-node pods L-IPAMD failed to start with error below:

2018-12-21T12:27:43Z [DEBUG] Setting up ENI's primary IP 100.64.22.53
2018-12-21T12:27:43Z [ERROR] Failed to setup eni eni-04beceffa0accb441 network: failed to setup eni eni-04beceffa0accb441 network: eni network setup: failed to add IP addr to ENI: file exists
2018-12-21T12:27:43Z [ERROR] initialization failureFailed to setup eni eni-04beceffa0accb441: failed to setup eni eni-04beceffa0accb441 network: eni network setup: failed to add IP addr to ENI: file exists

@ewbankkit
Copy link
Contributor Author

@md2k Thanks - I guess I'll need to add check for existing address on the device.

@ewbankkit ewbankkit changed the title Explicitly set the IP on secondary ENIs [WIP] Explicitly set the IP on secondary ENIs Dec 21, 2018
@ewbankkit
Copy link
Contributor Author

ewbankkit commented Dec 21, 2018

If I blindly just don't add the address on restart then the routes fail to add:

2018-12-21T17:14:07Z [DEBUG] Setting up ENI's primary IP 10.x.x.x
2018-12-21T17:14:07Z [DEBUG] Existing IP address netlink.Addr{IPNet:(*net.IPNet)(0xc42034f1d0), Label:"eth1", Flags:128, Scope:0, Peer:(*net.IPNet)(0xc42034f1a0), Broadcast:net.IP{0xa, 0xbe, 0x3f, 0x3f}, PreferedLft:4294967295, ValidLft:4294967295}
2018-12-21T17:14:07Z [DEBUG] Setting up ENI's default gateway 10.y.y.y
2018-12-21T17:14:07Z [DEBUG] Successfully added route route 10.y.y.y/0 via 10.y.y.y table 2
2018-12-21T17:14:07Z [DEBUG] Not able to add route route 0.0.0.0/0 via 10.y.y.y table 2 (attempt 1/5)
2018-12-21T17:14:12Z [DEBUG] Not able to add route route 0.0.0.0/0 via 10.y.y.y table 2 (attempt 2/5)
2018-12-21T17:14:17Z [DEBUG] Not able to add route route 0.0.0.0/0 via 10.y.y.y table 2 (attempt 3/5)
2018-12-21T17:14:22Z [DEBUG] Not able to add route route 0.0.0.0/0 via 10.y.y.y table 2 (attempt 4/5)
2018-12-21T17:14:27Z [DEBUG] Not able to add route route 0.0.0.0/0 via 10.y.y.y table 2 (attempt 5/5)
2018-12-21T17:14:32Z [ERROR] Failed to add route 0.0.0.0/0 via 10.y.y.ytable 2
2018-12-21T17:14:32Z [ERROR] Failed to setup eni eni-052e2177de14fc5a7 network: failed to setup eni eni-052e2177de14fc5a7 network: eni network setup: failed unable to add route 0.0.0.0/0 via 10.y.y.y table 2: network is unreachable
2018-12-21T17:14:32Z [ERROR] initialization failureFailed to setup eni eni-052e2177de14fc5a7: failed to setup eni eni-052e2177de14fc5a7 network: eni network setup: failed unable to add route 0.0.0.0/0 via 10.y.y.y table 2: network is unreachable

@ewbankkit
Copy link
Contributor Author

ewbankkit commented Dec 21, 2018

Deleting any existing address on the secondary ENI and then adding it again works:

2018-12-21T18:32:34Z [DEBUG] Setting up ENI's primary IP 10.x.x.x
2018-12-21T18:32:34Z [DEBUG] Deleting existing IP address 10.x.x.x/26 eth0
2018-12-21T18:32:34Z [DEBUG] Adding IP address 10.x.x.x/26
2018-12-21T18:32:34Z [DEBUG] Setting up ENI's default gateway 10.y.y.y
2018-12-21T18:32:34Z [DEBUG] Successfully added route route 10.y.y.y/0 via 10.y.y.y table 2
2018-12-21T18:32:34Z [DEBUG] Successfully added route route 0.0.0.0/0 via 10.y.y.y table 2

@ewbankkit
Copy link
Contributor Author

Verified that it also works as expected on stock Ubuntu 16.04:

$ uname -r
4.4.0-1073-aws
2018-12-21T20:06:41Z [DEBUG] Setting up ENI's primary IP 100.64.x.x
2018-12-21T20:06:41Z [DEBUG] Deleting existing IP address 100.64.x.x/20 ens5
2018-12-21T20:06:41Z [DEBUG] Adding IP address 100.64.x.x/20
2018-12-21T20:06:41Z [DEBUG] Setting up ENI's default gateway 100.64.y.y
2018-12-21T20:06:41Z [DEBUG] Successfully added route route 100.64.y.y/0 via 100.64.y.y table 3
2018-12-21T20:06:41Z [DEBUG] Successfully added route route 0.0.0.0/0 via 100.64.y.y table 3

Set netmask equal to subnet's netmask.
Check for existing IP on secondary ENI before adding.
Attempt to fix unit tests.
@ewbankkit ewbankkit changed the title [WIP] Explicitly set the IP on secondary ENIs Explicitly set the IP on secondary ENIs Dec 21, 2018
@ewbankkit
Copy link
Contributor Author

I can't seem to get the unit tests to pass.

=== RUN   TestSetupENINetwork
--- FAIL: TestSetupENINetwork (0.00s)
    <autogenerated>:1: Unexpected call to *mock_netlink.MockLink.Attrs([]) at /home/kit/wrk/src/github.com/aws/amazon-vpc-cni-k8s/pkg/netlinkwrapper/mock_netlink/link_mocks.go:52 because: 
        Expected call at /home/kit/wrk/src/github.com/aws/amazon-vpc-cni-k8s/pkg/networkutils/network_test.go:93 has already been called the max number of times.
        Expected call at /home/kit/wrk/src/github.com/aws/amazon-vpc-cni-k8s/pkg/networkutils/network_test.go:99 has already been called the max number of times.
        Expected call at /home/kit/wrk/src/github.com/aws/amazon-vpc-cni-k8s/pkg/networkutils/network_test.go:100 has already been called the max number of times.

AddrList() calls Link.Attr() but how to "EXPECT" this in the test?

// ip add del <eniIP> dev <link> (if necessary)
// ip add add <eniIP> dev <link>
log.Debugf("Setting up ENI's primary IP %s", eniIP)
addrs, err := netlink.AddrList(link, unix.AF_INET)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/netlink/netLink/g - (Please replace l with L.)
Then, netLink.AddrList() calls wrapper library and mockNetLink.EXPECT().AddrList in unit test will be called as you expected.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

replace with

 addrs, err := netLink.AddrList(link, unix.AF_INET),

unit-test should pass

@liwenwu-amazon
Copy link
Contributor

LGTM if you fixed the unit-test failure

@ewbankkit
Copy link
Contributor Author

@nak3 / @liwenwu-amazon Thanks for the tip; Unit tests are now passing.

Copy link
Contributor

@mogren mogren left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants