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

Fix regression in missing edges #465

Merged
merged 6 commits into from
Sep 15, 2015
Merged

Fix regression in missing edges #465

merged 6 commits into from
Sep 15, 2015

Conversation

tomwilkie
Copy link
Contributor

Fix for #446

  • When applying NAT mappings, merge with existing nodes
  • Catch NAT mappings which already exist when scope is started

@2opremio
Copy link
Contributor

2opremio commented Sep 9, 2015

@tomwilkie I know this is not done yet, but, continuing our conversation from yesterday, I am missing an edge from The Internet node to the httpserver node when connecting to it.

screen shot 2015-09-09 at 11 02 34

It could also be that I don't understand how the The Internet node is supposed to work.

@tomwilkie
Copy link
Contributor Author

Thanks alvaro; please send the output of /api/report for this kind of bug.

On Wed, Sep 9, 2015 at 11:07 AM, Alfonso Acosta [email protected]
wrote:

@tomwilkie https://github.com/tomwilkie I know this is not done yet,
but, continuing our conversation from yesterday, I am missing an edge from The
Internet node to the httpserver node when connecting to it.

[image: screen shot 2015-09-09 at 11 02 34]
https://cloud.githubusercontent.com/assets/2362916/9759132/e6e879be-56e2-11e5-9e88-55ab0f041049.png

It could also be that I don't understand how the The Internet node is
supposed to work.

—
Reply to this email directly or view it on GitHub
#465 (comment).

@2opremio
Copy link
Contributor

2opremio commented Sep 9, 2015

There you go. https://gist.github.com/2opremio/322736b35abceadf75a6

Probably unrelated, but I had rebased this PR on master when I saw this.

@peterbourgon peterbourgon changed the title WIP Fix regression in missing edges [WIP] Fix regression in missing edges Sep 10, 2015
@tomwilkie tomwilkie force-pushed the 446-missing-edges branch 4 times, most recently from 26486a5 to c2c0262 Compare September 15, 2015 04:43
@tomwilkie tomwilkie changed the title [WIP] Fix regression in missing edges Fix regression in missing edges Sep 15, 2015
@tomwilkie
Copy link
Contributor Author

@2opremio please try again, I believe this to be almost* fixed.

*almost in that I'm tracking down a small bug where the links 'flutter' - come and go.

@tomwilkie tomwilkie force-pushed the 446-missing-edges branch 3 times, most recently from a2b3391 to 6363612 Compare September 15, 2015 07:35
@tomwilkie
Copy link
Contributor Author

Flutter bug is fixed by the Nodes.Merge cset. This has introduce a bug in pseudo node rendering. The whackamole continues.

@tomwilkie
Copy link
Contributor Author

Pseudo node rendering & Hosts view rendering both fixed now too.

peterbourgon
peterbourgon reviewed Sep 15, 2015
View reviewed changes
app/router.go
func badRequest(r *http.Request, w http.ResponseWriter, err error) {
http.Error(w, err.Error(), http.StatusBadRequest)
log.Printf("Error procressing request for %s: %v", r.URL.Path, err)
}

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

@peterbourgon
Copy link
Contributor

Back to you @tomwilkie

@tomwilkie
Copy link
Contributor Author

@peterbourgon PTAL

@2opremio 2opremio mentioned this pull request Sep 15, 2015
Closed
@2opremio
Copy link
Contributor

I confirm that I get the expected edges both for application and system containers in the ECS demo. However, I am missing edges between the Internet node and the httpservers . I created a separate issue #489 at the request of @tomwilkie since this PR already improves the situation a lot.

@peterbourgon
Copy link
Contributor

LGTM

tomwilkie added a commit that referenced this pull request Sep 15, 2015
@tomwilkie
Merge pull request #465 from weaveworks/446-missing-edges
c562400 
Fix regression in missing edges
@tomwilkie tomwilkie merged commit c562400 into master Sep 15, 2015
@tomwilkie tomwilkie deleted the 446-missing-edges branch September 15, 2015 14:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

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