[WIP] Add Spot/On-demand price to table-wide output irrespective of the price filters #78
Conversation
…ce filters * Minor clean-up
krishna-birla
changed the title
There was a problem hiding this comment.
Overall very nice refactor and thanks for contributing to this project!! I've left a few minor comments in-line.
It looks like the unit-tests are also failing. There's some test coverage missing on the output, selector, and ec2-pricing pkgs too. Would you mind upping the coverage for those packages for this enhancement?
I commented about how to fix the pricing endpoint, no obligation for you fix it, but if you'd like to include that fix or open a new PR, feel free. If not, I'll fix that sometime this week.
Thanks again!
| import ( | ||
| "fmt" | ||
| "github.com/aws/aws-sdk-go/aws/endpoints" | ||
| "github.com/aws/aws-sdk-go/service/lightsail" |
There was a problem hiding this comment.
why is lightsail brought in here? I see it added to the main struct as well, but no use for it as far as I can tell.
|
|
||
| import ( | ||
| "fmt" | ||
| "github.com/aws/aws-sdk-go/aws/endpoints" |
There was a problem hiding this comment.
can you move these non-standard lib imports to the next stanza?
There was a problem hiding this comment.
Sure. Will add that to my fmt rules for this project.
| spotCache map[string]map[string][]spotPricingEntry | ||
| PricingClient pricingiface.PricingAPI | ||
| EC2Client ec2iface.EC2API | ||
| LightsailClient lightsailiface.LightsailAPI |
| // In simple words, make sure they don't write the same variable/file/row etc. which they don't (they have different cache maps) | ||
| HydrateOndemandCache() error | ||
| HydrateSpotCache(days int) error | ||
| LastOnDemandCachedUTC() *time.Time |
There was a problem hiding this comment.
can you change to LastOnDemandCacheUTC removing the d. Same for the spot timestamp
| AWSSession *session.Session | ||
| cache map[string]float64 | ||
| spotCache map[string]map[string][]spotPricingEntry | ||
| PricingClient pricingiface.PricingAPI |
There was a problem hiding this comment.
To fix https://docs.aws.amazon.com/awsaccountbilling/latest/aboutv2/using-pelong.html#pe-endpoint , I believe we can just copy the session and change the region to always use us-east-1 just for the PricingClient. The filters already include the location. I didn't realize pricing was a global endpoint :) I can do this fix if you'd like.
There was a problem hiding this comment.
Yes, I realized this. Will make the change and other changes.
| return nil, err | ||
| } | ||
| var locations []string | ||
| var locations, aZones []string |
There was a problem hiding this comment.
can we call aZones one of these instead: zones, availabilityZones, azs?
|
Hey @krishna-birla , just wanted to check in and see if there's anything I can help with to get this merged? No worries if you've just been busy! |
Unfortunately it is the later, typical end of quarter. Anyway, will dedicate this week to it. When do you plan to release? |
|
I'm in no rush, take your time and let me know if you need any help! :) |
|
Hey, just checking in. If you won't have the time to work on this soon, I could merge this PR and then I could work on a follow up to address some of the comments in this PR. Would that be okay with you? |
|
The UTs need changes. I hit a roadblock with just one thing: internal function to get price does not know when it was called by CLI (i.e. via main function) and when it was called directly by SDK. Because for SDK callers, we don't cache but want to report prices all the same. And so I was trying to make internal function aware of this, which is what I was trying with "LastCachedUTC()" function. This is what I was working on last around 3 weeks ago, but I was tested covid +ve and had to halt with it 😅. I don't know if we can merge this as is, as tests are failing and some finishing is required. If you want you can close this (and I will re-open it when I am able to) and mean while submit the much smaller fix for |
|
I may get some time this weekend, or next week to find a good solution for "LastCachedUTC()" stuff. 🤞🏽 |
|
I just saw this: #84 (comment). I'll try to prioritise. |
|
Made the pricing client endpoint change here: #85 |
* remove the importing of un-used Lightsail packages, as [requested by @bwagner](aws#78 (comment)) * move non-standard lib imports to their own stanza, as [requested by @bwagner](aws#78 (comment)) Signed-off-by: Mike Ball <[email protected]>
This was requested by @bwagner here: aws#78 (comment) Signed-off-by: Mike Ball <[email protected]>
Per code review from @bwagner, this renames various methods and fields to be `*CacheUTC` rather than `*CachedUTC`: aws#78 (comment)
By populating the `lastOnDemandCacheUTC` and `lastSpotCacheUTC` fields with non-`nil` values, the `Filter` tests exercise relevant code paths. Note that this doesn't change @krishna-birla's original implementation (see PR aws#78) or vision; it only ensures the tests pass. Signed-off-by: Mike Ball <[email protected]>
…ce filters (#106) * Add Spot/On-demand price to table-wide output irrespective of the price filters * Minor clean-up * adjust ec2pricing imports * remove the importing of un-used Lightsail packages, as [requested by @bwagner](#78 (comment)) * move non-standard lib imports to their own stanza, as [requested by @bwagner](#78 (comment)) Signed-off-by: Mike Ball <[email protected]> * rename aZones var to 'availabilityZones' This was requested by @bwagner here: #78 (comment) Signed-off-by: Mike Ball <[email protected]> * rename fields/methods `*CacheUTC` Per code review from @bwagner, this renames various methods and fields to be `*CacheUTC` rather than `*CachedUTC`: #78 (comment) * update selector to use `LastSpotCacheUTC()` Previously, `selector` was incorrectly using the old `LastSpotCachedUTC` method name. * make `ec2PricingMock` proper `EC2PricingIface` * remove invalid 'FIXME' comment This is now fixed, as the Pricing client is always initialized to us-east-1. Signed-off-by: Mike Ball <[email protected]> * move error check inside loop Per [code review feedback](#106 (comment)), the error checking should appear within the `range pricingOutput.PriceList`. Signed-off-by: Mike Ball <[email protected]> * remove invalid 'FIXME' code comment This issue is now resolved. Signed-off-by: Mike Ball <[email protected]> * commit `go mod tidy` results Signed-off-by: Mike Ball <[email protected]> * protect against panics during unit tests This protects against panics like the following, for example: ``` --- FAIL: TestFilter_X8664_AMD64 (0.00s) panic: runtime error: invalid memory address or nil pointer dereference [recovered] panic: runtime error: invalid memory address or nil pointer dereference [signal SIGSEGV: segmentation violation code=0x1 addr=0x38 pc=0x153ec41] goroutine 153 [running]: testing.tRunner.func1.2({0x1613ee0, 0x1cdfe70}) /usr/local/Cellar/go/1.17.2/libexec/src/testing/testing.go:1209 +0x24e testing.tRunner.func1() /usr/local/Cellar/go/1.17.2/libexec/src/testing/testing.go:1212 +0x218 panic({0x1613ee0, 0x1cdfe70}) /usr/local/Cellar/go/1.17.2/libexec/src/runtime/panic.go:1038 +0x215 github.com/aws/amazon-ec2-instance-selector/v2/pkg/selector.Selector.rawFilter.func1(0x100e6e7, 0x68) /Users/mball/dev/go/src/github.com/aws/amazon-ec2-instance-selector/pkg/selector/selector.go:192 +0x241 github.com/aws/amazon-ec2-instance-selector/v2/pkg/selector_test.mockedEC2.DescribeInstanceTypesPages(...) /Users/mball/dev/go/src/github.com/aws/amazon-ec2-instance-selector/pkg/selector/selector_test.go:70 github.com/aws/amazon-ec2-instance-selector/v2/pkg/selector.Selector.rawFilter({{_, _}, {_, _}, {_}}, {0x0, 0x0, 0x0, 0xc0003854a0, 0x0, ...}) /Users/mball/dev/go/src/github.com/aws/amazon-ec2-instance-selector/pkg/selector/selector.go:184 +0x5c2 github.com/aws/amazon-ec2-instance-selector/v2/pkg/selector.Selector.FilterWithOutput({{_, _}, {_, _}, {_}}, {0x0, 0x0, 0x0, 0xc0003854a0, 0x0, ...}, ...) /Users/mball/dev/go/src/github.com/aws/amazon-ec2-instance-selector/pkg/selector/selector.go:116 +0xc5 github.com/aws/amazon-ec2-instance-selector/v2/pkg/selector.Selector.Filter({{_, _}, {_, _}, {_}}, {0x0, 0x0, 0x0, 0xc0003854a0, 0x0, ...}) /Users/mball/dev/go/src/github.com/aws/amazon-ec2-instance-selector/pkg/selector/selector.go:98 +0xcc github.com/aws/amazon-ec2-instance-selector/v2/pkg/selector_test.TestFilter_X8664_AMD64(0xc0002de820) /Users/mball/dev/go/src/github.com/aws/amazon-ec2-instance-selector/pkg/selector/selector_test.go:583 +0x14e testing.tRunner(0xc00059b520, 0x177e1b0) /usr/local/Cellar/go/1.17.2/libexec/src/testing/testing.go:1259 +0x102 created by testing.(*T).Run /usr/local/Cellar/go/1.17.2/libexec/src/testing/testing.go:1306 +0x35a exit status 2 ``` Signed-off-by: Mike Ball <[email protected]> * report received value in test output This seeks to make the specifics of test failures a bit more clear. Signed-off-by: Mike Ball <[email protected]> * run `go mod tidy` This addresses the following: ``` $ make unit-test go test -bench=. /Users/mball/dev/go/src/github.com/aws/amazon-ec2-instance-selector//... -v -coverprofile=coverage.out -covermode=atomic -outputdir=/Users/mball/dev/go/src/github.com/aws/amazon-ec2-instance-selector//build go: updates to go.mod needed; to update it: go mod tidy make: *** [unit-test] Error 1 ``` Signed-off-by: Mike Ball <[email protected]> * fix failing selector.Filter unit tests By populating the `lastOnDemandCacheUTC` and `lastSpotCacheUTC` fields with non-`nil` values, the `Filter` tests exercise relevant code paths. Note that this doesn't change @krishna-birla's original implementation (see PR #78) or vision; it only ensures the tests pass. Signed-off-by: Mike Ball <[email protected]> Co-authored-by: Krishna Birla <[email protected]>
|
Finished in #106 Thanks for the collab on this @krishna-birla ! |
2 participants
Side effects:
.gitignorego.uber.org/multierr v1.1.0Issue, if available:
#71
Description of changes:
To show Spot and On Demand prices in table wide output irrespective of the price filters being set; if the filters are set, do filtration normally depending on usage class. For other output modes, do filtration normally depending on usage class.
Testing:
Table wide output - us-east-1 (both cache was hydrated)
Table wide output - ap-south-1 (both cache was hydrated)
Table output - ap-south-1 (both cache was NOT hydrated)
Table wide output with price filter - ap-south-1 (both cache was hydrated)
Table wide output with price filter and usage class - ap-south-1 (both cache was hydrated)
Table wide output with price range filter - ap-south-1 (both cache was hydrated)
Table output with price range filter - ap-south-1 (only on-demand cache was hydrated)
Table wide output with price range filter and usage class - ap-south-1 (both cache was hydrated)
Table output with price range filter and usage class - ap-south-1 (only spot cache was hydrated)
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.