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

Fix rounding USAGE.instance_per_server #246

Conversation

JacobValdemar
Copy link
Contributor

@JacobValdemar JacobValdemar commented Nov 21, 2023

These roundings doesn't really make sense AFAIK. It is unexpected that e.g. c6g.12xlarge and c6g.16xlarge have the same impact (which was the case before this PR).

@PierreRustOrange
Copy link

Hi, just to make sure I get things right , what make you think that the change is related to rounding (which has indeed changed in the last version).
Do you have an example ?

thanks

@da-ekchajzer
Copy link
Collaborator

@PierreRustOrange, the rounding is related to one data in the inventory. Nothing to do with how the API is handling rounding, no worries.

Sorry @JacobValdemar I just had the time to review the PR. I understand your point. It is a question of how we allocate the impacts of an all machine to one bare metal server. The main issue is that we consider that on machine host on type of EC2 instance, which is not the case (AWS must have an algorithm for optimizing VM distribution).

In the next version, I'd like the allocation to be done for each component and not at machine level, but the problem remains the same.

Pros :

  • Differentiate between nearby instances
  • Avoid double accounting

Cons :

  • Non-overcommit instances cannot have a portion of vCPU

@github-benjamin-davy @AirLoren @samuelrince any thought ?

@github-benjamin-davy
Copy link
Collaborator

I would need more explanation on the issue and suggested solution but my only comment at this stage would be that in the case of AWS, overcommit is marginal and limited to a few small instances.

@JacobValdemar
Copy link
Contributor Author

In conclusion, can this be merged?

@da-ekchajzer
Copy link
Collaborator

I think #252 will make instance_per_server not needed any more, as allocation will be computed at component level. The issue you highlighted will be fixed with this feature. If ok with you I'll close the PR.

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.

4 participants