Skip to content

Instance sorting #138

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 12 commits into from
Closed

Instance sorting #138

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
6c596cd
added instance type sorting
Jul 7, 2022
8ea3f10
readded examples and selector tests
Jul 7, 2022
ef7b781
added testing files for sortin tests
Jul 7, 2022
6c5ce45
updated readme code example and help printout
Jul 7, 2022
5035033
fixed typo in TruncateResults test
Jul 7, 2022
27a434d
made sorting flags private
Jul 8, 2022
f0fcb1b
fixed typos in comments
Jul 8, 2022
36bbfb3
fixed selector_test method typo
Jul 8, 2022
60fdce7
fixed overall typos and fixed structure of SortInstanceTypes
Jul 14, 2022
2871abf
refactored sorting filter validation
Jul 15, 2022
244789b
renamed validateFilter into validateSortFilter
Jul 15, 2022
bdf0a3e
functioning json path sorting solution
Jul 20, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
110 changes: 30 additions & 80 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -241,87 +241,24 @@ Suite Flags:


Global Flags:
--cache-dir string Directory to save the pricing and instance type caches (default "~/.ec2-instance-selector/")
--cache-ttl int Cache TTLs in hours for pricing and instance type caches. Setting the cache to 0 will turn off caching and cleanup any on-disk caches. (default 168)
-h, --help Help
--max-results int The maximum number of instance types that match your criteria to return (default 20)
-o, --output string Specify the output format (table, table-wide, one-line, simple) (default "simple")
--profile string AWS CLI profile to use for credentials and config
-r, --region string AWS Region to use for API requests (NOTE: if not passed in, uses AWS SDK default precedence)
-v, --verbose Verbose - will print out full instance specs
--version Prints CLI version
--cache-dir string Directory to save the pricing and instance type caches (default "~/.ec2-instance-selector/")
--cache-ttl int Cache TTLs in hours for pricing and instance type caches. Setting the cache to 0 will turn off caching and cleanup any on-disk caches. (default 168)
-h, --help Help
--max-results int The maximum number of instance types that match your criteria to return (default 20)
-o, --output string Specify the output format (table, table-wide, one-line, simple) (default "simple")
--profile string AWS CLI profile to use for credentials and config
-r, --region string AWS Region to use for API requests (NOTE: if not passed in, uses AWS SDK default precedence)
--sort-direction string Specify the direction to sort in (ascending, descending) (default "ascending")
--sort-filter string Specify the field to sort by (on-demand-price, spot-price, vcpu, memory, instance-type-name) (default "instance-type-name")
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: what do you think about changing this to sort-by ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah I agree with this change. sort-by makes more intuitive sense for what the purpose of the flag is.

-v, --verbose Verbose - will print out full instance specs
--version Prints CLI version
```


### Go Library

This is a minimal example of using the instance selector go package directly:

**NOTE:** The example below is intended for `v3+`. For versions `v2.3.1` and earlier, refer to the following dropdown:
<details>
<summary>Example for v2.3.1</summary>

```go
package main

import (
"fmt"

"github.com/aws/amazon-ec2-instance-selector/v2/pkg/bytequantity"
"github.com/aws/amazon-ec2-instance-selector/v2/pkg/selector"
"github.com/aws/aws-sdk-go/aws"
"github.com/aws/aws-sdk-go/aws/session"
)

func main() {
// Load an AWS session by looking at shared credentials or environment variables
// https://docs.aws.amazon.com/sdk-for-go/api/aws/session/
sess, err := session.NewSession(&aws.Config{
Region: aws.String("us-east-2"),
})
if err != nil {
fmt.Printf("Oh no, AWS session credentials cannot be found: %v", err)
return
}

// Instantiate a new instance of a selector with the AWS session
instanceSelector := selector.New(sess)

// Instantiate an int range filter to specify min and max vcpus
vcpusRange := selector.IntRangeFilter{
LowerBound: 2,
UpperBound: 4,
}
// Instantiate a byte quantity range filter to specify min and max memory in GiB
memoryRange := selector.ByteQuantityRangeFilter{
LowerBound: bytequantity.FromGiB(2),
UpperBound: bytequantity.FromGiB(4),
}
// Create a string for the CPU Architecture so that it can be passed as a pointer
// when creating the Filter struct
cpuArch := "x86_64"

// Create a Filter struct with criteria you would like to filter
// The full struct definition can be found here for all of the supported filters:
// https://github.com/aws/amazon-ec2-instance-selector/blob/main/pkg/selector/types.go
filters := selector.Filters{
VCpusRange: &vcpusRange,
MemoryRange: &memoryRange,
CPUArchitecture: &cpuArch,
}

// Pass the Filter struct to the Filter function of your selector instance
instanceTypesSlice, err := instanceSelector.Filter(filters)
if err != nil {
fmt.Printf("Oh no, there was an error :( %v", err)
return
}
// Print the returned instance types slice
fmt.Println(instanceTypesSlice)
}
```
</details>

**cmd/examples/example1.go**
```go#cmd/examples/example1.go
package main
Expand Down Expand Up @@ -374,19 +311,32 @@ func main() {
}

// Pass the Filter struct to the FilteredInstanceTypes function of your
// selector instance to get a list of filtered instance types and their details
// selector instance to get a list of filtered instance types and their details.
instanceTypesSlice, err := instanceSelector.FilterInstanceTypes(filters)
if err != nil {
fmt.Printf("Oh no, there was an error getting instance types: %v", err)
return
}

// Pass in your list of instance type details to the appropriate output function
// in order to format the instance types as printable strings.
maxResults := 100
// Pass in the list of instance type details to the SortInstanceTypes function
// if you wish to sort the instances based on set filters.
sortFilter := "instance-type-name"
sortDirection := "ascending"
instanceTypesSlice, err = instanceSelector.SortInstanceTypes(instanceTypesSlice, &sortFilter, &sortDirection)
if err != nil {
fmt.Printf("Oh no, there was an error sorting instance types: %v", err)
return
}

// Truncate results and format them for output with your desired formatting function.
// All formatting functions can be found here:
// https://github.com/aws/amazon-ec2-instance-selector/blob/main/pkg/selector/outputs/outputs.go
// Examples of formatted outputs can be found here:
// https://github.com/aws/amazon-ec2-instance-selector#examples
maxResults := 10
instanceTypesSlice, _, err = outputs.TruncateResults(&maxResults, instanceTypesSlice)
if err != nil {
fmt.Printf("Oh no, there was an error truncating instnace types: %v", err)
fmt.Printf("Oh no, there was an error truncating instance types: %v", err)
return
}
instanceTypes := outputs.SimpleInstanceTypeOutput(instanceTypesSlice)
Expand All @@ -403,7 +353,7 @@ func main() {
$ git clone https://github.com/aws/amazon-ec2-instance-selector.git
$ cd amazon-ec2-instance-selector/
$ go run cmd/examples/example1.go
[c4.large c5.large c5a.large c5ad.large c5d.large c6i.large t2.medium t3.medium t3.small t3a.medium t3a.small]
[c4.large c5.large c5a.large c5ad.large c5d.large c6a.large c6i.large c6id.large t2.medium t3.medium]
```

## Building
Expand Down
23 changes: 18 additions & 5 deletions cmd/examples/example1.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,19 +48,32 @@ func main() {
}

// Pass the Filter struct to the FilteredInstanceTypes function of your
// selector instance to get a list of filtered instance types and their details
// selector instance to get a list of filtered instance types and their details.
instanceTypesSlice, err := instanceSelector.FilterInstanceTypes(filters)
if err != nil {
fmt.Printf("Oh no, there was an error getting instance types: %v", err)
return
}

// Pass in your list of instance type details to the appropriate output function
// in order to format the instance types as printable strings.
maxResults := 100
// Pass in the list of instance type details to the SortInstanceTypes function
// if you wish to sort the instances based on set filters.
sortFilter := "instance-type-name"
sortDirection := "ascending"
instanceTypesSlice, err = instanceSelector.SortInstanceTypes(instanceTypesSlice, &sortFilter, &sortDirection)
Copy link
Contributor

Choose a reason for hiding this comment

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

thinking on this more, I think that we should make this compatible w/ v2 of instance-selector. The original Filter functions could maintain functionality. In fact, I think it makes sense to decouple sorting from the selector pkg all together. It's trivial to sort the instanceTypesSlice when using the SDK and is probably a better experience since you know exactly how it's operating. So sorting should really be part of the CLI which just be a incremental feature that doesn't break backwards compatibility.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This does make sense to me how a user of the instance-selector as a library could just manually sort the instance types themselves, so I do agree that sorting could just be part of the CLI. The idea of adding sorting to v2 is also one that I definitely agree with. There is one issue: in order to reduce the number of API calls relating to fetching spot and OD pricing, the pricing caches are only hydrated if the user sets the PricePerHour filter. In the current implementation of sorting, if sorting by pricing is desired, pricing caches will be refreshed if needed, but if we were to leave this to the users, then they will not be able to sort by price in call cases. I feel as though this might go against the user expectation of what information is included in the returned instance types. However, this can be solved by mentioning this characteristic of the PricePerHour filter in the documentation (comments in filtering method, in Filters struct, and possibly the readme). Would just documentation be enough for this or would a different implementation of filtering instance types be necessary?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think docs would be fine for now. If there's a strong signal from users that the limitation is hindering the use of the lib, then we can reevaluate at that time.

if err != nil {
fmt.Printf("Oh no, there was an error sorting instance types: %v", err)
return
}

// Truncate results and format them for output with your desired formatting function.
// All formatting functions can be found here:
// https://github.com/aws/amazon-ec2-instance-selector/blob/main/pkg/selector/outputs/outputs.go
// Examples of formatted outputs can be found here:
// https://github.com/aws/amazon-ec2-instance-selector#examples
maxResults := 10
instanceTypesSlice, _, err = outputs.TruncateResults(&maxResults, instanceTypesSlice)
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we really need this for outputs. It's trivial to truncate a slice. It's not clear why this returns 3 values too. What error could be returned from a truncation?

Copy link
Contributor Author

@digocorbellini digocorbellini Jul 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think we really need this for outputs. It's trivial to truncate a slice.

That is a good point. I am going to move TruncateResults to being a private function for the CLI tool to use.

It's not clear why this returns 3 values too. What error could be returned from a truncation?

The 3 values are the truncated list, the number of truncated items, and an error. The error exists to catch invalid maxResults such as negative numbers.

Copy link
Contributor

Choose a reason for hiding this comment

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

For this type of func, I'd drop the error and do something reasonable.

For example,

TruncateResults(-1, 5results) => [], 5
TruncateResults(0, 5results) => [], 5
TruncateResults(10, 5results) => [1,2,3,4,5], 0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh that is a very good idea. I'll change the output of TruncateResults to only output the truncated list and the number of truncated items while ensuring that a reasonable output occurs with previously "invalid" maxResults values.

if err != nil {
fmt.Printf("Oh no, there was an error truncating instnace types: %v", err)
fmt.Printf("Oh no, there was an error truncating instance types: %v", err)
return
}
instanceTypes := outputs.SimpleInstanceTypeOutput(instanceTypesSlice)
Expand Down
55 changes: 46 additions & 9 deletions cmd/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,15 +100,17 @@ const (

// Configuration Flag Constants
const (
maxResults = "max-results"
profile = "profile"
help = "help"
verbose = "verbose"
version = "version"
region = "region"
output = "output"
cacheTTL = "cache-ttl"
cacheDir = "cache-dir"
maxResults = "max-results"
profile = "profile"
help = "help"
verbose = "verbose"
version = "version"
region = "region"
output = "output"
cacheTTL = "cache-ttl"
cacheDir = "cache-dir"
sortDirection = "sort-direction"
sortFilter = "sort-filter"

// Output constants

Expand All @@ -117,6 +119,17 @@ const (
oneLineOutput = "one-line"
simpleOutput = "simple"
verboseOutput = "verbose"

// Sorting constants

sortODPrice = "on-demand-price"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think these should be more generic where you could select an arbitrary jsonpath of the struct def. For example, if I wanted to sort on memory I could do:

ec2-instance-selector --vcpus-min=4 --sort-by='.MemoryInfo.SizeInMiB'

maybe using this lib: https://github.com/oliveagle/jsonpath

I think some flag shorthands could be modeled as well, but it needs to be more generic than hardcoded options. Maybe you could just make all quantity fields sortable by inspecting the type and then using the base of the option to determine the sort key.

For example,

ec2-instance-selector --vcpu=4 --sort-by='vcpus'
ec2-instance-selector --vcpu=4 --sort-by='ebs-optimized-basedline-bandwidth'

Here's an example of this in the kubectl cli.
https://kubernetes.io/docs/reference/kubectl/cheatsheet/#:~:text=kubectl%20get%20services%20%2D%2Dsort%2Dby%3D.metadata.name

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Woah yea this is a great idea! I'll look into changing sorting to be more generic.

Copy link
Contributor

@bwagner5 bwagner5 Jul 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

stretch goal is to allow multiple sort dimensions. For example, if I wanted to sort by memory and then by vcpus when memory is equal, I could do this:

ec2-instance-selector --vcpus-min=4 --sort-by='.MemoryInfo.SizeInMiB, vcpus'

:)

sortSpotPrice = "spot-price"
sortVCPU = "vcpu"
sortMemory = "memory"
sortName = "instance-type-name"

sortAscending = "ascending"
Copy link
Contributor

Choose a reason for hiding this comment

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

can you make shorthands for ascending and descending (i.e.asc and desc) in addition to the fully spelled out.

sortDescending = "descending"
)

var (
Expand Down Expand Up @@ -147,6 +160,19 @@ Full docs can be found at github.com/aws/amazon-` + binName
simpleOutput,
}

cliSortCriteria := []string{
sortODPrice,
sortSpotPrice,
sortVCPU,
sortMemory,
sortName,
}

cliSortDirections := []string{
sortAscending,
sortDescending,
}

// Registers flags with specific input types from the cli pkg
// Filter Flags - These will be grouped at the top of the help flags

Expand Down Expand Up @@ -211,6 +237,8 @@ Full docs can be found at github.com/aws/amazon-` + binName
cli.ConfigBoolFlag(verbose, cli.StringMe("v"), nil, "Verbose - will print out full instance specs")
cli.ConfigBoolFlag(help, cli.StringMe("h"), nil, "Help")
cli.ConfigBoolFlag(version, nil, nil, "Prints CLI version")
cli.ConfigStringOptionsFlag(sortDirection, nil, cli.StringMe(sortAscending), fmt.Sprintf("Specify the direction to sort in (%s)", strings.Join(cliSortDirections, ", ")), cliSortDirections)
cli.ConfigStringOptionsFlag(sortFilter, nil, cli.StringMe(sortName), fmt.Sprintf("Specify the field to sort by (%s)", strings.Join(cliSortCriteria, ", ")), cliSortCriteria)

// Parses the user input with the registered flags and runs type specific validation on the user input
flags, err := cli.ParseAndValidateFlags()
Expand Down Expand Up @@ -335,6 +363,15 @@ Full docs can be found at github.com/aws/amazon-` + binName
os.Exit(1)
}

// sort instance types
sortFilterFlag := cli.StringMe(flags[sortFilter])
sortDirectionFlag := cli.StringMe(flags[sortDirection])
instanceTypeDetails, err = instanceSelector.SortInstanceTypes(instanceTypeDetails, sortFilterFlag, sortDirectionFlag)
if err != nil {
fmt.Printf("An error occurred when sorting instance types: %v", err)
os.Exit(1)
}

// format instance types as strings
maxOutputResults := cli.IntMe(flags[maxResults])
instanceTypes, itemsTruncated, err := formatInstanceTypes(instanceTypeDetails, maxOutputResults, outputFlag)
Expand Down
4 changes: 2 additions & 2 deletions pkg/selector/outputs/outputs_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,7 @@ func TestTruncateResults(t *testing.T) {
h.Assert(t, numTrucated == 0, fmt.Sprintf("Should truncate 0 results, but actually truncated: %d results", numTrucated))
}

func TestFormatInstanceTypes_NegativeMaxResults(t *testing.T) {
func TestTruncateResults_NegativeMaxResults(t *testing.T) {
instanceTypes := getInstanceTypes(t, "25_instances.json")

maxResults := aws.Int(-1)
Expand All @@ -157,7 +157,7 @@ func TestFormatInstanceTypes_NegativeMaxResults(t *testing.T) {
h.Assert(t, numTrucated == 0, fmt.Sprintf("No results should be truncated, but %d results were truncated", numTrucated))
}

func TestFormatInstanceTypes_NilMaxResults(t *testing.T) {
func TestTrucateResults_NilMaxResults(t *testing.T) {
instanceTypes := getInstanceTypes(t, "25_instances.json")

var maxResults *int = nil
Expand Down
Loading