add k8s combined view #2552

ekimekim · 2017-06-01T00:52:26Z

This adds a 'combined' kubernetes view that shows each pod at the 'highest level of abstraction', as follows:
* Any daemonsets the pod is in
* Any deployments the pod is in
* Any replica-sets the pod is in that do not map to a deployment
* If none of the above, the bare pod itself

Along the way, it fixes #2525 by removing the omitempty for the default value, further generalises render.Map2Parent, and introduces a new Renderer type for combining a set of nodes with a set containing the same nodes with more information, but with the output set of nodes only being the original set (but with the extra information), unlike Reduce which takes a union.

Still to do:
* How does the table view work in this setup?
* We want to assign shapes to the different topologies present to differentiate them graphically

ekimekim · 2017-06-01T00:58:12Z

cc @rade

rade · 2017-06-02T07:46:15Z

I've given this a whirl by feeding it 30s of recent production reports. When selecting the kube-system namespace, all the scope probes are shown as individual pods rather than a single daemonset. So something isn't working right.

ekimekim · 2017-06-06T18:28:42Z

That would be because something's wrong with the probes in prod, they are reporting no daemonsets:

$ jq '.DaemonSet' </tmp/report.json 
{
  "shape": "heptagon",
  "label": "daemonset",
  "label_plural": "daemonsets",
  "nodes": {}
}

I'm willing to bet this is because they're running an old version.
Yep. f3a9b61c in prod.

Dev is running 0ee3a3fa which is the commit that introduced daemonsets to the probe. And indeed it works with that report:

rade · 2017-06-06T18:33:41Z

D'oh; I should have figured that out myself.

Mind doing a rebase?

ekimekim · 2017-06-06T20:19:27Z

I'm having a lot of trouble understanding how the table code works, but it seems that currently it's combining auto-detected info from all listed nodes to pick a list of columns. This is probably a reasonable good-enough behaviour for now. Just waiting on @jpellizzari for shapes code - in the meantime @rade could you review?

jml · 2017-06-07T11:26:25Z

Scope Kubernetes View

jpellizzari · 2017-06-07T17:28:51Z

@ekimekim This commit will add the shapes to the UI: 7aa6ebd

Do you want to cherry-pick that into your branch?

ekimekim · 2017-06-07T18:33:01Z

done.

client/app/scripts/charts/node.js

@@ -14,27 +14,27 @@ import { NODE_BASE_SIZE } from '../constants/styles';
 import NodeShapeStack from './node-shape-stack';
 import NodeNetworksOverlay from './node-networks-overlay';
 import {
-  NodeShapeCloud,
  NodeShapeCircle,


rade · 2017-06-08T02:59:21Z

Feedback on current functionality:

We had decided that replicasets should go completely; currently they are still shown - as a separate topology, as part of the 'controllers' topology, as children of deployments, as parents of pods.
I cannot see the new shapes; everything's still a heptagon
'controllers' should be the default view, instead of 'services', since it typically gives a more complete view of the system
the table mode for 'controllers' should contain a column indicating the type (i.e. 'deployment', 'daemonset', ...)
as discussed, we should enable searching by type, which in turn would allow us to...
remove the type filter

ekimekim · 2017-06-08T11:38:24Z

- We had decided that replicasets should go completely; currently they are still shown - as a separate topology, as part of the 'controllers' topology, as children of deployments, as parents of pods. todo. removing replica sets entirely is non trivial - I cannot see the new shapes; everything's still a heptagon This comes from the report, you won't see new shapes using prod reports. - 'controllers' should be the default view, instead of 'services', since it typically gives a more complete view of the system TODO and trivial - the table mode for 'controllers' should contain a column indicating the type (i.e. 'deployment', 'daemonset', ...) TODO, may be tricky since Topology isn't a Latest key. - as discussed, we should enable searching by type, which in turn would allow us to... - remove the type filter latter is trivial once former is done. former is frontend I believe, i'll get jordan on it

rade · 2017-06-08T12:25:02Z

removing replica sets entirely is non trivial

In which case let's leave them as a separate topology for now, and not show them as part of the 'controllers' topology.

To be clear, getting rid of them is important, but we can do it as a follow-up issue.

report/report.go

 	Hexagon  = "hexagon"
+	Heptagon = "heptagon"
+	Octogon  = "octogon"


report/report.go

@@ -155,7 +158,7 @@ func MakeReport() Report {
 			WithLabel("host", "hosts"),

 		Pod: MakeTopology().
-			WithShape(Heptagon).
+			WithShape(Octogon).


report/report.go

@@ -167,11 +170,11 @@ func MakeReport() Report {
 			WithLabel("deployment", "deployments"),

 		ReplicaSet: MakeTopology().
-			WithShape(Heptagon).
+			WithShape(Pentagon).


ekimekim · 2017-06-08T12:42:03Z

It's also the most distinct. I want to avoid having hexagons, heptagons and octegons on the same screen since they're hard to tell apart at a glance.

…

On 8 June 2017 at 05:32, Matthias Radestock ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In report/report.go <#2552 (comment)>: > @@ -167,11 +170,11 @@ func MakeReport() Report { WithLabel("deployment", "deployments"), ReplicaSet: MakeTopology(). - WithShape(Heptagon). + WithShape(Pentagon). I'd make that a triangle since it's the most weird-looking shape and we are going to get rid of replicasets soon. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2552 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAiyxL8LyeK4E20pLbowLkO6ztAlU875ks5sB-nogaJpZM4NsXhM> .

rade · 2017-06-08T12:54:59Z

I want to avoid having hexagons, heptagons and octegons on the same screen since they're hard to tell apart at a glance.

You are not going to be able to avoid that once we add stateful sets and jobs, though I'd be happy to use triangles for the latter.

rade · 2017-06-09T16:20:52Z

In which case let's leave [replica sets] as a separate topology for now, and not show them as part of the 'controllers' topology.

The 2nd part hasn't been done. Looking at the code, that should simplify things a fair bit.

ekimekim · 2017-06-12T16:44:22Z

Addressed everything but the type search, type filter, and type name in table view. working on the latter now.

rade

Left some questions.

I am still thinking of better ways of constructing the renderer.

And I am having second thoughts on the shapes.

Other than that this looks fine.

render/detailed/summary.go

+var podGroupNodeTypeName = map[string]string{
+	report.Deployment: "Deployment",
+	report.DaemonSet:  "Daemon Set",
+	report.ReplicaSet: "Replica Set",


render/pod.go

+// have connections to each other.
+// We combine with all the full topologies using ReduceFirstOnly, which keeps the same
+// set of nodes but merges in the full data from the other renderers.
+var KubeControllerRenderer = ConditionalRenderer(renderKubernetesTopologies,


ekimekim · 2017-06-16T21:50:45Z

Ok, I'm locking off this branch. @rade please give final review then we can merge. Meanwhile I'll start getting rid of Replica Sets entirely in a seperate branch.

rade

Nearly there. Some tweaks needed.

Also, please benchmark this using the technique from #2559 (comment).

render/detailed/summary.go

+var podGroupNodeTypeName = map[string]string{
+	report.Deployment: "Deployment",
+	report.DaemonSet:  "Daemon Set",
+	report.ReplicaSet: "Replica Set",


app/api_topologies.go

@@ -343,7 +345,7 @@ func (a byName) Less(i, j int) bool { return a[i].Name < a[j].Name }
 type APITopologyOptionGroup struct {
 	ID string `json:"id"`
 	// Default value for the UI to adopt. NOT used as the default if the value is omitted, allowing "" as a distinct value.
-	Default string              `json:"defaultValue,omitempty"`
+	Default string              `json:"defaultValue"`


render/pod.go

+				),
+			),
+		},
+		selectors...,


render/pod.go

+			case NoParentsDrop:
+				// Do nothing, we will return an empty result
+			default:
+				panic(fmt.Sprintf("Map2Parent got bad noParentsAction %v", noParentsAction))


probe/kubernetes/daemonset.go

@@ -48,5 +48,6 @@ func (d *daemonSet) GetNode() report.Node {
 		DesiredReplicas:      fmt.Sprint(d.Status.DesiredNumberScheduled),
 		Replicas:             fmt.Sprint(d.Status.CurrentNumberScheduled),
 		MisscheduledReplicas: fmt.Sprint(d.Status.NumberMisscheduled),
+		NodeType:             "Daemon Set",


render/pod.go

-			MakeMap(
-				Map2Parent(report.DaemonSet, "", nil),
+// KubeControllerRenderer is a Renderer which combines all the 'controller' topologies.
+// We first map pods to all possible things they can map to, then we map again to


render/render.go

+	// we probably only care about the filter stats for the 'main' renderer.
+	return r.first.Stats(rpt, dct)
+}
+


app/api_topologies.go

@@ -196,6 +195,17 @@ func MakeRegistry() *Registry {
 		},
 	}

+	k8sControllersTypeFilter := APITopologyOptionGroup{
+		ID:         "grouptype",
+		Default:    "",


ekimekim · 2017-06-20T20:15:38Z

Addressed everything except the continuing saga of #2525, which I'm looking into now.

render/pod.go

@@ -109,16 +107,14 @@ func renderParents(childTopology string, parentTopologies []string, noParentsAct
 	}
 	// weird append form is to accomplish MakeReduce(subrenderer, selectors...) since that form isn't allowed
 	return MakeReduce(append(
-		[]Renderer{
+		selectors,


ekimekim · 2017-06-21T23:02:20Z

I've split off the #2525 fixes into #2633

ekimekim · 2017-06-21T23:04:50Z

Rebased and resolved minor conflict. PTAL?

ekimekim · 2017-06-21T23:05:26Z

Note it's a known issue that the type selector will have that minor display bug until #2633 lands

rade · 2017-06-23T07:35:51Z

LGTM but still needs benchmarking as per #2552 (review). Also please selectively squash some of the commits.

ekimekim · 2017-06-26T21:36:20Z

Perf results. Note I adapted the methodology a bit - my numbers were tiny and highly variable, so I did 6 runs: The first I discarded on the premise that it would prime any caches that may affect the result. I then took the top and bottom results and discarded them as outliers, and recorded the second-top and second-bottom. ie. I recorded the 20th and 80th percentiles. All times are in milliseconds.

type	before	after
processes	79 - 117	49 - 124
containers	151 - 215	148 - 185
pods	31 - 51	36 - 45
services	22 - 45	26 - 45
hosts	17 - 29	11 - 30

Based on the ranges of the results and the high variability, I don't think any of the changes are significant.

ekimekim · 2017-06-26T21:37:26Z

@rade final lgtm to merge?

ekimekim · 2017-06-26T22:27:13Z

or not, since there seems to be a test failing that make tests isn't hitting locally...will investigate tomorrow.

rade · 2017-06-27T01:01:33Z

Perf results. Note I adapted the methodology a bit - my numbers were tiny and highly variable, so I did 6 runs: The first I discarded on the premise that it would prime any caches that may affect the result.

All you are testing with this approach is memoization. You must restart scope after every single test. See #2613 for better instructions. Apologies for not being clear about this rather crucial detail.

rade · 2017-06-27T02:14:38Z

I've run the perf tests...

	old	new
processes	0.59	0.59
containers	1.6	1.6
pods	1.5	1.5
deployments	1.5
kube-controllers		1.5
services	1.5	1.5
hosts	2.0	2.1

(all figures in seconds, averaged over three runs)

So hosts is marginally slower. Rest is the same. LGTM. Just the test failures to go then.

The idea is that this view shows all 'pods or groups of pods' at 'the highest level of abstraction'. For now, we just show daemonsets and deployments.

…or deployment) This is a union set, so it will be suitable even as we continue to add more node types to this view.

The existing technique of "reducing" the two rendered graphs for daemonsets and deployments had a glaring issue that no connections would ever be made between nodes of different types, since that information would've been discarded earlier in the process. It also makes it hard to identify "parentless" pods. This commit extends the Map2Parent function, teaching it: * To check multiple topologies for parents * To pass through nodes with no parents found without modification Since we already had two 'modes' for what to do with nodes without parents, and it would've been clunky to try to encode the third option into the existing PseudoNodeID arg in some way, we instead split it into two args, with the first being an enum specifying either the old pseudo node behaviour, the old drop behaviour, or the new keep behaviour. We then use the new Map2Parent to map pods to: * A replica set, if it has one * A daemonset, if it has one * Itself, if neither of the above and then map again from the results to any deployment, leaving as-is any nodes that don't map to a deployment. Hence we are left with: * Deployments * Daemonsets * Replica sets, but only if they map to no deployment * Pods, but only if they map to none of the above and connections between all these will be calculated correctly.

While we're there, adopt a consistent ordering for all places that shapes are listed Order is least sides to most sides, with circle before polygons, and complex shapes (currently just Cloud) after. On shape choices for topologies: * Since the k8s logo is a heptagon, we want pods to be heptagons. * Since triangle is 'a bit weird', we put it on the least-important type, replica sets. * Pentagons look a little weirder than octogons (it's the lack of symmetry) so we put octogons on the most common (deployments)

Since there are multiple types in the same topology, displaying the type is important. We do this in multiple places: * Add node type to minor label * Add node type as metadata and include in metadata template. Even though this will always be the same for every node of that topology, this was the easiest way to add it so it displays in the table view. Note we can't control ordering of columns in table view, it's always alphabetical.

… and remove bare pods and replica sets Since we still need to map through replica sets to find matching deployments, we simply filter them out as a post-step.

* Maps metrics if there is a single pod in the controller, as per all other views. * Also added heavy commenting on the increasingly-complex render chain

Since all the renderers were doing almost-exactly the same thing, we abstract that out into a common function.

ekimekim · 2017-06-27T17:20:07Z

Looks like the broken test is a problem on the version of master I happened to rebase onto. I've just rebased onto a newer version of master and it should work this time.

ekimekim requested a review from 2opremio June 1, 2017 00:58

ekimekim force-pushed the mike/k8s/combined-view branch 2 times, most recently from 327bdb5 to 87a2655 Compare June 1, 2017 01:50

rade mentioned this pull request Jun 6, 2017

Pods subnavigation occupies lots of vertical space #2566

Closed

ekimekim force-pushed the mike/k8s/combined-view branch from 87a2655 to 0f40b02 Compare June 6, 2017 18:41

ekimekim force-pushed the mike/k8s/combined-view branch from 4b4dcbb to 5ff8d63 Compare June 7, 2017 18:39

ekimekim requested a review from rade June 7, 2017 19:16

jpellizzari reviewed Jun 7, 2017

View reviewed changes

rade mentioned this pull request Jun 8, 2017

hybrid view of multiple topologies #552

Open

rade reviewed Jun 8, 2017

View reviewed changes

report/report.go Outdated

Hexagon = "hexagon"

Heptagon = "heptagon"

Octogon = "octogon"

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

rade reviewed Jun 8, 2017

View reviewed changes

report/report.go Outdated

@@ -155,7 +158,7 @@ func MakeReport() Report {

WithLabel("host", "hosts"),

Pod: MakeTopology().

WithShape(Heptagon).

WithShape(Octogon).

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

rade reviewed Jun 8, 2017

View reviewed changes

report/report.go Outdated

@@ -167,11 +170,11 @@ func MakeReport() Report {

WithLabel("deployment", "deployments"),

ReplicaSet: MakeTopology().

WithShape(Heptagon).

WithShape(Pentagon).

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

ekimekim force-pushed the mike/k8s/combined-view branch from 65dc397 to 93a170b Compare June 8, 2017 18:58

ekimekim force-pushed the mike/k8s/combined-view branch from 2f7932e to e3115bb Compare June 12, 2017 17:14

rade reviewed Jun 12, 2017

View reviewed changes

rade reviewed Jun 17, 2017

View reviewed changes

rade mentioned this pull request Jun 19, 2017

combined, comprehensive kubernetes views #2620

Closed

7 tasks

rade reviewed Jun 20, 2017

View reviewed changes

ekimekim force-pushed the mike/k8s/combined-view branch from 8d4033a to 71db37e Compare June 21, 2017 23:04

ekimekim force-pushed the mike/k8s/combined-view branch from 71db37e to 5762ad7 Compare June 26, 2017 20:51

ekimekim and others added 9 commits June 27, 2017 10:19

Add new view for 'combined' daemonset/deployment view

6dae014

The idea is that this view shows all 'pods or groups of pods' at 'the highest level of abstraction'. For now, we just show daemonsets and deployments.

Add an option group to Combined view to select group type (daemonset …

d0cbf47

…or deployment) This is a union set, so it will be suitable even as we continue to add more node types to this view.

Remove deployment and daemonset views, change combined to controllers…

8eb72cf

… and remove bare pods and replica sets Since we still need to map through replica sets to find matching deployments, we simply filter them out as a post-step.

Prefer controllers view for default view

ec2a7fc

controllers topology: Map metrics for single nodes

f101f83

* Maps metrics if there is a single pod in the controller, as per all other views. * Also added heavy commenting on the increasingly-complex render chain

Rewrite renderers for k8s/ecs/swarm to abstract common parts

f7913ab

Since all the renderers were doing almost-exactly the same thing, we abstract that out into a common function.

ekimekim force-pushed the mike/k8s/combined-view branch from 5762ad7 to f7913ab Compare June 27, 2017 17:19

ekimekim merged commit 716a5df into master Jun 27, 2017

ekimekim deleted the mike/k8s/combined-view branch June 27, 2017 18:09

rade added this to the 1.6 milestone Jul 12, 2017

add k8s combined view #2552

add k8s combined view #2552

Conversation

ekimekim commented Jun 1, 2017 • edited Loading

ekimekim commented Jun 1, 2017

rade commented Jun 2, 2017

ekimekim commented Jun 6, 2017

rade commented Jun 6, 2017

ekimekim commented Jun 6, 2017

jml commented Jun 7, 2017

jpellizzari commented Jun 7, 2017

ekimekim commented Jun 7, 2017

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

rade commented Jun 8, 2017

ekimekim commented Jun 8, 2017 via email • edited Loading

rade commented Jun 8, 2017

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

ekimekim commented Jun 8, 2017 via email

rade commented Jun 8, 2017

rade commented Jun 9, 2017

ekimekim commented Jun 12, 2017

rade left a comment

Choose a reason for hiding this comment

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

ekimekim commented Jun 16, 2017

rade left a comment

Choose a reason for hiding this comment

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

ekimekim commented Jun 20, 2017

This comment was marked as abuse.

ekimekim commented Jun 21, 2017

ekimekim commented Jun 21, 2017

ekimekim commented Jun 21, 2017

rade commented Jun 23, 2017

ekimekim commented Jun 26, 2017

ekimekim commented Jun 26, 2017

ekimekim commented Jun 26, 2017

rade commented Jun 27, 2017 • edited Loading

rade commented Jun 27, 2017 • edited Loading

ekimekim commented Jun 27, 2017

ekimekim commented Jun 1, 2017 •

edited

Loading

ekimekim commented Jun 8, 2017 via email •

edited

Loading

rade commented Jun 27, 2017 •

edited

Loading

rade commented Jun 27, 2017 •

edited

Loading