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

[FLOC-2137] Allow the public IP address of the node to be statically configured #2059

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

wallrj
Copy link
Contributor

@wallrj wallrj commented Oct 22, 2015

Design / Implementation for https://clusterhq.atlassian.net/browse/FLOC-2137

  • Allow an optional hostname value in agent.yml
  • Use that hostname when reporting NodeState.
  • Fall back to the existing socket probing technique.
  • Have run-acceptance-tests set the hostname configuration option when configuring agents on acceptance nodes.

Review on Reviewable

@@ -31,12 +31,15 @@ The optional ``port`` variable is the port on the control node to connect to.
This value must agree with the configuration for the control service telling it on what port to listen.
Omit the ``port`` from both configurations and the services will automatically agree.

If node has a public address that is different than its local address (for example, on an EC2 instance),
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't strictly necessary though, right? We after all manage without right now. And it adds an extra burden of configuration on users that they don't necessarily need. Maybe indicate it's useful but not required? Or the caveats of not doing it?

Copy link
Contributor

Choose a reason for hiding this comment

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

One thing we need to keep in mind: bandwidth costs. Data sent within AWS private network is free, data sent over external IPs costs money. I would hope that DNS lookups for AWS hosts within AWS get turned into the private IP, in which case documenting we want explicit hostname (as opposed to "address" which implies IP) is important. If hostnames always resolve to external IPs then it's possible that following this advice may cost our users a bunch of money on AWS, depending how AWS does bandwidth accounting...

@itamarst
Copy link
Contributor

I am happy with the implementation mostly, other than some comments here and there and whatever more detailed code review would come up with. What worries me is change from IP to hostnames (are there consequences we haven't thought of beyond REST API?) and impact on users re bandwidth costs. So please address only those two points and resubmit.

@ci-jenkins-clusterhq
Copy link

Can someone verify this PR?

@sarum90
Copy link
Contributor

sarum90 commented Oct 30, 2015

Just FYI, I just killed a buildbot builder for this branch that had been running for 205 hrs (http://build.clusterhq.com/builders/flocker-centos-7/builds/7521). Hopefully it didn't need more time 😛. Probably worth investigating as part of review.

@wallrj
Copy link
Contributor Author

wallrj commented Nov 3, 2015

Thanks @itamar for the review.

I put this up for review because @exarkun and I thought we might need it for our benchmarking tool on AWS, but I'm working on something else now.

I'll put the issue into the backlog and @jongiddy may decide to pick this up while working on the benchmarking project.

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.

4 participants