Skip to content

Conversation

@khos2ow
Copy link
Contributor

@khos2ow khos2ow commented Dec 5, 2017

This PR fixes the issue in which there's a leak when doing API call for listing VPC with domain account and projectId=-1.
Note for reviewers: The code formatting changed so many lines in the commit but the actual change is in line 2467-2471.

Copy link
Member

@rafaelweingartner rafaelweingartner left a comment

Choose a reason for hiding this comment

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

@khos2ow thanks for the PR.
I have only one small comment in the code.

I also have one other question for you ,are you using the ApacheCloudStack code formatter?
https://cwiki.apache.org/confluence/display/CLOUDSTACK/Coding+conventions

permittedAccounts.addAll(_projectMgr.listPermittedProjectAccounts(caller.getId()));

//permittedAccounts can be empty when the caller is not a part of any project (a domain account)
if (permittedAccounts.isEmpty()) {
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible for this permittedAccounts be null?
If not, the code is ok, otherwise it might be a good idea to use CollectionUtils.isEmpty

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are multiple places which call AccountManagerImpl#buildACLSearchParameters and all of them have List<Long> permittedAccounts = new ArrayList<Long>();. So technically there's no need to check for null or use CollectionUtils.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

and yes, I'm using the same code formatter (checked in the repo) in Eclipse and not the eclipse.epf.

Copy link
Member

Choose a reason for hiding this comment

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

thanks!
So, it seems that the code was not formatted properly before.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would say so.

@khos2ow
Copy link
Contributor Author

khos2ow commented Dec 8, 2017

@DaanHoogland @PaulAngus @rhtyd would you please review/merge this PR?

@khos2ow
Copy link
Contributor Author

khos2ow commented Dec 18, 2017

@rhtyd Can you review this?

@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: ✔centos6 ✔centos7 ✔debian. JID-1411

@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 rohityadavcloud added this to the 4.11 milestone Dec 19, 2017
@blueorangutan
Copy link

Trillian test result (tid-1823)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 32782 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr2352-t1823-kvm-centos7.zip
Smoke tests completed. 63 look OK, 4 have error(s)
Only failed tests results shown below:

Test Result Time (s) Test File
test_04_verify_guest_lspci Error 651.57 test_deploy_virtio_scsi_vm.py
test_06_verify_guest_lspci_again Error 654.22 test_deploy_virtio_scsi_vm.py
test_01_vpc_privategw_acl Failure 71.90 test_privategw_acl.py
test_02_vpc_privategw_static_routes Failure 299.42 test_privategw_acl.py
test_03_vpc_privategw_restart_vpc_cleanup Failure 122.93 test_privategw_acl.py
test_04_rvpc_privategw_static_routes Failure 288.88 test_privategw_acl.py
test_02_create_template_with_checksum_sha1 Error 5.23 test_templates.py
test_03_create_template_with_checksum_sha256 Error 5.23 test_templates.py
test_04_create_template_with_checksum_md5 Error 5.22 test_templates.py
test_01_vpc_remote_access_vpn Error 91.53 test_vpc_vpn.py

@rohityadavcloud
Copy link
Member

@khos2ow the test_deploy_virtio_scsi_vm related failures are new, rest are known issues and ignorable. can you check?

@khos2ow
Copy link
Contributor Author

khos2ow commented Dec 19, 2017

@rhtyd yes I will.

@rohityadavcloud
Copy link
Member

@khos2ow any update?
@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: ✔centos6 ✔centos7 ✔debian. JID-1496

@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-1931)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 31360 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr2352-t1931-kvm-centos7.zip
Intermitten failure detected: /marvin/tests/smoke/test_outofbandmanagement.py
Smoke tests completed. 66 look OK, 0 have error(s)
Only failed tests results shown below:

Test Result Time (s) Test File

Copy link
Member

@rohityadavcloud rohityadavcloud left a comment

Choose a reason for hiding this comment

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

LGTM, code and test results.

@rohityadavcloud rohityadavcloud merged commit 2ab5ab1 into apache:master Dec 28, 2017
@rohityadavcloud
Copy link
Member

Merged this based on two code review lgtms and test results (0 errors).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants