Proposal for CSI Cluster Registry #2514

saad-ali · 2018-08-10T16:41:07Z

This proposal introduces a mechanism for CSI volume plugins to register themselves with Kubernetes and configure how Kubernetes interacts with them.

Still a work in progress, but feel free to start commenting.

xref kubernetes/enhancements#594

/assign @jsafrane @thockin

k8s-ci-robot · 2018-08-10T16:41:14Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: saad-ali

If they are not already assigned, you can assign the PR to them by writing /assign @saad-ali in a comment when ready.

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

contributors/design-proposals/storage/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

jsafrane · 2018-08-13T08:04:13Z

contributors/design-proposals/storage/container-storage-interface-cluster-registry.md

+    // (because it does not implement the ControllerPublishVolume() method),
+    // and that Kubernetes should not call or wait for any attach operation and
+    // just skip to mounting.
+	SkipAttach bool `json:"skipAttach"`


Just nitpicking: I think that CSIPluginSpec should be specification of the driver, not how Kubernetes should handle it. And the driver does not require attach, so RequiresAttach or something similar would be more appropriate.

Agree with the renaming.

jsafrane · 2018-08-13T08:04:33Z

contributors/design-proposals/storage/container-storage-interface-cluster-registry.md

+}
+
+// CSIPluginSpec is the specification of a CSIPlugin.
+type CSIPluginSpec struct {


Please add a flag to pass Pod info in NodePublish.

jsafrane · 2018-08-13T08:06:14Z

contributors/design-proposals/storage/container-storage-interface-cluster-registry.md

+The Kubernetes team will modify the CSI [`driver-registrar` container](https://github.com/kubernetes-csi/driver-registrar) to automatically do this.
+The plugin must set the `Driver` field to the same name returned by the CSI `GetPluginName()` call for that driver.
+The plugin may set any optional configuration fields (like `SkipAttach`) as appropriate.
+When the plugin is ready to serve, it must set `Ready` in the status to `true`.


Who sets Ready when the driver uses registration by driver-registrar?

@jsafrane If CSIPlugin object had something like a Registered field which is set by the kubelet's informer, then driver-registrar could watch for it and then set CSIPlugin as Ready once it has been seen. In this case there is no any race.

Good question. It would have to be the kubelet.

jsafrane · 2018-08-13T08:08:23Z

contributors/design-proposals/storage/container-storage-interface-cluster-registry.md

+#### CRD Installation
+
+The `CSIPlugin` object schema will checked in to kubernetes/kubernetes repo under a storage directory.
+The `CSIPlugin` `CustomResourceDefinition` (CRD) will be installed by the exiting Kubernetes attach/detach controller that is part of the kube-controller-manager binary.


I'd prefer single place for registration of CRD for all components, there will be more coming if CRD is the right way for new API objects now.

I agree. But it would be easier to use an existing controller for the alpha version.

I changed it to say new controller -- if we have enough time, I'll do new controller, otherwise I'll just do existing controller and we can add in a standalone controller later for beta.

jsafrane · 2018-08-13T08:10:39Z

contributors/design-proposals/storage/container-storage-interface-cluster-registry.md

+type CSIPluginStatus struct {
+    // Indicates the volume plugin has been successfully deployed on the cluster
+    // and is ready to use.
+	Ready bool `json:"ready"`


Who is consumer of this information? Shall A/D controller postpone any attach operation until CSI plugin gets Ready? Shall Kubelet postpone mounting of volumes? Who/when sets it to true? Most importantly, who/when sets it to false? What should A/D controller and kubelet do when the plugin becomes Unready?

For the initial version no consumers. In the future we can use this as one of the bits for smarter CSI volume aware scheduling.

jsafrane · 2018-08-13T08:14:15Z

contributors/design-proposals/storage/container-storage-interface-cluster-registry.md

+type CSIPluginStatus struct {
+    // Indicates the volume plugin has been successfully deployed on the cluster
+    // and is ready to use.
+	Ready bool `json:"ready"`


IMO, this should be an condition, with timestamp when the driver has become (un-)ready, with a message why and so on.

I'm hesitant on expanding the API more for a hypothetical use case. I'd prefer to leave it simple or drop it altogether until we have a real use case. If I drop it, there would be nothing in CSIPluginStatus so I would have to drop that for now too. What do you prefer?

I am torn on this. Currently I don't see any useful Status to report. But I may come in future. Can we have an object with .Spec and no .Status?

If we use Ready, we need clear semantics what it means and most importantly what is it good for. Is it enough to have one node with CSI driver installed to have the global object ready? Is it enough to have one node with CSI driver broken (failed liveness probe, driver not installed there, crash loop...) to have the global object not-ready?

In the future we can use this as one of the bits for smarter CSI volume aware scheduling.

For smarter CSI volume aware scheduling I would use decision based on availability / status of drivers on nodes, not a global object. What should scheduler do when the global object is not ready? All it can do is to stop scheduling.

jsafrane · 2018-08-13T08:17:37Z

contributors/design-proposals/storage/container-storage-interface-cluster-registry.md

+
+```go
+// CSIPlugin captures information about a Container Storage Interface (CSI)
+// volume plugin deployed on the cluster.


We call them CSI drivers everywhere else. Or did I miss anything?

saad-ali · 2018-08-14T08:05:16Z

@jsafrane Addressed feedback, PTAL

jsafrane

It looks usable, I still have some doubts about Ready status though. Time for API review?

jsafrane · 2018-08-15T11:49:41Z

contributors/design-proposals/storage/container-storage-interface-cluster-registry.md

+type CSIDriver struct {
+    metav1.TypeMeta   `json:",inline"`
+
+	// Standard object metadata.


wrong indentation

jsafrane · 2018-08-15T11:50:05Z

contributors/design-proposals/storage/container-storage-interface-cluster-registry.md

+
+## Design Overview
+
+A new custom resource will be automatically be installed on Kubernetes clusters.


a new custom resource definition? CR is optional.

jsafrane · 2018-08-15T11:50:32Z

contributors/design-proposals/storage/container-storage-interface-cluster-registry.md

+
+A new custom resource will be automatically be installed on Kubernetes clusters.
+
+Upon deployment a driver must create a new custom resource object.


Driver may create a new object? It's optional.

jsafrane · 2018-08-15T11:53:39Z

contributors/design-proposals/storage/container-storage-interface-cluster-registry.md

+type CSIDriverStatus struct {
+    // Indicates the volume driver has been successfully deployed on the cluster
+    // and is ready to use.
+	Ready bool `json:"ready"`


It's still unclear to me what "Ready" means. "the volume driver has been successfully deployed" - so all containers (DaemonSet with node components and Deployment with controller components) are running? What one of them fails, who sets Ready=false? What if I add a new node, shall something set it to Ready=false, wait until the DaemonSet runs a new pod with node components there and set it to Ready=true?

Will remove

We can also use kubernetes#2514 when it becomes available.

gnufied · 2018-08-15T21:55:40Z

contributors/design-proposals/storage/container-storage-interface-cluster-registry.md

+When a CSI volume driver is deployed on Kubernetes, it may optionally, register it self with Kubernetes by creating a new `CSIDriver` object.
+The Kubernetes team will modify the CSI [`driver-registrar` container](https://github.com/kubernetes-csi/driver-registrar) to automatically do this.
+If kubelet driver registration is enabled, the kubelet will automatically do this as part of plugin registration.
+The driver must set the `Driver` field to the same name returned by the CSI `GetPluginName()` call for that driver.


So does this mean that a CSI driver has to ship with some mechanism of instantiating CRs when it is deployed? Currently obviously that is not part of CSI spec, so how will that get done?

So does this mean that a CSI driver has to ship with some mechanism of instantiating CRs when it is deployed?

CSI driver authors can choose to but don't have to

Currently obviously that is not part of CSI spec, so how will that get done?

The same way all custom k8s behavior is done, side car containers provided by us (e.g. driver-registrar, external-attacher, etc.).

jsafrane · 2018-08-16T15:34:46Z

contributors/design-proposals/storage/container-storage-interface-cluster-registry.md

+
+	// Standard object metadata.
+	// More info: https://git.k8s.io/community/contributors/devel/api-conventions.md#metadata
+    metav1.ObjectMeta `json:"metadata"`


Shall we require that ObjectMeta.Name is the CSI driver name? How else can we ensure that each CSI driver has just one CSIDriver instance?

I don't think we should require it. We should have validation webhook to verify that you can't create two objects with the same CSIDriverSpec.Driver fields.

This validation is IMO racy. There may be several API servers running in parallel.

API server A gets POST CSIDriver.name=X for a driver.

API server B gets POST CSIDriver.name=Y for the same driver at the same time.

Both call validation webhook.

The webhook gets two queries, possibly in parallel. Let's say the webhook synchronizes the requests and processes X fist.

The webhook can GET CSIDriver to check if it already exists, but in this case it does not return anything useful, X and Y were not created yet.

The webhook allows X to be created. It remembers that X exists in local cache.

The webhook processes Y, it can check in the API server if X exists. It does not exist yet, either the API server is slow or it was rejected by another webhook / validation.

The webhook can check internal cache if it allowed X. It was allowed, so it rejects Y.

And this is wrong, because X might have been rejected by API server (e.g. another webhook failed) and Y should have been allowed.

saad-ali · 2018-08-17T05:03:02Z

@thockin @kubernetes/api-approvers PTAL
@kubernetes/sig-storage-api-reviews is happy with this.
I'm going to start implementing this to unblock other designs pending on this.

jsafrane · 2018-08-17T12:28:27Z

contributors/design-proposals/storage/container-storage-interface-cluster-registry.md

+And Kubernetes will continue to interact with those drivers as it does today.
+New drivers MAY create a `CSIDriver` object to change how Kubernetes interacts with them.
+
+If a Kubernetes cluster is downgraded to a version that does not support `CSIDriver`, but continues to have a CSI driver deployed on it that creates a `CSIDriver` object and expects non-default behavior, Kubernetes will not be able to interact with it correctly (e.g. it may call attach when driver requests attach not to be called resulting in the volumes not mounting). Therefore, cluster admins must ensure if they downgrade, that they also change the CSI driver, if needed.


It's very likely that the driver will be the same in this case. If I have Gluster CSI driver working in 1.11 with external attacher, I can use the same driver in 1.12 with the external attacher or I may create CSIDriver for it and stop the attacher. The driver itself is the same in all three cases, PVs are the same and pod definitions are the same.

Downgrade from a non-attachable driver with a CSIDriver instance to 1.11 is not that easy as you describe, some volumes can be mounted to running pods and we need to make sure they remain mounted after downgrade and unmounted when the pod is deleted. Let's discuss in #2523

kfox1111 · 2018-08-20T19:27:49Z

looks good to me.

kfox1111 · 2018-08-21T00:38:01Z

contributors/design-proposals/storage/container-storage-interface-cluster-registry.md

+#### CR Creation
+
+When a CSI volume driver is deployed on Kubernetes, it may optionally, register it self with Kubernetes by creating a new `CSIDriver` object.
+The Kubernetes team will modify the CSI [`driver-registrar` container](https://github.com/kubernetes-csi/driver-registrar) to automatically do this.


hmm.... Thinking about this some more, with my operator hat on, I'd rather it not. If the driver-registrar were to do it, then we'd have to rbac it well in order to restrict its access to a safe subset of things. but if you are going through that much effort, you could instead just upload the crd directly in whatever csi packaging is provided, like via k8s yaml / helm chart to the cluster using your admin credentials instead and not worry about it at all. Similar to some of the reasoning Rook took when they tightened security (https://github.com/rook/rook/blob/master/design/security-model.md for details)

vladimirvivien · 2018-08-22T22:24:17Z

contributors/design-proposals/storage/container-storage-interface-cluster-registry.md

+    // will not be passed on mount. 
+    // +optional
+    PodInfoRequiredOnMount *bool `json:"podInfoRequiredOnMount"`
+}


EDITED:
Saad, please add field to indicate whether volumeHandle needs to be automatically generated. I am proposing the following:

type VolumeHandleMode string const ( AutomaticVolumeHandleGeneration VolumeHandleMode = "automaticVolumeHandleGeneration" NoVolumeHandleGeneration VolumeHandleMode = "noVolumeHandleGeneration" ) < snip > type CSIDriverSpec struct { < snip > // Indicates how the volumeHandle for inline CSI volumes will be handled during volume operations. // If set to AutomaticVolumeHandleGeneration, this provides a hint to the Kubelet to // automatically generate handles for the inline volume. // // If set to `NoVolumeHandleGeneration`, this is a hint to Kubelet not to automatically // generate handles for volumes when not provided. VolumeHandleMode *VolumeHandleMode `json:"volumeHandleMode"` }

Should this be limited only to inline volumes? Or handles for PVs should be (optionally) autogenerated too?

hmm... pv's too.... say someone used the image driver with a pv (for some reason). It would break horribly if it wasn't given proper handles.

I think for now the language and implementation should be limited to Inline.

vladimirvivien

Suggestion for CSIDriverSpec field to indicate how to handle volume handle.

thockin · 2018-08-28T03:13:57Z

contributors/design-proposals/storage/container-storage-interface-cluster-registry.md

+    // (like podName, podUID, etc.) during mount operations.
+    // If this is set to true, Kubelet will pass pod information as
+    // VolumeAttributes in the CSI NodePublishVolume() calls.
+    // If value is not specified, default is false -- meaning pod information


Why not pass this always?

thockin · 2018-08-28T03:14:55Z

contributors/design-proposals/storage/container-storage-interface-cluster-registry.md

+    // implements the CSI ControllerPublishVolume() method), and that Kubernetes
+    // should call attach and wait for any attach operation to complete before
+    // proceeding to mounting.
+    // If value is not specified, default is true -- meaning attach will be


Is this a default as in the normal defaults (will be set in the object) or is it the implied behavior? To set a default will require admission control... Just clarify.

@jsafrane

Automatic merge from submit-queue (batch tested with PRs 64283, 67910, 67803, 68100). If you want to cherry-pick this change to another branch, please follow the instructions here: https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md. CSI Cluster Registry and Node Info CRDs **What this PR does / why we need it**: Introduces the new `CSIDriver` and `CSINodeInfo` API Object as proposed in kubernetes/community#2514 and kubernetes/community#2034 **Which issue(s) this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when PR gets merged)*: Fixes kubernetes/enhancements#594 **Special notes for your reviewer**: Per the discussion in https://groups.google.com/d/msg/kubernetes-sig-storage-wg-csi/x5CchIP9qiI/D_TyOrn2CwAJ the API is being added to the staging directory of the `kubernetes/kubernetes` repo because the consumers will be attach/detach controller and possibly kubelet, but it will be installed as a CRD (because we want to move in the direction where the API server is Kubernetes agnostic, and all Kubernetes specific types are installed). **Release note**: ```release-note Introduce CSI Cluster Registration mechanism to ease CSI plugin discovery and allow CSI drivers to customize Kubernetes' interaction with them. ``` CC @jsafrane

@jsafrane

Automatic merge from submit-queue (batch tested with PRs 64283, 67910, 67803, 68100). If you want to cherry-pick this change to another branch, please follow the instructions here: https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md. CSI Cluster Registry and Node Info CRDs **What this PR does / why we need it**: Introduces the new `CSIDriver` and `CSINodeInfo` API Object as proposed in kubernetes/community#2514 and kubernetes/community#2034 **Which issue(s) this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when PR gets merged)*: Fixes kubernetes/enhancements#594 **Special notes for your reviewer**: Per the discussion in https://groups.google.com/d/msg/kubernetes-sig-storage-wg-csi/x5CchIP9qiI/D_TyOrn2CwAJ the API is being added to the staging directory of the `kubernetes/kubernetes` repo because the consumers will be attach/detach controller and possibly kubelet, but it will be installed as a CRD (because we want to move in the direction where the API server is Kubernetes agnostic, and all Kubernetes specific types are installed). **Release note**: ```release-note Introduce CSI Cluster Registration mechanism to ease CSI plugin discovery and allow CSI drivers to customize Kubernetes' interaction with them. ``` CC @jsafrane Kubernetes-commit: 85300f4f5dd7b0bd36d0538fb6c3255c06d5e6c2

@jsafrane

Automatic merge from submit-queue (batch tested with PRs 64283, 67910, 67803, 68100). If you want to cherry-pick this change to another branch, please follow the instructions here: https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md. CSI Cluster Registry and Node Info CRDs **What this PR does / why we need it**: Introduces the new `CSIDriver` and `CSINodeInfo` API Object as proposed in kubernetes/community#2514 and kubernetes/community#2034 **Which issue(s) this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when PR gets merged)*: Fixes kubernetes/enhancements#594 **Special notes for your reviewer**: Per the discussion in https://groups.google.com/d/msg/kubernetes-sig-storage-wg-csi/x5CchIP9qiI/D_TyOrn2CwAJ the API is being added to the staging directory of the `kubernetes/kubernetes` repo because the consumers will be attach/detach controller and possibly kubelet, but it will be installed as a CRD (because we want to move in the direction where the API server is Kubernetes agnostic, and all Kubernetes specific types are installed). **Release note**: ```release-note Introduce CSI Cluster Registration mechanism to ease CSI plugin discovery and allow CSI drivers to customize Kubernetes' interaction with them. ``` CC @jsafrane Kubernetes-commit: 85300f4f5dd7b0bd36d0538fb6c3255c06d5e6c2

fejta-bot · 2019-01-20T08:32:41Z

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

bgrant0607 · 2019-01-25T06:23:21Z

If this is still relevant, please recreate as a KEP in the enhancements repo.
/close

jqmichael · 2020-05-08T01:23:57Z

contributors/design-proposals/storage/container-storage-interface-cluster-registry.md

+    // If value is not specified, default is true -- meaning attach will be
+    // called.
+    // +optional
+    AttachRequired *bool `json:"attachRequired"`


Probably a dump question.

Isn't this value already exposed from ControllerServiceCapability as PUBLISH_UNPUBLISH_VOLUME? Can't kubelet make decision based on that value?

The problem is where the call is being made. Kubelet is not doing the attach. A pod floating in the cluster takes attachment requests, converts them into the driver calls for attach/detach.

You could have the attacher sidecar running, probe the driver as you say, and then determine that it doesn't ever have to do anything. That does work today.

But that's a fair amount of extra pluming running in the cluster that isn't needed.

The flag above tells Kubernetes to skip entirely any of the handshaking at the cluster level required to do that attach so the sidecar that does nothing and its driver doesn't even need to exist.

Proposal for CSI Cluster Registry

ee6d992

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 10, 2018

k8s-ci-robot assigned jsafrane and thockin Aug 10, 2018

k8s-ci-robot requested review from childsb and jbeda August 10, 2018 16:41

jsafrane reviewed Aug 13, 2018

View reviewed changes

jsafrane mentioned this pull request Aug 13, 2018

CSI: Skip attach/detach for non-attachable volumes #2523

Merged

Address feedback

b6c8dd7

jsafrane reviewed Aug 15, 2018

View reviewed changes

jsafrane mentioned this pull request Aug 15, 2018

CSI: send pod information in NodePublishVolumeRequest #2439

Merged

gnufied added a commit to gnufied/community that referenced this pull request Aug 15, 2018

Add a plan of using CSI driver registry

2249bc5

We can also use kubernetes#2514 when it becomes available.

gnufied mentioned this pull request Aug 15, 2018

Add a design proposal for dynamic volume limits #2051

Closed

gnufied added a commit to gnufied/community that referenced this pull request Aug 15, 2018

Add a plan of using CSI driver registry

1ca2366

We can also use kubernetes#2514 when it becomes available.

gnufied added a commit to gnufied/community that referenced this pull request Aug 15, 2018

Add a plan of using CSI driver registry

eb28ce7

We can also use kubernetes#2514 when it becomes available.

gnufied reviewed Aug 15, 2018

View reviewed changes

jsafrane reviewed Aug 16, 2018

View reviewed changes

Address PR feedback

fcafb54

saad-ali changed the title ~~[WIP] Proposal for CSI Cluster Registry~~ Proposal for CSI Cluster Registry Aug 17, 2018

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 17, 2018

k8s-ci-robot added the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label Aug 17, 2018

jsafrane reviewed Aug 17, 2018

View reviewed changes

saad-ali mentioned this pull request Aug 17, 2018

Create https://github.com/kubernetes-csi/storage-drivers-api kubernetes/org#15

Closed

saad-ali mentioned this pull request Aug 18, 2018

[WIP] CSI Cluster Registry kubernetes/kubernetes#67554

Closed

saad-ali mentioned this pull request Aug 20, 2018

CSI: support for in-line volumes in pods. #2273

Closed

kfox1111 suggested changes Aug 21, 2018

View reviewed changes

saad-ali mentioned this pull request Aug 22, 2018

Create new https://github.com/kubernetes/csi-api repo kubernetes/org#30

Closed

jsafrane mentioned this pull request Aug 22, 2018

Updates with new discussed changes jsafrane/community#1

Merged

vladimirvivien reviewed Aug 22, 2018

View reviewed changes

saad-ali mentioned this pull request Aug 24, 2018

CSI Cluster Registry and Node Info CRDs kubernetes/kubernetes#67803

Merged

thockin reviewed Aug 28, 2018

View reviewed changes

justaugustus mentioned this pull request Sep 19, 2018

KEP-1a: Meta KEP Implementation #2655

Merged

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 20, 2019

bgrant0607 closed this Jan 25, 2019

jqmichael reviewed May 8, 2020

View reviewed changes


		## Design Overview

		A new custom resource will be automatically be installed on Kubernetes clusters.


		A new custom resource will be automatically be installed on Kubernetes clusters.

		Upon deployment a driver must create a new custom resource object.

Proposal for CSI Cluster Registry #2514

Proposal for CSI Cluster Registry #2514

Conversation

saad-ali commented Aug 10, 2018 • edited Loading

k8s-ci-robot commented Aug 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saad-ali commented Aug 14, 2018

jsafrane left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saad-ali commented Aug 17, 2018

Choose a reason for hiding this comment

kfox1111 commented Aug 20, 2018

Choose a reason for hiding this comment

vladimirvivien Aug 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vladimirvivien left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fejta-bot commented Jan 20, 2019

bgrant0607 commented Jan 25, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saad-ali commented Aug 10, 2018 •

edited

Loading

vladimirvivien Aug 22, 2018 •

edited

Loading