From 85d2f6309c8a7565c667788fd0fd4cdc9ee298a5 Mon Sep 17 00:00:00 2001 From: Bryan Boreham Date: Mon, 14 Oct 2019 15:31:34 +0000 Subject: [PATCH 1/5] performance: Send active controls as a single string per node Instead of a whole extra data structure which is quite expensive to marshal and unmarshal, just send the information in a string. No clever merging strategy is required - the states are all set in one place per node type. --- probe/awsecs/reporter.go | 13 +- probe/docker/container.go | 25 ++- probe/docker/container_test.go | 19 +-- render/detailed/node.go | 8 +- report/controls.go | 6 - report/latest_map_generated.go | 267 +-------------------------------- report/map_keys.go | 2 + report/marshal_test.go | 1 - report/node.go | 85 ++++------- 9 files changed, 64 insertions(+), 362 deletions(-) diff --git a/probe/awsecs/reporter.go b/probe/awsecs/reporter.go index 80baa66484..bba0a81607 100644 --- a/probe/awsecs/reporter.go +++ b/probe/awsecs/reporter.go @@ -150,17 +150,18 @@ func (r Reporter) Tag(rpt report.Report) (report.Report, error) { // Create all the services first for serviceName, service := range ecsInfo.Services { serviceID := report.MakeECSServiceNodeID(cluster, serviceName) + activeControls := []string{ScaleUp} + // Disable ScaleDown when only 1 task is desired, since + // scaling down to 0 would cause the service to disappear (#2085) + if service.DesiredCount < 1 { + activeControls = append(activeControls, ScaleDown) + } rpt.ECSService.AddNode(report.MakeNodeWith(serviceID, map[string]string{ Cluster: cluster, ServiceDesiredCount: fmt.Sprintf("%d", service.DesiredCount), ServiceRunningCount: fmt.Sprintf("%d", service.RunningCount), report.ControlProbeID: r.probeID, - }).WithLatestControls(map[string]report.NodeControlData{ - ScaleUp: {Dead: false}, - // We've decided for now to disable ScaleDown when only 1 task is desired, - // since scaling down to 0 would cause the service to disappear (#2085) - ScaleDown: {Dead: service.DesiredCount <= 1}, - })) + }).WithLatestActiveControls(activeControls...)) } log.Debugf("Created %v ECS service nodes", len(ecsInfo.Services)) diff --git a/probe/docker/container.go b/probe/docker/container.go index f4c7f7d8a5..e42f707e06 100644 --- a/probe/docker/container.go +++ b/probe/docker/container.go @@ -389,20 +389,18 @@ func (c *container) getBaseNode() report.Node { return result } -func (c *container) controlsMap() map[string]report.NodeControlData { +// Return a slice including all controls that should be shown on this container +func (c *container) controls() []string { paused := c.container.State.Paused - running := !paused && c.container.State.Running - stopped := !paused && !running - return map[string]report.NodeControlData{ - UnpauseContainer: {Dead: !paused}, - RestartContainer: {Dead: !running}, - StopContainer: {Dead: !running}, - PauseContainer: {Dead: !running}, - AttachContainer: {Dead: !running}, - ExecContainer: {Dead: !running}, - StartContainer: {Dead: !stopped}, - RemoveContainer: {Dead: !stopped}, + switch { + case paused: + return []string{UnpauseContainer} + case c.container.State.Running: + return []string{RestartContainer, StopContainer, PauseContainer, AttachContainer, ExecContainer} + case !c.container.State.Running: + return []string{StartContainer, RemoveContainer} } + return nil } func (c *container) GetNode() report.Node { @@ -413,7 +411,6 @@ func (c *container) GetNode() report.Node { ContainerState: c.StateString(), ContainerStateHuman: c.State(), } - controls := c.controlsMap() if !c.container.State.Paused && c.container.State.Running { uptimeSeconds := int(mtime.Now().Sub(c.container.State.StartedAt) / time.Second) @@ -427,7 +424,7 @@ func (c *container) GetNode() report.Node { } result := c.baseNode.WithLatests(latest) - result = result.WithLatestControls(controls) + result = result.WithLatestActiveControls(c.controls()...) result = result.WithMetrics(c.metrics()) return result } diff --git a/probe/docker/container_test.go b/probe/docker/container_test.go index 10c6f7074c..9b35dc2163 100644 --- a/probe/docker/container_test.go +++ b/probe/docker/container_test.go @@ -60,15 +60,12 @@ func TestContainer(t *testing.T) { // Now see if we go them { uptimeSeconds := int(now.Sub(startTime) / time.Second) - controls := map[string]report.NodeControlData{ - docker.UnpauseContainer: {Dead: true}, - docker.RestartContainer: {Dead: false}, - docker.StopContainer: {Dead: false}, - docker.PauseContainer: {Dead: false}, - docker.AttachContainer: {Dead: false}, - docker.ExecContainer: {Dead: false}, - docker.StartContainer: {Dead: true}, - docker.RemoveContainer: {Dead: true}, + controls := []string{ + docker.RestartContainer, + docker.StopContainer, + docker.PauseContainer, + docker.AttachContainer, + docker.ExecContainer, } want := report.MakeNodeWith("ping;", map[string]string{ "docker_container_command": "ping foo.bar.local", @@ -82,8 +79,8 @@ func TestContainer(t *testing.T) { "docker_container_state_human": c.Container().State.String(), "docker_container_uptime": strconv.Itoa(uptimeSeconds), "docker_env_FOO": "secret-bar", - }).WithLatestControls( - controls, + }).WithLatestActiveControls( + controls..., ).WithMetrics(report.Metrics{ "docker_cpu_total_usage": report.MakeMetric(nil), "docker_memory_usage": report.MakeSingletonMetric(now, 12345).WithMax(45678), diff --git a/render/detailed/node.go b/render/detailed/node.go index 7b8b291645..d96a739eda 100644 --- a/render/detailed/node.go +++ b/render/detailed/node.go @@ -2,7 +2,6 @@ package detailed import ( "sort" - "time" "github.com/ugorji/go/codec" @@ -112,10 +111,7 @@ func controlsFor(topology report.Topology, nodeID string) []ControlInstance { if !ok { return result } - node.LatestControls.ForEach(func(controlID string, _ time.Time, data report.NodeControlData) { - if data.Dead { - return - } + for _, controlID := range node.ActiveControls() { if control, ok := topology.Controls[controlID]; ok { result = append(result, ControlInstance{ ProbeID: probeID, @@ -123,7 +119,7 @@ func controlsFor(topology report.Topology, nodeID string) []ControlInstance { Control: control, }) } - }) + } return result } diff --git a/report/controls.go b/report/controls.go index fda4d1e0e9..b66658b1f4 100644 --- a/report/controls.go +++ b/report/controls.go @@ -50,9 +50,3 @@ func (cs Controls) AddControls(controls []Control) { cs[c.ID] = c } } - -// NodeControlData contains specific information about the control. It -// is used as a Value field of LatestEntry in NodeControlDataLatestMap. -type NodeControlData struct { - Dead bool `json:"dead"` -} diff --git a/report/latest_map_generated.go b/report/latest_map_generated.go index c084cb92d5..36e40ea16d 100644 --- a/report/latest_map_generated.go +++ b/report/latest_map_generated.go @@ -1,5 +1,5 @@ // Generated file, do not edit. -// To regenerate, run ../extras/generate_latest_map ./latest_map_generated.go string NodeControlData +// To regenerate, run ../extras/generate_latest_map ./latest_map_generated.go string package report @@ -276,268 +276,3 @@ func (StringLatestMap) MarshalJSON() ([]byte, error) { func (*StringLatestMap) UnmarshalJSON(b []byte) error { panic("UnmarshalJSON shouldn't be used, use CodecDecodeSelf instead") } - -type nodeControlDataLatestEntry struct { - key string - Timestamp time.Time `json:"timestamp"` - Value NodeControlData `json:"value"` - dummySelfer -} - -// String returns the StringLatestEntry's string representation. -func (e *nodeControlDataLatestEntry) String() string { - return fmt.Sprintf("%v (%s)", e.Value, e.Timestamp.Format(time.RFC3339)) -} - -// Equal returns true if the supplied StringLatestEntry is equal to this one. -func (e *nodeControlDataLatestEntry) Equal(e2 *nodeControlDataLatestEntry) bool { - return e.Timestamp.Equal(e2.Timestamp) && e.Value == e2.Value -} - -// NodeControlDataLatestMap holds latest NodeControlData instances, as a slice sorted by key. -type NodeControlDataLatestMap []nodeControlDataLatestEntry - -// MakeNodeControlDataLatestMap makes an empty NodeControlDataLatestMap. -func MakeNodeControlDataLatestMap() NodeControlDataLatestMap { - return NodeControlDataLatestMap{} -} - -// Size returns the number of elements. -func (m NodeControlDataLatestMap) Size() int { - return len(m) -} - -// Merge produces a NodeControlDataLatestMap containing the keys from both inputs. -// When both inputs contain the same key, the newer value is used. -// Tries to return one of its inputs, if that already holds the correct result. -func (m NodeControlDataLatestMap) Merge(n NodeControlDataLatestMap) NodeControlDataLatestMap { - switch { - case len(m) == 0: - return n - case len(n) == 0: - return m - } - if len(n) > len(m) { - m, n = n, m //swap so m is always at least as long as n - } else if len(n) == len(m) && m[0].Timestamp.Before(n[0].Timestamp) { - // Optimise common case where we merge two nodes with the same contents - // sampled at different times. - m, n = n, m // swap equal-length arrays so first element of m is newer - } - - i, j := 0, 0 -loop: - for i < len(m) { - switch { - case j >= len(n): - return m - case m[i].key == n[j].key: - if m[i].Timestamp.Before(n[j].Timestamp) { - break loop - } - i++ - j++ - case m[i].key < n[j].key: - i++ - default: - break loop - } - } - if i >= len(m) && j >= len(n) { - return m - } - - out := make([]nodeControlDataLatestEntry, i, len(m)) - copy(out, m[:i]) - - for i < len(m) { - switch { - case j >= len(n): - out = append(out, m[i:]...) - return out - case m[i].key == n[j].key: - if m[i].Timestamp.Before(n[j].Timestamp) { - out = append(out, n[j]) - } else { - out = append(out, m[i]) - } - i++ - j++ - case m[i].key < n[j].key: - out = append(out, m[i]) - i++ - default: - out = append(out, n[j]) - j++ - } - } - out = append(out, n[j:]...) - return out -} - -// Lookup the value for the given key. -func (m NodeControlDataLatestMap) Lookup(key string) (NodeControlData, bool) { - v, _, ok := m.LookupEntry(key) - if !ok { - var zero NodeControlData - return zero, false - } - return v, true -} - -// LookupEntry returns the raw entry for the given key. -func (m NodeControlDataLatestMap) LookupEntry(key string) (NodeControlData, time.Time, bool) { - i := sort.Search(len(m), func(i int) bool { - return m[i].key >= key - }) - if i < len(m) && m[i].key == key { - return m[i].Value, m[i].Timestamp, true - } - var zero NodeControlData - return zero, time.Time{}, false -} - -// locate the position where key should go, and make room for it if not there already -func (m *NodeControlDataLatestMap) locate(key string) int { - i := sort.Search(len(*m), func(i int) bool { - return (*m)[i].key >= key - }) - // i is now the position where key should go, either at the end or in the middle - if i == len(*m) || (*m)[i].key != key { - *m = append(*m, nodeControlDataLatestEntry{}) - copy((*m)[i+1:], (*m)[i:]) - (*m)[i] = nodeControlDataLatestEntry{} - } - return i -} - -// Set the value for the given key. -func (m NodeControlDataLatestMap) Set(key string, timestamp time.Time, value NodeControlData) NodeControlDataLatestMap { - i := sort.Search(len(m), func(i int) bool { - return m[i].key >= key - }) - // i is now the position where key should go, either at the end or in the middle - oldEntries := m - if i == len(m) { - m = make([]nodeControlDataLatestEntry, len(oldEntries)+1) - copy(m, oldEntries) - } else if m[i].key == key { - m = make([]nodeControlDataLatestEntry, len(oldEntries)) - copy(m, oldEntries) - } else { - m = make([]nodeControlDataLatestEntry, len(oldEntries)+1) - copy(m, oldEntries[:i]) - copy(m[i+1:], oldEntries[i:]) - } - m[i] = nodeControlDataLatestEntry{key: key, Timestamp: timestamp, Value: value} - return m -} - -// ForEach executes fn on each key value pair in the map. -func (m NodeControlDataLatestMap) ForEach(fn func(k string, timestamp time.Time, v NodeControlData)) { - for _, value := range m { - fn(value.key, value.Timestamp, value.Value) - } -} - -// String returns the NodeControlDataLatestMap's string representation. -func (m NodeControlDataLatestMap) String() string { - buf := bytes.NewBufferString("{") - for _, val := range m { - fmt.Fprintf(buf, "%s: %s,\n", val.key, val.String()) - } - fmt.Fprintf(buf, "}") - return buf.String() -} - -// DeepEqual tests equality with other NodeControlDataLatestMap. -func (m NodeControlDataLatestMap) DeepEqual(n NodeControlDataLatestMap) bool { - if m.Size() != n.Size() { - return false - } - for i := range m { - if m[i].key != n[i].key || !m[i].Equal(&n[i]) { - return false - } - } - return true -} - -// EqualIgnoringTimestamps returns true if all keys and values are the same. -func (m NodeControlDataLatestMap) EqualIgnoringTimestamps(n NodeControlDataLatestMap) bool { - if m.Size() != n.Size() { - return false - } - for i := range m { - if m[i].key != n[i].key || m[i].Value != n[i].Value { - return false - } - } - return true -} - -// CodecEncodeSelf implements codec.Selfer. -// Duplicates the output for a built-in map without generating an -// intermediate copy of the data structure, to save time. Note this -// means we are using undocumented, internal APIs, which could break -// in the future. See https://github.com/weaveworks/scope/pull/1709 -// for more information. -func (m NodeControlDataLatestMap) CodecEncodeSelf(encoder *codec.Encoder) { - z, r := codec.GenHelperEncoder(encoder) - if m == nil { - r.EncodeNil() - return - } - r.EncodeMapStart(m.Size()) - for _, val := range m { - z.EncSendContainerState(containerMapKey) - r.EncodeString(cUTF8, val.key) - z.EncSendContainerState(containerMapValue) - val.CodecEncodeSelf(encoder) - } - z.EncSendContainerState(containerMapEnd) -} - -// CodecDecodeSelf implements codec.Selfer. -// Decodes the input as for a built-in map, without creating an -// intermediate copy of the data structure to save time. Uses -// undocumented, internal APIs as for CodecEncodeSelf. -func (m *NodeControlDataLatestMap) CodecDecodeSelf(decoder *codec.Decoder) { - *m = nil - z, r := codec.GenHelperDecoder(decoder) - if r.TryDecodeAsNil() { - return - } - - length := r.ReadMapStart() - if length > 0 { - *m = make([]nodeControlDataLatestEntry, 0, length) - } - for i := 0; length < 0 || i < length; i++ { - if length < 0 && r.CheckBreak() { - break - } - z.DecSendContainerState(containerMapKey) - var key string - if !r.TryDecodeAsNil() { - key = lookupCommonKey(r.DecodeStringAsBytes()) - } - i := m.locate(key) - (*m)[i].key = key - z.DecSendContainerState(containerMapValue) - if !r.TryDecodeAsNil() { - (*m)[i].CodecDecodeSelf(decoder) - } - } - z.DecSendContainerState(containerMapEnd) -} - -// MarshalJSON shouldn't be used, use CodecEncodeSelf instead. -func (NodeControlDataLatestMap) MarshalJSON() ([]byte, error) { - panic("MarshalJSON shouldn't be used, use CodecEncodeSelf instead") -} - -// UnmarshalJSON shouldn't be used, use CodecDecodeSelf instead. -func (*NodeControlDataLatestMap) UnmarshalJSON(b []byte) error { - panic("UnmarshalJSON shouldn't be used, use CodecDecodeSelf instead") -} diff --git a/report/map_keys.go b/report/map_keys.go index 388f39dc23..e8bfac3f8d 100644 --- a/report/map_keys.go +++ b/report/map_keys.go @@ -2,6 +2,8 @@ package report // node metadata keys const ( + // Node + NodeActiveControls = "active_controls" // probe/endpoint ReverseDNSNames = "reverse_dns_names" SnoopedDNSNames = "snooped_dns_names" diff --git a/report/marshal_test.go b/report/marshal_test.go index 2926043b34..08518a4956 100644 --- a/report/marshal_test.go +++ b/report/marshal_test.go @@ -49,7 +49,6 @@ func makeTestReport() report.Report { r.Pod.WithShape("heptagon").WithLabel("pod", "pods"). AddNode(report.MakeNode("fceef9592ec3cf1a8e1d178fdd0de41a;"). WithTopology("pod"). - WithLatestControls(map[string]report.NodeControlData{"kubernetes_get_logs": {Dead: true}}). WithLatest("host_node_id", t1, "ip-172-20-1-168;")) r.Overlay.WithMetadataTemplates(report.MetadataTemplates{ "weave_encryption": report.MetadataTemplate{ID: "weave_encryption", Label: "Encryption", Priority: 4, From: "latest"}, diff --git a/report/node.go b/report/node.go index c64d70687f..9883cd7e23 100644 --- a/report/node.go +++ b/report/node.go @@ -1,6 +1,7 @@ package report import ( + "strings" "time" "github.com/weaveworks/common/mtime" @@ -10,29 +11,27 @@ import ( // about a given node in a given topology, along with the edges (aka // adjacency) emanating from the node. type Node struct { - ID string `json:"id,omitempty"` - Topology string `json:"topology,omitempty"` - Counters Counters `json:"counters,omitempty"` - Sets Sets `json:"sets,omitempty"` - Adjacency IDList `json:"adjacency,omitempty"` - LatestControls NodeControlDataLatestMap `json:"latestControls,omitempty"` - Latest StringLatestMap `json:"latest,omitempty"` - Metrics Metrics `json:"metrics,omitempty" deepequal:"nil==empty"` - Parents Sets `json:"parents,omitempty"` - Children NodeSet `json:"children,omitempty"` + ID string `json:"id,omitempty"` + Topology string `json:"topology,omitempty"` + Counters Counters `json:"counters,omitempty"` + Sets Sets `json:"sets,omitempty"` + Adjacency IDList `json:"adjacency,omitempty"` + Latest StringLatestMap `json:"latest,omitempty"` + Metrics Metrics `json:"metrics,omitempty" deepequal:"nil==empty"` + Parents Sets `json:"parents,omitempty"` + Children NodeSet `json:"children,omitempty"` } // MakeNode creates a new Node with no initial metadata. func MakeNode(id string) Node { return Node{ - ID: id, - Counters: MakeCounters(), - Sets: MakeSets(), - Adjacency: MakeIDList(), - LatestControls: MakeNodeControlDataLatestMap(), - Latest: MakeStringLatestMap(), - Metrics: Metrics{}, - Parents: MakeSets(), + ID: id, + Counters: MakeCounters(), + Sets: MakeSets(), + Adjacency: MakeIDList(), + Latest: MakeStringLatestMap(), + Metrics: Metrics{}, + Parents: MakeSets(), } } @@ -118,28 +117,16 @@ func (n Node) WithAdjacent(a ...string) Node { return n } -// WithLatestActiveControls returns a fresh copy of n, with active controls cs added to LatestControls. +// WithLatestActiveControls says which controls are active on this node. +// Implemented as a delimiter-separated string in Latest func (n Node) WithLatestActiveControls(cs ...string) Node { - lcs := map[string]NodeControlData{} - for _, control := range cs { - lcs[control] = NodeControlData{} - } - return n.WithLatestControls(lcs) -} - -// WithLatestControls returns a fresh copy of n, with lcs added to LatestControls. -func (n Node) WithLatestControls(lcs map[string]NodeControlData) Node { - ts := mtime.Now() - for k, v := range lcs { - n.LatestControls = n.LatestControls.Set(k, ts, v) - } - return n + return n.WithLatest(NodeActiveControls, mtime.Now(), strings.Join(cs, ScopeDelim)) } -// WithLatestControl produces a new Node with control added to it -func (n Node) WithLatestControl(control string, ts time.Time, data NodeControlData) Node { - n.LatestControls = n.LatestControls.Set(control, ts, data) - return n +// ActiveControls returns a string slice with the names of active controls. +func (n Node) ActiveControls(cs ...string) []string { + activeControls, _ := n.Latest.Lookup(NodeActiveControls) + return strings.Split(activeControls, ScopeDelim) } // WithParent returns a fresh copy of n, with one parent added @@ -186,16 +173,15 @@ func (n Node) Merge(other Node) Node { panic("Cannot merge nodes with different topology types: " + topology + " != " + other.Topology) } return Node{ - ID: id, - Topology: topology, - Counters: n.Counters.Merge(other.Counters), - Sets: n.Sets.Merge(other.Sets), - Adjacency: n.Adjacency.Merge(other.Adjacency), - LatestControls: n.LatestControls.Merge(other.LatestControls), - Latest: n.Latest.Merge(other.Latest), - Metrics: n.Metrics.Merge(other.Metrics), - Parents: n.Parents.Merge(other.Parents), - Children: n.Children.Merge(other.Children), + ID: id, + Topology: topology, + Counters: n.Counters.Merge(other.Counters), + Sets: n.Sets.Merge(other.Sets), + Adjacency: n.Adjacency.Merge(other.Adjacency), + Latest: n.Latest.Merge(other.Latest), + Metrics: n.Metrics.Merge(other.Metrics), + Parents: n.Parents.Merge(other.Parents), + Children: n.Children.Merge(other.Children), } } @@ -213,11 +199,6 @@ func (n *Node) UnsafeUnMerge(other Node) bool { // We either keep a whole section or drop it if anything changed // - a trade-off of some extra data size in favour of faster simpler code. // (in practice, very few values reported by Scope probes do change over time) - if n.LatestControls.EqualIgnoringTimestamps(other.LatestControls) { - n.LatestControls = nil - } else { - remove = false - } if n.Latest.EqualIgnoringTimestamps(other.Latest) { n.Latest = nil } else { From c88be40b193e824c91ae3df8054d0227c6da3363 Mon Sep 17 00:00:00 2001 From: Bryan Boreham Date: Mon, 14 Oct 2019 17:40:24 +0000 Subject: [PATCH 2/5] performance: Update plugins to new-style controls data --- probe/plugins/registry.go | 10 +++++----- probe/plugins/registry_internal_test.go | 11 +++-------- 2 files changed, 8 insertions(+), 13 deletions(-) diff --git a/probe/plugins/registry.go b/probe/plugins/registry.go index 202f476203..132dda94ef 100644 --- a/probe/plugins/registry.go +++ b/probe/plugins/registry.go @@ -252,8 +252,8 @@ func (r *Registry) updateAndGetControlsInTopology(pluginID string, topology *rep for name, node := range topology.Nodes { log.Debugf("plugins: checking node controls in node %s of %s", name, topology.Label) newNode := node.WithID(name) - newLatestControls := report.MakeNodeControlDataLatestMap() - node.LatestControls.ForEach(func(controlID string, ts time.Time, data report.NodeControlData) { + newLatestControls := []string{} + for _, controlID := range node.ActiveControls() { log.Debugf("plugins: got node control %s", controlID) newControlID := "" if _, found := topology.Controls[controlID]; !found { @@ -263,9 +263,9 @@ func (r *Registry) updateAndGetControlsInTopology(pluginID string, topology *rep newControlID = fakeControlID(pluginID, controlID) log.Debugf("plugins: will replace node control %s with %s", controlID, newControlID) } - newLatestControls = newLatestControls.Set(newControlID, ts, data) - }) - newNode.LatestControls = newLatestControls + newLatestControls = append(newLatestControls, newControlID) + } + newNode = newNode.WithLatestActiveControls(newLatestControls...) newNodes[newNode.ID] = newNode } topology.Controls = newControls diff --git a/probe/plugins/registry_internal_test.go b/probe/plugins/registry_internal_test.go index b47916818d..1259bf6ef6 100644 --- a/probe/plugins/registry_internal_test.go +++ b/probe/plugins/registry_internal_test.go @@ -627,14 +627,9 @@ func checkControls(t *testing.T, topology report.Topology, expectedControls, exp if !found { t.Fatalf("expected a node %s in a topology", nodeID) } - actualNodeControls := []string{} - node.LatestControls.ForEach(func(controlID string, _ time.Time, _ report.NodeControlData) { - actualNodeControls = append(actualNodeControls, controlID) - }) - nodeControlsSet := report.MakeStringSet(expectedNodeControls...) - actualNodeControlsSet := report.MakeStringSet(actualNodeControls...) - if !reflect.DeepEqual(nodeControlsSet, actualNodeControlsSet) { - t.Fatalf("node controls in node %s in topology %s are not equal:\n%s", nodeID, topology.Label, test.Diff(nodeControlsSet, actualNodeControlsSet)) + actualNodeControls := node.ActiveControls() + if !reflect.DeepEqual(expectedNodeControls, actualNodeControls) { + t.Fatalf("node controls in node %s in topology %s are not equal:\n%s", nodeID, topology.Label, test.Diff(expectedNodeControls, actualNodeControls)) } } From eb381f167d11b1804b9630620e1527ec9c39d609 Mon Sep 17 00:00:00 2001 From: Bryan Boreham Date: Mon, 14 Oct 2019 16:14:21 +0000 Subject: [PATCH 3/5] refactor: move Report backwards-compatibility code into its own file Makes report.go a little easier to read. --- report/backcompat.go | 102 +++++++++++++++++++++++++++++++++++++++++++ report/report.go | 99 ----------------------------------------- 2 files changed, 102 insertions(+), 99 deletions(-) create mode 100644 report/backcompat.go diff --git a/report/backcompat.go b/report/backcompat.go new file mode 100644 index 0000000000..28ca45932c --- /dev/null +++ b/report/backcompat.go @@ -0,0 +1,102 @@ +package report + +// Backwards-compatibility: code to read older reports and convert + +// Upgrade returns a new report based on a report received from the old probe. +// +func (r Report) Upgrade() Report { + return r.upgradePodNodes().upgradeNamespaces().upgradeDNSRecords() +} + +func (r Report) upgradePodNodes() Report { + // At the same time the probe stopped reporting replicasets, + // it also started reporting deployments as pods' parents + if len(r.ReplicaSet.Nodes) == 0 { + return r + } + + // For each pod, we check for any replica sets, and merge any deployments they point to + // into a replacement Parents value. + nodes := Nodes{} + for podID, pod := range r.Pod.Nodes { + if replicaSetIDs, ok := pod.Parents.Lookup(ReplicaSet); ok { + newParents := pod.Parents.Delete(ReplicaSet) + for _, replicaSetID := range replicaSetIDs { + if replicaSet, ok := r.ReplicaSet.Nodes[replicaSetID]; ok { + if deploymentIDs, ok := replicaSet.Parents.Lookup(Deployment); ok { + newParents = newParents.Add(Deployment, deploymentIDs) + } + } + } + // newParents contains a copy of the current parents without replicasets, + // PruneParents().WithParents() ensures replicasets are actually deleted + pod = pod.PruneParents().WithParents(newParents) + } + nodes[podID] = pod + } + r.Pod.Nodes = nodes + + return r +} + +func (r Report) upgradeNamespaces() Report { + if len(r.Namespace.Nodes) > 0 { + return r + } + + namespaces := map[string]struct{}{} + for _, t := range []Topology{r.Pod, r.Service, r.Deployment, r.DaemonSet, r.StatefulSet, r.CronJob} { + for _, n := range t.Nodes { + if state, ok := n.Latest.Lookup(KubernetesState); ok && state == "deleted" { + continue + } + if namespace, ok := n.Latest.Lookup(KubernetesNamespace); ok { + namespaces[namespace] = struct{}{} + } + } + } + + nodes := make(Nodes, len(namespaces)) + for ns := range namespaces { + // Namespace ID: + // Probes did not use to report namespace ids, but since creating a report node requires an id, + // the namespace name, which is unique, is passed to `MakeNamespaceNodeID` + namespaceID := MakeNamespaceNodeID(ns) + nodes[namespaceID] = MakeNodeWith(namespaceID, map[string]string{KubernetesName: ns}) + } + r.Namespace.Nodes = nodes + + return r +} + +func (r Report) upgradeDNSRecords() Report { + // For release 1.11.6, probes accidentally sent DNS records labeled "nodes". + // Translate the incorrect version here. Accident was in commit 951629a. + if len(r.BugDNS) > 0 { + r.DNS = r.BugDNS + r.BugDNS = nil + } + if len(r.DNS) > 0 { + return r + } + dns := make(DNSRecords) + for endpointID, endpoint := range r.Endpoint.Nodes { + _, addr, _, ok := ParseEndpointNodeID(endpointID) + snoopedNames, foundS := endpoint.Sets.Lookup(SnoopedDNSNames) + reverseNames, foundR := endpoint.Sets.Lookup(ReverseDNSNames) + if ok && (foundS || foundR) { + // Add address and names to report-level map + if existing, found := dns[addr]; found { + var sUnchanged, rUnchanged bool + snoopedNames, sUnchanged = snoopedNames.Merge(existing.Forward) + reverseNames, rUnchanged = reverseNames.Merge(existing.Reverse) + if sUnchanged && rUnchanged { + continue + } + } + dns[addr] = DNSRecord{Forward: snoopedNames, Reverse: reverseNames} + } + } + r.DNS = dns + return r +} diff --git a/report/report.go b/report/report.go index e47800b834..38c502b165 100644 --- a/report/report.go +++ b/report/report.go @@ -478,105 +478,6 @@ func (r Report) DropTopologiesOver(limit int) Report { return r } -// Upgrade returns a new report based on a report received from the old probe. -// -func (r Report) Upgrade() Report { - return r.upgradePodNodes().upgradeNamespaces().upgradeDNSRecords() -} - -func (r Report) upgradePodNodes() Report { - // At the same time the probe stopped reporting replicasets, - // it also started reporting deployments as pods' parents - if len(r.ReplicaSet.Nodes) == 0 { - return r - } - - // For each pod, we check for any replica sets, and merge any deployments they point to - // into a replacement Parents value. - nodes := Nodes{} - for podID, pod := range r.Pod.Nodes { - if replicaSetIDs, ok := pod.Parents.Lookup(ReplicaSet); ok { - newParents := pod.Parents.Delete(ReplicaSet) - for _, replicaSetID := range replicaSetIDs { - if replicaSet, ok := r.ReplicaSet.Nodes[replicaSetID]; ok { - if deploymentIDs, ok := replicaSet.Parents.Lookup(Deployment); ok { - newParents = newParents.Add(Deployment, deploymentIDs) - } - } - } - // newParents contains a copy of the current parents without replicasets, - // PruneParents().WithParents() ensures replicasets are actually deleted - pod = pod.PruneParents().WithParents(newParents) - } - nodes[podID] = pod - } - r.Pod.Nodes = nodes - - return r -} - -func (r Report) upgradeNamespaces() Report { - if len(r.Namespace.Nodes) > 0 { - return r - } - - namespaces := map[string]struct{}{} - for _, t := range []Topology{r.Pod, r.Service, r.Deployment, r.DaemonSet, r.StatefulSet, r.CronJob} { - for _, n := range t.Nodes { - if state, ok := n.Latest.Lookup(KubernetesState); ok && state == "deleted" { - continue - } - if namespace, ok := n.Latest.Lookup(KubernetesNamespace); ok { - namespaces[namespace] = struct{}{} - } - } - } - - nodes := make(Nodes, len(namespaces)) - for ns := range namespaces { - // Namespace ID: - // Probes did not use to report namespace ids, but since creating a report node requires an id, - // the namespace name, which is unique, is passed to `MakeNamespaceNodeID` - namespaceID := MakeNamespaceNodeID(ns) - nodes[namespaceID] = MakeNodeWith(namespaceID, map[string]string{KubernetesName: ns}) - } - r.Namespace.Nodes = nodes - - return r -} - -func (r Report) upgradeDNSRecords() Report { - // For release 1.11.6, probes accidentally sent DNS records labeled "nodes". - // Translate the incorrect version here. Accident was in commit 951629a. - if len(r.BugDNS) > 0 { - r.DNS = r.BugDNS - r.BugDNS = nil - } - if len(r.DNS) > 0 { - return r - } - dns := make(DNSRecords) - for endpointID, endpoint := range r.Endpoint.Nodes { - _, addr, _, ok := ParseEndpointNodeID(endpointID) - snoopedNames, foundS := endpoint.Sets.Lookup(SnoopedDNSNames) - reverseNames, foundR := endpoint.Sets.Lookup(ReverseDNSNames) - if ok && (foundS || foundR) { - // Add address and names to report-level map - if existing, found := dns[addr]; found { - var sUnchanged, rUnchanged bool - snoopedNames, sUnchanged = snoopedNames.Merge(existing.Forward) - reverseNames, rUnchanged = reverseNames.Merge(existing.Reverse) - if sUnchanged && rUnchanged { - continue - } - } - dns[addr] = DNSRecord{Forward: snoopedNames, Reverse: reverseNames} - } - } - r.DNS = dns - return r -} - // Summary returns a human-readable string summarising the contents, for diagnostic purposes func (r Report) Summary() string { ret := "" From 635cea0b5614cdecc02e2aff51d8f00cb980cc9e Mon Sep 17 00:00:00 2001 From: Bryan Boreham Date: Mon, 14 Oct 2019 18:50:28 +0000 Subject: [PATCH 4/5] backwards-compatibility: unmarshall latestControls data from older probes With a test. --- report/backcompat.go | 51 ++++++++++++++++++++++++++++++++++++++++++ report/marshal_test.go | 50 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 101 insertions(+) diff --git a/report/backcompat.go b/report/backcompat.go index 28ca45932c..49724e27da 100644 --- a/report/backcompat.go +++ b/report/backcompat.go @@ -2,6 +2,57 @@ package report // Backwards-compatibility: code to read older reports and convert +import ( + "strings" + "time" + + "github.com/ugorji/go/codec" +) + +// For backwards-compatibility with probes that sent a map of latestControls data +type bcNode struct { + Node + LatestControls map[string]nodeControlDataLatestEntry `json:"latestControls,omitempty"` +} + +type nodeControlDataLatestEntry struct { + Timestamp time.Time `json:"timestamp"` + Value nodeControlData `json:"value"` +} + +type nodeControlData struct { + Dead bool `json:"dead"` +} + +// CodecDecodeSelf implements codec.Selfer +func (n *Node) CodecDecodeSelf(decoder *codec.Decoder) { + var in bcNode + decoder.Decode(&in) + *n = in.Node + if len(in.LatestControls) > 0 { + // Convert the map into a delimited string + cs := make([]string, 0, len(in.LatestControls)) + var ts time.Time + for name, v := range in.LatestControls { + if !v.Value.Dead { + cs = append(cs, name) + // Pull out the newest timestamp to use for the whole set + if ts.Before(v.Timestamp) { + ts = v.Timestamp + } + } + } + n.Latest = n.Latest.Set(NodeActiveControls, ts, strings.Join(cs, ScopeDelim)) + } +} + +type _Node Node // just so we don't recurse inside CodecEncodeSelf + +// CodecEncodeSelf implements codec.Selfer +func (n *Node) CodecEncodeSelf(encoder *codec.Encoder) { + encoder.Encode((*_Node)(n)) +} + // Upgrade returns a new report based on a report received from the old probe. // func (r Report) Upgrade() Report { diff --git a/report/marshal_test.go b/report/marshal_test.go index 08518a4956..0275341c7c 100644 --- a/report/marshal_test.go +++ b/report/marshal_test.go @@ -1,12 +1,14 @@ package report_test import ( + "bytes" "context" "reflect" "testing" "time" "github.com/weaveworks/common/mtime" + "github.com/weaveworks/common/test" "github.com/weaveworks/scope/report" s_reflect "github.com/weaveworks/scope/test/reflect" ) @@ -71,3 +73,51 @@ func TestBiggerRoundtrip(t *testing.T) { t.Errorf("%v != %v", r1, *r2) } } + +func TestControlsCompat(t *testing.T) { + testData := `{ + "Container": { + "nodes": { + "031d;": { + "id": "031d;", + "latest": { + "control_probe_id": { + "timestamp": "2019-10-14T14:36:01Z", + "value": "29b4f381044a89a3" + } + }, + "latestControls": { + "docker_attach_container": { + "timestamp": "2019-10-14T14:36:01Z", + "value": {"dead": true} + }, + "docker_remove_container": { + "timestamp": "2019-10-14T14:36:01Z", + "value": {"dead": false} + } + }, + "topology": "container" + } + }, + "shape": "hexagon" + } +}` + + nowTime := time.Date(2019, 10, 14, 14, 36, 1, 0, time.UTC) + mtime.NowForce(nowTime) + expected := report.MakeReport() + expected.Container.AddNode(report.MakeNode(report.MakeContainerNodeID("031d")). + WithTopology("container"). + WithLatestActiveControls("docker_remove_container"). + WithLatests(map[string]string{"control_probe_id": "29b4f381044a89a3"}), + ) + + buf := bytes.NewBufferString(testData) + rpt, err := report.MakeFromBinary(context.Background(), buf, false, false) + if err != nil { + t.Fatal(err) + } + if !s_reflect.DeepEqual(&expected, rpt) { + t.Error(test.Diff(&expected, rpt)) + } +} From 1dcdfab05af2c1991b29a523e13d82e19be022a4 Mon Sep 17 00:00:00 2001 From: Bryan Boreham Date: Mon, 13 Jan 2020 09:04:55 +0000 Subject: [PATCH 5/5] fixup: from review feedback Fix a logic error in ECS scale-down button, bad copy/paste in ActiveControls() and neaten the switch cases in container controls. Co-Authored-By: Filip Barl --- probe/awsecs/reporter.go | 2 +- probe/docker/container.go | 5 ++--- report/node.go | 2 +- 3 files changed, 4 insertions(+), 5 deletions(-) diff --git a/probe/awsecs/reporter.go b/probe/awsecs/reporter.go index bba0a81607..13830c7e80 100644 --- a/probe/awsecs/reporter.go +++ b/probe/awsecs/reporter.go @@ -153,7 +153,7 @@ func (r Reporter) Tag(rpt report.Report) (report.Report, error) { activeControls := []string{ScaleUp} // Disable ScaleDown when only 1 task is desired, since // scaling down to 0 would cause the service to disappear (#2085) - if service.DesiredCount < 1 { + if service.DesiredCount > 1 { activeControls = append(activeControls, ScaleDown) } rpt.ECSService.AddNode(report.MakeNodeWith(serviceID, map[string]string{ diff --git a/probe/docker/container.go b/probe/docker/container.go index e42f707e06..da27ac6d44 100644 --- a/probe/docker/container.go +++ b/probe/docker/container.go @@ -391,13 +391,12 @@ func (c *container) getBaseNode() report.Node { // Return a slice including all controls that should be shown on this container func (c *container) controls() []string { - paused := c.container.State.Paused switch { - case paused: + case c.container.State.Paused: return []string{UnpauseContainer} case c.container.State.Running: return []string{RestartContainer, StopContainer, PauseContainer, AttachContainer, ExecContainer} - case !c.container.State.Running: + default: return []string{StartContainer, RemoveContainer} } return nil diff --git a/report/node.go b/report/node.go index 9883cd7e23..cd13982212 100644 --- a/report/node.go +++ b/report/node.go @@ -124,7 +124,7 @@ func (n Node) WithLatestActiveControls(cs ...string) Node { } // ActiveControls returns a string slice with the names of active controls. -func (n Node) ActiveControls(cs ...string) []string { +func (n Node) ActiveControls() []string { activeControls, _ := n.Latest.Lookup(NodeActiveControls) return strings.Split(activeControls, ScopeDelim) }