Arvados

19320-spot-pricing @ a98114557438e69fc7fc088d5d4c19ac2d4c5274 -- developer-run-tests: #3447

Example test run using the ec2 driver's live mode (see top of source:lib/cloud/ec2_test.go):

What next? test lib/cloud/ec2 -check.vv -live-ec2-cfg /tmp/ec2config.yml -check.f=PriceHistory
======= test lib/cloud/ec2
START: ec2_test.go:286: EC2InstanceSetSuite.TestInstancePriceHistory
instances are running, and identifiable as spot instances
i-0aaebc82dbb7994a2 price history: [{2023-01-14 17:40:14 +0000 UTC 0.0035}]
i-0423b0e8cbd6f68bd price history: [{2023-01-14 17:40:14 +0000 UTC 0.0035}]
i-027d69380093cf500 price history: [{2023-01-14 17:40:14 +0000 UTC 0.0035}]
i-0eb8ac5e664ead7f1 price history: [{2023-01-14 17:40:14 +0000 UTC 0.0035}]
destroy instance i-0423b0e8cbd6f68bd
destroy instance i-027d69380093cf500
destroy instance i-0eb8ac5e664ead7f1
PASS: ec2_test.go:286: EC2InstanceSetSuite.TestInstancePriceHistory     17.139s

OK: 1 passed
PASS
ok      git.arvados.org/arvados.git/lib/cloud/ec2       17.152s
======= test lib/cloud/ec2 -- 19s
Pass: lib/cloud/ec2 tests (19s)
All test suites passed.

• cloud.Instance interface has a new PriceHistory method, which returns the known price changes for the instance (or nil if not supported -- currently only supported by ec2 driver)
• dispatcher sends each instance's current PriceHistory to crunch-run during "run probe" (crunch-run --list)
• crunch-run --list writes the price history to a file in lockdir and sends SIGUSR2 to all crunch-run processes when checking aliveness (it used to send signal 0)
• while running a container, crunch-run catches SIGUSR2 and loads the latest price history
• when calculating cost to save in the container record, crunch-run uses the loaded price history if any, otherwise falls back to previous behavior (json in InstanceType env var)
• ec2 driver's "list instances" implementation notices when spot instances are in use, looks up their availability zones (this uses a separate AWS API call), and looks up recent spot price data for the relevant instance types in those zones
• ec2 driver parameter SpotPriceUpdateInterval (default 24h) is implemented as "if we already looked up pricing for all of the instance types/zones currently running, don't refresh; otherwise, refresh pricing for all running types/zones"
• there is no attempt to calculate cost of AddedScratch (where is the best place to document this?)

• Log entries in crunch-run.txt whenever the instance price changes, to provide feedback that the price checker is doing something, and make it possible to see how the total cost was calculated.

19320-spot-pricing @ c138f58b21edb574b101588f6fc61dce8a98ed3e -- developer-run-tests: #3453

• Line 248:
  • Would it make sense to get the default interval value from the config? I'm thinking that it could get out of sync if we ever change it and forget to update this.
  • I think it could be useful to log a warning message when correcting this interval setting.
  • Also, would it be healthy to prevent too many API calls in a short period of time because of a misconfiguration? Does it make sense to allow this interval to be smaller than say, 5 minutes?
• Line 322: The priceKey struct includes a 'spot' boolean member, that seems to always be true. Is this for future proofing?
• Line 355:
  • Should that append() call be inside the previous conditional? We are only interested on instance type prices with stale information, right?
  • How about duplicated instance types? If we have many hundreds of spot instances of the same type, the filter might get a bit too large for the API call?

Just in case you missed it: There are additional features listed on this ticket like regular instance prices and extra costs related to attached storages that weren't addressed on this branch.

Lucas Di Pentima wrote in #note-16:

• Would it make sense to get the default interval value from the config? I'm thinking that it could get out of sync if we ever change it and forget to update this.

The hard-coded default made tests work without specifying anything, but IRL config.default.yml already makes the default 24h, so it's only 0 if someone wrote that in the config. I've updated this so 0 now means "don't look up spot prices at all", which seems more useful than being able to write "0" as an alternate spelling of "24h".

• I think it could be useful to log a warning message when correcting this interval setting.

On that note, I'm inclined to not log anything when it's been explicitly disabled by writing 0 -- thoughts?

• Also, would it be healthy to prevent too many API calls in a short period of time because of a misconfiguration? Does it make sense to allow this interval to be smaller than say, 5 minutes?

I'm not sure about this. If someone wants to check every 30 seconds, who am I to say they shouldn't?

• Line 322: The priceKey struct includes a 'spot' boolean member, that seems to always be true. Is this for future proofing?

Yes. I've added a comment at the lookup part to highlight that (currently) looking up a non-spot instance will never return anything.

• Line 355:
  • Should that append() call be inside the previous conditional? We are only interested on instance type prices with stale information, right?

I could be persuaded otherwise, but my thinking here was that if we're going to do an API call to get prices, and that API lets us specify many instance types, we might as well get the latest prices for all of the types we're running. This way we do ~1 request when we start using a new instance type, plus 1 per specified interval. The other obvious way (update only the stale data) would have us doing multiple API calls in each interval even when there are no new instance types, because the cached data points' expiry times would start out staggered and never sync up.

If we're limiting the total number of API calls to less than { 1 per instance creation + 1 per interval }, and the cost of the price-list API call (even with lots of instance types) is much less than the cost of creating an instance, I'm thinking this is optimized enough. Does that sound right?

• How about duplicated instance types? If we have many hundreds of spot instances of the same type, the filter might get a bit too large for the API call?

Oops, good catch, I forgot to deduplicate these. Fixed.

Just in case you missed it: There are additional features listed on this ticket like regular instance prices and extra costs related to attached storages that weren't addressed on this branch.

I've added costs for AddedScratch EBS volumes. I'm inclined to get what we've got so far finished/merged before wading into the non-spot prices, though -- does that sound OK?

(will update branch when I find out why live tests are failing.)

19320-spot-pricing @ de36befd0c2d9d92a41e1f90f6353229d13b725a -- developer-run-tests: #3459

Tom Clegg wrote in #note-17:

On that note, I'm inclined to not log anything when it's been explicitly disabled by writing 0 -- thoughts?

Yes, if the software doesn't make automatic corrections, no logging needed.

I'm not sure about this. If someone wants to check every 30 seconds, who am I to say they shouldn't?

Fair enough. I just thought that a potential typo of using for example a value of 6000 (as times are sometimes expressed in seconds) would produce API throttling situation on the dispatcher because it's making requests every 6 millisecs.

If we're limiting the total number of API calls to less than { 1 per instance creation + 1 per interval }, and the cost of the price-list API call (even with lots of instance types) is much less than the cost of creating an instance, I'm thinking this is optimized enough. Does that sound right?

Yeah, that's a nice optimization I didn't get when reading the code. Thanks for clarifying it.

I've added costs for AddedScratch EBS volumes. I'm inclined to get what we've got so far finished/merged before wading into the non-spot prices, though -- does that sound OK?

Sounds great! This LGTM, please merge.

I just thought that a potential typo of using for example a value of 6000 (as times are sometimes expressed in seconds) would produce API throttling situation on the dispatcher because it's making requests every 6 millisecs.

Ah, yes. Specifying 6000 is already an error -- we don't accept unitless non-zero numbers as arvados.Duration values for exactly that reason.