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-614] multiple bugfixes and improvements to GCE #360

Closed
wants to merge 8 commits into
base: trunk
from

Conversation

Projects
None yet
3 participants
@Phreedom
Contributor

Phreedom commented Sep 18, 2014

No description provided.

Phreedom added some commits Jun 30, 2014

GCE: healthcheck: add description param to ex_create_healthcheck(); t…
…argetpool: multiple bugfixes

Fix #1:

targetpool = driver.ex_get_targetpool('tpname')
targetpool.add_node(node)
node.destroy() # targetpool still contains the node
targetpool = driver.ex_get_targetpool('tpname')
    # targetpool.nodes contains node uri string (in addition to possible other node objects)
    # as produced by _to_targetpool because the node is in the pool but is destroyed
targetpool.remove_node(node) # raises an exception, removes the node nevertheless.

Expected behavior: remove the node, return true

Fix #2:
targetpool.add(node)
targetpool.add(node)
    # targetpool.nodes contains 2 copies of node
    # actual targetpool resource on the GCE side doesn't contain 2 copies

Expected behavior: no duplicates in targetpool.nodes, the node list matches GCE side

Fix/Improvement #3:
Allow specifying nodes by fully-qualified node uri in add_node and remove_node.

if tp.nodes:
  tp.remove_node(tp.nodes[0]) # fails if the node in the list doesn't exist

exoected behavior: should be able to remove any node from the list

GCE allows adding non-existent nodes to the targetpool and doesn't automatically
remove nodes from the pool if you delete them. libcloud should support doing the same.
GCE: fix ex_targetpool_add_healthcheck and ex_targetpool_remove_healt…
…hcheck.

The requests were malformed and thus did nothing.
GCE: add description parameter to ex_create_forwarding_rule, improve …
…tests; fix documentation and test fixtures for health checks
GCE: ex_create_firewall: allow creation og firewalls with sourceRange…
…s = [], preserve behavior for sourceRanges = None, preserve default value.

GCE documentation states that firewall allows traffic
if it matches either sourceRanges or sourceTags values.
Thus, sourceRanges = [] is a valid parameter value.

Although it would be better to simply set ["0.0.0.0/0"]
as the default, for minimal API breakage, None value is
still supported.
@Kami

This comment has been minimized.

Show comment
Hide comment
@Kami
Member

Kami commented Sep 19, 2014

@wrigri

This comment has been minimized.

Show comment
Hide comment
@wrigri

wrigri Sep 19, 2014

Contributor

This all looks like good stuff to me. (Will need a couple tweaks for the lint stuff, but otherwise they seem like all good fixes.)

Contributor

wrigri commented Sep 19, 2014

This all looks like good stuff to me. (Will need a couple tweaks for the lint stuff, but otherwise they seem like all good fixes.)

@Phreedom

This comment has been minimized.

Show comment
Hide comment
@Phreedom

Phreedom Sep 19, 2014

Contributor

Sorry about the lint.

BTW this summer I had spent tens of hours testing GCE driver for completeness
and correctness. Except for images and multiple node creation code which I
didn't test, these fixes are the only problems I had discovered.

And libcloud now has another happy "client": NixOps deployment tool.

As far as I'm concerned, the GCE driver may not be complete, but it's very
bug-free now :)

Contributor

Phreedom commented Sep 19, 2014

Sorry about the lint.

BTW this summer I had spent tens of hours testing GCE driver for completeness
and correctness. Except for images and multiple node creation code which I
didn't test, these fixes are the only problems I had discovered.

And libcloud now has another happy "client": NixOps deployment tool.

As far as I'm concerned, the GCE driver may not be complete, but it's very
bug-free now :)

@Kami

This comment has been minimized.

Show comment
Hide comment
@Kami

Kami Sep 20, 2014

Member

@Phreedom no problem and glad to hear that :)

Do let us know if you encounter any other issues in the future or if there is anything else we can do to make the experience even better.

Member

Kami commented Sep 20, 2014

@Phreedom no problem and glad to hear that :)

Do let us know if you encounter any other issues in the future or if there is anything else we can do to make the experience even better.

@asfgit asfgit closed this in 51189ef Sep 20, 2014

@Kami

This comment has been minimized.

Show comment
Hide comment
@Kami

Kami Sep 20, 2014

Member

@Phreedom Changes merged into trunk - thanks!

Member

Kami commented Sep 20, 2014

@Phreedom Changes merged into trunk - thanks!

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