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

[SessionD] Clean up LocalEnforcer #2483

Merged

Conversation

themarwhal
Copy link
Member

@themarwhal themarwhal commented Aug 26, 2020

Summary

4 things are covered in this PR:

  1. Logic refactor on the session termination flow
    This change was brought up because I was noticing this log all the time when observing SessionD logs. [IMSI] Forcefully terminating session since it did not receive usage from pipelined in time. I initially thought there was a bug somewhere that was causing sessions to be terminated forcefully all the time. (As opposed to terminating itself after PipelineD's enforcement stats were cleared.) But it turns out the logs were inaccurate.
    This log comes from SessionState's complete_termination, which is used in both force termination and regular termination. So the fix here is to update the logs to reflect the behavior.
    Finally, while I was looking at the function, I noticed that LocalEnforcer passes the SessionReporter into the function, which is a bit of an anti-pattern since we do all reporting from either LocalSessionManagerHandler or LocalEnforcer. So I moved out the logic to LocalEnforcer.
  2. Some cosmetic changes to rename variable update_criteria -> uc
  3. Light refactor to remove unused parameters/return values from LocalEnforcer
    There are to main changes. Many of the LocalEnforcer functions had 'session_map' as a parameter when it is not used at all. (This is probably left over from legacy code). Second, PipelineDClient always returns true as a result of making an asynchronous call, so there is no value in casing on its return value.
  4. test_local_enforcer clean up + deprecate test-only methods
    Remove LocalEnforcer's get_charging_credit and get_monitor_credit since they are only used for testing. The functions also just have a questionable way to go from IMSI, bucket -> value. (Sessions are unique by IMSI+SessionID, not just IMSI). Instead, assert_charging_credit and assert_monitor_credit are used in test_local_enforcer.cpp. Additionally, instead of having hardcoded imsi/sessionID test values, migrate to a model where we use a predefined set of values.

Test Plan

unit tests, s1ap test (ran it locally it passed)

Additional Information

  • This change is backwards-breaking

@themarwhal themarwhal added component: agw Access gateway-related issue SessionD labels Aug 26, 2020
@themarwhal themarwhal force-pushed the cleanup-force-timeout-termination branch from e3b3c07 to 742ab66 Compare August 26, 2020 16:10
@themarwhal themarwhal changed the title [SessionD] Cleanup force timeout termination [SessionD] Cleanup Session termination logic Aug 26, 2020
@themarwhal themarwhal marked this pull request as ready for review August 26, 2020 16:23
@themarwhal themarwhal force-pushed the cleanup-force-timeout-termination branch 2 times, most recently from 00398d0 to e1b0d60 Compare August 28, 2020 13:18
@themarwhal themarwhal changed the title [SessionD] Cleanup Session termination logic [SessionD] Clean up LocalEnforcer Aug 28, 2020
@themarwhal themarwhal force-pushed the cleanup-force-timeout-termination branch 2 times, most recently from aa27e26 to 1d67e2f Compare August 31, 2020 08:47
@themarwhal themarwhal force-pushed the cleanup-force-timeout-termination branch 2 times, most recently from 337f41d to 642135c Compare September 2, 2020 14:50
@themarwhal themarwhal changed the base branch from master to develop September 2, 2020 14:53
@themarwhal themarwhal changed the base branch from develop to master September 2, 2020 14:56
@themarwhal themarwhal force-pushed the cleanup-force-timeout-termination branch from 642135c to 33c5684 Compare September 8, 2020 15:01
Copy link
Contributor

@uri200 uri200 left a comment

Choose a reason for hiding this comment

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

lgtm

Signed-off-by: Marie Bremner <marwhal@fb.com>
@themarwhal themarwhal force-pushed the cleanup-force-timeout-termination branch 2 times, most recently from 284684b to 8c38df6 Compare September 9, 2020 11:23
…e not LocalEnforcer + use const values for IMSI and SessionID

Signed-off-by: Marie Bremner <marwhal@fb.com>

Use SessionState's get_monitor instead of LocalEnforcer's get_monitor_credit in testing

Signed-off-by: Marie Bremner <marwhal@fb.com>
@themarwhal themarwhal force-pushed the cleanup-force-timeout-termination branch from 8c38df6 to a493e2b Compare September 9, 2020 11:49
@themarwhal themarwhal merged commit 695bf8d into magma:master Sep 9, 2020
@themarwhal themarwhal deleted the cleanup-force-timeout-termination branch September 15, 2020 14:24
ulaskozat pushed a commit to ulaskozat/magma that referenced this pull request Sep 16, 2020
* Clean up session force timeout logic

Signed-off-by: Marie Bremner <marwhal@fb.com>

* Migrate test_local_enforcer to use get_charging_credit in SessionState not LocalEnforcer + use const values for IMSI and SessionID

Signed-off-by: Marie Bremner <marwhal@fb.com>

Use SessionState's get_monitor instead of LocalEnforcer's get_monitor_credit in testing

Signed-off-by: Marie Bremner <marwhal@fb.com>
rdefosse pushed a commit to rdefosse/magma that referenced this pull request Oct 9, 2020
* Clean up session force timeout logic

Signed-off-by: Marie Bremner <marwhal@fb.com>

* Migrate test_local_enforcer to use get_charging_credit in SessionState not LocalEnforcer + use const values for IMSI and SessionID

Signed-off-by: Marie Bremner <marwhal@fb.com>

Use SessionState's get_monitor instead of LocalEnforcer's get_monitor_credit in testing

Signed-off-by: Marie Bremner <marwhal@fb.com>
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants