From 92ec7d939762e83d23446166bb554382825743e8 Mon Sep 17 00:00:00 2001 From: Paul Bellamy Date: Mon, 26 Oct 2015 10:41:51 +0000 Subject: [PATCH 1/2] Move probe main.go to prog/probe/, break out a probe struct with appropriate responsibilities. Also adds test for probe 'engine' --- .gitignore | 1 + Makefile | 2 +- circle.yml | 4 +- probe/hostname.go | 5 +- probe/instrumentation.go | 27 --- probe/probe.go | 162 ++++++++++++++++++ probe/probe_internal_test.go | 86 ++++++++++ probe/sync_report.go | 26 +++ probe/tag_report_test.go | 45 ----- probe/{tag_report.go => topology_tagger.go} | 35 +--- probe/topology_tagger_internal_test.go | 17 ++ {probe => prog/probe}/main.go | 155 +++-------------- {probe => xfer}/resolver.go | 15 +- .../resolver_internal_test.go | 14 +- 14 files changed, 337 insertions(+), 257 deletions(-) delete mode 100644 probe/instrumentation.go create mode 100644 probe/probe.go create mode 100644 probe/probe_internal_test.go create mode 100644 probe/sync_report.go delete mode 100644 probe/tag_report_test.go rename probe/{tag_report.go => topology_tagger.go} (53%) create mode 100644 probe/topology_tagger_internal_test.go rename {probe => prog/probe}/main.go (59%) rename {probe => xfer}/resolver.go (88%) rename probe/resolver_test.go => xfer/resolver_internal_test.go (87%) diff --git a/.gitignore b/.gitignore index 344ce9365..510fc51ee 100644 --- a/.gitignore +++ b/.gitignore @@ -38,6 +38,7 @@ app/app app/scope-app probe/probe probe/scope-probe +prog/probe/scope-probe docker/scope-app docker/scope-probe docker/docker* diff --git a/Makefile b/Makefile index fff6d3386..972e76f09 100644 --- a/Makefile +++ b/Makefile @@ -4,7 +4,7 @@ SUDO=sudo -E DOCKERHUB_USER=weaveworks APP_EXE=app/scope-app -PROBE_EXE=probe/scope-probe +PROBE_EXE=prog/probe/scope-probe FIXPROBE_EXE=experimental/fixprobe/fixprobe SCOPE_IMAGE=$(DOCKERHUB_USER)/scope SCOPE_EXPORT=scope.tar diff --git a/circle.yml b/circle.yml index ddd032480..f5839419d 100644 --- a/circle.yml +++ b/circle.yml @@ -45,9 +45,9 @@ test: parallel: true - cd $SRCDIR; make RM= static: parallel: true - - cd $SRCDIR; rm -f app/scope-app probe/scope-probe; if [ "$CIRCLE_NODE_INDEX" = "0" ]; then GOARCH=arm make RM= app/scope-app probe/scope-probe; else GOOS=darwin make RM= app/scope-app probe/scope-probe; fi: + - cd $SRCDIR; rm -f app/scope-app prog/probe/scope-probe; if [ "$CIRCLE_NODE_INDEX" = "0" ]; then GOARCH=arm make RM= app/scope-app prog/probe/scope-probe; else GOOS=darwin make RM= app/scope-app prog/probe/scope-probe; fi: parallel: true - - cd $SRCDIR; rm -f app/scope-app probe/scope-probe; make RM=: + - cd $SRCDIR; rm -f app/scope-app prog/probe/scope-probe; make RM=: parallel: true - cd $SRCDIR/experimental; ./build_on_circle.sh: parallel: true diff --git a/probe/hostname.go b/probe/hostname.go index 1bf88e2f7..305e6beea 100644 --- a/probe/hostname.go +++ b/probe/hostname.go @@ -1,8 +1,9 @@ -package main +package probe import "os" -func hostname() string { +// Hostname returns the hostname of this host. +func Hostname() string { if hostname := os.Getenv("SCOPE_HOSTNAME"); hostname != "" { return hostname } diff --git a/probe/instrumentation.go b/probe/instrumentation.go deleted file mode 100644 index 266f2a65f..000000000 --- a/probe/instrumentation.go +++ /dev/null @@ -1,27 +0,0 @@ -package main - -import ( - "net/http" - - "github.com/prometheus/client_golang/prometheus" - - "github.com/weaveworks/scope/probe/endpoint" -) - -var ( - publishTicks = prometheus.NewCounterVec( - prometheus.CounterOpts{ - Namespace: "scope", - Subsystem: "probe", - Name: "publish_ticks", - Help: "Number of publish ticks observed.", - }, - []string{}, - ) -) - -func makePrometheusHandler() http.Handler { - prometheus.MustRegister(publishTicks) - prometheus.MustRegister(endpoint.SpyDuration) - return prometheus.Handler() -} diff --git a/probe/probe.go b/probe/probe.go new file mode 100644 index 000000000..d429e7cdf --- /dev/null +++ b/probe/probe.go @@ -0,0 +1,162 @@ +package probe + +import ( + "log" + "sync" + "time" + + "github.com/weaveworks/scope/report" + "github.com/weaveworks/scope/xfer" +) + +// Probe sits there, generating and publishing reports. +type Probe struct { + spyInterval, publishInterval time.Duration + publisher xfer.Publisher + + tickers []Ticker + reporters []Reporter + taggers []Tagger + + quit chan struct{} + done sync.WaitGroup + rpt syncReport +} + +// Tagger tags nodes with value-add node metadata. +type Tagger interface { + Tag(r report.Report) (report.Report, error) +} + +// Reporter generates Reports. +type Reporter interface { + Report() (report.Report, error) +} + +// Ticker is something which will be invoked every spyDuration. +// It's useful for things that should be updated on that interval. +// For example, cached shared state between Taggers and Reporters. +type Ticker interface { + Tick() error +} + +// New makes a new Probe. +func New(spyInterval, publishInterval time.Duration, publisher xfer.Publisher) *Probe { + result := &Probe{ + spyInterval: spyInterval, + publishInterval: publishInterval, + publisher: publisher, + quit: make(chan struct{}), + } + result.rpt.swap(report.MakeReport()) + return result +} + +// AddTagger adds a new Tagger to the Probe +func (p *Probe) AddTagger(ts ...Tagger) { + p.taggers = append(p.taggers, ts...) +} + +// AddReporter adds a new Reported to the Probe +func (p *Probe) AddReporter(rs ...Reporter) { + p.reporters = append(p.reporters, rs...) +} + +// AddTicker adds a new Ticker to the Probe +func (p *Probe) AddTicker(ts ...Ticker) { + p.tickers = append(p.tickers, ts...) +} + +// Start starts the probe +func (p *Probe) Start() { + p.done.Add(2) + go p.spyLoop() + go p.publishLoop() +} + +// Stop stops the probe +func (p *Probe) Stop() { + close(p.quit) + p.done.Wait() +} + +func (p *Probe) spyLoop() { + defer p.done.Done() + spyTick := time.Tick(p.spyInterval) + + for { + select { + case <-spyTick: + start := time.Now() + for _, ticker := range p.tickers { + if err := ticker.Tick(); err != nil { + log.Printf("error doing ticker: %v", err) + } + } + + localReport := p.rpt.copy() + localReport = localReport.Merge(p.report()) + localReport = p.tag(localReport) + p.rpt.swap(localReport) + + if took := time.Since(start); took > p.spyInterval { + log.Printf("report generation took too long (%s)", took) + } + + case <-p.quit: + return + } + } +} + +func (p *Probe) report() report.Report { + reports := make(chan report.Report, len(p.reporters)) + for _, rep := range p.reporters { + go func(rep Reporter) { + newReport, err := rep.Report() + if err != nil { + log.Printf("error generating report: %v", err) + newReport = report.MakeReport() // empty is OK to merge + } + reports <- newReport + }(rep) + } + + result := report.MakeReport() + for i := 0; i < cap(reports); i++ { + result = result.Merge(<-reports) + } + return result +} + +func (p *Probe) tag(r report.Report) report.Report { + var err error + for _, tagger := range p.taggers { + r, err = tagger.Tag(r) + if err != nil { + log.Printf("error applying tagger: %v", err) + } + } + return r +} + +func (p *Probe) publishLoop() { + defer p.done.Done() + var ( + pubTick = time.Tick(p.publishInterval) + rptPub = xfer.NewReportPublisher(p.publisher) + ) + + for { + select { + case <-pubTick: + localReport := p.rpt.swap(report.MakeReport()) + if err := rptPub.Publish(localReport); err != nil { + log.Printf("publish: %v", err) + } + + case <-p.quit: + return + } + } +} diff --git a/probe/probe_internal_test.go b/probe/probe_internal_test.go new file mode 100644 index 000000000..6aa1f4869 --- /dev/null +++ b/probe/probe_internal_test.go @@ -0,0 +1,86 @@ +package probe + +import ( + "compress/gzip" + "encoding/gob" + "io" + "reflect" + "testing" + "time" + + "github.com/weaveworks/scope/report" + "github.com/weaveworks/scope/test" +) + +func TestApply(t *testing.T) { + var ( + endpointNodeID = "c" + addressNodeID = "d" + endpointNode = report.MakeNodeWith(map[string]string{"5": "6"}) + addressNode = report.MakeNodeWith(map[string]string{"7": "8"}) + ) + + p := New(0, 0, nil) + p.AddTagger(NewTopologyTagger()) + + r := report.MakeReport() + r.Endpoint.AddNode(endpointNodeID, endpointNode) + r.Address.AddNode(addressNodeID, addressNode) + r = p.tag(r) + + for _, tuple := range []struct { + want report.Node + from report.Topology + via string + }{ + {endpointNode.Merge(report.MakeNodeWith(map[string]string{"topology": "endpoint"})), r.Endpoint, endpointNodeID}, + {addressNode.Merge(report.MakeNodeWith(map[string]string{"topology": "address"})), r.Address, addressNodeID}, + } { + if want, have := tuple.want, tuple.from.Nodes[tuple.via]; !reflect.DeepEqual(want, have) { + t.Errorf("want %+v, have %+v", want, have) + } + } +} + +type mockReporter struct { + r report.Report +} + +func (m mockReporter) Report() (report.Report, error) { + return m.r.Copy(), nil +} + +type mockPublisher struct { + have chan report.Report +} + +func (m mockPublisher) Publish(in io.Reader) error { + var r report.Report + if reader, err := gzip.NewReader(in); err != nil { + return err + } else if err := gob.NewDecoder(reader).Decode(&r); err != nil { + return err + } + m.have <- r + return nil +} + +func (m mockPublisher) Stop() { + close(m.have) +} + +func TestProbe(t *testing.T) { + want := report.MakeReport() + node := report.MakeNodeWith(map[string]string{"b": "c"}) + want.Endpoint.AddNode("a", node) + pub := mockPublisher{make(chan report.Report)} + + p := New(10*time.Millisecond, 100*time.Millisecond, pub) + p.AddReporter(mockReporter{want}) + p.Start() + defer p.Stop() + + test.Poll(t, 300*time.Millisecond, want, func() interface{} { + return <-pub.have + }) +} diff --git a/probe/sync_report.go b/probe/sync_report.go new file mode 100644 index 000000000..830214920 --- /dev/null +++ b/probe/sync_report.go @@ -0,0 +1,26 @@ +package probe + +import ( + "sync" + + "github.com/weaveworks/scope/report" +) + +type syncReport struct { + mtx sync.RWMutex + rpt report.Report +} + +func (r *syncReport) swap(other report.Report) report.Report { + r.mtx.Lock() + defer r.mtx.Unlock() + old := r.rpt + r.rpt = other + return old +} + +func (r *syncReport) copy() report.Report { + r.mtx.RLock() + defer r.mtx.RUnlock() + return r.rpt.Copy() +} diff --git a/probe/tag_report_test.go b/probe/tag_report_test.go deleted file mode 100644 index c94ae0c27..000000000 --- a/probe/tag_report_test.go +++ /dev/null @@ -1,45 +0,0 @@ -package main - -import ( - "reflect" - "testing" - - "github.com/weaveworks/scope/report" -) - -func TestApply(t *testing.T) { - var ( - endpointNodeID = "c" - addressNodeID = "d" - endpointNode = report.MakeNodeWith(map[string]string{"5": "6"}) - addressNode = report.MakeNodeWith(map[string]string{"7": "8"}) - ) - - r := report.MakeReport() - r.Endpoint.AddNode(endpointNodeID, endpointNode) - r.Address.AddNode(addressNodeID, addressNode) - r = Apply(r, []Tagger{newTopologyTagger()}) - - for _, tuple := range []struct { - want report.Node - from report.Topology - via string - }{ - {endpointNode.Merge(report.MakeNodeWith(map[string]string{"topology": "endpoint"})), r.Endpoint, endpointNodeID}, - {addressNode.Merge(report.MakeNodeWith(map[string]string{"topology": "address"})), r.Address, addressNodeID}, - } { - if want, have := tuple.want, tuple.from.Nodes[tuple.via]; !reflect.DeepEqual(want, have) { - t.Errorf("want %+v, have %+v", want, have) - } - } -} - -func TestTagMissingID(t *testing.T) { - const nodeID = "not-found" - r := report.MakeReport() - rpt, _ := newTopologyTagger().Tag(r) - _, ok := rpt.Endpoint.Nodes[nodeID] - if ok { - t.Error("TopologyTagger erroneously tagged a missing node ID") - } -} diff --git a/probe/tag_report.go b/probe/topology_tagger.go similarity index 53% rename from probe/tag_report.go rename to probe/topology_tagger.go index 310e15f94..49e825b7e 100644 --- a/probe/tag_report.go +++ b/probe/topology_tagger.go @@ -1,40 +1,9 @@ -package main +package probe import ( - "log" - "github.com/weaveworks/scope/report" ) -// Tagger tags nodes with value-add node metadata. -type Tagger interface { - Tag(r report.Report) (report.Report, error) -} - -// Reporter generates Reports. -type Reporter interface { - Report() (report.Report, error) -} - -// Ticker is something which will be invoked every spyDuration. -// It's useful for things that should be updated on that interval. -// For example, cached shared state between Taggers and Reporters. -type Ticker interface { - Tick() error -} - -// Apply tags the report with all the taggers. -func Apply(r report.Report, taggers []Tagger) report.Report { - var err error - for _, tagger := range taggers { - r, err = tagger.Tag(r) - if err != nil { - log.Printf("error applying tagger: %v", err) - } - } - return r -} - // Topology is the Node key for the origin topology. const Topology = "topology" @@ -42,7 +11,7 @@ type topologyTagger struct{} // NewTopologyTagger tags each node with the topology that it comes from. It's // kind of a proof-of-concept tagger, useful primarily for debugging. -func newTopologyTagger() Tagger { +func NewTopologyTagger() Tagger { return &topologyTagger{} } diff --git a/probe/topology_tagger_internal_test.go b/probe/topology_tagger_internal_test.go new file mode 100644 index 000000000..8fee5e0f8 --- /dev/null +++ b/probe/topology_tagger_internal_test.go @@ -0,0 +1,17 @@ +package probe + +import ( + "testing" + + "github.com/weaveworks/scope/report" +) + +func TestTagMissingID(t *testing.T) { + const nodeID = "not-found" + r := report.MakeReport() + rpt, _ := NewTopologyTagger().Tag(r) + _, ok := rpt.Endpoint.Nodes[nodeID] + if ok { + t.Error("TopologyTagger erroneously tagged a missing node ID") + } +} diff --git a/probe/main.go b/prog/probe/main.go similarity index 59% rename from probe/main.go rename to prog/probe/main.go index f39b968ed..4b4e19400 100644 --- a/probe/main.go +++ b/prog/probe/main.go @@ -12,10 +12,10 @@ import ( "os/signal" "strconv" "strings" - "sync" "syscall" "time" + "github.com/weaveworks/scope/probe" "github.com/weaveworks/scope/probe/controls" "github.com/weaveworks/scope/probe/docker" "github.com/weaveworks/scope/probe/endpoint" @@ -36,7 +36,6 @@ func main() { httpListen = flag.String("http.listen", "", "listen address for HTTP profiling and instrumentation server") publishInterval = flag.Duration("publish.interval", 3*time.Second, "publish (output) interval") spyInterval = flag.Duration("spy.interval", time.Second, "spy (scan) interval") - prometheusEndpoint = flag.String("prometheus.endpoint", "/metrics", "Prometheus metrics exposition endpoint (requires -http.listen)") spyProcs = flag.Bool("processes", true, "report processes (needs root)") dockerEnabled = flag.Bool("docker", false, "collect Docker-related attributes for processes") dockerInterval = flag.Duration("docker.interval", 10*time.Second, "how often to update Docker attributes") @@ -72,7 +71,7 @@ func main() { rand.Seed(time.Now().UnixNano()) probeID := strconv.FormatInt(rand.Int63(), 16) var ( - hostName = hostname() + hostName = probe.Hostname() hostID = hostName // TODO(pb): we should sanitize the hostname ) log.Printf("probe starting, version %s, ID %s", version, probeID) @@ -112,49 +111,39 @@ func main() { }, xfer.ControlHandlerFunc(controls.HandleControlRequest), xfer.NewAppClient) defer clients.Stop() - resolver := newStaticResolver(targets, publishers.Set, clients.Set) + resolver := xfer.NewStaticResolver(targets, publishers.Set, clients.Set) defer resolver.Stop() endpointReporter := endpoint.NewReporter(hostID, hostName, *spyProcs, *useConntrack) defer endpointReporter.Stop() processCache := process.NewCachingWalker(process.NewWalker(*procRoot)) - - var ( - tickers = []Ticker{processCache} - reporters = []Reporter{endpointReporter, host.NewReporter(hostID, hostName, localNets), process.NewReporter(processCache, hostID)} - taggers = []Tagger{newTopologyTagger(), host.NewTagger(hostID, probeID)} + p := probe.New(*spyInterval, *publishInterval, publishers) + p.AddTicker(processCache) + p.AddReporter( + endpointReporter, + host.NewReporter(hostID, hostName, localNets), + process.NewReporter(processCache, hostID), ) + p.AddTagger(probe.NewTopologyTagger(), host.NewTagger(hostID, probeID)) - dockerTagger, dockerReporter, dockerRegistry := func() (*docker.Tagger, *docker.Reporter, docker.Registry) { - if !*dockerEnabled { - return nil, nil, nil - } + if *dockerEnabled { if err := report.AddLocalBridge(*dockerBridge); err != nil { log.Printf("Docker: problem with bridge %s: %v", *dockerBridge, err) - return nil, nil, nil } - registry, err := docker.NewRegistry(*dockerInterval) - if err != nil { + if registry, err := docker.NewRegistry(*dockerInterval); err == nil { + defer registry.Stop() + p.AddTagger(docker.NewTagger(registry, processCache)) + p.AddReporter(docker.NewReporter(registry, hostID)) + } else { log.Printf("Docker: failed to start registry: %v", err) - return nil, nil, nil } - return docker.NewTagger(registry, processCache), docker.NewReporter(registry, hostID), registry - }() - if dockerTagger != nil { - taggers = append(taggers, dockerTagger) - } - if dockerReporter != nil { - reporters = append(reporters, dockerReporter) - } - if dockerRegistry != nil { - defer dockerRegistry.Stop() } if *kubernetesEnabled { if client, err := kubernetes.NewClient(*kubernetesAPI, *kubernetesInterval); err == nil { defer client.Stop() - reporters = append(reporters, kubernetes.NewReporter(client)) + p.AddReporter(kubernetes.NewReporter(client)) } else { log.Printf("Kubernetes: failed to start client: %v", err) } @@ -162,127 +151,27 @@ func main() { if *weaveRouterAddr != "" { weave := overlay.NewWeave(hostID, *weaveRouterAddr) - tickers = append(tickers, weave) - taggers = append(taggers, weave) - reporters = append(reporters, weave) + p.AddTicker(weave) + p.AddTagger(weave) + p.AddReporter(weave) } if *httpListen != "" { go func() { log.Printf("Profiling data being exported to %s", *httpListen) log.Printf("go tool pprof http://%s/debug/pprof/{profile,heap,block}", *httpListen) - if *prometheusEndpoint != "" { - log.Printf("exposing Prometheus endpoint at %s%s", *httpListen, *prometheusEndpoint) - http.Handle(*prometheusEndpoint, makePrometheusHandler()) - } log.Printf("Profiling endpoint %s terminated: %v", *httpListen, http.ListenAndServe(*httpListen, nil)) }() } - quit, done := make(chan struct{}), sync.WaitGroup{} - done.Add(2) - defer func() { done.Wait() }() // second, wait for the main loops to be killed - defer close(quit) // first, kill the main loops - - var rpt syncReport - rpt.swap(report.MakeReport()) - - go func() { - defer done.Done() - spyTick := time.Tick(*spyInterval) - - for { - select { - case <-spyTick: - start := time.Now() - for _, ticker := range tickers { - if err := ticker.Tick(); err != nil { - log.Printf("error doing ticker: %v", err) - } - } - - localReport := rpt.copy() - localReport = localReport.Merge(doReport(reporters)) - localReport = Apply(localReport, taggers) - rpt.swap(localReport) - - if took := time.Since(start); took > *spyInterval { - log.Printf("report generation took too long (%s)", took) - } - - case <-quit: - return - } - } - }() - - go func() { - defer done.Done() - var ( - pubTick = time.Tick(*publishInterval) - p = xfer.NewReportPublisher(publishers) - ) - - for { - select { - case <-pubTick: - publishTicks.WithLabelValues().Add(1) - localReport := rpt.swap(report.MakeReport()) - localReport.Window = *publishInterval - if err := p.Publish(localReport); err != nil { - log.Printf("publish: %v", err) - } - - case <-quit: - return - } - } - }() + p.Start() + defer p.Stop() log.Printf("%s", <-interrupt()) } -func doReport(reporters []Reporter) report.Report { - reports := make(chan report.Report, len(reporters)) - for _, rep := range reporters { - go func(rep Reporter) { - newReport, err := rep.Report() - if err != nil { - log.Printf("error generating report: %v", err) - newReport = report.MakeReport() // empty is OK to merge - } - reports <- newReport - }(rep) - } - - result := report.MakeReport() - for i := 0; i < cap(reports); i++ { - result = result.Merge(<-reports) - } - return result -} - func interrupt() <-chan os.Signal { c := make(chan os.Signal) signal.Notify(c, syscall.SIGINT, syscall.SIGTERM) return c } - -type syncReport struct { - mtx sync.RWMutex - rpt report.Report -} - -func (r *syncReport) swap(other report.Report) report.Report { - r.mtx.Lock() - defer r.mtx.Unlock() - old := r.rpt - r.rpt = other - return old -} - -func (r *syncReport) copy() report.Report { - r.mtx.RLock() - defer r.mtx.RUnlock() - return r.rpt.Copy() -} diff --git a/probe/resolver.go b/xfer/resolver.go similarity index 88% rename from probe/resolver.go rename to xfer/resolver.go index 1814b68b1..c2524a8f1 100644 --- a/probe/resolver.go +++ b/xfer/resolver.go @@ -1,4 +1,4 @@ -package main +package xfer import ( "log" @@ -6,8 +6,6 @@ import ( "strconv" "strings" "time" - - "github.com/weaveworks/scope/xfer" ) const ( @@ -21,6 +19,11 @@ var ( type setter func(string, []string) +// Resolver is a thing that can be stopped... +type Resolver interface { + Stop() +} + type staticResolver struct { setters []setter targets []target @@ -31,10 +34,10 @@ type target struct{ host, port string } func (t target) String() string { return net.JoinHostPort(t.host, t.port) } -// newStaticResolver periodically resolves the targets, and calls the set +// NewStaticResolver periodically resolves the targets, and calls the set // function with all the resolved IPs. It explictiy supports targets which // resolve to multiple IPs. -func newStaticResolver(targets []string, setters ...setter) staticResolver { +func NewStaticResolver(targets []string, setters ...setter) Resolver { r := staticResolver{ targets: prepare(targets), setters: setters, @@ -73,7 +76,7 @@ func prepare(strs []string) []target { continue } } else { - host, port = s, strconv.Itoa(xfer.AppPort) + host, port = s, strconv.Itoa(AppPort) } targets = append(targets, target{host, port}) } diff --git a/probe/resolver_test.go b/xfer/resolver_internal_test.go similarity index 87% rename from probe/resolver_test.go rename to xfer/resolver_internal_test.go index 4463e8f22..c1d57b948 100644 --- a/probe/resolver_test.go +++ b/xfer/resolver_internal_test.go @@ -1,4 +1,4 @@ -package main +package xfer import ( "fmt" @@ -7,8 +7,6 @@ import ( "sync" "testing" "time" - - "github.com/weaveworks/scope/xfer" ) func TestResolver(t *testing.T) { @@ -46,7 +44,7 @@ func TestResolver(t *testing.T) { } } - r := newStaticResolver([]string{"symbolic.name" + port, "namewithnoport", ip1 + port, ip2}, set) + r := NewStaticResolver([]string{"symbolic.name" + port, "namewithnoport", ip1 + port, ip2}, set) assertAdd := func(want ...string) { remaining := map[string]struct{}{} @@ -70,22 +68,22 @@ func TestResolver(t *testing.T) { } // Initial resolve should just give us IPs - assertAdd(ip1+port, fmt.Sprintf("%s:%d", ip2, xfer.AppPort)) + assertAdd(ip1+port, fmt.Sprintf("%s:%d", ip2, AppPort)) // Trigger another resolve with a tick; again, // just want ips. c <- time.Now() - assertAdd(ip1+port, fmt.Sprintf("%s:%d", ip2, xfer.AppPort)) + assertAdd(ip1+port, fmt.Sprintf("%s:%d", ip2, AppPort)) ip3 := "1.2.3.4" updateIPs("symbolic.name", makeIPs(ip3)) c <- time.Now() // trigger a resolve - assertAdd(ip3+port, ip1+port, fmt.Sprintf("%s:%d", ip2, xfer.AppPort)) + assertAdd(ip3+port, ip1+port, fmt.Sprintf("%s:%d", ip2, AppPort)) ip4 := "10.10.10.10" updateIPs("symbolic.name", makeIPs(ip3, ip4)) c <- time.Now() // trigger another resolve, this time with 2 adds - assertAdd(ip3+port, ip4+port, ip1+port, fmt.Sprintf("%s:%d", ip2, xfer.AppPort)) + assertAdd(ip3+port, ip4+port, ip1+port, fmt.Sprintf("%s:%d", ip2, AppPort)) done := make(chan struct{}) go func() { r.Stop(); close(done) }() From a89c0b9b88878b79a9378eba38ddd3700e7910f9 Mon Sep 17 00:00:00 2001 From: Tom Wilkie Date: Mon, 9 Nov 2015 16:25:49 +0000 Subject: [PATCH 2/2] Make an empty StringSet nil. --- render/filters.go | 2 +- render/filters_test.go | 110 ++++++++++++++++++++++++++++++++++++++++ render/render_test.go | 100 ------------------------------------ report/topology.go | 10 +++- report/topology_test.go | 4 +- 5 files changed, 122 insertions(+), 104 deletions(-) create mode 100644 render/filters_test.go diff --git a/render/filters.go b/render/filters.go index 6c08aed3f..84711d177 100644 --- a/render/filters.go +++ b/render/filters.go @@ -78,7 +78,7 @@ func (f Filter) render(rpt report.Report) (RenderableNodes, int) { // Deleted nodes also need to be cut as destinations in adjacency lists. for id, node := range output { - newAdjacency := make(report.IDList, 0, len(node.Adjacency)) + newAdjacency := report.MakeIDList() for _, dstID := range node.Adjacency { if _, ok := output[dstID]; ok { newAdjacency = newAdjacency.Add(dstID) diff --git a/render/filters_test.go b/render/filters_test.go new file mode 100644 index 000000000..905fea177 --- /dev/null +++ b/render/filters_test.go @@ -0,0 +1,110 @@ +package render_test + +import ( + "reflect" + "testing" + + "github.com/weaveworks/scope/render" + "github.com/weaveworks/scope/report" + "github.com/weaveworks/scope/test" +) + +func TestFilterRender(t *testing.T) { + renderer := render.FilterUnconnected( + mockRenderer{RenderableNodes: render.RenderableNodes{ + "foo": {ID: "foo", Node: report.MakeNode().WithAdjacent("bar")}, + "bar": {ID: "bar", Node: report.MakeNode().WithAdjacent("foo")}, + "baz": {ID: "baz", Node: report.MakeNode()}, + }}) + want := render.RenderableNodes{ + "foo": {ID: "foo", Node: report.MakeNode().WithAdjacent("bar")}, + "bar": {ID: "bar", Node: report.MakeNode().WithAdjacent("foo")}, + } + have := renderer.Render(report.MakeReport()).Prune() + if !reflect.DeepEqual(want, have) { + t.Error(test.Diff(want, have)) + } +} + +func TestFilterRender2(t *testing.T) { + // Test adjacencies are removed for filtered nodes. + renderer := render.Filter{ + FilterFunc: func(node render.RenderableNode) bool { + return node.ID != "bar" + }, + Renderer: mockRenderer{RenderableNodes: render.RenderableNodes{ + "foo": {ID: "foo", Node: report.MakeNode().WithAdjacent("bar")}, + "bar": {ID: "bar", Node: report.MakeNode().WithAdjacent("foo")}, + "baz": {ID: "baz", Node: report.MakeNode()}, + }}, + } + want := render.RenderableNodes{ + "foo": {ID: "foo", Node: report.MakeNode()}, + "baz": {ID: "baz", Node: report.MakeNode()}, + } + have := renderer.Render(report.MakeReport()).Prune() + if !reflect.DeepEqual(want, have) { + t.Error(test.Diff(want, have)) + } +} + +func TestFilterUnconnectedPesudoNodes(t *testing.T) { + // Test pseudo nodes that are made unconnected by filtering + // are also removed. + { + nodes := render.RenderableNodes{ + "foo": {ID: "foo", Node: report.MakeNode().WithAdjacent("bar")}, + "bar": {ID: "bar", Node: report.MakeNode().WithAdjacent("baz")}, + "baz": {ID: "baz", Node: report.MakeNode(), Pseudo: true}, + } + renderer := render.Filter{ + FilterFunc: func(node render.RenderableNode) bool { + return true + }, + Renderer: mockRenderer{RenderableNodes: nodes}, + } + want := nodes.Prune() + have := renderer.Render(report.MakeReport()).Prune() + if !reflect.DeepEqual(want, have) { + t.Error(test.Diff(want, have)) + } + } + { + renderer := render.Filter{ + FilterFunc: func(node render.RenderableNode) bool { + return node.ID != "bar" + }, + Renderer: mockRenderer{RenderableNodes: render.RenderableNodes{ + "foo": {ID: "foo", Node: report.MakeNode().WithAdjacent("bar")}, + "bar": {ID: "bar", Node: report.MakeNode().WithAdjacent("baz")}, + "baz": {ID: "baz", Node: report.MakeNode(), Pseudo: true}, + }}, + } + want := render.RenderableNodes{ + "foo": {ID: "foo", Node: report.MakeNode()}, + } + have := renderer.Render(report.MakeReport()).Prune() + if !reflect.DeepEqual(want, have) { + t.Error(test.Diff(want, have)) + } + } + { + renderer := render.Filter{ + FilterFunc: func(node render.RenderableNode) bool { + return node.ID != "bar" + }, + Renderer: mockRenderer{RenderableNodes: render.RenderableNodes{ + "foo": {ID: "foo", Node: report.MakeNode()}, + "bar": {ID: "bar", Node: report.MakeNode().WithAdjacent("foo")}, + "baz": {ID: "baz", Node: report.MakeNode().WithAdjacent("bar"), Pseudo: true}, + }}, + } + want := render.RenderableNodes{ + "foo": {ID: "foo", Node: report.MakeNode()}, + } + have := renderer.Render(report.MakeReport()).Prune() + if !reflect.DeepEqual(want, have) { + t.Error(test.Diff(want, have)) + } + } +} diff --git a/render/render_test.go b/render/render_test.go index 1604562ed..df2f19d86 100644 --- a/render/render_test.go +++ b/render/render_test.go @@ -180,104 +180,4 @@ func TestMapEdge(t *testing.T) { } } -func TestFilterRender(t *testing.T) { - renderer := render.FilterUnconnected( - mockRenderer{RenderableNodes: render.RenderableNodes{ - "foo": {ID: "foo", Node: report.MakeNode().WithAdjacent("bar")}, - "bar": {ID: "bar", Node: report.MakeNode().WithAdjacent("foo")}, - "baz": {ID: "baz", Node: report.MakeNode()}, - }}) - want := render.RenderableNodes{ - "foo": {ID: "foo", Origins: report.IDList{}, Node: report.MakeNode().WithAdjacent("bar")}, - "bar": {ID: "bar", Origins: report.IDList{}, Node: report.MakeNode().WithAdjacent("foo")}, - }.Prune() - have := renderer.Render(report.MakeReport()).Prune() - if !reflect.DeepEqual(want, have) { - t.Error(test.Diff(want, have)) - } -} - -func TestFilterRender2(t *testing.T) { - // Test adjacencies are removed for filtered nodes. - renderer := render.Filter{ - FilterFunc: func(node render.RenderableNode) bool { - return node.ID != "bar" - }, - Renderer: mockRenderer{RenderableNodes: render.RenderableNodes{ - "foo": {ID: "foo", Node: report.MakeNode().WithAdjacent("bar")}, - "bar": {ID: "bar", Node: report.MakeNode().WithAdjacent("foo")}, - "baz": {ID: "baz", Node: report.MakeNode()}, - }}, - } - want := render.RenderableNodes{ - "foo": {ID: "foo", Origins: report.IDList{}, Node: report.MakeNode()}, - "baz": {ID: "baz", Origins: report.IDList{}, Node: report.MakeNode()}, - }.Prune() - have := renderer.Render(report.MakeReport()).Prune() - if !reflect.DeepEqual(want, have) { - t.Error(test.Diff(want, have)) - } -} - -func TestFilterUnconnectedPesudoNodes(t *testing.T) { - // Test pseudo nodes that are made unconnected by filtering - // are also removed. - { - nodes := render.RenderableNodes{ - "foo": {ID: "foo", Node: report.MakeNode().WithAdjacent("bar")}, - "bar": {ID: "bar", Node: report.MakeNode().WithAdjacent("baz")}, - "baz": {ID: "baz", Node: report.MakeNode(), Pseudo: true}, - } - renderer := render.Filter{ - FilterFunc: func(node render.RenderableNode) bool { - return true - }, - Renderer: mockRenderer{RenderableNodes: nodes}, - } - want := nodes.Prune() - have := renderer.Render(report.MakeReport()).Prune() - if !reflect.DeepEqual(want, have) { - t.Error(test.Diff(want, have)) - } - } - { - renderer := render.Filter{ - FilterFunc: func(node render.RenderableNode) bool { - return node.ID != "bar" - }, - Renderer: mockRenderer{RenderableNodes: render.RenderableNodes{ - "foo": {ID: "foo", Node: report.MakeNode().WithAdjacent("bar")}, - "bar": {ID: "bar", Node: report.MakeNode().WithAdjacent("baz")}, - "baz": {ID: "baz", Node: report.MakeNode(), Pseudo: true}, - }}, - } - want := render.RenderableNodes{ - "foo": {ID: "foo", Origins: report.IDList{}, Node: report.MakeNode()}, - }.Prune() - have := renderer.Render(report.MakeReport()).Prune() - if !reflect.DeepEqual(want, have) { - t.Error(test.Diff(want, have)) - } - } - { - renderer := render.Filter{ - FilterFunc: func(node render.RenderableNode) bool { - return node.ID != "bar" - }, - Renderer: mockRenderer{RenderableNodes: render.RenderableNodes{ - "foo": {ID: "foo", Node: report.MakeNode()}, - "bar": {ID: "bar", Node: report.MakeNode().WithAdjacent("foo")}, - "baz": {ID: "baz", Node: report.MakeNode().WithAdjacent("bar"), Pseudo: true}, - }}, - } - want := render.RenderableNodes{ - "foo": {ID: "foo", Origins: report.IDList{}, Node: report.MakeNode()}, - }.Prune() - have := renderer.Render(report.MakeReport()).Prune() - if !reflect.DeepEqual(want, have) { - t.Error(test.Diff(want, have)) - } - } -} - func newu64(value uint64) *uint64 { return &value } diff --git a/report/topology.go b/report/topology.go index 9fa6de15c..3700b1496 100644 --- a/report/topology.go +++ b/report/topology.go @@ -271,7 +271,7 @@ type StringSet []string // MakeStringSet makes a new StringSet with the given strings. func MakeStringSet(strs ...string) StringSet { if len(strs) <= 0 { - return StringSet{} + return nil } result := make([]string, len(strs)) copy(result, strs) @@ -305,8 +305,11 @@ func (s StringSet) Add(strs ...string) StringSet { // Merge combines the two StringSets and returns a new result. func (s StringSet) Merge(other StringSet) StringSet { - if len(other) == 0 { // Optimise special case, to avoid allocating + switch { + case len(other) <= 0: // Optimise special case, to avoid allocating return s // (note unit test DeepEquals breaks if we don't do this) + case len(s) <= 0: + return other } result := make(StringSet, len(s)+len(other)) for i, j, k := 0, 0, 0; ; k++ { @@ -333,6 +336,9 @@ func (s StringSet) Merge(other StringSet) StringSet { // Copy returns a value copy of the StringSet. func (s StringSet) Copy() StringSet { + if s == nil { + return s + } result := make(StringSet, len(s)) copy(result, s) return result diff --git a/report/topology_test.go b/report/topology_test.go index bd0a3a978..c965f9b7d 100644 --- a/report/topology_test.go +++ b/report/topology_test.go @@ -12,6 +12,7 @@ func TestMakeStringSet(t *testing.T) { input []string want report.StringSet }{ + {input: nil, want: nil}, {input: []string{}, want: report.MakeStringSet()}, {input: []string{"a"}, want: report.MakeStringSet("a")}, {input: []string{"a", "a"}, want: report.MakeStringSet("a")}, @@ -29,6 +30,7 @@ func TestStringSetAdd(t *testing.T) { strs []string want report.StringSet }{ + {input: report.StringSet(nil), strs: []string{}, want: report.StringSet(nil)}, {input: report.MakeStringSet(), strs: []string{}, want: report.MakeStringSet()}, {input: report.MakeStringSet("a"), strs: []string{}, want: report.MakeStringSet("a")}, {input: report.MakeStringSet(), strs: []string{"a"}, want: report.MakeStringSet("a")}, @@ -49,6 +51,7 @@ func TestStringSetMerge(t *testing.T) { other report.StringSet want report.StringSet }{ + {input: report.StringSet(nil), other: report.StringSet(nil), want: report.StringSet(nil)}, {input: report.MakeStringSet(), other: report.MakeStringSet(), want: report.MakeStringSet()}, {input: report.MakeStringSet("a"), other: report.MakeStringSet(), want: report.MakeStringSet("a")}, {input: report.MakeStringSet(), other: report.MakeStringSet("a"), want: report.MakeStringSet("a")}, @@ -62,5 +65,4 @@ func TestStringSetMerge(t *testing.T) { t.Errorf("%v + %v: want %v, have %v", testcase.input, testcase.other, want, have) } } - }