Skip to content

[lte][agw] Updating deletion of s1ap imsi map entry when UE context is deleted#6800

Merged
ardzoht merged 2 commits intomagma:masterfrom
ardzoht:remove_s1ap_imsi_map_ue
May 11, 2021
Merged

[lte][agw] Updating deletion of s1ap imsi map entry when UE context is deleted#6800
ardzoht merged 2 commits intomagma:masterfrom
ardzoht:remove_s1ap_imsi_map_ue

Conversation

@ardzoht
Copy link
Copy Markdown
Contributor

@ardzoht ardzoht commented May 6, 2021 •
edited
Loading

Signed-off-by: Alex Rodriguez [email protected]

Summary

  • s1ap_imsi_map entries were not always being deleted when UE context was removed, previously they were only deleted on s1ap_mme_handle_ue_context_rel_comp_timer_expiry function handler, this PR updates the removal of the entry when the S1AP UE context is deleted as well.
  • Removing dead code on s1ap_state
  • Updating state_cli to parse s1ap_imsi_map

Test Plan

  • Running scale tests on spirent to ensure s1ap_imsi_map is not leaving stale entries (300 UEs with 5 UE/sec attach rate, stable for 12 hours)

image

  • state_cli.py parse s1ap_imsi_map entries (master) => 3376
  • state_cli.py parse s1ap_imsi_map entries (PR) => 300

Additional Information

  • This change is backwards-breaking

@ardzoht ardzoht requested review from a team, pshelar and ulaskozat May 6, 2021 16:32
@pull-request-size pull-request-size bot added the size/M Denotes a PR that changes 30-99 lines. label May 6, 2021
@ardzoht ardzoht force-pushed the remove_s1ap_imsi_map_ue branch from 9d06c20 to ca0efc8 Compare May 6, 2021 16:32
@magmabot magmabot added the component: agw Access gateway-related issue label May 6, 2021
ulaskozat
ulaskozat reviewed May 6, 2021
View reviewed changes
Comment thread lte/gateway/c/oai/tasks/s1ap/s1ap_mme.c Outdated
s1ap_imsi_map->mme_ue_id_imsi_htbl, (const hash_key_t) mme_ue_s1ap_id,
&imsi64);
delete_s1ap_ue_state(imsi64);
hashtable_uint64_ts_free(s1ap_imsi_map->mme_ue_id_imsi_htbl, mme_ue_s1ap_id);
Copy link
Copy Markdown
Contributor

@ulaskozat ulaskozat May 6, 2021

Choose a reason for hiding this comment

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

what is the difference between free vs remove?

Copy link
Copy Markdown
Contributor Author

@ardzoht ardzoht May 6, 2021

Choose a reason for hiding this comment

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

same question came to me, after investigating:

  • for hashtable_uint64 implementation, there is no difference between the ts_free and ts_remove handlers, both of them call free_wrapper on the node to be removed, and update the nodes accordingly
  • for hashtable, ts_free frees the object on the hashtable, and ts_remove removes the object on the hashtable nodes, but does not free the object itself

Copy link
Copy Markdown
Contributor

@ulaskozat ulaskozat May 6, 2021

Choose a reason for hiding this comment

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

I see that hashtable_uint64_ts_remove used everywhere except for one. Let's eliminate the usage of hashtable_uint64_ts_free as it would lead to confusion. on a separate PR maybe we should revisit hashtable itself and use a better naming than remove if it is not freeing but simply popping the entry.

Copy link
Copy Markdown
Contributor Author

@ardzoht ardzoht May 6, 2021

Choose a reason for hiding this comment

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

@ulaskozat Removed hashtable_uint64_ts_free usage and function

@ardzoht ardzoht force-pushed the remove_s1ap_imsi_map_ue branch from ca0efc8 to f875b47 Compare May 6, 2021 20:44
@pull-request-size 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 May 6, 2021
ulaskozat
ulaskozat approved these changes May 6, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@ulaskozat ulaskozat left a comment

Choose a reason for hiding this comment

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

LGTM! Great fix...

@ulaskozat ulaskozat linked an issue May 6, 2021 that may be closed by this pull request
[lte][agw] Duplicate initial UE request #5315
Closed
@ulaskozat ulaskozat added apply-v1.4 Needs to be applied to v1.4 release branch as well apply-v1.5 Apply this commit to the v1.5 release branch as well. labels May 6, 2021
@ardzoht ardzoht force-pushed the remove_s1ap_imsi_map_ue branch from f875b47 to 9d569f1 Compare May 7, 2021 18:00
@electronjoe electronjoe mentioned this pull request May 7, 2021
Closed
@bbarritt
Copy link
Copy Markdown
Contributor

bbarritt commented May 8, 2021

Nice find!

Ideally, I'd like to see us get in the habit of adding the missing unit test (or else a presubmit CI test) that could have caught this every time we fix a critical bug (or a SEV). I understand that sometimes it's not practical, thought.

Thoughts on that the context of this fix and PR?

@ardzoht ardzoht force-pushed the remove_s1ap_imsi_map_ue branch from 9d569f1 to 74c48e1 Compare May 10, 2021 15:58
@ardzoht
Copy link
Copy Markdown
Contributor Author

ardzoht commented May 10, 2021 •
edited by bbarritt
Loading

Nice find!

Ideally, I'd like to see us get in the habit of adding the missing unit test (or else a presubmit CI test) that could have caught this every time we fix a critical bug (or a SEV). I understand that sometimes it's not practical, though.

Thoughts on that the context of this fix and PR?

@bbarritt Definitely, I think due to the nature of this change an integration test makes more sense here, I will add one in this PR

@rdefosse
Copy link
Copy Markdown
Contributor

rdefosse commented May 10, 2021

@ardzoht Alex

False negative on the OAI pipeline.

Raphael

@ardzoht ardzoht requested a review from a team as a code owner May 11, 2021 18:22
@ardzoht ardzoht requested review from a team and HannaFar May 11, 2021 18:22
@ardzoht ardzoht force-pushed the remove_s1ap_imsi_map_ue branch from cda0e7d to 0f3d1ae Compare May 11, 2021 18:23
@themarwhal themarwhal removed request for a team and HannaFar May 11, 2021 18:36
@ardzoht
Copy link
Copy Markdown
Contributor Author

ardzoht commented May 11, 2021

I ended up including a post check for every s1ap test run, that will validate that all the entries of s1ap_imsi_map are indeed deleted cc @bbarritt @electronjoe

@ardzoht ardzoht merged commit 8067222 into magma:master May 11, 2021
themarwhal pushed a commit that referenced this pull request May 11, 2021
@ardzoht @themarwhal
[lte][agw] Updating deletion of s1ap imsi map entry when UE context i…
e6e970e 
…s deleted (#6800)

* Removing s1ap imsi map entry on s1ap_remove_ue

Signed-off-by: Alex Rodriguez <[email protected]>

* Adding s1ap imsi map entries check to s1ap tests cleanup step

Signed-off-by: Alex Rodriguez <[email protected]>
@electronjoe
Copy link
Copy Markdown
Member

electronjoe commented May 12, 2021

I ended up including a post check for every s1ap test run, that will validate that all the entries of s1ap_imsi_map are indeed deleted cc @bbarritt @electronjoe

Thanks Alex! We <3 automated testing regression prevention =D

@themarwhal themarwhal added the backported-v1.4 Has been backported to v1.4 release branch label May 12, 2021
themarwhal pushed a commit that referenced this pull request May 12, 2021
@ardzoht @themarwhal
[lte][agw] Updating deletion of s1ap imsi map entry when UE context i…
6c676cd 
…s deleted (#6800)

* Removing s1ap imsi map entry on s1ap_remove_ue

Signed-off-by: Alex Rodriguez <[email protected]>

* Adding s1ap imsi map entries check to s1ap tests cleanup step

Signed-off-by: Alex Rodriguez <[email protected]>
@ardzoht ardzoht mentioned this pull request Jul 12, 2021
Closed
m-trojanowski pushed a commit to openEPC/magma that referenced this pull request Oct 20, 2021
@ardzoht @themarwhal
[lte][agw] Updating deletion of s1ap imsi map entry when UE context i…
49d1286 
…s deleted (magma#6800)

* Removing s1ap imsi map entry on s1ap_remove_ue

Signed-off-by: Alex Rodriguez <[email protected]>

* Adding s1ap imsi map entries check to s1ap tests cleanup step

Signed-off-by: Alex Rodriguez <[email protected]>
m-trojanowski pushed a commit to openEPC/magma that referenced this pull request Oct 20, 2021
@ardzoht @themarwhal
[lte][agw] Updating deletion of s1ap imsi map entry when UE context i…
e8245f3 
…s deleted (magma#6800)

* Removing s1ap imsi map entry on s1ap_remove_ue

Signed-off-by: Alex Rodriguez <[email protected]>

* Adding s1ap imsi map entries check to s1ap tests cleanup step

Signed-off-by: Alex Rodriguez <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pshelar pshelar pshelar approved these changes

@ulaskozat ulaskozat ulaskozat approved these changes

@themarwhal themarwhal themarwhal approved these changes

Assignees

No one assigned

Labels

apply-v1.4 Needs to be applied to v1.4 release branch as well apply-v1.5 Apply this commit to the v1.5 release branch as well. backported-v1.4 Has been backported to v1.4 release branch backported-v1.5 component: agw Access gateway-related issue size/L Denotes a Pull Request that changes 100-499 lines.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[lte][agw] Duplicate initial UE request

8 participants

@ardzoht @bbarritt @rdefosse @electronjoe @pshelar @ulaskozat @themarwhal @magmabot