Skip to content

AWS: Cache instances for ELB to avoid #45050 #47410

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
k8s-github-robot merged 1 commit into from
Jun 16, 2017
Merged

AWS: Cache instances for ELB to avoid #45050 #47410

k8s-github-robot merged 1 commit into from
Jun 16, 2017
+313 −106

Conversation

@justinsb
Copy link
Member

@justinsb justinsb commented Jun 13, 2017 •
edited
Loading

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

AWS: Maintain a cache of all instances, to fix problem with > 200 nodes with ELBs

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 13, 2017
@k8s-github-robot k8s-github-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Jun 13, 2017
@k8s-github-robot k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jun 14, 2017
@justinsb justinsb force-pushed the fix_45050 branch 2 times, most recently from f37f616 to 2ae83f0 Compare June 14, 2017 14:49
@justinsb justinsb changed the title WIP: Use instance id and caching to avoid #45050 AWS: Cache instances for ELB to avoid #45050 Jun 14, 2017
@justinsb justinsb added this to the v1.7 milestone Jun 14, 2017
@justinsb justinsb mentioned this pull request Jun 14, 2017
Merged
@justinsb
Copy link
Member Author

justinsb commented Jun 14, 2017

@k8s-bot pull-kubernetes-kubemark-e2e-gce test this

@marun marun added the sig/aws label Jun 14, 2017
gnufied
gnufied reviewed Jun 14, 2017
View reviewed changes
pkg/cloudprovider/providers/aws/instances.go Outdated
Copy link
Member

@gnufied gnufied Jun 14, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we document this? What does HasInstances do?

Copy link
Member Author

@justinsb justinsb Jun 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding field docs

pkg/cloudprovider/providers/aws/instances.go Outdated
Copy link
Member

@gnufied gnufied Jun 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it may be worth writing c.snapshot.Younger(snapshot) so as to encapsulate out internal time keeping?

Copy link
Member Author

@justinsb justinsb Jun 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

olderThan proved clearer, but good suggestion!

pkg/cloudprovider/providers/aws/instances.go Outdated
Copy link
Member

@gnufied gnufied Jun 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: we can drop this else altogether.

Copy link
Member Author

@justinsb justinsb Jun 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we can - we only want to log if we are using a pre-existing snapshot

pkg/cloudprovider/providers/aws/instances.go Outdated
Copy link
Member

@gnufied gnufied Jun 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should add the word aws here..

Copy link
Member Author

@justinsb justinsb Jun 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

pkg/cloudprovider/providers/aws/instances.go Outdated
Copy link
Member

@gnufied gnufied Jun 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: we can drop the else.

Copy link
Member Author

@justinsb justinsb Jun 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure we can?

@gnufied
Copy link
Member

gnufied commented Jun 14, 2017

mostly looks good to me. Some minor style changes and nits.

@justinsb
Copy link
Member Author

justinsb commented Jun 14, 2017

Pushed suggested changes (other than the if blocks) - please take a look @gnufied :-)

@gnufied
Copy link
Member

gnufied commented Jun 14, 2017

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 14, 2017
@k8s-github-robot
Copy link

k8s-github-robot commented Jun 14, 2017

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

Details Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@justinsb
AWS: Maintain a cache of all instances for ELB
3d2b71b 
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 k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 15, 2017
@justinsb
Copy link
Member Author

justinsb commented Jun 15, 2017

@k8s-bot pull-kubernetes-e2e-gce-etcd3 test this

@justinsb
Copy link
Member Author

justinsb commented Jun 15, 2017

Readding lgtm after trivial rebase

@justinsb justinsb added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 15, 2017
@justinsb
Copy link
Member Author

justinsb commented Jun 15, 2017

@k8s-bot pull-kubernetes-federation-e2e-gce test this

@justinsb
Copy link
Member Author

justinsb commented Jun 15, 2017

/retest

@justinsb
Copy link
Member Author

justinsb commented Jun 15, 2017

@k8s-bot pull-kubernetes-federation-e2e-gce test this

@justinsb
Copy link
Member Author

justinsb commented Jun 16, 2017

/retest

@k8s-github-robot
Copy link

k8s-github-robot commented Jun 16, 2017

Automatic merge from submit-queue (batch tested with PRs 47451, 47410, 47598, 47616, 47473)

@k8s-github-robot k8s-github-robot merged commit 06e8e0c into kubernetes:master Jun 16, 2017
@rahanar rahanar mentioned this pull request Aug 17, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@gnufied gnufied gnufied left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@gnufied gnufied

@zmerlynn zmerlynn

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Milestone

v1.7

Development

Successfully merging this pull request may close these issues.

running more than 200 instances on AWS breaks ELB LoadBalancers

6 participants

@justinsb @gnufied @k8s-github-robot @marun @zmerlynn @k8s-ci-robot