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.

Sign up for GitHub

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

Jump to bottom

In containers view, show short lived connections to/from the internet. #493

Merged
merged 3 commits into from
Oct 5, 2015
Merged

In containers view, show short lived connections to/from the internet. #493

merged 3 commits into from
Oct 5, 2015

Conversation

tomwilkie
Copy link
Contributor

Problem: short-lived, incoming connections from the internet to a container don't show up in the UI. This is because the local end of the connection is to the hosts IP (not a container), and we drop nodes without a container on them.

Outgoing short lived connections to the internet do work.

We should take into account the container port mappings when translating this node to a container ID. This can be done unambiguously.

@tomwilkie tomwilkie force-pushed the short-lived-internet-node-connections branch from 86d2a98 to 1828b8c Compare September 16, 2015 10:52
@tomwilkie tomwilkie changed the title In containers view, show short lived connections to/from the internet. [WIP] In containers view, show short lived connections to/from the internet. Sep 16, 2015
@tomwilkie tomwilkie self-assigned this Sep 16, 2015
@tomwilkie tomwilkie mentioned this pull request Sep 17, 2015
Closed
@tomwilkie tomwilkie force-pushed the short-lived-internet-node-connections branch from 1828b8c to 04690b5 Compare September 22, 2015 03:39
@tomwilkie tomwilkie changed the title [WIP] In containers view, show short lived connections to/from the internet. In containers view, show short lived connections to/from the internet. Sep 22, 2015
@tomwilkie tomwilkie force-pushed the short-lived-internet-node-connections branch from 04690b5 to e62dd51 Compare September 22, 2015 04:16
@tomwilkie
Copy link
Contributor Author

@fons would you mind giving this a test? I can't test easily from singapore.

@peterbourgon its a bit untidy, so I'd welcome suggestions on how to clean this up.

@tomwilkie tomwilkie assigned 2opremio and unassigned tomwilkie Sep 22, 2015
@tomwilkie
Copy link
Contributor Author

This should finally fix #446

@tomwilkie tomwilkie force-pushed the short-lived-internet-node-connections branch from e62dd51 to 5f8f4c4 Compare September 23, 2015 08:11
@2opremio
Copy link
Contributor

@tomwilkie Ops, I missed this (I wish I could have the fons user though :) )

Unfortunately it still doesn't work. The Internet node is now gone from the non-system containers view:
screen shot 2015-09-23 at 10 38 24

Report: https://gist.github.com/2opremio/2d03e27de9a3bf39bfc2

Here's the view with system containers:
screen shot 2015-09-23 at 10 40 11

@tomwilkie
Copy link
Contributor Author

Okay I'll have to take a look next week. Copying binaries to ec2 takes too long from here.

@tomwilkie tomwilkie assigned tomwilkie and unassigned 2opremio Sep 23, 2015
@peterbourgon peterbourgon changed the title In containers view, show short lived connections to/from the internet. [WIP] In containers view, show short lived connections to/from the internet. Sep 25, 2015
@tomwilkie tomwilkie force-pushed the short-lived-internet-node-connections branch from 5f8f4c4 to 184d7eb Compare September 29, 2015 08:55
@tomwilkie tomwilkie changed the title [WIP] In containers view, show short lived connections to/from the internet. In containers view, show short lived connections to/from the internet. Sep 29, 2015
@tomwilkie
Copy link
Contributor Author

@fons I've got this working for my test case, could you confirm it works for yours?

@tomwilkie tomwilkie assigned 2opremio and unassigned tomwilkie Sep 29, 2015
@tomwilkie tomwilkie force-pushed the short-lived-internet-node-connections branch from 184d7eb to 5b239a0 Compare September 29, 2015 09:11
@2opremio
Copy link
Contributor

@tomwilkie Same result :S

@2opremio 2opremio assigned tomwilkie and unassigned 2opremio Sep 29, 2015
@tomwilkie tomwilkie force-pushed the short-lived-internet-node-connections branch 2 times, most recently from 5764df8 to d3aa975 Compare October 1, 2015 16:48
@tomwilkie
Copy link
Contributor Author

@2opremio I'm confident I've got it this time. Just writing a integration test. Fancy giving it a try?

@2opremio
Copy link
Contributor

2opremio commented Oct 1, 2015

@tomwilkie Yes, you did!

screen shot 2015-10-01 at 19 39 37

screen shot 2015-10-01 at 19 39 57

@2opremio
Copy link
Contributor

2opremio commented Oct 1, 2015

Test failures aside, LGTM

@tomwilkie tomwilkie force-pushed the short-lived-internet-node-connections branch 5 times, most recently from b613f33 to ab94da5 Compare October 2, 2015 10:08
@tomwilkie tomwilkie force-pushed the short-lived-internet-node-connections branch from ab94da5 to ab83f41 Compare October 2, 2015 10:19
@tomwilkie
Copy link
Contributor Author

@2opremio does your lgtm extend to the code, or just saying it works?

I've added a unit test as well, to ensure this case is covered. Please take a look.

2opremio
2opremio reviewed Oct 2, 2015
View reviewed changes
integration/300_internet_edge_test.sh
has_container $HOST1 nginx 1
has_connection $HOST1 "The Internet" nginx

kill %do_connections

This comment was marked as abuse.

Sign in to view

tomwilkie added a commit that referenced this pull request Oct 5, 2015
@tomwilkie
Merge pull request #493 from weaveworks/short-lived-internet-node-con…
356acd7 
…nections

In containers view, show short lived connections to/from the internet.
@tomwilkie tomwilkie merged commit 356acd7 into master Oct 5, 2015
@tomwilkie tomwilkie deleted the short-lived-internet-node-connections branch October 5, 2015 09:37
@paulbellamy paulbellamy mentioned this pull request Oct 28, 2015
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@tomwilkie tomwilkie

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@tomwilkie @2opremio