feat(amf): Added Implicit Deregistration Support by Lkishor123 · Pull Request #11995 · magma/magma

Lkishor123 · 2022-03-07T13:35:38Z

Signed-off-by: LKishor123 [email protected]

Summary

Removed additional maps which were storing redundant information

ue_context_map (already having state_ue_map), amf_supi_guti_map (already existing maps can be used)

Introduced amf_nas_proc_implicit_deregister_ue_ind() for cleaning the older UE when
getting a new registration from same (without deregistration)
Functions : amf_nas_proc_implicit_deregister_ue_ind,
amf_cn_implicit_deregister_ue,
Introduced the related timers (mobile rechability and implicit deregistration)
Introduced new create_new_registration_info

For storing the information of new registration when their is already a registred UE
This will also trigger amf_nas_proc_implicit_deregister_ue_ind

Introduced new function proc_new_registration_req() that will be triggered at the end
of implicit_deregistration
Added the handling of context release complete message
Corrected the usage for cm_state (connection Management) to track GNB connectivity.
This will also be used for triggere
Flattned the Ngcause and replaced with simple enums n2cause
Code re-organization and new lookup functions

Test Plan

Unit Testcases added:

ImplicitDeregDuplicateSuciReg
ServiceRequestWrongTMSI
SctpShutWithServiceRequest
ServiceRequestSignalWithPDU

Additional Information

This change is backwards-breaking

github-actions · 2022-03-07T13:35:56Z

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-03-07T13:46:21Z

feg-workflow

    2 files 202 suites 33s ⏱️
362 tests 362 ✔️ 0 💤 0 ❌
376 runs 376 ✔️ 0 💤 0 ❌

Results for commit 52cef39.

♻️ This comment has been updated with latest results.

github-actions · 2022-03-07T14:41:59Z

agw-workflow

    71 files   101 suites 6m 4s ⏱️
1 016 tests 1 006 ✔️ 9 💤 0 ❌ 1 🔥
1 017 runs 1 007 ✔️ 9 💤 0 ❌ 1 🔥

For more details on these errors, see this check.

Results for commit b62411d.

♻️ This comment has been updated with latest results.

uri200

please divide this PR into smaller PRs

Lkishor123 · 2022-03-10T12:49:42Z

please divide this PR into smaller PRs

@uri200 thanks for the input.
This PR turned out as a feature and all components are tightly coupled. As it is related to connection management, all the related state machines has to be synced. Breaking it into smaller PRs was having an impact on regression suit, specially in service request handling. We tried it but overall regression results were getting impacted, hence we kept as single PR.

rsarwad

Left some comments on this PR

rsarwad · 2022-03-16T10:26:50Z

+  m5gmm_state_t mm_state;
+  int rc = RETURNerror;
+
+  rc = amf_get_ue_context_mm_state(ue_id, &mm_state);


You can minimize the statements

There are some cleanups required on the Unit Tests and we will take it up in subsequent PR. Also can you please elaborate your suggestion You can minimize the statements, Thanks.

With minimum statement; the above statements can be written as
if ((amf_get_ue_context_mm_state(ue_id, &mm_state)) != RETURNok) {
return false;
}

rsarwad · 2022-03-16T10:37:03Z

+  ula_ans = util_amf_send_s6a_ula(IMSI_STR);
+
+  int rc = amf_handle_s6a_update_location_ans(&ula_ans);
+  EXPECT_TRUE(rc == RETURNok);


We can add some more checks like apn sent in ULA matches with updated UE context.
If we don't send apn; what is expected.

There are changes planed for this and it is planned for a upcoming PR.

Could you please add TODO statement and as part of new PR, you can remove TODO statement

https://app.zenhub.com/workspaces/magma--5g-sa-60871e99b9326f001696b1ee/issues/magma/magma/12156

rsarwad · 2022-03-16T10:48:16Z

+      sizeof(ue_auth_response_hexbuf));
+  EXPECT_TRUE(rc == RETURNok);
+
+  /* Send uplink nas message for security mode complete response from UE */


General comment, use // for single line comment

https://app.zenhub.com/workspaces/magma--5g-sa-60871e99b9326f001696b1ee/issues/magma/magma/12156

rsarwad · 2022-03-16T10:58:23Z

 #define T3586_DEFAULT_VALUE 0  // TODO-RECHECK
 #define T3589_DEFAULT_VALUE 0  // TODO-RECHECK
 #define T3595_DEFAULT_VALUE 0
+#define IMPLICIT_DEREG_TIMER 30


Could you rename to IMPLICIT_DEREG_TIMER_VALUE.
Implicit detach timer value is 30 or the value of T3512 timer + 4 minutes

Yes agreed, we will raise a new PR. Raised a zenhub task for this.

https://app.zenhub.com/workspaces/magma--5g-sa-60871e99b9326f001696b1ee/issues/magma/magma/12156

rsarwad · 2022-03-16T11:51:21Z

+  if (amf_ctx_p) {
+    rc = amf_proc_deregistration_request(ue_id, &params);
+  } else {
+    OAILOG_ERROR(LOG_NAS_AMF, "Error: amf_ctx does not exits");


Can you include ue_ngap_id while logging

General comment: While logging if amf context have valid imsi64 then use OAILOG_ERROR_UE with appropriate log level

https://app.zenhub.com/workspaces/magma--5g-sa-60871e99b9326f001696b1ee/issues/magma/magma/12156

rsarwad · 2022-03-17T03:52:27Z

LGTM and hoping remaining comments would be taken care in https://app.zenhub.com/workspaces/magma--5g-sa-60871e99b9326f001696b1ee/issues/magma/magma/12156

Lkishor123 · 2022-03-21T20:47:08Z

LGTM and hoping remaining comments would be taken care in https://app.zenhub.com/workspaces/magma--5g-sa-60871e99b9326f001696b1ee/issues/magma/magma/12156

@ulaskozat , @rsarwad please merge the PR.

rsarwad · 2022-03-24T09:25:12Z

  NGAP_NAS_UE_NOT_AVAILABLE_FOR_PS,
  NGAP_USER_INACTIVITY
-};
+} n2cause;


Could you rename to n2cause_e to indicate as enum

Comment addressed.

rsarwad · 2022-03-24T09:48:03Z

-  if ((amf_ue_ngap_id != INVALID_AMF_UE_NGAP_ID) && (imsi != 0)) {
-    m_rc = amf_ue_context_p->imsi_amf_ue_id_htbl.remove(
-        ue_context_p->amf_context.imsi64);
+  m_rc = amf_ue_context_p->imsi_amf_ue_id_htbl.remove(


Removal of imsi from imsi_amf_ue_id_htbl, should be under below condition, because if we update gnb_ngap_id_key, we don't return from there so we will hit this statement and removes the entry from hash list.

m_rc is not required here

Comment addressed.

rsarwad · 2022-03-24T09:53:36Z

+    amf_ue_context_t* const amf_ue_context_p, const guti_m5_t* const guti_p) {
+  magma::map_rc_t m_rc = magma::MAP_OK;
+  uint64_t amf_ue_ngap_id64 = 0;
+  ue_m5gmm_context_t* ue_context = NULL;


For pointer, it's preferred as ue_context_p

Comment addressed.

rsarwad · 2022-03-24T11:19:16Z

+    amf_ue_context_t* const amf_ue_context_p,
+    struct ue_m5gmm_context_s* ue_context_p, m5gcm_state_t new_cm_state) {
+  // Function is used to update UE's Signaling Connection State
+  hashtable_rc_t hash_rc = HASH_TABLE_OK;


Fix the cpp warning

Comment addressed.

rsarwad · 2022-03-24T11:27:34Z

+ **                                                                        **
+ **                                                                        **
+ ***************************************************************************/
+ue_m5gmm_context_s* amf_ue_context_exists_gnb_ue_ngap_id(


Can be implemented something similar to amf_ue_context_exists_amf_ue_ngap_id() to decrease the number of lines

Comment addressed.

rsarwad · 2022-03-24T11:54:43Z

+  if (ue_context_p->m5_implicit_deregistration_timer.id !=
+      NAS5G_TIMER_INACTIVE_ID) {
+    amf_app_stop_timer(ue_context_p->m5_implicit_deregistration_timer.id);
+    ue_context_p->m5_implicit_deregistration_timer.id = NAS5G_TIMER_INACTIVE_ID;


This statement is not required because anyway m5_implicit_deregistration_timer.id gets updated while starting the timer

Comment addressed.

rsarwad · 2022-03-24T11:57:01Z

-  amf_ue_context_p->tun11_ue_context_htbl.remove(ue_context_p->amf_teid_n11);
+// Api for release UE context
+void amf_ctx_release_ue_context(ue_m5gmm_context_s* ue_context_p,
+                                n2cause n2_cause) {


Add OAILOG_FUNC_IN and OAILOG_FUNC_OUT and for successful case, there is no return statement

Comment addressed.

rsarwad · 2022-03-24T12:00:12Z

+                 "Ue Context Release Timer: ue_amf_context is NULL for "
+                 "ue id: " AMF_UE_NGAP_ID_FMT "\n",
+                 ue_id);
+    OAILOG_FUNC_RETURN(LOG_NAS_AMF, RETURNok);


Module name should be LOG_AMF_APP.
General comment is to use appropriate module name

Comment addressed.

rsarwad · 2022-03-24T12:00:39Z

+      amf_ue_context_exists_amf_ue_ngap_id(ue_id);
+
+  if (ue_context_p == NULL) {
+    return RETURNerror;


Use OAI provided macros while retuning

Comment addressed.

rsarwad · 2022-03-24T12:00:55Z

+      amf_ue_context_exists_amf_ue_ngap_id(ue_id);
+
+  if (ue_context_p == NULL) {
+    return RETURNerror;


Comment addressed.

rsarwad · 2022-03-24T12:03:32Z

 int amf_proc_registration_abort(amf_context_t* amf_ctx,
                                struct ue_m5gmm_context_s* ue_amf_context);
+
+// Fetch the ue contet from imsi


Comment addressed.

Signed-off-by: LKishor123 <[email protected]> Signed-off-by: LKishor123 <[email protected]>

Signed-off-by: LKishor123 <[email protected]>

rsarwad

LGTM

Signed-off-by: sreedharkumartn <[email protected]>

github-actions · 2022-03-29T22:26:43Z

Oops! Looks like you failed the Semantic PR check.

Howto

Instructions on formatting your PR title
For PRs with only one commit, the commit message must also be semantic. See Changing a commit message for a howto
Guide to the different CI checks and resolution guidelines

* feat(amf): Added Implicit Deregistration Support Signed-off-by: LKishor123 <[email protected]> Signed-off-by: LKishor123 <[email protected]> * fix(amf): fixed incorrect flag for implicit deregistration Signed-off-by: LKishor123 <[email protected]> * fix(amf): Review comments addressed Signed-off-by: LKishor123 <[email protected]>

Signed-off-by: sreedharkumartn <[email protected]>

* feat(amf): Added Implicit Deregistration Support Signed-off-by: LKishor123 <[email protected]> Signed-off-by: LKishor123 <[email protected]> * fix(amf): fixed incorrect flag for implicit deregistration Signed-off-by: LKishor123 <[email protected]> * fix(amf): Review comments addressed Signed-off-by: LKishor123 <[email protected]>

Lkishor123 requested review from a team and pruthvihebbani March 7, 2022 13:35

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

Lkishor123 marked this pull request as draft March 7, 2022 13:35

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

uri200 reviewed Mar 10, 2022

View reviewed changes

Lkishor123 marked this pull request as ready for review March 10, 2022 19:28

github-actions bot reviewed Mar 11, 2022

View reviewed changes

Comment thread lte/gateway/c/core/oai/test/amf/amf_app_test_util.h Outdated