chore(pipelined): datapath refactor inout #12495

nstng · 2022-04-19T12:58:18Z

Summary

See #12072.

Done in pairing with @vktng

This PR is best reviewed commit by commit

Add ingress, middle and egress controller and super class mandatory_service. This is basically copying logic from inout.py to the three (+1 super class) new files. Note that for read- and reviewability code was just copied without any refactoring.
Switch sudo unit tests to using the new controllers
Make rpc servicer using new controllers.
Remove inout controller and use constants from new controllers.
Adapt documentation.
Reflect changes here in Bazel.

Open questions:

datapath: Refactor in-out controller into three separate controllers #12072 states that unit tests should be split up as well - is it better to test the three controllers in context?
- If this is still requested, then we suggest to do it in a follow up PR.

Test Plan

CI
integ tests on fork: https://github.com/nstng/magma/actions/runs/2203084202

Additional Information

This change is backwards-breaking

github-actions · 2022-04-19T12:58:37Z

Thanks for opening a PR! 💯

A couple initial guidelines

All commits must be signed off. This is enforced by DCO check.
All PR titles must follow the semantic commits format. This is enforced by Semantic PR.

Howto

Reviews. The "Reviewers" listed for this PR are the Magma maintainers who will shepherd it.
Checks. All required CI checks must pass before merge.
Merge. Once approved and passing CI checks, use the ready2merge label to indicate the maintainers can merge your PR.

More info

Please take a moment to read through the Magma project's

Contributing Conventions for norms around contributed code

If this is your first Magma PR, also consider reading

Developer Onboarding for onboarding as a new Magma developer
Development Workflow for guidance on your first pull request
CI Checks for points of contact for failing or flaky CI checks
GitHub-to-Slack mappings for Magma maintainers for guidance on how to contact maintainers on Slack

lte/gateway/python/magma/pipelined/app/middle.py

github-actions · 2022-04-19T13:00:37Z

Oops! Looks like you failed the Python Format Check.

Howto

Instructions on running the formatter and linter locally are provided in the format AGW doc
Guide to the different CI checks and resolution guidelines

♻️ Updated: ✅ The check is passing the Python Format Check after the last commit.

github-actions · 2022-04-19T13:00:59Z

Oops! Looks like you failed the Markdown lint check.

Howto

♻️ Updated: ✅ The check is passing the Markdown lint check after the last commit.

github-actions · 2022-04-19T13:09:29Z

feg-workflow

    2 files 202 suites 39s ⏱️
371 tests 371 ✔️ 0 💤 0 ❌
385 runs 385 ✔️ 0 💤 0 ❌

Results for commit 72107cb.

♻️ This comment has been updated with latest results.

github-actions · 2022-04-19T14:02:44Z

agw-workflow

    77 files   122 suites 7m 40s ⏱️
1 158 tests 1 149 ✔️ 9 💤 0 ❌
1 159 runs 1 150 ✔️ 9 💤 0 ❌

Results for commit 72107cb.

♻️ This comment has been updated with latest results.

koolzz · 2022-04-20T13:21:46Z

lte/gateway/python/magma/pipelined/app/mandatory_service.py

+from ryu.controller.handler import MAIN_DISPATCHER, set_ev_cls
+
+
+class MandatoryController(RestartMixin, MagmaController):


Hm I'm not sure why we're adding unnecessary inheritance here, whats the purpose of MandatoryController class?

I think we shouldn't introduce this

The reason is the common logic used in the three new controllers - would you prefer to handle this in a util class? (at least the ryu annotated functions would need to be duplicated in ingress, middle and egress apps).

Yeah we already have a MagmaController class, lets either put stuff there(only if its applicable to all controllers) or duplicate the code

Hi Nick, MagmaController is super class of a lot of apps so we should not move the common logic for ingress, middle and egress there. We will remove the new abstract controller and inline the code.

f.e this seems to be only used in middle

_get_vlan_egress_flow_msgs is used in middle and egress and please note that this does not belong to the class MandatoryController - this is the same patter as in inout.py before. We will move it to a util file/module.

pushed - running integ test on fork to be sure
https://github.com/nstng/magma/actions/runs/2203084202

themarwhal

doc changes seem reasonable, I'll let the pipelined codeowners validate the correctness :D Also, out of scope for this PR but is the diagram better handled with mermaid? (https://mermaid-js.github.io/mermaid/#/)

koolzz · 2022-04-20T14:01:49Z

lte/gateway/python/magma/pipelined/app/mandatory_service.py

+        flows.delete_all_flows_from_table(datapath, self.tbl_num)
+
+
+def _get_vlan_egress_flow_msgs(


f.e this seems to be only used in middle

koolzz · 2022-04-20T14:02:38Z

lte/gateway/python/magma/pipelined/app/mandatory_service.py

+from ryu.controller.handler import MAIN_DISPATCHER, set_ev_cls
+
+
+class MandatoryController(RestartMixin, MagmaController):


Yeah we already have a MagmaController class, lets either put stuff there(only if its applicable to all controllers) or duplicate the code

nstng · 2022-04-21T16:21:11Z

FYI for reviewers: added an additional commit for Bazel adaptions. Reason: the pipelined bazelification was merged after this PR was created and the changes here need to be reflected in Bazel.

sebathomas

Looking good, just a minor point.

lte/gateway/python/magma/pipelined/service_manager.py

lte/gateway/python/magma/pipelined/app/vlan_egress_flow_msgs.py

Neudrino

Only reviewed the "docs" change, which LGTM.

Neudrino · 2022-04-29T10:57:32Z

Afaiks there would be review needed again from @koolzz or @pshelar.
Could you review again, please, as this is blocking the next PR #12566 already.

koolzz · 2022-04-29T15:19:26Z

lte/gateway/python/magma/pipelined/app/vlan_egress_flow_msgs.py

@@ -0,0 +1,91 @@
+"""


Move this file up one directory and rename it to vlan_utils.py

👍 pushed

koolzz

I have one nit please fix before merging. Otherwise looks really clean, thanks for working on it!
@pshelar give this a quick glance through maybe

Signed-off-by: Nils Semmelrock <nils.semmelrock@tngtech.com>

…ess controller Signed-off-by: Nils Semmelrock <nils.semmelrock@tngtech.com>

…ller Signed-off-by: Nils Semmelrock <nils.semmelrock@tngtech.com>

…middle and egress controllers Signed-off-by: Nils Semmelrock <nils.semmelrock@tngtech.com>

…f date register table - reg10->reg12) Signed-off-by: Nils Semmelrock <nils.semmelrock@tngtech.com>

Signed-off-by: Nils Semmelrock <nils.semmelrock@tngtech.com>

pshelar · 2022-04-30T23:02:37Z

lte/gateway/python/magma/pipelined/vlan_utils.py

+            priority=priority, output_reg=output_reg, output_port=out_port,
+        ),
+    )
+    return msgs


did you ran non-nat datapath tests from S1AP integ tests with this PR?

@pshelar, we run all tests in the "lte integ tests" workflow. Last run on fork: https://github.com/nstng/magma/runs/6115971885?check_suite_focus=true
Edit latest run: https://github.com/nstng/magma/actions/runs/2252702388

I dont see test_attach_detach_non_nat_dp_ul_tcp.py in the test log, can you check it.

test_attach_detach_non_nat_dp_ul_tcp.py is in the NON_SANITY_TESTS list. It looks like the NON_SANITY_TESTS are not executed with the LTE integ-tests workflow. I do not see them executed anywhere automatically. Do you happen to know if this is intentionally?
I will execute them manually - but this will take some time.

Lets move this test to 'PRECOMMIT_TESTS' set.
cc: @ssanadhya

@pshelar executed the tests locally - test_attach_detach_non_nat_dp_ul_tcp.py was successful.

In general the tests seem to be very flaky (on a fresh vm setup). I still do not have all tests green. Do you have an opinion if one of the following tests might be relevant for the change?:

test_s1_handover_ping_pong.py test_activate_deactivate_multiple_dedicated.py test_sctp_shutdown_while_mme_is_stopped.py test_secondary_pdn_with_dedicated_bearer_multiple_services_restart.py test_3495_timer_for_dedicated_bearer_with_mme_restart.py test_no_attach_complete_with_mme_restart.py

these are not related datapath, so we can ignore it.

prabinakpattnaik · 2022-05-04T21:28:55Z

Looks good to me for these changes.

pshelar · 2022-05-04T23:17:57Z

lte/gateway/python/magma/pipelined/app/middle.py

+            )
+        return msgs
+
+    def _install_default_flows(self, datapath):


function, _install_default_flows() is duplicated in ingress, middle and egress, can we move it to base calss.

@pshelar: base.py is the super class of other controllers too. And those other controllers also have _install_default_flows() - but these are diverging implementations. Do we really want to implement _install_default_flows() of ingress, middle and egress in base.py and let all other controllers override this function?
Another approach would be to add an inheritance layer between base.py and ingress, middle and egress and put common logic there - this was already discussed with @koolzz above - this has the downside of adding inheritance complexity to the model.

ok, thats fine.

pshelar · 2022-05-04T23:18:35Z

lte/gateway/python/magma/pipelined/app/middle.py

+        chan = self._msg_hub.send(default_msgs, datapath)
+        self._wait_for_responses(chan, len(default_msgs))
+
+    def _wait_for_responses(self, chan, response_count):


same as above, this function can be moved to base class.

See answer above - additionally this function is not used by a lot of controllers that extend base.by.

pshelar · 2022-05-04T23:19:41Z

lte/gateway/python/magma/pipelined/app/middle.py

+            if not result.ok():
+                return fail(result.exception())
+
+    def cleanup_on_disconnect(self, datapath):


same for this function.

See answers above.

pshelar

Thanks for working on this.

* chore(pipelined): split up controllers from inout are added Signed-off-by: Nils Semmelrock <nils.semmelrock@tngtech.com> * chore(pipelined): inout (sudo unit) tests use ingress, middle and egress controller Signed-off-by: Nils Semmelrock <nils.semmelrock@tngtech.com> * chore(pipelined): rpc servicer uses ingress, middle and egress controller Signed-off-by: Nils Semmelrock <nils.semmelrock@tngtech.com> * chore(pipelined): inout is removed and logic is switched to ingress, middle and egress controllers Signed-off-by: Nils Semmelrock <nils.semmelrock@tngtech.com> * chore(pipelined): pipelined documentation is updated (including out of date register table - reg10->reg12) Signed-off-by: Nils Semmelrock <nils.semmelrock@tngtech.com> * chore(pipelined): bazel is adapted to use the new services Signed-off-by: Nils Semmelrock <nils.semmelrock@tngtech.com>

pull-request-size bot added the size/XXL Denotes a Pull Request that changes 1000+ lines. label Apr 19, 2022

github-actions bot added component: agw Access gateway-related issue component: docs Documentation-related issue labels Apr 19, 2022

github-actions bot reviewed Apr 19, 2022

View reviewed changes

lte/gateway/python/magma/pipelined/app/middle.py Outdated Show resolved Hide resolved

nstng force-pushed the datapath_refactor_inout branch 3 times, most recently from ad0537c to 928db64 Compare April 19, 2022 18:17

nstng assigned nstng and vktng Apr 20, 2022

nstng marked this pull request as ready for review April 20, 2022 12:56

nstng requested review from a team, hcgatewood, koolzz and pshelar April 20, 2022 12:56

koolzz reviewed Apr 20, 2022

View reviewed changes

themarwhal approved these changes Apr 20, 2022

View reviewed changes

koolzz reviewed Apr 20, 2022

View reviewed changes

pshelar requested review from vikramgreddy and prabinakpattnaik April 20, 2022 16:46

nstng linked an issue Apr 21, 2022 that may be closed by this pull request

datapath: Refactor in-out controller into three separate controllers #12072

Closed

nstng requested a review from sebathomas April 21, 2022 06:49

nstng force-pushed the datapath_refactor_inout branch 2 times, most recently from c17ab9a to c9adac6 Compare April 21, 2022 16:19

nstng requested a review from a team April 21, 2022 16:19

nstng requested a review from LKreutzer April 21, 2022 16:19

nstng force-pushed the datapath_refactor_inout branch from c9adac6 to 5952159 Compare April 21, 2022 17:01

LKreutzer approved these changes Apr 22, 2022

View reviewed changes

sebathomas mentioned this pull request Apr 25, 2022

datapath: Refactor ARP handling from inout app #12071

Closed

sebathomas approved these changes Apr 26, 2022

View reviewed changes

lte/gateway/python/magma/pipelined/service_manager.py Outdated Show resolved Hide resolved

sebathomas reviewed Apr 26, 2022

View reviewed changes

lte/gateway/python/magma/pipelined/app/vlan_egress_flow_msgs.py Outdated Show resolved Hide resolved

mpfirrmann mentioned this pull request Apr 27, 2022

refactor(pipelined): ARP handling is separated from the mandatory apps #12566

Merged

1 task

nstng force-pushed the datapath_refactor_inout branch 2 times, most recently from 2a73083 to 6f9e8ac Compare April 28, 2022 13:22

Neudrino requested a review from koolzz April 29, 2022 10:53

Neudrino approved these changes Apr 29, 2022

View reviewed changes

koolzz reviewed Apr 29, 2022

View reviewed changes

koolzz approved these changes Apr 29, 2022

View reviewed changes

nstng added 6 commits April 29, 2022 18:30

chore(pipelined): split up controllers from inout are added

a79b781

Signed-off-by: Nils Semmelrock <nils.semmelrock@tngtech.com>

chore(pipelined): inout (sudo unit) tests use ingress, middle and egr…

b6e6ef0

…ess controller Signed-off-by: Nils Semmelrock <nils.semmelrock@tngtech.com>

chore(pipelined): rpc servicer uses ingress, middle and egress contro…

3bff8c3

…ller Signed-off-by: Nils Semmelrock <nils.semmelrock@tngtech.com>

chore(pipelined): inout is removed and logic is switched to ingress, …

2242ed2

…middle and egress controllers Signed-off-by: Nils Semmelrock <nils.semmelrock@tngtech.com>

chore(pipelined): pipelined documentation is updated (including out o…

56f2bd9

…f date register table - reg10->reg12) Signed-off-by: Nils Semmelrock <nils.semmelrock@tngtech.com>

chore(pipelined): bazel is adapted to use the new services

72107cb

Signed-off-by: Nils Semmelrock <nils.semmelrock@tngtech.com>

nstng force-pushed the datapath_refactor_inout branch from 6f9e8ac to 72107cb Compare April 29, 2022 16:30

pshelar reviewed Apr 30, 2022

View reviewed changes

pshelar requested changes May 4, 2022

View reviewed changes

pshelar approved these changes May 5, 2022

View reviewed changes

pshelar merged commit d18ba2f into magma:master May 5, 2022

		from ryu.controller.handler import MAIN_DISPATCHER, set_ev_cls


		class MandatoryController(RestartMixin, MagmaController):

		flows.delete_all_flows_from_table(datapath, self.tbl_num)


		def _get_vlan_egress_flow_msgs(

chore(pipelined): datapath refactor inout #12495

chore(pipelined): datapath refactor inout #12495

Conversation

nstng commented Apr 19, 2022 • edited

Summary

Test Plan

Additional Information

github-actions bot commented Apr 19, 2022

Howto

More info

github-actions bot commented Apr 19, 2022 • edited

Howto

github-actions bot commented Apr 19, 2022 • edited

Howto

github-actions bot commented Apr 19, 2022 • edited

feg-workflow

github-actions bot commented Apr 19, 2022 • edited

agw-workflow

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

koolzz Apr 20, 2022 • edited

Choose a reason for hiding this comment

nstng Apr 21, 2022 • edited

Choose a reason for hiding this comment

nstng Apr 21, 2022 • edited

Choose a reason for hiding this comment

themarwhal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

koolzz Apr 20, 2022 • edited

Choose a reason for hiding this comment

nstng commented Apr 21, 2022

sebathomas left a comment

Choose a reason for hiding this comment

Neudrino left a comment

Choose a reason for hiding this comment

Neudrino commented Apr 29, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

koolzz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nstng May 1, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prabinakpattnaik commented May 4, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pshelar left a comment

Choose a reason for hiding this comment

nstng commented Apr 19, 2022 •

edited

github-actions bot commented Apr 19, 2022 •

edited

github-actions bot commented Apr 19, 2022 •

edited

github-actions bot commented Apr 19, 2022 •

edited

github-actions bot commented Apr 19, 2022 •

edited

koolzz Apr 20, 2022 •

edited

nstng Apr 21, 2022 •

edited

nstng Apr 21, 2022 •

edited

koolzz Apr 20, 2022 •

edited

Neudrino commented Apr 29, 2022 •

edited

nstng May 1, 2022 •

edited