Fix device number and update table name the device index by mogren · Pull Request #1071 · aws/amazon-vpc-cni-k8s

mogren · 2020-07-03T19:01:27Z

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:

Store the actual device number value in deviceNumber in the ENI struct (visible through introspect endpoint)
Have a tableNumber that matches the existing number used by previous versions
Set a hard upper limit of 100 for the device number to avoid clashes with vlan table names.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

jayanthvn

Looks good :)

pkg/awsutils/awsutils.go

pkg/networkutils/network.go

SaranBalaji90 · 2020-07-06T17:53:48Z

This change is failing in our internal pipeline. Ip rule table number and actual route table table numbers were different.

SaranBalaji90 · 2020-08-30T18:03:24Z

Does eth1 gets route table number: 1 ?because I usually see different route table for eth1

mogren · 2020-08-30T18:15:10Z

pkg/awsutils/awsutils.go

-	// 0 is reserved for primary ENI, the rest of them has to +1 to avoid collision at 0
-	return eni, int(deviceNum + 1), nil


@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.

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.

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.

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.

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.

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.

Good call, set 100 as a hard limit.

SaranBalaji90 · 2020-08-30T21:27:11Z

pkg/awsutils/awsutils.go

-	// 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


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.

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.

💯 If we're stuck with the +1 thing, we should absolutely stop calling it deviceNumber

pkg/networkutils/network.go

cmd/routed-eni-cni-plugin/driver/driver.go

SaranBalaji90 · 2020-08-31T18:36:46Z

pkg/awsutils/awsutils.go

+			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


Should we return 0 instead? (logger will help us to debug if something is not right?)

Yes, better to always return 0 here, together with an error log.

SaranBalaji90

Can you test this change by upgrading running cluster and make sure everything works as expected?

mogren · 2020-09-01T02:19:35Z

@SaranBalaji90 Updated a running cluster both from v1.6.3, and from v1.7.1 to this build. Table numbers match.

pkg/networkutils/network_test.go

SaranBalaji90 · 2020-09-01T05:04:59Z

pkg/networkutils/network.go

 			Dst:       &net.IPNet{IP: gw, Mask: net.CIDRMask(32, 32)},
 			Scope:     netlink.SCOPE_LINK,
-			Table:     eniTable,
+			Table:     tableNumber,


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?

SaranBalaji90 · 2020-09-01T05:06:31Z

pkg/awsutils/awsutils.go

-	// 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


This change should have failed some unit test right? For eg instead of returning x now it will be returning x-1

@SaranBalaji90 Yes, this test had to be updated:

https://github.com/aws/amazon-vpc-cni-k8s/pull/1071/files/6d847db2bcd471c1b180fd8d7e81712ec6dbd942#diff-52f7ba503946a19ecad23c70607d5112R825

SaranBalaji90 · 2020-09-01T23:25:36Z

Thanks @mogren. Will definitely help in future when reading the code.

jayanthvn approved these changes Jul 3, 2020

View reviewed changes

jaypipes approved these changes Jul 6, 2020

View reviewed changes

pkg/awsutils/awsutils.go Show resolved Hide resolved

pkg/networkutils/network.go Show resolved Hide resolved

mogren force-pushed the fix-device-id branch from 1f765f9 to f7c508a Compare July 8, 2020 16:21

mogren force-pushed the fix-device-id branch 2 times, most recently from 6665f20 to 85dcb40 Compare July 17, 2020 16:52

mogren force-pushed the fix-device-id branch from 85dcb40 to a0a547b Compare August 4, 2020 06:18

mogren mentioned this pull request Aug 4, 2020

Avoid caching dynamic VPC CIDR info #1113

Merged

mogren force-pushed the fix-device-id branch 3 times, most recently from 0fa2f25 to 0799752 Compare August 6, 2020 22:02

mogren requested a review from anguslees August 10, 2020 18:19

mogren force-pushed the fix-device-id branch 2 times, most recently from 7b40f4d to fd8532a Compare August 30, 2020 01:38

prithviramesh closed this Aug 30, 2020

prithviramesh reopened this Aug 30, 2020

mogren commented Aug 30, 2020

View reviewed changes

SaranBalaji90 reviewed Aug 30, 2020

View reviewed changes

mogren force-pushed the fix-device-id branch 2 times, most recently from a3b4b3f to c19ef53 Compare August 30, 2020 23:07

SaranBalaji90 reviewed Aug 31, 2020

View reviewed changes

pkg/networkutils/network.go Show resolved Hide resolved

mogren force-pushed the fix-device-id branch 3 times, most recently from e690fa9 to 3c38011 Compare August 31, 2020 16:29

SaranBalaji90 reviewed Aug 31, 2020

View reviewed changes

cmd/routed-eni-cni-plugin/driver/driver.go Show resolved Hide resolved

SaranBalaji90 reviewed Aug 31, 2020

View reviewed changes

mogren force-pushed the fix-device-id branch from 3c38011 to 6d847db Compare September 1, 2020 01:32

SaranBalaji90 reviewed Sep 1, 2020

View reviewed changes

pkg/networkutils/network_test.go Show resolved Hide resolved

SaranBalaji90 reviewed Sep 1, 2020

View reviewed changes

Fix device number and update table name the device index

4396c3b

mogren force-pushed the fix-device-id branch from 6d847db to 4396c3b Compare September 1, 2020 21:32

SaranBalaji90 approved these changes Sep 1, 2020

View reviewed changes

SaranBalaji90 merged commit 6eb28fa into aws:master Sep 1, 2020

		// 0 is reserved for primary ENI, the rest of them has to +1 to avoid collision at 0
		return eni, int(deviceNum + 1), nil

Conversation

mogren commented Jul 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jayanthvn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

SaranBalaji90 commented Jul 6, 2020

Uh oh!

SaranBalaji90 commented Aug 30, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SaranBalaji90 Aug 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anguslees Aug 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SaranBalaji90 Aug 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

SaranBalaji90 Aug 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SaranBalaji90 left a comment

Choose a reason for hiding this comment

Uh oh!

mogren commented Sep 1, 2020

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SaranBalaji90 commented Sep 1, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

mogren commented Jul 3, 2020 •

edited

Loading

SaranBalaji90 Aug 30, 2020 •

edited

Loading

anguslees Aug 31, 2020 •

edited

Loading

SaranBalaji90 Aug 31, 2020 •

edited

Loading

SaranBalaji90 Aug 31, 2020 •

edited

Loading