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

Log and continue on a single record processing failures. #2385

Merged
merged 2 commits into from
Dec 12, 2023

Conversation

r2k1
Copy link
Contributor

@r2k1 r2k1 commented Dec 11, 2023

What does this PR change?

"I'm experiencing an issue with multiple AKS clusters where opencost is returning a 500 error. The error message is Error: unable to locate allocation totals for allocation,. This message might be truncated and could contain more details, but I don't have access to that information (or any logs)

The current processing logic seems to halt opencost entirely when a single record fails. The proposed change is to ignore such failing records and allow processing to continue.

Signed-off-by: r2k1 <yokree@gmail.com>
Copy link

vercel bot commented Dec 11, 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 Dec 12, 2023 8:22pm

@r2k1
Copy link
Contributor Author

r2k1 commented Dec 11, 2023

@ameijer how do you think, is it something that is going to work?

I can't imagine how it can make things worse. But is it something that may help or just raises an error somewhere further in the processing?

Unfortunately, I can't replicate the issue. But I see no harm in this attempt to fix it.

@@ -2555,8 +2555,7 @@ func (cm *CostModel) QueryAllocation(window kubecost.Window, resolution, step ti

if totals == nil {
log.Errorf("unable to locate asset totals for allocation %s", key)
return nil, fmt.Errorf("unable to locate allocation totals for allocation")

continue
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is good @r2k1 . For others: this changes the behavior of a PARC for which we cannot locate totals for to simply not be included, rather than bubbling up an error.

This doesn't fix whatever the underlying issue is with no totals in the store, but this at least lets us degrade gracefully

pkg/costmodel/costmodel.go Outdated Show resolved Hide resolved
@ameijer
Copy link
Contributor

ameijer commented Dec 12, 2023

happy to approve @r2k1 if you agree with my idea about making the error message a little clearer that we are going to skip the PARC return

Co-authored-by: Alex Meijer <ameijer@users.noreply.github.com>
Signed-off-by: Artur Khantimirov <yokree@gmail.com>
Copy link
Contributor

@ameijer ameijer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM @r2k1 , I will work on getting this merged for you

Copy link
Member

@cliffcolvin cliffcolvin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@cliffcolvin cliffcolvin merged commit 40ab230 into opencost:develop Dec 12, 2023
6 checks passed
@r2k1 r2k1 deleted the fix-unable-to-locate branch May 6, 2024 22:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants