Fix/price multiplier

[mello7tre]

Issue Type

• Bugfix Pull Request

Summary

Since 6938027, OnDemandPriceMultiplier were no more taken in account.
The fix is easy, just restore the multiplier when setting instance price (i.price) inside belongsToEnabledASG.

(include a commit that change go.mod to use and updated ec2-instances-info)

Code contribution checklist

1. I hereby allow the Copyright holder the rights to distribute this piece of
   code under any software license.
2. The contribution fixes a single existing github issue, and it is linked
   to it.
3. The code is as simple as possible, readable and follows the idiomatic Go
   guidelines.
4. All new functionality is covered by automated test cases so the overall
   test coverage doesn't decrease.
5. No issues are reported when running make full-test.
6. Functionality not applicable to all users should be configurable.
7. Configurations should be exposed through Lambda function environment
   variables which are also passed as parameters to the
   CloudFormation
   and
   Terraform
   stacks defined as infrastructure code.
8. Global configurations set from the infrastructure stack level should also
   support per-group overrides using tags.
9. Tags names and expected values should be similar to the other existing
   configurations.
10. Both global and tag-based configuration mechanisms should be tested and
    proven to work using log output from various test runs.
11. The logs should be kept as clean as possible (use log levels as
    appropriate) and formatted consistently to the existing log output.
12. The documentation is updated to cover the new behavior, as well as the
    new configuration options for both stack parameters and tag overrides.
13. A code reviewer reproduced the problem and can confirm the code
    contribution actually resolves it.

[cristim]

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.

[mello7tre]

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

[cristim]

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.

[mello7tre]

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 ?

[mello7tre]

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.

[mello7tre]

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.

[cristim]

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

[cristim]

Taking another look into this, I'll try to sort it out over the next few days.

[cristim]

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

[cristim]

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

[cristim]

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