dockerclient: Agent supported Docker versions from client min,API Versions #1014

sharanyad · 2017-10-12T22:38:14Z

Summary

Finds Agent supported Docker version clients from client version's min, API Versions returned

Implementation details

dockerclient: Finding agent supported Docker versions by getting the default client version and if the min API version is present, add all the clients between min API version and API version supported, else follow current logic
utils: reuse the semantic version comparator to compare Docker API versions with only major and minor versions too

Testing

Builds on Linux (make release)
Builds on Windows (go build -out amazon-ecs-agent.exe ./agent)
Unit tests on Linux (make test) pass
Unit tests on Windows (go test -timeout=25s ./agent/...) pass
Integration tests on Linux (make run-integ-tests) pass
Integration tests on Windows (.\scripts\run-integ-tests.ps1) pass
Functional tests on Linux (make run-functional-tests) pass
Functional tests on Windows (.\scripts\run-functional-tests.ps1) pass

New tests cover the changes: yes|no

Description for the changelog

Supported Docker version clients from default client's min supported API and API version returned

Licensing

This contribution is under the terms of the Apache 2.0 License: yes

sharanyad · 2017-10-12T22:40:25Z

This addresses #1010 required for #996

aaithal · 2017-10-12T22:45:26Z

@sharanyad can you please fix the unit test failure?

aaithal · 2017-10-12T22:56:05Z

agent/utils/compare_versions.go

@@ -93,6 +96,9 @@ func (lhs Version) Matches(selector string) (bool, error) {

 func parseSemver(version string) (semver, error) {
 	var result semver
+	if ok := checkForNoPatch(version); ok {


It feels wrong to make parseSemver parse a version string that is not actually a semver. If you want to reuse this logic for comparing versions, we should at least pass a valid semver to this method. We can move the check for NoPatch outside of this method and make it respect only valid sermver's.

aaithal · 2017-10-12T22:57:05Z

agent/engine/dockerclient/dockerclientfactory.go

+	if clientVersion.Exists(minAPIVersionKey) {
+		minAPIVersion := clientVersion.Get(minAPIVersionKey)
+		apiVersion := clientVersion.Get(apiVersionKey)
+		for _, version := range getKnownAPIVersions() {


Please move the contents of the for loop into a method, which makes this method short and reduces the levels of indentation and continue statements in the inner loop.

aaithal · 2017-10-12T23:01:33Z

agent/engine/dockerclient/dockerclientfactory.go

 		}
 		clients[version] = client
 	}
 	return clients
 }
+
+func findDockerVersionsfromMinMaxVersions(endpoint string) (map[DockerVersion]dockeriface.Client, bool) {


You're using the second parameter type (bool) to indicate the success/failure of this method, which means that returning an error instead is the better way to do that.

I am not sure if it can be called a failure if min api version key is not found, nevertheless changed the return type to error

vsiddharth · 2017-10-12T23:04:17Z

agent/engine/dockerclient/dockerclientfactory.go

 		}
 		clients[version] = client
 	}
 	return clients
 }
+
+func findDockerVersionsfromMinMaxVersions(endpoint string) (map[DockerVersion]dockeriface.Client, bool) {
+	clients := make(map[DockerVersion]dockeriface.Client)


This line can be moved down below

samuelkarp · 2017-10-13T18:12:56Z

agent/engine/dockerclient/dockerclientfactory.go

 )

+const (
+	// minDockerAPILinux is the min Docker API version supported by agent
+	minDockerAPILinux = Version_1_17


You need to specify this differently for Linux and Windows. Windows won't respond at this API version. You can change the constants in versionsupport_windows.go and versionsupport_unix.go to have the same name and then just use those.

samuelkarp · 2017-10-13T18:15:35Z

agent/utils/compare_versions.go

@@ -20,6 +20,9 @@ import (
 	"strings"
 )

+// zeroPatch is a string to append patch number zero if the major minor version lacks it
+const zeroPatch = ".0"


Instead of doing this, I think I'd actually prefer implementing a different non-semver version comparator. API versions from Docker are not semver and making them look like semver can just be a source of confusion.

I just wanted to reuse the existing semver comparator due to time constraints for this fix. I agree this is not the right way, but I would prefer to add it to my list, come back and implement a non-semver comparator once this goes out.

That's fair. Want to add a TODO comment and possibly open an issue to track?

Sure, will do that.

samuelkarp · 2017-10-13T18:18:00Z

agent/engine/dockerclient/dockerclientfactory.go

+		for _, version := range getKnownAPIVersions() {
+			dockerClient, err := getDockerClientForVersion(endpoint, string(version), minAPIVersion, apiVersion)
+			if err != nil {
+				log.Infof("Error getting client version: %v", err)


Is INFO the right level for this? I think you actually want DEBUG and probably don't call it an error (since it is expected that some won't match).

vsiddharth · 2017-10-13T18:32:27Z

agent/engine/dockerclient/dockerclientfactory.go

+	// get a Docker client with the default supported version
+	client, err := newVersionedClient(endpoint, string(minDockerAPILinux))
+	if err != nil {
+		return nil, errors.Wrap(err, "Error while creating client")


Please add more context to the error and not use upper case characters in the message.
The code base has plenty of examples using errors.Wrap

vsiddharth · 2017-10-13T18:32:36Z

agent/engine/dockerclient/dockerclientfactory.go

+
+	clientVersion, err := client.Version()
+	if err != nil {
+		return nil, errors.Wrap(err, "Error while getting client version")


same as above

vsiddharth · 2017-10-13T18:33:38Z

agent/engine/dockerclient/dockerclientfactory.go

+	minVersionCheck, minErr := utils.Version(version).Matches(lessThanMinCheck)
+	maxVersionCheck, maxErr := utils.Version(version).Matches(moreThanMaxCheck)
+	if minErr != nil {
+		return nil, errors.Wrapf(minErr, "Error while comparing version %s with minAPIVersion %s", version, minAPIVersion)


same as above

vsiddharth · 2017-10-13T18:33:43Z

agent/engine/dockerclient/dockerclientfactory.go

+		return nil, errors.Wrapf(minErr, "Error while comparing version %s with minAPIVersion %s", version, minAPIVersion)
+	}
+	if maxErr != nil {
+		return nil, errors.Wrapf(minErr, "Error while comparing version %s with apiVersion %s", version, apiVersion)


same as above

vsiddharth · 2017-10-13T18:33:51Z

agent/engine/dockerclient/dockerclientfactory.go

+	// do not add the version when it is less than min api version or greater
+	// than api version
+	if minVersionCheck || maxVersionCheck {
+		return nil, errors.Errorf("Min and API versions comparison check failed for version: %s", version)


same as above

vsiddharth · 2017-10-13T18:33:57Z

agent/engine/dockerclient/dockerclientfactory.go

+	}
+	client, err := newVersionedClient(endpoint, string(version))
+	if err != nil {
+		return nil, errors.Wrap(err, "Error while creating client")


same as above

vsiddharth · 2017-10-13T18:36:57Z

agent/engine/dockerclient/dockerclientfactory.go

 )

+const (
+	// minDockerAPILinux is the min Docker API version supported by agent
+	minDockerAPILinux = Version_1_17


I don't think this would work on both linux and windows.
Please ensure that this change works well on all supported platforms.

sharanyad · 2017-10-13T21:48:39Z

@aithal @samuelkarp @vsiddharth Addressed the review comments. Could you please have a look again?

aaithal · 2017-10-16T16:16:18Z

agent/engine/dockerclient/dockerclientfactory.go

 )

+const (
+	// minAPIVersionKey is the docker.Env key for min API version
+	minAPIVersionKey = "MinAPIVersion"


Can you also document here that this is supported in API versions >=1.25 as per https://docs.docker.com/engine/api/version-history/#v125-api-changes ?

aaithal · 2017-10-16T16:18:18Z

agent/engine/dockerclient/dockerclientfactory.go

@@ -110,7 +116,15 @@ func (f *factory) getClient(version DockerVersion) (dockeriface.Client, error) {
 // findDockerVersions loops over all known API versions and finds which ones
 // are supported by the docker daemon on the host
 func findDockerVersions(endpoint string) map[DockerVersion]dockeriface.Client {
-	clients := make(map[DockerVersion]dockeriface.Client)
+	// if the client version returns a min version and api version, then use it to return
+	// Docker versions


Let's make this more explicit: use it to return versions between MinAPIVersion and APIVerison

aaithal · 2017-10-16T16:29:09Z

agent/engine/dockerclient/dockerclientfactory.go

+
+	clients := make(map[DockerVersion]dockeriface.Client)
+	// check if the docker.Env obj has MinAPIVersion key
+	if clientVersion.Exists(minAPIVersionKey) {


let's refactor this a bit more:

if !clientVersion.Exists(minAPIVersionKey) { return ... } minAPIVersion := clientVersion.Get(minAPIVersionKey) ...

aaithal · 2017-10-16T16:31:40Z

agent/engine/dockerclient/dockerclientfactory.go

+	apiVersion string) (dockeriface.Client, error) {
+	lessThanMinCheck := "<"+minAPIVersion
+	moreThanMaxCheck := ">"+apiVersion
+	minVersionCheck, minErr := utils.Version(version).Matches(lessThanMinCheck)


I don't think you need multiple error variables here. If you stick to the policy of creating/initializing a variable only when needed, you'll do something like this:

minVersionCheck, err := utils.Version(version).Matches(lessThanMinCheck) if err != nil { ... } maxVersionCheck, err := utils.Version(version).Matches(moreThanMaxCheck) if err != nil { ... } if minVersionCheck || ...

Earlier was doing a check of the error messages together, didn't modify it later. changed now.

aaithal · 2017-10-16T16:32:54Z

agent/engine/dockerclient/dockerclientfactory.go

+	// Docker versions
+	clients, err := findDockerVersionsfromMinMaxVersions(endpoint)
+	if err != nil {
+		log.Debugf("Unable to get Docker clients from min API version: %v", err)


I think this should be Infof

aaithal · 2017-10-16T16:33:02Z

agent/engine/dockerclient/dockerclientfactory.go

+		for _, version := range getKnownAPIVersions() {
+			dockerClient, err := getDockerClientForVersion(endpoint, string(version), minAPIVersion, apiVersion)
+			if err != nil {
+				log.Debugf("Unable to get Docker client for version %s: %v", version, err)


I think this should be Infof

It makes more sense to have it as Debugf to me, since we are just trying to obtain docker client for a particular version and it might not always be successful, which is expected. All the older versions' agent logs would contain these messages looking like errors in the beginning. So made it Debugf. What do you think?

If there are asynchronous failures for containers because of a specific versioned docker client not being there, having these errors printed out during discovery helps us debug the issue. If we make these Debug, we'll lose these messages when logging at default Info level.

@aaithal Theoretically...we shouldn't have async failures due to missing clients. This discovery logic is used for capability registration, and if we don't register a capability there shouldn't be a task scheduled that requires an unavailable client.

@samuelkarp yeah, but bugs.. Since this is just a once time discovery thing, logging it at Info shouldn't contribute to too much noise IMO.

aaithal · 2017-10-16T16:35:38Z

agent/utils/compare_versions.go

@@ -40,7 +43,8 @@ type semver struct {
 // * <x.y.z -- Matches a version less than the selector version
 // * x.y.z,a.b.c -- Matches if the version matches either of the two selector versions
 func (lhs Version) Matches(selector string) (bool, error) {
-	lhsVersion, err := parseSemver(string(lhs))
+	lhsVer := appendIfNoPatch(string(lhs))


Let's not do appendIfNoPatch anywhere in this method. Instead, do that in getDockerClientForVersion so that you don't have to keep doing that at multiple places here. Let's minimize changes to Version type as much as possible.

aaithal

I have a bunch of minor comments for error messages. It's mostly making sure that you follow the syntax here: https://github.com/golang/go/wiki/CodeReviewComments#error-strings

Otherwise, lgtm

aaithal · 2017-10-16T17:38:06Z

agent/engine/dockerclient/dockerclientfactory.go

+	// do not add the version when it is less than min api version or greater
+	// than api version
+	if minVersionCheck || maxVersionCheck {
+		return nil, errors.Errorf("min and API versions comparison check failed for version: %s", version)


Can you rephrase this? "version detection using MinAPIVersion: unsupported version: %s"

aaithal · 2017-10-16T17:41:23Z

agent/engine/dockerclient/dockerclientfactory.go

+	}
+	maxVersionCheck, err := utils.Version(version).Matches(moreThanMaxCheck)
+	if err != nil {
+		return nil, errors.Wrapf(err, "error while comparing version %s with apiVersion %s", version, apiVersion)


Can you rephrase this? "version detection using MinAPIVersion: unable to get max version: %s"

aaithal · 2017-10-16T17:41:35Z

agent/engine/dockerclient/dockerclientfactory.go

+	moreThanMaxCheck := ">" + apiVersion + zeroPatch
+	minVersionCheck, err := utils.Version(version).Matches(lessThanMinCheck)
+	if err != nil {
+		return nil, errors.Wrapf(err, "error while comparing version %s with minAPIVersion %s", version, minAPIVersion)


Can you rephrase this? "version detection using MinAPIVersion: unable to get min version: %s"

aaithal · 2017-10-16T17:43:00Z

agent/engine/dockerclient/dockerclientfactory.go

+	}
+	client, err := newVersionedClient(endpoint, string(version))
+	if err != nil {
+		return nil, errors.Wrapf(err, "unable to create Docker client for version: %s", version)


Can you rephrase this? "version detection using MinAPIVersion: unable to create Docker client for version: %s"

aaithal · 2017-10-16T18:06:41Z

agent/utils/compare_versions_test.go

@@ -185,6 +185,11 @@ func TestVersionMatches(t *testing.T) {
 			selector:       ">=1.5.0",
 			expectedOutput: true,
 		},
+		{
+			version:	"1.17",


Shouldn't you move this to engine/dockerclient package as well?

yeah, this test should not be here, will remove it. There is a general test added in engine/dockerclient for getting Docker versions from minAPIVersion.

aaithal

petderek · 2017-10-16T20:05:46Z

agent/engine/dockerclient/dockerclientfactory.go

-	clients := make(map[DockerVersion]dockeriface.Client)
+	// if the client version returns a MinAPIVersion and APIVersion, then use it to return
+	// all the Docker clients between MinAPIVersion and APIVersion
+	clients, err := findDockerVersionsfromMinMaxVersions(endpoint)


Two concerns here. The first is that this is going to behave differently on windows. Windows only supports API version 1.24 or greater. If we put this check here, it will skip over the capability logic that ensures tasks are schedule-able to windows.

Yeah, I agree. I missed this in my review. Good catch!

According to current logic for FindSupportedAPIVersions, if minimum version client 1.17 is not supported in Windows, the tasks should anyway not be scheduled on Windows right?

No, not quite right. We register capabilities for API versions below 1.24 on Windows because we still want tasks that use features in those APIs to work on Windows too. Our capability logic is pretty naive right now: we map a set of features to the API version in which they were introduced and then use that as an indicator of the minimum version of Docker they'll run on. Because Docker on Windows still supports (many of) the features that work on older versions of Docker, this was an inexpensive way to get that support without rebuilding our capability logic.

I agree that this is confusing and non-obvious. I think long-term we probably want to revisit this logic and see if we can improve it, given that we now know different things from when we did when it was designed. However, we haven't done that yet and we still need to register capabilities for older API versions on Windows.

Got it.
From reading today's code, in findDockerVersions for windows, we iterate through the KnownAPIVersions from 1.17. Wouldn't trying to create a client with version 1.17 or trying to ping, fail in Docker Windows? Eventually, those versions would not be added to the list of clients that Docker supports and hence the capability for 1.17 would not be added?
https://github.com/aws/amazon-ecs-agent/blob/master/agent/engine/dockerclient/dockerclientfactory.go#L115

We don't actually want to create a client at 1.17 on Windows or even to attempt to ping it; we know that it'll fail (except on Docker 1.12 beta). I'm talking strictly about capability registration here in that we still need to register those capabilities (and then override the incoming API version with the minimum of 1.24).

aah got it now :) so when we register for capabilities, if we want to get clients from the list of agent supported versions between 1.17 to 1.23 in windows, we simply return client 1.24 for them.
Since this PR is not affecting that logic, I think we should be good.

But yes, I agree we should revisit this logic sometime and adapt it for windows.

richardpen · 2017-10-16T19:47:45Z

CHANGELOG.md

@@ -2,12 +2,14 @@

 ## UNRELEASED
 * Feature - Support for provisioning Tasks with ENIs
+* Bug - Fixed a bug where unsupported Docker API client versions could be registered
+[#1010](https://github.com/aws/amazon-ecs-agent/issues/1010)


The link in the changelog should be the pr not the issue.

richardpen · 2017-10-16T20:04:51Z

agent/engine/dockerclient/dockerclientfactory.go

+		return nil, errors.New("min API version not found in client version")
+	}
+	minAPIVersion := clientVersion.Get(minAPIVersionKey)
+	apiVersion := clientVersion.Get(apiVersionKey)


Can you add a check here for minAPIVersion or apiVersion is empty?

petderek · 2017-10-16T20:20:21Z

agent/engine/dockerclient/dockerclientfactory.go

+	if err != nil {
+		log.Infof("Unable to get Docker clients from min API version: %v", err)
+	} else {
+		return clients


The second concern is tangential. Putting this here puts a hard branch in our detection logic, and I think we should consider composing it differently.

// As written, the code boils down to this clients, err := findDockerVersionsOneWay() if err == nil { return clients } clients, err := findDockerVersionsADifferentWay()

I'd rather see us handle the detection in the same place using existing logic instead of branching like this. One suggestion would be to look at the version_support_{platform} files and the Min version variable. You could write a function that does something like this:

// Use either the docker minimum API version or rely on the version set in version_support_{platform}.go func getMinVersion() DockerVersion { if functionThatChecksIfDockerHasMinAPIParam() { return functionThatExtractsMinAPIFromDockerAPI() } else { return minDockerAPIVersion } }

Oh — You'd want to write a function to check for the max version, too :)

What would the default max version here be?

vsiddharth

Changes look good for the most part. Just had minor nits and questions.

vsiddharth · 2017-10-16T23:07:42Z

agent/engine/dockerclient/dockerclientfactory.go

+	// if the client version returns a MinAPIVersion and APIVersion, then use it to return
+	// all the Docker clients between MinAPIVersion and APIVersion, else try pinging
+	// the clients in getKnownAPIVersions
+	minAPIVersion := ""


This can be composed more idiomatically
var minAPIVersion, apiVersion string

vsiddharth · 2017-10-16T23:10:15Z

agent/engine/dockerclient/dockerclientfactory.go

 		if err != nil {
-			log.Infof("Failed to ping with Docker version %s: %v", version, err)
+			return nil, errors.Wrapf(err, "version detection using MinAPIVersion: unable to get max version: %s", apiVersion)


Should this be MinAPIVersion?

No, this means something was wrong with apiVersion and the comparison check failed.

vsiddharth · 2017-10-16T23:11:19Z

agent/engine/dockerclient/dockerclientfactory.go

+		// do not add the version when it is less than min api version or greater
+		// than api version
+		if minVersionCheck || maxVersionCheck {
+			return nil, errors.Errorf("version detection using MinAPIVersion: unsupported version: %s", version)


Should this be MinAPIVersion again ?

No, we are simply logging the version from knownVersions , which does not fall in the range [minAPIVersion, apiVersion]

…sions Finding agent supported Docker versions by getting the default client version and if the min API version is present, add all the clients between min API version and API version supported, else follow current logic

samuelkarp · 2017-10-17T03:55:55Z

agent/engine/dockerclient/dockerclientfactory.go

+	var minAPIVersion, apiVersion string
+	// get a Docker client with the default supported version
+	client, err := newVersionedClient(endpoint, string(minDockerAPIVersion))
+	if err == nil {


I would find this easier to read if it were refactored a bit and the conditions were inverted, something like this:

func findDockerVersionsFromAPI(endpoint string) (int, int, bool) { client, err := newVersionedClient(endpoint, string(minDockerAPIVersion)) if err != nil { seelog.Debug("Failed to query the version API", err) return "", "", false } clientVersion, err := client.Version() if err != nil { seelog.Debug("Failed to extract version", err) return "", "", false } if !clientVersion.Exists(minAPIVersionKey) || !clientVersion.Exists(apiVersionKey) { seelog.Debug("Failed to extract minimum and maximum version") return "", "", false } return clientVersion.Get(minAPIVersionKey), clientVersion.Get(apiVersionKey), true }

This way the error conditions are more clear and there's less to keep in mind as you're reading through.

sharanyad requested review from samuelkarp, aaithal and petderek October 12, 2017 22:38

sharanyad force-pushed the capability-detection-fix branch 2 times, most recently from 2ec3d45 to b026db7 Compare October 12, 2017 22:58

aaithal requested changes Oct 12, 2017

View reviewed changes

vsiddharth reviewed Oct 12, 2017

View reviewed changes

samuelkarp reviewed Oct 13, 2017

View reviewed changes

vsiddharth reviewed Oct 13, 2017

View reviewed changes

sharanyad force-pushed the capability-detection-fix branch 2 times, most recently from e84db1a to 63c013e Compare October 13, 2017 21:24

aaithal reviewed Oct 16, 2017

View reviewed changes

sharanyad force-pushed the capability-detection-fix branch from 0ee2113 to 60357cc Compare October 16, 2017 18:03

aaithal reviewed Oct 16, 2017

View reviewed changes

sharanyad force-pushed the capability-detection-fix branch from 60357cc to 07d64ba Compare October 16, 2017 18:22

aaithal approved these changes Oct 16, 2017

View reviewed changes

petderek reviewed Oct 16, 2017

View reviewed changes

richardpen reviewed Oct 16, 2017

View reviewed changes

petderek reviewed Oct 16, 2017

View reviewed changes

sharanyad force-pushed the capability-detection-fix branch 2 times, most recently from 97377f6 to 8c8cccc Compare October 16, 2017 22:42

richardpen approved these changes Oct 16, 2017

View reviewed changes

vsiddharth reviewed Oct 16, 2017

View reviewed changes

petderek approved these changes Oct 16, 2017

View reviewed changes

sharanyad force-pushed the capability-detection-fix branch from 8c8cccc to 7aebdb5 Compare October 17, 2017 00:58

sharanyad merged commit 6fdb330 into aws:dev Oct 17, 2017

sharanyad mentioned this pull request Oct 17, 2017

Docker capability testing is broken with Docker >= 1.13.0 #1010

Closed

samuelkarp reviewed Oct 17, 2017

View reviewed changes

jhaynes mentioned this pull request Oct 17, 2017

V1.15.0 release #1018

Merged

samuelkarp added this to the 1.15.0 milestone Oct 26, 2017

dockerclient: Agent supported Docker versions from client min,API Versions #1014

dockerclient: Agent supported Docker versions from client min,API Versions #1014

Conversation

sharanyad commented Oct 12, 2017 • edited Loading

Summary

Implementation details

Testing

Description for the changelog

Licensing

sharanyad commented Oct 12, 2017

aaithal commented Oct 12, 2017

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

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

sharanyad commented Oct 13, 2017

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

aaithal 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

aaithal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaithal Oct 16, 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

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

sharanyad Oct 16, 2017 • edited Loading

Choose a reason for hiding this comment

vsiddharth 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

sharanyad Oct 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharanyad commented Oct 12, 2017 •

edited

Loading

aaithal Oct 16, 2017 •

edited

Loading

sharanyad Oct 16, 2017 •

edited

Loading

sharanyad Oct 16, 2017 •

edited

Loading