[AGW][pipelined] Add support for APN AMBR #2304

karthiksubraveti · 2020-08-12T16:42:45Z

Signed-off-by: Karthik Subraveti karthikshyam@gmail.com

Summary

Changed enforcement code and pipelined to pass in the apn_ambr all the way to Qos enforcement
Added parent qid as an optional arg for adding Qos. We can use this for supporting APN AMBR. APN AMBR will be created as a parent class and all the subscriber qos profiles will be created as leaf classes under it. The parent class is created with a ceil based on APN AMBR and child classes are created with ceil based on MBR. The rate params are defaulted to 12Kbits. When the child class exceeds the rate, it can borrow tokens from parent up-to its ceil value.
http://luxik.cdi.cz/~devik/qos/htb/manual/userg.htm
Also cleaned up existing code to ensure that we create fq_codel and filters only for leaf classes.
Added test case for verifying sanity by creating a simple parent/child class.
Added test case to verify the APN AMBR sanity from QosManager code
Added logic to handle clean_restart=false case, needs more verification here

Test Plan

Added testcase(testSanityTrafficClass) to qos test and it passes.
Added testcase(testApnAmbrSanity) for verifying APN Ambr sanity
Made associated changes in other qos tests to make this change.

testBrokenMeter (test_qos.TestMeters) ... ok
testIdManager (test_qos.TestQosCommon) ... ok
testQosKeyUtils (test_qos.TestQosCommon) ... ok
This test verifies that qos configuration gets programmed correctly ... ok
testMultipleSubscribers (test_qos.TestQosManager) ... ok
testSanity (test_qos.TestQosManager) ... ok
testUncleanRestart (test_qos.TestQosManager) ... ok
testError (test_qos.TestTrafficClass) ... ok
testSanityTrafficClass (test_qos.TestTrafficClass) ... RTNETLINK answers: No such file or directory
RTNETLINK answers: No such file or directory
We have an error talking to the kernel
RTNETLINK answers: No such file or directory
ok
testSudoUser (test_qos.TestTrafficClass) ... ok

Name                                     Stmts   Miss Branch BrPart  Cover
--------------------------------------------------------------------------
magma/pipelined/__init__.py                  0      0      0      0   100%
magma/pipelined/metrics.py                   8      0      0      0   100%
magma/pipelined/openflow/__init__.py         0      0      0      0   100%
magma/pipelined/openflow/exceptions.py       2      0      0      0   100%
magma/pipelined/openflow/messages.py       127     32     36      7    69%
magma/pipelined/openflow/meters.py          37     23      0      0    38%
magma/pipelined/qos/__init__.py              0      0      0      0   100%
magma/pipelined/qos/common.py              230      8     88      9    94%
magma/pipelined/qos/qos_meter_impl.py       76     14     14      1    79%
magma/pipelined/qos/qos_tc_impl.py         153      8     38      3    94%
magma/pipelined/qos/types.py                11      0      0      0   100%
magma/pipelined/qos/utils.py                42      0     14      0   100%
magma/common/__init__.py                     0      0      0      0   100%
magma/common/redis/__init__.py               0      0      0      0   100%
magma/common/redis/client.py                 6      6      0      0     0%
magma/common/redis/containers.py           168    168     46      0     0%
magma/common/redis/serializers.py           42     42      0      0     0%
magma/configuration/__init__.py             12     12      2      0     0%
magma/configuration/exceptions.py            2      2      0      0     0%
magma/configuration/service_configs.py      50     50      8      0     0%
--------------------------------------------------------------------------
TOTAL                                      966    365    246     20    62%
----------------------------------------------------------------------
Ran 10 tests in 0.112s

OK```

ulaskozat · 2020-08-13T22:40:06Z

lte/gateway/python/magma/pipelined/qos/qos_tc_impl.py

-                handle {qid} fw flowid 1:{qid}".format(intf, qid=qid_hex)
+        parent_qid_hex = hex(parent_qid) if parent_qid else TrafficClass.ROOT_QID
+        tc_cmd = "tc class add dev {intf} parent 1:{parent_qid} classid 1:{qid} htb \
+            rate 12Kbit ceil {maxbw}".format(intf=intf, parent_qid=parent_qid_hex, 


let's have the rate parameter also as a function argument. We can use it for GBR where gbr rates can be used for the rate parameter.

I added these as default args but it failed due to invalid inputs.. I will add cleaner check at the top of the method instead of shorthand if else. It might make it easier to read

lte/gateway/python/magma/pipelined/qos/common.py

pshelar · 2020-08-19T07:37:57Z

lte/gateway/python/magma/pipelined/qos/common.py

-                self._redis_conn_retry_secs,
-            )
-            self._loop.call_later(self._redis_conn_retry_secs, self.setup)
+        self._setupInternal()


even if it is qos is disabled pipelined need to call _setupInternal() to cleanup state from previous run.

lte/gateway/python/magma/pipelined/qos/qos_tc_impl.py

pshelar · 2020-08-19T07:46:26Z

lte/gateway/python/magma/pipelined/qos/qos_tc_impl.py

+            for qid_tuple in qid_list:
+                qid, pqid = qid_tuple
                if qid < self._start_idx or qid > (self._max_idx - 1):
                    continue


we need to relax checks around idx since this could be valid id from previous run.

Are you saying a scenario where we change the min and max idx on a restart ? I would think that would change more on an upgrade.

lets not assume it, user can expand or shrink the range.

We have some inherent assumption of ROOT_QID == max_idx, if user is going to make change here, we can no longer recover properly. I think it is reasonable to say we won't support unclean clean_restart then and ask user to set clean_restart==true.

we should add comment in config file about significance of these values.

pshelar · 2020-08-19T07:51:39Z

lte/gateway/python/magma/pipelined/qos/common.py

-
 def normalizeIMSI(imsi: str) -> str:
    imsi = imsi.lower()
    if imsi.startswith("imsi"):


I know it is not part of this patch, but can we use encode_imsi() here.

@pshelar I will change this in a future PR. Currently lot of tests(test_qos.py) use a random IMSI and it is breaking when i changed to use encode_imsi. I will avoid making this change for now.

lte/gateway/python/magma/pipelined/qos/common.py

karthiksubraveti

Thanks Pravin for the review. I will cleanup the code based on the feedback and submit.

lte/gateway/python/magma/pipelined/qos/common.py

lte/gateway/python/magma/pipelined/qos/qos_tc_impl.py

karthiksubraveti · 2020-08-19T19:14:56Z

lte/gateway/python/magma/pipelined/qos/qos_tc_impl.py

+            for qid_tuple in qid_list:
+                qid, pqid = qid_tuple
                if qid < self._start_idx or qid > (self._max_idx - 1):
                    continue


Are you saying a scenario where we change the min and max idx on a restart ? I would think that would change more on an upgrade.

Signed-off-by: Karthik Subraveti <karthikshyam@gmail.com>

karthiksubraveti requested review from koolzz and pshelar as code owners August 12, 2020 16:42

karthiksubraveti added the component: agw Access gateway-related issue label Aug 12, 2020

ulaskozat reviewed Aug 13, 2020

View reviewed changes

ulaskozat linked an issue Aug 13, 2020 that may be closed by this pull request

[pipelined] Enforce AMBR across bearer level and APN level (subscriber level is enforced by RAN) #2276

Closed

karthiksubraveti force-pushed the qos.enforcement.0 branch 2 times, most recently from 274f507 to 3d66724 Compare August 18, 2020 18:54

karthiksubraveti changed the title ~~[AGW][pipelined] Enable addition of parent class(for supporting APN AMBR) to linux tc qos implementation~~ [AGW][pipelined] Add support for APN AMBR Aug 18, 2020

karthiksubraveti force-pushed the qos.enforcement.0 branch 3 times, most recently from f1f379b to 67f3c3c Compare August 19, 2020 06:09

pshelar requested changes Aug 19, 2020

View reviewed changes

karthiksubraveti commented Aug 19, 2020

View reviewed changes

karthiksubraveti force-pushed the qos.enforcement.0 branch 3 times, most recently from f5ea484 to aedfd34 Compare August 20, 2020 02:00

Add APN AMBR support to pipelined

110191a

Signed-off-by: Karthik Subraveti <karthikshyam@gmail.com>

karthiksubraveti force-pushed the qos.enforcement.0 branch from aedfd34 to 110191a Compare August 20, 2020 05:47

ulaskozat approved these changes Aug 21, 2020

View reviewed changes

pshelar approved these changes Aug 24, 2020

View reviewed changes

karthiksubraveti merged commit bb99449 into magma:master Aug 24, 2020

karthiksubraveti deleted the qos.enforcement.0 branch August 25, 2020 04:02

[AGW][pipelined] Add support for APN AMBR #2304

[AGW][pipelined] Add support for APN AMBR #2304

Uh oh!

Conversation

karthiksubraveti commented Aug 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test Plan

Uh oh!

Choose a reason for hiding this comment

Uh oh!

karthiksubraveti Aug 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

karthiksubraveti left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

karthiksubraveti commented Aug 12, 2020 •

edited

Loading

karthiksubraveti Aug 20, 2020 •

edited

Loading