Added SpotInstance Percentage Multiplier with dynamic tags #175

kartik894 · 2017-12-03T10:09:29Z

Issue Type

Feature Pull Request, implements Implement multiple spot bidding strategies #119.

Summary

Added configurable policy to place bids at a multiple of spot instance price or at on-demand price. Also added dynamic tag to enable this feature for an auto-scaling group.

Code review process:

The issue should be tagged with 'review wanted' label before the checklist below
is satisfied from the perspective of the PR author and the code is ready to be
peer-reviewed.

Code contribution checklist

cristim

Thanks for this contribution, this piece of functionality is great to have, I'm really excited about it and looking forward to see it merged.

cristim · 2017-12-03T13:36:34Z

START.md

+        runtime hour before spot termination is free of charge.
+        (default 1.1)
+
+  -spot_price_multiplier_flag bool


I would prefer to have this a multi-option flag instead of a boolean, perhaps named something like 'bidding_policy', and accepting string values such as 'normal' and 'aggressive' and document them in detail. This way we can add more policies later on.

The idea would be to have a "policy type" and a tag associated with the configuration of this specific policy, is that correct @cristim ?

cristim · 2017-12-03T13:36:54Z

autospotting.go

@@ -30,7 +30,8 @@ func main() {
 func run() {
 	log.Println("Starting autospotting agent, build ", Version, "expiring on", ExpirationDate)

-	if isExpired(ExpirationDate) {
+	if false {


Please don't change the expiration logic.

I'm not sure if the PR is done or not, but please do not leave commented code

Will change this.

xlr-8

Thanks for the contribution! Always appreciate to see new contributors! :)

I've made one comment which has big impacts, basically the flag option seems unnecessary to me, by defaulting the multiplier to 1.0, then it removes many code pieces which are made more complicated than they could be.

I had the same question/issue when implementing the flags for the algorithm https://github.com/cristim/autospotting/pull/54; and I realized I could make it easier by removing such cases.

If there is anything that seems unclear to you, or you disagree with, please let me know - I'd be happy to clarify/discuss.

xlr-8 · 2017-12-03T23:04:31Z

START.md

+        Multiplier for the spot price. Placing bids a fraction of a cent over the
+        current price would aggressively optimize for cost savings since the last
+        runtime hour before spot termination is free of charge.
+        (default 1.1)


I'd rather put 1.0, so that it doesn't change people's cost optimization/bidding strategy.
That could however be a note/advise in the documentation and/or be updated later on if we realize many people are updating this value to something slightly higher.

The 1.0 will place spot instance bids at exactly the current market price, the 1.1 is there to offer a 10% buffer above the current price.

But I also find it a bit confusing, perhaps we should switch to percentages and also support absolute value increments, something like spot_price_buffer_percentage and spot_price_buffer_amount, what do you guys think?

@kartik894, @xlr-8 what do you think about my proposal to use percentage or absolut values for the spot price buffer?

I think something like spot_price_buffer_percentage=10(and make it a default when the aggressive policy is used) is more intuitive than the current spot_price_multiplier=1.1 for setting the bid to 10% over the current spot price. Also, negative percentage numbers don't make sense in this context, but the multiplier gives the users the chance to mess it up by giving 0.9, which will fail to launch any instances.

This is also quite consistent to the existing percentage/value concept which we use for keeping on-demand capacity, where we can have an absolute value as well as a percentage. This would bid X+current_price, where X may be given as a small fractional number, such as spot_price_buffer=0.001.

Your various comments made me think of: https://github.com/cristim/autospotting/pull/175/files#r156044213

We need to check the value anyway, just like it's done for the percentage/raw-value for on-demand instances. Also this value can't be set at an autoscaling-level if I'm not mistaken. Don't know if that's a goal, but if so some code need to be adapted.

But I agree that percentage might make more sense here, as it's a multiplier rather a raw number.

xlr-8 · 2017-12-03T23:05:16Z

autospotting.go

@@ -30,7 +30,8 @@ func main() {
 func run() {
 	log.Println("Starting autospotting agent, build ", Version, "expiring on", ExpirationDate)

-	if isExpired(ExpirationDate) {
+	if false {


I'm not sure if the PR is done or not, but please do not leave commented code

xlr-8 · 2017-12-03T23:08:07Z

core/autoscaling.go

+					a.SpotPriceMultiplierFlag = newValue
+					return done
+				}
+			}


I like the idea of loading the value dynamically, I guess it should be adapted to have a map of string/func based on the tag's value.

xlr-8 · 2017-12-03T23:32:49Z

core/autoscaling.go

@@ -39,6 +47,8 @@ type autoScalingGroup struct {
 	// spot instance requests generated for the current group
 	spotInstanceRequests []*spotInstanceRequest
 	minOnDemand          int64
+
+	SpotPriceMultiplierFlag bool


I believe this variable doesn't need to be exported

xlr-8 · 2017-12-03T23:35:44Z

core/autoscaling.go

+
+	// DefaultSpotPriceMultiplierFlag stores the default flag value for the choice
+	// of bid on a per-group level
+	DefaultSpotPriceMultiplierFlag = false


Regarding @cristim comments I assume that should be the policy type, rather than a flag

xlr-8 · 2017-12-04T00:30:46Z

core/autoscaling.go

@@ -182,6 +239,8 @@ func (a *autoScalingGroup) process() {
 	a.scanInstances()
 	a.loadDefaultConfig()
 	a.loadConfigFromTags()
+	a.loadDefaultSpotConfig()
+	a.loadSpotConfigFromTags()


I think those 2 methods could be put directly in the previous 2 methods (loadDefaultConfig & loadConfigFromTags) that were created for that; in case extra elements were to be added.

While re-reading the code I also realised it could be slightly re-arranged in order to load the default configuration and ASG configuration earlier than during the processing, see https://github.com/cristim/autospotting/issues/180.

loadDefaultConfig has dedicated tests and merging mine with that will be a bit messy. What do you think @cristim ?

I agree with @xlr-8, the purpose of tests is for making such refactoring easier, let's merge these because they do more or less the same thing.

xlr-8 · 2017-12-04T00:34:08Z

core/autoscaling.go

+}
+
+func (a *autoScalingGroup) loadConfSpot() bool {
+	tagList := [1]string{SpotBidChoice}


Why declaring an array of string of 1 and not directly a string tagKey?

xlr-8 · 2017-12-04T00:48:28Z

core/autoscaling.go

+	logger.Println("Launching spot instance with a bid =", baseOnDemandPrice)
+	return baseOnDemandPrice
+}
+


I suppose the multiplier could be used here: https://github.com/cristim/autospotting/blob/master/core/region.go#L236 or here: https://github.com/cristim/autospotting/blob/master/core/instance.go#L160-L162 - that would avoid using this method.

Just like this new parameter actually: https://github.com/cristim/autospotting/pull/163/files#diff-17a0faaa6296d309bb099b250ff6ead1R162.

I'd tend to prefer putting them in the instance.go file as it seems more logical, but the main method would need to take new parameters such as biding_policies or multipliers or something like this.

@cristim what are your thoughts on this?

It can be done, but at the repo level - requestSpotPrices() might be used for another bidding policy in the future and I wouldn't want to touch that. isPriceCompatible() is used to determine the choice of spot instances to be spawned for the current on-demand instance - Also wouldn't want to change that. I did think of these scenarios before I begun the implementation, but I figured it would be best placed in autoscaling.go since that is where you call for the bidPrice to be placed.

xlr-8 · 2017-12-04T00:49:44Z

core/autoscaling_test.go

+				tt.multiplier, tt.flag, actualPrice, tt.want, currentSpotPrice)
+		}
+	}
+}


Rest of the tests seem good to me!

cristim · 2017-12-04T01:45:52Z

@xlr-8 we can't entirely get rid of the boolean or the string flag because this is meant to introduce an additional bidding policy, so we basically need a switch to choose between the old and this one and a second piece of configuration (or maybe later more) for tweaking this additional bidding strategy.

In theory we could swing to the new bidding strategy only if its configuration tweaking option is different from a default value, but I would prefer to use an explicit, dedicated flag for this switch.

xlr-8 · 2017-12-05T16:38:20Z

Alright, thx @cristim I'll update my comments accordingly, sorry for the delay @kartik894

xlr-8

I try to update all previous comments, if anything's missing please let me know. As the strategy is to go with policy name then some code has to be updated

xlr-8 · 2017-12-05T23:23:09Z

core/autoscaling_test.go

+		}
+
+	}
+}


Would need to be updated to fetch various decided policy names

cristim · 2017-12-10T18:26:04Z

START.md

+        Multiplier for the spot price. Placing bids a fraction of a cent over the
+        current price would aggressively optimize for cost savings since the last
+        runtime hour before spot termination is free of charge.
+        (default 1.1)


@kartik894, @xlr-8 what do you think about my proposal to use percentage or absolut values for the spot price buffer?

I think something like spot_price_buffer_percentage=10(and make it a default when the aggressive policy is used) is more intuitive than the current spot_price_multiplier=1.1 for setting the bid to 10% over the current spot price. Also, negative percentage numbers don't make sense in this context, but the multiplier gives the users the chance to mess it up by giving 0.9, which will fail to launch any instances.

This is also quite consistent to the existing percentage/value concept which we use for keeping on-demand capacity, where we can have an absolute value as well as a percentage. This would bid X+current_price, where X may be given as a small fractional number, such as spot_price_buffer=0.001.

cristim · 2017-12-10T18:36:29Z

autospotting.go

+	flag.Float64Var(&c.SpotPriceMultiplier, "spot_price_multiplier", 1.1,
+		"Multiplier for the spot price. Placing bids a fraction of a cent over the current price\n"+
+			"\twould aggressively optimize for cost savings since the last runtime hour before spot termination\n"+
+			"\tis free of charge.")


It turns out after the per-second billing the last hour is no longer free of charge.

As of now the main benefit is that it protects the group from running spot instances that got significantly more expensive than when they were initially launched, but still somewhat less than the on-demand price.

For example:

on-demand price: 1

spot-price: 0.2 but increased to 0.9 over time

this instance would be terminated by this new policy as soon as the price was exceeding 0.22 (using the vehiculated 10% price buffer we want to use as default the bid price would be 0.22, instead of 1).

With the default policy the spot instance in the above example may run forever after this significant price increase unless the price goes over 1.

This allows us to find other spot instance types that may be priced better as of the moment when the price increased.

I'm going ahead with percentage value as it seems more intuitive. Is that cool @xlr-8 @cristim ?

xlr-8 · 2017-12-11T11:11:00Z

core/autoscaling.go

+	logger.Println("a.BiddingPolicy: ", a.BiddingPolicy)
+	if a.BiddingPolicy == "aggressive" {
+		logger.Println("Launching spot instance with a bid =", currentSpotPrice*a.region.conf.SpotPriceMultiplier)
+		return currentSpotPrice * a.region.conf.SpotPriceMultiplier


I realised that there might be a potential issue here, if the spot price is close to the on-demand price for some reasons (or the multiplier very high), then having the SpotPriceMultiplier might allow the bidding to go above that value. e.g.:

spotPrice: 0.5 onDemandPrice: 1 spotPriceMultiplier: 3.0 spotBid: => 1.5 # Other case spotPrice: 0.9 (couldn't find better) onDemandPrice: 1 spotPriceMultiplier: 1.5 spotBid: => 1.35

I think we might want to check that this potential maximum price isn't higher than the on-demand price, just for the sake of avoiding it; or we might end with spot instances running costing more than on-demand.

Good catch!

We can just take the minimum between ondemand price and the calculated spot price

Cool, will do.

cristim

I approve what I saw so far, although I can't try it out these days.

@xlr-8 do you have any concerns about this or can we merge it?

xlr-8

Couple questions/changes that seem worth checking Also, I haven't yet tested it.
However the overall logic seems good to me, did it react as you expected?

xlr-8 · 2017-12-12T22:36:06Z

terraform/variables.tf

@@ -14,6 +14,16 @@ variable "asg_on_demand_price_multiplier" {
  default     = "1.0"
 }

+variable "asg_spot_price_buffer_percentage" {
+  description = "Percentage above the current spot price to place the bid"
+  default = "1.1"


Isn't it supposed to be 10.0?

Ah yes, my bad. Will change and ping you.

xlr-8 · 2017-12-12T22:36:25Z

terraform/autospotting.tf

@@ -4,6 +4,8 @@ module "autospotting" {
  autospotting_min_on_demand_number       = "${var.asg_min_on_demand_number}"
  autospotting_min_on_demand_percentage   = "${var.asg_min_on_demand_percentage}"
  autospotting_on_demand_price_multiplier = "${var.asg_on_demand_price_multiplier}"
+  autospotting_spot_price_buffer_percentage = "${var.asg_spot_price_buffer_percentage}"
+  autospotting_bidding_policy = "${var.asg_bidding_policy}"


Could run terraform fmt -write=true to format it properly

xlr-8 · 2017-12-12T22:36:49Z

terraform/variables.tf

+  description = "Choice of bidding policy for the spot instance"
+  default = "normal"
+}
+


Could run terraform fmt -write=true on those as well; I shall update the doc on this point

xlr-8 · 2017-12-12T22:39:21Z

core/autoscaling_test.go

+			currentOnDemandPrice: 0.0464,
+			policy:               "aggressive",
+			want:                 0.02376,
+		},


Good testing of all cases! :)

xlr-8 · 2017-12-12T22:46:45Z

core/autoscaling.go

+		logger.Printf("Ignoring out of range value : %f\n", spotPriceBufferPercentage)
+	}
+	return DefaultSpotPriceBufferPercentage, false
+}


If you don't mind, I'd rather have the function rewritten as such:

func (a *autoScalingGroup) loadSpotPriceBufferPercentage(tagValue *string) (float64, bool) { spotPriceBufferPercentage, err := strconv.ParseFloat(*tagValue, 64) if err != nil { logger.Printf("Error with ParseFloat: %s\n", err.Error()) return DefaultSpotPriceBufferPercentage, false } else if spotPriceBufferPercentage <= 0 { logger.Printf("Ignoring out of range value : %f\n", spotPriceBufferPercentage) return DefaultSpotPriceBufferPercentage, false } logger.Printf("Loaded SpotPriceBufferPercentage value to %f from tag %s\n", spotPriceBufferPercentage, SpotPriceBufferPercentageTag) return spotPriceBufferPercentage, true }

I believe it works the same way as yours, but just seems more clear to me

xlr-8 · 2017-12-12T22:55:15Z

core/autoscaling.go

+
+	// DefaultBiddingPolicy stores the default bidding policy for
+	// the spot bid on a per-group level
+	DefaultBiddingPolicy = "normal"


I think those new variables (except SpotPriceBufferPercentageTag) don't need to be exported.
It's most likely true for other older one as well, would need to be corrected. (Opened https://github.com/cristim/autospotting/issues/186)

Should I leave it as is in this pull request?

I believe it's fine like this, I'll pass on those later on

xlr-8 · 2017-12-12T23:10:51Z

core/autoscaling.go

+
+	return baseOnDemandPrice
+}
+


I would also rewrite that as:

func (a *autoScalingGroup) getPricetoBid( baseOnDemandPrice float64, currentSpotPrice float64) float64 { logger.Println("BiddingPolicy: ", a.region.conf.BiddingPolicy) if a.region.conf.BiddingPolicy == DefaultBiddingPolicy { logger.Println("Launching spot instance with a bid =", baseOnDemandPrice) return baseOnDemandPrice } logger.Println("Launching spot instance with a bid =", math.Min(baseOnDemandPrice, currentSpotPrice*(1.0+a.region.conf.SpotPriceBufferPercentage/100.0))) return math.Min(baseOnDemandPrice, currentSpotPrice*(1.0+a.region.conf.SpotPriceBufferPercentage/100.0)) }

Please note I removed the check of the value, as it seemed already done here: https://github.com/cristim/autospotting/pull/175/files#diff-de828bac595094756ea2a28fa415583bR219

My logic is always to avoid imbricated if, if there is an error check or direct return, then do it. I find that it usually simplifies reading/understanding of the code.

kartik894 · 2017-12-13T04:37:29Z

@xlr-8 @cristim Apart from running unit tests, I tested it on AWS too. Works fine. Ran the terraform fmt command to clean the formatting too.

xlr-8 · 2017-12-13T08:14:19Z

Cool, code looks good to me, please:

fix the code climate issues
rebase the PR for git log history purposes

On my side, I'll give it a quick shot tonight on AWS and I'll merge if everything worked as expected.

kartik894 · 2017-12-13T08:59:52Z

Will do the rebasing. The code climate points to cognitive complexity in two sections of the code which I'm not sure how to fix.

xlr-8 · 2017-12-13T09:29:19Z

core/autoscaling.go

+		debug.Println("Couldn't find tag", SpotPriceBufferPercentageTag)
+	}
+	return false
+}


I believe this could be rewritten as:

func (a *autoScalingGroup) loadConfSpotPrice() bool { tagValue := a.getTagValue(SpotPriceBufferPercentageTag) if tagValue == nil { return false } newValue, done := a.loadSpotPriceBufferPercentage(tagValue) if !done { debug.Println("Couldn't find tag", SpotPriceBufferPercentageTag) return false } a.region.conf.SpotPriceBufferPercentage = newValue return done }

Which would also solve code climate issue I believe, as the comment about imbricated if/else is something that adds complexity to the code and that code climate dislike.

xlr-8 · 2017-12-13T09:40:49Z

core/autoscaling.go

+					logger.Println("BiddingPolicy =", a.region.conf.BiddingPolicy)
+					return done
+				}
+			}
 		} else {
 			debug.Println("Couldn't find tag", tagKey)
 		}
 	}
 	return false
 }


Applying the same principle as before, it could be re-written as:

func (a *autoScalingGroup) loadConfSpot() bool { tagList := []string{BiddingPolicyTag} loadDyn := map[string]func(*string) (string, bool){ BiddingPolicyTag: a.loadBiddingPolicy, } for _, tagKey := range tagList { tagValue := a.getTagValue(tagKey) if tagValue == nil { debug.Println("Couldn't find tag", tagKey) continue } if _, ok := loadDyn[tagKey]; !ok { debug.Println("No method associated with tag", tagKey) continue } if newValue, done := loadDyn[tagKey](tagValue); done { a.region.conf.BiddingPolicy = newValue logger.Println("BiddingPolicy =", a.region.conf.BiddingPolicy) return done } } return false }

Or considering we have only one policy, even as simple as:

func (a *autoScalingGroup) loadConfSpot() bool { tagValue := a.getTagValue(BiddingPolicyTag) if tagValue == nil { debug.Println("Couldn't find tag", BiddingPolicyTag) return false } if newValue, done := a.loadBiddingPolicy(tagValue); done { a.region.conf.BiddingPolicy = newValue logger.Println("BiddingPolicy =", a.region.conf.BiddingPolicy) return done } return false }

I presume both could help/should solve the codeClimate issue

The first one still failed the CodeClimate run.... So, I used the second one.

xlr-8 · 2017-12-13T22:03:17Z

Tests on AWS worked as expected! Waiting for the rebase before giving it a final look/merge. Thanks again!

It is now possible to bid at a value based on the spot instance price ('aggressive' policy) rather than on-demand price ('normal' policy). Thus allowing to bid for a price slightly higher than the spot price. This is particularly useful to avoid having spot instances that cost too much. To enable this, the policy has to be set as 'aggressive' ('normal' default) and a 'buffer percentage' needs to be defined (10% higher default) to know how much more users are willing to pay above the spot price found. Details about the tags are provided in START.md. Those parameters are configurable from Cloudfront/Terraform and the ASGs.

coveralls · 2017-12-13T23:12:39Z

Coverage increased (+1.2%) to 73.455% when pulling 17a39d1 on kartik894:master into 0898534 on cristim:master.

xlr-8 · 2017-12-13T23:19:05Z

All good! 👍 🎉

cristim

@kartik894 Huge thanks for this contribution, this is a great feature and I'm really glad to finally see it land.

kartik894 · 2017-12-14T20:27:53Z

My pleasure 👍

cristim requested changes Dec 3, 2017

View reviewed changes

xlr-8 added the Status: Need review label Dec 3, 2017

xlr-8 mentioned this pull request Dec 4, 2017

Create NewObject methods & improve (default) conf loading #180

Open

xlr-8 suggested changes Dec 4, 2017

View reviewed changes

xlr-8 reviewed Dec 5, 2017

View reviewed changes

core/autoscaling_test.go Outdated

}

}

}

Copy link

Contributor

xlr-8 Dec 5, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would need to be updated to fetch various decided policy names

cristim requested changes Dec 10, 2017

View reviewed changes

xlr-8 reviewed Dec 11, 2017

View reviewed changes

cristim approved these changes Dec 12, 2017

View reviewed changes

xlr-8 suggested changes Dec 12, 2017

View reviewed changes

kartik894 changed the title ~~Added SpotInstance Multipler with dynamic tags~~ Added SpotInstance Percentage Multiplier with dynamic tags Dec 13, 2017

xlr-8 reviewed Dec 13, 2017

View reviewed changes

p-kar force-pushed the master branch 3 times, most recently from e8e87e0 to a1cac1e Compare December 13, 2017 22:49

kartik894 force-pushed the master branch from a1cac1e to 17a39d1 Compare December 13, 2017 23:09

LeanerCloud deleted a comment from coveralls Dec 13, 2017

xlr-8 approved these changes Dec 13, 2017

View reviewed changes

xlr-8 merged commit 053135e into LeanerCloud:master Dec 13, 2017

cristim reviewed Dec 14, 2017

View reviewed changes

Added SpotInstance Percentage Multiplier with dynamic tags #175

Added SpotInstance Percentage Multiplier with dynamic tags #175

Conversation

kartik894 commented Dec 3, 2017 • edited Loading

Issue Type

Summary

Code contribution checklist

cristim 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

xlr-8 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

xlr-8 Dec 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xlr-8 Dec 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xlr-8 Dec 3, 2017 • edited Loading

Choose a reason for hiding this comment

xlr-8 Dec 4, 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

xlr-8 Dec 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xlr-8 Dec 4, 2017 • edited Loading

Choose a reason for hiding this comment

cristim commented Dec 4, 2017

xlr-8 commented Dec 5, 2017

xlr-8 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

kartik894 Dec 11, 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

cristim left a comment

Choose a reason for hiding this comment

xlr-8 left a comment • 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

kartik894 commented Dec 13, 2017 • edited Loading

xlr-8 commented Dec 13, 2017

kartik894 commented Dec 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xlr-8 commented Dec 13, 2017

coveralls commented Dec 13, 2017

xlr-8 commented Dec 13, 2017

cristim left a comment

Choose a reason for hiding this comment

kartik894 commented Dec 14, 2017

kartik894 commented Dec 3, 2017 •

edited

Loading

xlr-8 Dec 11, 2017 •

edited

Loading

xlr-8 Dec 3, 2017 •

edited

Loading

xlr-8 Dec 3, 2017 •

edited

Loading

xlr-8 Dec 4, 2017 •

edited

Loading

xlr-8 Dec 4, 2017 •

edited

Loading

xlr-8 Dec 4, 2017 •

edited

Loading

kartik894 Dec 11, 2017 •

edited

Loading

xlr-8 left a comment •

edited

Loading

kartik894 commented Dec 13, 2017 •

edited

Loading