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(pipelined): QFI support in Pipelined and Sessiond #12529
Conversation
Thanks for opening a PR! 💯
Howto
More infoPlease take a moment to read through the Magma project's
If this is your first Magma PR, also consider reading
|
Oops! Looks like you failed the Howto
♻️ Updated: ✅ The check is passing the Python Format Check after the last commit. |
@@ -866,6 +866,14 @@ bool SessionStateEnforcer::insert_pdr_from_core( | |||
rule.mutable_activate_flow_req()->set_downlink_tunnel(teid); | |||
rule.mutable_deactivate_flow_req()->set_downlink_tunnel(teid); | |||
|
|||
/*Set the AMBR QCI Value */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same set of lines repeated in another function. Can we move these to a new function and call that function from two places wherever it needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. Moved to common place function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pipelined changes look fine to me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sessiond Changes LGTM!!
This PR is dependency with RYU changes (#12514 ). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need unit test that matches and sets QFI field in OVS flows.
@pshelar , right now RYU and OVS changes are not present in master. So UT for QFI set value will not work and will get python traceback. My suggestion is that Will create new PR for UT once all three PRs (#10556 , #12514 and this PR) are getting merge into Master. Please provide any other solution or suggestion. |
can we wait for other PRs to merge ? |
Yes, We can wait for it and even we can not push this PR until unless RYU and OVS not merged |
@@ -346,6 +346,7 @@ message SetGroupPDR { | |||
SetGroupFAR set_gr_far = 8; | |||
DeactivateFlowsRequest deactivate_flow_req = 9; | |||
ActivateFlowsRequest activate_flow_req = 10; | |||
QCI session_qfi = 11; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is the QFI typed to QCI? My understanding is that the QFI is some dynamic identifier and the QCI classifies by a fixed value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The QFI may be dynamically assigned or may be equal to the 5QI. So here, QFI (5G) is used as QCI (4G).
731d347
to
f6c4eed
Compare
17d12ff
to
4f5291c
Compare
This PR has merge conflicts: |
Unit testing add. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
The CI flaggs the following:
|
7004dcc
to
f046195
Compare
@maxhbr , changes done. We have added the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, executing the bazelified test on the magma VM fails - ./bazel/scripts/run_sudo_tests.sh lte/gateway/python/magma/pipelined/tests:test_qfi_gtp
. I realize that the ryu patches from #12514 are missing for the bazel case. Do you want to add them here or should I raise a separate PR that needs to be merged first? Edit: I went ahead and created #13437 - this becomes hopefully merged fast.
After adding the patches, the test QfigtpTest::test_qfi_set_tunnel_flows
still fails for me with a flow diff [1]. I will check if I can reproduce this with the make build and come back to you.
Edit: I have the same result when executing the test on the make build. Please have a look.
[1]
E - cookie=0x0, table=classifier(main_table), n_packets=0, n_bytes=0, priority=10,tun_id=0x1,qfi=9,in_port=32768 actions=mod_dl_src:02:00:00:00:00:01,mod_dl_dst:ff:ff:ff:ff:ff:ff,set_field:0x186a0->reg9,load:0x1388->OXM_OF_METADATA[],resubmit(,ingress(main_table))
E - cookie=0x0, table=classifier(main_table), n_packets=0, n_bytes=0, priority=10,ip,in_port=LOCAL,nw_dst=192.168.128.30 actions=load:0x186a0->NXM_NX_TUN_ID[],load:0xc0a83cb2->NXM_NX_TUN_IPV4_DST[],set_field:0x8000->reg8,load:0x1->NXM_NX_TUN_FLAGS[],load:0x9->NXM_NX_QFI[],set_field:0x1->reg9,load:0x1388->OXM_OF_METADATA[],resubmit(,ingress(main_table))
E - cookie=0x0, table=classifier(main_table), n_packets=0, n_bytes=0, priority=10,ip,in_port=15577,nw_dst=192.168.128.30 actions=load:0x186a0->NXM_NX_TUN_ID[],load:0xc0a83cb2->NXM_NX_TUN_IPV4_DST[],set_field:0x8000->reg8,load:0x1->NXM_NX_TUN_FLAGS[],load:0x9->NXM_NX_QFI[],set_field:0x1->reg9,load:0x1388->OXM_OF_METADATA[],resubmit(,ingress(main_table))
E cookie=0x0, table=classifier(main_table), n_packets=0, n_bytes=0, priority=10,arp,in_port=LOCAL,arp_tpa=192.168.128.30 actions=load:0x1388->OXM_OF_METADATA[],resubmit(,ingress(main_table))
E cookie=0x0, table=classifier(main_table), n_packets=0, n_bytes=0, priority=10,arp,in_port=15577,arp_tpa=192.168.128.30 actions=load:0x1388->OXM_OF_METADATA[],resubmit(,ingress(main_table))
E To fix the error, update "/home/vagrant/magma/lte/gateway/python/magma/pipelined/tests/snapshots/test_qfi_gtp.QfigtpTest.test_qfi_set_tunnel_flows.snapshot" to the current snapshot:
E cookie=0x0, table=classifier(main_table), n_packets=0, n_bytes=0, priority=10,arp,in_port=LOCAL,arp_tpa=192.168.128.30 actions=load:0x1388->OXM_OF_METADATA[],resubmit(,ingress(main_table))
E cookie=0x0, table=classifier(main_table), n_packets=0, n_bytes=0, priority=10,arp,in_port=15577,arp_tpa=192.168.128.30 actions=load:0x1388->OXM_OF_METADATA[],resubmit(,ingress(main_table))
@nstng , after apply #13437 patch, it's working fine for Bazel. Please find the output:
Looks like you need to upgrade the VM and latest OVS patch is not applied in your vm. |
Hi @prabinakpattnaik thanks for checking. What VM upgrade/OVS patch do you mean? I built the VM from scratch based on this branch. |
@nstng , for this PR dependent on latest OVS packages like Below is the steps for upgrade the packages:
|
@prabinakpattnaik got it, I will set this up tomorrow and run the test again. But this means, that merging this PR is blocked until a new magma VM base image is pre-burned, right? Edit: tested with ovs 2.15.4-9 -> successful. I will leave my change request open so that this PR does not get accidentally merged before
|
@nstng , Yes, this PR is blocked until a new magma VM base image is pre-burned. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bazel changes lgtm
Signed-off-by: prabina pattnaik <prabinak@wavelabs.ai>
f046195
to
56da628
Compare
Rebased with master. Verified the s1ap test cases for traffic and 5G traffic testing. It's working for all cases. Please find the attached logs as reference. |
you run the following integtests:
is that a complete list? why don't you just run all of them? |
Changes related to traffic flows in OVS. |
@pshelar can you approve as its blocked on your request for change |
Signed-off-by: prabina pattnaik prabinak@wavelabs.ai
Summary
AMBR Based QFI support in pipelined and sessiond for 5G SA.
QFI value Set per PDU session.
The QFI that identifies the flow is carried in an extension header on N3 in the GTP-U protocol, using DL and UL PDU session information frames.
The DL and UL PDU session information frame includes a QoS Flow Identifier (QFI) field for each packet.
Test Plan
Please find UT logs:
Table 0 flow entries:
Additional Information