From 97fadd6d9f7fe12c9cb5b4b95472af376c7becfc Mon Sep 17 00:00:00 2001 From: Matthias Radestock Date: Wed, 22 Nov 2017 23:58:11 +0000 Subject: [PATCH 1/7] optimising refactor: FilterUnconnected{Pseudo} w/o LatestMap Instead of three passes 1. building a 'connected' node set 2. marking nodes from that set with a LatestMap entry 3. removing unmarked nodes we just do two 1. building a 'connected' node set 2. removing nodes not in that set This does entail duplication of the adjecency list pruning code from FilterFunc.Apply. We will be able to eliminate that eventually, but not just yet. Also, we cannot get rid of ColorConnected completely; ColorConnectedProcessRenderer uses it to mark nodes, and that information is required by detailed.processNodeSummary to determine whether a process in the details panel can be rendered as a link. --- render/filters.go | 73 ++++++++++++++++++++++++++++++++--------------- 1 file changed, 50 insertions(+), 23 deletions(-) diff --git a/render/filters.go b/render/filters.go index 150a6098eb..16b11971f9 100644 --- a/render/filters.go +++ b/render/filters.go @@ -143,6 +143,22 @@ func IsConnected(node report.Node) bool { return ok } +// connected returns the node ids of nodes which have edges to/from +// them, excluding edges to/from themselves. +func connected(nodes report.Nodes) map[string]struct{} { + res := map[string]struct{}{} + void := struct{}{} + for id, node := range nodes { + for _, adj := range node.Adjacency { + if adj != id { + res[id] = void + res[adj] = void + } + } + } + return res +} + // ColorConnected colors nodes with the IsConnectedMark key if they // have edges to or from them. Edges to/from yourself are not counted // here (see #656). @@ -150,20 +166,8 @@ func ColorConnected(r Renderer) Renderer { return CustomRenderer{ Renderer: r, RenderFunc: func(input Nodes) Nodes { - connected := map[string]struct{}{} - void := struct{}{} - - for id, node := range input.Nodes { - for _, adj := range node.Adjacency { - if adj != id { - connected[id] = void - connected[adj] = void - } - } - } - output := input.Copy() - for id := range connected { + for id := range connected(input.Nodes) { output[id] = output[id].WithLatest(IsConnectedMark, mtime.Now(), "true") } return Nodes{Nodes: output, Filtered: input.Filtered} @@ -171,24 +175,47 @@ func ColorConnected(r Renderer) Renderer { } } +func filterUnconnected(input Nodes, onlyPseudo bool) Nodes { + connected := connected(input.Nodes) + output := report.Nodes{} + filtered := input.Filtered + for id, node := range input.Nodes { + if _, ok := connected[id]; ok || (onlyPseudo && !IsPseudoTopology(node)) { + output[id] = node + } else { + filtered++ + } + } + // Deleted nodes also need to be cut as destinations in adjacency lists. + for id, node := range output { + newAdjacency := report.MakeIDList() + for _, dstID := range node.Adjacency { + if _, ok := output[dstID]; ok { + newAdjacency = newAdjacency.Add(dstID) + } + } + node.Adjacency = newAdjacency + output[id] = node + } + return Nodes{Nodes: output, Filtered: filtered} +} + // FilterUnconnected produces a renderer that filters unconnected nodes // from the given renderer func FilterUnconnected(r Renderer) Renderer { - return MakeFilterPseudo(IsConnected, ColorConnected(r)) + return CustomRenderer{ + Renderer: r, + RenderFunc: func(input Nodes) Nodes { return filterUnconnected(input, false) }, + } } // FilterUnconnectedPseudo produces a renderer that filters // unconnected pseudo nodes from the given renderer func FilterUnconnectedPseudo(r Renderer) Renderer { - return MakeFilterPseudo( - func(node report.Node) bool { - if !IsPseudoTopology(node) { - return true - } - return IsConnected(node) - }, - ColorConnected(r), - ) + return CustomRenderer{ + Renderer: r, + RenderFunc: func(input Nodes) Nodes { return filterUnconnected(input, true) }, + } } // Noop allows all nodes through From 7bbded9e84bc440a9c908cea7e3185e090354272 Mon Sep 17 00:00:00 2001 From: Matthias Radestock Date: Wed, 22 Nov 2017 18:44:49 +0000 Subject: [PATCH 2/7] refactor(ish): introduce Tranformers so we can generalise the filter step in render.Render et al. That will allow us to apply whole-topology filters in that step. --- app/api_topologies.go | 12 ++++++------ app/api_topologies_test.go | 8 ++++---- app/api_topology.go | 20 +++++++++----------- render/container_test.go | 4 ++-- render/filters.go | 6 +++--- render/filters_test.go | 12 ++++++------ render/render.go | 23 ++++++++++++++++++----- 7 files changed, 48 insertions(+), 37 deletions(-) diff --git a/app/api_topologies.go b/app/api_topologies.go index 53405196a3..a7f3d9328a 100644 --- a/app/api_topologies.go +++ b/app/api_topologies.go @@ -496,13 +496,13 @@ func (r *Registry) renderTopologies(rpt report.Report, req *http.Request) []APIT return updateFilters(rpt, topologies) } -func computeStats(rpt report.Report, renderer render.Renderer, filter render.FilterFunc) topologyStats { +func computeStats(rpt report.Report, renderer render.Renderer, transformer render.Transformer) topologyStats { var ( nodes int realNodes int edges int ) - r := render.Render(rpt, renderer, filter) + r := render.Render(rpt, renderer, transformer) for _, n := range r.Nodes { nodes++ if n.Topology != render.Pseudo { @@ -519,7 +519,7 @@ func computeStats(rpt report.Report, renderer render.Renderer, filter render.Fil } // RendererForTopology .. -func (r *Registry) RendererForTopology(topologyID string, values url.Values, rpt report.Report) (render.Renderer, render.FilterFunc, error) { +func (r *Registry) RendererForTopology(topologyID string, values url.Values, rpt report.Report) (render.Renderer, render.Transformer, error) { topology, ok := r.get(topologyID) if !ok { return nil, nil, fmt.Errorf("topology not found: %s", topologyID) @@ -528,7 +528,7 @@ func (r *Registry) RendererForTopology(topologyID string, values url.Values, rpt if len(values) == 0 { // Do not apply filtering if no options where provided - return topology.renderer, nil, nil + return topology.renderer, render.Transformers(nil), nil } var filters []render.FilterFunc @@ -541,7 +541,7 @@ func (r *Registry) RendererForTopology(topologyID string, values url.Values, rpt if len(filters) > 0 { return topology.renderer, render.ComposeFilterFuncs(filters...), nil } - return topology.renderer, nil, nil + return topology.renderer, render.Transformers(nil), nil } type reporterHandler func(context.Context, Reporter, http.ResponseWriter, *http.Request) @@ -552,7 +552,7 @@ func captureReporter(rep Reporter, f reporterHandler) CtxHandlerFunc { } } -type rendererHandler func(context.Context, render.Renderer, render.FilterFunc, report.RenderContext, http.ResponseWriter, *http.Request) +type rendererHandler func(context.Context, render.Renderer, render.Transformer, report.RenderContext, http.ResponseWriter, *http.Request) func (r *Registry) captureRenderer(rep Reporter, f rendererHandler) CtxHandlerFunc { return func(ctx context.Context, w http.ResponseWriter, req *http.Request) { diff --git a/app/api_topologies_test.go b/app/api_topologies_test.go index a238adcf41..2dbfeb6a5e 100644 --- a/app/api_topologies_test.go +++ b/app/api_topologies_test.go @@ -164,14 +164,14 @@ func getTestContainerLabelFilterTopologySummary(t *testing.T, exclude bool) (det var ( topologyRegistry = app.MakeRegistry() - filter render.FilterFunc + filterFunc render.FilterFunc ) if exclude == true { - filter = render.DoesNotHaveLabel(fixture.TestLabelKey2, fixture.ApplicationLabelValue2) + filterFunc = render.DoesNotHaveLabel(fixture.TestLabelKey2, fixture.ApplicationLabelValue2) } else { - filter = render.HasLabel(fixture.TestLabelKey1, fixture.ApplicationLabelValue1) + filterFunc = render.HasLabel(fixture.TestLabelKey1, fixture.ApplicationLabelValue1) } - option := app.MakeAPITopologyOption(customAPITopologyOptionFilterID, "title", filter, false) + option := app.MakeAPITopologyOption(customAPITopologyOptionFilterID, "title", filterFunc, false) topologyRegistry.AddContainerFilters(option) urlvalues := url.Values{} diff --git a/app/api_topology.go b/app/api_topology.go index e0a3ded161..145b5aa85b 100644 --- a/app/api_topology.go +++ b/app/api_topology.go @@ -29,14 +29,14 @@ type APINode struct { } // Full topology. -func handleTopology(ctx context.Context, renderer render.Renderer, filter render.FilterFunc, rc report.RenderContext, w http.ResponseWriter, r *http.Request) { +func handleTopology(ctx context.Context, renderer render.Renderer, transformer render.Transformer, rc report.RenderContext, w http.ResponseWriter, r *http.Request) { respondWith(w, http.StatusOK, APITopology{ - Nodes: detailed.Summaries(rc, render.Render(rc.Report, renderer, filter).Nodes), + Nodes: detailed.Summaries(rc, render.Render(rc.Report, renderer, transformer).Nodes), }) } // Individual nodes. -func handleNode(ctx context.Context, renderer render.Renderer, filter render.FilterFunc, rc report.RenderContext, w http.ResponseWriter, r *http.Request) { +func handleNode(ctx context.Context, renderer render.Renderer, transformer render.Transformer, rc report.RenderContext, w http.ResponseWriter, r *http.Request) { var ( vars = mux.Vars(r) topologyID = vars["topology"] @@ -53,14 +53,12 @@ func handleNode(ctx context.Context, renderer render.Renderer, filter render.Fil http.NotFound(w, r) return } - if filter != nil { - nodes = filter.Apply(nodes) - if filteredNode, ok := nodes.Nodes[nodeID]; ok { - node = filteredNode - } else { // we've lost the node during filtering; put it back - nodes.Nodes[nodeID] = node - nodes.Filtered-- - } + nodes = transformer.Transform(nodes) + if filteredNode, ok := nodes.Nodes[nodeID]; ok { + node = filteredNode + } else { // we've lost the node during filtering; put it back + nodes.Nodes[nodeID] = node + nodes.Filtered-- } respondWith(w, http.StatusOK, APINode{Node: detailed.MakeNode(topologyID, rc, nodes.Nodes, node)}) } diff --git a/render/container_test.go b/render/container_test.go index 9ec27887ca..cc0dd41489 100644 --- a/render/container_test.go +++ b/render/container_test.go @@ -74,7 +74,7 @@ func TestContainerFilterRenderer(t *testing.T) { } func TestContainerHostnameRenderer(t *testing.T) { - have := utils.Prune(render.Render(fixture.Report, render.ContainerHostnameRenderer, nil).Nodes) + have := utils.Prune(render.Render(fixture.Report, render.ContainerHostnameRenderer, render.Transformers(nil)).Nodes) want := utils.Prune(expected.RenderedContainerHostnames) if !reflect.DeepEqual(want, have) { t.Error(test.Diff(want, have)) @@ -93,7 +93,7 @@ func TestContainerHostnameFilterRenderer(t *testing.T) { } func TestContainerImageRenderer(t *testing.T) { - have := utils.Prune(render.Render(fixture.Report, render.ContainerImageRenderer, nil).Nodes) + have := utils.Prune(render.Render(fixture.Report, render.ContainerImageRenderer, render.Transformers(nil)).Nodes) want := utils.Prune(expected.RenderedContainerImages) if !reflect.DeepEqual(want, have) { t.Error(test.Diff(want, have)) diff --git a/render/filters.go b/render/filters.go index 16b11971f9..7093c84bf7 100644 --- a/render/filters.go +++ b/render/filters.go @@ -60,8 +60,8 @@ func Complement(f FilterFunc) FilterFunc { return func(node report.Node) bool { return !f(node) } } -// Apply applies the filter to all nodes -func (f FilterFunc) Apply(nodes Nodes) Nodes { +// Transform applies the filter to all nodes +func (f FilterFunc) Transform(nodes Nodes) Nodes { output := report.Nodes{} inDegrees := map[string]int{} filtered := nodes.Filtered @@ -128,7 +128,7 @@ func MakeFilterPseudo(f FilterFunc, r Renderer) Renderer { // Render implements Renderer func (f Filter) Render(rpt report.Report) Nodes { - return f.FilterFunc.Apply(f.Renderer.Render(rpt)) + return f.FilterFunc.Transform(f.Renderer.Render(rpt)) } // IsConnectedMark is the key added to Node.Metadata by diff --git a/render/filters_test.go b/render/filters_test.go index 5854641fd7..8def19720d 100644 --- a/render/filters_test.go +++ b/render/filters_test.go @@ -20,7 +20,7 @@ func TestFilterRender(t *testing.T) { "baz": report.MakeNode("baz"), }} have := report.MakeIDList() - for id := range render.Render(report.MakeReport(), render.ColorConnected(renderer), render.IsConnected).Nodes { + for id := range render.Render(report.MakeReport(), render.ColorConnected(renderer), render.FilterFunc(render.IsConnected)).Nodes { have = have.Add(id) } want := report.MakeIDList("foo", "bar") @@ -36,7 +36,7 @@ func TestFilterRender2(t *testing.T) { "bar": report.MakeNode("bar").WithAdjacent("foo"), "baz": report.MakeNode("baz"), }} - have := render.Render(report.MakeReport(), renderer, isNotBar).Nodes + have := render.Render(report.MakeReport(), renderer, render.FilterFunc(isNotBar)).Nodes if have["foo"].Adjacency.Contains("bar") { t.Error("adjacencies for removed nodes should have been removed") } @@ -53,7 +53,7 @@ func TestFilterUnconnectedPseudoNodes(t *testing.T) { } renderer := mockRenderer{Nodes: nodes} want := nodes - have := render.Render(report.MakeReport(), renderer, nil).Nodes + have := render.Render(report.MakeReport(), renderer, render.Transformers(nil)).Nodes if !reflect.DeepEqual(want, have) { t.Error(test.Diff(want, have)) } @@ -64,7 +64,7 @@ func TestFilterUnconnectedPseudoNodes(t *testing.T) { "bar": report.MakeNode("bar").WithAdjacent("baz"), "baz": report.MakeNode("baz").WithTopology(render.Pseudo), }} - have := render.Render(report.MakeReport(), renderer, isNotBar).Nodes + have := render.Render(report.MakeReport(), renderer, render.FilterFunc(isNotBar)).Nodes if _, ok := have["baz"]; ok { t.Error("expected the unconnected pseudonode baz to have been removed") } @@ -75,7 +75,7 @@ func TestFilterUnconnectedPseudoNodes(t *testing.T) { "bar": report.MakeNode("bar").WithAdjacent("foo"), "baz": report.MakeNode("baz").WithTopology(render.Pseudo).WithAdjacent("bar"), }} - have := render.Render(report.MakeReport(), renderer, isNotBar).Nodes + have := render.Render(report.MakeReport(), renderer, render.FilterFunc(isNotBar)).Nodes if _, ok := have["baz"]; ok { t.Error("expected the unconnected pseudonode baz to have been removed") } @@ -89,7 +89,7 @@ func TestFilterUnconnectedSelf(t *testing.T) { "foo": report.MakeNode("foo").WithAdjacent("foo"), } renderer := mockRenderer{Nodes: nodes} - have := render.Render(report.MakeReport(), render.ColorConnected(renderer), render.IsConnected).Nodes + have := render.Render(report.MakeReport(), render.ColorConnected(renderer), render.FilterFunc(render.IsConnected)).Nodes if len(have) > 0 { t.Error("expected node only connected to self to be removed") } diff --git a/render/render.go b/render/render.go index bcf3e24953..05901aa529 100644 --- a/render/render.go +++ b/render/render.go @@ -29,15 +29,28 @@ func (r Nodes) Merge(o Nodes) Nodes { } } -// Render renders the report and then applies the filter -func Render(rpt report.Report, renderer Renderer, filter FilterFunc) Nodes { - nodes := renderer.Render(rpt) - if filter != nil { - nodes = filter.Apply(nodes) +// Transformer is something that transforms one set of Nodes to +// another set of Nodes. +type Transformer interface { + Transform(nodes Nodes) Nodes +} + +// Transformers is a composition of Transformers +type Transformers []Transformer + +// Transform implements Transformer +func (ts Transformers) Transform(nodes Nodes) Nodes { + for _, t := range ts { + nodes = t.Transform(nodes) } return nodes } +// Render renders the report and then transforms it +func Render(rpt report.Report, renderer Renderer, transformer Transformer) Nodes { + return transformer.Transform(renderer.Render(rpt)) +} + // Reduce renderer is a Renderer which merges together the output of several // other renderers. type Reduce []Renderer From 2d964b669a0102d99d66d54268fdc80312588a5f Mon Sep 17 00:00:00 2001 From: Matthias Radestock Date: Wed, 22 Nov 2017 19:08:55 +0000 Subject: [PATCH 3/7] refactor: separate filtering from rendering in topology description This is a step towards filtering unconnected nodes after all custom filters have been applied. --- app/api_topologies.go | 42 ++++++++++++++++++++++++++++-------------- 1 file changed, 28 insertions(+), 14 deletions(-) diff --git a/app/api_topologies.go b/app/api_topologies.go index a7f3d9328a..f27b93023d 100644 --- a/app/api_topologies.go +++ b/app/api_topologies.go @@ -200,7 +200,8 @@ func MakeRegistry() *Registry { registry.Add( APITopologyDesc{ id: processesID, - renderer: render.FilterUnconnected(render.ProcessWithContainerNameRenderer), + renderer: render.ProcessWithContainerNameRenderer, + filter: render.FilterUnconnected, Name: "Processes", Rank: 1, Options: unconnectedFilter, @@ -209,14 +210,16 @@ func MakeRegistry() *Registry { APITopologyDesc{ id: processesByNameID, parent: processesID, - renderer: render.FilterUnconnected(render.ProcessNameRenderer), + renderer: render.ProcessNameRenderer, + filter: render.FilterUnconnected, Name: "by name", Options: unconnectedFilter, HideIfEmpty: true, }, APITopologyDesc{ id: containersID, - renderer: render.FilterUnconnectedPseudo(render.ContainerWithImageNameRenderer), + renderer: render.ContainerWithImageNameRenderer, + filter: render.FilterUnconnectedPseudo, Name: "Containers", Rank: 2, Options: containerFilters, @@ -224,20 +227,23 @@ func MakeRegistry() *Registry { APITopologyDesc{ id: containersByHostnameID, parent: containersID, - renderer: render.FilterUnconnectedPseudo(render.ContainerHostnameRenderer), + renderer: render.ContainerHostnameRenderer, + filter: render.FilterUnconnectedPseudo, Name: "by DNS name", Options: containerFilters, }, APITopologyDesc{ id: containersByImageID, parent: containersID, - renderer: render.FilterUnconnectedPseudo(render.ContainerImageRenderer), + renderer: render.ContainerImageRenderer, + filter: render.FilterUnconnectedPseudo, Name: "by image", Options: containerFilters, }, APITopologyDesc{ id: podsID, - renderer: render.FilterUnconnectedPseudo(render.PodRenderer), + renderer: render.PodRenderer, + filter: render.FilterUnconnectedPseudo, Name: "Pods", Rank: 3, Options: []APITopologyOptionGroup{unmanagedFilter}, @@ -246,7 +252,8 @@ func MakeRegistry() *Registry { APITopologyDesc{ id: kubeControllersID, parent: podsID, - renderer: render.FilterUnconnectedPseudo(render.KubeControllerRenderer), + renderer: render.KubeControllerRenderer, + filter: render.FilterUnconnectedPseudo, Name: "controllers", Options: []APITopologyOptionGroup{unmanagedFilter}, HideIfEmpty: true, @@ -254,14 +261,16 @@ func MakeRegistry() *Registry { APITopologyDesc{ id: servicesID, parent: podsID, - renderer: render.FilterUnconnectedPseudo(render.PodServiceRenderer), + renderer: render.PodServiceRenderer, + filter: render.FilterUnconnectedPseudo, Name: "services", Options: []APITopologyOptionGroup{unmanagedFilter}, HideIfEmpty: true, }, APITopologyDesc{ id: ecsTasksID, - renderer: render.FilterUnconnectedPseudo(render.ECSTaskRenderer), + renderer: render.ECSTaskRenderer, + filter: render.FilterUnconnectedPseudo, Name: "Tasks", Rank: 3, Options: []APITopologyOptionGroup{unmanagedFilter}, @@ -270,14 +279,16 @@ func MakeRegistry() *Registry { APITopologyDesc{ id: ecsServicesID, parent: ecsTasksID, - renderer: render.FilterUnconnectedPseudo(render.ECSServiceRenderer), + renderer: render.ECSServiceRenderer, + filter: render.FilterUnconnectedPseudo, Name: "services", Options: []APITopologyOptionGroup{unmanagedFilter}, HideIfEmpty: true, }, APITopologyDesc{ id: swarmServicesID, - renderer: render.FilterUnconnectedPseudo(render.SwarmServiceRenderer), + renderer: render.SwarmServiceRenderer, + filter: render.FilterUnconnectedPseudo, Name: "services", Rank: 3, Options: []APITopologyOptionGroup{unmanagedFilter}, @@ -285,14 +296,16 @@ func MakeRegistry() *Registry { }, APITopologyDesc{ id: hostsID, - renderer: render.FilterUnconnectedPseudo(render.HostRenderer), + renderer: render.HostRenderer, + filter: render.FilterUnconnectedPseudo, Name: "Hosts", Rank: 4, }, APITopologyDesc{ id: weaveID, parent: hostsID, - renderer: render.FilterUnconnectedPseudo(render.WeaveRenderer), + renderer: render.WeaveRenderer, + filter: render.FilterUnconnectedPseudo, Name: "Weave Net", }, ) @@ -305,6 +318,7 @@ type APITopologyDesc struct { id string parent string renderer render.Renderer + filter func(render.Renderer) render.Renderer Name string `json:"name"` Rank int `json:"rank"` @@ -433,7 +447,7 @@ func (r *Registry) Add(ts ...APITopologyDesc) { defer r.Unlock() for _, t := range ts { t.URL = apiTopologyURL + t.id - t.renderer = render.Memoise(t.renderer) + t.renderer = render.Memoise(t.filter(t.renderer)) if t.parent != "" { parent := r.items[t.parent] From 9dca7627b64937d2f168d2de1db6c205849539eb Mon Sep 17 00:00:00 2001 From: Matthias Radestock Date: Wed, 22 Nov 2017 20:10:13 +0000 Subject: [PATCH 4/7] filter unconnected nodes after applying user-specified filters ...rather than before. That way, nodes which become unconnected during filtering are removed, which is what we want. ATM we are depending on some 'unconnected' filtering inside every filter, which is expensive and largely redundant. We should soon be able to remove that. downside: 'unconnected' filtering is no longer memoised. --- app/api_topologies.go | 12 ++++++------ render/filters.go | 30 ++++++++++++------------------ 2 files changed, 18 insertions(+), 24 deletions(-) diff --git a/app/api_topologies.go b/app/api_topologies.go index f27b93023d..5aaff9d180 100644 --- a/app/api_topologies.go +++ b/app/api_topologies.go @@ -318,7 +318,7 @@ type APITopologyDesc struct { id string parent string renderer render.Renderer - filter func(render.Renderer) render.Renderer + filter render.Transformer Name string `json:"name"` Rank int `json:"rank"` @@ -447,7 +447,7 @@ func (r *Registry) Add(ts ...APITopologyDesc) { defer r.Unlock() for _, t := range ts { t.URL = apiTopologyURL + t.id - t.renderer = render.Memoise(t.filter(t.renderer)) + t.renderer = render.Memoise(t.renderer) if t.parent != "" { parent := r.items[t.parent] @@ -541,8 +541,8 @@ func (r *Registry) RendererForTopology(topologyID string, values url.Values, rpt topology = updateFilters(rpt, []APITopologyDesc{topology})[0] if len(values) == 0 { - // Do not apply filtering if no options where provided - return topology.renderer, render.Transformers(nil), nil + // if no options where provided, only apply base filter + return topology.renderer, topology.filter, nil } var filters []render.FilterFunc @@ -553,9 +553,9 @@ func (r *Registry) RendererForTopology(topologyID string, values url.Values, rpt } } if len(filters) > 0 { - return topology.renderer, render.ComposeFilterFuncs(filters...), nil + return topology.renderer, render.Transformers([]render.Transformer{render.ComposeFilterFuncs(filters...), topology.filter}), nil } - return topology.renderer, render.Transformers(nil), nil + return topology.renderer, topology.filter, nil } type reporterHandler func(context.Context, Reporter, http.ResponseWriter, *http.Request) diff --git a/render/filters.go b/render/filters.go index 7093c84bf7..5ffe6dc3d3 100644 --- a/render/filters.go +++ b/render/filters.go @@ -175,12 +175,17 @@ func ColorConnected(r Renderer) Renderer { } } -func filterUnconnected(input Nodes, onlyPseudo bool) Nodes { +type filterUnconnected struct { + onlyPseudo bool +} + +// Transform implements Transformer +func (f filterUnconnected) Transform(input Nodes) Nodes { connected := connected(input.Nodes) output := report.Nodes{} filtered := input.Filtered for id, node := range input.Nodes { - if _, ok := connected[id]; ok || (onlyPseudo && !IsPseudoTopology(node)) { + if _, ok := connected[id]; ok || (f.onlyPseudo && !IsPseudoTopology(node)) { output[id] = node } else { filtered++ @@ -200,23 +205,12 @@ func filterUnconnected(input Nodes, onlyPseudo bool) Nodes { return Nodes{Nodes: output, Filtered: filtered} } -// FilterUnconnected produces a renderer that filters unconnected nodes -// from the given renderer -func FilterUnconnected(r Renderer) Renderer { - return CustomRenderer{ - Renderer: r, - RenderFunc: func(input Nodes) Nodes { return filterUnconnected(input, false) }, - } -} +// FilterUnconnected is a transformer that filters unconnected nodes +var FilterUnconnected = filterUnconnected{onlyPseudo: false} -// FilterUnconnectedPseudo produces a renderer that filters -// unconnected pseudo nodes from the given renderer -func FilterUnconnectedPseudo(r Renderer) Renderer { - return CustomRenderer{ - Renderer: r, - RenderFunc: func(input Nodes) Nodes { return filterUnconnected(input, true) }, - } -} +// FilterUnconnectedPseudo is a transformer that filters unconnected +// pseudo nodes +var FilterUnconnectedPseudo = filterUnconnected{onlyPseudo: true} // Noop allows all nodes through func Noop(_ report.Node) bool { return true } From 1416fe928fc3171b622b77b4d8e02961e238f725 Mon Sep 17 00:00:00 2001 From: Matthias Radestock Date: Wed, 22 Nov 2017 21:23:19 +0000 Subject: [PATCH 5/7] remove filtering of unconnected pseudo nodes from ordinary filters It's now done via a special filter, once, after all other filters have been applied. Some tests need updating since they were relying on ordinary filters doing that filtering. --- render/container_test.go | 10 ++++++++-- render/filters.go | 15 --------------- render/filters_test.go | 15 +++++++++------ render/pod_test.go | 5 ++++- 4 files changed, 21 insertions(+), 24 deletions(-) diff --git a/render/container_test.go b/render/container_test.go index cc0dd41489..ca9f51dfad 100644 --- a/render/container_test.go +++ b/render/container_test.go @@ -16,8 +16,14 @@ import ( ) var ( - filterApplication = render.AnyFilterFunc(render.IsPseudoTopology, render.IsApplication) - filterSystem = render.AnyFilterFunc(render.IsPseudoTopology, render.IsSystem) + filterApplication = render.Transformers([]render.Transformer{ + render.AnyFilterFunc(render.IsPseudoTopology, render.IsApplication), + render.FilterUnconnectedPseudo, + }) + filterSystem = render.Transformers([]render.Transformer{ + render.AnyFilterFunc(render.IsPseudoTopology, render.IsSystem), + render.FilterUnconnectedPseudo, + }) ) func TestMapProcess2Container(t *testing.T) { diff --git a/render/filters.go b/render/filters.go index 5ffe6dc3d3..8dba85ecde 100644 --- a/render/filters.go +++ b/render/filters.go @@ -63,12 +63,10 @@ func Complement(f FilterFunc) FilterFunc { // Transform applies the filter to all nodes func (f FilterFunc) Transform(nodes Nodes) Nodes { output := report.Nodes{} - inDegrees := map[string]int{} filtered := nodes.Filtered for id, node := range nodes.Nodes { if f(node) { output[id] = node - inDegrees[id] = 0 } else { filtered++ } @@ -80,25 +78,12 @@ func (f FilterFunc) Transform(nodes Nodes) Nodes { for _, dstID := range node.Adjacency { if _, ok := output[dstID]; ok { newAdjacency = newAdjacency.Add(dstID) - inDegrees[dstID]++ } } node.Adjacency = newAdjacency output[id] = node } - // Remove unconnected pseudo nodes, see #483. - for id, inDegree := range inDegrees { - if inDegree > 0 { - continue - } - node := output[id] - if node.Topology != Pseudo || len(node.Adjacency) > 0 { - continue - } - delete(output, id) - filtered++ - } return Nodes{Nodes: output, Filtered: filtered} } diff --git a/render/filters_test.go b/render/filters_test.go index 8def19720d..b31dfc1fb2 100644 --- a/render/filters_test.go +++ b/render/filters_test.go @@ -9,9 +9,12 @@ import ( "github.com/weaveworks/scope/test/reflect" ) -func isNotBar(node report.Node) bool { - return node.ID != "bar" -} +var filterBar = render.Transformers([]render.Transformer{ + render.FilterFunc(func(node report.Node) bool { + return node.ID != "bar" + }), + render.FilterUnconnectedPseudo, +}) func TestFilterRender(t *testing.T) { renderer := mockRenderer{Nodes: report.Nodes{ @@ -36,7 +39,7 @@ func TestFilterRender2(t *testing.T) { "bar": report.MakeNode("bar").WithAdjacent("foo"), "baz": report.MakeNode("baz"), }} - have := render.Render(report.MakeReport(), renderer, render.FilterFunc(isNotBar)).Nodes + have := render.Render(report.MakeReport(), renderer, filterBar).Nodes if have["foo"].Adjacency.Contains("bar") { t.Error("adjacencies for removed nodes should have been removed") } @@ -64,7 +67,7 @@ func TestFilterUnconnectedPseudoNodes(t *testing.T) { "bar": report.MakeNode("bar").WithAdjacent("baz"), "baz": report.MakeNode("baz").WithTopology(render.Pseudo), }} - have := render.Render(report.MakeReport(), renderer, render.FilterFunc(isNotBar)).Nodes + have := render.Render(report.MakeReport(), renderer, filterBar).Nodes if _, ok := have["baz"]; ok { t.Error("expected the unconnected pseudonode baz to have been removed") } @@ -75,7 +78,7 @@ func TestFilterUnconnectedPseudoNodes(t *testing.T) { "bar": report.MakeNode("bar").WithAdjacent("foo"), "baz": report.MakeNode("baz").WithTopology(render.Pseudo).WithAdjacent("bar"), }} - have := render.Render(report.MakeReport(), renderer, render.FilterFunc(isNotBar)).Nodes + have := render.Render(report.MakeReport(), renderer, filterBar).Nodes if _, ok := have["baz"]; ok { t.Error("expected the unconnected pseudonode baz to have been removed") } diff --git a/render/pod_test.go b/render/pod_test.go index 01c5983410..d707110d39 100644 --- a/render/pod_test.go +++ b/render/pod_test.go @@ -20,7 +20,10 @@ func TestPodRenderer(t *testing.T) { } } -var filterNonKubeSystem = render.Complement(render.IsNamespace("kube-system")) +var filterNonKubeSystem = render.Transformers([]render.Transformer{ + render.Complement(render.IsNamespace("kube-system")), + render.FilterUnconnectedPseudo, +}) func TestPodFilterRenderer(t *testing.T) { // tag on containers or pod namespace in the topology and ensure From f193a2101cf45adfa46de173f51ff83ed43d6ec5 Mon Sep 17 00:00:00 2001 From: Matthias Radestock Date: Wed, 22 Nov 2017 22:32:05 +0000 Subject: [PATCH 6/7] refactor: remove duplication of filtering logic --- render/filters.go | 25 +++++-------------------- 1 file changed, 5 insertions(+), 20 deletions(-) diff --git a/render/filters.go b/render/filters.go index 8dba85ecde..6620344c75 100644 --- a/render/filters.go +++ b/render/filters.go @@ -167,27 +167,12 @@ type filterUnconnected struct { // Transform implements Transformer func (f filterUnconnected) Transform(input Nodes) Nodes { connected := connected(input.Nodes) - output := report.Nodes{} - filtered := input.Filtered - for id, node := range input.Nodes { - if _, ok := connected[id]; ok || (f.onlyPseudo && !IsPseudoTopology(node)) { - output[id] = node - } else { - filtered++ - } - } - // Deleted nodes also need to be cut as destinations in adjacency lists. - for id, node := range output { - newAdjacency := report.MakeIDList() - for _, dstID := range node.Adjacency { - if _, ok := output[dstID]; ok { - newAdjacency = newAdjacency.Add(dstID) - } + return FilterFunc(func(node report.Node) bool { + if _, ok := connected[node.ID]; ok || (f.onlyPseudo && !IsPseudoTopology(node)) { + return true } - node.Adjacency = newAdjacency - output[id] = node - } - return Nodes{Nodes: output, Filtered: filtered} + return false + }).Transform(input) } // FilterUnconnected is a transformer that filters unconnected nodes From 956303694a7fdaa693c283c4f7df08e3f8bd8913 Mon Sep 17 00:00:00 2001 From: Matthias Radestock Date: Fri, 24 Nov 2017 15:05:44 +0000 Subject: [PATCH 7/7] always apply ColorConnected in process renderers This eliminates the awkward distinction between ProcessRenderer and ColorConnectedProcessRenderer. It also ensures that processes resulting from direct rendering of the process topology (/api/topology/processes is invoking ProcessWithContainerNameRenderer and /api/topology/processes-by-name is invoking ProcessNameRenderer) are colored and hence summarising them correctly sets the 'linkable' property. This was the behaviour prior to the revamping of the rendering pipeline. However, it doesn't actually make a practical difference since process detail panels only show other processes as connection endpoints, and these are always marked linkable anyway. --- render/container.go | 2 +- render/host.go | 2 +- render/process.go | 11 ++++------- 3 files changed, 6 insertions(+), 9 deletions(-) diff --git a/render/container.go b/render/container.go index 0dda442866..1c3eb044e3 100644 --- a/render/container.go +++ b/render/container.go @@ -33,7 +33,7 @@ var ContainerRenderer = Memoise(MakeFilter( MakeReduce( MakeMap( MapProcess2Container, - ColorConnectedProcessRenderer, + ProcessRenderer, ), ConnectionJoin(MapContainer2IP, SelectContainer), ), diff --git a/render/host.go b/render/host.go index 4f00356f32..3d7ceccbcc 100644 --- a/render/host.go +++ b/render/host.go @@ -10,7 +10,7 @@ import ( // not memoised var HostRenderer = MakeReduce( endpoints2Hosts{}, - CustomRenderer{RenderFunc: nodes2Hosts, Renderer: ColorConnectedProcessRenderer}, + CustomRenderer{RenderFunc: nodes2Hosts, Renderer: ProcessRenderer}, CustomRenderer{RenderFunc: nodes2Hosts, Renderer: ContainerRenderer}, CustomRenderer{RenderFunc: nodes2Hosts, Renderer: ContainerImageRenderer}, CustomRenderer{RenderFunc: nodes2Hosts, Renderer: PodRenderer}, diff --git a/render/process.go b/render/process.go index b69586e211..9dfde7d130 100644 --- a/render/process.go +++ b/render/process.go @@ -25,14 +25,11 @@ func renderProcesses(rpt report.Report) bool { var EndpointRenderer = SelectEndpoint // ProcessRenderer is a Renderer which produces a renderable process -// graph by merging the endpoint graph and the process topology. -var ProcessRenderer = Memoise(endpoints2Processes{}) - -// ColorConnectedProcessRenderer colors connected nodes from -// ProcessRenderer. Since the process topology views only show -// connected processes, we need this info to determine whether +// graph by merging the endpoint graph and the process topology. It +// also colors connected nodes. Since the process topology views only +// show connected processes, we need this info to determine whether // processes appearing in a details panel are linkable. -var ColorConnectedProcessRenderer = Memoise(ColorConnected(ProcessRenderer)) +var ProcessRenderer = Memoise(ColorConnected(endpoints2Processes{})) // processWithContainerNameRenderer is a Renderer which produces a process // graph enriched with container names where appropriate