Avoid caching dynamic VPC CIDR info #1113

anguslees · 2020-08-03T05:57:53Z

We expect the list of VPC CIDRs to change. Rather than cache the VPC CIDRs and then asynchronously update that cache, just don't cache :) and fetch the latest VPC CIDRs on demand.

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

anguslees · 2020-08-03T06:00:20Z

Note to reviewer:

This PR anticipates the merge of two cleanup PRs Remove old pre-1.3 migration code #1110 and Remove references to unused metadata owner-id #1111 (and includes them in the commits in this PR). I suggest they be reviewed/merged first, and (if necessary) rebase this PR.
Also, the two logical steps (refactor, then change) are broken into two separate commits. It might be easier to review them separately.

mogren

#1110 and #1111 have been merged, a rebase will make this change easier to see.

I agree that caching IMDS is probably not worth it. We don't check metadata all that often anyway.

mogren · 2020-08-04T03:12:26Z

pkg/ec2metadata/client.go

@@ -22,6 +24,7 @@ import (
 // EC2Metadata wraps the methods from the amazon-sdk-go's ec2metadata package
 type EC2Metadata interface {
 	GetMetadata(path string) (string, error)
+	GetMetadataWithContext(ctx context.Context, path string) (string, error)


Ah... We already set the default config to have WithMaxRetries(10)...

anguslees · 2020-08-04T04:25:52Z

pkg/awsutils/awsutils.go

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


Note: I removed the +1 thing here, since we're already assuming that 0 == primary (and the previous code did too).

I think (with high confidence) that that's ok. I think (with much less confidence) that's it's also ok across upgrade. Upgrade testing/failures should look here for the cause - especially upgrade tests involving multiple interfaces...

I started to do something similar in #1071, but was not 100% sure that the primary ENI is always at index 0: https://github.com/aws/amazon-vpc-cni-k8s/pull/1071/files#diff-b55f09f925e40eba84053fdae2096d9cR648

mogren · 2020-08-04T06:26:51Z

pkg/awsutils/imds.go

+	return strconv.Atoi(data)
+}
+
+// GetDeviceNumber returns the unique device number associated with an interface.  The primary interface is 0.


Is the primary interface always 0? I have kind of assumed it is, but is that documented somewhere?

Not explicitly afaics :( Primary ENI is assumed to be always "eth0" in all our scripts/docs/tutorials and the metadata docs for device-number say "eg eth3 is device-number 3" (*). Primary ENI is also the first/only interface attached at boot, so assuming indices go up then the first is also always 0.

(*) .. which is obviously nonsense in the general case, because you can't know how the OS is going to do device enumeration - but I believe this is the fantasy that the original Xen API was based around.

I can remove this comment if you want. The rest of our code already assumes 0==primary (eg here and here) and this hasn't caused us any problems - yet.

Mostly I wanted to be more consistent across our codebase instead of using a mix of .Primary and .DeviceIndex==0, and here seemed a reasonable place to declare this assumption. I can certainly soften it to say "vpc-cni assumes that primary interface is always 0"

This is slightly different:

// TODO: Verify that the first IP is always the primary primary := isFirst

This is checking with IP should not be used by pods. I've checked in the aws/amazon-ec2-net-util repo where they also use index 0 to be the "attchment IP". We should definitely see if we can get rid of this limitation though, and just reserve the "node IP".

The aws.Int64Value(ec2res.Attachment.DeviceIndex) == 0 check to add that log line was to help customers who don't have delete_on_termination set for the primary ENI. #1023

(Oh yes, you're right, one of my examples was primary IP not primary ENI. Anything that calls this getENIDeviceNumber() function (ie: anything that calls GetAttachedENIs()) is another example of code that always sees primaryENI as DeviceNumber==0.)

I'm happy to remove this, and instead make the code track primary ENI separately, and allow any DeviceNumber (including 0) for any ENI. What I don't want to do is have a function called EC2InstanceMetadataCache.getENIDeviceNumber() that is hard-coded to return 0 for the primaryENI and a value that is subtly not the real IMDS device-number for everything else :P

Shall I put in a PR ahead of this one that tracks "primary-ness" explicitly and calculates our legacy "table number" without calling it device-number?

pkg/awsutils/awsutils.go

mogren · 2020-08-04T06:31:33Z

pkg/awsutils/awsutils.go

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


I started to do something similar in #1071, but was not 100% sure that the primary ENI is always at index 0: https://github.com/aws/amazon-vpc-cni-k8s/pull/1071/files#diff-b55f09f925e40eba84053fdae2096d9cR648

anguslees · 2020-09-22T07:24:04Z

Rebased. With the additional changes, I think this is too big now. I'm going to split out the first commit into a new PR..

Edit: -> #1225 Will rebase this PR on that PR if/when it's merged.

Rather than cache and then update the cache - just don't cache :)

anguslees · 2020-09-23T02:50:16Z

Drastically reduced the scope of this PR, and reworded the PR description accordingly. You can ignore most of the previous discussion.

mogren

One less go-routine... Nice!

Triggered another e2e test: https://github.com/aws/amazon-vpc-cni-k8s/runs/1152850334?check_suite_focus=true

mogren · 2020-09-23T03:07:04Z

pkg/awsutils/awsutils.go

+func (cache *EC2InstanceMetadataCache) GetVPCIPv4CIDRs() ([]string, error) {
+	ctx := context.TODO()
+
+	ipnets, err := cache.imds.GetVPCIPv4CIDRBlocks(ctx, cache.primaryENImac)
+	if err != nil {
+		return nil, err
+	}
+
+	// TODO: keep as net.IPNet and remove this round-trip to/from string
+	asStrs := make([]string, len(ipnets))
+	for i, ipnet := range ipnets {
+		asStrs[i] = ipnet.String()
+	}
+
+	return asStrs, nil


This is the main change. Just always fetch the CIDR blocks from the metadata, instead of having a loop to update a cache.

jayanthvn

Nice, as @mogren mentioned one less thread :)

anguslees added the tech debt label Aug 3, 2020

anguslees requested a review from mogren August 3, 2020 05:57

anguslees force-pushed the no-cache-md branch 2 times, most recently from a9e2b5c to 8cec203 Compare August 3, 2020 23:40

mogren reviewed Aug 4, 2020

View reviewed changes

anguslees force-pushed the no-cache-md branch from 8cec203 to 9ffe9e6 Compare August 4, 2020 04:13

anguslees commented Aug 4, 2020

View reviewed changes

mogren reviewed Aug 4, 2020

View reviewed changes

anguslees force-pushed the no-cache-md branch from 9ffe9e6 to 40db7fb Compare August 17, 2020 08:41

anguslees force-pushed the no-cache-md branch from 40db7fb to 0870553 Compare September 22, 2020 07:23

anguslees marked this pull request as draft September 22, 2020 07:24

Don't cache dynamic VPC IPv4 CIDR info

d4ee76c

Rather than cache and then update the cache - just don't cache :)

anguslees force-pushed the no-cache-md branch from 0870553 to d4ee76c Compare September 23, 2020 02:46

anguslees changed the title ~~Avoid caching dynamic metadata~~ Avoid caching dynamic VPC CIDR info Sep 23, 2020

anguslees requested review from jayanthvn and SaranBalaji90 September 23, 2020 02:49

anguslees marked this pull request as ready for review September 23, 2020 02:50

anguslees requested a review from mogren September 23, 2020 02:50

mogren approved these changes Sep 23, 2020

View reviewed changes

jayanthvn approved these changes Sep 23, 2020

View reviewed changes

mogren merged commit e2e1900 into aws:master Sep 23, 2020

anguslees deleted the no-cache-md branch September 25, 2020 00:59

eks-bot mentioned this pull request Jun 11, 2021

🥳 aws-vpc-cni v1.8.0 Automated Release! 🥑 aws/eks-charts#538

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid caching dynamic VPC CIDR info #1113

Avoid caching dynamic VPC CIDR info #1113

anguslees commented Aug 3, 2020 •

edited

anguslees commented Aug 3, 2020 •

edited

mogren left a comment

mogren Aug 4, 2020

anguslees Aug 4, 2020 •

edited

mogren Aug 4, 2020

mogren Aug 4, 2020

anguslees Aug 5, 2020

mogren Aug 6, 2020

anguslees Aug 17, 2020 •

edited

mogren Aug 4, 2020

anguslees commented Sep 22, 2020 •

edited

anguslees commented Sep 23, 2020 •

edited

mogren left a comment

mogren Sep 23, 2020

jayanthvn left a comment

Avoid caching dynamic VPC CIDR info #1113

Avoid caching dynamic VPC CIDR info #1113

Conversation

anguslees commented Aug 3, 2020 • edited

anguslees commented Aug 3, 2020 • edited

mogren left a comment

Choose a reason for hiding this comment

mogren Aug 4, 2020

Choose a reason for hiding this comment

anguslees Aug 4, 2020 • edited

Choose a reason for hiding this comment

mogren Aug 4, 2020

Choose a reason for hiding this comment

mogren Aug 4, 2020

Choose a reason for hiding this comment

anguslees Aug 5, 2020

Choose a reason for hiding this comment

mogren Aug 6, 2020

Choose a reason for hiding this comment

anguslees Aug 17, 2020 • edited

Choose a reason for hiding this comment

mogren Aug 4, 2020

Choose a reason for hiding this comment

anguslees commented Sep 22, 2020 • edited

anguslees commented Sep 23, 2020 • edited

mogren left a comment

Choose a reason for hiding this comment

mogren Sep 23, 2020

Choose a reason for hiding this comment

jayanthvn left a comment

Choose a reason for hiding this comment

anguslees commented Aug 3, 2020 •

edited

anguslees commented Aug 3, 2020 •

edited

anguslees Aug 4, 2020 •

edited

anguslees Aug 17, 2020 •

edited

anguslees commented Sep 22, 2020 •

edited

anguslees commented Sep 23, 2020 •

edited