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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 2 commits into from May 11, 2021
Merged

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

merged 2 commits into from May 11, 2021

Conversation

ardzoht
Copy link
Contributor

@ardzoht ardzoht commented May 6, 2021 •
edited

Signed-off-by: Alex Rodriguez ardzoht@gmail.com

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, ulaskozat and pshelar 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
@magmabot magmabot added the component: agw Access gateway-related issue label May 6, 2021
ulaskozat
ulaskozat reviewed May 6, 2021
View reviewed changes
lte/gateway/c/oai/tasks/s1ap/s1ap_mme.c Outdated
@@ -577,6 +555,7 @@ void s1ap_remove_ue(s1ap_state_t* state, ue_description_t* ue_ref) {
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
Contributor

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
Contributor Author

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
Contributor

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
Contributor Author

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

@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
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
@electronjoe electronjoe mentioned this pull request May 7, 2021
Closed
@bbarritt
Copy link
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
Copy link
Contributor Author

ardzoht commented May 10, 2021 •
edited by bbarritt

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
Contributor

@ardzoht Alex

False negative on the OAI pipeline.

Raphael

@ardzoht
Removing s1ap imsi map entry on s1ap_remove_ue
7a0b300 
Signed-off-by: Alex Rodriguez <ardzoht@gmail.com>
@ardzoht
Adding s1ap imsi map entries check to s1ap tests cleanup step
0f3d1ae 
Signed-off-by: Alex Rodriguez <ardzoht@gmail.com>
@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
@themarwhal themarwhal removed request for a team and HannaFar May 11, 2021 18:36
@ardzoht
Copy link
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 <ardzoht@gmail.com>

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

Signed-off-by: Alex Rodriguez <ardzoht@gmail.com>
@electronjoe
Copy link
Member

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 <ardzoht@gmail.com>

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

Signed-off-by: Alex Rodriguez <ardzoht@gmail.com>
@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 <ardzoht@gmail.com>

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

Signed-off-by: Alex Rodriguez <ardzoht@gmail.com>
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 <ardzoht@gmail.com>

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

Signed-off-by: Alex Rodriguez <ardzoht@gmail.com>
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