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(agw): Migrated ue_id_coll hashlist to protobuf map #12965

Merged
merged 62 commits into from
Jun 29, 2022

Conversation

rsarwad
Copy link
Contributor

@rsarwad rsarwad commented Jun 13, 2022

Summary

Migrated ue_id_coll hashlist to protobuf map. The PR partially addresses the issue, #11190

All tasks are initialized from oai_mme.c file in main function. As we port the individual tasks to cpp, initialize functions are called from main function. And declaration of initialization function of all tasks shall we moved to include/mme_init.hpp file

Test Plan

Executed s1ap sanity test suite

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

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

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

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

Signed-off-by: Rashmi <rashmi.sarwad@radisys.com>
…ad/magma into rsarwad_s1ap_integrate_proto_map

Signed-off-by: Rashmi <rashmi.sarwad@radisys.com>
Signed-off-by: Rashmi <rashmi.sarwad@radisys.com>
Signed-off-by: Rashmi <rashmi.sarwad@radisys.com>
Signed-off-by: Rashmi <rashmi.sarwad@radisys.com>
Signed-off-by: Rashmi <rashmi.sarwad@radisys.com>
…ad/magma into rsarwad_s1ap_integrate_proto_map

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

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

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

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

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

Signed-off-by: Rashmi <rashmi.sarwad@radisys.com>
Signed-off-by: Rashmi <rashmi.sarwad@radisys.com>
@rsarwad rsarwad force-pushed the rsarwad_s1ap_integrate_proto_map branch from 9e36fe0 to 83a96b9 Compare June 20, 2022 11:39
@github-actions github-actions bot added the component: orc8r Orchestrator-related issue label Jun 20, 2022
…grate_proto_map

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

@nstng nstng left a comment

Choose a reason for hiding this comment

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

bazel changes lgtm (only BUILD.bazel change)

Copy link
Collaborator

@ssanadhya ssanadhya left a comment

Choose a reason for hiding this comment

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

This version is still using proto_map_uint32_uint64_to_proto to copy the in-memory object to the proto generated object. Do we still need it after changing s1ap_state.proto?

@@ -34,7 +34,7 @@ message EnbDescription {
uint32 instreams = 8; // sctp_stream_id_t
uint32 outstreams = 9; // sctp_stream_id_t

map<uint64, uint64> ue_ids = 10; // mme_ue_s1ap_id -> comp_s1ap_id
Copy link
Collaborator

Choose a reason for hiding this comment

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

You cannot modify the proto field since it will not be backwards compatible. We need to mark this as deprecated, and add a new field

map<uint32, uint64> ue_id_map = 14;

@rsarwad rsarwad force-pushed the rsarwad_s1ap_integrate_proto_map branch from 5d50a9f to f97500f Compare June 24, 2022 05:09
@rsarwad
Copy link
Contributor Author

rsarwad commented Jun 24, 2022

This version is still using proto_map_uint32_uint64_to_proto to copy the in-memory object to the proto generated object. Do we still need it after changing s1ap_state.proto?
You are right. We will not be using proto_map_uint32_uint64_to_proto to copy the in-memory object to the proto generated object. My mistake, I had changes in another branch but forgot take those changes here.
Thanks for letting me know.
Currently copy is done, https://github.com/magma/magma/pull/12965/files#diff-5fca6c74984139ea6b202f288979263dd8a33dcba810034f1075c5932d6868e7R125 and https://github.com/magma/magma/pull/12965/files#diff-5fca6c74984139ea6b202f288979263dd8a33dcba810034f1075c5932d6868e7R152

@rsarwad rsarwad force-pushed the rsarwad_s1ap_integrate_proto_map branch from f97500f to c99727b Compare June 24, 2022 06:18
@pull-request-size pull-request-size bot added size/XL Denotes a Pull Request that changes 500-999 lines. and removed size/L Denotes a Pull Request that changes 100-499 lines. labels Jun 24, 2022
@rsarwad rsarwad force-pushed the rsarwad_s1ap_integrate_proto_map branch from c99727b to d8e3e36 Compare June 24, 2022 07:19
Signed-off-by: Rashmi <rashmi.sarwad@radisys.com>
…grate_proto_map

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

Signed-off-by: Rashmi <rashmi.sarwad@radisys.com>
@rsarwad
Copy link
Contributor Author

rsarwad commented Jun 27, 2022

proto_map_uint32_uint64_to_proto

This version is still using proto_map_uint32_uint64_to_proto to copy the in-memory object to the proto generated object. Do we still need it after changing s1ap_state.proto?

Incorporated review comment.

…grate_proto_map

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

@ssanadhya ssanadhya left a comment

Choose a reason for hiding this comment

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

Minor comments, will continue reviewing

hash_table_uint64_ts_t ue_id_coll; ///< Contains comp_s1ap_id assoc to
///< enodeb, key is mme_ue_s1ap_id;
magma::proto_map_uint32_uint64_t
ue_id_coll; ///< Contains comp_s1ap_id assoc to
Copy link
Collaborator

Choose a reason for hiding this comment

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

The comment here can be simplified to

/// key: mme_ue_s1ap_id, value: comp_s1ap_id

@@ -0,0 +1,32 @@
/**
* Copyright 2022 The Magma Authors.
Copy link
Collaborator

Choose a reason for hiding this comment

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

But the init function for each task should be part of that task directory, instead of being in a common header. It will be good to retain this under s1ap_mme.hpp.

rc = s1ap_mme_generate_s1_setup_failure(
assoc_id, S1ap_Cause_PR_transport,
S1ap_CauseTransport_transport_resource_unavailable,
S1ap_TimeToWait_v20s);
increment_counter("s1_setup", 1, 2, "result", "failure", "cause",
"invalid_state");
/* UE state at s1ap task is created on reception of initial ue message
* Hash list, ue_id_coll is updated after mme_app_task assigns and provides
* map, ue_id_coll is updated after mme_app_task assigns and provides
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
* map, ue_id_coll is updated after mme_app_task assigns and provides
* The map ue_id_coll is updated after mme_app_task assigns and provides

@@ -3704,12 +3699,12 @@ status_code_e s1ap_handle_sctp_disconnection(s1ap_state_t* state,

if (reset) {
/* UE state at s1ap task is created on reception of initial ue message
* Hash list, ue_id_coll is updated after mme_app_task assigns and provides
* map, ue_id_coll is updated after mme_app_task assigns and provides
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
* map, ue_id_coll is updated after mme_app_task assigns and provides
* The map ue_id_coll is updated after mme_app_task assigns and provides

Signed-off-by: Rashmi <rashmi.sarwad@radisys.com>
@rsarwad rsarwad force-pushed the rsarwad_s1ap_integrate_proto_map branch from cf803ed to 5eac6aa Compare June 27, 2022 18:13
Copy link
Collaborator

@ssanadhya ssanadhya left a comment

Choose a reason for hiding this comment

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

Lgtm, with minor nit

@@ -3704,12 +3699,12 @@ status_code_e s1ap_handle_sctp_disconnection(s1ap_state_t* state,

if (reset) {
/* UE state at s1ap task is created on reception of initial ue message
* Hash list, ue_id_coll is updated after mme_app_task assigns and provides
* The map, ue_id_coll is updated after mme_app_task assigns and provides
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
* The map, ue_id_coll is updated after mme_app_task assigns and provides
* The map ue_id_coll is updated after mme_app_task assigns and provides

…grate_proto_map

Signed-off-by: Rashmi <rashmi.sarwad@radisys.com>
@rsarwad rsarwad force-pushed the rsarwad_s1ap_integrate_proto_map branch from 2937e05 to d1a4597 Compare June 29, 2022 04:23
if (free_callback_func) {
valueT value;
if (get(key, &value) == PROTO_MAP_OK) {
free_callback_func( reinterpret_cast<void**>(&value));
Copy link
Contributor

Choose a reason for hiding this comment

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

[cpplint] reported by reviewdog 🐶
Extra space after ( in function call [whitespace/parens] [4]

…grate_proto_map

Signed-off-by: Rashmi <rashmi.sarwad@radisys.com>
@rsarwad rsarwad force-pushed the rsarwad_s1ap_integrate_proto_map branch from 7f60dba to 920b433 Compare June 29, 2022 05:02
…b_hl

Revert "fix(agw): Modified code to replace hashlist with protobuf map for enb_description_t in s1ap module"
@rsarwad rsarwad merged commit 5c2638b into magma:master Jun 29, 2022
emakeev pushed a commit to emakeev/magma that referenced this pull request Aug 5, 2022
* fix(agw): Added code to migrate ue_id_coll hashlist to ptotbuf map
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: agw Access gateway-related issue component: orc8r Orchestrator-related issue size/XL Denotes a Pull Request that changes 500-999 lines.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants