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 CSV provider lookup precedence does not seem correct #2310

Open
wants to merge 21 commits into
base: develop
Choose a base branch
from

Conversation

aorjoa
Copy link

@aorjoa aorjoa commented Nov 16, 2023

What does this PR change?

  • This MR is to change some precedence of the CSV provider processing step then OpenCost can get more precisely pricing what it should be.

Does this PR relate to any other PRs?

  • N/A

How will this PR impact users?

  • Pricing that import from CSV file should get more precise pricing.

Does this PR address any GitHub or Zendesk issues?

  • N/A

How was this PR tested?

  1. Create CSV file for testing purpose fix-csv-file.csv and add this to the content.
EndTimestamp,InstanceID,Region,AssetClass,InstanceIDField,InstanceType,MarketPriceHourly,Version
2023-07-24 16:00:00 UTC,anuchito-1,us-east-1,node,metadata.labels.instance-group,c5.4xlarge,0.2591,
2023-07-24 16:00:00 UTC,aorjoa-1,us-east-1,node,metadata.labels.instance-group,c5.4xlarge,0.60000,
2023-07-24 16:00:00 UTC,aorjoa-1,ap-southeast-1,node,metadata.labels.instance-group,c5.4xlarge,0.90000,

and don't forget to config environment variable

USE_CSV_PROVIDER=true
CSV_PATH=<test-csv-file.csv>
  1. if you're on local machine then mock the AWS instance with this additional label
instance-group: aorjoa-1
node.kubernetes.io/instance-type: c5.4xlarge
topology.kubernetes.io/region: us-east-1
  1. the expected pricing should be 0.60000 due to it match with metadata.labels.instance-group=aorjoa-1. Unfortunately, the current situation is that we got 0.2591.
  2. You can try with other K8s node information if needed.

Does this PR require changes to documentation?

  • No document is required due to it is internal behavior change.

Have you labeled this PR and its corresponding Issue as "next release" if it should be part of the next OpenCost release? If not, why not?

  • Not yet, because I not quite sure about testing process of the OpenCost and how long it take.

Copy link

vercel bot commented Nov 16, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
opencost ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 6, 2024 6:22pm

@mattray mattray added opencost OpenCost issues vs. external/downstream P2 Estimated Priority (P0 is highest, P4 is lowest) kubecost Relevant to Kubecost's downstream project E2 Estimated level of Effort (1 is easiest, 4 is hardest) labels Nov 17, 2023
@mattray
Copy link
Collaborator

mattray commented Nov 17, 2023

@aorjoa thanks for the PR! We definitely appreciate the effort (and tests!) and I'll raise it with Kubecost Engineering. If anyone else sees this and wants to test and review it, that will help accelerate getting it merged.

@aorjoa
Copy link
Author

aorjoa commented Nov 23, 2023

Hi @AjayTripathy, IDK why the Quality Gate was failed but I just update the branch with develop.

@AjayTripathy
Copy link
Contributor

@aorjoa I have a question on one of your tests-- mind taking another look?

@aorjoa
Copy link
Author

aorjoa commented Nov 28, 2023

@aorjoa I have a question on one of your tests-- mind taking another look?

I happy to take a look 👀 please give me some more context @AjayTripathy.

@AjayTripathy
Copy link
Contributor

AjayTripathy commented Nov 28, 2023

@aorjoa I have a question on one of your tests-- mind taking another look?

I happy to take a look 👀 please give me some more context @AjayTripathy.

whoops forgot to hit submit on the review question. should be added as a comment now: #2310 (comment)

@aorjoa
Copy link
Author

aorjoa commented Dec 12, 2023

Hi @AjayTripathy. I just follow up this MR, if this need any further action please let me know. 😄

@AjayTripathy
Copy link
Contributor

@aorjoa sorry for the late response during holidays. @nik-kc looked at this as well and I believe had some thoughts. We'll get this in after the holidays though.

@aorjoa
Copy link
Author

aorjoa commented Dec 22, 2023

Thanks you in advance guys ⛳️, Happy holidays 🏖️

Copy link

sonarcloud bot commented Jan 5, 2024

Quality Gate Passed Quality Gate passed

The SonarCloud Quality Gate passed, but some issues were introduced.

2 New issues
0 Security Hotspots
25.7% Coverage on New Code
2.0% Duplication on New Code

See analysis details on SonarCloud

@aorjoa
Copy link
Author

aorjoa commented Jan 25, 2024

Hi @nik-kc, hope you are doing well. I just want to follow this up if anything I can help with it please let me know.

Add case insensitivity matching

Signed-off-by: Ajay Tripathy <[email protected]>
@cliffcolvin cliffcolvin changed the base branch from develop to v1.106 February 6, 2024 18:30
@cliffcolvin cliffcolvin changed the base branch from v1.106 to develop February 6, 2024 18:31
Copy link

github-actions bot commented May 7, 2024

This pull request has been marked as stale because it has been open for 90 days with no activity. Please remove the stale label or comment or this pull request will be closed in 5 days.

@github-actions github-actions bot added the Stale label May 7, 2024
@mattray
Copy link
Collaborator

mattray commented May 10, 2024

@nik-kc or @AjayTripathy do you want this kept open?

@mattray mattray removed the Stale label May 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
E2 Estimated level of Effort (1 is easiest, 4 is hardest) kubecost Relevant to Kubecost's downstream project needs-follow-up opencost OpenCost issues vs. external/downstream P2 Estimated Priority (P0 is highest, P4 is lowest)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants