Fix device number and update table name the device index #1071
Conversation
|
This change is failing in our internal pipeline. Ip rule table number and actual route table table numbers were different. |
mogren
force-pushed
the
fix-device-id
branch
2 times, most recently
from
6665f20
to
85dcb40
Compare
mogren
force-pushed
the
fix-device-id
branch
3 times, most recently
from
0fa2f25
to
0799752
Compare
mogren
force-pushed
the
fix-device-id
branch
2 times, most recently
from
7b40f4d
to
fd8532a
Compare
|
Does eth1 gets route table number: 1 ?because I usually see different route table for eth1 |
| // 0 is reserved for primary ENI, the rest of them has to +1 to avoid collision at 0 | ||
| return eni, int(deviceNum + 1), nil |
There was a problem hiding this comment.
@SaranBalaji90 Before this PR, eth1 gets table 2 because of this code, since it has device number 1. eth2 gets table 3 and so on.
There was a problem hiding this comment.
Are you sure? Because on a c5.18xlarge eth1 got 15 but rest of them got similar number as device number - for eg eth2 got route table number 2 and so on.
There was a problem hiding this comment.
Was the device index of eth1 14 at that time? The device index does not always match the interface name, especially when deleting and attaching a lot of ENIs. I have also seen eth4 get table 3 since it had the device number 2.
There was a problem hiding this comment.
I don't have the node now but I think looking at the code it should be 14. I wonder how is this number populated? Because of pod ENI feature we don't want this number to go beyond 100.
There was a problem hiding this comment.
Because of pod ENI feature we don't want this number to go beyond 100
I know we're assuming that currently, but do we enforce that in the code anywhere? It sounds like this warrants a nice loud/obvious error message - if we ever accidentally trigger it.
Fwiw, I think the relevant code currently always looks for the lowest unoccupied DeviceNumber, so it will continue to reuse the same low numbers (bounded indirectly by max-eni) (good). In addition, I note the relevant bit of code imposes an artifical max of 128, for no particular reason (the code uses a fixed size array, whereas it could have dynamically resized the array, or used a sparse hash). We should probably lower maxENIs to 100, with a comment explaining why 100 is actually a real limit we have.
There was a problem hiding this comment.
Good point Gus. For some reason I missed that we were setting the device index. I agree we should change it to 100 and raise error if there is no available index within 100.
There was a problem hiding this comment.
Good call, set 100 as a hard limit.
| // 0 is reserved for primary ENI, the rest of them has to +1 to avoid collision at 0 | ||
| return eni, int(deviceNum + 1), nil | ||
| // 0 is reserved for primary ENI | ||
| return eniID, deviceNumber, nil |
There was a problem hiding this comment.
This has to be deviceNumber +1 to maintain backward compatibility otherwise how would this work on existing clusters? existing clusters might already have route for deviceNumber.
There was a problem hiding this comment.
You are right that we can't change the table name now and stay backwards compatible on nodes that are already running. We are stuck using the name we currently have.
I have a different suggestion on how to solve it though. Let's use tableNumber := deviceNumber + 1 where we set up the routes, and stop having a variable called deviceNumber that doesn't actually store the device number. I'll update the PR.
There was a problem hiding this comment.
💯 If we're stuck with the +1 thing, we should absolutely stop calling it deviceNumber
mogren
force-pushed
the
fix-device-id
branch
2 times, most recently
from
a3b4b3f
to
c19ef53
Compare
mogren
force-pushed
the
fix-device-id
branch
3 times, most recently
from
e690fa9
to
3c38011
Compare
| log.Errorf("Device number of primary ENI is %d! It was expected to be 0", deviceNumber) | ||
| // TODO: Can this happen? Is the only option to panic? | ||
| } | ||
| return eniID, deviceNumber, nil |
There was a problem hiding this comment.
Should we return 0 instead? (logger will help us to debug if something is not right?)
There was a problem hiding this comment.
Yes, better to always return 0 here, together with an error log.
mogren
force-pushed
the
fix-device-id
branch
from
3c38011
to
6d847db
Compare
|
@SaranBalaji90 Updated a running cluster both from v1.6.3, and from v1.7.1 to this build. Table numbers match. |
| Dst: &net.IPNet{IP: gw, Mask: net.CIDRMask(32, 32)}, | ||
| Scope: netlink.SCOPE_LINK, | ||
| Table: eniTable, | ||
| Table: tableNumber, |
There was a problem hiding this comment.
This didn't fail any test is it? Because now the value that is passed to this method and table number are different, I would expect mock to fail?
| // 0 is reserved for primary ENI, the rest of them has to +1 to avoid collision at 0 | ||
| return eni, int(deviceNum + 1), nil | ||
| // 0 is reserved for primary ENI | ||
| return eniID, deviceNumber, nil |
There was a problem hiding this comment.
This change should have failed some unit test right? For eg instead of returning x now it will be returning x-1
There was a problem hiding this comment.
mogren
force-pushed
the
fix-device-id
branch
from
6d847db
to
4396c3b
Compare
|
Thanks @mogren. Will definitely help in future when reading the code. |
Description of changes:
When we attach new ENIs, we store the
deviceNumber. We used to add +1 to all except the primary (device number 0). This was done because of how we named the route tables. This PR does the following:deviceNumberin theENIstruct (visible through introspect endpoint)tableNumberthat matches the existing number used by previous versionsBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.