fix(agw): Modified code to port teid has list to protobuf in sgw_s8 task #14006

rsarwad · 2022-09-23T13:55:26Z

Summary

fix(agw): Modified code to port teid has list to protobuf in sgw_s8 task

Test Plan

Executed only unit test suite. For in-bound roaming, End2End scenarios used to be tested in Tera VM setup. Since we are not using the Tera VM setup. We couldn't validate End2End scenario.

Additional Information

Once federated integ_test supports mocked external PGW, we can validate the End2End scenarios for inbound roaming UE

…edure_id to protobuf map Signed-off-by: Rashmi <rashmi.sarwad@radisys.com>

Signed-off-by: Rashmi <rashmi.sarwad@radisys.com>

github-actions · 2022-09-23T13:55:42Z

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

github-actions · 2022-09-23T13:57:45Z

lte/gateway/c/core/oai/tasks/sgw_s8/sgw_s8_state.cpp

  }
-  free_wrapper((void**)sgw_eps_context);
+  free_wrapper((void**)ptr);


[cpplint] _{reported by reviewdog 🐶}
Using C-style cast. Use reinterpret_cast<void**>(...) instead [readability/casting] [4]

github-actions · 2022-09-23T13:57:45Z

lte/gateway/c/core/oai/tasks/sgw_s8/sgw_s8_state_manager.cpp

+    }
+  }
+  state_cache_p->temporary_create_session_procedure_id_map.map = nullptr;
+  free_cpp_wrapper((void**)&state_cache_p);


[cpplint] _{reported by reviewdog 🐶}
Using C-style cast. Use reinterpret_cast<void**>(...) instead [readability/casting] [4]

Fix the warning

github-actions · 2022-09-23T14:06:02Z

feg-workflow

    2 files 203 suites 39s ⏱️
374 tests 374 ✔️ 0 💤 0 ❌
388 runs 388 ✔️ 0 💤 0 ❌

Results for commit 5215f3e.

♻️ This comment has been updated with latest results.

github-actions · 2022-09-23T14:27:33Z

dp-workflow

14 tests 14 ✔️ 2m 12s ⏱️
  1 suites   0 💤
  1 files   0 ❌

Results for commit 5215f3e.

♻️ This comment has been updated with latest results.

github-actions · 2022-09-23T14:44:29Z

agw-workflow

615 tests 611 ✔️ 4m 31s ⏱️
    2 suites     4 💤
    2 files     0 ❌

Results for commit 5215f3e.

♻️ This comment has been updated with latest results.

ajahl · 2022-09-26T12:36:08Z

@rsarwad Can you fix the linter warnings and rebase your PR?

pruthvihebbani · 2022-10-12T12:37:06Z

lte/gateway/c/core/oai/include/proto_map.hpp

@@ -170,15 +170,15 @@ struct proto_map_s {
  **              the map. If key does not exists returns error             **
  **                                                                        **
  ***************************************************************************/
-  proto_map_rc_t remove(const keyT key) {
+  proto_map_rc_t remove(const keyT key, bool free_an_entry = true) {


What is the need of this flag? Shouldn`t the entry be freed always?

If flag is set, an entry for corresponding key would be removed and also contents are freed.
But in this PR, sgw_context would be initially in map, temporary_create_session_procedure_id_map. On reception of create session response, sgw_context would be removed from temporary_create_session_procedure_id_map and inserted into state_tied_map

pruthvihebbani · 2022-10-12T12:37:55Z

lte/gateway/c/core/oai/tasks/sgw_s8/sgw_s8_handlers.cpp

-  sgw_eps_bearer_context_information_t* sgw_bearer_context_info = NULL;
-  hash_table_ts_t* state_imsi_ht = get_sgw_ue_state();
+  sgw_eps_bearer_context_information_t* sgw_bearer_context_info = nullptr;
+  map_uint32_sgw_eps_bearer_context_t* state_teid_ht = get_s8_state_teid_map();


NULL check for state_teid_ht

pruthvihebbani · 2022-10-12T12:40:04Z

lte/gateway/c/core/oai/tasks/sgw_s8/sgw_s8_state_manager.cpp

+    }
+  }
+  state_cache_p->temporary_create_session_procedure_id_map.map = nullptr;
+  free_cpp_wrapper((void**)&state_cache_p);


Fix the warning

pruthvihebbani · 2022-10-12T12:40:52Z

lte/gateway/c/core/oai/tasks/sgw_s8/sgw_s8_state_manager.cpp

@@ -130,5 +148,14 @@ status_code_e SgwStateManager::read_ue_state_from_db() {
 sgw_state_t* SgwStateManager::get_state(bool read_from_db) {
  return state_cache_p;
 }
+
+map_uint32_sgw_eps_bearer_context_t* SgwStateManager::get_s8_state_teid_map() {
+  AssertFatal(


Use Error log instead of assertFatal

It's mandatory to have this flag, is_initialized to be initialized. So I think assertFatal should be fine

Yeah, if the state is not cleanly initialized from Redis, the service should fail to start up.

Signed-off-by: Rashmi <rashmi.sarwad@radisys.com>

…d_hl Signed-off-by: Rashmi <rashmi.sarwad@radisys.com>

ssanadhya · 2022-10-13T16:07:55Z

lte/gateway/c/core/oai/tasks/sgw_s8/sgw_s8_state_manager.hpp

    "s11_bearer_context_information_htbl";
 constexpr char SGW_STATE_TABLE_NAME[] = "sgw_state";
 constexpr char SGW_TASK_NAME[] = "SGW";
+constexpr char SGW_S8_CSR_PROC_ID_MAP[] = "sgw_s8_csr_proc_id_map";


Suggested change

constexpr char SGW_S8_CSR_PROC_ID_MAP[] = "sgw_s8_csr_proc_id_map";

constexpr char SGW_S8_CSR_PROC_ID_MAP_NAME[] = "sgw_s8_csr_proc_id_map";

ssanadhya · 2022-10-13T16:08:51Z

lte/gateway/c/core/oai/tasks/sgw_s8/sgw_s8_state_manager.cpp

@@ -130,5 +148,14 @@ status_code_e SgwStateManager::read_ue_state_from_db() {
 sgw_state_t* SgwStateManager::get_state(bool read_from_db) {
  return state_cache_p;
 }
+
+map_uint32_sgw_eps_bearer_context_t* SgwStateManager::get_s8_state_teid_map() {
+  AssertFatal(


Yeah, if the state is not cleanly initialized from Redis, the service should fail to start up.

ssanadhya · 2022-10-13T16:39:37Z

lte/gateway/c/core/oai/tasks/sgw_s8/sgw_s8_handlers.cpp

-      (const hash_key_t)session_rsp_p->temporary_create_session_procedure_id,
-      (void**)&sgw_context_p);
+  sgw_eps_bearer_context_information_t* sgw_context_p = nullptr;
+  sgw_state->temporary_create_session_procedure_id_map.get(


What's the purpose of getting this entry before removing it?

While handling create session request, sgw_context is created and inserted into map, temporary_create_session_procedure_id_map. Because for roaming users, sgw_s11_teid would be allocated by external PGW, shall be received in create session response message. So on reception of create session response, sgw_context is removed from map, but not freed.
So first we get the sgw_context, remove from map, temporary_create_session_procedure_id_map and insert into state_teid_map

ssanadhya · 2022-10-13T16:45:40Z

lte/gateway/c/core/oai/tasks/sgw_s8/sgw_s8_handlers.cpp

@@ -1846,17 +1850,30 @@ sgw_eps_bearer_context_information_t* update_sgw_context_to_s11_teid_map(
    OAILOG_FUNC_RETURN(LOG_SGW_S8, sgw_context_p);
  }

+  if (sgw_state->temporary_create_session_procedure_id_map.remove(
+          session_rsp_p->temporary_create_session_procedure_id, false) !=


Why do we need the remove with false flag here? In the original logic the entry is being removed and freed from the hash table.

In the original code, only entry is removed but not freed from hash table. This is taken care by API, hashtable_ts_remove() which only removes an entry from hash table but doesn't free. For API, hashtable_ts_free() free_wrapper is called as here https://github.com/magma/magma/blob/master/lte/gateway/c/core/oai/lib/hashtable/hashtable.c#L664
But same is not true for hashtable_ts_remove(), https://github.com/magma/magma/blob/master/lte/gateway/c/core/oai/lib/hashtable/hashtable.c#L691
The original code is here, https://github.com/magma/magma/pull/14006/files#diff-89dd268674e287dc1dc45f2515f69e41f864be6074f3e03529324cb1eeecb374L1837

To implement same behavior, I added new flag.

ssanadhya · 2022-10-13T16:46:55Z

lte/gateway/c/core/oai/tasks/sgw_s8/sgw_s8_state.cpp

  }
-  free_wrapper((void**)sgw_eps_context);
+  free_wrapper(reinterpret_cast<void**>(ptr));


Suggested change

free_wrapper(reinterpret_cast<void**>(ptr));

free_cpp_wrapper(reinterpret_cast<void**>(ptr));

In this PR, memory for sgw_eps_bearer_context_information_t is allocated with calloc. But in other PR, memory is allocated with new(). It's here, https://github.com/magma/magma/pull/13895/files#diff-89dd268674e287dc1dc45f2515f69e41f864be6074f3e03529324cb1eeecb374R189
It's shall be taken care while rebasing.

ajahl · 2022-10-28T09:17:39Z

lte/gateway/c/core/oai/tasks/sgw_s8/sgw_s8_state_manager.cpp

  }
+  s8_state_teid_map.map = nullptr;


Is there a reason that you can not set map to nullptr in function destroy_map()?

I had missed it. Thanks for bringing into notice

ajahl · 2022-10-28T09:19:21Z

lte/gateway/c/core/oai/tasks/sgw_s8/sgw_s8_state_manager.cpp

+                   "temporary_create_session_procedure_id_map ");
+    }
+  }
+  state_cache_p->temporary_create_session_procedure_id_map.map = nullptr;


Signed-off-by: Rashmi <rashmi.sarwad@radisys.com>

…d_hl Signed-off-by: Rashmi <rashmi.sarwad@radisys.com>

pruthvihebbani

LGTM

pruthvihebbani · 2022-11-04T13:30:50Z

@rsarwad Please rebase the PR

…d_hl Signed-off-by: Rashmi <rashmi.sarwad@radisys.com>

Signed-off-by: Rashmi <rashmi.sarwad@radisys.com>

ssanadhya · 2022-11-09T05:19:44Z

lte/gateway/c/core/oai/tasks/sgw_s8/sgw_s8_state_manager.cpp

@@ -124,7 +124,6 @@ void SgwStateManager::free_state() {
                   "temporary_create_session_procedure_id_map ");
    }
  }
-  s8_state_teid_map.map = nullptr;


Why is this removed?

This is removed because map is set to nullptr within destroy_map() itself. This change is done while addressing the comment, #14006 (comment)

ssanadhya · 2022-11-09T05:20:01Z

lte/gateway/c/core/oai/tasks/sgw_s8/sgw_s8_state_manager.cpp

@@ -136,7 +135,6 @@ void SgwStateManager::free_state() {
                   "temporary_create_session_procedure_id_map ");
    }
  }
-  state_cache_p->temporary_create_session_procedure_id_map.map = nullptr;


Why is this removed?

This is removed because map is set to nullptr within destroy_map() itself. This change is done while addressing the comment, #14006 (comment)

Signed-off-by: Rashmi <rashmi.sarwad@radisys.com>

ssanadhya

Lgtm

ssanadhya · 2022-11-16T21:57:37Z

@rsarwad , please rebase to retrigger CI.

…d_hl Signed-off-by: Rashmi <rashmi.sarwad@radisys.com>

github-actions · 2022-11-17T05:12:02Z

FeG Lint & Test

    2 files 203 suites 40s ⏱️
374 tests 374 ✔️ 0 💤 0 ❌
388 runs 388 ✔️ 0 💤 0 ❌

Results for commit 64f82c3.

github-actions · 2022-11-17T05:32:12Z

DP Lint & Test

14 tests 14 ✔️ 2m 14s ⏱️
  1 suites   0 💤
  1 files   0 ❌

Results for commit 64f82c3.

…ask (magma#14006)

rsarwad added 2 commits September 22, 2022 11:22

fix(agw): Modified code port hash list, temporary_create_session_proc…

8bcdef5

…edure_id to protobuf map Signed-off-by: Rashmi <rashmi.sarwad@radisys.com>

fix(agw): Modified code to port teid has list to protobuf in sgw_s8 task

b960f3f

Signed-off-by: Rashmi <rashmi.sarwad@radisys.com>

rsarwad requested review from a team and ajahl September 23, 2022 13:55

pull-request-size bot added the size/L Denotes a Pull Request that changes 100-499 lines. label Sep 23, 2022

github-actions bot added the component: agw Access gateway-related issue label Sep 23, 2022

github-actions bot reviewed Sep 23, 2022

View reviewed changes

pruthvihebbani requested changes Oct 12, 2022

View reviewed changes

rsarwad added 2 commits October 13, 2022 12:29

Addressed review comment

bac2380

Signed-off-by: Rashmi <rashmi.sarwad@radisys.com>

Merge remote-tracking branch 'upstream/master' into 11190_s8_port_tei…

20600a5

…d_hl Signed-off-by: Rashmi <rashmi.sarwad@radisys.com>

rsarwad requested a review from pruthvihebbani October 13, 2022 07:12

ssanadhya requested changes Oct 13, 2022

View reviewed changes

rsarwad force-pushed the 11190_s8_port_teid_hl branch from 1ca936d to 5215f3e Compare October 14, 2022 06:51

ajahl reviewed Oct 28, 2022

View reviewed changes

Addressed review comment

2ecc05e

Signed-off-by: Rashmi <rashmi.sarwad@radisys.com>

rsarwad force-pushed the 11190_s8_port_teid_hl branch from 5215f3e to 2ecc05e Compare November 4, 2022 10:13

Merge remote-tracking branch 'upstream/master' into 11190_s8_port_tei…

1a5c720

…d_hl Signed-off-by: Rashmi <rashmi.sarwad@radisys.com>

pruthvihebbani approved these changes Nov 4, 2022

View reviewed changes

rsarwad requested a review from ssanadhya November 7, 2022 04:35

rsarwad added 2 commits November 7, 2022 10:06

Merge remote-tracking branch 'upstream/master' into 11190_s8_port_tei…

e686766

…d_hl Signed-off-by: Rashmi <rashmi.sarwad@radisys.com>

Resolved merge conflicts

b49a9fa

Signed-off-by: Rashmi <rashmi.sarwad@radisys.com>

rsarwad force-pushed the 11190_s8_port_teid_hl branch from 5161a80 to b49a9fa Compare November 8, 2022 09:07

ssanadhya requested changes Nov 9, 2022

View reviewed changes

rsarwad requested a review from ssanadhya November 9, 2022 17:10

pull-request-size bot added size/M Denotes a PR that changes 30-99 lines. and removed size/L Denotes a Pull Request that changes 100-499 lines. labels Nov 10, 2022

rsarwad force-pushed the 11190_s8_port_teid_hl branch from 2b6408c to 0a83b01 Compare November 10, 2022 06:41

Resolved merge conflicts

6be4c38

Signed-off-by: Rashmi <rashmi.sarwad@radisys.com>

rsarwad force-pushed the 11190_s8_port_teid_hl branch from 0a83b01 to 6be4c38 Compare November 10, 2022 07:02

ssanadhya approved these changes Nov 16, 2022

View reviewed changes

Merge remote-tracking branch 'upstream/master' into 11190_s8_port_tei…

64f82c3

…d_hl Signed-off-by: Rashmi <rashmi.sarwad@radisys.com>

pull-request-size bot added size/L Denotes a Pull Request that changes 100-499 lines. and removed size/M Denotes a PR that changes 30-99 lines. labels Nov 17, 2022

rsarwad merged commit 2674612 into magma:master Nov 17, 2022

lucasgonze pushed a commit to lucasgonze/magma that referenced this pull request Feb 29, 2024

fix(agw): Modified code to port teid has list to protobuf in sgw_s8 t…

adffc2d

…ask (magma#14006)

	constexpr char SGW_S8_CSR_PROC_ID_MAP[] = "sgw_s8_csr_proc_id_map";
	constexpr char SGW_S8_CSR_PROC_ID_MAP_NAME[] = "sgw_s8_csr_proc_id_map";

	free_wrapper(reinterpret_cast<void**>(ptr));
	free_cpp_wrapper(reinterpret_cast<void**>(ptr));

fix(agw): Modified code to port teid has list to protobuf in sgw_s8 task #14006

fix(agw): Modified code to port teid has list to protobuf in sgw_s8 task #14006

Conversation

rsarwad commented Sep 23, 2022

Summary

Test Plan

Additional Information

github-actions bot commented Sep 23, 2022

Howto

More info

github-actions bot Sep 23, 2022

Choose a reason for hiding this comment

github-actions bot Sep 23, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Sep 23, 2022 • edited

feg-workflow

github-actions bot commented Sep 23, 2022 • edited

dp-workflow

github-actions bot commented Sep 23, 2022 • edited

agw-workflow

ajahl commented Sep 26, 2022 • edited

Choose a reason for hiding this comment

rsarwad Oct 13, 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

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

rsarwad Oct 14, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rsarwad Oct 14, 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

pruthvihebbani left a comment

Choose a reason for hiding this comment

pruthvihebbani commented Nov 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

ssanadhya left a comment

Choose a reason for hiding this comment

ssanadhya commented Nov 16, 2022

github-actions bot commented Nov 17, 2022

FeG Lint & Test

github-actions bot commented Nov 17, 2022

DP Lint & Test

github-actions bot commented Sep 23, 2022 •

edited

github-actions bot commented Sep 23, 2022 •

edited

github-actions bot commented Sep 23, 2022 •

edited

ajahl commented Sep 26, 2022 •

edited

rsarwad Oct 13, 2022 •

edited

rsarwad Oct 14, 2022 •

edited

rsarwad Oct 14, 2022 •

edited