Repository navigation
Cache IMDS data that doesn't change within one boot cycle. - #4847
Conversation
| if err != nil { | ||
| return "", err | ||
| } | ||
| return c.getCachedMetadata("user-data", func() (string, error) { |
There was a problem hiding this comment.
"user-data" can be a constant.
There was a problem hiding this comment.
Taking a closer look. I think it's OK and maybe clearer to just use literal "user-data" here. Not because the value "user-data" is only used once. But for how IMDS categorize metadata.
There are three categories of instance metadata.
- Instance metadata properties. Accessed through GetMetadata(path).
- Dynamic data. Accessed through GetDynamicData(path).
- User data. Access through GetUserData().
https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/ec2-instance-metadata.html
We define consts for paths used in 1. and 2. These constants are one-to-one bind to IMDS metadata path, e.g. MacResource -> "/mac", InstanceIDResource -> /instance-id. However, "user-data" is not a IMDS path. It is just User Data.
| // getCachedMetadata is a helper that implements the cache-aside pattern for metadata values. | ||
| // It checks the cache first, and if not found, calls the fetch function and stores the result. | ||
| func (c *ec2MetadataClientImpl) getCachedMetadata(key string, fetch func() (string, error)) (string, error) { | ||
| c.cacheMu.RLock() |
There was a problem hiding this comment.
nit: perhaps we can consider having just one lock operation. WDYT?
for e.g.
c.cacheMu.Lock()
defer c.cacheMu.Unlock()
...
return v, nil
There was a problem hiding this comment.
The cacheMu here is a read-write lock, sync.RWMutex. For read operation, RLock(). For write operation, use Lock(). The RLock doesn't block other RLock.
JoseVillalta
left a comment
JoseVillalta
left a comment
There was a problem hiding this comment.
changes LGTM, I would like to discuss offline the long-term test strategy for this change. Should there be end-to-end tests?
Summary
Some instance metadata does not change once the instance is provisioned, and some metadata remains constant within a single boot cycle. The agent can cache these data instead of retrieving it from IMDS repeatedly. This helps mitigating the following issue caused by linklocal service throttle: 1,024 Packet Per Second (PPS) per ENI.
Implementation details
There are two caches added.
cache map[string]stringinstanceIdentityDocCache *imds.InstanceIdentityDocument.The
cachecaches following responses of Metadata.Testing
Created a Go cli that starts 1500 goroutines where each goroutine calls the
EC2MetadataClient.PrimaryENIMAC()for 60 seconds.Without cache.
With cache
New tests cover the changes: yes
Description for the changelog
Enhancement - Cached IMDS responses to mitigate linklocal throttle.
Additional Information
Does this PR include breaking model changes? If so, Have you added transformation functions?
no
Does this PR include the addition of new environment variables in the README?
no
Licensing
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.