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

Push mini-reports when container states changes #640

Merged
merged 3 commits into from
Nov 11, 2015

Conversation

tomwilkie
Copy link
Contributor

Fixes #629

@tomwilkie tomwilkie self-assigned this Nov 9, 2015
@tomwilkie tomwilkie force-pushed the 629-container-state-push branch 2 times, most recently from 00bbd04 to 8a2bf58 Compare November 9, 2015 17:12
@tomwilkie
Copy link
Contributor Author

Right now, this change bypasses the 3s probe publish interval for docker container state changes, pushing a mini-report containing just the container node. As discussed, we're going to add a flag to reports (fastforward?) to indicate to the app that it should push a delta to the UI quickly too, eliminating another source of delay.

@tomwilkie tomwilkie changed the title [WIP] Push mini-reports when container states changes Push mini-reports when container states changes Nov 10, 2015
@tomwilkie tomwilkie removed their assignment Nov 10, 2015
@tomwilkie
Copy link
Contributor Author

Status: when a container state changes, a probe -> app mini report is pushed immediately; this causes an app -> ui topology push to also happen, and you get sub-second updated of the rendered graph (yay!)

Unfortunately, the details panel is on a 1 sec poll, driven by the UI, so is beyond scope for these changes. @davkal is there anything we can do? I suspect we're going to have to rework the app <-> UI websocket when pipes come along, so we could delay the work until then.

@davkal
Copy link
Contributor

davkal commented Nov 10, 2015

@tomwilkie I was waiting for something like this to come up. I've been meaning to look into GraphQL, or Netflix's Falcor for this. But then again maybe this is overblown. Until then we can send arbitrary messages along the websocket, in addition to node set changes.

@tomwilkie
Copy link
Contributor Author

I think for pipes we're going to need the websocket to be 'stationary' - ie not connected / disconnected for every topology the user clicks. If so, the websocket protocol might need to evolve to something along the lines of the UI 'subscribing' to interesting 'channels' - such as the rendered topology updates, node detail updates, pipes etc. Just a though, lets move this discussion off this PR.

@davkal
Copy link
Contributor

davkal commented Nov 10, 2015

Yes, the current implementation with one websocket per topology is not optimal. It's a garbled REST approach. Maybe we can transition to a single socket.io connection that will also have built-in fallbacks if websockets are not supported somewhere along the wire.

@tomwilkie tomwilkie force-pushed the 629-container-state-push branch from 980b9be to 47ef1e0 Compare November 10, 2015 13:47
@tomwilkie
Copy link
Contributor Author

The failure was unrelated (and is now fixed). This should be ready for review.

p.AddReporter(docker.NewReporter(registry, hostID))

reporter := docker.NewReporter(registry, hostID, p)
registry.WatchContainerUpdates(reporter.ContainerUpdated)

This comment was marked as abuse.

p.drainAndPublish(report.MakeReport(), p.spiedReports)

case rpt := <-p.shortcutReports:
p.drainAndPublish(rpt, p.shortcutReports)

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

@paulbellamy
Copy link
Contributor

LGTM, then.

tomwilkie added a commit that referenced this pull request Nov 11, 2015
Push mini-reports when container states changes
@tomwilkie tomwilkie merged commit 57e2046 into master Nov 11, 2015
@tomwilkie tomwilkie deleted the 629-container-state-push branch November 11, 2015 13:32
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.

3 participants