Skip to content
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-879] Add support of node without public IP in LB #952

Closed
wants to merge 1 commit into from
Closed

[LIBCLOUD-879] Add support of node without public IP in LB #952

wants to merge 1 commit into from

Conversation

charly37
Copy link
Contributor

@charly37 charly37 commented Nov 23, 2016

Add support of Node without public IP in GCP load balancer

Description

Link to jira ticket 879
https://issues.apache.org/jira/browse/LIBCLOUD-879

I had a condition to only try to grab the publicIP of a node if it had one since publicIP is not mandatory.

Status

This PR is ready to be review. I run the test (tox) before and after the change and no impact.

Checklist (tick everything that applies)

  • [Code linting] I tried to followed the guidline
  • [Documentation] No change require
  • [Tests] Added 1 test case to cover the new behavior
  • [ICLA] Minor change so I skip it

else:
member_id = node

if (hasattr(node, 'public_ips') and (len(node.public_ips) > 0)):
Copy link
Contributor

@tonybaloney tonybaloney Nov 28, 2016

Choose a reason for hiding this comment

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

no need for parens here and it's not very Pythonic to use them either ;-)

@charly37
Copy link
Contributor Author

charly37 commented Nov 28, 2016

Updated following @tonybaloney review.

@charly37
Copy link
Contributor Author

charly37 commented Dec 6, 2016

I was wandering if I need to provide any extra information to move forward on this PR following the changes I made last week ?

else:
member_id = node

Copy link
Contributor

@allardhoeve allardhoeve Dec 13, 2016

Choose a reason for hiding this comment

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

Although a correct solution, I think we should solve this by fixing the Node object, which should always have a public_ips attribute that is empty if there are no public IPs. This has the benefit of us needing to code that only once instead of all over the place.

Also, I would like to see a testcase, so this functionality never gets accidentally removed.

Copy link
Contributor

@allardhoeve allardhoeve Dec 13, 2016

Choose a reason for hiding this comment

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

Testcases are added by adding the Google API output for a node without public_ips to the fixture, then adding a test to the Node object to make sure the node has an empty list, then this function needs no conditionals, just:

try:
    member_ip = node.public_ips[0]
except IndexError:
    member_ip = None

Copy link
Contributor

@allardhoeve allardhoeve Dec 13, 2016

Choose a reason for hiding this comment

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

The reason I ask for a testcase is that it makes it possible to refactor this kind of thing without introducing new bugs.

Copy link
Contributor Author

@charly37 charly37 Dec 13, 2016

Choose a reason for hiding this comment

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

@allardhoeve Thx a lot for the feedback.

1/ node object : In fact the node object is good. The node IP has indeed an empty "publicIP" which lead to the error since we try to access the first element. Maybe I did not fully understand your feedback ?
2/ test case : I can definitively try to add a test case (will be a good experience for me).

Will update the PR with a test case ASAP and wait for your clarification about the point 1/

Thx again guys for the feedback.

Copy link
Contributor

@allardhoeve allardhoeve Dec 13, 2016

Choose a reason for hiding this comment

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

1/ you check if the Node object has a public_ips attribute, why? Does it not always have the attribute? If not, we should make sure it always has the attribute. If so, you can remove the hasattr.

Copy link
Contributor Author

@charly37 charly37 Dec 17, 2016

Choose a reason for hiding this comment

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

Thx @allardhoeve for your clarification.
In fact checking the length of "public_ips" was my first idea but it do not work (break some test case). The "Node" object can in fact be a string and not a proper node object. That is probably why the actual code test for the attribute "name". This is also written in a comment that I saw after my initial test break the regression

        # A balancer can have a node as a member, even if the node doesn't
        # exist.  In this case, 'node' is simply a string to where the resource
        # would be found if it was there.

That is why I keep the same design as the actual code where the attribute "name" is tested before accessing it for my new change. I hope it clarify ?
PS : test case in progress
PSS : Node is a string when the load balancer reference a non longer existing node

Copy link
Contributor

@allardhoeve allardhoeve Dec 18, 2016

Choose a reason for hiding this comment

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

Huh, I think that is not a really good design from a user's perspective. Maybe we can check with the original author what the intent was? I think having incompatible mixed types is a smell, so maybe we should set node to none and have a different field for the original node name?

Copy link
Contributor Author

@charly37 charly37 Dec 19, 2016

Choose a reason for hiding this comment

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

I think it would be indeed possible to design it another way to avoid mixing String and Node objects nevertheless it would change the actual behavior (that could be use by some existing application) and that is why I limited my change to fixing the publicIP issue with the actual design.

What if I open another dedicated JIRA ticket to discuss this point ?

Copy link
Contributor

@allardhoeve allardhoeve left a comment

I'd like a new test for this behaviour and also some refactoring of the logic into the Node object.

@allardhoeve
Copy link
Contributor

allardhoeve commented Dec 13, 2016

Hey Charles, sorry to spring more work on you after 20 days, but I'd like this to keep working next year :)

@charly37
Copy link
Contributor Author

charly37 commented Dec 19, 2016

@allardhoeve I added a test case as suggested.

Copy link
Contributor

@allardhoeve allardhoeve left a comment

👍 looks good, but I think tests should be separate.

@@ -188,6 +188,12 @@ def test_node_to_member(self):
self.assertEqual(member.ip, node.public_ips[0])
self.assertEqual(member.id, node.name)
self.assertEqual(member.port, balancer.port)
# Test node without publicIP
Copy link
Contributor

@allardhoeve allardhoeve Dec 20, 2016

Choose a reason for hiding this comment

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

Can you make this a test of its own?

Copy link
Contributor Author

@charly37 charly37 Dec 20, 2016

Choose a reason for hiding this comment

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

Sure. Will do it today

@allardhoeve
Copy link
Contributor

allardhoeve commented Dec 20, 2016

Thanks Charles! I'll merge this momentarily.

@allardhoeve
Copy link
Contributor

allardhoeve commented Dec 20, 2016

The tests are failing, but that has nothing to do with this PR

@allardhoeve
Copy link
Contributor

allardhoeve commented Dec 20, 2016

@tonybaloney, any idea why the tests might be failing? They fail on apt-get install libvirt, but I cannot find that provisioning code in the repo anywhere.

@charly37
Copy link
Contributor Author

charly37 commented Dec 20, 2016

@allardhoeve Tests are failing because of the Travis setup. Looking at some other PR it is the same issue but it seems someone find a workaround by using libvirt-bin package instead of libvirt (see 20ab4cb )I was thinking to do it in my PR too but i m afraid it generate conflict.

@allardhoeve
Copy link
Contributor

allardhoeve commented Dec 20, 2016

Please try to merge master into this branch or rebase against it, thanks.

@charly37
Copy link
Contributor Author

charly37 commented Dec 21, 2016

@allardhoeve Tests have been split as suggest. Rebase from trunk to fix the TRAVIS jobs.
I think it is all clean to be merge ;)

@allardhoeve
Copy link
Contributor

allardhoeve commented Dec 22, 2016

Yes! Will merge today

@charly37
Copy link
Contributor Author

charly37 commented Dec 22, 2016

Thx again for the review and help.
I also open a ticket to discuss the design of mixing string and node object as discuss previously in the PR review : https://issues.apache.org/jira/browse/LIBCLOUD-883

@allardhoeve
Copy link
Contributor

allardhoeve commented Dec 23, 2016

Merged, thanks. Should be closed in a minute.

@charly37 charly37 closed this Jan 25, 2018
@pquentin
Copy link
Contributor

pquentin commented Jan 25, 2018

Nice catch @charly37!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants