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: resolve parseCDR error and add concatenated CDRs update #37

Merged
merged 2 commits into from
Dec 16, 2024

Conversation

TungChiaMing
Copy link
Contributor

Fix Two Issues in CHF When Processing CDR

  1. When the traffic exceeds a certain threshold (around 700KB), the Webconsole displays the error:
    [ERRO][WEBUI][BillingLog] parseCDR error when unmarshal with params: offset > next,
    and the usage data cannot be displayed on the Webconsole.

  2. According to TS 32.297 Section 5.2 "File Format Principles," a CDR file contains multiple CDRs. However, in CHF, charging data is updated to only one CDR. This causes the cdrBytes length in internal/sbi/processor's dumpCdrFile() to grow increasingly larger, eventually exceeding the uint16 range and resulting in an overflow in cdrHdr.CdrLength.

@andy89923 andy89923 self-requested a review November 13, 2024 04:52
@andy89923 andy89923 requested a review from Alonza0314 November 27, 2024 03:21
@andy89923
Copy link
Contributor

Hi @TungChiaMing ,

Thanks for the PR!
@Alonza0314 Please help test & review this PR! Thanks!

@Alonza0314
Copy link

Hi @TungChiaMing ,

I have tested this code via this senario: https://free5gc.org/guide/Charging/setting/#3-test-with-ueransim

I used ping -I ueransim0 1.1.1.1 ping -I ueransim0 8.8.8.8 to create traffic, but still got this error:
image

I'm sorry but can you provide your testing process?😣

@TungChiaMing
Copy link
Contributor Author

Hi @TungChiaMing ,

I have tested this code via this senario: https://free5gc.org/guide/Charging/setting/#3-test-with-ueransim

I used ping -I ueransim0 1.1.1.1 ping -I ueransim0 8.8.8.8 to create traffic, but still got this error: image

I'm sorry but can you provide your testing process?😣

Hi Alonza0314,

The decode function used by the Webconsole may not be this CHF with fix error in your environment.

Did you replace the go module of Webconsole by using 'replace'? the decode function of CHF will be used in the webconolse.

@Alonza0314
Copy link

Hi @TungChiaMing ,
I have tested this code via this senario: https://free5gc.org/guide/Charging/setting/#3-test-with-ueransim
I used ping -I ueransim0 1.1.1.1 ping -I ueransim0 8.8.8.8 to create traffic, but still got this error: image
I'm sorry but can you provide your testing process?😣

Hi Alonza0314,

The decode function used by the Webconsole may not be this CHF with fix error in your environment.

Did you replace the go module of Webconsole by using 'replace'? the decode function of CHF will be used in the webconolse.

Oh, I didn't notice it. Thanks a lot. I will replace and test it now.❤️

Copy link

@Alonza0314 Alonza0314 left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines -58 to -59
{"intTest4", -128, "020180", ""},
{"intTest5", -129, "0202ff7f", ""},
Copy link
Contributor

Choose a reason for hiding this comment

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

Why these two test cases be deleted?

Copy link
Contributor Author

@TungChiaMing TungChiaMing Nov 28, 2024

Choose a reason for hiding this comment

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

Since I remove 2’s complement conversion so these two test cases will not be passed.

The function parseInt64 in the file cdr/asn/ber_unmarshal.go originally handles the 2’s complement conversion to deal with the negative value.
If the first byte value greater than 127, the function will convert the input to a negative number.

This will eventually cause the parseCDR error because the next value will become negative.
The byte value here is the cdrBytes, which is from the asn.BerMarshalWithParams with input CHF ChargingRecord. (Actually, removing the 2's complement conversion may have issue in the online charging because the value may become negative but I didn't notice it.)

Copy link
Contributor

Choose a reason for hiding this comment

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

removing the 2's complement conversion may have issue in the online charging because the value may become negative but I didn't notice it.

This means that the online charging CDR file is not correct if we merge this PR?

Copy link
Contributor Author

@TungChiaMing TungChiaMing Nov 30, 2024

Choose a reason for hiding this comment

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

Not sure, but we’d better test the online charging first.

Copy link
Contributor Author

@TungChiaMing TungChiaMing Dec 10, 2024

Choose a reason for hiding this comment

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

Hello @andy89923,

I have tested the online billing functionality and found no issues. During the testing, I observed that online billing uses the Quota variable to manage the remaining traffic quota rather than directly deducting available traffic from the CDR. As a result, there are no concerns about negative cdrBytes values.

Based on this, we can proceed with merging this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @TungChiaMing ,

Thanks for your testing result!
@ianchen0119 FYI!

@ianchen0119 ianchen0119 merged commit a3ca118 into free5gc:main Dec 16, 2024
3 checks passed
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