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

Libcloud 737 create dimension data load balancing driver #567

Closed
wants to merge 16 commits into
base: trunk
from

Conversation

Projects
None yet
2 participants
@tonybaloney
Contributor

tonybaloney commented Aug 28, 2015

Created a dimension data driver for load balancing

placed proprietary logic into ex_methods
supported existing interfaces for LoadBalancer
moved common classes and the connection class used in compute into a common library
added unit tests and fixtures for the load balancer driver
updated the docs
tested against expected results and API docs

kwargs['region'] = self.selected_region
return kwargs
def create_balancer(self, name, port, protocol, algorithm, members):

This comment has been minimized.

@Kami

Kami Aug 29, 2015

Member

Is members a required argument or you can create a load-balancer with no initial members and attach them later?

@Kami

Kami Aug 29, 2015

Member

Is members a required argument or you can create a load-balancer with no initial members and attach them later?

This comment has been minimized.

@tonybaloney

tonybaloney Aug 29, 2015

Contributor

I didn't test that case of having members as None but technically you can create a virtual listener pool with no members. I'll add a check before it adds members to check members is not None and add a unit test for that scenario. 

On Sat, Aug 29, 2015 at 9:19 AM -0700, "Tomaz Muraus" notifications@github.com wrote:

In libcloud/loadbalancer/drivers/dimensiondata.py:

  •                                                port=port,
    
  •                                                api_version=api_version,
    
  •                                                region=region,
    
  •                                                **kwargs)
    
  • def _ex_connection_class_kwargs(self):
  •    """
    
  •        Add the region to the kwargs before the connection is instantiated
    
  •    """
    
  •    kwargs = super(DimensionDataLBDriver,
    
  •                   self)._ex_connection_class_kwargs()
    
  •    kwargs['region'] = self.selected_region
    
  •    return kwargs
    
  • def create_balancer(self, name, port, protocol, algorithm, members):

Is members a required argument or you can create a load-balancer with no initial members and attach them later?


Reply to this email directly or view it on GitHub.

@tonybaloney

tonybaloney Aug 29, 2015

Contributor

I didn't test that case of having members as None but technically you can create a virtual listener pool with no members. I'll add a check before it adds members to check members is not None and add a unit test for that scenario. 

On Sat, Aug 29, 2015 at 9:19 AM -0700, "Tomaz Muraus" notifications@github.com wrote:

In libcloud/loadbalancer/drivers/dimensiondata.py:

  •                                                port=port,
    
  •                                                api_version=api_version,
    
  •                                                region=region,
    
  •                                                **kwargs)
    
  • def _ex_connection_class_kwargs(self):
  •    """
    
  •        Add the region to the kwargs before the connection is instantiated
    
  •    """
    
  •    kwargs = super(DimensionDataLBDriver,
    
  •                   self)._ex_connection_class_kwargs()
    
  •    kwargs['region'] = self.selected_region
    
  •    return kwargs
    
  • def create_balancer(self, name, port, protocol, algorithm, members):

Is members a required argument or you can create a load-balancer with no initial members and attach them later?


Reply to this email directly or view it on GitHub.

This comment has been minimized.

@Kami

Kami Aug 29, 2015

Member

Alright, thanks.

Besides that, it looks good to me.

@Kami

Kami Aug 29, 2015

Member

Alright, thanks.

Besides that, it looks good to me.

[LIBCLOUD-737] Support (and test) for creating a balancer that has an…
… empty list of members, or a NoneType for the list of members. Also creating tests for the destroy instance methods and added docstrings.
@tonybaloney

This comment has been minimized.

Show comment
Hide comment
@tonybaloney

tonybaloney Aug 30, 2015

Contributor

Checked in updates and tests to support the use case you mentioned. Also added a few more tests and some docstrings for when the docs get generated

Contributor

tonybaloney commented Aug 30, 2015

Checked in updates and tests to support the use case you mentioned. Also added a few more tests and some docstrings for when the docs get generated

@Kami

This comment has been minimized.

Show comment
Hide comment
@Kami

Kami Aug 30, 2015

Member

Thanks.

I thought that the base driver and standard API defaults members to None and makes it optional, but looks like that is not the case.

Imo, it makes sense to allow users to create load balancer without any initial members (e.g. you want to pre-allocate a balancer so you don't need to wait for the creation later on when you need it), so I will also go ahead and make the change in the base API as well.

Member

Kami commented Aug 30, 2015

Thanks.

I thought that the base driver and standard API defaults members to None and makes it optional, but looks like that is not the case.

Imo, it makes sense to allow users to create load balancer without any initial members (e.g. you want to pre-allocate a balancer so you don't need to wait for the creation later on when you need it), so I will also go ahead and make the change in the base API as well.

@Kami

This comment has been minimized.

Show comment
Hide comment
@Kami

Kami Aug 30, 2015

Member

I'm having issues (a bunch of merge conflicts) while trying to merge those changes into trunk.

Can you please synchronize your branch with the latest upstream trunk?

Member

Kami commented Aug 30, 2015

I'm having issues (a bunch of merge conflicts) while trying to merge those changes into trunk.

Can you please synchronize your branch with the latest upstream trunk?

tonybaloney added some commits Aug 30, 2015

@tonybaloney

This comment has been minimized.

Show comment
Hide comment
@tonybaloney

tonybaloney Aug 30, 2015

Contributor

I must be missing something here, some scaffolding logic to add the new unit tests class to the general test suite. I was always running the tests directly.

Contributor

tonybaloney commented Aug 30, 2015

I must be missing something here, some scaffolding logic to add the new unit tests class to the general test suite. I was always running the tests directly.

@Kami

This comment has been minimized.

Show comment
Hide comment
@Kami

Kami Aug 30, 2015

Member

You mean you are not sure why tests fail on Travis CI or something else?

I can look while merging it. Could be an issue with an import or similar.

Member

Kami commented Aug 30, 2015

You mean you are not sure why tests fail on Travis CI or something else?

I can look while merging it. Could be an issue with an import or similar.

@tonybaloney

This comment has been minimized.

Show comment
Hide comment
@tonybaloney

tonybaloney Aug 30, 2015

Contributor

I've done the sync upstream, merged into my fork, then the branch, so auto
merge should be good now, but travis is failing:

loadTestsFromName

parent, obj = obj, getattr(obj, part)

AttributeError: 'module' object has no attribute 'test_dimensiondata'

But, it exists..

https://github.com/DimensionDataCBUSydney/libcloud/blob/LIBCLOUD-737_Create_Dimension_Data_Load_Balancing_driver/libcloud/test/loadbalancer/test_dimensiondata.py

On Sun, Aug 30, 2015 at 10:08 PM, Tomaz Muraus notifications@github.com
wrote:

You mean you are not sure why tests fail on Travis CI or something else?

I can look while merging it. Could be an issue with an import or similar.


Reply to this email directly or view it on GitHub
#567 (comment).

Contributor

tonybaloney commented Aug 30, 2015

I've done the sync upstream, merged into my fork, then the branch, so auto
merge should be good now, but travis is failing:

loadTestsFromName

parent, obj = obj, getattr(obj, part)

AttributeError: 'module' object has no attribute 'test_dimensiondata'

But, it exists..

https://github.com/DimensionDataCBUSydney/libcloud/blob/LIBCLOUD-737_Create_Dimension_Data_Load_Balancing_driver/libcloud/test/loadbalancer/test_dimensiondata.py

On Sun, Aug 30, 2015 at 10:08 PM, Tomaz Muraus notifications@github.com
wrote:

You mean you are not sure why tests fail on Travis CI or something else?

I can look while merging it. Could be an issue with an import or similar.


Reply to this email directly or view it on GitHub
#567 (comment).

@Kami

This comment has been minimized.

Show comment
Hide comment
@Kami

Kami Aug 30, 2015

Member

@tonybaloney compute driver was the one causing failure. I've fixed the issue locally and will merge it shortly:

python libcloud/test/compute/test_dimensiondata.py
Traceback (most recent call last):
  File "libcloud/test/compute/test_dimensiondata.py", line 21, in <module>
    from libcloud.compute.drivers.dimensiondata import DimensionDataAPIException
ImportError: cannot import name DimensionDataAPIException

I will also look into the test loading code and see if it's possible to make it report more user-friendly errors.

Member

Kami commented Aug 30, 2015

@tonybaloney compute driver was the one causing failure. I've fixed the issue locally and will merge it shortly:

python libcloud/test/compute/test_dimensiondata.py
Traceback (most recent call last):
  File "libcloud/test/compute/test_dimensiondata.py", line 21, in <module>
    from libcloud.compute.drivers.dimensiondata import DimensionDataAPIException
ImportError: cannot import name DimensionDataAPIException

I will also look into the test loading code and see if it's possible to make it report more user-friendly errors.

@tonybaloney

This comment has been minimized.

Show comment
Hide comment
@tonybaloney

tonybaloney Aug 30, 2015

Contributor

We just reached the same conclusion. Threw me off having 2 test classes of the same name! Pushed and passing. It should be clear to merge now.

Contributor

tonybaloney commented Aug 30, 2015

We just reached the same conclusion. Threw me off having 2 test classes of the same name! Pushed and passing. It should be clear to merge now.

@asfgit asfgit closed this in c382e45 Aug 30, 2015

@Kami

This comment has been minimized.

Show comment
Hide comment
@Kami

Kami Aug 30, 2015

Member

Merged, thanks!

Member

Kami commented Aug 30, 2015

Merged, thanks!

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