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.

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

feat: add option for extra hourly cost per host #7609

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jasonwbarnett
Copy link

@jasonwbarnett jasonwbarnett commented Jan 19, 2025

Fixes #5163

Description

This feature allows specifying an extra hourly cost per host for thing like Datadog APM agent, etc.

How was this change tested?

  1. Create a nodepool with only xlarge and 2xlarge instance sizes.
  2. Created a test Deployment with 1 pod with 1 cpu/2GB memory requests with 100 replicas.
  3. Modify the nodepool and add all instance sizes.
  4. Modify the karpenter-aws-provider deployment and set EXTRA_HOURLY_COST_PER_HOST to "1.0"

Does this change impact docs?

  • Yes, PR includes docs updates
  • Yes, issue opened: #
  • No

Other Info

I considered adding the extra hourly cost per host to the static pricing for environments without access to the pricing API. However, this would require passing context into Reset() based on the current implementation, which introduces significant ripple effects across the entire test stack. I’m curious if anyone has alternative ideas or solutions to address this challenge.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@jasonwbarnett jasonwbarnett requested a review from a team as a code owner January 19, 2025 20:09
Copy link

netlify bot commented Jan 19, 2025

Deploy Preview for karpenter-docs-prod canceled.

Name Link
🔨 Latest commit ae81123
🔍 Latest deploy log https://app.netlify.com/sites/karpenter-docs-prod/deploys/678d5f1aa0ef360008bb7436

@jasonwbarnett jasonwbarnett force-pushed the feat/extra-hourly-cost-per-host branch 2 times, most recently from 2467bed to f53e234 Compare January 19, 2025 20:21
@jasonwbarnett jasonwbarnett force-pushed the feat/extra-hourly-cost-per-host branch from f53e234 to ae81123 Compare January 19, 2025 20:22
@jasonwbarnett
Copy link
Author

I just submitted this PR and wanted to reach out for help and guidance. If I missed anything or there are changes you’d like me to make, please let me know—I’m more than happy to address them promptly. Thanks for taking the time to review this!

@sidewinder12s
Copy link
Contributor

(I would want to point out that this doesn't quite solve the linked issue, though is a related cost modification that would be helpful for Karpenter)

@jmdeal jmdeal assigned jmdeal and engedaam and unassigned jmdeal Jan 27, 2025
@dlemfh
Copy link

dlemfh commented Jan 28, 2025

Hey @jasonwbarnett, thanks for taking the time to put up the PR!

I'm a karpenter user that is currently facing the same needs.
In fact, it seems like the implementation is similar to what I've suggested here: #5163 (comment)

I have just two question/suggestions regarding the code, if I may:

  1. I'm curious whether there's no need to also edit the UpdateSpotPricing function (in the same way as you did for UpdateOnDemandPricing)?

    To my knowledge, spot instances incur the same (e.g. Datadog) extra hourly costs as ondemand instances do, and my understanding of the pricing.go code was that the UpdateOnDemandPricing and UpdateSpotPricing functions act separately. So I wonder if there's no need to also edit the UpdateSpotPricing function (as I did here for example). If I've got something wrong and you could point it out for me I'd appreciate it!

  2. Also, with some time having passed since my initial take at it I've come to suspect that you might also need to apply the same edits to the ondemand/spot prices in the Reset function (in addition to the UpdateOnDemandPricing and UpdateSpotPricing functions) in order to make it a more complete and reliable solution. I'm curious what you think?

    My reasoning is as follows:

    • There is a write path and a read path for the ondemand/spot prices.
    • The write path uses the pricingProvider. Karpenter first initializes ondemand/spot prices with static defaults in populateInitialSpotPricing called by Reset (in case network requests fail), then attempts to update ondemand/spot prices in UpdateOnDemandPricing/UpdateSpotPricing (which usually succeed).
    • The read path has to do with the instanceTypesProvider? And karpenter makes provisioning and consolidation decisions based on the pricing values read.
    • The problem is there is no fixed order between the read and write paths? So there may be a race condition between the "read path" and the "update" action of the write path. That is, a provisioning/consolidation decision may happen before the first price update(= UpdateOnDemandPricing/UpdateSpotPricing). This will lead to a brief period of time where karpenter makes provisioning/consolidation decisions while disregarding the extra hourly cost info. Therefore the extra hourly cost info should also be reflected on the initial static default values as well.

    I'm hoping for a second opinion on this because I'm not entirely confident in my understanding the code.

Again, thanks to the team and community for getting the PR going. I'd appreciate any second opinions and/or corrections regarding my comment!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Custom Pricing Overrides for RIs, Savings Plans, and other strategies
5 participants