Add DynamicProvisioningScheduling support for GCE PD and RePD #67530

ddebroy · 2018-08-17T07:24:46Z

What this PR does / why we need it:
This PR adds support for the DynamicProvisioningScheduling feature for GCE PD and RePD. With this in place, if VolumeBindingMode: WaitForFirstConsumer is specified in a GCE storageclass and DynamicProvisioningScheduling is enabled, GCE PD provisioner will use the selected node's LabelZoneFailureDomain as (1) the zone to provision a GCE PD volume in (2) one of the zones to provision GCE RePD volume in.

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 #

Special notes for your reviewer:
E2E tests for DynamicProvisioningScheduling scenarios for GCE PD to follow

Release note:

none

/sig storage
/assign @msau42

idealhack · 2018-08-17T07:43:55Z

/ok-to-test
/assign @saad-ali

ddebroy · 2018-08-17T15:35:32Z

/test pull-kubernetes-e2e-gce-device-plugin-gpu

msau42 · 2018-08-17T16:21:40Z

pkg/volume/gce_pd/gce_util.go

+	var activezones sets.String
+	activezones, err = cloud.GetAllCurrentZones()
+	if err != nil {
+		glog.V(2).Infof("error getting zone information from GCE: %v", err)


does this need to be logged since it will be returned as an error?

This was in the original code but unnecessary. Will remove.

msau42 · 2018-08-17T16:37:55Z

pkg/volume/util/util_test.go

+				},
+			},
+			Reject: true,
+		},


Another negative test case could be not enough active zones

msau42 · 2018-08-17T16:38:34Z

pkg/volume/util/util_test.go

+			Reject:       true,
+		},
+
+		// Zones parameter specified do not match ReplicaCount [Fail]


it's technically "not enough" replicaCount

msau42 · 2018-08-17T16:43:21Z

pkg/volume/util/util_test.go

+		},
+
+		// Select zones from node label and AllowedTopologies [Pass]
+		// [1] nil Node


this comment is not correct

msau42 · 2018-08-17T16:43:52Z

pkg/volume/util/util_test.go

+			},
+			Reject:        false,
+			ExpectSpecificZones: true,
+			ExpectedZones: "zoneX,zoneY",


ExpectedZone

msau42 · 2018-08-17T16:44:05Z

pkg/volume/util/util_test.go

+		},
+
+		// Select zones from node label and AllowedTopologies [Pass]
+		// [1] nil Node


not correct

msau42 · 2018-08-17T16:47:29Z

pkg/volume/util/util_test.go

+			ReplicaCount:        2,
+			Reject:              false,
+			ExpectSpecificZones: true,
+			ExpectedZones:       "zoneW,zoneX",


I know ExpectedZone is redundant when ExpectSpecificZones is true, but may be good to explicitly call out anyway.

msau42 · 2018-08-17T16:53:39Z

pkg/volume/util/util_test.go

+			},
+			Reject:             false,
+			ExpectSpecificZones: true,
+			ExpectedZones:      "zoneX,zoneY",


ExpectedZone

msau42 · 2018-08-17T16:57:26Z

/assign @verult

msau42 · 2018-08-17T19:00:34Z

lgtm! will also let @verult review

msau42 · 2018-08-17T19:01:17Z

/test pull-kubernetes-kubemark-e2e-gce-big

davidz627 · 2018-08-17T19:06:17Z

pkg/volume/gce_pd/gce_util.go

-			configuredZones = v
+			configuredZones, err = volumeutil.ZonesToSet(v)
+			if err != nil {
+				return "", 0, nil, "", fmt.Errorf("error parsing zones %s, must be strings separated by commas: %v", v, err)


this error message seems to belong more inside the "ZonesToSet" method since its pretty specific, then it can just bubble up to here

I have added the error to ZonesToSet. Will adjust the error message in other consumers (like EBS/Azure) in a separate PR.

davidz627 · 2018-08-17T19:09:47Z

pkg/volume/util/util.go

 	}

 	// pick zone from allowedZones if specified
 	allowedZones, err := ZonesFromAllowedTopologies(allowedTopologies)
 	if err != nil {
-		return "", err
+		return sets.NewString(), err


when returning error I think you can just return nil for the sets.String value

davidz627 · 2018-08-17T19:14:09Z

pkg/volume/util/util.go

 		}
-		return ChooseZoneForVolume(*allowedZones, pvcName), nil
+		return chooseZonesForVolumeIncludingZone(*allowedZones, pvcName, zoneFromNode, "allowedTopologies", numReplicas)


why are we dereferencing allowedZones here. I'm not sure we need to pass in "allowedTopologies" here, its only used in an error message in the function. But if you absolutely have to have it, make it a constant

I clearly missed the fact that nil can be used to indicate an empty set. Thanks for pointing that above. With that, no dereferencing is necessary and I will fix the code around this.

I was using the label to be able to print out meaningful error messages without having to process the errors in the callers. However I am not sure that saves me much - so I will get rid of this label and error string inside chooseZonesForVolumeIncludingZone and process the errors in the callers.

davidz627 · 2018-08-17T19:19:51Z

pkg/volume/util/util_test.go

@@ -1460,6 +1460,659 @@ func TestSelectZoneForVolume(t *testing.T) {
 	}
 }

+func TestSelectZonesForVolume(t *testing.T) {


Could you also add unit tests for: chooseZonesForVolumeIncludingZone

ddebroy · 2018-08-17T23:38:14Z

/test pull-kubernetes-integration

verult

Initial pass

verult · 2018-08-17T19:05:40Z

pkg/volume/gce_pd/gce_pd.go

 	if len(labels) != 0 {
 		if pv.Labels == nil {
 			pv.Labels = make(map[string]string)
 		}
 		for k, v := range labels {
 			pv.Labels[k] = v
+			requirements = append(requirements, v1.NodeSelectorRequirement{Key: k, Operator: v1.NodeSelectorOpIn, Values: []string{v}})


For regional PD, the label value would be something like us-central1-a__us-central1-b

Thanks for catching this.

msau42 · 2018-08-21T16:17:02Z

pkg/volume/gce_pd/gce_pd.go

@@ -526,7 +527,17 @@ func (c *gcePersistentDiskProvisioner) Provision(selectedNode *v1.Node, allowedT
 		}
 		for k, v := range labels {
 			pv.Labels[k] = v
-			requirements = append(requirements, v1.NodeSelectorRequirement{Key: k, Operator: v1.NodeSelectorOpIn, Values: []string{v}})
+			var values []string
+			if k == kubeletapis.LabelZoneFailureDomain {


can this have a unit test?

verult · 2018-08-21T18:44:23Z

pkg/volume/gce_pd/gce_pd.go

 		}
 	}

+	if utilfeature.DefaultFeatureGate.Enabled(features.VolumeScheduling) {


Check whether requirements is empty

verult · 2018-08-21T18:46:55Z

pkg/volume/gce_pd/gce_util.go

+	var activezones sets.String
+	activezones, err = cloud.GetAllCurrentZones()
+	if err != nil {
+		return "", 0, nil, "", err


nit: Keep the same glog message as before: glog.V(2).Infof("error getting zone information from GCE: %v", err)

There was a review comment earlier to remove the log as the error already captures it. #67530 (comment) . Do we really need it?

Generally, we should avoid logging error messages that we also return because that creates duplicate log messages.

verult · 2018-08-21T18:54:17Z

pkg/volume/util/util.go

 	}

 	// pick zone from parameters if present
 	if zoneParameterPresent {
-		return zoneParameter, nil
+		if numReplicas > 1 {


The volumeutil.ValidateZone() logic is missing here

Is it necessary? All ValidateZone is doing is making sure the string does not have spaces/tabs. The cloud API call for PD creation will fail for an invalid zone like that eventually. Maybe I can invoke ValidateZone on the list of returned zones from SelectZonesForVolume if it is important.

verult · 2018-08-21T18:58:38Z

pkg/volume/gce_pd/gce_util.go

-	if zonePresent && zonesPresent {
-		return "", 0, nil, "", fmt.Errorf("the 'zone' and 'zones' StorageClass parameters must not be used at the same time")
-	}
-
 	if replicationType == replicationTypeRegionalPD && zonePresent {


I think this is no longer needed, because it's captured by line 388 in pkg/volume/util/util.go, inside SelectZonesForVolume()?

verult · 2018-08-21T19:10:35Z

pkg/volume/util/util.go

 	}

 	// pick zone from parameters if present
 	if zoneParameterPresent {
-		return zoneParameter, nil
+		if numReplicas > 1 {
+			return sets.NewString(), fmt.Errorf("zone cannot be specified if desired number of replicas for pv is greather than 1. Please specify zones or allowedTopologies to specify desired zones")


return nil to be consistent

verult · 2018-08-21T19:12:12Z

pkg/volume/util/util.go

+// chooseZonesForVolumeIncludingZone is a wrapper around ChooseZonesForVolume that ensures zoneToInclude is chosen
+func chooseZonesForVolumeIncludingZone(zones sets.String, pvcName, zoneToInclude string, numReplicas uint32) (sets.String, error) {
+	if uint32(zones.Len()) < numReplicas {
+		return sets.NewString(), fmt.Errorf("not enough zones found to provision a volume with %d replicas. Need at least %d distinct zones for a volume with %d replicas", numReplicas, numReplicas, numReplicas)


return nil to be consistent with the rest of SelectZonesForVolume().

verult · 2018-08-21T19:17:06Z

pkg/volume/util/util.go

+	if uint32(zones.Len()) == numReplicas {
+		return zones, nil
+	}
+	if zoneToInclude != "" {


What if zoneToInclude is not in zones?

If this function assumes zoneToInclude is always in zones, it'd be helpful to call it out in comments

The logic here makes sure zoneToInclude and zones is mutually exclusive. If zoneToInclude is set, zones should not be specified and that is what the logic here detects and error below calls out. To specify a list of "allowed" zones, the error also instructs that allowedTopologies should be specified with the list of allowed zones since that is integrated into the scheduler while zones is not.

Maybe you are referring to a different function? Both calls to choozeZonesForVolumeIncludingZone() pass in non-empty zones and they both allow for the possibility of non-empty zoneToInclude

Sorry - I was indeed referring to a different function. Agree with your comment above - choozeZonesForVolumeIncludingZone() does expect zoneToInclude to be part of zones and I will clarify that in a comment.

verult · 2018-08-21T19:19:51Z

pkg/volume/util/util.go

-			return "", fmt.Errorf("zone[s] cannot be specified in StorageClass if allowedTopologies specified")
+			return nil, fmt.Errorf("zone[s] cannot be specified in StorageClass if allowedTopologies specified")
+		}
+		if zones, err := chooseZonesForVolumeIncludingZone(allowedZones, pvcName, zoneFromNode, numReplicas); err != nil {


Should we validate that zoneFromNode is inside allowedZones before this call?

Not necessary since the scheduler guarantees that. I can call out the assumption explicitly in a comment.

verult · 2018-08-21T19:28:57Z

pkg/volume/util/util.go

+		}
+		// if single replica volume and node with zone found, return immediately
+		if numReplicas == 1 {
+			return sets.NewString(zoneFromNode), nil


zoneFromNode could be outside allowed zones

Can you clarify what you are referring to as allowed zones here? If you are referring to the zones parameter, then zonesParameterPresent need to be set and we check and make sure zonesParameterPresent is not specified in this code path up above in L349 (since the zones parameter is not integrated well with the scheduler). Unit test Node_with_Zone_labels_and_Zones_parameter_present covers this as well.

If you are referring to zones from allowedTopologies, then the scheduler guarantees the node selected has a zone (i.e. zoneFromNode) that is part of allowedTopologies. Maybe I can add a comment here to clarify this.

Sorry I was referring to allowedTopologies. Thanks for the explanation!

verult · 2018-08-21T19:58:00Z

pkg/volume/util/util.go

+// SelectZonesForVolume selects zones for a volume based on several factors:
+// node.zone, allowedTopologies, zone/zones parameters from storageclass,
+// zones with active nodes from the cluster. The number of zones = replicas.
+func SelectZonesForVolume(zoneParameterPresent, zonesParameterPresent bool, zoneParameter string, zonesParameter, zonesWithNodes sets.String, node *v1.Node, allowedTopologies []v1.TopologySelectorTerm, pvcName string, numReplicas uint32) (sets.String, error) {


nit: Is it possible to do without some of these parameters? If we can reduce the input set, it'll help guarantee test coverage and possibly improve code maintainability

since a lot of these parameters are mutually exclusive to each other, it might be worth having a SelectZonesForVolume that only takes in one of these params called zones, and a wrapper function that does all this logic to determine mutual exclusivity as well as picking which of the zone|zones|topology to pass to SelectZonesForVolume

@verult The idea behind SelectZonesForVolume is all the in-tree providers like EBS, Azure and GCE PD should be able to parse out volume zone and topology parameters from the storage class, invoke SelectZoneForVolume/SelectZonesForVolume and get a (set of) volume(s) back to pass to the cloud provider API without having to duplicate any of the logic to enforce the rules. So SelectZonesForVolume is the wrapper that implements all the common logic to determine mutual exclusivity and for that it needs the full set of parameters around zone/zones/node/topology. I agree that the long list of parameters is not pretty but to help with maintenance, we have an extensive set of unit tests in TestSelectZonesForVolume and TestSelectZoneForVolume. Those should make sure none of the rules around exclusivity of the parameters are broken.

@davidz627 SelectZonesForVolume is the wrapper that enforces the compatibility rules among the parameters. chooseZonesForVolumeIncludingZone/ChooseZonesForVolume invoked from SelectZonesForVolume does the actual picking of zone(s) from a set of zones. Let me know if it makes sense and already aligns with the above suggestion.

davidz627 · 2018-08-21T20:08:42Z

pkg/volume/gce_pd/gce_pd.go

+			if k == kubeletapis.LabelZoneFailureDomain {
+				zones, err := util.LabelZonesToSet(v)
+				if err != nil {
+					return nil, fmt.Errorf("failed to convert label string for Zone: %s to a Set", v)


nit: wrap error here

davidz627 · 2018-08-21T20:09:32Z

pkg/volume/gce_pd/gce_pd.go

+				if err != nil {
+					return nil, fmt.Errorf("failed to convert label string for Zone: %s to a Set", v)
+				}
+				values = zones.List()


can we go straight from label to list here instead of going through an intermediate set?

davidz627 · 2018-08-21T20:13:36Z

pkg/volume/util/util.go

+// SelectZonesForVolume selects zones for a volume based on several factors:
+// node.zone, allowedTopologies, zone/zones parameters from storageclass,
+// zones with active nodes from the cluster. The number of zones = replicas.
+func SelectZonesForVolume(zoneParameterPresent, zonesParameterPresent bool, zoneParameter string, zonesParameter, zonesWithNodes sets.String, node *v1.Node, allowedTopologies []v1.TopologySelectorTerm, pvcName string, numReplicas uint32) (sets.String, error) {


since a lot of these parameters are mutually exclusive to each other, it might be worth having a SelectZonesForVolume that only takes in one of these params called zones, and a wrapper function that does all this logic to determine mutual exclusivity as well as picking which of the zone|zones|topology to pass to SelectZonesForVolume

davidz627 · 2018-08-21T20:15:12Z

pkg/volume/util/util.go

+	if zones, err := stringToSet(zonesString, ","); err != nil {
+		return nil, fmt.Errorf("error parsing zones %s, must be strings separated by commas: %v", zonesString, err)
+	} else {
+		return zones, nil


nit: doesn't have to be in else block

verult · 2018-08-22T18:44:23Z

pkg/volume/util/util.go

-		// pick node's zone and ignore any allowedTopologies (since scheduler is assumed to have accounted for it already)
-		z, ok := node.ObjectMeta.Labels[kubeletapis.LabelZoneFailureDomain]
+		// pick node's zone for one of the replicas
+		zoneFromNode, ok := node.ObjectMeta.Labels[kubeletapis.LabelZoneFailureDomain]


Is zoneFromNode only being set inside the scope of this block?

Yes this is the only place zoneFromNode is set.

Sorry I meant that by using :=, I think zoneFromNode is redeclared inside the block and the variable outside the if branch isn't actually being set.

Good catch! This is something I clearly missed. I was incorrectly thinking that := will just initialize the ok and honor zoneFromNode from the broader scope. But as you mentioned, zoneFromNode gets reinitialized due to :=. The unit-test to detect this was also getting lucky it seems - will fix that too.

ddebroy · 2018-08-24T04:18:18Z

Addressed all code review feedback so far.

verult · 2018-08-24T18:43:47Z

pkg/volume/util/util.go

@@ -539,6 +604,33 @@ func ChooseZoneForVolume(zones sets.String, pvcName string) string {
 	return zone
 }

+// chooseZonesForVolumeIncludingZone is a wrapper around ChooseZonesForVolume that ensures zoneToInclude is chosen
+// zoneToInclude can either be empty in which case it is ignored. If non-empty, zoneToInclude is expected to be member of zones.
+// numReplicas is expected to be > 0 and >= zones.Len()


"<= zones.Len()"

davidz627

final nits from me, lgtm otherwise!

davidz627 · 2018-08-24T20:07:46Z

pkg/volume/gce_pd/gce_pd_test.go

@@ -91,6 +96,16 @@ func (fake *fakePDManager) DeleteVolume(cd *gcePersistentDiskDeleter) error {
 	return nil
 }

+func getNodeSelectorRequirementWithKey(key string, term v1.NodeSelectorTerm) (*v1.NodeSelectorRequirement, error) {
+	for _, r := range term.MatchExpressions {
+		if r.Key != key {


nit: use positive expressions when possible, especially here this feels a little backwards

instead, maybe:

if r.Key == key{ return &r, nil }

davidz627 · 2018-08-24T20:10:37Z

pkg/volume/util/util.go

-		// pick node's zone and ignore any allowedTopologies (since scheduler is assumed to have accounted for it already)
-		z, ok := node.ObjectMeta.Labels[kubeletapis.LabelZoneFailureDomain]
+		// pick node's zone for one of the replicas
+		ok := false


nit: var ok bool

ddebroy · 2018-08-24T20:48:46Z

/test pull-kubernetes-node-e2e

davidz627 · 2018-08-24T21:03:13Z

/lgtm

ddebroy · 2018-08-24T21:14:51Z

/test pull-kubernetes-node-e2e

verult · 2018-08-24T21:21:33Z

/lgtm

msau42 · 2018-08-24T21:27:08Z

Can you squash your commits?

Signed-off-by: Deep Debroy <[email protected]>

ddebroy · 2018-08-24T21:39:15Z

squashed commits

saad-ali · 2018-08-24T22:07:42Z

CC @verult

Please ensure func isRegionalPD(spec *volume.Spec) bool { is updated in pkg/volume/gce_pd/gce_util.go to handle this new logic.

ddebroy · 2018-08-24T22:19:22Z

@saad-ali isRegionalPD should continue to work as it used to from what I understand. There are no changes in how LabelZoneFailureDomain will get applied to PD/RePD due to this PR.

davidz627 · 2018-08-24T22:21:06Z

/lgtm

ddebroy · 2018-08-24T22:53:48Z

/test pull-kubernetes-e2e-kops-aws

verult · 2018-08-24T23:52:12Z

@ddebroy yep it should still work, but since eventually we plan to use NodeAffinity as the main way to specify topology, isRegionalPD() should be able to determine if a spec is for regional PD based on NodeAffinity too. On the other hand, since it doesn't break existing functionality, I'm OK with tracking it as an issue and adding it in a separate PR.

saad-ali · 2018-08-25T00:02:31Z

isRegionalPD will continue to work but it relies on the labels that this PR is replacing. We should update the method to use nodeAffinity (introduced in this PR) instead. But like @verult said we can do that in a follow up PR (he has a PR out touching that code, so we can do it there).

Otherwise LGTM.

/lgtm

k8s-ci-robot · 2018-08-25T00:03:31Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: davidz627, ddebroy, saad-ali, verult

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:

~~pkg/volume/gce_pd/OWNERS~~ [saad-ali]
~~pkg/volume/util/OWNERS~~ [saad-ali]

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

msau42 · 2018-08-25T00:04:08Z

For backwards compatibility, we cannot replace the labels check until the labels method is fully removed.

k8s-github-robot · 2018-08-25T00:04:13Z

/test all [submit-queue is verifying that this PR is safe to merge]

msau42 · 2018-08-25T00:04:46Z

We intentionally keep the old labeling + NodeAffinity for this backwards compatibility reason.

k8s-github-robot · 2018-08-25T01:23:18Z

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

verult · 2018-08-27T22:29:47Z

@msau42 right, we could check using both NodeAffinity and labels, prioritizing NodeAffinity

k8s-ci-robot assigned msau42 Aug 17, 2018

k8s-ci-robot requested review from davidz627 and jingxu97 August 17, 2018 07:24

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 17, 2018

k8s-ci-robot assigned saad-ali Aug 17, 2018

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 17, 2018

msau42 reviewed Aug 17, 2018

View reviewed changes

k8s-ci-robot assigned verult Aug 17, 2018

davidz627 reviewed Aug 17, 2018

View reviewed changes

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Aug 17, 2018

verult reviewed Aug 20, 2018

View reviewed changes

msau42 reviewed Aug 21, 2018

View reviewed changes

verult suggested changes Aug 21, 2018

View reviewed changes

verult reviewed Aug 21, 2018

View reviewed changes

davidz627 reviewed Aug 21, 2018

View reviewed changes

verult reviewed Aug 22, 2018

View reviewed changes

msau42 mentioned this pull request Aug 23, 2018

Move volume dynamic provisioning scheduling to beta #67432

Merged

k8s-ci-robot added the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Aug 24, 2018

verult reviewed Aug 24, 2018

View reviewed changes

davidz627 reviewed Aug 24, 2018

View reviewed changes

k8s-ci-robot assigned davidz627 Aug 24, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 24, 2018

Add DynamicProvisioningScheduling support for GCE PD and RePD

a2de7d2

Signed-off-by: Deep Debroy <[email protected]>

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 24, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 24, 2018

saad-ali mentioned this pull request Aug 24, 2018

GCE PD: Resolving disk name collisions #65198

Closed

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 25, 2018

k8s-github-robot merged commit b2b3c36 into kubernetes:master Aug 25, 2018

ddebroy mentioned this pull request Aug 28, 2018

REQUEST: New membership for ddebroy kubernetes/org#46

Closed

6 tasks

sngchlko mentioned this pull request Sep 12, 2018

Add VolumeScheduling support for Cinder #68536

Merged

Add DynamicProvisioningScheduling support for GCE PD and RePD #67530

Add DynamicProvisioningScheduling support for GCE PD and RePD #67530

Conversation

ddebroy commented Aug 17, 2018 • edited Loading

idealhack commented Aug 17, 2018

ddebroy commented Aug 17, 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

msau42 commented Aug 17, 2018

msau42 commented Aug 17, 2018

msau42 commented Aug 17, 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

ddebroy Aug 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ddebroy commented Aug 17, 2018

verult 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ddebroy Aug 21, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ddebroy Aug 21, 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

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

ddebroy commented Aug 24, 2018

Choose a reason for hiding this comment

davidz627 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ddebroy commented Aug 24, 2018

davidz627 commented Aug 24, 2018

ddebroy commented Aug 24, 2018

verult commented Aug 24, 2018

msau42 commented Aug 24, 2018

ddebroy commented Aug 24, 2018

saad-ali commented Aug 24, 2018

ddebroy commented Aug 24, 2018

davidz627 commented Aug 24, 2018

ddebroy commented Aug 24, 2018

ddebroy commented Aug 17, 2018 •

edited

Loading

ddebroy Aug 17, 2018 •

edited

Loading

ddebroy Aug 21, 2018 •

edited

Loading

ddebroy Aug 21, 2018 •

edited

Loading