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.

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

Support ELB pagination for environments with 400+ ELBs #25002

Open
wants to merge 1 commit into
base: devel
from

Conversation

Projects
None yet
6 participants
@davidsuehring

davidsuehring commented May 24, 2017

SUMMARY

These changes are to support boto's pagination for environments with 400+ ELBs.

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

ec2_elb_facts.py
ec2_elb_lb.py

ANSIBLE VERSION

ansible 2.2.2.0

ADDITIONAL INFORMATION

The pagination built into ec2_elb_facts.py was not functioning correctly, because the pagination marker is actually being returned on the next_marker attribute, not next_token. To ensure compatibility with any version that may have used next_token, both attributes are being checked for the pagination marker.

@ansibot

This comment has been minimized.

Contributor

ansibot commented May 24, 2017

@bobobox

This comment has been minimized.

Contributor

bobobox commented May 24, 2017

shipit

@bobobox

This comment has been minimized.

Contributor

bobobox commented May 26, 2017

This fixes #25073

@bobobox

This comment has been minimized.

Contributor

bobobox commented May 26, 2017

This also fixes #25075

return elb
token = None
while True:
elbs = self.elb_conn.get_all_load_balancers(marker=token)

This comment has been minimized.

@willthames

willthames May 27, 2017

Contributor

It would make much more sense to pass load_balancer_names to this call rather than handle paging (we expect to find zero or one load balancers with the given name)

http://boto.cloudhackers.com/en/latest/ref/elb.html#boto.ec2.elb.ELBConnection.get_all_load_balancers

@willthames

Let's turn this into a one-line fix rather than unnecessarily complicate the code - in addition, not retrieving and iterating through all load balancers will make the code much more efficient for everyone in addition to actually working for those with 400+ load balancers.

except AttributeError:
token = None
if token is not None:
break

This comment has been minimized.

@bcoca

bcoca May 31, 2017

Member

look in module_utils, there are generic funcions/decorators, no need to reinvent

@bcoca bcoca removed the needs_triage label May 31, 2017

@s-hertel

This comment has been minimized.

Contributor

s-hertel commented Jun 19, 2017

One line fix for this: #25838

Edit: Derp. Too many PRs to keep straight.

@ansibot ansibot added the stale_ci label Jun 19, 2017

@willthames

This comment has been minimized.

Contributor

willthames commented Jun 21, 2017

@s-hertel that's a different one line fix for a different module. I think ec2_elb_lb can be fixed by simply passing the elb name as an argument to get_all_load_balancers. ec2_elb_facts does need the change (although again, it could be improved by passing names if they are passed as a module parameter, but we'll need to cope with when names aren't passed and we do need to return all 400+ load balancers)

@davidsuehring

This comment has been minimized.

davidsuehring commented Jun 23, 2017

@willthames Thanks for the feedback on this. Is the general expectation that users will have the latest boto installed? I saw some references that implied the name filter didn't exist on some earlier bots releases, so I was trying to keep from breaking compatibility with those. If the expectation is that everyone should be using the latest release, I'll gladly get the pull request updated.

@willthames

This comment has been minimized.

Contributor

willthames commented Jun 26, 2017

@davidsuehring I guess it depends how recently boto changed - if it's in the last year we might have go with your approach. We're moving towards dropping boto support entirely, although I wouldn't want to break boto too hard without moving to boto3!

@ansibot

This comment has been minimized.

Contributor

ansibot commented Jul 26, 2017

@davidsuehring this PR contains more than one new module.

Please submit only one new module per pullrequest. For further explanation, please read grouped module documentation

click here for bot help

@ansibot

This comment has been minimized.

Contributor

ansibot commented Nov 19, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment