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

Add a links argument to GearClient.add (fixes #150) #183

Merged
merged 41 commits into from
Jul 2, 2014

Conversation

wallrj
Copy link
Contributor

@wallrj wallrj commented Jun 30, 2014

  • I've added a links argument to GearClient.add which,
  • accepts a list of PortMap instances.
  • The links are currently fixed to point to the host's loopback address,...
  • which geard will actually (confusingly / understandably) route to the host's non-loopback addresses.
  • My functional test for this starts a twisted server on an ephemeral port and then starts a custom Docker image (via gear) which attempts to connect and send bytes to its localhost.
  • Gear.add creates the links necessary to get route the connection from the loopback port in the container to the listening port on one of the non-loopback addresses of the host.

I was initially confused by the fact that when you ask gear to link to 127.0.0.1 it actually routes traffic to one of the host's non-loopback IPs. There has been some discussion on the mailing list about the possibility of actually routing traffic to the host's loopback IP address.

I had a notion that the tests would only work if you first run setenforce 0 on the host machine. But actually I may have misread the output of that command. It's not saying that "SELinux has been disabled", it's saying that "SELinux is disabled altogether, so setenforce has no effect".

$ setenforce 0
setenforce: SELinux is disabled

$ setenforce 1
setenforce: SELinux is disabled

See openshift/geard#110 (comment)

Fixes #150

wallrj and others added 23 commits June 26, 2014 17:30
…y and test that links also work with other local addresses.
…ar-links-150

Conflicts:
	flocker/node/functional/test_gear.py
	flocker/node/gear.py
…lly routes to the host's non-loopback ip. So I just need to listen on all interfaces for this test to pass
@wallrj wallrj changed the title Add a links argument to GearClient.add Add a links argument to GearClient.add (fixes #150) Jul 1, 2014
@@ -0,0 +1,4 @@
FROM busybox
MAINTAINER Hybrid Logic <[email protected]>
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that this should say ClusterHQ. This is because of https://zulip.com/#narrow/stream/engineering/subject/Mentions.20of.20.E2.80.9Chybrid.20logic.22:

Adam Dangoor: Am I right in thinking that the licences should mention Hybrid Logic but everything else is ClusterHQ?

Rob: yes correct


class ProtocolPoppingFactory(Factory):
"""
A factory which only creates a fixed list of protocols.
Copy link
Contributor

Choose a reason for hiding this comment

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

only?

@adamtheturtle
Copy link
Contributor

Thanks, please address the comments and resubmit.

data['NetworkLinks'].append(
{u'FromHost': u'127.0.0.1',
u'FromPort': link.internal_port,
u'ToHost': u'127.0.0.1',
Copy link
Contributor

Choose a reason for hiding this comment

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

Comment here about why the behaviour when using 127.0.0.1 might not be as expected

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I created openshift/geard#223 and openshift/geard#224 to track these issues.


Send BYTES to HOST:PORT using a TCP connection.

Retry until a connection can be established or until the TIMEOUT period is
Copy link
Contributor

Choose a reason for hiding this comment

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

How is this better than just relying on trial's timeout?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It means that the error will be clearer if the test does fail and it means that the gear unit will fail in a timely manner and with a meaningful log entry if the test fails to clean it up for some reason.

@wallrj
Copy link
Contributor Author

wallrj commented Jul 2, 2014

Thanks @adamtheturtle

I think I've addressed and answered all your comments. Now ready for another review.

esac
done

HOST=${1:?"Error: Missing parameter 1:HOST"}
Copy link
Contributor

Choose a reason for hiding this comment

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

❓ In this case I guess I'm also missing parameters 2, 3, 4, right?

@adamtheturtle
Copy link
Contributor

Thank you, merging.

adamtheturtle added a commit that referenced this pull request Jul 2, 2014
Add a links argument to `GearClient.add` (fixes #150)
@adamtheturtle adamtheturtle merged commit a2dbcad into master Jul 2, 2014
@adamtheturtle adamtheturtle deleted the gear-links-150 branch July 2, 2014 12:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GearClient should allow units to be configured with internal links to ports on the host
2 participants