Skip to content
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

Jump to bottom

Fix/price multiplier #486

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Fix/price multiplier #486

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5faaa6b
use updated ec2-instances-info
mello7tre Mar 3, 2022
93ce118
update
mello7tre Mar 3, 2022
677930c
test for fixing price multiplier that stopped working after 6938027
mello7tre Apr 12, 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
2 changes: 1 addition & 1 deletion core/instance_queries.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ func (i *instance) belongsToEnabledASG() bool {
asg.loadLaunchConfiguration()
asg.loadLaunchTemplate()
i.asg = &asg
i.price = i.typeInfo.pricing.onDemand
i.price = i.typeInfo.pricing.onDemand / i.region.conf.OnDemandPriceMultiplier * i.asg.config.OnDemandPriceMultiplier
Copy link
Member

Choose a reason for hiding this comment

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

Did you test this?

We used to have an issue with this logic(it used to consider the square of the multiplier value instead of the actual value) which I tried to fix a few months ago, but the current fix only considers the parameter from the Lambda environment variables, not from the tag-based ASG overrides.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, i had a windows ASG (unfortunately) that used a mutiplier (by tag) of 1.3.
It stopped working and AutoSpotting was no more able to launch spot replacement.
After i deployed the PR version, it started working again.

Linux price for t3a.small are:
{'t3a.small': {'memory': 2.0, 'price': 0.0204, 'spot': 1, 'spotprice': 0.0071, 'vcpu': 2}}
Before change:

           2022/04/12 12:16:59 instance_queries.go:22: Comparing price spot/instance:
           2022/04/12 12:16:59 instance_queries.go:29:  Spot price:  0.0071
           2022/04/12 12:16:59 instance_queries.go:30:  Instance price:  0.0204
           2022/04/12 12:16:59 instance_queries.go:333: Comparing current type t3a.small with price 0.0204 with candidate t3a.small with price 0.0071

After change:

           2022/04/12 12:29:04 instance_queries.go:22: Comparing price spot/instance:
           2022/04/12 12:29:04 instance_queries.go:29:  Spot price:  0.0071
           2022/04/12 12:29:04 instance_queries.go:30:  Instance price:  0.026520000000000002
           2022/04/12 12:29:04 instance_queries.go:333: Comparing current type t3a.small with price 0.026520000000000002 with candidate 
t3a.small with price 0.0071

Copy link
Member

@cristim cristim Apr 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

right, but what if you also set the value as env var and have the tag as override?

I suspect it will be squared, because the value comes from both tag and env var and gets multiplied twice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, i suspect that too.
So we should add a condition to ignore i.region.conf.OnDemandPriceMultiplier if i.asg.config.OnDemandPriceMultiplier is != 1.
Only one multiplier should be taken in account.
What do you think ?

Copy link
Contributor Author

@mello7tre mello7tre Apr 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

i recovered my original comment to the commit, maybe can help to figure out why i have done it this way (sincerely i do not remember)

    * Specify/override multiplier for the on-demand price on a group
    level
    
    As i.price already have been multiplied by the global value, if
    specified, i need first to divide it by the same value and then multiply
    it by the multiplier specific to the ASG.
    
    We need to do this for both scheduled and event actions.
    For event we act in function belongsToEnabledASG.
    For schedule we act in launchSpotReplacement.
    
    Need deep testing...
    
    * Specify/override multiplier for the on-demand price on a group
        level - fix
    
    in loadConfOnDemandPriceMultiplier need to use
    a.config.OnDemandPriceMultiplier in place of
    a.region.conf.OnDemandPriceMultiplier
    
    this way a.region.conf.OnDemandPriceMultiplier will conserve the
    original global value

Think the key words are:

As i.price already have been multiplied by the global value, if
    specified, i need first to divide it by the same value and then multiply
    it by the multiplier specific to the ASG.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We calculate region instance ondemand price in method determineInstanceTypeInformation.
And we multiply it by the global OnDemandPriceMultiplier:
price.onDemand = it.Pricing[r.name].Linux.OnDemand * cfg.OnDemandPriceMultiplier

We call determineInstanceTypeInformation for both instance launch event and cron event.
So we need to divide by that global value before multiplying by the OnDemandPriceMultiplier ASG specific.

Copy link
Member

Choose a reason for hiding this comment

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

thanks, I'll look into this over the next few days, I want to address this issue somehow.

log.Printf("%s instace %s belongs to enabled ASG %s", i.region.name,
*i.InstanceId, i.asg.name)
return true
Expand Down
2 changes: 1 addition & 1 deletion go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ go 1.14
require (
github.com/aws/aws-lambda-go v1.26.0
github.com/aws/aws-sdk-go v1.40.39
github.com/cristim/ec2-instances-info v0.0.0-20210909050335-b239c40fcad0
github.com/cristim/ec2-instances-info v0.0.0-20220302182402-4b7a873a84cf
Copy link
Member

Choose a reason for hiding this comment

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

let's keep this dependency change out of this PR.

github.com/davecgh/go-spew v1.1.1
github.com/mattn/goveralls v0.0.9
github.com/namsral/flag v0.0.0-20170814194028-67f268f20922
Expand Down
2 changes: 2 additions & 0 deletions go.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ github.com/cpuguy83/go-md2man/v2 v2.0.0-20190314233015-f79a8a8ca69d/go.mod h1:ma
github.com/cpuguy83/go-md2man/v2 v2.0.0/go.mod h1:maD7wRr/U5Z6m/iR4s+kqSMx2CaBsrgA7czyZG/E6dU=
github.com/cristim/ec2-instances-info v0.0.0-20210909050335-b239c40fcad0 h1:X6u+vwxB2EQwA8+A7jhkYdNK0oE0p0LuuiPbAezZH+g=
github.com/cristim/ec2-instances-info v0.0.0-20210909050335-b239c40fcad0/go.mod h1:0yCjO4zBzlwWSGh/zGfW2Zq1NX605qCYVBHD1fPXKNs=
github.com/cristim/ec2-instances-info v0.0.0-20220302182402-4b7a873a84cf h1:/0fth76qMtSWpTbkOLRKHX/k6KgUAK6HMbXArUi7dTc=
github.com/cristim/ec2-instances-info v0.0.0-20220302182402-4b7a873a84cf/go.mod h1:0yCjO4zBzlwWSGh/zGfW2Zq1NX605qCYVBHD1fPXKNs=
Copy link
Member

Choose a reason for hiding this comment

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

also here, let's keep this change out of this PR.

github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c=
github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
Expand Down