Add pv and pvc support in weave scope #3132

satyamz · 2018-04-05T19:39:57Z

As a part of this, a new sub-topology called Persistent Volumes which contain the graph of Persistent Volume and Persistent Volume Claim.
Edges are drawn on the basis of Volume Claim Name in PV and PVC.

Fixes: #3130

rade · 2018-04-10T05:00:17Z

This is great! Good effort!

Is there anything showing which pods use which claims? Would that be useful? Which makes me wonder whether persistent volumes and claims should be shown in the existing k8s views rather than getting their own view. Or, as a stepping stone, showing lists of volumes on the pod/controller detail panels, and linking to the PV node details and vice versa.

Why do the PVCs in your example have such cryptic names?

Could you post a screen shot of the table mode, please?

Does the namespace selector apply to PVCs?

Does searching/filtering provide reasonable results?

render/detailed/summary.go

+	base = addKubernetesLabelAndRank(base, n)
+	return base
+}
+


probe/kubernetes/persistentvolumeclaim.go

+	return p.MetaNode(report.MakePersistentVolumeClaimNodeID(p.UID())).WithLatests(map[string]string{
+		report.ControlProbeID: probeID,
+		NodeType:              "Persistent Volume Claim",
+		Namespace:             p.GetNamespace(),


probe/kubernetes/reporter.go

+		Status:      {ID: Status, Label: "Status", From: report.FromLatest, Priority: 3},
+		VolumeName:  {ID: VolumeName, Label: "Volume", From: report.FromLatest, Priority: 4},
+		AccessModes: {ID: AccessModes, Label: "Access Modes", From: report.FromLatest, Priority: 5},
+	}


render/persistentvolume.go

+	for _, n := range inputNodes {
+		pvcName := c.toPV(n)
+		pvNodes[pvcName] = n.ID
+	}


render/persistentvolume.go

+
+	return MapStorageEndpoints(
+		func(m report.Node) string {
+			pvcName, ok := m.Latest.Lookup(kubernetes.Name)


render/persistentvolume.go

+}
+
+// Map2PVCName returns PVC name for the given Pod.
+func Map2PVCName(m report.Node) string {


render/persistentvolume.go

+
+// Map2PVCName returns PVC name for the given Pod.
+func Map2PVCName(m report.Node) string {
+	pvcName, ok := m.Latest.Lookup(kubernetes.VolumeClaim)


render/persistentvolume.go

+}
+
+// Map2SC returns pvc node ID
+func Map2SC(n report.Node) string {


render/persistentvolume.go

+		MapEndpoints(
+			Map2SC,
+			report.PersistentVolumeClaim,
+		)))


render/render.go

+	}
+	return Nodes{Nodes: ret.nodes}
+}
+


test/fixture/report_fixture.go

@@ -354,12 +358,48 @@ var (
 		},
 		Window: 2 * time.Second,
 	}
+	PVReport = report.Report{


rade · 2018-04-10T06:32:33Z

Couldn't the edges from PVCs to PVs be added by the probes, thus obviating the need for any special rendering code?

satyamz · 2018-04-11T08:00:59Z

Is there anything showing which pods use which claims? Would that be useful? Which makes me wonder whether persistent volumes and claims should be shown in the existing k8s views rather than getting their own view. Or, as a stepping stone, showing lists of volumes on the pod/controller detail panels, and linking to the PV node details and vice versa.

Pods has VolumeClaim in the Spec. We are using the same to retrieve PVC for the given Pod. However, this change is not included in this PR. We're sending another PR for that as soon as this PR is merged (just to reduce the size of PR we've split it in two).

Regarding keeping PV, PVC along with pods, We want to show complete kubernetes storage support eventually. Another reason for this can be, for storage relations we do not need any networking based linking. Hence, We have our own storage specific and simple renderer for PV, PVC objects.
This is how final view looks:

Cylinder : PV
DottedCylinder: PVC
Sheet : Storage Class (SC)

Why do the PVCs in your example have such cryptic names?

These are not PVCs. These are PVs. Names for the PV is generated by K8s storage controller. You can check the same https://github.com/kubernetes-incubator/external-storage/blob/e3789b84521b5e75f161a6885010d2b7b2f50033/lib/controller/controller.go#L1160:34

Could you post a screen shot of the table mode, please?

Does the namespace selector apply to PVCs?

Yes! PVCs can have Namespace but PVs, Storage Classes don't have Namespace. We're still not clear whether to use Namespace filter for PV sub-topology. Also, We're not able to remove the Namespace filter for PV sub-topology as Namespace filter is invoked on the basis of topology and not sub-topology. I would like to know your opinion on this.

Does searching/filtering provide reasonable results?

Yes! Searching works properly. However, we're not able to apply Namespace filters.

rade · 2018-04-11T08:32:11Z

Ah, I thought the dotted cylinders were PVs, and the solid ones PVCs, since the latter have "pvc-..." names in your screenshots.

IMO the pvc-pv edge has the wrong polarity. The PVC references the PV, so the arrow should go from the former to the latter. Then, by extension, we get pod -> pvc -> pv.

Regarding having a separate view... views with pods get crowded very quickly since that is the granularity at which things get scaled in k8s. Surely a user would want to see what PVs their workloads (i.e. groups of pods managed by a controller) are accessing. So IMO PVs and PVCs should go in the Pod and Controller views, possible with a selector that lets users decide whether they want to see them.

satyamz · 2018-04-11T11:01:29Z

IMO the pvc-pv edge has the wrong polarity. The PVC references the PV, so the arrow should go from the former to the latter. Then, by extension, we get pod -> pvc -> pv.

Fixing this and other suggested changes. Will push all soon.

Regarding having a separate view... views with pods get crowded very quickly since that is the granularity at which things get scaled in k8s. Surely a user would want to see what PVs their workloads (i.e. groups of pods managed by a controller) are accessing. So IMO PVs and PVCs should go in the Pod and Controller views, possible with a selector that lets users decide whether they want to see them.
Merge state

Yes, I agree with you on the part where user would want to have separate storage view. Also, we already have PVC controllers and replicas in the PODS & CONTROLLERS section. As you are suggesting, we can add storage filter option there which will give mapping of stateful pods and PVC controller replicas only (no other pods). Indeed this is good suggestion, I will raise issue for this and will start work on this.

Having different storage view can help better storage management for K8s stateful workloads. Can we have new sub-topology under Hosts as Storage (below weave net) to show all the K8s storage components? As you know, we're finding storage relationship as pod->pvc->pv. In this phase, PV is the last components but in the Storage section we're extending this view to show which Disks are providing PV along with storage pool with opetations (in our scope term controls) such as snapshots, clone, backup etc. Hence, We're adding Persistent Volumes as a new sub-topology. We want to move this Persistent Volumes to Storage under Hosts once we've disks support. In that case, we can show disk and host relationship too. Storage topology will be big plus for weave scope wrt storage management. We are in the middle of development of Node Disk Manager (NDM) which we'll push to the Kubernetes. NDM will help us getting all the disk related info and operations handy.
If you're fine with this, we can move Persistent Volumes to the Storage section in this PR itself.

Thanks!

satyamz · 2018-04-13T08:21:19Z

Here's screenshot for PVC to PV mapping:

2opremio · 2018-05-10T07:16:42Z

@satyamz any chance of implementing the suggested changes anytime soon?

satyamz · 2018-05-10T12:33:15Z

@2opremio I am sending changes today.

satyamz · 2018-05-10T13:30:22Z

Hi @rade , @bboreham , @2opremio ,
Latest changes will remove Persistent Volumes sub topology from PODS topology and add K8s storage components in the storage filter in PODS topology as per suggestion from @rade

Storage Filter On:

Storage Filter Off:

Why isn't the existing result() method sufficient?

Currently, if we want to connect pod1 with pod2 based on network connection then we've network endpoints to do that. endpoints are working as a reference node & they have all the adjacencies. Further, we're passing endpoints nodes with adjacencies to result(). In storage we don't have any common reference node such as endpoint. Storage components are connected together directly through one to one mapping. Hence, we're introducing storageResult() to handle resource(PVC) to resource (PV) mapping.

Thanks

bboreham · 2018-05-21T17:06:10Z

@satyamz apologies I have been away from this for some time. I finally got my own version to work - see master...k8s-volumes

It uses lots of code from this PR, but the key thing I wanted to show is that it has virtually no changes on the rendering side. No new methods on joinResult and none of the code you used to feed things into it. I simply create the adjacency I want in the probe. I don't include probeID in the storage node; can't see any need for it.

I left in that branch a previous commit where the information goes via the parent field; this seemed plausible but in the end unnecessary.

What I suggest is you look at my branch, then simplify this PR based on those ideas. I left out all your tests, also the most recent changes, because all I wanted to do was understand the rendering/adjacency side.

satyamz · 2018-05-21T18:41:01Z

Hi @bboreham Thanks for the all the work and reply. I must say, you opened up my mind with these changes. I will reflect these changes in this PR and will push commits here soon.

Thanks a lot 👍

satyamz · 2018-05-23T11:06:09Z

Hi @bboreham,
I tried the solution given you. It works fine for PV to PVC adjacency. However, this method will not work for adding adjacency from pod to pvc. If we send PVC as a adjacency for pod from probe then that'll be deleted by the call connectionJoin() here and here.

If we want to add adjacency for pod -> pvc with the method proposed by you then we'll have to consider ClaimName in the pod spec to create PVC nodeID because in pod spec UID for PVC is not available, ClaimName is unique. Similarly for PVC to StorageClass adjacency, PVC to PV adjacency (Your solution is for PV to PVC).

Thanks

bboreham · 2018-05-23T11:40:50Z

We had a more detailed conversation in Slack.
I suggested writing a self-contained Render() function that shows Pods related to Storage, then you can add that into the existing Pod screen by appending to the MakeReduce list.

2opremio · 2018-05-29T08:40:39Z

Awesome! Looking forward to the changes and getting this merged!

satyamz · 2018-05-29T10:00:30Z

Hi @bboreham please review this approach mayadata-io/scope@add-pv-pvc-support...satyamz:refactor-render

satyamz · 2018-06-01T09:41:50Z

Hi @bboreham @2opremio
I updated this PR as per suggestions given by Bryan.
Thanks

bboreham

I think we're getting there!
Few more clean-up and nit-picking requests.

probe/kubernetes/persistentvolume.go

+		VolumeClaim:           p.Spec.ClaimRef.Name,
+		StorageClassName:      p.Spec.StorageClassName,
+		Status:                string(p.Status.Phase),
+		AccessModes:           string(p.Spec.AccessModes[0]),


probe/kubernetes/persistentvolumeclaim.go

+		NodeType:              "Persistent Volume Claim",
+		Status:                string(p.Status.Phase),
+		VolumeName:            p.Spec.VolumeName,
+		AccessModes:           string(p.Spec.AccessModes[0]),


probe/kubernetes/persistentvolume.go

+	return p.MetaNode(report.MakePersistentVolumeNodeID(p.UID())).WithLatests(map[string]string{
+		report.ControlProbeID: probeID,
+		NodeType:              "Persistent Volume",
+		VolumeClaim:           p.Spec.ClaimRef.Name,


probe/kubernetes/storageclass.go

+// GetNode returns StorageClass as Node
+func (p *storageClass) GetNode(probeID string) report.Node {
+	return p.MetaNode(report.MakeStorageClassNodeID(p.UID())).WithLatests(map[string]string{
+		report.ControlProbeID: probeID,


probe/kubernetes/persistentvolume.go

+// GetNode returns Persistent Volume as Node
+func (p *persistentVolume) GetNode(probeID string) report.Node {
+	return p.MetaNode(report.MakePersistentVolumeNodeID(p.UID())).WithLatests(map[string]string{
+		report.ControlProbeID: probeID,


render/persistentvolume.go

+}
+
+//PodToVolumeRenderer renders Pod and PVC resources
+func PodToVolumeRenderer() Renderer {


render/persistentvolume.go

+}
+
+// PVCToStorageClassRenderer renders PVC and Storage class objects.
+func PVCToStorageClassRenderer() Renderer {


render/render.go

@@ -251,6 +251,29 @@ func (ret *joinResults) rewriteAdjacency(outID string, adjacency report.IDList)
 	ret.nodes[outID] = out
 }

+// storageAdjacency sets adjacency for the given node ID
+func (ret *joinResults) storageAdjacency(outID string, adjacency string) {


report/report.go

+
+	// Storage Class represent all kubernetes Storage Classes on hosts running probes.
+	// Metadata is limited for now, more to come later.
+	StorageClass Topology


render/render.go

+}
+
+// storageResult returns Nodes for after adding adjacencies
+func (ret *joinResults) storageResult(input Nodes) Nodes {


foot

Quick tidy up of the JS would be great, but otherwise lgtm 👌

client/app/scripts/charts/edge.js

@@ -5,6 +5,20 @@ import classNames from 'classnames';
 import { enterEdge, leaveEdge } from '../actions/app-actions';
 import { encodeIdAttribute, decodeIdAttribute } from '../utils/dom-utils';

+
+function getAdjacencyClass(id) {


client/app/scripts/charts/edge.js

+  if (from[1] !== undefined && to[1] !== undefined) {
+    from[1] = from[1].slice(1, -1);
+    to[1] = to[1].slice(1, -1);
+    if ((from[1] === 'persistent_volume' || from[1] === 'storage_class' || from[1] === 'persistent_volume_claim') || (to[1] === 'persistent_volume' || to[1] === 'storage_class' || to[1] === 'persistent_volume_claim')) {


satyamz · 2018-06-05T15:59:33Z

@foot Please review the changes. Thanks

satyamz · 2018-06-05T16:43:46Z

I will squash commits in this PR once it's approved by all stakeholders.

foot

Thanks for the quick response! Just a couple of small changes.

client/app/scripts/charts/edge.js

@@ -5,6 +5,30 @@ import classNames from 'classnames';
 import { enterEdge, leaveEdge } from '../actions/app-actions';
 import { encodeIdAttribute, decodeIdAttribute } from '../utils/dom-utils';

+function IsStorageComponent(id) {


client/app/scripts/charts/edge.js

+  const to = topologyId[1].split(';');
+  if (from[1] !== undefined && to[1] !== undefined) {
+    from[1] = from[1].slice(1, -1);
+    to[1] = to[1].slice(1, -1);


client/app/scripts/charts/edge.js

+}
+
+// getAdjacencyClass takes id which contains information about edge as a topology
+// of parent and child node.


bboreham · 2018-06-06T13:19:25Z

I just built this branch and tried it out. Working, but I think one part of @rade's comment got misunderstood:

Regarding having a separate view... views with pods get crowded very quickly since that is the granularity at which things get scaled in k8s. Surely a user would want to see what PVs their workloads (i.e. groups of pods managed by a controller) are accessing. So IMO PVs and PVCs should go in the Pod and Controller views, possible with a selector that lets users decide whether they want to see them.

It's in the Pod view, but with a switch that gives "only pods" or "only storage", which is basically a separate view. If I make the following patch to app/api_topologies.go I get storage interspersed with pods, which is what I expect:

-                       {Value: "show", Label: "Show Storage", filter: render.IsVolumesComponent, filterPseudo: false},
+                       {Value: "show", Label: "Show Storage", filter: nil, filterPseudo: false},

Controller view can come later.

bboreham · 2018-06-06T14:03:26Z

Correction to my last comment: the code has "only storage, plus pods that have a volumeClaim"

Think I need to do more experiments.

satyamz · 2018-06-06T14:06:16Z

@bboreham If I remove the constraint on the only storage view then I get something like this:

And

Do we want it to be like this? I am bit confused here because this will make pods view visually more complicated.

umamukkara · 2018-06-06T14:55:34Z

@satyamz , my understanding also is that "show storage" will show every pod + graph nodes related to storage. It could become bit cluttered, but with the search filter, the resulting nodes could be thinned down.

umamukkara · 2018-06-06T14:56:48Z

May be as we add more storage related features to scope, we might need a special filter called "Show ONLY storage", but that is for future discussion I suppose

client/app/scripts/charts/edge.js

+  const topologyId = id.split('---');
+  const fromNode = topologyId[0].split(';');
+  const toNode = topologyId[1].split(';');
+  if (fromNode[1] !== undefined && toNode[1] !== undefined) {


rndstr · 2018-06-06T20:49:00Z

@satyamz we recently switched to sentenced cased text, would you mind changing all the strings (e.g., from "Show Storage" to "Show storage"). Thanks!

foot

LGTM

satyamz · 2018-06-07T10:38:42Z

@rndstr I made changes suggested by you on sentence cased.

@bboreham , @umamukkara I also made the change suggested by you on storage view. Now storage view have all the storage components along with pods.

This will - Add Kubernetes volume resources such as PV, PVC. - Add shapes for Kubernetes PV and PVC - Add `Cylinder` shape for PV and `Dotted Cylinder` shape for PVC. Signed-off-by: Satyam Zode <satyam.zode@openebs.io>

- Kubernetes storage components such as PV and PVC are connected based on two parameters Persistent volume claim name and Persistent Volume name. - PVC contains the volume name which is, PV name itself. Hence, we can show edge for PVC and PV. - This will bring higher level visibility for kubernetes storage components. Signed-off-by: Satyam Zode <satyam.zode@openebs.io>

This will: - Add StorageClass resource. Storage classes are mentioned in the PVC spec. We're using storage class name from PVC spec to add adjacency to the PVC node. - Add square sheet shape for StorageClass. - Add storage filter in the PODS topology. Storage Filter will allow user to see distinct view of stateful applications. - Add visually distinct edge to show storage adjacency. Signed-off-by: Satyam Zode <satyam.zode@openebs.io>

- Remove the complex volume rendering code. - Remove volume specific code from Transform. - Rename filtering methods - Remove extra params from PV, PVC nodes to make table view look good. Signed-off-by: Satyam Zode <satyam.zode@openebs.io>

- Refactor dotted edge logic. - Change Storage view to show storage components as well as all the pods. - Sentence case storage related variables. Signed-off-by: Satyam Zode <satyam.zode@openebs.io>

bboreham

LGTM; we can fix anything else with further PRs.

probe/kubernetes/persistentvolume.go

-		ReclaimPolicy:         string(p.Spec.PersistentVolumeReclaimPolicy),
-		Message:               p.Status.Message,
+		NodeType:         "Persistent Volume",
+		VolumeClaim:      p.GetVolume(),


All points have been addressed

tallaxes · 2018-07-24T23:39:56Z

Note that at this time PVs and PVCs won't show up out of the box (if one follows the default install instructions) because ClusterRole/weave-scope does not grant access to these. persistentvolumes, persistentvolumeclaims and storageclasses need to be added:

      - apiGroups:
          - ''
        resources:
          - pods/log
          - services
          - nodes
          - namespaces
          - persistentvolumes
          - persistentvolumeclaims
        verbs:
          - get
          - list
          - watch
      - apiGroups:
          - storage.k8s.io
        resources:
          - storageclasses
        verbs:
          - get
          - list
          - watch

satyamz · 2018-07-25T07:51:26Z

Hi @tallaxes , I am sending PR for the same. Thanks for pointing this out. 👍

rade previously requested changes Apr 10, 2018

View reviewed changes

rade reviewed Apr 10, 2018

View reviewed changes

test/fixture/report_fixture.go Outdated

@@ -354,12 +358,48 @@ var (

},

Window: 2 * time.Second,

}

PVReport = report.Report{

This comment was marked as abuse.

Sign in to view

bboreham requested changes Jun 1, 2018

View reviewed changes

bboreham approved these changes Jun 5, 2018

View reviewed changes

foot reviewed Jun 5, 2018

View reviewed changes

foot reviewed Jun 6, 2018

View reviewed changes

foot approved these changes Jun 7, 2018

View reviewed changes

Satyam Zode added 2 commits June 7, 2018 17:01

Add Kubernetes volumes support in Weave Scope

23210a6

This will - Add Kubernetes volume resources such as PV, PVC. - Add shapes for Kubernetes PV and PVC - Add `Cylinder` shape for PV and `Dotted Cylinder` shape for PVC. Signed-off-by: Satyam Zode <satyam.zode@openebs.io>

satyamz force-pushed the add-pv-pvc-support branch from 8c08737 to c4dcaa3 Compare June 7, 2018 11:35

Satyam Zode added 3 commits June 8, 2018 16:36

Simplify volume rendering code in render

24741be

- Remove the complex volume rendering code. - Remove volume specific code from Transform. - Rename filtering methods - Remove extra params from PV, PVC nodes to make table view look good. Signed-off-by: Satyam Zode <satyam.zode@openebs.io>

Refactor edge frontend code and reporter

d9bd17a

- Refactor dotted edge logic. - Change Storage view to show storage components as well as all the pods. - Sentence case storage related variables. Signed-off-by: Satyam Zode <satyam.zode@openebs.io>

satyamz force-pushed the add-pv-pvc-support branch from c4dcaa3 to d9bd17a Compare June 8, 2018 11:17

bboreham approved these changes Jun 8, 2018

View reviewed changes

probe/kubernetes/persistentvolume.go Outdated

ReclaimPolicy: string(p.Spec.PersistentVolumeReclaimPolicy),

Message: p.Status.Message,

NodeType: "Persistent Volume",

VolumeClaim: p.GetVolume(),

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

bboreham merged commit 8c18a78 into weaveworks:master Jun 8, 2018

satyamz mentioned this pull request Jun 8, 2018

Generalise visualizing volumes (Docker et al) #2693

Open

umamukkara mentioned this pull request Jun 12, 2018

Display status of K8s resources on graph #3224

Open

satyamz mentioned this pull request Jul 27, 2018

"Nothing to show" on kubernetes-HA v1.11.1 #3289

Open

abuehrle mentioned this pull request Aug 14, 2018

Create workload with PV storage to show storage visualization feature in Scope microservices-demo/microservices-demo#777

Closed

Add pv and pvc support in weave scope #3132

Add pv and pvc support in weave scope #3132

Conversation

satyamz commented Apr 5, 2018

rade commented Apr 10, 2018 • edited Loading

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.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

rade commented Apr 10, 2018

satyamz commented Apr 11, 2018

rade commented Apr 11, 2018

satyamz commented Apr 11, 2018

satyamz commented Apr 13, 2018

2opremio commented May 10, 2018 • edited Loading

satyamz commented May 10, 2018

satyamz commented May 10, 2018

bboreham commented May 21, 2018

satyamz commented May 21, 2018

satyamz commented May 23, 2018 • edited Loading

bboreham commented May 23, 2018

2opremio commented May 29, 2018 • edited Loading

satyamz commented May 29, 2018

satyamz commented Jun 1, 2018

bboreham 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.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

foot left a comment

Choose a reason for hiding this comment

This comment was marked as abuse.

This comment was marked as abuse.

satyamz commented Jun 5, 2018

satyamz commented Jun 5, 2018

foot 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.

bboreham commented Jun 6, 2018

bboreham commented Jun 6, 2018

satyamz commented Jun 6, 2018

umamukkara commented Jun 6, 2018

umamukkara commented Jun 6, 2018

This comment was marked as abuse.

This comment was marked as abuse.

rndstr commented Jun 6, 2018

foot left a comment

Choose a reason for hiding this comment

satyamz commented Jun 7, 2018 • edited Loading

bboreham left a comment

Choose a reason for hiding this comment

This comment was marked as abuse.

This comment was marked as abuse.

tallaxes commented Jul 24, 2018 • edited Loading

rade commented Apr 10, 2018 •

edited

Loading

2opremio commented May 10, 2018 •

edited

Loading

satyamz commented May 23, 2018 •

edited

Loading

2opremio commented May 29, 2018 •

edited

Loading

satyamz commented Jun 7, 2018 •

edited

Loading

tallaxes commented Jul 24, 2018 •

edited

Loading