From 583e81d733e030d8f0968154bcce8bdf1a6f69a3 Mon Sep 17 00:00:00 2001 From: Matthias Radestock Date: Sun, 24 Dec 2017 19:11:24 +0000 Subject: [PATCH 1/4] make Report.Toplogy(name) fast Previously this was buidling a fresh map of all topologies, just so it could look up the one given as an argument. Node summarisation (via detailed.Summaries) in particular was badly affected by that. --- report/report.go | 35 +++++++++++++++++++++++++++++++++-- 1 file changed, 33 insertions(+), 2 deletions(-) diff --git a/report/report.go b/report/report.go index e25f35b69..c314e34a4 100644 --- a/report/report.go +++ b/report/report.go @@ -314,8 +314,39 @@ func (r *Report) WalkPairedTopologies(o *Report, f func(*Topology, *Topology)) { // Topology gets a topology by name func (r Report) Topology(name string) (Topology, bool) { - if t, ok := r.TopologyMap()[name]; ok { - return *t, true + switch name { + case Endpoint: + return r.Endpoint, true + case Process: + return r.Process, true + case Container: + return r.Container, true + case ContainerImage: + return r.ContainerImage, true + case Pod: + return r.Pod, true + case Service: + return r.Service, true + case Deployment: + return r.Deployment, true + case ReplicaSet: + return r.ReplicaSet, true + case DaemonSet: + return r.DaemonSet, true + case StatefulSet: + return r.StatefulSet, true + case CronJob: + return r.CronJob, true + case Host: + return r.Host, true + case Overlay: + return r.Overlay, true + case ECSTask: + return r.ECSTask, true + case ECSService: + return r.ECSService, true + case SwarmService: + return r.SwarmService, true } return Topology{}, false } From 9419c3ef5cdbf4e8a75aecc2f8a2eb83692be07a Mon Sep 17 00:00:00 2001 From: Matthias Radestock Date: Sun, 24 Dec 2017 20:20:59 +0000 Subject: [PATCH 2/4] refactor(ish): reduce number of topology lists Having 6 lists of topolgies in the same file is a bit much: 1. consts for topology names 2. Report type definition 3. MakeReport() Report initialisation 4. Report.Topology(name) lookup 5. Report.TopologyMap() mapping of names to topology references 6. Report.WalkPairedTopologies() iterator over topology references We get rid of 5 and 6 by introducing a topologyNames slice. So we are down to 5. We replace Report.TopologyMap() with a new function, WalkNamedTopologies, that uses topologyNames. WalkPairedTopologies() is updated to operate in a similar fashion. Likewise for WalkTopologies() and Topologies() - these were previously calling Walk[Paired]Topologies, but it is clearer to simply implement them directly. --- probe/topology_tagger.go | 4 +- report/report.go | 128 ++++++++++++++++++++------------------- 2 files changed, 68 insertions(+), 64 deletions(-) diff --git a/probe/topology_tagger.go b/probe/topology_tagger.go index ce37e19a3..49dfc3bb3 100644 --- a/probe/topology_tagger.go +++ b/probe/topology_tagger.go @@ -16,10 +16,10 @@ func (topologyTagger) Name() string { return "Topology" } // Tag implements Tagger func (topologyTagger) Tag(r report.Report) (report.Report, error) { - for name, t := range r.TopologyMap() { + r.WalkNamedTopologies(func(name string, t *report.Topology) { for _, node := range t.Nodes { t.AddNode(node.WithTopology(name)) } - } + }) return r, nil } diff --git a/report/report.go b/report/report.go index c314e34a4..71a67456d 100644 --- a/report/report.go +++ b/report/report.go @@ -43,6 +43,26 @@ const ( ContainersKey = "containers" ) +// topologyNames are the names of all report topologies. +var topologyNames = []string{ + Endpoint, + Process, + Container, + ContainerImage, + Pod, + Service, + Deployment, + ReplicaSet, + DaemonSet, + StatefulSet, + CronJob, + Host, + Overlay, + ECSTask, + ECSService, + SwarmService, +} + // Report is the core data type. It's produced by probes, and consumed and // stored by apps. It's composed of multiple topologies, each representing // a different (related, but not equivalent) view of the network. @@ -226,28 +246,6 @@ func MakeReport() Report { } } -// TopologyMap gets a map from topology names to pointers to the respective topologies -func (r *Report) TopologyMap() map[string]*Topology { - return map[string]*Topology{ - Endpoint: &r.Endpoint, - Process: &r.Process, - Container: &r.Container, - ContainerImage: &r.ContainerImage, - Pod: &r.Pod, - Service: &r.Service, - Deployment: &r.Deployment, - ReplicaSet: &r.ReplicaSet, - DaemonSet: &r.DaemonSet, - StatefulSet: &r.StatefulSet, - CronJob: &r.CronJob, - Host: &r.Host, - Overlay: &r.Overlay, - ECSTask: &r.ECSTask, - ECSService: &r.ECSService, - SwarmService: &r.SwarmService, - } -} - // Copy returns a value copy of the report. func (r Report) Copy() Report { newReport := Report{ @@ -277,76 +275,82 @@ func (r Report) Merge(other Report) Report { // Topologies returns a slice of Topologies in this report func (r Report) Topologies() []Topology { - result := []Topology{} - r.WalkTopologies(func(t *Topology) { - result = append(result, *t) - }) + result := make([]Topology, len(topologyNames)) + for i, name := range topologyNames { + t, _ := r.Topology(name) + result[i] = t + } return result } // WalkTopologies iterates through the Topologies of the report, // potentially modifying them func (r *Report) WalkTopologies(f func(*Topology)) { - var dummy Report - r.WalkPairedTopologies(&dummy, func(t, _ *Topology) { f(t) }) + for _, name := range topologyNames { + f(r.topology(name)) + } +} + +// WalkNamedTopologies iterates through the Topologies of the report, +// potentially modifying them. +func (r *Report) WalkNamedTopologies(f func(string, *Topology)) { + for _, name := range topologyNames { + f(name, r.topology(name)) + } } // WalkPairedTopologies iterates through the Topologies of this and another report, // potentially modifying one or both. func (r *Report) WalkPairedTopologies(o *Report, f func(*Topology, *Topology)) { - f(&r.Endpoint, &o.Endpoint) - f(&r.Process, &o.Process) - f(&r.Container, &o.Container) - f(&r.ContainerImage, &o.ContainerImage) - f(&r.Pod, &o.Pod) - f(&r.Service, &o.Service) - f(&r.Deployment, &o.Deployment) - f(&r.ReplicaSet, &o.ReplicaSet) - f(&r.DaemonSet, &o.DaemonSet) - f(&r.StatefulSet, &o.StatefulSet) - f(&r.CronJob, &o.CronJob) - f(&r.Host, &o.Host) - f(&r.Overlay, &o.Overlay) - f(&r.ECSTask, &o.ECSTask) - f(&r.ECSService, &o.ECSService) - f(&r.SwarmService, &o.SwarmService) + for _, name := range topologyNames { + f(r.topology(name), o.topology(name)) + } } -// Topology gets a topology by name -func (r Report) Topology(name string) (Topology, bool) { +// topology returns a reference to one of the report's topologies, +// selected by name. +func (r *Report) topology(name string) *Topology { switch name { case Endpoint: - return r.Endpoint, true + return &r.Endpoint case Process: - return r.Process, true + return &r.Process case Container: - return r.Container, true + return &r.Container case ContainerImage: - return r.ContainerImage, true + return &r.ContainerImage case Pod: - return r.Pod, true + return &r.Pod case Service: - return r.Service, true + return &r.Service case Deployment: - return r.Deployment, true + return &r.Deployment case ReplicaSet: - return r.ReplicaSet, true + return &r.ReplicaSet case DaemonSet: - return r.DaemonSet, true + return &r.DaemonSet case StatefulSet: - return r.StatefulSet, true + return &r.StatefulSet case CronJob: - return r.CronJob, true + return &r.CronJob case Host: - return r.Host, true + return &r.Host case Overlay: - return r.Overlay, true + return &r.Overlay case ECSTask: - return r.ECSTask, true + return &r.ECSTask case ECSService: - return r.ECSService, true + return &r.ECSService case SwarmService: - return r.SwarmService, true + return &r.SwarmService + } + return nil +} + +// Topology returns one of the report's topologies, selected by name. +func (r Report) Topology(name string) (Topology, bool) { + if t := r.topology(name); t != nil { + return *t, true } return Topology{}, false } From e2eef50cda29b0871135c947918be2003595aaf6 Mon Sep 17 00:00:00 2001 From: Matthias Radestock Date: Sun, 24 Dec 2017 21:32:15 +0000 Subject: [PATCH 3/4] eliminate (out-of-date) list of topologies in plugin code --- probe/plugins/registry.go | 23 ++--------------------- 1 file changed, 2 insertions(+), 21 deletions(-) diff --git a/probe/plugins/registry.go b/probe/plugins/registry.go index 11d9d1db1..9b46f88e8 100644 --- a/probe/plugins/registry.go +++ b/probe/plugins/registry.go @@ -231,32 +231,13 @@ func (r *Registry) updateAndRegisterControlsInReport(rpt *report.Report) { key := rpt.Plugins.Keys()[0] spec, _ := rpt.Plugins.Lookup(key) pluginID := spec.ID - topologies := topologyPointers(rpt) var newPluginControls []string - for _, topology := range topologies { + rpt.WalkTopologies(func(topology *report.Topology) { newPluginControls = append(newPluginControls, r.updateAndGetControlsInTopology(pluginID, topology)...) - } + }) r.updatePluginControls(pluginID, report.MakeStringSet(newPluginControls...)) } -func topologyPointers(rpt *report.Report) []*report.Topology { - // We cannot use rpt.Topologies(), because it makes a slice of - // topology copies and we need original locations to modify - // them. - return []*report.Topology{ - &rpt.Endpoint, - &rpt.Process, - &rpt.Container, - &rpt.ContainerImage, - &rpt.Pod, - &rpt.Service, - &rpt.Deployment, - &rpt.ReplicaSet, - &rpt.Host, - &rpt.Overlay, - } -} - func (r *Registry) updateAndGetControlsInTopology(pluginID string, topology *report.Topology) []string { var pluginControls []string newControls := report.Controls{} From dcda88471a8ac3e773e3c1f047faec85faed790e Mon Sep 17 00:00:00 2001 From: Matthias Radestock Date: Sun, 24 Dec 2017 22:18:38 +0000 Subject: [PATCH 4/4] get rid of report.Topologies() The three report.Walk* functions are quite enough. --- report/report.go | 14 ++------------ report/report_test.go | 8 ++++++-- 2 files changed, 8 insertions(+), 14 deletions(-) diff --git a/report/report.go b/report/report.go index 71a67456d..19bf9e780 100644 --- a/report/report.go +++ b/report/report.go @@ -273,16 +273,6 @@ func (r Report) Merge(other Report) Report { return newReport } -// Topologies returns a slice of Topologies in this report -func (r Report) Topologies() []Topology { - result := make([]Topology, len(topologyNames)) - for i, name := range topologyNames { - t, _ := r.Topology(name) - result[i] = t - } - return result -} - // WalkTopologies iterates through the Topologies of the report, // potentially modifying them func (r *Report) WalkTopologies(f func(*Topology)) { @@ -358,8 +348,8 @@ func (r Report) Topology(name string) (Topology, bool) { // Validate checks the report for various inconsistencies. func (r Report) Validate() error { var errs []string - for _, topology := range r.Topologies() { - if err := topology.Validate(); err != nil { + for _, name := range topologyNames { + if err := r.topology(name).Validate(); err != nil { errs = append(errs, err.Error()) } } diff --git a/report/report_test.go b/report/report_test.go index bf5dc2a36..f65576e79 100644 --- a/report/report_test.go +++ b/report/report_test.go @@ -20,14 +20,18 @@ func TestReportTopologies(t *testing.T) { topologyType = reflect.TypeOf(report.MakeTopology()) ) - var want int + var want, have int for i := 0; i < reportType.NumField(); i++ { if reportType.Field(i).Type == topologyType { want++ } } - if have := len(report.MakeReport().Topologies()); want != have { + r := report.MakeReport() + r.WalkTopologies(func(_ *report.Topology) { + have++ + }) + if want != have { t.Errorf("want %d, have %d", want, have) } }