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

Fix duplicate user entries for vpn usage #4085

Conversation

RodrigoDLopez
Copy link
Contributor

@RodrigoDLopez RodrigoDLopez commented May 15, 2020

Description

Refactor on UsageManagerImpl.createVPNUserEvent. Currently, the present method creates and removes a VPN user event. So this PR abstract this method into an handle, and creates methods to create and delete VPN user events.

Additionally, the new created method prevents duplicated entries for same user, with same userId, domainId and zoneId

Types of changes

  • Breaking change (fix or feature that would cause existing functionality to change)
  • New feature (non-breaking change which adds functionality)
  • Bug fix (non-breaking change which fixes an issue)
  • Enhancement (improves an existing feature and functionality)
  • Cleanup (Code refactoring and cleanup, that may add test cases)

Screenshots (if appropriate):

How Has This Been Tested?

Manual test
To test VPN.USER.ADD event I duplicate a entry on cloud_usage.usage_event with:
type = 'VPN.USER.ADD'
and processed = 0

Expected: Do not create a entry on cloud_usage.usage_vpn_user
Result:
2020-04-29 21:35:02,011 DEBUG [cloud.usage.UsageManagerImpl] (Usage-Job-1:null) (logid:) We do not need to create the usage VPN user [4] assigned to account [4] because it already exists.

To test VPN.USER.REMOVE event I duplicate a entry on cloud_usage.usage_event with:
type = 'VPN.USER.REMOVE'
and processed = 0

Expected: If we have more than one entry for this event, we will delete then all
Result:
2020-04-30 12:01:29,481 DEBUG [cloud.usage.UsageManagerImpl] (Usage-Job-1:null) (logid:) Deleting vpn user [23] assigned to account [4] domain [2] and zone [0] that was created at [Thu Apr 30 11:43:52 AMT 2020].

Result2:
2020-04-30 12:16:06,339 WARN [cloud.usage.UsageManagerImpl] (Usage-Job-1:null) (logid:) No usage entry for vpn user [23] assigned to account [4] domain [2] and zone [0] was found.

@GabrielBrascher GabrielBrascher added this to the 4.15.0.0 milestone May 18, 2020
@RodrigoDLopez RodrigoDLopez changed the title refactor on UsageManagerImpl.createVPNUserEvent Fix duplicate user entries for vpn usage May 25, 2020
Copy link
Member

@GabrielBrascher GabrielBrascher left a comment

Choose a reason for hiding this comment

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

LGTM based on code review.

@GabrielBrascher
Copy link
Member

@blueorangutan package

@blueorangutan
Copy link

@GabrielBrascher a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

@blueorangutan
Copy link

Packaging result: ✖centos7 ✖debian. JID-1290

@rohityadavcloud
Copy link
Member

@blueorangutan package

@blueorangutan
Copy link

@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

@blueorangutan
Copy link

Packaging result: ✖centos7 ✖debian. JID-1293

Create a handle for VPN user event. 
Abstract methods to create and delete VPN user Events
Prevents replicated entries for the same user with userId, domainId and zoneId

create unit tests to cover new code
@RodrigoDLopez RodrigoDLopez force-pushed the fix-duplicate-entries-for-vpn-usage branch from 63acfe9 to b6a97ad Compare June 5, 2020 14:55
@rohityadavcloud
Copy link
Member

@blueorangutan package

@blueorangutan
Copy link

@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

@blueorangutan
Copy link

Packaging result: ✔centos7 ✖debian. JID-1305

@rohityadavcloud
Copy link
Member

@blueorangutan package

@blueorangutan
Copy link

@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

@blueorangutan
Copy link

Packaging result: ✔centos7 ✖debian. JID-1350

@rohityadavcloud
Copy link
Member

@blueorangutan package

@blueorangutan
Copy link

@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

@blueorangutan
Copy link

Packaging result: ✔centos7 ✔debian. JID-1360

@rohityadavcloud
Copy link
Member

@blueorangutan test

@blueorangutan
Copy link

@rhtyd a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

@rohityadavcloud
Copy link
Member

@blueorangutan package

@blueorangutan
Copy link

@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

@blueorangutan
Copy link

Packaging result: ✖centos7 ✖debian. JID-1381

@RodrigoDLopez
Copy link
Contributor Author

Hello guys, this is an interesting one.
Can I get some reviews here?

@rohityadavcloud
Copy link
Member

@RodrigoDLopez I'm not sure why the packaging failed, I'll kick again
@blueorangutan package

@apache apache deleted a comment from blueorangutan Jun 17, 2020
@apache apache deleted a comment from blueorangutan Jun 17, 2020
@rohityadavcloud
Copy link
Member

@blueorangutan package

@blueorangutan
Copy link

@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

@blueorangutan
Copy link

Packaging result: ✖centos7 ✔debian. JID-1402

@rohityadavcloud
Copy link
Member

@blueorangutan package

@blueorangutan
Copy link

@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

@blueorangutan
Copy link

Packaging result: ✔centos7 ✔debian. JID-1403

@rohityadavcloud
Copy link
Member

@blueorangutan test

@blueorangutan
Copy link

@rhtyd a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

@blueorangutan
Copy link

Trillian test result (tid-1755)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 53467 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4085-t1755-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_vpc_redundant.py
Intermittent failure detected: /marvin/tests/smoke/test_vpc_vpn.py
Smoke tests completed. 83 look OK, 0 have error(s)
Only failed tests results shown below:

Test Result Time (s) Test File

@rohityadavcloud rohityadavcloud merged commit 7b5700a into apache:master Jun 18, 2020
@RodrigoDLopez RodrigoDLopez deleted the fix-duplicate-entries-for-vpn-usage branch October 30, 2020 13:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants