From 1ed07f4a0a5444d8d278eb062e18744e5865d9f8 Mon Sep 17 00:00:00 2001 From: Bryan Boreham Date: Fri, 20 Jul 2018 19:57:44 +0000 Subject: [PATCH] Refactor: streamline Node.WithParents() Make it take parameters just like all the callers have, instead of making them create a different structure. It is now only used in tests. --- render/expected/expected.go | 100 +++++++++++++-------------- render/metrics_test.go | 130 ++++++++++++++++-------------------- report/node.go | 10 +-- report/node_set.go | 41 ------------ test/utils/prune.go | 6 +- 5 files changed, 112 insertions(+), 175 deletions(-) diff --git a/render/expected/expected.go b/render/expected/expected.go index 290ceb128..1094dd695 100644 --- a/render/expected/expected.go +++ b/render/expected/expected.go @@ -50,28 +50,28 @@ var ( unknownPseudoNode1 = func(adjacent ...string) report.Node { return pseudo(UnknownPseudoNode1ID, adjacent...). - WithChildren(report.MakeNodeSet( + WithChildren( RenderedEndpoints[fixture.UnknownClient1NodeID], RenderedEndpoints[fixture.UnknownClient2NodeID], - )) + ) } unknownPseudoNode2 = func(adjacent ...string) report.Node { return pseudo(UnknownPseudoNode2ID, adjacent...). - WithChildren(report.MakeNodeSet( + WithChildren( RenderedEndpoints[fixture.UnknownClient3NodeID], - )) + ) } theIncomingInternetNode = func(adjacent ...string) report.Node { return pseudo(render.IncomingInternetID, adjacent...). - WithChildren(report.MakeNodeSet( + WithChildren( RenderedEndpoints[fixture.RandomClientNodeID], - )) + ) } - theOutgoingInternetNode = pseudo(render.OutgoingInternetID).WithChildren(report.MakeNodeSet( + theOutgoingInternetNode = pseudo(render.OutgoingInternetID).WithChildren( RenderedEndpoints[fixture.GoogleEndpointNodeID], - )) + ) RenderedEndpoints = report.Nodes{ fixture.Client54001NodeID: endpoint(fixture.Client54001NodeID, fixture.Server80NodeID), @@ -92,24 +92,24 @@ var ( process.PID: fixture.Client1PID, process.Name: fixture.Client1Name, }). - WithChildren(report.MakeNodeSet( + WithChildren( RenderedEndpoints[fixture.Client54001NodeID], - )), + ), fixture.ClientProcess2NodeID: processNode(fixture.ClientProcess2NodeID, fixture.ServerProcessNodeID). - WithChildren(report.MakeNodeSet( + WithChildren( RenderedEndpoints[fixture.Client54002NodeID], - )), + ), fixture.ServerProcessNodeID: processNode(fixture.ServerProcessNodeID). - WithChildren(report.MakeNodeSet( + WithChildren( RenderedEndpoints[fixture.Server80NodeID], - )), + ), fixture.NonContainerProcessNodeID: processNode(fixture.NonContainerProcessNodeID, render.OutgoingInternetID). - WithChildren(report.MakeNodeSet( + WithChildren( RenderedEndpoints[fixture.NonContainerNodeID], - )), + ), // due to https://github.com/weaveworks/scope/issues/1323 we are dropping // all non-internet pseudo nodes for now. @@ -123,26 +123,26 @@ var ( fixture.Client1Name: processNameNode(fixture.Client1Name, fixture.ServerName). WithLatests(map[string]string{process.Name: fixture.Client1Name}). WithCounters(map[string]int{report.Process: 2}). - WithChildren(report.MakeNodeSet( + WithChildren( RenderedEndpoints[fixture.Client54001NodeID], RenderedEndpoints[fixture.Client54002NodeID], RenderedProcesses[fixture.ClientProcess1NodeID], RenderedProcesses[fixture.ClientProcess2NodeID], - )), + ), fixture.ServerName: processNameNode(fixture.ServerName). WithLatests(map[string]string{process.Name: fixture.ServerName}). WithCounters(map[string]int{report.Process: 1}). - WithChildren(report.MakeNodeSet( + WithChildren( RenderedEndpoints[fixture.Server80NodeID], RenderedProcesses[fixture.ServerProcessNodeID], - )), + ), fixture.NonContainerName: processNameNode(fixture.NonContainerName, render.OutgoingInternetID). - WithChildren(report.MakeNodeSet( + WithChildren( RenderedEndpoints[fixture.NonContainerNodeID], RenderedProcesses[fixture.NonContainerProcessNodeID], - )), + ), // due to https://github.com/weaveworks/scope/issues/1323 we are dropping // all non-internet pseudo nodes for now. @@ -153,10 +153,10 @@ var ( } uncontainedServerID = render.MakePseudoNodeID(render.UncontainedID, fixture.ServerHostID) - uncontainedServerNode = pseudo(uncontainedServerID, render.OutgoingInternetID).WithChildren(report.MakeNodeSet( + uncontainedServerNode = pseudo(uncontainedServerID, render.OutgoingInternetID).WithChildren( RenderedEndpoints[fixture.NonContainerNodeID], RenderedProcesses[fixture.NonContainerProcessNodeID], - )) + ) RenderedContainers = report.Nodes{ fixture.ClientContainerNodeID: container(fixture.ClientContainerNodeID, fixture.ServerContainerNodeID). @@ -166,18 +166,18 @@ var ( docker.ContainerName: fixture.ClientContainerName, docker.ImageName: fixture.ClientContainerImageName, }). - WithChildren(report.MakeNodeSet( + WithChildren( RenderedEndpoints[fixture.Client54001NodeID], RenderedEndpoints[fixture.Client54002NodeID], RenderedProcesses[fixture.ClientProcess1NodeID], RenderedProcesses[fixture.ClientProcess2NodeID], - )), + ), fixture.ServerContainerNodeID: container(fixture.ServerContainerNodeID). - WithChildren(report.MakeNodeSet( + WithChildren( RenderedEndpoints[fixture.Server80NodeID], RenderedProcesses[fixture.ServerProcessNodeID], - )), + ), uncontainedServerID: uncontainedServerNode, render.IncomingInternetID: theIncomingInternetNode(fixture.ServerContainerNodeID), @@ -192,23 +192,23 @@ var ( WithCounters(map[string]int{ report.Container: 1, }). - WithChildren(report.MakeNodeSet( + WithChildren( RenderedEndpoints[fixture.Client54001NodeID], RenderedEndpoints[fixture.Client54002NodeID], RenderedProcesses[fixture.ClientProcess1NodeID], RenderedProcesses[fixture.ClientProcess2NodeID], RenderedContainers[fixture.ClientContainerNodeID], - )), + ), fixture.ServerContainerHostname: containerHostnameNode(fixture.ServerContainerHostname). WithLatests(map[string]string{ docker.ContainerHostname: fixture.ServerContainerHostname, }). - WithChildren(report.MakeNodeSet( + WithChildren( RenderedEndpoints[fixture.Server80NodeID], RenderedProcesses[fixture.ServerProcessNodeID], RenderedContainers[fixture.ServerContainerNodeID], - )), + ), uncontainedServerID: uncontainedServerNode, render.IncomingInternetID: theIncomingInternetNode(fixture.ServerContainerHostname), @@ -228,20 +228,20 @@ var ( WithCounters(map[string]int{ report.Container: 1, }). - WithChildren(report.MakeNodeSet( + WithChildren( RenderedEndpoints[fixture.Client54001NodeID], RenderedEndpoints[fixture.Client54002NodeID], RenderedProcesses[fixture.ClientProcess1NodeID], RenderedProcesses[fixture.ClientProcess2NodeID], RenderedContainers[fixture.ClientContainerNodeID], - )), + ), ServerContainerImageNodeID: containerImage(ServerContainerImageNodeID). - WithChildren(report.MakeNodeSet( + WithChildren( RenderedEndpoints[fixture.Server80NodeID], RenderedProcesses[fixture.ServerProcessNodeID], RenderedContainers[fixture.ServerContainerNodeID], - )), + ), uncontainedServerID: uncontainedServerNode, render.IncomingInternetID: theIncomingInternetNode(ServerContainerImageNodeID), @@ -249,28 +249,28 @@ var ( } UnmanagedServerID = render.MakePseudoNodeID(render.UnmanagedID, fixture.ServerHostID) - unmanagedServerNode = pseudo(UnmanagedServerID, render.OutgoingInternetID).WithChildren(report.MakeNodeSet( + unmanagedServerNode = pseudo(UnmanagedServerID, render.OutgoingInternetID).WithChildren( uncontainedServerNode, RenderedEndpoints[fixture.NonContainerNodeID], RenderedProcesses[fixture.NonContainerProcessNodeID], - )) + ) RenderedPods = report.Nodes{ fixture.ClientPodNodeID: pod(fixture.ClientPodNodeID, fixture.ServerPodNodeID). - WithChildren(report.MakeNodeSet( + WithChildren( RenderedEndpoints[fixture.Client54001NodeID], RenderedEndpoints[fixture.Client54002NodeID], RenderedProcesses[fixture.ClientProcess1NodeID], RenderedProcesses[fixture.ClientProcess2NodeID], RenderedContainers[fixture.ClientContainerNodeID], - )), + ), fixture.ServerPodNodeID: pod(fixture.ServerPodNodeID). - WithChildren(report.MakeNodeSet( + WithChildren( RenderedEndpoints[fixture.Server80NodeID], RenderedProcesses[fixture.ServerProcessNodeID], RenderedContainers[fixture.ServerContainerNodeID], - )), + ), fixture.PersistentVolumeClaimNodeID: persistentVolumeClaim(fixture.PersistentVolumeClaimNodeID, fixture.PersistentVolumeNodeID). WithLatests(map[string]string{ @@ -280,7 +280,7 @@ var ( kubernetes.VolumeName: "pongvolume", kubernetes.AccessModes: "ReadWriteOnce", kubernetes.StorageClassName: "standard", - }).WithChild(report.MakeNode(fixture.PersistentVolumeNodeID).WithTopology(report.PersistentVolume)), + }).WithChildren(report.MakeNode(fixture.PersistentVolumeNodeID).WithTopology(report.PersistentVolume)), fixture.PersistentVolumeNodeID: persistentVolume(fixture.PersistentVolumeNodeID). WithLatests(map[string]string{ @@ -296,7 +296,7 @@ var ( WithLatests(map[string]string{ kubernetes.Name: "standard", kubernetes.Provisioner: "pong", - }).WithChild(report.MakeNode(fixture.PersistentVolumeClaimNodeID).WithTopology(report.PersistentVolumeClaim)), + }).WithChildren(report.MakeNode(fixture.PersistentVolumeClaimNodeID).WithTopology(report.PersistentVolumeClaim)), UnmanagedServerID: unmanagedServerNode, render.IncomingInternetID: theIncomingInternetNode(fixture.ServerPodNodeID), @@ -305,7 +305,7 @@ var ( RenderedPodServices = report.Nodes{ fixture.ServiceNodeID: service(fixture.ServiceNodeID, fixture.ServiceNodeID). - WithChildren(report.MakeNodeSet( + WithChildren( RenderedEndpoints[fixture.Client54001NodeID], RenderedEndpoints[fixture.Client54002NodeID], RenderedEndpoints[fixture.Server80NodeID], @@ -316,7 +316,7 @@ var ( RenderedContainers[fixture.ServerContainerNodeID], RenderedPods[fixture.ClientPodNodeID], RenderedPods[fixture.ServerPodNodeID], - )), + ), UnmanagedServerID: unmanagedServerNode, render.IncomingInternetID: theIncomingInternetNode(fixture.ServiceNodeID), @@ -328,7 +328,7 @@ var ( WithLatests(map[string]string{ host.HostName: fixture.ClientHostName, }). - WithChildren(report.MakeNodeSet( + WithChildren( RenderedEndpoints[fixture.Client54001NodeID], RenderedEndpoints[fixture.Client54002NodeID], RenderedProcesses[fixture.ClientProcess1NodeID], @@ -336,10 +336,10 @@ var ( RenderedContainers[fixture.ClientContainerNodeID], RenderedContainerImages[ClientContainerImageNodeID], RenderedPods[fixture.ClientPodNodeID], - )), + ), fixture.ServerHostNodeID: hostNode(fixture.ServerHostNodeID, render.OutgoingInternetID). - WithChildren(report.MakeNodeSet( + WithChildren( RenderedEndpoints[fixture.Server80NodeID], RenderedEndpoints[fixture.NonContainerNodeID], RenderedProcesses[fixture.ServerProcessNodeID], @@ -347,7 +347,7 @@ var ( RenderedContainers[fixture.ServerContainerNodeID], RenderedContainerImages[ServerContainerImageNodeID], RenderedPods[fixture.ServerPodNodeID], - )), + ), // due to https://github.com/weaveworks/scope/issues/1323 we are dropping // all non-internet pseudo nodes for now. diff --git a/render/metrics_test.go b/render/metrics_test.go index f604d2fab..304a4fdae 100644 --- a/render/metrics_test.go +++ b/render/metrics_test.go @@ -27,33 +27,44 @@ func TestPropagateSingleMetrics(t *testing.T) { { name: "one child", input: report.MakeNode("a").WithChildren( - report.MakeNodeSet( - report.MakeNode("child1"). - WithTopology(report.Container). - WithMetrics(report.Metrics{ - "metric1": report.MakeMetric(nil), - }), - ), + report.MakeNode("child1"). + WithTopology(report.Container). + WithMetrics(report.Metrics{ + "metric1": report.MakeMetric(nil), + }), ), topology: report.Container, output: report.Nodes{ "a": report.MakeNode("a").WithMetrics(report.Metrics{ "metric1": report.MakeMetric(nil), }).WithChildren( - report.MakeNodeSet( - report.MakeNode("child1"). - WithTopology(report.Container). - WithMetrics(report.Metrics{ - "metric1": report.MakeMetric(nil), - }), - ), + report.MakeNode("child1"). + WithTopology(report.Container). + WithMetrics(report.Metrics{ + "metric1": report.MakeMetric(nil), + }), ), }, }, { name: "ignores other topologies", input: report.MakeNode("a").WithChildren( - report.MakeNodeSet( + report.MakeNode("child1"). + WithTopology(report.Container). + WithMetrics(report.Metrics{ + "metric1": report.MakeMetric(nil), + }), + report.MakeNode("child2"). + WithTopology("otherTopology"). + WithMetrics(report.Metrics{ + "metric2": report.MakeMetric(nil), + }), + ), + topology: report.Container, + output: report.Nodes{ + "a": report.MakeNode("a").WithMetrics(report.Metrics{ + "metric1": report.MakeMetric(nil), + }).WithChildren( report.MakeNode("child1"). WithTopology(report.Container). WithMetrics(report.Metrics{ @@ -65,31 +76,25 @@ func TestPropagateSingleMetrics(t *testing.T) { "metric2": report.MakeMetric(nil), }), ), - ), - topology: report.Container, - output: report.Nodes{ - "a": report.MakeNode("a").WithMetrics(report.Metrics{ - "metric1": report.MakeMetric(nil), - }).WithChildren( - report.MakeNodeSet( - report.MakeNode("child1"). - WithTopology(report.Container). - WithMetrics(report.Metrics{ - "metric1": report.MakeMetric(nil), - }), - report.MakeNode("child2"). - WithTopology("otherTopology"). - WithMetrics(report.Metrics{ - "metric2": report.MakeMetric(nil), - }), - ), - ), }, }, { name: "two children", input: report.MakeNode("a").WithChildren( - report.MakeNodeSet( + report.MakeNode("child1"). + WithTopology(report.Container). + WithMetrics(report.Metrics{ + "metric1": report.MakeMetric(nil), + }), + report.MakeNode("child2"). + WithTopology(report.Container). + WithMetrics(report.Metrics{ + "metric2": report.MakeMetric(nil), + }), + ), + topology: report.Container, + output: report.Nodes{ + "a": report.MakeNode("a").WithChildren( report.MakeNode("child1"). WithTopology(report.Container). WithMetrics(report.Metrics{ @@ -101,29 +106,28 @@ func TestPropagateSingleMetrics(t *testing.T) { "metric2": report.MakeMetric(nil), }), ), - ), - topology: report.Container, - output: report.Nodes{ - "a": report.MakeNode("a").WithChildren( - report.MakeNodeSet( - report.MakeNode("child1"). - WithTopology(report.Container). - WithMetrics(report.Metrics{ - "metric1": report.MakeMetric(nil), - }), - report.MakeNode("child2"). - WithTopology(report.Container). - WithMetrics(report.Metrics{ - "metric2": report.MakeMetric(nil), - }), - ), - ), }, }, { name: "ignores k8s pause container", input: report.MakeNode("a").WithChildren( - report.MakeNodeSet( + report.MakeNode("child1"). + WithTopology(report.Container). + WithMetrics(report.Metrics{ + "metric1": report.MakeMetric(nil), + }), + report.MakeNode("child2"). + WithLatest(report.DoesNotMakeConnections, now, ""). + WithTopology(report.Container). + WithMetrics(report.Metrics{ + "metric2": report.MakeMetric(nil), + }), + ), + topology: report.Container, + output: report.Nodes{ + "a": report.MakeNode("a").WithMetrics(report.Metrics{ + "metric1": report.MakeMetric(nil), + }).WithChildren( report.MakeNode("child1"). WithTopology(report.Container). WithMetrics(report.Metrics{ @@ -136,26 +140,6 @@ func TestPropagateSingleMetrics(t *testing.T) { "metric2": report.MakeMetric(nil), }), ), - ), - topology: report.Container, - output: report.Nodes{ - "a": report.MakeNode("a").WithMetrics(report.Metrics{ - "metric1": report.MakeMetric(nil), - }).WithChildren( - report.MakeNodeSet( - report.MakeNode("child1"). - WithTopology(report.Container). - WithMetrics(report.Metrics{ - "metric1": report.MakeMetric(nil), - }), - report.MakeNode("child2"). - WithLatest(report.DoesNotMakeConnections, now, ""). - WithTopology(report.Container). - WithMetrics(report.Metrics{ - "metric2": report.MakeMetric(nil), - }), - ), - ), }, }, } { diff --git a/report/node.go b/report/node.go index 3a11b6027..7d8df55c1 100644 --- a/report/node.go +++ b/report/node.go @@ -169,14 +169,8 @@ func (n Node) PruneParents() Node { } // WithChildren returns a fresh copy of n, with children merged in. -func (n Node) WithChildren(children NodeSet) Node { - n.Children = n.Children.Merge(children) - return n -} - -// WithChild returns a fresh copy of n, with one child merged in. -func (n Node) WithChild(child Node) Node { - n.Children = n.Children.Merge(MakeNodeSet(child)) +func (n Node) WithChildren(children ...Node) Node { + n.Children = n.Children.Add(children...) return n } diff --git a/report/node_set.go b/report/node_set.go index 376e0b489..fdb3a8e5e 100644 --- a/report/node_set.go +++ b/report/node_set.go @@ -5,7 +5,6 @@ import ( "fmt" "github.com/davecgh/go-spew/spew" - "github.com/ugorji/go/codec" "github.com/weaveworks/ps" "github.com/weaveworks/scope/test/reflect" @@ -146,43 +145,3 @@ func (n NodeSet) String() string { func (n NodeSet) DeepEqual(o NodeSet) bool { return mapEqual(n.psMap, o.psMap, reflect.DeepEqual) } - -func (n NodeSet) toIntermediate() []Node { - intermediate := make([]Node, 0, n.Size()) - n.ForEach(func(node Node) { - intermediate = append(intermediate, node) - }) - return intermediate -} - -func (n NodeSet) fromIntermediate(nodes []Node) NodeSet { - return MakeNodeSet(nodes...) -} - -// CodecEncodeSelf implements codec.Selfer -func (n *NodeSet) CodecEncodeSelf(encoder *codec.Encoder) { - if n.psMap != nil { - encoder.Encode(n.toIntermediate()) - } else { - encoder.Encode(nil) - } -} - -// CodecDecodeSelf implements codec.Selfer -func (n *NodeSet) CodecDecodeSelf(decoder *codec.Decoder) { - in := []Node{} - if err := decoder.Decode(&in); err != nil { - return - } - *n = NodeSet{}.fromIntermediate(in) -} - -// MarshalJSON shouldn't be used, use CodecEncodeSelf instead -func (NodeSet) MarshalJSON() ([]byte, error) { - panic("MarshalJSON shouldn't be used, use CodecEncodeSelf instead") -} - -// UnmarshalJSON shouldn't be used, use CodecDecodeSelf instead -func (*NodeSet) UnmarshalJSON(b []byte) error { - panic("UnmarshalJSON shouldn't be used, use CodecDecodeSelf instead") -} diff --git a/test/utils/prune.go b/test/utils/prune.go index 42a8aa268..7479aac38 100644 --- a/test/utils/prune.go +++ b/test/utils/prune.go @@ -18,13 +18,13 @@ func Prune(nodes report.Nodes) report.Nodes { // necessary for rendering nodes and edges stripped away. Specifically, that // means cutting out parts of the Node. func PruneNode(node report.Node) report.Node { - prunedChildren := report.MakeNodeSet() + prunedChildren := []report.Node{} node.Children.ForEach(func(child report.Node) { - prunedChildren = prunedChildren.Add(PruneNode(child)) + prunedChildren = append(prunedChildren, PruneNode(child)) }) return report.MakeNode( node.ID). WithTopology(node.Topology). WithAdjacent(node.Adjacency...). - WithChildren(prunedChildren) + WithChildren(prunedChildren...) }