AWS: Cache instances for ELB to avoid #45050 #47410
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
k8s-github-robot
added
approved
k8s-github-robot
added
size/L
justinsb
force-pushed
the
fix_45050
branch
2 times, most recently
from
f37f616 to
2ae83f0
Compare
justinsb
changed the title
|
@k8s-bot pull-kubernetes-kubemark-e2e-gce test this |
| // cacheCriteria holds criteria that must hold to use a cached snapshot | ||
| type cacheCriteria struct { | ||
| MaxAge time.Duration | ||
| HasInstances []awsInstanceID |
There was a problem hiding this comment.
Can we document this? What does HasInstances do?
| defer c.mutex.Unlock() | ||
|
|
||
| // After() is technically broken by time changes until we have monotonic time | ||
| if c.snapshot != nil && c.snapshot.timestamp.After(snapshot.timestamp) { |
There was a problem hiding this comment.
it may be worth writing c.snapshot.Younger(snapshot) so as to encapsulate out internal time keeping?
There was a problem hiding this comment.
olderThan proved clearer, but good suggestion!
| if err != nil { | ||
| return nil, err | ||
| } | ||
| } else { |
There was a problem hiding this comment.
nit: we can drop this else altogether.
There was a problem hiding this comment.
I don't think we can - we only want to log if we are using a pre-existing snapshot
| // After() is technically broken by time changes until we have monotonic time | ||
| if c.snapshot != nil && c.snapshot.timestamp.After(snapshot.timestamp) { | ||
| // If this happens a lot, we could run this function in a mutex and only return one result | ||
| glog.Infof("Not caching concurrent DescribeInstances results") |
There was a problem hiding this comment.
We should add the word aws here..
| if c.snapshot != nil && c.snapshot.timestamp.After(snapshot.timestamp) { | ||
| // If this happens a lot, we could run this function in a mutex and only return one result | ||
| glog.Infof("Not caching concurrent DescribeInstances results") | ||
| } else { |
|
mostly looks good to me. Some minor style changes and nits. |
|
Pushed suggested changes (other than the if blocks) - please take a look @gnufied :-) |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gnufied, justinsb Associated issue: 45050 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
We maintain a cache of all instances, and we invalidate the cache whenever we see a new instance. For ELBs that should be sufficient, because our usage is limited to instance ids and security groups, which should not change. Fix kubernetes#45050
k8s-github-robot
removed
the
lgtm
|
@k8s-bot pull-kubernetes-e2e-gce-etcd3 test this |
|
Readding lgtm after trivial rebase |
justinsb
added
the
lgtm
|
@k8s-bot pull-kubernetes-federation-e2e-gce test this |
|
/retest |
|
@k8s-bot pull-kubernetes-federation-e2e-gce test this |
|
/retest |
|
Automatic merge from submit-queue (batch tested with PRs 47451, 47410, 47598, 47616, 47473) |
We maintain a cache of all instances, and we invalidate the cache
whenever we see a new instance. For ELBs that should be sufficient,
because our usage is limited to instance ids and security groups, which
should not change.
Fix #45050