Skip to content

fix(pipelined): Use correct ip-addr value to get hash entry#7526

Merged
pshelar merged 1 commit intomagma:masterfrom
pshelar:fix-pipelined1-stats-del
Jun 14, 2021
Merged

fix(pipelined): Use correct ip-addr value to get hash entry#7526
pshelar merged 1 commit intomagma:masterfrom
pshelar:fix-pipelined1-stats-del

Conversation

@pshelar
Copy link
Copy Markdown
Contributor

@pshelar pshelar commented Jun 14, 2021

Signed-off-by: Pravin B Shelar [email protected]

Summary

Test Plan

make test

Additional Information

  • This change is backwards-breaking

@pshelar pshelar requested review from a team and koolzz June 14, 2021 06:14
@pull-request-size pull-request-size bot added the size/XS Denotes a PR that changes 0-9 lines. label Jun 14, 2021
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Jun 14, 2021

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

If this is your first Magma PR, also consider reading

@magmabot magmabot added the component: agw Access gateway-related issue label Jun 14, 2021
@codecov
Copy link
Copy Markdown

codecov bot commented Jun 14, 2021 •
edited
Loading

Codecov Report

Merging #7526 (3f76e5f) into master (00259b5) will increase coverage by 30.41%.
The diff coverage is n/a.

❗ Current head 3f76e5f differs from pull request most recent head f6b0bfa. Consider uploading reports for the commit f6b0bfa to get more accurate results
Impacted file tree graph

@@             Coverage Diff             @@
##           master    #7526       +/-   ##
===========================================
+ Coverage   34.02%   64.43%   +30.41%     
===========================================
  Files        1132      680      -452     
  Lines       99654    47024    -52630     
  Branches     1307     1323       +16     
===========================================
- Hits        33908    30301     -3607     
+ Misses      62404    13267    -49137     
- Partials     3342     3456      +114
Flag Coverage Δ
c_cpp ?
cloud_lint 65.72% <ø> (-0.66%) ⬇️
feg-lint 56.36% <ø> (+0.17%) ⬆️
lte-test 72.59% <ø> (-0.35%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...d/go/services/nprobe/obsidian/models/conversion.go 66.66% <0.00%> (-33.34%) ⬇️
orc8r/cloud/go/test_utils/service.go 38.46% <0.00%> (-22.52%) ⬇️
...way/python/magma/enodebd/state_machines/enb_acs.py 74.33% <0.00%> (-13.28%) ⬇️
...r/gateway/python/magma/common/redis/serializers.py 89.58% <0.00%> (-10.42%) ⬇️
...ateway/services/s8_proxy/servicers/gtp_handlers.go 69.23% <0.00%> (-9.72%) ⬇️
orc8r/cloud/go/services/directoryd/types/types.go 70.73% <0.00%> (-9.27%) ⬇️
feg/gateway/policydb/streamer.go 39.13% <0.00%> (-8.70%) ⬇️
...es/configurator/storage/sql_entity_load_helpers.go 77.66% <0.00%> (-8.63%) ⬇️
...ateway/python/magma/pipelined/openflow/messages.py 50.31% <0.00%> (-5.67%) ⬇️
feg/gateway/services/aaa/servicers/accounting.go 41.66% <0.00%> (-5.18%) ⬇️
... and 713 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0eddd10...f6b0bfa. Read the comment docs.

koolzz
koolzz reviewed Jun 14, 2021
View reviewed changes
lte/gateway/python/magma/pipelined/rule_mappers.py Outdated
encoded_imsi = encode_imsi(imsi)
if ip_addr is None or ip_addr.address is None:
ip_addr_str = ""
ip_addr_str = None
Copy link
Copy Markdown
Contributor

@koolzz koolzz Jun 14, 2021

Choose a reason for hiding this comment

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

Can you keep it "", when saving I used the empty str (helps with cwf compat)

magma/lte/gateway/python/magma/pipelined/rule_mappers.py

Line 122 in 3f76e5f

ip_addr_str = ""

Copy link
Copy Markdown
Contributor Author

@pshelar pshelar Jun 14, 2021

Choose a reason for hiding this comment

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

sure

@pshelar pshelar force-pushed the fix-pipelined1-stats-del branch from 3f76e5f to f6b0bfa Compare June 14, 2021 15:42
koolzz
koolzz reviewed Jun 14, 2021
View reviewed changes
lte/gateway/python/magma/pipelined/rule_mappers.py
_, imsi, ip_addr_str, _ = SubscriberRuleKey(*json.loads(k))
if imsi == encoded_imsi and ip_addr_str == ip_addr_str:
_, cur_imsi, cur_ip_addr_str, _ = SubscriberRuleKey(*json.loads(k))
if cur_imsi == encoded_imsi and (ip_addr_str == "" or
Copy link
Copy Markdown
Contributor

@koolzz koolzz Jun 14, 2021

Choose a reason for hiding this comment

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

I think its better to do cur_imsi == encoded_imsi and ip_addr_str== cur_ip_addr_str (we don't want this check to pass when ip_addr_str is "" but cur_ip_addr_str is some ip addr)

Copy link
Copy Markdown
Contributor Author

@pshelar pshelar Jun 14, 2021

Choose a reason for hiding this comment

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

this function needs to update version for all rules for given IMSI when IP address is not specified.
if the ip_addr_str == "" match is dropped it would not match any rule with valid IP address. Am I missing something?

Copy link
Copy Markdown
Contributor

@koolzz koolzz Jun 14, 2021

Choose a reason for hiding this comment

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

it updates all version when rule_id is not given/ip address is assumed to be available

@pshelar pshelar added the apply-v1.5 Apply this commit to the v1.5 release branch as well. label Jun 14, 2021
@pshelar pshelar merged commit 81319f0 into magma:master Jun 14, 2021
themarwhal pushed a commit that referenced this pull request Jun 14, 2021
@pshelar @themarwhal
fix(pipelined): Use currect ip-addr value to get the entry (#7526)
23dba39 
Signed-off-by: Pravin B Shelar <[email protected]>
wangyyt1013 pushed a commit to wangyyt1013/magma that referenced this pull request Jun 21, 2021
@pshelar @wangyyt1013
fix(pipelined): Use currect ip-addr value to get the entry (magma#7526)
83ce4d2 
Signed-off-by: Pravin B Shelar <[email protected]>
rmeleromira pushed a commit to rmeleromira/magma that referenced this pull request Jul 24, 2021
@pshelar @rmeleromira
fix(pipelined): Use currect ip-addr value to get the entry (magma#7526)
f964236 
Signed-off-by: Pravin B Shelar <[email protected]>
Signed-off-by: Ramon Melero <[email protected]>
m-trojanowski pushed a commit to openEPC/magma that referenced this pull request Oct 20, 2021
@pshelar @themarwhal
fix(pipelined): Use currect ip-addr value to get the entry (magma#7526)
5829010 
Signed-off-by: Pravin B Shelar <[email protected]>
m-trojanowski pushed a commit to openEPC/magma that referenced this pull request Oct 20, 2021
@pshelar @themarwhal
fix(pipelined): Use currect ip-addr value to get the entry (magma#7526)
e42995c 
Signed-off-by: Pravin B Shelar <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@koolzz koolzz koolzz approved these changes

Assignees

No one assigned

Labels

apply-v1.5 Apply this commit to the v1.5 release branch as well. backported-v1.5 component: agw Access gateway-related issue size/XS Denotes a PR that changes 0-9 lines.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@pshelar @koolzz @themarwhal @magmabot