From a82d245e93082a965a12e7ba9e5d3a1af7e7b599 Mon Sep 17 00:00:00 2001 From: Matthias Radestock Date: Sat, 18 Nov 2017 15:30:00 +0000 Subject: [PATCH] simplify render decoration Decoration is in fact quite a simple process that is applied on entry to rendering: we take a base renderer, transform it with a decorator, and then render a report with it. The new render.Decorate() function does exactly that. There is one exception. When rendering an individual node, e.g. for showing its details panel in the UI, we must not lose the node during decoration. That requires some special logic, which previously resided in the PreciousNodeRenderer, and now lives in handleNode. --- app/api_topologies.go | 7 ++----- app/api_topologies_test.go | 6 +++--- app/api_topology.go | 35 +++++++++++++++++++++++++--------- app/benchmark_internal_test.go | 2 +- render/container_test.go | 15 +++++---------- render/filters.go | 18 ----------------- render/pod_test.go | 8 ++------ render/render.go | 24 ++++++++--------------- 8 files changed, 47 insertions(+), 68 deletions(-) diff --git a/app/api_topologies.go b/app/api_topologies.go index 78481e355..0a8ca0a98 100644 --- a/app/api_topologies.go +++ b/app/api_topologies.go @@ -504,7 +504,7 @@ func decorateWithStats(rpt report.Report, renderer render.Renderer, decorator re realNodes int edges int ) - r := renderer.Render(rpt, decorator) + r := render.Decorate(rpt, renderer, decorator) for _, n := range r.Nodes { nodes++ if n.Topology != render.Pseudo { @@ -541,10 +541,7 @@ func (r *Registry) RendererForTopology(topologyID string, values url.Values, rpt } } if len(decorators) > 0 { - // Here we tell the topology renderer to apply the filtering decorator - // that we construct as a composition of all the selected filters. - composedFilterDecorator := render.ComposeDecorators(decorators...) - return render.ApplyDecorator(topology.renderer), composedFilterDecorator, nil + return topology.renderer, render.ComposeDecorators(decorators...), nil } return topology.renderer, nil, nil } diff --git a/app/api_topologies_test.go b/app/api_topologies_test.go index 64fda080f..d468c7947 100644 --- a/app/api_topologies_test.go +++ b/app/api_topologies_test.go @@ -118,7 +118,7 @@ func TestRendererForTopologyWithFiltering(t *testing.T) { input.Container.Nodes[fixture.ClientContainerNodeID] = input.Container.Nodes[fixture.ClientContainerNodeID].WithLatests(map[string]string{ docker.LabelPrefix + "works.weave.role": "system", }) - have := utils.Prune(renderer.Render(input, decorator).Nodes) + have := utils.Prune(render.Decorate(input, renderer, decorator).Nodes) want := utils.Prune(expected.RenderedContainers.Copy()) delete(want, fixture.ClientContainerNodeID) delete(want, render.MakePseudoNodeID(render.UncontainedID, fixture.ServerHostID)) @@ -149,7 +149,7 @@ func TestRendererForTopologyNoFiltering(t *testing.T) { input.Container.Nodes[fixture.ClientContainerNodeID] = input.Container.Nodes[fixture.ClientContainerNodeID].WithLatests(map[string]string{ docker.LabelPrefix + "works.weave.role": "system", }) - have := utils.Prune(renderer.Render(input, decorator).Nodes) + have := utils.Prune(render.Decorate(input, renderer, decorator).Nodes) want := utils.Prune(expected.RenderedContainers.Copy()) delete(want, render.MakePseudoNodeID(render.UncontainedID, fixture.ServerHostID)) delete(want, render.OutgoingInternetID) @@ -183,7 +183,7 @@ func getTestContainerLabelFilterTopologySummary(t *testing.T, exclude bool) (det return nil, err } - return detailed.Summaries(report.RenderContext{Report: fixture.Report}, renderer.Render(fixture.Report, decorator).Nodes), nil + return detailed.Summaries(report.RenderContext{Report: fixture.Report}, render.Decorate(fixture.Report, renderer, decorator).Nodes), nil } func TestAPITopologyAddsKubernetes(t *testing.T) { diff --git a/app/api_topology.go b/app/api_topology.go index 6c95e7642..ffa339e86 100644 --- a/app/api_topology.go +++ b/app/api_topology.go @@ -31,25 +31,42 @@ type APINode struct { // Full topology. func handleTopology(ctx context.Context, renderer render.Renderer, decorator render.Decorator, rc report.RenderContext, w http.ResponseWriter, r *http.Request) { respondWith(w, http.StatusOK, APITopology{ - Nodes: detailed.Summaries(rc, renderer.Render(rc.Report, decorator).Nodes), + Nodes: detailed.Summaries(rc, render.Decorate(rc.Report, renderer, decorator).Nodes), }) } // Individual nodes. func handleNode(ctx context.Context, renderer render.Renderer, decorator render.Decorator, rc report.RenderContext, w http.ResponseWriter, r *http.Request) { var ( - vars = mux.Vars(r) - topologyID = vars["topology"] - nodeID = vars["id"] - preciousRenderer = render.PreciousNodeRenderer{PreciousNodeID: nodeID, Renderer: renderer} - rendered = preciousRenderer.Render(rc.Report, decorator).Nodes - node, ok = rendered[nodeID] + vars = mux.Vars(r) + topologyID = vars["topology"] + nodeID = vars["id"] ) + // We must not lose the node during decoration. We achieve that by + // (1) rendering the report with the base renderer, without + // decoration, which gives us the node (if it exists at all), then + // (2) performing a normal decorated render of the report. If the + // node is lost in the second step, we simply put it back. + // + // To avoid repeating the work from step (1) in step (2), we + // replace the renderer in the latter with a constant renderer of + // the result obtained in step (1). + nodes := renderer.Render(rc.Report, nil) + node, ok := nodes.Nodes[nodeID] if !ok { http.NotFound(w, r) return } - respondWith(w, http.StatusOK, APINode{Node: detailed.MakeNode(topologyID, rc, rendered, node)}) + if decorator != nil { + nodes = render.Decorate(rc.Report, render.ConstantRenderer{Nodes: nodes}, decorator) + if decoratedNode, ok := nodes.Nodes[nodeID]; ok { + node = decoratedNode + } else { // we've lost the node during decoration; put it back + nodes.Nodes[nodeID] = node + nodes.Filtered-- + } + } + respondWith(w, http.StatusOK, APINode{Node: detailed.MakeNode(topologyID, rc, nodes.Nodes, node)}) } // Websocket for the full topology. @@ -123,7 +140,7 @@ func handleWebsocket( log.Errorf("Error generating report: %v", err) return } - newTopo := detailed.Summaries(RenderContextForReporter(rep, re), renderer.Render(re, decorator).Nodes) + newTopo := detailed.Summaries(RenderContextForReporter(rep, re), render.Decorate(re, renderer, decorator).Nodes) diff := detailed.TopoDiff(previousTopo, newTopo) previousTopo = newTopo diff --git a/app/benchmark_internal_test.go b/app/benchmark_internal_test.go index 9eae97bfb..bbdd87c5d 100644 --- a/app/benchmark_internal_test.go +++ b/app/benchmark_internal_test.go @@ -69,6 +69,6 @@ func benchmarkOneTopology(b *testing.B, topologyID string) { if err != nil { b.Fatal(err) } - renderer.Render(report, decorator) + render.Decorate(report, renderer, decorator) }) } diff --git a/render/container_test.go b/render/container_test.go index 0d765b21d..156a41ade 100644 --- a/render/container_test.go +++ b/render/container_test.go @@ -70,13 +70,12 @@ func TestContainerFilterRenderer(t *testing.T) { // tag on of the containers in the topology and ensure // it is filtered out correctly. input := fixture.Report.Copy() - renderer := render.ApplyDecorator(render.ContainerWithImageNameRenderer) input.Container.Nodes[fixture.ClientContainerNodeID] = input.Container.Nodes[fixture.ClientContainerNodeID].WithLatests(map[string]string{ docker.LabelPrefix + "works.weave.role": "system", }) - have := utils.Prune(renderer.Render(input, FilterApplication).Nodes) + have := utils.Prune(render.Decorate(input, render.ContainerWithImageNameRenderer, FilterApplication).Nodes) want := utils.Prune(expected.RenderedContainers.Copy()) delete(want, fixture.ClientContainerNodeID) if !reflect.DeepEqual(want, have) { @@ -85,8 +84,7 @@ func TestContainerFilterRenderer(t *testing.T) { } func TestContainerHostnameRenderer(t *testing.T) { - renderer := render.ApplyDecorator(render.ContainerHostnameRenderer) - have := utils.Prune(renderer.Render(fixture.Report, FilterNoop).Nodes) + have := utils.Prune(render.Decorate(fixture.Report, render.ContainerHostnameRenderer, FilterNoop).Nodes) want := utils.Prune(expected.RenderedContainerHostnames) if !reflect.DeepEqual(want, have) { t.Error(test.Diff(want, have)) @@ -94,8 +92,7 @@ func TestContainerHostnameRenderer(t *testing.T) { } func TestContainerHostnameFilterRenderer(t *testing.T) { - renderer := render.ApplyDecorator(render.ContainerHostnameRenderer) - have := utils.Prune(renderer.Render(fixture.Report, FilterSystem).Nodes) + have := utils.Prune(render.Decorate(fixture.Report, render.ContainerHostnameRenderer, FilterSystem).Nodes) want := utils.Prune(expected.RenderedContainerHostnames.Copy()) delete(want, fixture.ClientContainerHostname) delete(want, fixture.ServerContainerHostname) @@ -106,8 +103,7 @@ func TestContainerHostnameFilterRenderer(t *testing.T) { } func TestContainerImageRenderer(t *testing.T) { - renderer := render.ApplyDecorator(render.ContainerImageRenderer) - have := utils.Prune(renderer.Render(fixture.Report, FilterNoop).Nodes) + have := utils.Prune(render.Decorate(fixture.Report, render.ContainerImageRenderer, FilterNoop).Nodes) want := utils.Prune(expected.RenderedContainerImages) if !reflect.DeepEqual(want, have) { t.Error(test.Diff(want, have)) @@ -115,8 +111,7 @@ func TestContainerImageRenderer(t *testing.T) { } func TestContainerImageFilterRenderer(t *testing.T) { - renderer := render.ApplyDecorator(render.ContainerImageRenderer) - have := utils.Prune(renderer.Render(fixture.Report, FilterSystem).Nodes) + have := utils.Prune(render.Decorate(fixture.Report, render.ContainerImageRenderer, FilterSystem).Nodes) want := utils.Prune(expected.RenderedContainerHostnames.Copy()) delete(want, fixture.ClientContainerHostname) delete(want, fixture.ServerContainerHostname) diff --git a/render/filters.go b/render/filters.go index aa60ebe0b..6bb888ec4 100644 --- a/render/filters.go +++ b/render/filters.go @@ -14,24 +14,6 @@ const ( swarmNamespaceLabel = "com.docker.stack.namespace" ) -// PreciousNodeRenderer ensures a node is never filtered out by decorators -type PreciousNodeRenderer struct { - PreciousNodeID string - Renderer -} - -// Render implements Renderer -func (p PreciousNodeRenderer) Render(rpt report.Report, dct Decorator) Nodes { - undecoratedNodes := p.Renderer.Render(rpt, nil) - preciousNode, foundBeforeDecoration := undecoratedNodes.Nodes[p.PreciousNodeID] - finalNodes := applyDecorator{ConstantRenderer{undecoratedNodes}}.Render(rpt, dct) - if _, ok := finalNodes.Nodes[p.PreciousNodeID]; !ok && foundBeforeDecoration { - finalNodes.Nodes[p.PreciousNodeID] = preciousNode - finalNodes.Filtered-- - } - return finalNodes -} - // CustomRenderer allow for mapping functions that received the entire topology // in one call - useful for functions that need to consider the entire graph. // We should minimise the use of this renderer type, as it is very inflexible. diff --git a/render/pod_test.go b/render/pod_test.go index 9be35ffb1..92a7d3a9c 100644 --- a/render/pod_test.go +++ b/render/pod_test.go @@ -28,13 +28,11 @@ func TestPodFilterRenderer(t *testing.T) { // tag on containers or pod namespace in the topology and ensure // it is filtered out correctly. input := fixture.Report.Copy() - renderer := render.ApplyDecorator(render.PodRenderer) - input.Pod.Nodes[fixture.ClientPodNodeID] = input.Pod.Nodes[fixture.ClientPodNodeID].WithLatests(map[string]string{ kubernetes.Namespace: "kube-system", }) - have := utils.Prune(renderer.Render(input, filterNonKubeSystem).Nodes) + have := utils.Prune(render.Decorate(input, render.PodRenderer, filterNonKubeSystem).Nodes) want := utils.Prune(expected.RenderedPods.Copy()) delete(want, fixture.ClientPodNodeID) if !reflect.DeepEqual(want, have) { @@ -54,13 +52,11 @@ func TestPodServiceFilterRenderer(t *testing.T) { // tag on containers or pod namespace in the topology and ensure // it is filtered out correctly. input := fixture.Report.Copy() - renderer := render.ApplyDecorator(render.PodServiceRenderer) - input.Service.Nodes[fixture.ServiceNodeID] = input.Service.Nodes[fixture.ServiceNodeID].WithLatests(map[string]string{ kubernetes.Namespace: "kube-system", }) - have := utils.Prune(renderer.Render(input, filterNonKubeSystem).Nodes) + have := utils.Prune(render.Decorate(input, render.PodServiceRenderer, filterNonKubeSystem).Nodes) want := utils.Prune(expected.RenderedPodServices.Copy()) delete(want, fixture.ServiceNodeID) delete(want, render.IncomingInternetID) diff --git a/render/render.go b/render/render.go index 3741e89f4..317901d01 100644 --- a/render/render.go +++ b/render/render.go @@ -29,6 +29,14 @@ func (r Nodes) Merge(o Nodes) Nodes { } } +// Decorate renders the report with a decorated renderer +func Decorate(rpt report.Report, renderer Renderer, dct Decorator) Nodes { + if dct != nil { + renderer = dct(renderer) + } + return renderer.Render(rpt, nil) +} + // Reduce renderer is a Renderer which merges together the output of several // other renderers. type Reduce []Renderer @@ -124,22 +132,6 @@ func ComposeDecorators(decorators ...Decorator) Decorator { } } -type applyDecorator struct { - Renderer -} - -func (ad applyDecorator) Render(rpt report.Report, dct Decorator) Nodes { - if dct != nil { - return dct(ad.Renderer).Render(rpt, nil) - } - return ad.Renderer.Render(rpt, nil) -} - -// ApplyDecorator returns a renderer which will apply the given decorator to the child render. -func ApplyDecorator(renderer Renderer) Renderer { - return applyDecorator{renderer} -} - func propagateLatest(key string, from, to report.Node) report.Node { if value, timestamp, ok := from.Latest.LookupEntry(key); ok { to.Latest = to.Latest.Set(key, timestamp, value)