Refactors fs and mock_vmdkcmd modules #1504

venilnoronha · 2017-06-28T18:29:53Z

Description

This PR refactors fs.* APIs to make vmdk_driver platform independent. It also refactors the vmdkops module to explicitly disallow mocking of VmdkCmdRunner on non-linux platforms.

Refactors fs.* APIs.
Refactors photon and vmdk drivers.
Disallows VmdkCmdRunner mocking on non-linux platforms.
Adds fs module unit tests.
Updates vsphere driver version.

Test Results

The test-all target passes locally, and following is the tail'd output:

ssh  -kTax -o StrictHostKeyChecking=no root@10.160.205.225 /tmp/docker-volume-vsphere/vmdkops.test -test.v
=== RUN   TestCommands
Detaching loopback device /dev/loop1001
Detaching loopback device /dev/loop1001
Detaching loopback device /dev/loop1001
--- PASS: TestCommands (0.79s)
	cmd_linux_test.go:30: 
		Creating Test Volume with name = [TestVol]...
PASS
coverage: 41.8% of statements
ssh  -kTax -o StrictHostKeyChecking=no root@10.160.205.225 /tmp/docker-volume-vsphere/docker-volume-vsphere.test -test.v \
		-v DefaultTestVol \
		-H1 tcp://10.160.205.225:2375 -H2 tcp://10.160.207.83:2375
=== RUN   TestSanity
2017-06-28T11:26:46-07:00 START: Running TestSanity on  tcp://10.160.205.225:2375 (may take a while)...
2017-06-28T11:26:53-07:00 END: Running TestSanity on  tcp://10.160.205.225:2375 (may take a while)...
--- PASS: TestSanity (6.60s)
	sanity_test.go:201: Successfully connected to tcp://10.160.205.225:2375
	sanity_test.go:201: Successfully connected to tcp://10.160.207.83:2375
	sanity_test.go:219: Creating vol=DefaultTestVol on client tcp://10.160.205.225:2375.
	sanity_test.go:105: Running cmd=&[touch /mnt/testvol/DefaultTestVol/file_to_touch] with vol=DefaultTestVol on client tcp://10.160.205.225:2375
	sanity_test.go:105: Running cmd=&[stat /mnt/testvol/DefaultTestVol/file_to_touch] with vol=DefaultTestVol on client tcp://10.160.205.225:2375
=== RUN   TestConcurrency
2017-06-28T11:26:53-07:00 Running concurrent tests on tcp://10.160.205.225:2375 and tcp://10.160.207.83:2375 (may take a while)...
2017-06-28T11:26:53-07:00 START: Running create/delete multi-host concurrent test ...
2017-06-28T11:27:07-07:00 END: Running create/delete multi-host concurrent test ...
Running same docker host concurrent create/delete test on tcp://10.160.205.225:2375...
2017-06-28T11:27:13-07:00 START: Running clone concurrent test...
2017-06-28T11:27:22-07:00 END: Running clone concurrent test...
--- PASS: TestConcurrency (28.65s)
	sanity_test.go:201: Successfully connected to tcp://10.160.205.225:2375
	sanity_test.go:201: Successfully connected to tcp://10.160.207.83:2375
PASS
coverage: 37.4% of statements

This commit refactors fs.* APIs to make vmdk_driver platform independent. It also refactors the vmdkops module to explicitly disallow mocking of VmdkCmdRunner on non-linux platforms. * Refactors fs.* APIs. * Refactors photon and vmdk drivers. * Disallows VmdkCmdRunner mocking on non-linux platforms.

pshahzeb

Overall good. I have some comments.

pshahzeb · 2017-06-28T20:42:17Z

vmdk_plugin/drivers/vmdk/vmdkops/mock_vmdkcmd_linux.go

@@ -142,11 +145,11 @@ func createBlockDevice(label string, opts map[string]string) error {
 	if _, result := opts["fstype"]; result == false {
 		opts["fstype"] = fs.FstypeDefault
 	}
-	mkfscmd, result := fs.MkfsLookup()[opts["fstype"]]
-	if result == false {
+	errFstype := fs.AssertFsSupport(opts["fstype"])


Lets' make the name simple like IsFSSupported or IsFSValid

pshahzeb · 2017-06-28T20:47:49Z

vmdk_plugin/drivers/vmdk/vmdk_driver.go

-	sleepBeforeMount = 1 * time.Second
-	watchPath        = "/dev/disk/by-path"
-	version          = "vSphere Volume Driver v0.4"
+	version = "vSphere Volume Driver v0.4"


nit: if it is just one constant, its fine to not wrap it in parenthesis.

pshahzeb · 2017-06-28T20:56:27Z

vmdk_plugin/drivers/vmdk/vmdk_driver.go

 	}

-	device, err := fs.GetDevicePath(dev)
+	volDev, err := d.ops.Attach(name, nil)
 	if err != nil {
 		return mountpoint, err


Not your change, but we can have a log here.

pshahzeb · 2017-06-28T21:17:25Z

vmdk_plugin/drivers/vmdk/vmdkops/vmdkops.go

+func (v VmdkOps) Attach(name string, opts map[string]string) (*fs.VolumeDevSpec, error) {
+	str, err := v.RawAttach(name, opts)
+	if err != nil {
+		return nil, err


Log before this. Similarly L83

I've added this log inside RawAttach.

pshahzeb · 2017-06-28T21:26:51Z

vmdk_plugin/utils/fs/fs_linux.go

+func DevAttachWait(watcher *inotify.Watcher, volDev *VolumeDevSpec) error {
+	device, err := getDevicePath(volDev)
+	if err != nil {
+		return err


Log before this line

pshahzeb · 2017-06-28T21:28:10Z

vmdk_plugin/utils/fs/fs_linux.go

+func Mkfs(fstype string, label string, volDev *VolumeDevSpec) error {
+	device, err := getDevicePath(volDev)
+	if err != nil {
+		return err


Log before this error.

venilnoronha · 2017-06-28T22:14:04Z

@pshahzeb I've addressed your comments in 8629110.

shaominchen

Looks quite clean. This PR is still too big to get an efficient review. Suggest to split into multiple small PRs in the future.

shaominchen · 2017-06-28T22:35:46Z

vmdk_plugin/drivers/vmdk/vmdk_driver.go

+	_, fstypeRes := r.Options["fstype"]
+	_, cloneFromRes := r.Options["clone-from"]
+	if !fstypeRes && !cloneFromRes {
+		r.Options["fstype"] = fs.FstypeDefault


Nit: Should we add a log here for easier debugging?

shaominchen · 2017-06-28T23:01:52Z

vmdk_plugin/drivers/vmdk/vmdk_driver.go

-		return mountpoint, fs.Mount(mountpoint, fstype, string(dev[:]), false)
+		dev, err := d.ops.RawAttach(name, nil)
+		if err != nil {
+			return mountpoint, err


Nit: add a log here

shaominchen · 2017-06-28T23:03:54Z

vmdk_plugin/drivers/vmdk/vmdk_driver.go

+}
+
+// detachOrWarn detaches a volume, or prints a warning log on failure.
+func (d *VolumeDriver) detachOrWarn(name string, opts map[string]string) {


I'd suggest to have a simple name "detach"

shaominchen · 2017-06-28T23:04:18Z

vmdk_plugin/drivers/vmdk/vmdk_driver.go

+}
+
+// removeOrWarn removes a volume, or prints a warning log on failure.
+func (d *VolumeDriver) removeOrWarn(name string, opts map[string]string) {


Same as above - have a simple name "remove"

venilnoronha · 2017-06-28T23:26:26Z

@shaominchen I've addressed logging issues in 60618d4. Will make sure future PRs are smaller.

shaominchen

LGTM.

msterin · 2017-06-29T01:41:18Z

vmdk_plugin/drivers/vmdk/vmdk_driver.go

@@ -65,7 +60,7 @@ func NewVolumeDriver(port int, useMockEsx bool, mountDir string, driverName stri
 	if useMockEsx {
 		d = &VolumeDriver{
 			useMockEsx: true,
-			ops:        vmdkops.VmdkOps{Cmd: vmdkops.MockVmdkCmd{}},
+			ops:        vmdkops.VmdkOps{Cmd: vmdkops.NewMockCmd()},


general request - please do not call thing "New". In a very short time it will be old and will be confusing. Pick up semantically meaningful name please

NewMockCmd is a function that returns a new instance of MockVmdkCmd for linux. By New I mean a new instance here. Do you want the method name changed?

Ah. looks like I rushed with the comment. Ignore it please, you are correct.

msterin · 2017-06-29T01:41:33Z

vmdk_plugin/drivers/vmdk/vmdk_driver.go

-	watchPath        = "/dev/disk/by-path"
-	version          = "vSphere Volume Driver v0.4"
-)
+const version = "vSphere Volume Driver v0.4"


time to update this.

nah. Just 0.5 will work . Otherwise we'd need to sync it with releases and it's not important at the moment, we can still match

Addressed this in 59cde99.

govint

I think we need a write up of exactly whats driving these changes and a brief outline of the design before proceeding to make these changes into the code.

Do a write up, review it, sign off on the approach and that would help a lot to understand why the refactoring of this working code.

govint · 2017-06-29T04:13:04Z

vmdk_plugin/drivers/vmdk/vmdk_driver.go

-		if errRemove != nil {
-			log.WithFields(log.Fields{"name": r.Name, "error": errRemove}).Warning("Remove volume failed ")
-		}
+		d.forceDetachAndRemove(r.Name, nil)


detachAndRemove()? - the function is doing that and there is no force option. The second arg is nil always in all places where this function is called and there are really no options to pass for a detach and/or remove request to the ESX service.

Second arg. is not needed and should be removed.

Addressed this in 9afadf6.

govint · 2017-06-29T04:22:39Z

vmdk_plugin/drivers/vmdk/vmdk_driver.go

-	if skipInotify {
-		time.Sleep(sleepBeforeMount)
+	if errWatch != nil {
+		time.Sleep(fs.SleepBeforeMount)


This is introducing a new way to sleep for mounts. Why isn't the existing logic correct as is.

If this is being done for Windows I'd really avoid refactoring the VMDK driver to accommodate these changes. Instead take out the common parts into a separate file and the functions specific to windows/linux make separate files for those that can be invoked via an interface that can be instantiated depending on the platform.

skipInotify was specific to linux, errWatch is an attempt to make it platform independent.

Ok, as mentioned above, can define a FS pkg function that does the wait or later what ever we want it to do for a sleep wait.

govint · 2017-06-29T04:29:11Z

vmdk_plugin/drivers/vmdk/vmdk_driver.go

+}
+
+// remove removes a volume, or prints a warning log on failure.
+func (d *VolumeDriver) remove(name string, opts map[string]string) {


Needs to return error?

govint · 2017-06-29T04:32:39Z

vmdk_plugin/drivers/vmdk/vmdkops/vmdkops.go

+			"error": err}).Error("Failed to unmarshal ")
+		return nil, err
+	}
+	return &volDev, nil


Why not use the vmdkops.Attach() vs. this here? The VolumeDevSpec doesn;'t have to be different from what vmdkops.Attach() returns.

Same as above.

govint · 2017-06-29T04:35:03Z

vmdk_plugin/drivers/vmdk/vmdkops/vmdkops.go

 		return nil, err
 	}
-	return str, err
+	return str, nil


Why is this needed if Attach() can be used and callers are updated as needed.

Lets have one set of functions and fix the results that they generate.

Attach(..) now returns a VolumeDevSpec by converting the []byte response from RawAttach(..). RawAttach is made public only for mocking purposes, where string device paths are used.

govint · 2017-06-29T04:41:01Z

vmdk_plugin/drivers/vmdk/vmdk_driver.go

 	}
-	errMkfs := fs.Mkfs(mkfscmd, r.Name, device)
+
+	errMkfs := fs.Mkfs(r.Options["fstype"], r.Name, volDev)


What if fstype doesn't exist in the Options map passed in

prepareCreateOptions() sets fstype if not already set.

govint · 2017-06-29T04:41:42Z

vmdk_plugin/drivers/vmdk/vmdk_driver.go

-			"fstype": r.Options["fstype"]}).Error("Not found ")
-		return volume.Response{Err: msg + validfs}
+	// Check whether the fstype filesystem is supported.
+	errFstype := fs.VerifyFSSupport(r.Options["fstype"])


What if Options map doesn't have the fstype key in it.

prepareCreateOptions() sets fstype if not already set.

msterin

I did only cursory review. The major chunk seems to be code rearrangement to support windows ,not the actual code changes; plus some (nicely IMO) refactored places.
So all in all LGTM

msterin · 2017-06-30T18:04:09Z

vmdk_plugin/drivers/vmdk/vmdk_driver.go

-	dev, err := d.ops.Attach(name, nil)
-	if err != nil {
-		return mountpoint, err
+	watchCtx, errWatch := fs.DevAttachWaitPrep()


is the var rename really needed ? I understand it'slinux dependent. but it was semantically valid and I am not sure what errWatch means. Or is it standard "err" from a go func ? then name it 'err' :-)

There are too many err* variables in that func, so I'd like to have it more specific like errWatch or maybe errWatchPrep :)

What do you say?

well, they are all the same :-) - they indicate an error from a function and handled right away, right ?

It's not handled right away in this scenario. It is later used to check if a time.Sleep(..) is needed.

Actually, how does this code know here that its doing a watch and the returned value is a watchCtx? Except the error, the ctx is opaque to the caller here. Preferably change to something like errWait and waitCtx?

govint · 2017-07-03T04:50:10Z

vmdk_plugin/drivers/photon/photon_driver.go

-		}
+	// Check whether the fstype filesystem is supported.
+	errFstype := fs.VerifyFSSupport(r.Options[fsTypeTag])
+	if errFstype != nil {
 		log.WithFields(log.Fields{"name": r.Name,
 			"fstype": r.Options[fsTypeTag]}).Error("Not found ")


Can we change this to "Not supported" vs. a not-found log.

govint · 2017-07-03T04:52:31Z

vmdk_plugin/drivers/photon/photon_driver.go

@@ -156,22 +157,12 @@ func validateCreateOptions(r volume.Request, fsMap map[string]string) error {
 		r.Options[fsTypeTag] = fs.FstypeDefault


Can this be done inside of the VerifyFSSupport() because, this generic driver code doesn't need to handle selecting the default datastore. If FS interface handles all the FS related options then the decision to choose a default file system type can be left to it.

I have a slightly different opinion here. fs is only managing the filesystem, and it could export the FstypeDefault constant to be used by other modules. Setting the create request options inside fs would introduce some coupling i.e. the fs module will now also have to know about volume requests.

As such, VerifyFSSupport(fstype string) only takes a string as an input, and the implementation knows nothing about volume requests.

Thoughts?

Not really, the volume driver (this file) knows of volumes and it relies on the FS module to handle all of the format part. Which can include the selection of a default filesystem to use. The FS driver is the one thats keeping data on what file system types are available on the docker host anyway. In fact why set the fstype in the options here. When the format is done, the FS module can check that the option isn't present and select the default. The volume driver really has no idea what the default is anyway and no way to configure the default either by the plugin.

We would still need the fstype in the request for the VMDK Ops Service to persist it in the KV Store. However, we could move the default fstype selection logic to fs if needed.

As per discussion with @shaominchen and @pshahzeb, I've created issue #1538 as we want to have this in a different PR.

govint · 2017-07-03T04:55:40Z

vmdk_plugin/drivers/photon/photon_driver.go

@@ -532,7 +520,7 @@ func (d *VolumeDriver) Create(r volume.Request) volume.Response {
 		return volume.Response{Err: errGetDevicePath.Error()}
 	}

-	errMkfs := fs.Mkfs(supportedFs[r.Options[fsTypeTag]], r.Name, device)
+	errMkfs := fs.MkfsByDevicePath(r.Options[fsTypeTag], r.Name, device)


Isn't the mkfs always on a device path? Do we need the new name to differentiate a mkfs by a different reference to a disk?

Mkfs(..) only handles VolumeDevSpecs, and as golang doesn't support method overloading, I thought of creating MkfsByDevicePath(..) to handle string based device paths.

govint · 2017-07-03T04:58:26Z

vmdk_plugin/drivers/vmdk/vmdk_driver.go

-	dev, err := d.ops.Attach(name, nil)
-	if err != nil {
-		return mountpoint, err
+	watchCtx, errWatch := fs.DevAttachWaitPrep()


Actually, how does this code know here that its doing a watch and the returned value is a watchCtx? Except the error, the ctx is opaque to the caller here. Preferably change to something like errWait and waitCtx?

govint · 2017-07-03T04:59:34Z

vmdk_plugin/drivers/vmdk/vmdk_driver.go

-		return mountpoint, err
+	watchCtx, errWatch := fs.DevAttachWaitPrep()
+	if errWatch != nil {
+		log.WithFields(log.Fields{"name": name, "error": errWatch}).Warning("Failed to watch disk attach ")


Possibly change the log to reflect the prep stage and that the logic is continuing vs. bailing out here.

govint · 2017-07-03T05:54:40Z

vmdk_plugin/drivers/vmdk/vmdk_driver.go

-	if skipInotify {
-		time.Sleep(sleepBeforeMount)
+	if errWatch != nil {
+		time.Sleep(fs.SleepBeforeMount)


Ok, as mentioned above, can define a FS pkg function that does the wait or later what ever we want it to do for a sleep wait.

govint · 2017-07-03T06:04:05Z

vmdk_plugin/drivers/vmdk/vmdkops/vmdkops.go

@@ -59,14 +60,32 @@ func (v VmdkOps) Remove(name string, opts map[string]string) error {
 	return err
 }

-// Attach a volume
-func (v VmdkOps) Attach(name string, opts map[string]string) ([]byte, error) {
+// RawAttach attaches a volume and returns a raw response.


... and returns a string of ....?

govint · 2017-07-03T06:08:14Z

vmdk_plugin/drivers/vmdk/vmdkops/vmdkops.go

+	if err != nil {
+		log.WithFields(log.Fields{"name": name, "opts": opts, "bytes": str,
+			"error": err}).Error("Failed to unmarshal ")
+		return nil, err


If for some reason this json decode fails then is the device going to be left attached to the VM? The raw-attach itself succeeded so the volume is attached to the VM at this point.

Good catch, thanks! There are 2 places from where Attach(..) is now called, which is exactly where fs.GetDevicePath(..) was called earlier and it seems that the json unmarshalling error could have caused a bug inside MountVolume(..).

govint · 2017-07-03T06:13:59Z

vmdk_plugin/utils/fs/fs_linux.go

+// This is the filesystem interface for mounting volumes on a linux guest.
+
+package fs
+


Since we are separating into a platform specific FS implementation its correct to make this an interface and let the VMDK driver instantiate an FS-linux or FS-Windows instance. The interface can then be defined as a set of functions and the platform specific FS implementation will implement those. Like driver.go that the refcounter uses to invoke driver specific functions (photon vs. vmdk).

Since it involves major redesigning of the fs module, should we do it in a different PR? I had a similar plan earlier, but we chose not to do it because a minimal funcs refactoring would be enough to implement the Windows Containers feature.

@pshahzeb @shaominchen what do you guys think?

If refactoring its ok to add that piece now so the windows implementation can be added in seamlessly in the vmdk driver. Its only an interface definition that can use the names already in fs_linux.go. Extract those fs functions used in vmdk_driver.go and define that in an interface, instantiate the FS interface in vmdk_driver init() and thats done. Its not a major change.

As per discussion with @shaominchen and @pshahzeb, I've created issue #1538 as we want to have this in a different PR.

govint · 2017-07-03T06:16:07Z

vmdk_plugin/utils/fs/fs_test.go

+// limitations under the License.
+
+package fs_test
+


The convention is to have all files in a dir. to belong to a single package and separate dirs for separate packages. So, it should be like,

fs/
fs_linux/
fs_windows/
fs_test/

The moby package/file structuring seems a little better. I could rename the package as just fs. The idea is that fs.go, fs_linux.go and fs_windows.go stay in a single directory and Go would automatically pick the correct set of files based on the platform for which the binary is being built. Also, keeping fs_test.go in the same directory would facilitate access to the module's private members for the purpose of testing.

venilnoronha · 2017-07-03T19:55:08Z

@govint addressed your comments in c4777b8 and 4bf56ec.

govint

Don't have any code changes requested in the review, hence approving.

govint · 2017-07-04T05:15:03Z

vmdk_plugin/drivers/photon/photon_driver.go

@@ -156,22 +157,12 @@ func validateCreateOptions(r volume.Request, fsMap map[string]string) error {
 		r.Options[fsTypeTag] = fs.FstypeDefault


Not really, the volume driver (this file) knows of volumes and it relies on the FS module to handle all of the format part. Which can include the selection of a default filesystem to use. The FS driver is the one thats keeping data on what file system types are available on the docker host anyway. In fact why set the fstype in the options here. When the format is done, the FS module can check that the option isn't present and select the default. The volume driver really has no idea what the default is anyway and no way to configure the default either by the plugin.

govint · 2017-07-04T05:21:02Z

vmdk_plugin/utils/fs/fs_linux.go

+// This is the filesystem interface for mounting volumes on a linux guest.
+
+package fs
+


If refactoring its ok to add that piece now so the windows implementation can be added in seamlessly in the vmdk driver. Its only an interface definition that can use the names already in fs_linux.go. Extract those fs functions used in vmdk_driver.go and define that in an interface, instantiate the FS interface in vmdk_driver init() and thats done. Its not a major change.

govint · 2017-07-04T08:35:10Z

vmdk_plugin/drivers/vmdk/vmdk_driver.go

+	_, fstypeRes := r.Options["fstype"]
+	_, cloneFromRes := r.Options["clone-from"]
+	if !fstypeRes && !cloneFromRes {
+		log.WithFields(log.Fields{"req": r}).Debugf("Setting fstype to %s ", fs.FstypeDefault)


Please separate this as mentioned earlier. we don't do this coupling of two options today.

There's an explicit check in vmdk_ops.py that raises an error on seeing fstype in a clone-from request. Therefore, this separation is not directly possible.

venilnoronha requested review from msterin, govint, pdhamdhere, shaominchen and pshahzeb June 28, 2017 18:29

vmwclabot added the cla-not-required label Jun 28, 2017

pshahzeb reviewed Jun 28, 2017

View reviewed changes

Review changes

8629110

shaominchen reviewed Jun 28, 2017

View reviewed changes

Adds logging

60618d4

shaominchen approved these changes Jun 28, 2017

View reviewed changes

pshahzeb approved these changes Jun 29, 2017

View reviewed changes

msterin reviewed Jun 29, 2017

View reviewed changes

govint suggested changes Jun 29, 2017

View reviewed changes

venilnoronha added 3 commits June 30, 2017 10:20

Adds fs unit test

571bc2c

Review changes

9afadf6

Updates vmdk driver version

59cde99

msterin approved these changes Jun 30, 2017

View reviewed changes

govint suggested changes Jul 3, 2017

View reviewed changes

venilnoronha added 2 commits July 3, 2017 12:08

Review changes

c4777b8

Adds json unmarshalling error handling in Attach

4bf56ec

govint approved these changes Jul 4, 2017

View reviewed changes

govint suggested changes Jul 4, 2017

View reviewed changes

govint approved these changes Jul 4, 2017

View reviewed changes

venilnoronha mentioned this pull request Jul 5, 2017

Refactor fs module #1538

Closed

venilnoronha merged commit e74a73b into vmware-archive:master Jul 5, 2017

		@@ -156,22 +157,12 @@ func validateCreateOptions(r volume.Request, fsMap map[string]string) error {
		r.Options[fsTypeTag] = fs.FstypeDefault

		// This is the filesystem interface for mounting volumes on a linux guest.

		package fs

Refactors fs and mock_vmdkcmd modules #1504

Refactors fs and mock_vmdkcmd modules #1504

Conversation

venilnoronha commented Jun 28, 2017 • edited Loading

Description

Test Results

pshahzeb 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

venilnoronha commented Jun 28, 2017 • edited Loading

shaominchen 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

venilnoronha commented Jun 28, 2017 • edited Loading

shaominchen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

venilnoronha Jun 29, 2017 • 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

govint 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

msterin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

venilnoronha Jun 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

venilnoronha Jun 30, 2017 • 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

venilnoronha Jul 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

venilnoronha Jul 3, 2017 • 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

venilnoronha Jul 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

venilnoronha Jul 3, 2017 • edited Loading

Choose a reason for hiding this comment

venilnoronha commented Jun 28, 2017 •

edited

Loading

venilnoronha commented Jun 28, 2017 •

edited

Loading

venilnoronha commented Jun 28, 2017 •

edited

Loading

venilnoronha Jun 29, 2017 •

edited

Loading

venilnoronha Jun 30, 2017 •

edited

Loading

venilnoronha Jun 30, 2017 •

edited

Loading

venilnoronha Jul 5, 2017 •

edited

Loading

venilnoronha Jul 3, 2017 •

edited

Loading

venilnoronha Jul 3, 2017 •

edited

Loading

venilnoronha Jul 3, 2017 •

edited

Loading

venilnoronha Jul 3, 2017 •

edited

Loading