fix: [sessiond] only unsuspend credits that have non 0 quota by themarwhal · Pull Request #7037 · magma/magma

themarwhal · 2021-05-20T22:09:54Z

Summary

See attached issue for the reported problem.

In short, we have to make sure that a credit has >0 quota before un-suspending.
We currently only check for these two things:

credit received is valid
credit is in suspension

But since we could receive a valid credit with 0 grant, we should also check that the there is quota available.

The logic is slightly messy as we handle 'suspended' credits slightly differently from credits that have been deactivated due to FUA-redirect/restrict.

In order to make this work, I've had to make two changes:

When determining whether to remove Gy rules, check that the credit needs to be activated. This is done by the logic check:

// in update_charging_credits
bool should_activate = session->is_credit_ready_to_be_activated(credit_key);
if (!should_activate) {
  continue;
}
...
if (prev_final_action) {
  // uninstall gy rules
}

When receiving a new credit, determine if the credit needs to be deactivated. We didn't check for this before, but this is useful for the edge case where we receive a FUA-terminate for an already empty quota.

Unit tests added

test_no_credit_reauth_with_redirected_suspended_credit (fails on master w/o the changes)
test_terminate_credit_reauth_with_redirected_suspended_credit (fails on master w/o the changes)

Test Plan

make precommit_sm

the new unit test fails on master w/o this change

Additional Information

This change is backwards-breaking

github-actions · 2021-05-20T22:10:06Z

Thanks for opening a PR! 💯 Please note that all commits must be signed off. This is enforced by the DCO check.

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

uri200

note that there is a difference of being suspended and being with credit 0

If we receive credit, but the credit is 0 and we generate traffic, I guess our session should be terminated.

However, with those changes we will keep the session.

So imagine a situation were the subscriber does not have data anymore, PCRF keeps sending grants 0 (not sure if the PCRF would be so dumb), but the subscriber is never allowed to leave.

Not sure if this situation is even possible, but maybe worth thinking on it before landing this

themarwhal · 2021-05-21T16:50:54Z

note that there is a difference of being suspended and being with credit 0

If we receive credit, but the credit is 0 and we generate traffic, I guess our session should be terminated.

However, with those changes we will keep the session.

So imagine a situation were the subscriber does not have data anymore, PCRF keeps sending grants 0 (not sure if the PCRF would be so dumb), but the subscriber is never allowed to leave.

Not sure if this situation is even possible, but maybe worth thinking on it before landing this

@uri200

Yeah well this case is it is already suspended AND credit is 0. I think in this case we don't want to unsuspend the credit even if the OCS keeps sending 0.

Although the case we might not handling might be:

we receive a credit with FUA-redirect/restrict
we use up our credit and go into redirect/restrict aka suspension
we get an update by expiry timer / RAR still with 0 grant but now with fua-terminate

I think in this case hopefully we terminate the session. But who knows :D

I'll write a unit test and see what happens in that case.

uri200 · 2021-05-24T17:12:24Z

+                 << " for " << session_id << " during update. Not action taken";
+  }
+  return true;
+}


I remember I tried with a similar function and I felt that the function was mixing too many things, doing stuff in some objects, and returning a value. That is why I prefered to had the logic on update_charging_credits

Yeah it's a bit opaque but tried to make it so that the boolean value returned just indicates whether we want to further process the credit or not.
The update_charging_credits function was getting too long/ hard to read :/

I can add a comment on the line where we call the function too, if that helps with clarity.

yes, maybe a comment will help us in the future.

uri200 · 2021-05-24T17:19:27Z

  }
+  if (grant->should_deactivate_service()) {
+    grant->set_service_state(SERVICE_NEEDS_DEACTIVATION, credit_uc);
+  }


why do we check it here. Don't we have to wait for the next cycle to check?

@uri200 This case will only cover the case where the credit is already exhausted and FUA-terminate is set. Since at this point no new usage is added, it just acts on the terminate instruction.
(I don't think it makes much sense to wait until the next cycle in this case)

uri200 · 2021-05-24T17:43:42Z

I would recommend to run this on TeraVM with tests

gxgy05.ntl
gxgy05_capacity.ntl
gxgy06.ntl
gxgy06_capacity.ntl

(5 is gy credit, and 6 is gx monitors)

example

~/magma/automation$ ng40test gxgy05_capacity.ntl

themarwhal · 2021-05-24T19:00:06Z

I would recommend to run this on TeraVM with tests
gxgy05.ntl
gxgy05_capacity.ntl
gxgy06.ntl
gxgy06_capacity.ntl
(5 is gy credit, and 6 is gx monitors)

example
~/magma/automation$ ng40test gxgy05_capacity.ntl

Ran the tests

Signed-off-by: Marie Bremner <[email protected]>

talkhasib · 2021-05-24T21:38:23Z

is this getting backported to 1.5? 1.4?

themarwhal · 2021-05-24T21:42:01Z

is this getting backported to 1.5? 1.4?

@talkhasib yes, but it takes a bit of work. Will put up a PR against 1.5 tomorrow. (Will have to see how difficult backporting to 1.4 will be)

talkhasib · 2021-05-24T21:54:29Z

is this getting backported to 1.5? 1.4?

@talkhasib yes, but it takes a bit of work. Will put up a PR against 1.5 tomorrow. (Will have to see how difficult backporting to 1.4 will be)

Lets only backport to 1.5 for now

Signed-off-by: Marie Bremner <[email protected]>

) Signed-off-by: Marie Bremner <[email protected]>

…7139) Signed-off-by: Marie Bremner <[email protected]>

) Signed-off-by: Ramon Melero <[email protected]>

) (magma#7139) Signed-off-by: Marie Bremner <[email protected]>

pull-request-size bot added the size/L Denotes a Pull Request that changes 100-499 lines. label May 20, 2021

magmabot added the component: agw Access gateway-related issue label May 20, 2021

themarwhal force-pushed the fix-fua-suspension branch from 0371cd2 to aee40b2 Compare May 20, 2021 22:11

themarwhal linked an issue May 20, 2021 that may be closed by this pull request

Gy Redirect Policy Rule removed after Validity Timer triggered CCR-U with FUA Redirect #6993

Closed

themarwhal marked this pull request as ready for review May 21, 2021 01:45

themarwhal requested review from a team and uri200 May 21, 2021 01:45

themarwhal force-pushed the fix-fua-suspension branch 2 times, most recently from e7ee635 to 7a5d793 Compare May 21, 2021 01:53

uri200 reviewed May 21, 2021

View reviewed changes

themarwhal force-pushed the fix-fua-suspension branch from 7a5d793 to 965a2a8 Compare May 21, 2021 18:13

themarwhal requested review from a team and uri200 May 21, 2021 18:15

themarwhal force-pushed the fix-fua-suspension branch 7 times, most recently from c83ab2a to 8a04f03 Compare May 22, 2021 02:31

themarwhal requested a review from a team as a code owner May 22, 2021 02:31

magmabot added the component: cwag CWAG related issues label May 22, 2021

themarwhal force-pushed the fix-fua-suspension branch from 8a04f03 to 515df91 Compare May 22, 2021 13:41

themarwhal requested a review from a team May 22, 2021 13:41

themarwhal force-pushed the fix-fua-suspension branch 2 times, most recently from e39db86 to f3ce648 Compare May 24, 2021 12:35

themarwhal marked this pull request as draft May 24, 2021 12:35

themarwhal force-pushed the fix-fua-suspension branch from f3ce648 to 8b5216e Compare May 24, 2021 12:41

themarwhal force-pushed the fix-fua-suspension branch 6 times, most recently from 7c1ae36 to 6c8fc4c Compare May 24, 2021 16:08

themarwhal marked this pull request as ready for review May 24, 2021 16:12

themarwhal removed request for a team May 24, 2021 16:27

uri200 reviewed May 24, 2021

View reviewed changes

uri200 approved these changes May 24, 2021

View reviewed changes

themarwhal force-pushed the fix-fua-suspension branch from 6c8fc4c to 84a8d73 Compare May 24, 2021 19:10

fix: [sessiond] only unsuspend credits that have non 0 quota

d34e56d

Signed-off-by: Marie Bremner <[email protected]>

themarwhal force-pushed the fix-fua-suspension branch from 84a8d73 to d34e56d Compare May 24, 2021 19:12

themarwhal merged commit f9d6a5f into magma:master May 24, 2021

themarwhal deleted the fix-fua-suspension branch May 24, 2021 19:47

themarwhal added a commit that referenced this pull request May 25, 2021

fix: [sessiond] only unsuspend credits that have non 0 quota (#7037)

fd0fa66

Signed-off-by: Marie Bremner <[email protected]>

themarwhal added a commit that referenced this pull request May 25, 2021

fix: [sessiond] only unsuspend credits that have non 0 quota (#7037)

cad2009

Signed-off-by: Marie Bremner <[email protected]>

themarwhal added a commit to themarwhal/magma that referenced this pull request May 25, 2021

fix: [sessiond] only unsuspend credits that have non 0 quota (magma#7037

8445ac0

) Signed-off-by: Marie Bremner <[email protected]>

themarwhal mentioned this pull request May 25, 2021

fix: [sessiond] only unsuspend credits that have non 0 quota (#7037) #7139

Merged

1 task

themarwhal added a commit that referenced this pull request May 25, 2021

fix: [sessiond] only unsuspend credits that have non 0 quota (#7037) (#…

2af1e2c

…7139) Signed-off-by: Marie Bremner <[email protected]>

rmeleromira pushed a commit to rmeleromira/magma that referenced this pull request Jul 24, 2021

fix: [sessiond] only unsuspend credits that have non 0 quota (magma#7037

f269257

) Signed-off-by: Ramon Melero <[email protected]>

m-trojanowski pushed a commit to openEPC/magma that referenced this pull request Oct 20, 2021

fix: [sessiond] only unsuspend credits that have non 0 quota (magma#7037

511df2a

) (magma#7139) Signed-off-by: Marie Bremner <[email protected]>

m-trojanowski pushed a commit to openEPC/magma that referenced this pull request Oct 20, 2021

fix: [sessiond] only unsuspend credits that have non 0 quota (magma#7037

5793bb6

) (magma#7139) Signed-off-by: Marie Bremner <[email protected]>

Conversation

themarwhal commented May 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Unit tests added

Test Plan

Additional Information

Uh oh!

github-actions bot commented May 20, 2021

Howto

More info

Uh oh!

uri200 left a comment

Choose a reason for hiding this comment

Uh oh!

themarwhal commented May 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

uri200 May 24, 2021

Choose a reason for hiding this comment

Uh oh!

themarwhal May 24, 2021

Choose a reason for hiding this comment

Uh oh!

themarwhal May 24, 2021

Choose a reason for hiding this comment

Uh oh!

uri200 May 24, 2021

Choose a reason for hiding this comment

Uh oh!

themarwhal May 24, 2021

Choose a reason for hiding this comment

Uh oh!

uri200 May 24, 2021

Choose a reason for hiding this comment

Uh oh!

themarwhal May 24, 2021

Choose a reason for hiding this comment

Uh oh!

uri200 commented May 24, 2021

Uh oh!

themarwhal commented May 24, 2021

Uh oh!

talkhasib commented May 24, 2021

Uh oh!

themarwhal commented May 24, 2021

Uh oh!

talkhasib commented May 24, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

themarwhal commented May 20, 2021 •

edited

Loading

themarwhal commented May 21, 2021 •

edited

Loading