#1300 Supporting regional clusters for node pools #1320

darrenhaken · 2018-04-11T17:25:15Z

This is a WIP for creating node pools with regional clusters.

darrenhaken · 2018-04-11T17:28:06Z

See #1300 issue for further context

danawillow · 2018-04-11T22:03:00Z

Hey @darrenhaken, are you looking for preliminary feedback or did you want us to wait until it's no longer WIP?

kri5 · 2018-04-12T13:23:07Z

It seems that you are assuming people are using the v1Beta1 api.

Why remove the code handling the stable api calls?

darrenhaken · 2018-04-12T13:46:43Z

@kri5 I tested using the v1Beta1 API to create zonal clusters and it seemed to work fine (the tests passed). With that, I felt it wouldn't impact TF users which version of the API to use and it simplifies the codebase.

Is there some context I may have missed?

kri5 · 2018-04-12T13:51:20Z

If you look at the cluster_container resource, they choose to maintain both version of the API, by adding a "region" field, and handling location by generating it from region or zone for the beta api only.

As the beta API are provided as-is, with no SLA, i think we should always have a way to call the stable API, which is provided with SLA.

darrenhaken · 2018-04-12T16:58:56Z

@kri5 So Justification over the SLA makes sense, I can look to add back in the logic to use the different API version when a zone is provided.

I'm not completely sold on parsing zone to the location in the code, I'd prefer to add location as a new param to the TF. location is part of the API and zone is in fact deprecated so encouraging TF users to migrate to location felt sensible.

Example:

resource "google_container_node_pool" "np" {
	name = "%s"
	cluster = "regional-test-2"
	location = "us-central1"
	initial_node_count = 1
}

Thoughts?

Also, is there a way in TF to mark a field as deprecated?

kri5 · 2018-04-12T17:08:30Z

My point here is, that until this API is GA, zone is the proper way of provision it, and location should be marked Beta, but no the other way around, ie. marking zone as deprecated, which is not yet the case.

darrenhaken · 2018-04-12T17:25:40Z

Sure, so how about I add location and in the docs I say that's a beta param for regional clusters?

kri5 · 2018-04-12T17:29:38Z

I'm not against adding location to the nodepool ressource, but the main issue for me is the non uniformity of having location in the node pool resource, and region in the container cluster resource.

That might confuse users, as theses two resources are usually used together.

What do you think?

darrenhaken · 2018-04-12T17:31:37Z

@kri5 do you think region for conformity then? The negative to this is it looks like the underlying SDK uses the location language so it is a deviation.

kri5 · 2018-04-12T17:52:46Z

@darrenhaken I do not know what would be the more appropriate.

If you really want to reflect the changes of API in the beta, maybe the right thing would be to change region to location in the cluster resource?

darrenhaken · 2018-04-12T18:01:26Z

The other option is extract the location from the cluster resource using the cluster name when Zone is not provided and assume it's regional The advantage to this is that the node pool needs to be in the same region as the cluster so it maintains this coupling. No idea if it's possible to extract from another resource but I can't see why it wouldn't.

…

On Thu, 12 Apr 2018, 18:52 Kri5, ***@***.***> wrote: @darrenhaken <https://github.com/darrenhaken> I do not know what would be the more appropriate. If you really want to reflect the changes of API in the beta, maybe the right thing would be to change region to location in the cluster resource? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1320 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA6Me1rPM9oXGkJ_gZdpZLKIrWXuV4Jyks5tn5RxgaJpZM4TQb8T> .

danawillow · 2018-04-12T18:20:09Z

Hey all, just piping in on the conversation (without looking at the code)- I'd prefer that node pool matches cluster with having region and zone separately instead of location. Both approaches (region/zone vs location) have their merits, but from talking to my team and the original PR author for regional clusters, we came to the decision to do region/zone. Whether it's really the best way or not is still up for debate, but for consistency the two resources should definitely match.

As for the beta/v1 APIs thing, it actually doesn't matter which endpoint of the API you call (this is new information my team and I learned not too long ago), but for code review purposes it's much easier if a given PR does exactly one thing, and so I'd recommend keeping the logic as-is. We have some changes coming down the road with how we handle beta features (part of our codegen efforts), so stay tuned for that as well.

darrenhaken · 2018-04-12T18:27:19Z

Thanks for the input Dana, I'll change to region as advised. I had removed the switch between code versions because it simplifies the changes I need to make. Would it be ok to stick with that?

…

On Thu, 12 Apr 2018, 19:20 Dana Hoffman, ***@***.***> wrote: Hey all, just piping in on the conversation (without looking at the code)- I'd prefer that node pool matches cluster with having region and zone separately instead of location. Both approaches (region/zone vs location) have their merits, but from talking to my team and the original PR author for regional clusters, we came to the decision to do region/zone. Whether it's really the best way or not is still up for debate, but for consistency the two resources should definitely match. As for the beta/v1 APIs thing, it actually doesn't matter which endpoint of the API you call (this is new information my team and I learned not too long ago), but for code review purposes it's much easier if a given PR does exactly one thing, and so I'd recommend keeping the logic as-is. We have some changes coming down the road with how we handle beta features (part of our codegen efforts), so stay tuned for that as well. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1320 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA6Me1qI7SQtbkFO-jsE0xsDuGo9qGpDks5tn5rdgaJpZM4TQb8T> .

danawillow · 2018-04-12T18:29:13Z

For now let's say tentatively yes

darrenhaken · 2018-04-12T18:31:42Z

Ok, I should have a working PR tomorrow so I'll message for review. Thanks for looping back

…

On Thu, 12 Apr 2018, 19:29 Dana Hoffman, ***@***.***> wrote: For now let's say tentatively yes — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1320 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA6Me0e9h3x90Ic7wwNnJI8JwY2gbMvxks5tn5z7gaJpZM4TQb8T> .

Note: Seems to be an issue when setting initial node count, this is the next thing to investigate.

- Had to remove retrieving the node pool info (which was added in when refactoring the reusable code) as it causes an error to occur. - Change the regional test case to create the test cluster - Migrate to using region instead of location.

darrenhaken · 2018-04-13T14:56:08Z

@danawillow I believe this PR is code complete now.
I have also updated the documentation.

I am seeing a couple of flaky tests so I'm just investigating whether these were pre-existing to the work I did.

@Stono is going to test this as well with the existing TF he uses

…vider-google into regional_node_pools

danawillow

Thanks @darrenhaken, this looks great! Here's some feedback for you :)

Also as an FYI, I'm going to be on vacation for two weeks starting Monday evening (pacific time), so if this isn't merged by then, feel free to ping rosbo/ndmckinley/paddycarver to have one of them finish the review.

danawillow · 2018-04-13T22:43:46Z

google/resource_container_node_pool.go

+	return GetResourceNameFromSelfLink(res.(string)), nil
+}
+
+func generateLocation(d TerraformResourceData, config *Config) (string, error) {


It probably makes sense to use the same logic here and in resource_container_cluster

Yep makes sense.

danawillow · 2018-04-13T22:44:12Z

google/resource_container_node_pool.go

-	containerApiVersion := getContainerApiVersion(d, ContainerNodePoolBaseApiVersion, ContainerNodePoolVersionedFeatures)
-	config := meta.(*Config)
+func getNodePoolRegion(d TerraformResourceData, config *Config) (string, error) {
+	res, ok := d.GetOk("region")


What if the node pool is defined inside a cluster resource? (this will affect the call in nodePoolUpdate)

@danawillow Need further info please, I'm not too sure what you mean. Are you saying an error would occur when the node pool is nested?

Sure, happy to elaborate. You can define a cluster with a node pool resource inside of it:

resource "google_container_cluster" "with_node_pool" { name = "cluster" zone = "us-central1-a" node_pool { name = "np" initial_node_count = 2 } }

If you check out the code for google_container_cluster, you can see where it calls into some of the functions inside resource_container_node_pool.go. If you do a d.Get() from there, you're still doing it on the cluster resource, and so it'll use the cluster schema rather than the node pool schema. That's why a lot of the code inside of the node pool resource looks like d.Get(prefix + "something"): that lets the cluster and node pool resource share the code.

danawillow · 2018-04-13T22:47:56Z

google/resource_container_node_pool.go

-	d.SetId(fmt.Sprintf("%s/%s/%s", zone, cluster, nodePool.Name))
+	waitErr := containerBetaOperationWait(config,
+		operation, nodePoolInfo.project,
+		nodePoolInfo.location, "timeout creating GKE NodePool", timeoutInMinutes, 3)


can you leave this as "creating GKE NodePool"? That way the full sentence that gets returned in case of an error is "Error waiting for creating GKE NodePool"

Sure changed this.

danawillow · 2018-04-13T22:48:48Z

google/resource_container_node_pool.go

 	if waitErr != nil {
 		// The resource didn't actually create
 		d.SetId("")
 		return waitErr
 	}

+	d.SetId(fmt.Sprintf("%s/%s/%s", nodePoolInfo.location, nodePoolInfo.cluster, nodePool.Name))


Any reason to move this? Leaving it before the wait means that if the Terraform process ended before we were done waiting, the resource would still be stored in state, and so the next time Terraform is run it would know to do a read() and fill in the resource in the state, instead of trying to create it again.

Moved it back up, I didn't realise it could have such an impact

danawillow · 2018-04-13T22:52:06Z

google/resource_container_node_pool.go

@@ -327,6 +351,11 @@ func resourceContainerNodePoolDelete(d *schema.ResourceData, meta interface{}) e
 	return nil
 }

+func getNodePoolName(id string) string {


just curious, why move this?

I've moved it back to the bottom of the file. I had originally deleted the function as it wasn't needed but of course it was in the end :)

danawillow · 2018-04-13T22:57:47Z

google/resource_container_node_pool.go

@@ -435,7 +481,7 @@ func flattenNodePool(d *schema.ResourceData, config *Config, np *containerBeta.N
 		if len(matches) < 4 {
 			return nil, fmt.Errorf("Error reading instance group manage URL '%q'", url)
 		}
-		igm, err := config.clientCompute.InstanceGroupManagers.Get(matches[1], matches[2], matches[3]).Do()
+		igm, err := config.clientComputeBeta.InstanceGroupManagers.Get(matches[1], matches[2], matches[3]).Do()


Either API is fine here, we're not trying to get any beta features out of the IGM. feel free to leave it or change it back.

danawillow · 2018-04-13T22:58:48Z

google/resource_container_node_pool.go

 	if err != nil {
 		return err
 	}

 	npName := d.Get(prefix + "name").(string)
-	lockKey := containerClusterMutexKey(project, zone, clusterName)
+
+	name := getNodePoolName(d.Id())


do we need two separate name variables? if so, can you comment on the difference between them? Also, this won't work if the node pool is defined inside the cluster resource.

danawillow · 2018-04-13T23:00:47Z

google/utils.go

@@ -48,6 +48,17 @@ func getZone(d TerraformResourceData, config *Config) (string, error) {
 	return GetResourceNameFromSelfLink(res.(string)), nil
 }

+func getParent(d TerraformResourceData, config *Config) (string, error) {


I don't think this gets used anywhere

danawillow · 2018-04-13T23:01:10Z

website/docs/r/container_node_pool.html.markdown

+
+resource "google_container_node_pool" "regional-np" {
+  name       = "my-node-pool"
+  region       = "us-central1"


nit: = alignment

I actually appreciate all your attention to detail. This TF code is very vast! 👍

danawillow · 2018-04-13T23:01:33Z

google/resource_container_node_pool.go

@@ -59,7 +50,12 @@ func resourceContainerNodePool() *schema.Resource {
 				},
 				"cluster": &schema.Schema{
 					Type:     schema.TypeString,
-					Required: true,
+					Optional: true,


Why is Cluster optional now?

I can revert this back, my original design was to expose the parent parameter and expect the TF user to construct the string to match. As we have gone with region this can definitely be reverted.

- Fix alignment of `=` in docs - Share functions for regional logic in both cluster and node pool. The extra functions have been moved to the regional_utils. - Removed unused getParent - Moved getRegion back to bottom of node_pool - Change cluster back to required for the node pool - Move setting node pool ID during creation back to above the wait - Used the node pool info within the exists function for node pool

darrenhaken · 2018-04-14T15:09:58Z

@danawillow I have resolved most of the things you asked me to look at. There's a couple of things I wasn't clear on, can you get back to me and I'll try and fix them today.

As an aside, do you know if there's any appetite to explore unit tests or writing some tests against a stubbed interface? It feels like we could shorten the feedback loop on errors.

I am also having a couple of tests failing (one of them does on master too because of a K8 version which is no longer provided). Got any time to give some input if I share the test log?

darrenhaken

This is worth a review again, couple of things I was unsure about. I've left comments to them and I will try and turn this around ASAP.

@danawillow you're in Seattle right? I'll try to be around on your timezone

darrenhaken · 2018-04-14T16:00:07Z

Thought I'd mention I am seeing intermittent raise conditions, an example is deleting a cluster when it's got another operation being applied to it.

Got any ideas looking at the code? I'm not sure how reliable the tests generally are, I noticed some of them randomly failing on master when I tried that too.

An example failure I am seeing:

* google_container_cluster.cluster: google_container_cluster.cluster: Error reading Container Cluster "tf-nodepool-test-end4cflprw": Cluster "tf-nodepool-test-end4cflprw" has status "RECONCILING" with message ""

darrenhaken · 2018-04-24T19:07:02Z

Wow OK, I suspect I would never have gotten to the bottom of that one!

Suppressing the diff sounds fine with me, how do I actually do that?

nat-henderson · 2018-04-24T19:12:26Z

You'll want to look at the DiffSuppressFunc here: https://github.com/hashicorp/terraform/blob/master/helper/schema/schema.go and you'll want to apply that to the taint field in node_config.go, I think. I'm only pretty sure that's a good solution... Mind giving it a try?

darrenhaken · 2018-04-24T19:19:15Z

@ndmckinley looking now. Would the string comparison be on the old or new string value?

I have something like this on taint

DiffSuppressFunc: func(k, old, new string, d *schema.ResourceData) bool {
					return strings.ToLower(new) == "nvidia.com/gpu"
				},

nat-henderson · 2018-04-24T19:24:32Z

You may also want to try CustomDiff, depending - I admit, I am not 100% sure which solution will work. I think DiffSuppressFunc might be limited to single-field diffs and won't let you suppress a diff in taint conditional on the GPU setting.

darrenhaken · 2018-04-24T19:35:00Z

Trying a run through of the test with DiffSuppressFunc

darrenhaken · 2018-04-24T20:04:21Z

@ndmckinley The above function didn't work with the DiffSuppressFunc.
I might struggle to do much more tonight (getting late here), would you be able to carry on?
Feel free to commit anything to the branch

One other idea I had is adding back in the old v1 API calls for zonal clusters, would that fix the issue?
This would mean introducing the switch based on region.

nat-henderson · 2018-04-24T20:46:52Z

Sure, I'll give it a shot.

darrenhaken · 2018-04-24T20:48:13Z

Awesome, I only ask because it's a birthday...

…

On Tue, 24 Apr 2018, 21:46 Nathan McKinley, ***@***.***> wrote: Sure, I'll give it a shot. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1320 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA6Me1yg53xVnl0JeBTQZWCIwFsWSF_kks5tr489gaJpZM4TQb8T> .

nat-henderson · 2018-04-24T21:42:22Z

:) I've proven to myself that this is definitely 100% the issue, and I have a fix that works, but is too overbroad. I'll get it down to the minimum possible interference and send it to your branch there.

Assuming it's all right with you if I merge the PR once that's done?

darrenhaken · 2018-04-24T21:47:31Z

Absolutely and awesome on proving it! If you fix and merge I'll be incredibly happy :-) Will a release be quick to follow on?

…

On Tue, 24 Apr 2018, 22:42 Nathan McKinley, ***@***.***> wrote: :) I've proven to myself that this is definitely 100% the issue, and I have a fix that *works*, but is too overbroad. I'll get it down to the minimum possible interference and send it to your branch there. Assuming it's all right with you if I merge the PR once that's done? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1320 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA6MezpxjgoVyG-qXfzpo-8ndMSxXdu7ks5tr5w_gaJpZM4TQb8T> .

Note included here for posterity: ===== The problem is, we add a GPU, and as per the docs, GKE adds a taint to the node pool saying "don't schedule here unless you tolerate GPUs", which is pretty sensible. Terraform doesn't know about that, because it didn't ask for the taint to be added. So after apply, on refresh, it sees the state of the world (1 taint) and the state of the config (0 taints) and wants to set the world equal to the config. This introduces a diff, which makes the test fail - tests fail if there's a diff after they run. Taints are a beta feature, though. :) And since the config doesn't contain any taints, terraform didn't see any beta features in that node pool ... so it used to send the request to the v1 API. And since the v1 API didn't return anything about taints (since they're a beta feature), terraform happily checked the state of the world (0 taints I know about) vs the config (0 taints), and all was well. This PR makes every node pool refresh request hit the beta API. So now terraform finds out about the taints (which were always there) and the test fails (which it always should have done). The solution is probably to write a little bit of code which suppresses the report of the diff of any taint with value 'nvidia.com/gpu', but only if GPUs are enabled. I think that's something that can be done.

nat-henderson · 2018-04-24T23:34:44Z

Tests running now. GPU test passes locally ... fingers crossed!

nat-henderson · 2018-04-25T00:39:48Z

All right, I've got one thing which I think is a spurious test failure ... I'm re-running it.

nat-henderson · 2018-04-25T00:53:28Z

Good news - the failed test passes on a retry, as do all the others. It's going in! Happy birthday, @Stono. 😆

morgante · 2018-04-25T00:55:57Z

Yay! So happy to see this merged—it's not my birthday, but it might as well be. 🎆

darrenhaken · 2018-04-25T08:46:33Z

@ndmckinley awesome work! Thanks so much for helping me get over the final line with it. When can we expect it to be released?

Stono · 2018-04-25T13:16:00Z

IM SO HAPPY I COULD CRY.
@ndmckinley we had brownies to celebrate. I ate yours for you

darrenhaken · 2018-04-25T13:20:02Z

Brownie celebrations should be when its released 😆

herrewig · 2018-04-25T16:33:17Z

Thank you!!!!!! ♥️ 🌟 ♥️ 🌟 ♥️ 🌟 ♥️ 🌟 ♥️ 🌟 ♥️ 🌟 ♥️ 🌟 ♥️ 🌟 ♥️ 🌟 ♥️ 🌟 ♥️ 🌟

nat-henderson · 2018-04-25T17:57:51Z

@darrenhaken I see the questions around release! Our releases are usually on a biweekly schedule, and last release was last Friday.

AndrewFarley · 2018-04-25T23:47:07Z

Heck yes! Just built the provider myself to test, I really needed this, and it works great. Thanks!!! +1 +1 +1

…#1320) This PR also switched us to using the beta API in all cases, and that had a side effect which is worth noting, note included here for posterity. ===== The problem is, we add a GPU, and as per the docs, GKE adds a taint to the node pool saying "don't schedule here unless you tolerate GPUs", which is pretty sensible. Terraform doesn't know about that, because it didn't ask for the taint to be added. So after apply, on refresh, it sees the state of the world (1 taint) and the state of the config (0 taints) and wants to set the world equal to the config. This introduces a diff, which makes the test fail - tests fail if there's a diff after they run. Taints are a beta feature, though. :) And since the config doesn't contain any taints, terraform didn't see any beta features in that node pool ... so it used to send the request to the v1 API. And since the v1 API didn't return anything about taints (since they're a beta feature), terraform happily checked the state of the world (0 taints I know about) vs the config (0 taints), and all was well. This PR makes every node pool refresh request hit the beta API. So now terraform finds out about the taints (which were always there) and the test fails (which it always should have done). The solution is probably to write a little bit of code which suppresses the report of the diff of any taint with value 'nvidia.com/gpu', but only if GPUs are enabled. I think that's something that can be done.

ghost · 2018-11-18T14:14:56Z

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks!

WIP for supporting regional clusters

b450384

Check if location is zone and set fields as needed

b312a0c

darrenhaken added 3 commits April 12, 2018 20:06

Fix update and delete functions to support regionals

8df3682

Create regional node pools.

6e8b290

Note: Seems to be an issue when setting initial node count, this is the next thing to investigate.

Finished regional clusters.

6dd0ae3

- Had to remove retrieving the node pool info (which was added in when refactoring the reusable code) as it causes an error to occur. - Change the regional test case to create the test cluster - Migrate to using region instead of location.

darrenhaken added 3 commits April 13, 2018 17:16

Updated the docs for regional node pools

6b6c9ab

Merge branch 'master' of github.com:terraform-providers/terraform-pro…

2a8c44a

…vider-google into regional_node_pools

Fix fmt issue

70d4b3a

danawillow suggested changes Apr 13, 2018

View reviewed changes

darrenhaken commented Apr 14, 2018

View reviewed changes

Merge remote-tracking branch 'upstream/master' into regional_node_pools

512f49f

remove log statements.

b8138ea

nat-henderson force-pushed the regional_node_pools branch from 3826301 to b8138ea Compare April 25, 2018 00:02

nat-henderson approved these changes Apr 25, 2018

View reviewed changes

nat-henderson merged commit 2b1b668 into hashicorp:master Apr 25, 2018

darrenhaken deleted the regional_node_pools branch April 25, 2018 08:46

ghost locked and limited conversation to collaborators Nov 18, 2018

#1300 Supporting regional clusters for node pools #1320

#1300 Supporting regional clusters for node pools #1320

Conversation

darrenhaken commented Apr 11, 2018

darrenhaken commented Apr 11, 2018 • edited Loading

danawillow commented Apr 11, 2018

kri5 commented Apr 12, 2018

darrenhaken commented Apr 12, 2018 • edited Loading

kri5 commented Apr 12, 2018

darrenhaken commented Apr 12, 2018 • edited Loading

kri5 commented Apr 12, 2018

darrenhaken commented Apr 12, 2018

kri5 commented Apr 12, 2018

darrenhaken commented Apr 12, 2018 • edited Loading

kri5 commented Apr 12, 2018

darrenhaken commented Apr 12, 2018 via email

danawillow commented Apr 12, 2018

darrenhaken commented Apr 12, 2018 via email

danawillow commented Apr 12, 2018

darrenhaken commented Apr 12, 2018 via email

darrenhaken commented Apr 13, 2018 • edited Loading

danawillow 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

darrenhaken Apr 14, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darrenhaken commented Apr 14, 2018

darrenhaken left a comment

Choose a reason for hiding this comment

darrenhaken commented Apr 14, 2018 • edited Loading

darrenhaken commented Apr 24, 2018

nat-henderson commented Apr 24, 2018

darrenhaken commented Apr 24, 2018 • edited Loading

nat-henderson commented Apr 24, 2018

darrenhaken commented Apr 24, 2018

darrenhaken commented Apr 24, 2018 • edited Loading

nat-henderson commented Apr 24, 2018

darrenhaken commented Apr 24, 2018 via email

nat-henderson commented Apr 24, 2018

darrenhaken commented Apr 24, 2018 via email

nat-henderson commented Apr 24, 2018

nat-henderson commented Apr 25, 2018

nat-henderson commented Apr 25, 2018

morgante commented Apr 25, 2018

darrenhaken commented Apr 25, 2018

Stono commented Apr 25, 2018

darrenhaken commented Apr 25, 2018

herrewig commented Apr 25, 2018

nat-henderson commented Apr 25, 2018

AndrewFarley commented Apr 25, 2018

ghost commented Nov 18, 2018

darrenhaken commented Apr 11, 2018 •

edited

Loading

darrenhaken commented Apr 12, 2018 •

edited

Loading

darrenhaken commented Apr 12, 2018 •

edited

Loading

darrenhaken commented Apr 12, 2018 •

edited

Loading

darrenhaken commented Apr 13, 2018 •

edited

Loading

darrenhaken Apr 14, 2018 •

edited

Loading

darrenhaken commented Apr 14, 2018 •

edited

Loading

darrenhaken commented Apr 24, 2018 •

edited

Loading

darrenhaken commented Apr 24, 2018 •

edited

Loading